On Mon, Nov 20, 2023 at 09:36:05AM +0000, Jianyong Wu wrote:
>
>
> > -----Original Message-----
> > From: Russell King <linux@xxxxxxxxxxxxxxx>
> > Sent: 2023年11月20日 17:25
> > To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > Cc: linux-pm@xxxxxxxxxxxxxxx; loongarch@xxxxxxxxxxxxxxx;
> > linux-acpi@xxxxxxxxxxxxxxx; linux-arch@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-riscv@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
> > linux-csky@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> > linux-ia64@xxxxxxxxxxxxxxx; linux-parisc@xxxxxxxxxxxxxxx; Salil Mehta
> > <salil.mehta@xxxxxxxxxx>; Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>;
> > Justin He <Justin.He@xxxxxxx>; James Morse <James.Morse@xxxxxxx>;
> > Catalin Marinas <Catalin.Marinas@xxxxxxx>; Will Deacon <will@xxxxxxxxxx>;
> > Mark Rutland <Mark.Rutland@xxxxxxx>; Lorenzo Pieralisi
> > <lpieralisi@xxxxxxxxxx>
> > Subject: Re: [PATCH 34/39] arm64: psci: Ignore DENIED CPUs
> >
> > On Thu, Nov 16, 2023 at 07:45:51AM +0000, Jianyong Wu wrote:
> > > Hi Russell,
> > >
> > > One inline comment.
> > ...
> > > > Changes since RFC v2
> > > > * Add specification reference
> > > > * Use EPERM rather than EPROBE_DEFER
> > ...
> > > > @@ -40,7 +40,7 @@ static int cpu_psci_cpu_boot(unsigned int cpu) {
> > > > phys_addr_t pa_secondary_entry = __pa_symbol(secondary_entry);
> > > > int err = psci_ops.cpu_on(cpu_logical_map(cpu), pa_secondary_entry);
> > > > - if (err)
> > > > + if (err && err != -EPROBE_DEFER)
> > >
> > > Should this be EPERM? As the following psci cpu_on op will return it.
> > > I think you miss to change this when apply Jean-Philippe's patch.
> >
> > It looks like James didn't properly update all places. Also,
> >
> > > > diff --git a/drivers/firmware/psci/psci.c
> > > > b/drivers/firmware/psci/psci.c index d9629ff87861..ee82e7880d8c
> > > > 100644
> > > > --- a/drivers/firmware/psci/psci.c
> > > > +++ b/drivers/firmware/psci/psci.c
> > > > @@ -218,6 +218,8 @@ static int __psci_cpu_on(u32 fn, unsigned long
> > > > cpuid, unsigned long entry_point)
> > > > int err;
> > > >
> > > > err = invoke_psci_fn(fn, cpuid, entry_point, 0);
> > > > + if (err == PSCI_RET_DENIED)
> > > > + return -EPERM;
> > > > return psci_to_linux_errno(err);
> >
> > This change is unnecessary - probably comes from when -EPROBE_DEFER was
> > being used. psci_to_linux_errno() already does:
>
> But may print lots of noise like:
>
> [ 0.008955] smp: Bringing up secondary CPUs ...
> [ 0.009661] psci: failed to boot CPU1 (-1)
> [ 0.010360] psci: failed to boot CPU2 (-1)
> [ 0.011164] psci: failed to boot CPU3 (-1)
> [ 0.011946] psci: failed to boot CPU4 (-1)
> [ 0.012764] psci: failed to boot CPU5 (-1)
> [ 0.013534] psci: failed to boot CPU6 (-1)
> [ 0.014349] psci: failed to boot CPU7 (-1)
> [ 0.014820] smp: Brought up 1 node, 1 CPU
>
> Is this expected?
Please read my email again, and take note of the _context_ above the
places that I've commented. Context matters.
What I'm saying is that this change:
err = invoke_psci_fn(fn, cpuid, entry_point, 0);
+ if (err == PSCI_RET_DENIED)
+ return -EPERM;
return psci_to_linux_errno(err);
Is unnecessary when psci_to_linux_errno() already does:
static __always_inline int psci_to_linux_errno(int errno)
{
switch (errno) {
...
case PSCI_RET_DENIED:
return -EPERM;
So, a return of PSCI_RET_DENIED from invoke_psci_fn() above will
_already_ be translated to -EPERM (which is -1) by
psci_to_linux_errno(). There is no need to add that extra if()
statement in __psci_cpu_on().
I was _not_ saying that the entire patch was unnecessary.
Context matters. That's why we include context in replies.
Standard email etiquette (before Microsoft messed it up) is to quote the
email that is being replied to, trimming hard irrelevant content, and to
place the reply comments immediately below the original content to which
the comments relate, to give the reply comments the context necessary
for correct interpretation.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
|