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.
