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

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

On Fri, 12 Jan 2024 16:01:40 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Fri, Jan 12, 2024 at 10:25 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 11 Jan 2024 18:46:47 +0000
> > "Russell King (Oracle)" <linux@armlinux.org.uk> 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@kernel.org> wrote:
> > > >  
> > > > > On Wed, Dec 13, 2023 at 1:49 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:  
> > > > > >
> > > > > > From: James Morse <james.morse@arm.com>  
> > > >
> > > > 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?  
> 
> There have been changes, but I can't recall when exactly without some
> git history research.
> 
> In any case, it is always better to work on top of the current
> mainline code IMO.

Absolutely - just in this case the series has been rebased for 
a few years because the standards discussions took far far too long!

Jonathan