Re: [PATCH RFC v3 02/21] ACPI: processor: Add support for processors described as container packages

[Jonathan Cameron]

On Thu, 11 Jan 2024 18:46:47 +0000
"Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> wrote:

> On Thu, Jan 11, 2024 at 05:59:08PM +0000, Jonathan Cameron wrote:
> > On Mon, 18 Dec 2023 21:17:34 +0100
> > "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote:
> >   
> > > On Wed, Dec 13, 2023 at 1:49 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:  
> > > >
> > > > From: James Morse <james.morse@xxxxxxx>  
> > 
> > Done some digging + machine faking.  This is mid stage results at best.
> > 
> > Summary: I don't think this patch is necessary.  If anyone happens to be in
> > the mood for testing on various platforms, can you drop this patch and
> > see if everything still works.
> > 
> > With this patch in place, and a processor container containing
> > Processor() objects acpi_process_add is called twice - once via
> > the path added here and once via acpi_bus_attach etc.
> > 
> > Maybe it's a left over from earlier approaches to some of this?  
> 
> From what you're saying, it seems that way. It would be really good to
> get a reply from James to see whether he agrees - or at least get the
> reason why this patch is in the series... but I suspect that will never
> come.
> 
> > Both cases are covered by the existing handling without this.
> > 
> > I'm far from clear on why we need this patch.  Presumably
> > it's the reference in the description on it breaking for
> > Processor Package containing Processor() objects that matters
> > after a move... I'm struggling to find that move though!  
> 
> I do know that James did a lot of testing, so maybe he found some
> corner case somewhere which made this necessary - but without input
> from James, we can't know that.
> 
> So, maybe the right way forward on this is to re-test the series
> with this patch dropped, and see whether there's any ill effects.
> It should be possible to resurect the patch if it does turn out to
> be necessary.
> 
> Does that sound like a good way forward?
> 
> Thanks.
> 

Yes that sounds like the best plan. Note this patch can only make a
difference on non arm64 arches because it's a firmware bug to combine
Processor() with a GICC entry in APIC/MADT.  To even test on ARM64
you have to skip the bug check.

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/processor_core.c#L101

	/* device_declaration means Device object in DSDT, in the
	 * GIC interrupt model, logical processors are required to
	 * have a Processor Device object in the DSDT, so we should
	 * check device_declaration here
	 */
//	if (device_declaration && (gicc->uid == acpi_id)) {
	if (gicc->uid == acpi_id) {
		*mpidr = gicc->arm_mpidr;
		return 0;
	}

Only alternative is probably to go history diving and try and
find another change that would have required this and is now gone.

The ACPI scanning code has had a lot of changes whilst this work has
been underway.  More than possible that this was papering over some
issue that has long since been fixed. I can't find any deliberate
functional changes, but there is some code generalization that 'might'
have side effects in this area. Rafael, any expectation that anything
changed in how scanning processor containers works?

Jonathan