diff mbox series

[RFC,v2,14/35] ACPI: Only enumerate enabled (or functional) devices

Message ID 20230913163823.7880-15-james.morse@arm.com (mailing list archive)
State RFC, archived
Headers show
Series ACPI/arm64: add support for virtual cpuhotplug | expand

Commit Message

James Morse Sept. 13, 2023, 4:38 p.m. UTC
Today the ACPI enumeration code 'visits' all devices that are present.

This is a problem for arm64, where CPUs are always present, but not
always enabled. When a device-check occurs because the firmware-policy
has changed and a CPU is now enabled, the following error occurs:
| acpi ACPI0007:48: Enumeration failure

This is ultimately because acpi_dev_ready_for_enumeration() returns
true for a device that is not enabled. The ACPI Processor driver
will not register such CPUs as they are not 'decoding their resources'.

Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
ACPI allows a device to be functional instead of maintaining the
present and enabled bit. Make this behaviour an explicit check with
a reference to the spec, and then check the present and enabled bits.
This is needed to avoid enumerating present && functional devices that
are not enabled.

Signed-off-by: James Morse <james.morse@arm.com>
---
If this change causes problems on deployed hardware, I suggest an
arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
acpi_dev_ready_for_enumeration() to only check the present bit.
---
 drivers/acpi/device_pm.c    |  2 +-
 drivers/acpi/device_sysfs.c |  2 +-
 drivers/acpi/internal.h     |  1 -
 drivers/acpi/property.c     |  2 +-
 drivers/acpi/scan.c         | 23 +++++++++++++----------
 5 files changed, 16 insertions(+), 14 deletions(-)

Comments

Jonathan Cameron Sept. 14, 2023, 12:27 p.m. UTC | #1
On Wed, 13 Sep 2023 16:38:02 +0000
James Morse <james.morse@arm.com> wrote:

> Today the ACPI enumeration code 'visits' all devices that are present.
> 
> This is a problem for arm64, where CPUs are always present, but not
> always enabled. When a device-check occurs because the firmware-policy
> has changed and a CPU is now enabled, the following error occurs:
> | acpi ACPI0007:48: Enumeration failure
> 
> This is ultimately because acpi_dev_ready_for_enumeration() returns
> true for a device that is not enabled. The ACPI Processor driver
> will not register such CPUs as they are not 'decoding their resources'.
> 
> Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
> ACPI allows a device to be functional instead of maintaining the
> present and enabled bit. Make this behaviour an explicit check with
> a reference to the spec, and then check the present and enabled bits.

"and the" only applies if the functional route hasn't been followed
"if not this case check the present and enabled bits."

