Re: [PATCH RFC v3 01/21] ACPI: Only enumerate enabled (or functional) devices

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 2 Jan 2024 14:39:25 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 15 Dec 2023 20:47:31 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > On Friday, December 15, 2023 5:15:39 PM CET Jonathan Cameron wrote:  
> > > On Fri, 15 Dec 2023 15:31:55 +0000
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > >     
> > > > On Thu, Dec 14, 2023 at 07:37:10PM +0100, Rafael J. Wysocki wrote:    
> > > > > On Thu, Dec 14, 2023 at 7:16 PM Rafael J. Wysocki <rafael@kernel.org> wrote:      
> > > > > >
> > > > > > On Thu, Dec 14, 2023 at 7:10 PM Russell King (Oracle)
> > > > > > <linux@armlinux.org.uk> wrote:      
> > > > > > > I guess we need something like:
> > > > > > >
> > > > > > >         if (device->status.present)
> > > > > > >                 return device->device_type != ACPI_BUS_TYPE_PROCESSOR ||
> > > > > > >                        device->status.enabled;
> > > > > > >         else
> > > > > > >                 return device->status.functional;
> > > > > > >
> > > > > > > so we only check device->status.enabled for processor-type devices?      
> > > > > >
> > > > > > Yes, something like this.      
> > > > > 
> > > > > However, that is not sufficient, because there are
> > > > > ACPI_BUS_TYPE_DEVICE devices representing processors.
> > > > > 
> > > > > I'm not sure about a clean way to do it ATM.      
> > > > 
> > > > Ok, how about:
> > > > 
> > > > static bool acpi_dev_is_processor(const struct acpi_device *device)
> > > > {
> > > > 	struct acpi_hardware_id *hwid;
> > > > 
> > > > 	if (device->device_type == ACPI_BUS_TYPE_PROCESSOR)
> > > > 		return true;
> > > > 
> > > > 	if (device->device_type != ACPI_BUS_TYPE_DEVICE)
> > > > 		return false;
> > > > 
> > > > 	list_for_each_entry(hwid, &device->pnp.ids, list)
> > > > 		if (!strcmp(ACPI_PROCESSOR_OBJECT_HID, hwid->id) ||
> > > > 		    !strcmp(ACPI_PROCESSOR_DEVICE_HID, hwid->id))
> > > > 			return true;
> > > > 
> > > > 	return false;
> > > > }
> > > > 
> > > > and then:
> > > > 
> > > > 	if (device->status.present)
> > > > 		return !acpi_dev_is_processor(device) || device->status.enabled;
> > > > 	else
> > > > 		return device->status.functional;
> > > > 
> > > > ?
> > > >     
> > > Changing it to CPU only for now makes sense to me and I think this code snippet should do the
> > > job.  Nice and simple.    
> > 
> > Well, except that it does checks that are done elsewhere slightly
> > differently, which from the maintenance POV is not nice.
> > 
> > Maybe something like the appended patch (untested).  
> 
> Hi Rafael,
> 
> As far as I can see that's functionally equivalent, so looks good to me.
> I'm not set up to test this today though, so will defer to Russell on whether
> there is anything missing
> 
> Thanks for putting this together.

This is rather embarrassing...

I span this up on a QEMU instance with some prints to find out we need
the !acpi_device_is_processor() restriction.
On my 'random' test setup it fails on one device. ACPI0017 - which I
happen to know rather well. It's the weird pseudo device that lets
a CXL aware OS know there is a CEDT table to probe.

Whilst I really don't like that hack (it is all about making software
distribution of out of tree modules easier rather than something
fundamental), I'm the CXL QEMU maintainer :(

Will fix that, but it shows there is at least one broken firmware out
there.

On plus side, Rafael's code seems to work as expected and lets that
buggy firwmare carry on working :) So lets pretend the bug in qemu
is a deliberate test case!

Jonathan

p.s. My test setup blows up later for an unrelated reason with latest
kernel, so I'll be off debugging that for a while :(


> 
> Jonathan
> 
> > 
> > ---
> >  drivers/acpi/acpi_processor.c |   11 +++++++++++
> >  drivers/acpi/internal.h       |    3 +++
> >  drivers/acpi/scan.c           |   24 +++++++++++++++++++++++-
> >  3 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > Index: linux-pm/drivers/acpi/acpi_processor.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/acpi_processor.c
> > +++ linux-pm/drivers/acpi/acpi_processor.c
> > @@ -644,6 +644,17 @@ static struct acpi_scan_handler processo
> >  	},
> >  };
> >  
> > +bool acpi_device_is_processor(const struct acpi_device *adev)
> > +{
> > +	if (adev->device_type == ACPI_BUS_TYPE_PROCESSOR)
> > +		return true;
> > +
> > +	if (adev->device_type != ACPI_BUS_TYPE_DEVICE)
> > +		return false;
> > +
> > +	return acpi_scan_check_handler(adev, &processor_handler);
> > +}
> > +
> >  static int acpi_processor_container_attach(struct acpi_device *dev,
> >  					   const struct acpi_device_id *id)
> >  {
> > Index: linux-pm/drivers/acpi/internal.h
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/internal.h
> > +++ linux-pm/drivers/acpi/internal.h
> > @@ -62,6 +62,8 @@ void acpi_sysfs_add_hotplug_profile(stru
> >  int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
> >  				       const char *hotplug_profile_name);
> >  void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val);
> > +bool acpi_scan_check_handler(const struct acpi_device *adev,
> > +			     struct acpi_scan_handler *handler);
> >  
> >  #ifdef CONFIG_DEBUG_FS
> >  extern struct dentry *acpi_debugfs_dir;
> > @@ -133,6 +135,7 @@ int acpi_bus_register_early_device(int t
> >  const struct acpi_device *acpi_companion_match(const struct device *dev);
> >  int __acpi_device_uevent_modalias(const struct acpi_device *adev,
> >  				  struct kobj_uevent_env *env);
> > +bool acpi_device_is_processor(const struct acpi_device *adev);
> >  
> >  /* --------------------------------------------------------------------------
> >                                    Power Resource
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -1938,6 +1938,19 @@ static bool acpi_scan_handler_matching(s
> >  	return false;
> >  }
> >  
> > +bool acpi_scan_check_handler(const struct acpi_device *adev,
> > +			     struct acpi_scan_handler *handler)
> > +{
> > +	struct acpi_hardware_id *hwid;
> > +
> > +	list_for_each_entry(hwid, &adev->pnp.ids, list) {
> > +		if (acpi_scan_handler_matching(handler, hwid->id, NULL))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static struct acpi_scan_handler *acpi_scan_match_handler(const char *idstr,
> >  					const struct acpi_device_id **matchid)
> >  {
> > @@ -2410,7 +2423,16 @@ bool acpi_dev_ready_for_enumeration(cons
> >  	if (device->flags.honor_deps && device->dep_unmet)
> >  		return false;
> >  
> > -	return acpi_device_is_present(device);
> > +	if (device->status.functional)
> > +		return true;
> > +
> > +	if (!device->status.present)
> > +		return false;
> > +
> > +	if (device->status.enabled)
> > +		return true; /* Fast path. */
> > +
> > +	return !acpi_device_is_processor(device);
> >  }
> >  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