Re: [PATCH v12 14/25] irqchip/sifive-plic: Convert PLIC driver into a platform driver

From: Anup Patel <apatel@ventanamicro.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: "Marc Zyngier" <maz@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Saravana Kannan" <saravanak@google.com>,
	devicetree@vger.kernel.org, "Anup Patel" <anup@brainfault.org>,
	"Atish Patra" <atishp@atishpatra.org>,
	linux-kernel@vger.kernel.org, "Björn Töpel" <bjorn@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	linux-riscv@lists.infradead.org,
	"Frank Rowand" <frowand.list@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	"Andrew Jones" <ajones@ventanamicro.com>
Subject: Re: [PATCH v12 14/25] irqchip/sifive-plic: Convert PLIC driver into a platform driver
Date: Sat, 17 Feb 2024 11:12:29 +0530	[thread overview]
Message-ID: <CAK9=C2WWBrUae2hduUBwuxd8=23_KO3-x1fF=BaTtxJMHVyFrg@mail.gmail.com> (raw)
In-Reply-To: <878r3kcg7e.ffs@tglx>

On Sat, Feb 17, 2024 at 1:52 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Fri, Feb 16 2024 at 22:41, Anup Patel wrote:
> > On Fri, Feb 16, 2024 at 9:03 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> I don't think that removing the setup protection is correct.
> >>
> >> Assume you have maxcpus=N on the kernel command line, then the above
> >> for_each_online_cpu() loop would result in cpuhp_setup == true when the
> >> instances for the not onlined CPUs are set up, no?
> >
> > A platform can have multiple PLIC instances where each PLIC
> > instance targets a subset of HARTs (or CPUs).
> >
> > Previously (before this patch), we were probing PLIC very early so on
> > a platform with multiple PLIC instances, we need to ensure that cpuhp
> > setup is done only after PLIC context associated with boot CPU is
> > initialized hence the plic_cpuhp_setup_done check.
> >
> > This patch converts PLIC driver into a platform driver so now PLIC
> > instances are probed after all available CPUs are brought-up. In this
> > case, the cpuhp setup must be done only after PLIC context of all
> > available CPUs are initialized otherwise some of the CPUs crash
> > in plic_starting_cpu() due to lack of PLIC context initialization.
>
> You're missing the point.
>
> Assume you have 8 CPUs and 2 PLIC instances one for CPU0-3 and one for
> CPU4-7.
>
> Add "maxcpus=4" on the kernel command line, then only the first 4 CPUs
> are brought up.
>
> So at probe time cpu_online_mask has bit 0,1,2,3 set.
>
> When the first PLIC it probed the loop which checks the context for each
> online CPU will not clear cpuhp_setup and the hotplug state is installed.
>
> Now the second PLIC is probed (the one for the offline CPUs 4-7) and the
> loop will again not clear cpuhp_setup and it tries to install the state
> again, no?

Ahh, yes. Good catch.

For the "maxcpus" in kernel command-line, we can't rely on the
cpu_online_mask. I will preserve the plic_cpuhp_setup_done
check in the next revision.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv