| Message ID | E1qvJBQ-00AqS8-8B@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
Hi Russell, One inline comment. > -----Original Message----- > From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King > Sent: 2023年10月24日 23:19 > To: linux-pm@vger.kernel.org; loongarch@lists.linux.dev; > linux-acpi@vger.kernel.org; linux-arch@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-riscv@lists.infradead.org; kvmarm@lists.linux.dev; x86@kernel.org; > linux-csky@vger.kernel.org; linux-doc@vger.kernel.org; > linux-ia64@vger.kernel.org; linux-parisc@vger.kernel.org > Cc: Salil Mehta <salil.mehta@huawei.com>; Jean-Philippe Brucker > <jean-philippe@linaro.org>; Jianyong Wu <Jianyong.Wu@arm.com>; Justin He > <Justin.He@arm.com>; James Morse <James.Morse@arm.com>; Catalin > Marinas <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; Mark > Rutland <Mark.Rutland@arm.com>; Lorenzo Pieralisi <lpieralisi@kernel.org> > Subject: [PATCH 34/39] arm64: psci: Ignore DENIED CPUs > > From: Jean-Philippe Brucker <jean-philippe@linaro.org> > > When a CPU is marked as disabled, but online capable in the MADT, PSCI applies > some firmware policy to control when it can be brought online. > PSCI returns DENIED to a CPU_ON request if this is not currently permitted. The > OS can learn the current policy from the _STA enabled bit. > > Handle the PSCI DENIED return code gracefully instead of printing an error. > > See https://developer.arm.com/documentation/den0022/f/?lang=en page 58. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> [ morse: > Rewrote commit message ] > Signed-off-by: James Morse <james.morse@arm.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > Changes since RFC v2 > * Add specification reference > * Use EPERM rather than EPROBE_DEFER > --- > arch/arm64/kernel/psci.c | 2 +- > arch/arm64/kernel/smp.c | 3 ++- > drivers/firmware/psci/psci.c | 2 ++ > 3 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index > 29a8e444db83..4fcc0cdd757b 100644 > --- a/arch/arm64/kernel/psci.c > +++ b/arch/arm64/kernel/psci.c > @@ -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. Thanks Jianyong > pr_err("failed to boot CPU%d (%d)\n", cpu, err); > > return err; > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index > 8c8f55721786..68ec7fbe166f 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -124,7 +124,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) > /* Now bring the CPU into our world */ > ret = boot_secondary(cpu, idle); > if (ret) { > - pr_err("CPU%u: failed to boot: %d\n", cpu, ret); > + if (ret != -EPERM) > + pr_err("CPU%u: failed to boot: %d\n", cpu, ret); > return ret; > } > > 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); > } > > -- > 2.30.2
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: case PSCI_RET_DENIED: return -EPERM; Thanks.
> -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: 2023年11月20日 17:25 > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: linux-pm@vger.kernel.org; loongarch@lists.linux.dev; > linux-acpi@vger.kernel.org; linux-arch@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-riscv@lists.infradead.org; kvmarm@lists.linux.dev; x86@kernel.org; > linux-csky@vger.kernel.org; linux-doc@vger.kernel.org; > linux-ia64@vger.kernel.org; linux-parisc@vger.kernel.org; Salil Mehta > <salil.mehta@huawei.com>; Jean-Philippe Brucker <jean-philippe@linaro.org>; > Justin He <Justin.He@arm.com>; James Morse <James.Morse@arm.com>; > Catalin Marinas <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; > Mark Rutland <Mark.Rutland@arm.com>; Lorenzo Pieralisi > <lpieralisi@kernel.org> > 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? Thanks Jianyong > > case PSCI_RET_DENIED: > return -EPERM; > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Nov 20, 2023 at 09:36:05AM +0000, Jianyong Wu wrote: > > > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: 2023年11月20日 17:25 > > To: Jianyong Wu <Jianyong.Wu@arm.com> > > Cc: linux-pm@vger.kernel.org; loongarch@lists.linux.dev; > > linux-acpi@vger.kernel.org; linux-arch@vger.kernel.org; > > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > linux-riscv@lists.infradead.org; kvmarm@lists.linux.dev; x86@kernel.org; > > linux-csky@vger.kernel.org; linux-doc@vger.kernel.org; > > linux-ia64@vger.kernel.org; linux-parisc@vger.kernel.org; Salil Mehta > > <salil.mehta@huawei.com>; Jean-Philippe Brucker <jean-philippe@linaro.org>; > > Justin He <Justin.He@arm.com>; James Morse <James.Morse@arm.com>; > > Catalin Marinas <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; > > Mark Rutland <Mark.Rutland@arm.com>; Lorenzo Pieralisi > > <lpieralisi@kernel.org> > > 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.
> -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: 2023年11月20日 17:58 > To: Jianyong Wu <Jianyong.Wu@arm.com> > Cc: linux-pm@vger.kernel.org; loongarch@lists.linux.dev; > linux-acpi@vger.kernel.org; linux-arch@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-riscv@lists.infradead.org; kvmarm@lists.linux.dev; x86@kernel.org; > linux-csky@vger.kernel.org; linux-doc@vger.kernel.org; > linux-ia64@vger.kernel.org; linux-parisc@vger.kernel.org; Salil Mehta > <salil.mehta@huawei.com>; Jean-Philippe Brucker <jean-philippe@linaro.org>; > Justin He <Justin.He@arm.com>; James Morse <James.Morse@arm.com>; > Catalin Marinas <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; > Mark Rutland <Mark.Rutland@arm.com>; Lorenzo Pieralisi > <lpieralisi@kernel.org> > Subject: Re: [PATCH 34/39] arm64: psci: Ignore DENIED CPUs > > On Mon, Nov 20, 2023 at 09:36:05AM +0000, Jianyong Wu wrote: > > > > > > > -----Original Message----- > > > From: Russell King <linux@armlinux.org.uk> > > > Sent: 2023年11月20日 17:25 > > > To: Jianyong Wu <Jianyong.Wu@arm.com> > > > Cc: linux-pm@vger.kernel.org; loongarch@lists.linux.dev; > > > linux-acpi@vger.kernel.org; linux-arch@vger.kernel.org; > > > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > linux-riscv@lists.infradead.org; kvmarm@lists.linux.dev; > > > x86@kernel.org; linux-csky@vger.kernel.org; > > > linux-doc@vger.kernel.org; linux-ia64@vger.kernel.org; > > > linux-parisc@vger.kernel.org; Salil Mehta <salil.mehta@huawei.com>; > > > Jean-Philippe Brucker <jean-philippe@linaro.org>; Justin He > > > <Justin.He@arm.com>; James Morse <James.Morse@arm.com>; Catalin > > > Marinas <Catalin.Marinas@arm.com>; Will Deacon <will@kernel.org>; > > > Mark Rutland <Mark.Rutland@arm.com>; Lorenzo Pieralisi > > > <lpieralisi@kernel.org> > > > 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. > Oh, sorry, my mistake. Ignore my last comment. Thanks Jianyong > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 29a8e444db83..4fcc0cdd757b 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -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) pr_err("failed to boot CPU%d (%d)\n", cpu, err); return err; diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 8c8f55721786..68ec7fbe166f 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -124,7 +124,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) /* Now bring the CPU into our world */ ret = boot_secondary(cpu, idle); if (ret) { - pr_err("CPU%u: failed to boot: %d\n", cpu, ret); + if (ret != -EPERM) + pr_err("CPU%u: failed to boot: %d\n", cpu, ret); return ret; } 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); }