On Fri, Jan 12, 2024 at 10:25 AM Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote: > > 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? 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.