> This is needed to avoid enumerating present && functional devices that
> are not enabled.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> If this change causes problems on deployed hardware, I suggest an
> arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
> acpi_dev_ready_for_enumeration() to only check the present bit.
> ---
>  drivers/acpi/device_pm.c    |  2 +-
>  drivers/acpi/device_sysfs.c |  2 +-
>  drivers/acpi/internal.h     |  1 -
>  drivers/acpi/property.c     |  2 +-
>  drivers/acpi/scan.c         | 23 +++++++++++++----------
>  5 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index f007116a8427..76c38478a502 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
>  		return -EINVAL;
>  
>  	device->power.state = ACPI_STATE_UNKNOWN;
> -	if (!acpi_device_is_present(device)) {
> +	if (!acpi_dev_ready_for_enumeration(device)) {
>  		device->flags.initialized = false;
>  		return -ENXIO;
>  	}
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index b9bbf0746199..16e586d74aa2 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
>  	struct acpi_hardware_id *id;
>  
>  	/* Avoid unnecessarily loading modules for non present devices. */
> -	if (!acpi_device_is_present(acpi_dev))
> +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
>  		return 0;
>  
>  	/*
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 866c7c4ed233..a1b45e345bcc 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
>  void acpi_device_remove_files(struct acpi_device *dev);
>  void acpi_device_add_finalize(struct acpi_device *device);
>  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> -bool acpi_device_is_present(const struct acpi_device *adev);
>  bool acpi_device_is_battery(struct acpi_device *adev);
>  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
>  					const struct device *dev);
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 413e4fcadcaf..e03f00b98701 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
>  	if (!is_acpi_device_node(fwnode))
>  		return false;
>  
> -	return acpi_device_is_present(to_acpi_device_node(fwnode));
> +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
>  }
>  
>  static const void *
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 17ab875a7d4e..f898591ce05f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
>  	int error;
>  
>  	acpi_bus_get_status(adev);
> -	if (acpi_device_is_present(adev)) {
> +	if (acpi_dev_ready_for_enumeration(adev)) {
>  		/*
>  		 * This function is only called for device objects for which
>  		 * matching scan handlers exist.  The only situation in which
> @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
>  	int error;
>  
>  	acpi_bus_get_status(adev);
> -	if (!acpi_device_is_present(adev)) {
> +	if (!acpi_dev_ready_for_enumeration(adev)) {
>  		acpi_scan_device_not_enumerated(adev);
>  		return 0;
>  	}
> @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
>  	return true;
>  }
>  
> -bool acpi_device_is_present(const struct acpi_device *adev)
> -{
> -	return adev->status.present || adev->status.functional;
> -}
> -
>  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
>  				       const char *idstr,
>  				       const struct acpi_device_id **matchid)
> @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
>   * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
>   * @device: Pointer to the &struct acpi_device to check
>   *
> - * Check if the device is present and has no unmet dependencies.
> + * Check if the device is functional or enabled and has no unmet dependencies.
>   *
> - * Return true if the device is ready for enumeratino. Otherwise, return false.
> + * Return true if the device is ready for enumeration. Otherwise, return false.
>   */
>  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>  {
>  	if (device->flags.honor_deps && device->dep_unmet)
>  		return false;
>  
> -	return acpi_device_is_present(device);
> +	/*
> +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
> +	 * (!present && functional) for certain types of devices that should be
> +	 * enumerated.

I'd call out the fact that enumeration isn't same as "device driver should be loaded"
which is the thing that functional is supposed to indicate should not happen.

> +	 */
> +	if (!device->status.present && !device->status.enabled)

In theory no need to check !enabled if !present
"If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
We could report an ACPI bug if that's seen.  If that bug case is ignored this code can
become the simpler.

	if (device->status.present)
		return device->status_enabled;
	else
		return device->status.functional;

Or the following also valid here (as functional should be set for enabled present devices
unless they failed diagnostics).

	if (dev->status.functional)
		return true;
	return device->status.present && device->status.enabled;

On assumption we want to enumerate dead devices for debug purposes...


> +		return device->status.functional;
> +
> +	return device->status.present && device->status.enabled;


>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>
Jonathan Cameron Sept. 14, 2023, 1:09 p.m. UTC | #2
On Thu, 14 Sep 2023 13:27:32 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 13 Sep 2023 16:38:02 +0000
> James Morse <james.morse@arm.com> wrote:
> 
> > Today the ACPI enumeration code 'visits' all devices that are present.
> > 
> > This is a problem for arm64, where CPUs are always present, but not
> > always enabled. When a device-check occurs because the firmware-policy
> > has changed and a CPU is now enabled, the following error occurs:
> > | acpi ACPI0007:48: Enumeration failure
> > 
> > This is ultimately because acpi_dev_ready_for_enumeration() returns
> > true for a device that is not enabled. The ACPI Processor driver
> > will not register such CPUs as they are not 'decoding their resources'.
> > 
> > Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
> > ACPI allows a device to be functional instead of maintaining the
> > present and enabled bit. Make this behaviour an explicit check with
> > a reference to the spec, and then check the present and enabled bits.  
> 
> "and the" only applies if the functional route hasn't been followed
> "if not this case check the present and enabled bits."
> 
> > This is needed to avoid enumerating present && functional devices that
> > are not enabled.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> > If this change causes problems on deployed hardware, I suggest an
> > arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
> > acpi_dev_ready_for_enumeration() to only check the present bit.
> > ---
> >  drivers/acpi/device_pm.c    |  2 +-
> >  drivers/acpi/device_sysfs.c |  2 +-
> >  drivers/acpi/internal.h     |  1 -
> >  drivers/acpi/property.c     |  2 +-
> >  drivers/acpi/scan.c         | 23 +++++++++++++----------
> >  5 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index f007116a8427..76c38478a502 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
> >  		return -EINVAL;
> >  
> >  	device->power.state = ACPI_STATE_UNKNOWN;
> > -	if (!acpi_device_is_present(device)) {
> > +	if (!acpi_dev_ready_for_enumeration(device)) {
> >  		device->flags.initialized = false;
> >  		return -ENXIO;
> >  	}
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index b9bbf0746199..16e586d74aa2 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
> >  	struct acpi_hardware_id *id;
> >  
> >  	/* Avoid unnecessarily loading modules for non present devices. */
> > -	if (!acpi_device_is_present(acpi_dev))
> > +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
> >  		return 0;
> >  
> >  	/*
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 866c7c4ed233..a1b45e345bcc 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
> >  void acpi_device_remove_files(struct acpi_device *dev);
> >  void acpi_device_add_finalize(struct acpi_device *device);
> >  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> > -bool acpi_device_is_present(const struct acpi_device *adev);
> >  bool acpi_device_is_battery(struct acpi_device *adev);
> >  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
> >  					const struct device *dev);
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 413e4fcadcaf..e03f00b98701 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
> >  	if (!is_acpi_device_node(fwnode))
> >  		return false;
> >  
> > -	return acpi_device_is_present(to_acpi_device_node(fwnode));
> > +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
> >  }
> >  
> >  static const void *
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 17ab875a7d4e..f898591ce05f 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
> >  	int error;
> >  
> >  	acpi_bus_get_status(adev);
> > -	if (acpi_device_is_present(adev)) {
> > +	if (acpi_dev_ready_for_enumeration(adev)) {
> >  		/*
> >  		 * This function is only called for device objects for which
> >  		 * matching scan handlers exist.  The only situation in which
> > @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
> >  	int error;
> >  
> >  	acpi_bus_get_status(adev);
> > -	if (!acpi_device_is_present(adev)) {
> > +	if (!acpi_dev_ready_for_enumeration(adev)) {
> >  		acpi_scan_device_not_enumerated(adev);
> >  		return 0;
> >  	}
> > @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
> >  	return true;
> >  }
> >  
> > -bool acpi_device_is_present(const struct acpi_device *adev)
> > -{
> > -	return adev->status.present || adev->status.functional;
> > -}
> > -
> >  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
> >  				       const char *idstr,
> >  				       const struct acpi_device_id **matchid)
> > @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
> >   * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
> >   * @device: Pointer to the &struct acpi_device to check
> >   *
> > - * Check if the device is present and has no unmet dependencies.
> > + * Check if the device is functional or enabled and has no unmet dependencies.
> >   *
> > - * Return true if the device is ready for enumeratino. Otherwise, return false.
> > + * Return true if the device is ready for enumeration. Otherwise, return false.
> >   */
> >  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
> >  {
> >  	if (device->flags.honor_deps && device->dep_unmet)
> >  		return false;
> >  
> > -	return acpi_device_is_present(device);
> > +	/*
> > +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
> > +	 * (!present && functional) for certain types of devices that should be
> > +	 * enumerated.  
> 
> I'd call out the fact that enumeration isn't same as "device driver should be loaded"
> which is the thing that functional is supposed to indicate should not happen.
> 
> > +	 */
> > +	if (!device->status.present && !device->status.enabled)  
> 
> In theory no need to check !enabled if !present
> "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
> We could report an ACPI bug if that's seen.  If that bug case is ignored this code can
> become the simpler.
> 
> 	if (device->status.present)
> 		return device->status_enabled;
> 	else
> 		return device->status.functional;
> 
> Or the following also valid here (as functional should be set for enabled present devices
> unless they failed diagnostics).
> 
> 	if (dev->status.functional)
> 		return true;
> 	return device->status.present && device->status.enabled;
> 
> On assumption we want to enumerate dead devices for debug purposes...
Actually ignore this.  Could have weird race with present, functional true,
but enabled not quite set - despite the device being there and self
tests having passed.


> 
> 
> > +		return device->status.functional;
> > +
> > +	return device->status.present && device->status.enabled;  
> 
> 
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
> >    
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Gavin Shan Sept. 18, 2023, 4:38 a.m. UTC | #3
On 9/14/23 22:27, Jonathan Cameron wrote:
> On Wed, 13 Sep 2023 16:38:02 +0000
> James Morse <james.morse@arm.com> wrote:
> 
>> Today the ACPI enumeration code 'visits' all devices that are present.
>>
>> This is a problem for arm64, where CPUs are always present, but not
>> always enabled. When a device-check occurs because the firmware-policy
>> has changed and a CPU is now enabled, the following error occurs:
>> | acpi ACPI0007:48: Enumeration failure
>>
>> This is ultimately because acpi_dev_ready_for_enumeration() returns
>> true for a device that is not enabled. The ACPI Processor driver
>> will not register such CPUs as they are not 'decoding their resources'.
>>
>> Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
>> ACPI allows a device to be functional instead of maintaining the
>> present and enabled bit. Make this behaviour an explicit check with
>> a reference to the spec, and then check the present and enabled bits.
> 
> "and the" only applies if the functional route hasn't been followed
> "if not this case check the present and enabled bits."
> 
>> This is needed to avoid enumerating present && functional devices that
>> are not enabled.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>> If this change causes problems on deployed hardware, I suggest an
>> arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
>> acpi_dev_ready_for_enumeration() to only check the present bit.
>> ---
>>   drivers/acpi/device_pm.c    |  2 +-
>>   drivers/acpi/device_sysfs.c |  2 +-
>>   drivers/acpi/internal.h     |  1 -
>>   drivers/acpi/property.c     |  2 +-
>>   drivers/acpi/scan.c         | 23 +++++++++++++----------
>>   5 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index f007116a8427..76c38478a502 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
>>   		return -EINVAL;
>>   
>>   	device->power.state = ACPI_STATE_UNKNOWN;
>> -	if (!acpi_device_is_present(device)) {
>> +	if (!acpi_dev_ready_for_enumeration(device)) {
>>   		device->flags.initialized = false;
>>   		return -ENXIO;
>>   	}
>> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
>> index b9bbf0746199..16e586d74aa2 100644
>> --- a/drivers/acpi/device_sysfs.c
>> +++ b/drivers/acpi/device_sysfs.c
>> @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
>>   	struct acpi_hardware_id *id;
>>   
>>   	/* Avoid unnecessarily loading modules for non present devices. */
>> -	if (!acpi_device_is_present(acpi_dev))
>> +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
>>   		return 0;
>>   
>>   	/*
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 866c7c4ed233..a1b45e345bcc 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
>>   void acpi_device_remove_files(struct acpi_device *dev);
>>   void acpi_device_add_finalize(struct acpi_device *device);
>>   void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
>> -bool acpi_device_is_present(const struct acpi_device *adev);
>>   bool acpi_device_is_battery(struct acpi_device *adev);
>>   bool acpi_device_is_first_physical_node(struct acpi_device *adev,
>>   					const struct device *dev);
>> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
>> index 413e4fcadcaf..e03f00b98701 100644
>> --- a/drivers/acpi/property.c
>> +++ b/drivers/acpi/property.c
>> @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
>>   	if (!is_acpi_device_node(fwnode))
>>   		return false;
>>   
>> -	return acpi_device_is_present(to_acpi_device_node(fwnode));
>> +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
>>   }
>>   
>>   static const void *
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 17ab875a7d4e..f898591ce05f 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
>>   	int error;
>>   
>>   	acpi_bus_get_status(adev);
>> -	if (acpi_device_is_present(adev)) {
>> +	if (acpi_dev_ready_for_enumeration(adev)) {
>>   		/*
>>   		 * This function is only called for device objects for which
>>   		 * matching scan handlers exist.  The only situation in which
>> @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
>>   	int error;
>>   
>>   	acpi_bus_get_status(adev);
>> -	if (!acpi_device_is_present(adev)) {
>> +	if (!acpi_dev_ready_for_enumeration(adev)) {
>>   		acpi_scan_device_not_enumerated(adev);
>>   		return 0;
>>   	}
>> @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
>>   	return true;
>>   }
>>   
>> -bool acpi_device_is_present(const struct acpi_device *adev)
>> -{
>> -	return adev->status.present || adev->status.functional;
>> -}
>> -
>>   static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
>>   				       const char *idstr,
>>   				       const struct acpi_device_id **matchid)
>> @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
>>    * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
>>    * @device: Pointer to the &struct acpi_device to check
>>    *
>> - * Check if the device is present and has no unmet dependencies.
>> + * Check if the device is functional or enabled and has no unmet dependencies.
>>    *
>> - * Return true if the device is ready for enumeratino. Otherwise, return false.
>> + * Return true if the device is ready for enumeration. Otherwise, return false.
>>    */
>>   bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>>   {
>>   	if (device->flags.honor_deps && device->dep_unmet)
>>   		return false;
>>   
>> -	return acpi_device_is_present(device);
>> +	/*
>> +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
>> +	 * (!present && functional) for certain types of devices that should be
>> +	 * enumerated.
> 
> I'd call out the fact that enumeration isn't same as "device driver should be loaded"
> which is the thing that functional is supposed to indicate should not happen.
> 
>> +	 */
>> +	if (!device->status.present && !device->status.enabled)
> 
> In theory no need to check !enabled if !present
> "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
> We could report an ACPI bug if that's seen.  If that bug case is ignored this code can
> become the simpler.
> 
> 	if (device->status.present)
> 		return device->status_enabled;
> 	else
> 		return device->status.functional;
> 
> Or the following also valid here (as functional should be set for enabled present devices
> unless they failed diagnostics).
> 
> 	if (dev->status.functional)
> 		return true;
> 	return device->status.present && device->status.enabled;
> 
> On assumption we want to enumerate dead devices for debug purposes...
> 

I think it's worthy to include the words about the synchronization between present/enabled
bits into comments, outlined by Jonathan, to help readers to understand the code. Something
like below for the comments:

	/*
          * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
	 * (!present && functional) for certain types of devices that should be
          * enumerated. Note that the enabled bit can't be set until the present
          * bit is set.
          */

> 
>> +		return device->status.functional;
>> +
>> +	return device->status.present && device->status.enabled;
> 
> 
>>   }
>>   EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>>   


Thanks,
Gavin
Gavin Shan Sept. 18, 2023, 11:43 p.m. UTC | #4
On 9/14/23 02:38, James Morse wrote:
> Today the ACPI enumeration code 'visits' all devices that are present.
> 
> This is a problem for arm64, where CPUs are always present, but not
> always enabled. When a device-check occurs because the firmware-policy
> has changed and a CPU is now enabled, the following error occurs:
> | acpi ACPI0007:48: Enumeration failure
> 
> This is ultimately because acpi_dev_ready_for_enumeration() returns
> true for a device that is not enabled. The ACPI Processor driver
> will not register such CPUs as they are not 'decoding their resources'.
> 
> Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
> ACPI allows a device to be functional instead of maintaining the
> present and enabled bit. Make this behaviour an explicit check with
> a reference to the spec, and then check the present and enabled bits.
> This is needed to avoid enumerating present && functional devices that
> are not enabled.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> If this change causes problems on deployed hardware, I suggest an
> arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
> acpi_dev_ready_for_enumeration() to only check the present bit.
> ---
>   drivers/acpi/device_pm.c    |  2 +-
>   drivers/acpi/device_sysfs.c |  2 +-
>   drivers/acpi/internal.h     |  1 -
>   drivers/acpi/property.c     |  2 +-
>   drivers/acpi/scan.c         | 23 +++++++++++++----------
>   5 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index f007116a8427..76c38478a502 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
>   		return -EINVAL;
>   
>   	device->power.state = ACPI_STATE_UNKNOWN;
> -	if (!acpi_device_is_present(device)) {
> +	if (!acpi_dev_ready_for_enumeration(device)) {
>   		device->flags.initialized = false;
>   		return -ENXIO;
>   	}
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index b9bbf0746199..16e586d74aa2 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
>   	struct acpi_hardware_id *id;
>   
>   	/* Avoid unnecessarily loading modules for non present devices. */
> -	if (!acpi_device_is_present(acpi_dev))
> +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
>   		return 0;
>   
>   	/*
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 866c7c4ed233..a1b45e345bcc 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
>   void acpi_device_remove_files(struct acpi_device *dev);
>   void acpi_device_add_finalize(struct acpi_device *device);
>   void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> -bool acpi_device_is_present(const struct acpi_device *adev);
>   bool acpi_device_is_battery(struct acpi_device *adev);
>   bool acpi_device_is_first_physical_node(struct acpi_device *adev,
>   					const struct device *dev);
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 413e4fcadcaf..e03f00b98701 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
>   	if (!is_acpi_device_node(fwnode))
>   		return false;
>   
> -	return acpi_device_is_present(to_acpi_device_node(fwnode));
> +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
>   }
>   
>   static const void *
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 17ab875a7d4e..f898591ce05f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
>   	int error;
>   
>   	acpi_bus_get_status(adev);
> -	if (acpi_device_is_present(adev)) {
> +	if (acpi_dev_ready_for_enumeration(adev)) {
>   		/*
>   		 * This function is only called for device objects for which
>   		 * matching scan handlers exist.  The only situation in which
> @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
>   	int error;
>   
>   	acpi_bus_get_status(adev);
> -	if (!acpi_device_is_present(adev)) {
> +	if (!acpi_dev_ready_for_enumeration(adev)) {
>   		acpi_scan_device_not_enumerated(adev);
>   		return 0;
>   	}
> @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
>   	return true;
>   }
>   
> -bool acpi_device_is_present(const struct acpi_device *adev)
> -{
> -	return adev->status.present || adev->status.functional;
> -}
> -
>   static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
>   				       const char *idstr,
>   				       const struct acpi_device_id **matchid)
> @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
>    * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
>    * @device: Pointer to the &struct acpi_device to check
>    *
> - * Check if the device is present and has no unmet dependencies.
> + * Check if the device is functional or enabled and has no unmet dependencies.
>    *
> - * Return true if the device is ready for enumeratino. Otherwise, return false.
> + * Return true if the device is ready for enumeration. Otherwise, return false.
>    */
>   bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>   {
>   	if (device->flags.honor_deps && device->dep_unmet)
>   		return false;
>   
> -	return acpi_device_is_present(device);
> +	/*
> +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
> +	 * (!present && functional) for certain types of devices that should be
> +	 * enumerated.
> +	 */
> +	if (!device->status.present && !device->status.enabled)
> +		return device->status.functional;
> +
> +	return device->status.present && device->status.enabled;
>   }
>   EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>   

Looking at Salil's latest branch (vcpu-hotplug-RFCv2-rc7), there are 3 possible statuses:

   0x0       when CPU isn't present
   0xD       when CPU is present, but not enabled
   0xF       when CPU is present and enabled

Previously, the ACPI device is enumerated on 0xD and 0xF. We want to avoid the enumeration
on 0xD since the processor isn't ready for enumeration in this specific case. The changed
check (device->status.present && device->status.enabled) can ensure it. So the addition
of checking @device->state.functional seems irrelevant to ARM64 vCPU hot-add? I guess we
probably want a relaxation after the condition (device->status.present || device->status.enabled)
becomes a more strict one (device->status.present && device->status.enabled)

Thanks,
Gavin
Russell King (Oracle) Oct. 20, 2023, 3:32 p.m. UTC | #5
On Thu, Sep 14, 2023 at 02:09:40PM +0100, Jonathan Cameron wrote:
> On Thu, 14 Sep 2023 13:27:32 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Wed, 13 Sep 2023 16:38:02 +0000
> > James Morse <james.morse@arm.com> wrote:
> > 
> > > Today the ACPI enumeration code 'visits' all devices that are present.
> > > 
> > > This is a problem for arm64, where CPUs are always present, but not
> > > always enabled. When a device-check occurs because the firmware-policy
> > > has changed and a CPU is now enabled, the following error occurs:
> > > | acpi ACPI0007:48: Enumeration failure
> > > 
> > > This is ultimately because acpi_dev_ready_for_enumeration() returns
> > > true for a device that is not enabled. The ACPI Processor driver
> > > will not register such CPUs as they are not 'decoding their resources'.
> > > 
> > > Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
> > > ACPI allows a device to be functional instead of maintaining the
> > > present and enabled bit. Make this behaviour an explicit check with
> > > a reference to the spec, and then check the present and enabled bits.  
> > 
> > "and the" only applies if the functional route hasn't been followed
> > "if not this case check the present and enabled bits."
> > 
> > > This is needed to avoid enumerating present && functional devices that
> > > are not enabled.
> > > 
> > > Signed-off-by: James Morse <james.morse@arm.com>
> > > ---
> > > If this change causes problems on deployed hardware, I suggest an
> > > arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
> > > acpi_dev_ready_for_enumeration() to only check the present bit.
> > > ---
> > >  drivers/acpi/device_pm.c    |  2 +-
> > >  drivers/acpi/device_sysfs.c |  2 +-
> > >  drivers/acpi/internal.h     |  1 -
> > >  drivers/acpi/property.c     |  2 +-
> > >  drivers/acpi/scan.c         | 23 +++++++++++++----------
> > >  5 files changed, 16 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > index f007116a8427..76c38478a502 100644
> > > --- a/drivers/acpi/device_pm.c
> > > +++ b/drivers/acpi/device_pm.c
> > > @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
> > >  		return -EINVAL;
> > >  
> > >  	device->power.state = ACPI_STATE_UNKNOWN;
> > > -	if (!acpi_device_is_present(device)) {
> > > +	if (!acpi_dev_ready_for_enumeration(device)) {
> > >  		device->flags.initialized = false;
> > >  		return -ENXIO;
> > >  	}
> > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > > index b9bbf0746199..16e586d74aa2 100644
> > > --- a/drivers/acpi/device_sysfs.c
> > > +++ b/drivers/acpi/device_sysfs.c
> > > @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
> > >  	struct acpi_hardware_id *id;
> > >  
> > >  	/* Avoid unnecessarily loading modules for non present devices. */
> > > -	if (!acpi_device_is_present(acpi_dev))
> > > +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
> > >  		return 0;
> > >  
> > >  	/*
> > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > > index 866c7c4ed233..a1b45e345bcc 100644
> > > --- a/drivers/acpi/internal.h
> > > +++ b/drivers/acpi/internal.h
> > > @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
> > >  void acpi_device_remove_files(struct acpi_device *dev);
> > >  void acpi_device_add_finalize(struct acpi_device *device);
> > >  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> > > -bool acpi_device_is_present(const struct acpi_device *adev);
> > >  bool acpi_device_is_battery(struct acpi_device *adev);
> > >  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
> > >  					const struct device *dev);
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index 413e4fcadcaf..e03f00b98701 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
> > >  	if (!is_acpi_device_node(fwnode))
> > >  		return false;
> > >  
> > > -	return acpi_device_is_present(to_acpi_device_node(fwnode));
> > > +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
> > >  }
> > >  
> > >  static const void *
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 17ab875a7d4e..f898591ce05f 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
> > >  	int error;
> > >  
> > >  	acpi_bus_get_status(adev);
> > > -	if (acpi_device_is_present(adev)) {
> > > +	if (acpi_dev_ready_for_enumeration(adev)) {
> > >  		/*
> > >  		 * This function is only called for device objects for which
> > >  		 * matching scan handlers exist.  The only situation in which
> > > @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
> > >  	int error;
> > >  
> > >  	acpi_bus_get_status(adev);
> > > -	if (!acpi_device_is_present(adev)) {
> > > +	if (!acpi_dev_ready_for_enumeration(adev)) {
> > >  		acpi_scan_device_not_enumerated(adev);
> > >  		return 0;
> > >  	}
> > > @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
> > >  	return true;
> > >  }
> > >  
> > > -bool acpi_device_is_present(const struct acpi_device *adev)
> > > -{
> > > -	return adev->status.present || adev->status.functional;
> > > -}
> > > -
> > >  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
> > >  				       const char *idstr,
> > >  				       const struct acpi_device_id **matchid)
> > > @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
> > >   * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
> > >   * @device: Pointer to the &struct acpi_device to check
> > >   *
> > > - * Check if the device is present and has no unmet dependencies.
> > > + * Check if the device is functional or enabled and has no unmet dependencies.
> > >   *
> > > - * Return true if the device is ready for enumeratino. Otherwise, return false.
> > > + * Return true if the device is ready for enumeration. Otherwise, return false.
> > >   */
> > >  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
> > >  {
> > >  	if (device->flags.honor_deps && device->dep_unmet)
> > >  		return false;
> > >  
> > > -	return acpi_device_is_present(device);
> > > +	/*
> > > +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
> > > +	 * (!present && functional) for certain types of devices that should be
> > > +	 * enumerated.  
> > 
> > I'd call out the fact that enumeration isn't same as "device driver should be loaded"
> > which is the thing that functional is supposed to indicate should not happen.
> > 
> > > +	 */
> > > +	if (!device->status.present && !device->status.enabled)  
> > 
> > In theory no need to check !enabled if !present
> > "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
> > We could report an ACPI bug if that's seen.  If that bug case is ignored this code can
> > become the simpler.
> > 
> > 	if (device->status.present)
> > 		return device->status_enabled;
> > 	else
> > 		return device->status.functional;
> > 
> > Or the following also valid here (as functional should be set for enabled present devices
> > unless they failed diagnostics).
> > 
> > 	if (dev->status.functional)
> > 		return true;
> > 	return device->status.present && device->status.enabled;
> > 
> > On assumption we want to enumerate dead devices for debug purposes...
> Actually ignore this.  Could have weird race with present, functional true,
> but enabled not quite set - despite the device being there and self
> tests having passed.

Are you suggesting to ignore you're entire suggestion or just this
suggestion and go with the first one?

So, the code was originally effectively:

	return adev->status.present || adev->status.functional;

So it has the truth table:

present	functional	result
false	false		false
false	true		true
true	don't care	true

James' replacement code makes this:

	if (!device->status.present && !device->status.enabled)
		return device->status.functional;

	return device->status.present && device->status.enabled;

giving:

present	enabled	functional	result
false	false	false		false
false	false	true		true
false	true	don't care	false	<== invalid according to spec
true	false	don't care	false
true	true	don't care	true

So, I think what you're getting at is that we want the logic to be
according to the above table, but simplified, not caring about the
invalid state too much?

In which case, I would suggest going with your first suggestion, in
other words:

	if (device->status.present)
		return device->status.enabled;
	else
		return device->status.functional;

Yes?
Russell King (Oracle) Oct. 20, 2023, 3:45 p.m. UTC | #6
On Tue, Sep 19, 2023 at 09:43:46AM +1000, Gavin Shan wrote:
> On 9/14/23 02:38, James Morse wrote:
> > +	if (!device->status.present && !device->status.enabled)
> > +		return device->status.functional;
> > +
> > +	return device->status.present && device->status.enabled;
> >   }
> >   EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
> 
> Looking at Salil's latest branch (vcpu-hotplug-RFCv2-rc7), there are 3 possible statuses:
> 
>   0x0       when CPU isn't present
>   0xD       when CPU is present, but not enabled
>   0xF       when CPU is present and enabled
> 
> Previously, the ACPI device is enumerated on 0xD and 0xF. We want to avoid the enumeration
> on 0xD since the processor isn't ready for enumeration in this specific case. The changed
> check (device->status.present && device->status.enabled) can ensure it. So the addition
> of checking @device->state.functional seems irrelevant to ARM64 vCPU hot-add? I guess we
> probably want a relaxation after the condition (device->status.present || device->status.enabled)
> becomes a more strict one (device->status.present && device->status.enabled)

Okay, I'm confused by your comment.

As mentioned in my reply to Jonathan, the current code tests for
device->status.present || device->status.functional, not
device->status.present || device->status.enabled.

Digging back in the history, the acpi_device_is_present() helper
was added in 202317a573b2 "ACPI / scan: Add acpi_device objects for all
device nodes in the namespace". The commit description states:

    Modify the ACPI namespace scanning code to register a struct
    acpi_device object for every namespace node representing a device,
    processor and so on, even if the device represented by that namespace
    node is reported to be not present and not functional by _STA.

It seems the code originally used this test

-       if (!(sta & ACPI_STA_DEVICE_PRESENT) &&
-           !(sta & ACPI_STA_DEVICE_FUNCTIONING)) {

So this commit is just continuing that "tradition".

Digging further back, we find:

778cbc1d3abd ACPI: factor out device type and status checking

-       case ACPI_BUS_TYPE_PROCESSOR:
-       case ACPI_BUS_TYPE_DEVICE:
...
-               /*
-                * When the device is neither present nor functional, the
-                * device should not be added to Linux ACPI device tree.
-                * When the status of the device is not present but functinal,
-                * it should be added to Linux ACPI tree. For example : bay
-                * device , dock device.
-                * In such conditions it is unncessary to check whether it is
-                * bay device or dock device.
-                */
-               if (!device->status.present && !device->status.functional) {

and that comment seems to indicate where the !present && functional
case comes from.

So, I think it's necessary to continue supporting the !present &&
functional case otherwise it seems to me that we'll be regressing some
platforms.
Jonathan Cameron Oct. 20, 2023, 4:43 p.m. UTC | #7
On Fri, 20 Oct 2023 16:32:17 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 14, 2023 at 02:09:40PM +0100, Jonathan Cameron wrote:
> > On Thu, 14 Sep 2023 13:27:32 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Wed, 13 Sep 2023 16:38:02 +0000
> > > James Morse <james.morse@arm.com> wrote:
> > >   
> > > > Today the ACPI enumeration code 'visits' all devices that are present.
> > > > 
> > > > This is a problem for arm64, where CPUs are always present, but not
> > > > always enabled. When a device-check occurs because the firmware-policy
> > > > has changed and a CPU is now enabled, the following error occurs:
> > > > | acpi ACPI0007:48: Enumeration failure
> > > > 
> > > > This is ultimately because acpi_dev_ready_for_enumeration() returns
> > > > true for a device that is not enabled. The ACPI Processor driver
> > > > will not register such CPUs as they are not 'decoding their resources'.
> > > > 
> > > > Change acpi_dev_ready_for_enumeration() to also check the enabled bit.
> > > > ACPI allows a device to be functional instead of maintaining the
> > > > present and enabled bit. Make this behaviour an explicit check with
> > > > a reference to the spec, and then check the present and enabled bits.    
> > > 
> > > "and the" only applies if the functional route hasn't been followed
> > > "if not this case check the present and enabled bits."
> > >   
> > > > This is needed to avoid enumerating present && functional devices that
> > > > are not enabled.
> > > > 
> > > > Signed-off-by: James Morse <james.morse@arm.com>
> > > > ---
> > > > If this change causes problems on deployed hardware, I suggest an
> > > > arch opt-in: ACPI_IGNORE_STA_ENABLED, that causes
> > > > acpi_dev_ready_for_enumeration() to only check the present bit.
> > > > ---
> > > >  drivers/acpi/device_pm.c    |  2 +-
> > > >  drivers/acpi/device_sysfs.c |  2 +-
> > > >  drivers/acpi/internal.h     |  1 -
> > > >  drivers/acpi/property.c     |  2 +-
> > > >  drivers/acpi/scan.c         | 23 +++++++++++++----------
> > > >  5 files changed, 16 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > > index f007116a8427..76c38478a502 100644
> > > > --- a/drivers/acpi/device_pm.c
> > > > +++ b/drivers/acpi/device_pm.c
> > > > @@ -313,7 +313,7 @@ int acpi_bus_init_power(struct acpi_device *device)
> > > >  		return -EINVAL;
> > > >  
> > > >  	device->power.state = ACPI_STATE_UNKNOWN;
> > > > -	if (!acpi_device_is_present(device)) {
> > > > +	if (!acpi_dev_ready_for_enumeration(device)) {
> > > >  		device->flags.initialized = false;
> > > >  		return -ENXIO;
> > > >  	}
> > > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > > > index b9bbf0746199..16e586d74aa2 100644
> > > > --- a/drivers/acpi/device_sysfs.c
> > > > +++ b/drivers/acpi/device_sysfs.c
> > > > @@ -141,7 +141,7 @@ static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
> > > >  	struct acpi_hardware_id *id;
> > > >  
> > > >  	/* Avoid unnecessarily loading modules for non present devices. */
> > > > -	if (!acpi_device_is_present(acpi_dev))
> > > > +	if (!acpi_dev_ready_for_enumeration(acpi_dev))
> > > >  		return 0;
> > > >  
> > > >  	/*
> > > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > > > index 866c7c4ed233..a1b45e345bcc 100644
> > > > --- a/drivers/acpi/internal.h
> > > > +++ b/drivers/acpi/internal.h
> > > > @@ -107,7 +107,6 @@ int acpi_device_setup_files(struct acpi_device *dev);
> > > >  void acpi_device_remove_files(struct acpi_device *dev);
> > > >  void acpi_device_add_finalize(struct acpi_device *device);
> > > >  void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> > > > -bool acpi_device_is_present(const struct acpi_device *adev);
> > > >  bool acpi_device_is_battery(struct acpi_device *adev);
> > > >  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
> > > >  					const struct device *dev);
> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > index 413e4fcadcaf..e03f00b98701 100644
> > > > --- a/drivers/acpi/property.c
> > > > +++ b/drivers/acpi/property.c
> > > > @@ -1418,7 +1418,7 @@ static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
> > > >  	if (!is_acpi_device_node(fwnode))
> > > >  		return false;
> > > >  
> > > > -	return acpi_device_is_present(to_acpi_device_node(fwnode));
> > > > +	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
> > > >  }
> > > >  
> > > >  static const void *
> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > > index 17ab875a7d4e..f898591ce05f 100644
> > > > --- a/drivers/acpi/scan.c
> > > > +++ b/drivers/acpi/scan.c
> > > > @@ -304,7 +304,7 @@ static int acpi_scan_device_check(struct acpi_device *adev)
> > > >  	int error;
> > > >  
> > > >  	acpi_bus_get_status(adev);
> > > > -	if (acpi_device_is_present(adev)) {
> > > > +	if (acpi_dev_ready_for_enumeration(adev)) {
> > > >  		/*
> > > >  		 * This function is only called for device objects for which
> > > >  		 * matching scan handlers exist.  The only situation in which
> > > > @@ -338,7 +338,7 @@ static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
> > > >  	int error;
> > > >  
> > > >  	acpi_bus_get_status(adev);
> > > > -	if (!acpi_device_is_present(adev)) {
> > > > +	if (!acpi_dev_ready_for_enumeration(adev)) {
> > > >  		acpi_scan_device_not_enumerated(adev);
> > > >  		return 0;
> > > >  	}
> > > > @@ -1908,11 +1908,6 @@ static bool acpi_device_should_be_hidden(acpi_handle handle)
> > > >  	return true;
> > > >  }
> > > >  
> > > > -bool acpi_device_is_present(const struct acpi_device *adev)
> > > > -{
> > > > -	return adev->status.present || adev->status.functional;
> > > > -}
> > > > -
> > > >  static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
> > > >  				       const char *idstr,
> > > >  				       const struct acpi_device_id **matchid)
> > > > @@ -2375,16 +2370,24 @@ EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
> > > >   * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
> > > >   * @device: Pointer to the &struct acpi_device to check
> > > >   *
> > > > - * Check if the device is present and has no unmet dependencies.
> > > > + * Check if the device is functional or enabled and has no unmet dependencies.
> > > >   *
> > > > - * Return true if the device is ready for enumeratino. Otherwise, return false.
> > > > + * Return true if the device is ready for enumeration. Otherwise, return false.
> > > >   */
> > > >  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
> > > >  {
> > > >  	if (device->flags.honor_deps && device->dep_unmet)
> > > >  		return false;
> > > >  
> > > > -	return acpi_device_is_present(device);
> > > > +	/*
> > > > +	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
> > > > +	 * (!present && functional) for certain types of devices that should be
> > > > +	 * enumerated.    
> > > 
> > > I'd call out the fact that enumeration isn't same as "device driver should be loaded"
> > > which is the thing that functional is supposed to indicate should not happen.
> > >   
> > > > +	 */
> > > > +	if (!device->status.present && !device->status.enabled)    
> > > 
> > > In theory no need to check !enabled if !present
> > > "If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled)."
> > > We could report an ACPI bug if that's seen.  If that bug case is ignored this code can
> > > become the simpler.
> > > 
> > > 	if (device->status.present)
> > > 		return device->status_enabled;
> > > 	else
> > > 		return device->status.functional;
> > > 
> > > Or the following also valid here (as functional should be set for enabled present devices
> > > unless they failed diagnostics).
> > > 
> > > 	if (dev->status.functional)
> > > 		return true;
> > > 	return device->status.present && device->status.enabled;
> > > 
> > > On assumption we want to enumerate dead devices for debug purposes...  
> > Actually ignore this.  Could have weird race with present, functional true,
> > but enabled not quite set - despite the device being there and self
> > tests having passed.  
> 
> Are you suggesting to ignore you're entire suggestion or just this
> suggestion and go with the first one?

I meant just the last one.  Sorry for confusion.

> 
> So, the code was originally effectively:
> 
> 	return adev->status.present || adev->status.functional;
> 
> So it has the truth table:
> 
> present	functional	result
> false	false		false
> false	true		true
> true	don't care	true
> 
> James' replacement code makes this:
> 
> 	if (!device->status.present && !device->status.enabled)
> 		return device->status.functional;
> 
> 	return device->status.present && device->status.enabled;
> 
> giving:
> 
> present	enabled	functional	result
> false	false	false		false
> false	false	true		true
> false	true	don't care	false	<== invalid according to spec
> true	false	don't care	false
> true	true	don't care	true
> 
> So, I think what you're getting at is that we want the logic to be
> according to the above table, but simplified, not caring about the
> invalid state too much?
> 
> In which case, I would suggest going with your first suggestion, in
> other words:
> 
> 	if (device->status.present)
> 		return device->status.enabled;
> 	else
> 		return device->status.functional;
> 
> Yes?
> 
Yes I agree.
diff mbox series

Patch

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index f007116a8427..76c38478a502 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -313,7 +313,7 @@  int acpi_bus_init_power(struct acpi_device *device)
 		return -EINVAL;
 
 	device->power.state = ACPI_STATE_UNKNOWN;
-	if (!acpi_device_is_present(device)) {
+	if (!acpi_dev_ready_for_enumeration(device)) {
 		device->flags.initialized = false;
 		return -ENXIO;
 	}
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index b9bbf0746199..16e586d74aa2 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -141,7 +141,7 @@  static int create_pnp_modalias(const struct acpi_device *acpi_dev, char *modalia
 	struct acpi_hardware_id *id;
 
 	/* Avoid unnecessarily loading modules for non present devices. */
-	if (!acpi_device_is_present(acpi_dev))
+	if (!acpi_dev_ready_for_enumeration(acpi_dev))
 		return 0;
 
 	/*
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 866c7c4ed233..a1b45e345bcc 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -107,7 +107,6 @@  int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
-bool acpi_device_is_present(const struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
 					const struct device *dev);
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 413e4fcadcaf..e03f00b98701 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1418,7 +1418,7 @@  static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 	if (!is_acpi_device_node(fwnode))
 		return false;
 
-	return acpi_device_is_present(to_acpi_device_node(fwnode));
+	return acpi_dev_ready_for_enumeration(to_acpi_device_node(fwnode));
 }
 
 static const void *
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 17ab875a7d4e..f898591ce05f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -304,7 +304,7 @@  static int acpi_scan_device_check(struct acpi_device *adev)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (acpi_device_is_present(adev)) {
+	if (acpi_dev_ready_for_enumeration(adev)) {
 		/*
 		 * This function is only called for device objects for which
 		 * matching scan handlers exist.  The only situation in which
@@ -338,7 +338,7 @@  static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
 	int error;
 
 	acpi_bus_get_status(adev);
-	if (!acpi_device_is_present(adev)) {
+	if (!acpi_dev_ready_for_enumeration(adev)) {
 		acpi_scan_device_not_enumerated(adev);
 		return 0;
 	}
@@ -1908,11 +1908,6 @@  static bool acpi_device_should_be_hidden(acpi_handle handle)
 	return true;
 }
 
-bool acpi_device_is_present(const struct acpi_device *adev)
-{
-	return adev->status.present || adev->status.functional;
-}
-
 static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
 				       const char *idstr,
 				       const struct acpi_device_id **matchid)
@@ -2375,16 +2370,24 @@  EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
  * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
  * @device: Pointer to the &struct acpi_device to check
  *
- * Check if the device is present and has no unmet dependencies.
+ * Check if the device is functional or enabled and has no unmet dependencies.
  *
- * Return true if the device is ready for enumeratino. Otherwise, return false.
+ * Return true if the device is ready for enumeration. Otherwise, return false.
  */
 bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 {
 	if (device->flags.honor_deps && device->dep_unmet)
 		return false;
 
-	return acpi_device_is_present(device);
+	/*
+	 * ACPI 6.5's 6.3.7 "_STA (Device Status)" allows firmware to return
+	 * (!present && functional) for certain types of devices that should be
+	 * enumerated.
+	 */
+	if (!device->status.present && !device->status.enabled)
+		return device->status.functional;
+
+	return device->status.present && device->status.enabled;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);