Re: [PATCH v7 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

[Marc Zyngier]

On Thu, 25 Apr 2024 17:55:27 +0100,
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
> On Thu, 25 Apr 2024 16:00:17 +0100
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
> > On Thu, 25 Apr 2024 13:31:50 +0100
> > Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> > 
> > > On Wed, 24 Apr 2024 16:33:22 +0100
> > > Marc Zyngier <maz@xxxxxxxxxx> wrote:

[...]

> > >   
> > > > I'd expect the handling side to look like this (will not compile, but
> > > > you'll get the idea):    
> > > Hi Marc,
> > > 
> > > In general this looks good - but...
> > > 
> > > I haven't gotten to the bottom of why yet (and it might be a side
> > > effect of how I hacked the test by lying in minimal fashion and
> > > just frigging the MADT read functions) but the hotplug flow is only getting
> > > as far as calling __cpu_up() before it seems to enter an infinite loop.
> > > That is it never gets far enough to fail this test.
> > > 
> > > Getting stuck in a psci cpu_on call.  I'm guessing something that
> > > we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
> > > Looks like it gets to
> > > SMCCC smc
> > > and is never seen again.
> > > 
> > > Any ideas on where to look?  The one advantage so far of the higher level
> > > approach is we never tried the hotplug callbacks at all so avoided hitting
> > > that call.  One (little bit horrible) solution that might avoid this would 
> > > be to add another cpuhp state very early on and fail at that stage.
> > > I'm not keen on doing that without a better explanation than I have so far!  
> > 
> > Whilst it still doesn't work I suspect I'm loosing ability to print to the console
> > between that point and somewhat later and real problem is
> > elsewhere.

Sorry, travelling at the moment, so only spotted this now.

> 
> Hi again,
> 
> Found it I think.  cpuhp calls between cpu:bringup and ap:online 
> arm made from notify_cpu_starting() are clearly marked as nofail with a comment.
> STARTING must not fail!
> 
> https://elixir.bootlin.com/linux/latest/source/kernel/cpu.c#L1642

Ah, now that rings a bell! ;-)

> 
> Whilst I have no immediate idea why that comment is there it is pretty strong
> argument against trying to have the CPUHP_AP_IRQ_GIC_STARTING callback fail
> and expecting it to carry on working :( 
> There would have been a nice print message, but given I don't appear to have
> a working console after that stage I never see it.
> 
> So the best I have yet come up with for this is the option of a new callback registered
> in gic_smp_init()
> 
> cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
> 			  "irqchip/arm/gicv3:checkrdist",
> 			  gic_broken_rdist, NULL);
> 
> with callback being simply 
> 
> static int gic_broken_rdist(unsigned int cpu)
> {
> 	if (cpumask_test_cpu(cpu, &broken_rdists))
> 		return -EINVAL;
> 
> 	return 0;
> }
> 
> That gets called cpuhp_up_callbacks() and is allows to fail and roll back the steps.
> 
> Not particularly satisfying but keeps the logic confined to the gicv3 driver.
> 
> What do you think?

Good enough for me. Cc me on the resulting patch when you repost it so
that I can eyeball it, but this is IMO the right direction.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.