Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon

[Christophe Leroy]

Le 20/08/2022 à 21:45, Arminder Singh a écrit :
> This is the first time I'm interacting with the Linux mailing lists, so
> please don't eviscerate me *too much* if I get the formatting wrong.
> Of course I'm always willing to take criticism and improve my formatting
> in the future.

The above text should go after you signed-off-by, after the --- , so 
that it get's ignored by 'git am' when applying.

> 
> This patch adds support for IRQs to the PASemi I2C controller driver.
> This will allow for faster performing I2C transactions on Apple Silicon
> hardware, as previously, the driver was forced to poll the SMSTA register
> for a set amount of time.
> 
> With this patchset the driver on Apple silicon hardware will instead wait
> for an interrupt which will signal the completion of the I2C transaction.
> The timeout value for this completion will be the same as the current
> amount of time the I2C driver polls for.
> 
> This will result in some performance improvement since the driver will be
> waiting for less time than it does right now on Apple Silicon hardware.
> 
> The patch right now will only enable IRQs for Apple Silicon I2C chips,
> and only if it's able to successfully request the IRQ from the kernel.
> 
> === Testing ===
> 
> This patch has been tested on both the mainline Linux kernel tree and
> the Asahi branch (https://github.com/AsahiLinux/linux.git) on both an
> M1 and M2 MacBook Air, and it compiles successfully as both a module and
> built-in to the kernel itself. The patch in both trees successfully boots
> to userspace without any hitch.
> 
> I do not have PASemi hardware on hand unfortunately, so I'm unable to test
> the impact of this patch on old PASemi hardware. This is also why I've
> elected to do the IRQ request and enablement on the Apple platform driver
> and not in the common file, as I'm not sure if PASemi hardware supports
> IRQs.
> 
> I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++".
> 
> Any and all critiques of the patch would be well appreciated.

That as well shouldn't be part of the commit message.

> 
> 
> 
> 
> Signed-off-by: Arminder Singh 
> ---
>   drivers/i2c/busses/i2c-pasemi-core.c | 29 
>   drivers/i2c/busses/i2c-pasemi-core.h |  5 
>   drivers/i2c/busses/i2c-pasemi-platform.c |  8 +++
>   3 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c 
> b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9028ffb58cc0..375aa9528233 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -21,6 +21,7 @@
>   #define REG_MTXFIFO0x00
>   #define REG_MRXFIFO0x04
>   #define REG_SMSTA  0x14
> +#define REG_IMASK   0x18
>   #define REG_CTL0x1c
>   #define REG_REV0x28
> 
> @@ -80,14 +81,21 @@ static int pasemi_smb_waitready(struct pasemi_smbus 
> *smbus)
>   {
>  int timeout = 10;
>  unsigned int status;
> +   unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;

Used only inside the if (smbus->use_irq), bitmask should be defined only 
in that scope.

> 
> -   status = reg_read(smbus, REG_SMSTA);

This should go in the else block.

> -
> -   while (!(status & SMSTA_XEN) && timeout--) {
> -   msleep(1);
> +   if (smbus->use_irq) {
> +   reinit_completion(&smbus->irq_completion);
> +   reg_write(smbus, REG_IMASK, bitmask);
> +   wait_for_completion_timeout(&smbus->irq_completion, 
> msecs_to_jiffies(10));
>  status = reg_read(smbus, REG_SMSTA);
> +   } else {

status is uninitialised when entering the while loop.

> +   while (!(status & SMSTA_XEN) && timeout--) {
> +   msleep(1);
> +   status = reg_read(smbus, REG_SMSTA);
> +   }
>  }
> 
> +

Why a second blank line here ?

>  /* Got NACK? */
>  if (status & SMSTA_MTN)
>  return -ENXIO;
> @@ -300,7 +308,7 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
>  case I2C_SMBUS_BLOCK_DATA:
>  case I2C_SMBUS_BLOCK_PROC_CALL:
>  data->block[0] = len;
> -   for (i = 1; i <= len; i ++) {
> +   for (i = 1; i <= len; i++) {

This shouldn't be part of this patch, it is unrelated.

>  rd = RXFIFO_RD(smbus);
>  if (rd & MRXFIFO_EMPTY) {
>  err = -ENODATA;
> @@ -348,6 +356,8 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
>  if (smbus->hw_rev != PASEMI_HW_REV_PCI)
>  smbus->hw_rev = reg_read(smbus, REG_REV);
> 
> +   reg_write(smbus, REG_IMASK, 0);
> +
>  pasemi_reset(smbus);
> 
>  error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
> @@ -356,3 +366,12 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> 
>  return 0;
>   }
> +
> +irqr

Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon

[Sven Peter]

Hi,

Thanks for the patch! Some additional comments:

On Sat, Aug 20, 2022, at 21:45, Arminder Singh wrote:
> This is the first time I'm interacting with the Linux mailing lists, so 
> please don't eviscerate me *too much* if I get the formatting wrong.
> Of course I'm always willing to take criticism and improve my formatting 
> in the future.
>
> This patch adds support for IRQs to the PASemi I2C controller driver.
> This will allow for faster performing I2C transactions on Apple Silicon
> hardware, as previously, the driver was forced to poll the SMSTA register
> for a set amount of time.
>
> With this patchset the driver on Apple silicon hardware will instead wait
> for an interrupt which will signal the completion of the I2C transaction.
> The timeout value for this completion will be the same as the current
> amount of time the I2C driver polls for.
>
> This will result in some performance improvement since the driver will be
> waiting for less time than it does right now on Apple Silicon hardware.
>
> The patch right now will only enable IRQs for Apple Silicon I2C chips,
> and only if it's able to successfully request the IRQ from the kernel.
>
> === Testing ===
>
> This patch has been tested on both the mainline Linux kernel tree and
> the Asahi branch (https://github.com/AsahiLinux/linux.git) on both an
> M1 and M2 MacBook Air, and it compiles successfully as both a module and
> built-in to the kernel itself. The patch in both trees successfully boots
> to userspace without any hitch.
>
> I do not have PASemi hardware on hand unfortunately, so I'm unable to test
> the impact of this patch on old PASemi hardware. This is also why I've
> elected to do the IRQ request and enablement on the Apple platform driver
> and not in the common file, as I'm not sure if PASemi hardware supports
> IRQs.
>
> I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++".
>
> Any and all critiques of the patch would be well appreciated.
>
>
>
>
> Signed-off-by: Arminder Singh 
> ---
>  drivers/i2c/busses/i2c-pasemi-core.c | 29 
>  drivers/i2c/busses/i2c-pasemi-core.h |  5 
>  drivers/i2c/busses/i2c-pasemi-platform.c |  8 +++
>  3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.c 
> b/drivers/i2c/busses/i2c-pasemi-core.c
> index 9028ffb58cc0..375aa9528233 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.c
> +++ b/drivers/i2c/busses/i2c-pasemi-core.c
> @@ -21,6 +21,7 @@
>  #define REG_MTXFIFO  0x00
>  #define REG_MRXFIFO  0x04
>  #define REG_SMSTA0x14
> +#define REG_IMASK   0x18
>  #define REG_CTL  0x1c
>  #define REG_REV  0x28
> 
> @@ -80,14 +81,21 @@ static int pasemi_smb_waitready(struct pasemi_smbus 
> *smbus)
>  {
>   int timeout = 10;
>   unsigned int status;
> + unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
> 
> - status = reg_read(smbus, REG_SMSTA);
> -
> - while (!(status & SMSTA_XEN) && timeout--) {
> - msleep(1);
> + if (smbus->use_irq) {
> + reinit_completion(&smbus->irq_completion);
> + reg_write(smbus, REG_IMASK, bitmask);

s/bitmask/SMSTA_XEN | SMSTA_MTN/ and then you can just drop the bitmask
variable which isn't used anywhere else.

> + wait_for_completion_timeout(&smbus->irq_completion, 
> msecs_to_jiffies(10));
>   status = reg_read(smbus, REG_SMSTA);

If the irq hasn't fired and wait_for_completion_timeout timed out the irq
is still enabled here. I'd put a reg_write(smbus, REG_IMASK, 0); here
to be safe.

> + } else {

You also need status = reg_read(smbus, REG_SMSTA); here.

> + while (!(status & SMSTA_XEN) && timeout--) {
> + msleep(1);
> + status = reg_read(smbus, REG_SMSTA);
> + }
>   }
> 
> +
>   /* Got NACK? */
>   if (status & SMSTA_MTN)
>   return -ENXIO;
> @@ -300,7 +308,7 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
>   case I2C_SMBUS_BLOCK_DATA:
>   case I2C_SMBUS_BLOCK_PROC_CALL:
>   data->block[0] = len;
> - for (i = 1; i <= len; i ++) {
> + for (i = 1; i <= len; i++) {
>   rd = RXFIFO_RD(smbus);
>   if (rd & MRXFIFO_EMPTY) {
>   err = -ENODATA;
> @@ -348,6 +356,8 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
>   if (smbus->hw_rev != PASEMI_HW_REV_PCI)
>   smbus->hw_rev = reg_read(smbus, REG_REV);
> 
> + reg_write(smbus, REG_IMASK, 0);
> +
>   pasemi_reset(smbus);
> 
>   error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter);
> @@ -356,3 +366,12 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus)
> 
>   return 0;
>  }
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id)
> +{
> + struct pasemi_smbus *smbus = (struct pasemi_smbus *)dev_id;
> +
> + reg_write(smbus, REG_IMASK, 0);
> + comp

Re: [PATCH] net: move from strlcpy with unused retval to strscpy

[Leon Romanovsky]

On Thu, Aug 18, 2022 at 11:00:34PM +0200, Wolfram Sang wrote:
> Follow the advice of the below link and prefer 'strscpy' in this
> subsystem. Conversion is 1:1 because the return value is not used.
> Generated by a coccinelle script.
> 
> Link: 
> https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=v6a6g1ouzcprm...@mail.gmail.com/
> Signed-off-by: Wolfram Sang 
> ---

<...>

>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c  |  6 +++---
>  drivers/net/ethernet/mellanox/mlx4/fw.c  |  2 +-
>  .../net/ethernet/mellanox/mlx5/core/en_ethtool.c |  4 ++--
>  drivers/net/ethernet/mellanox/mlx5/core/en_rep.c |  2 +-
>  .../ethernet/mellanox/mlx5/core/ipoib/ethtool.c  |  2 +-

Thanks,
Reviewed-by: Leon Romanovsky 

Re: [PATCH] kernel: exit: cleanup release_thread()

[Catalin Marinas]

On Fri, Aug 19, 2022 at 09:44:06AM +0800, Kefeng Wang wrote:
> Only x86 has own release_thread(), introduce a new weak
> release_thread() function to clean empty definitions in
> other ARCHs.
> 
> Signed-off-by: Kefeng Wang 
[...]
>  arch/arm64/include/asm/processor.h  | 3 ---
>  arch/arm64/kernel/process.c | 4 

Acked-by: Catalin Marinas 

Re: [PATCH] kernel: exit: cleanup release_thread()

[Huacai Chen]

For LoongArch parts:

Acked-by: Huacai Chen 

On Sun, Aug 21, 2022 at 7:54 PM Catalin Marinas  wrote:
>
> On Fri, Aug 19, 2022 at 09:44:06AM +0800, Kefeng Wang wrote:
> > Only x86 has own release_thread(), introduce a new weak
> > release_thread() function to clean empty definitions in
> > other ARCHs.
> >
> > Signed-off-by: Kefeng Wang 
> [...]
> >  arch/arm64/include/asm/processor.h  | 3 ---
> >  arch/arm64/kernel/process.c | 4 
>
> Acked-by: Catalin Marinas 

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Rob Herring]

On Sat, 20 Aug 2022 12:29:23 +0200, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Default level was always 192.
> 
> Signed-off-by: Pali Rohár 
> ---
>  .../devicetree/bindings/power/reset/syscon-reboot.yaml| 4 
>  1 file changed, 4 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml: 
Unresolvable JSON pointer: 'definitions/sint32'

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH] kernel: exit: cleanup release_thread()

[Stafford Horne]

On Fri, Aug 19, 2022 at 09:44:06AM +0800, Kefeng Wang wrote:
> Only x86 has own release_thread(), introduce a new weak
> release_thread() function to clean empty definitions in
> other ARCHs.
> 
> Signed-off-by: Kefeng Wang 
> ---

>  arch/openrisc/include/asm/processor.h   | 1 -
>  arch/openrisc/kernel/process.c  | 4 

> diff --git a/arch/openrisc/include/asm/processor.h 
> b/arch/openrisc/include/asm/processor.h
> index aa1699c18add..ed9efb430afa 100644
> --- a/arch/openrisc/include/asm/processor.h
> +++ b/arch/openrisc/include/asm/processor.h
> @@ -72,7 +72,6 @@ struct thread_struct {
>  
>  
>  void start_thread(struct pt_regs *regs, unsigned long nip, unsigned long sp);
> -void release_thread(struct task_struct *);
>  unsigned long __get_wchan(struct task_struct *p);
>  
>  #define cpu_relax() barrier()
> diff --git a/arch/openrisc/kernel/process.c b/arch/openrisc/kernel/process.c
> index 52dc983ddeba..f94b5ec06786 100644
> --- a/arch/openrisc/kernel/process.c
> +++ b/arch/openrisc/kernel/process.c
> @@ -125,10 +125,6 @@ void show_regs(struct pt_regs *regs)
>   show_registers(regs);
>  }
>  
> -void release_thread(struct task_struct *dead_task)
> -{
> -}
> -
>  /*
>   * Copy the thread-specific (arch specific) info from the current
>   * process to the new one p

For OpenRISC bits.

Acked-by: Stafford Horne 

Re: [PATCH] kernel: exit: cleanup release_thread()

[Michael Ellerman]

Kefeng Wang  writes:
> Only x86 has own release_thread(), introduce a new weak
> release_thread() function to clean empty definitions in
> other ARCHs.
>
> Signed-off-by: Kefeng Wang 
> ---
..
>  arch/powerpc/include/asm/processor.h| 1 -
>  arch/powerpc/kernel/process.c   | 5 -

Acked-by: Michael Ellerman  (powerpc)

cheers

[RFC 08/10] cpuhp: Replace cpumask_any_but(cpu_online_mask, cpu)

[Pingfan Liu]

In a kexec quick reboot path, the dying cpus are still on
cpu_online_mask. During the teardown of cpu, a subsystem needs to
migrate its broker to a real online cpu.

This patch replaces cpumask_any_but(cpu_online_mask, cpu) in a teardown
procedure with cpumask_not_dying_but(cpu_online_mask, cpu).

Signed-off-by: Pingfan Liu 
Cc: Russell King 
Cc: Shawn Guo 
Cc: Sascha Hauer 
Cc: Pengutronix Kernel Team 
Cc: Fabio Estevam 
Cc: NXP Linux Team 
Cc: Fenghua Yu 
Cc: Dave Jiang 
Cc: Vinod Koul 
Cc: Wu Hao 
Cc: Tom Rix 
Cc: Moritz Fischer 
Cc: Xu Yilun 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Rodrigo Vivi 
Cc: Tvrtko Ursulin 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Frank Li 
Cc: Shaokun Zhang 
Cc: Qi Liu 
Cc: Andy Gross 
Cc: Bjorn Andersson 
Cc: Konrad Dybcio 
Cc: Khuong Dinh 
Cc: Li Yang 
Cc: Yury Norov 
To: linux-arm-ker...@lists.infradead.org
To: dmaeng...@vger.kernel.org
To: linux-f...@vger.kernel.org
To: intel-...@lists.freedesktop.org
To: dri-de...@lists.freedesktop.org
To: linux-arm-...@vger.kernel.org
To: linuxppc-dev@lists.ozlabs.org
To: linux-ker...@vger.kernel.org
---
 arch/arm/mach-imx/mmdc.c | 2 +-
 arch/arm/mm/cache-l2x0-pmu.c | 2 +-
 drivers/dma/idxd/perfmon.c   | 2 +-
 drivers/fpga/dfl-fme-perf.c  | 2 +-
 drivers/gpu/drm/i915/i915_pmu.c  | 2 +-
 drivers/perf/arm-cci.c   | 2 +-
 drivers/perf/arm-ccn.c   | 2 +-
 drivers/perf/arm-cmn.c   | 4 ++--
 drivers/perf/arm_dmc620_pmu.c| 2 +-
 drivers/perf/arm_dsu_pmu.c   | 2 +-
 drivers/perf/arm_smmuv3_pmu.c| 2 +-
 drivers/perf/fsl_imx8_ddr_perf.c | 2 +-
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 2 +-
 drivers/perf/marvell_cn10k_tad_pmu.c | 2 +-
 drivers/perf/qcom_l2_pmu.c   | 2 +-
 drivers/perf/qcom_l3_pmu.c   | 2 +-
 drivers/perf/xgene_pmu.c | 2 +-
 drivers/soc/fsl/qbman/bman_portal.c  | 2 +-
 drivers/soc/fsl/qbman/qman_portal.c  | 2 +-
 19 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
index af12668d0bf5..a109a7ea8613 100644
--- a/arch/arm/mach-imx/mmdc.c
+++ b/arch/arm/mach-imx/mmdc.c
@@ -220,7 +220,7 @@ static int mmdc_pmu_offline_cpu(unsigned int cpu, struct 
hlist_node *node)
if (!cpumask_test_and_clear_cpu(cpu, &pmu_mmdc->cpu))
return 0;
 
-   target = cpumask_any_but(cpu_online_mask, cpu);
+   target = cpumask_not_dying_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids)
return 0;
 
diff --git a/arch/arm/mm/cache-l2x0-pmu.c b/arch/arm/mm/cache-l2x0-pmu.c
index 993fefdc167a..1b0037ef7fa5 100644
--- a/arch/arm/mm/cache-l2x0-pmu.c
+++ b/arch/arm/mm/cache-l2x0-pmu.c
@@ -428,7 +428,7 @@ static int l2x0_pmu_offline_cpu(unsigned int cpu)
if (!cpumask_test_and_clear_cpu(cpu, &pmu_cpu))
return 0;
 
-   target = cpumask_any_but(cpu_online_mask, cpu);
+   target = cpumask_not_dying_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids)
return 0;
 
diff --git a/drivers/dma/idxd/perfmon.c b/drivers/dma/idxd/perfmon.c
index d73004f47cf4..f3f1ccb55f73 100644
--- a/drivers/dma/idxd/perfmon.c
+++ b/drivers/dma/idxd/perfmon.c
@@ -528,7 +528,7 @@ static int perf_event_cpu_offline(unsigned int cpu, struct 
hlist_node *node)
if (!cpumask_test_and_clear_cpu(cpu, &perfmon_dsa_cpu_mask))
return 0;
 
-   target = cpumask_any_but(cpu_online_mask, cpu);
+   target = cpumask_not_dying_but(cpu_online_mask, cpu);
 
/* migrate events if there is a valid target */
if (target < nr_cpu_ids)
diff --git a/drivers/fpga/dfl-fme-perf.c b/drivers/fpga/dfl-fme-perf.c
index 587c82be12f7..57804f28357e 100644
--- a/drivers/fpga/dfl-fme-perf.c
+++ b/drivers/fpga/dfl-fme-perf.c
@@ -948,7 +948,7 @@ static int fme_perf_offline_cpu(unsigned int cpu, struct 
hlist_node *node)
if (cpu != priv->cpu)
return 0;
 
-   target = cpumask_any_but(cpu_online_mask, cpu);
+   target = cpumask_not_dying_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids)
return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 958b37123bf1..f866f9223492 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -1068,7 +1068,7 @@ static int i915_pmu_cpu_offline(unsigned int cpu, struct 
hlist_node *node)
return 0;
 
if (cpumask_test_and_clear_cpu(cpu, &i915_pmu_cpumask)) {
-   target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
+   target = cpumask_not_dying_but(topology_sibling_cpumask(cpu), 
cpu);
 
/* Migrate events if there is a valid target */
if (target < nr_cpu_ids) {
diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c
index 03b1309875ae..481

Re: [PATCH] powerpc: export cpu_smallcore_map for modules

[Michael Ellerman]

Randy Dunlap  writes:
> Fix build error when CONFIG_DRM_AMDGPU=m:
>
> ERROR: modpost: "cpu_smallcore_map" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] 
> undefined!
>
> by exporting 'cpu_smallcore_map' just as other per_cpu
> symbols are exported.
>
> drivers/gpu/drm/amd/amdkfd/kfd_device.c calls cpu_smt_mask().
> This is an inline function on powerpc which references
> cpu_smallcore_map.
>
> Fixes: 425752c63b6f ("powerpc: Detect the presence of big-cores via "ibm, 
> thread-groups"")
> Fixes: 7bc913085765 ("drm/amdkfd: Try to schedule bottom half on same core")

That 2nd commit is not in mainline, only linux-next.

I don't mind merging this fix preemptively, but is that SHA stable?

cheers

Re: [PATCH] powerpc: export cpu_smallcore_map for modules

[Randy Dunlap]

On 8/21/22 20:40, Michael Ellerman wrote:
> Randy Dunlap  writes:
>> Fix build error when CONFIG_DRM_AMDGPU=m:
>>
>> ERROR: modpost: "cpu_smallcore_map" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] 
>> undefined!
>>
>> by exporting 'cpu_smallcore_map' just as other per_cpu
>> symbols are exported.
>>
>> drivers/gpu/drm/amd/amdkfd/kfd_device.c calls cpu_smt_mask().
>> This is an inline function on powerpc which references
>> cpu_smallcore_map.
>>
>> Fixes: 425752c63b6f ("powerpc: Detect the presence of big-cores via "ibm, 
>> thread-groups"")
>> Fixes: 7bc913085765 ("drm/amdkfd: Try to schedule bottom half on same core")
> 
> That 2nd commit is not in mainline, only linux-next.
> 
> I don't mind merging this fix preemptively, but is that SHA stable?

Felix, Alex, can you answer that, please?

-- 
~Randy

Re: [PATCH v3 14/18] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Rohan McLure]

> On 19 Aug 2022, at 4:52 pm, Christophe Leroy  
> wrote:
> 
> 
> 
> Le 19/08/2022 à 05:38, Rohan McLure a écrit :
>> Clear user state in gprs (assign to zero) to reduce the influence of user
>> registers on speculation within kernel syscall handlers. Clears occur
>> at the very beginning of the sc and scv 0 interrupt handlers, with
>> restores occurring following the execution of the syscall handler.
>> 
>> Signed-off-by: Rohan McLure 
>> ---
>> V1 -> V2: Update summary
>> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
>> ---
>>  arch/powerpc/kernel/interrupt_64.S | 22 ++
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/interrupt_64.S 
>> b/arch/powerpc/kernel/interrupt_64.S
>> index 0178aeba3820..d9625113c7a5 100644
>> --- a/arch/powerpc/kernel/interrupt_64.S
>> +++ b/arch/powerpc/kernel/interrupt_64.S
>> @@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
>>  ld  r2,PACATOC(r13)
>>  mfcrr12
>>  li  r11,0
>> -/* Can we avoid saving r3-r8 in common case? */
>> +/* Save syscall parameters in r3-r8 */
>>  std r3,GPR3(r1)
>>  std r4,GPR4(r1)
>>  std r5,GPR5(r1)
>> @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>   * but this is the best we can do.
>>   */
>> 
>> +/*
>> + * Zero user registers to prevent influencing speculative execution
>> + * state of kernel code.
>> + */
>> +NULLIFY_GPRS(5, 12)
> 
> Macro name has changed.
> 
>> +NULLIFY_NVGPRS()
> 
> Why clearing non volatile GPRs ? They are supposed to be callee saved so 
> I can't see how they could be used for speculation. Do you have any 
> exemple ?
> 
>> +
>>  /* Calling convention has r3 = orig r0, r4 = regs */
>>  mr  r3,r0
>>  bl  system_call_exception
>> @@ -139,6 +146,7 @@ BEGIN_FTR_SECTION
>>  HMT_MEDIUM_LOW
>>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> 
>> +REST_NVGPRS(r1)
> 
> 
> 
>>  cmpdi   r3,0
>>  bne .Lsyscall_vectored_\name\()_restore_regs
>> 
>> @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>  ld  r4,_LINK(r1)
>>  ld  r5,_XER(r1)
>> 
>> -REST_NVGPRS(r1)
>>  ld  r0,GPR0(r1)
>>  mtcrr2
>>  mtctr   r3
>> @@ -249,7 +256,7 @@ END_BTB_FLUSH_SECTION
>>  ld  r2,PACATOC(r13)
>>  mfcrr12
>>  li  r11,0
>> -/* Can we avoid saving r3-r8 in common case? */
>> +/* Save syscall parameters in r3-r8 */
>>  std r3,GPR3(r1)
>>  std r4,GPR4(r1)
>>  std r5,GPR5(r1)
>> @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION
>>  wrteei  1
>>  #endif
>> 
>> +/*
>> + * Zero user registers to prevent influencing speculative execution
>> + * state of kernel code.
>> + */
>> +NULLIFY_GPRS(5, 12)
>> +NULLIFY_NVGPRS()
>> +
> 
> Name has changed.
> 
>>  /* Calling convention has r3 = orig r0, r4 = regs */
>>  mr  r3,r0
>>  bl  system_call_exception
>> @@ -342,6 +356,7 @@ BEGIN_FTR_SECTION
>>  stdcx.  r0,0,r1 /* to clear the reservation */
>>  END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> 
>> +REST_NVGPRS(r1)
> 
> Same question.
> 
>>  cmpdi   r3,0
>>  bne .Lsyscall_restore_regs
>>  /* Zero volatile regs that may contain sensitive kernel data */
>> @@ -377,7 +392,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>  .Lsyscall_restore_regs:
>>  ld  r3,_CTR(r1)
>>  ld  r4,_XER(r1)
>> -REST_NVGPRS(r1)
>>  mtctr   r3
>>  mtspr   SPRN_XER,r4
>>  ld  r0,GPR0(r1)


> Why clearing non volatile GPRs ? They are supposed to be callee saved so 
> I can't see how they could be used for speculation. Do you have any 
> exemple ?

Even though non-volatiles will be callee-saved subject to ABI in the syscall 
handler prologue, it is still conceivable that a syscall handler will leave a 
register uninitialised on one branch outcome, assigning that register in the 
other.

On speculative processors, there remains the possibility for untaken branches 
to be executed microarchitecturally (by mistraining the branch predictor or 
otherwise),
whereby the microarchitectural effects of the execution present a side-channel.

Such a hardening measure removes the burden for developers to prove that 
toolchains
will never emit code with these properties in code reachable via a syscall. For 
what
it’s worth, precedent already exists in arm64 and x86.

Links:
https://lore.kernel.org/linux-arm-kernel/20180702110415.10465-13-mark.rutl...@arm.com/
 

https://lore.kernel.org/lkml/20180405095307.3730-8-li...@dominikbrodowski.net/ 


> What is the link between this change and the description in the commit 
> message ?

I’ll include a detail on why we must restore nvgprs in both cas

Re: [6.0-rc1] Kernel crash while running MCE tests

[Sachin Sant]

> On 19-Aug-2022, at 10:12 AM, Ganesh  wrote
>> 
>> We'll have to make sure everything get_pseries_errorlog() is either
>> forced inline, or marked noinstr.
>> 
> Making the following functions always_inline and noinstr is fixing the issue.
> __always_inline pseries_errorlog_id()
> __always_inline pseries_errorlog_length()
> __always_inline do_enter_rtas()
> __always_inline srr_regs_clobbered()
> noinstr va_rtas_call_unlocked()
> 
> Shall I post the patch?

Yes, thanks. I can help with testing.

- Sachin

Re: [6.0-rc1] Kernel crash while running MCE tests

[Michael Ellerman]

Ganesh  writes:
> On 8/17/22 11:28, Michael Ellerman wrote:
>
>> Sachin Sant  writes:
>>> Following crash is seen while running powerpc/mce subtest on
>>> a Power10 LPAR.
>>>
>>> 1..1
>>> # selftests: powerpc/mce: inject-ra-err
>>> [  155.240591] BUG: Unable to handle kernel data access on read at 
>>> 0xc00e00022d55b503
>>> [  155.240618] Faulting instruction address: 0xc06f1f0c
>>> [  155.240627] Oops: Kernel access of bad area, sig: 11 [#1]
>>> [  155.240633] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>>> [  155.240642] Modules linked in: dm_mod mptcp_diag xsk_diag tcp_diag 
>>> udp_diag raw_diag inet_diag unix_diag af_packet_diag netlink_diag 
>>> nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet 
>>> nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat 
>>> nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 bonding rfkill tls ip_set 
>>> nf_tables nfnetlink sunrpc binfmt_misc pseries_rng drm 
>>> drm_panel_orientation_quirks xfs libcrc32c sd_mod t10_pi sr_mod 
>>> crc64_rocksoft_generic cdrom crc64_rocksoft crc64 sg ibmvscsi ibmveth 
>>> scsi_transport_srp xts vmx_crypto fuse
>>> [  155.240750] CPU: 4 PID: 3645 Comm: inject-ra-err Not tainted 6.0.0-rc1 #2
>>> [  155.240761] NIP:  c06f1f0c LR: c00630d0 CTR: 
>>> 
>>> [  155.240768] REGS: c000ff887890 TRAP: 0300   Not tainted  (6.0.0-rc1)
>>> [  155.240776] MSR:  80001003   CR: 48002828  XER: 
>>> 
>>  ^
>>  MMU is off, aka. real mode.
>>
>>> [  155.240792] CFAR: c00630cc DAR: c00e00022d55b503 DSISR: 4000 
>>> IRQMASK: 3
>>> [  155.240792] GPR00: c00630d0 c000ff887b30 c44afe00 
>>> c0116aada818
>>> [  155.240792] GPR04: 4d43 0008 c00630d0 
>>> 004d4249
>>> [  155.240792] GPR08: 0001 18022d55b503 a80e 
>>> 0348
>>> [  155.240792] GPR12:  c000b700  
>>> 
>>> [  155.240792] GPR16:    
>>> 
>>> [  155.240792] GPR20:    
>>> 1b30
>>> [  155.240792] GPR24: 7fff8dad 7fff8dacf6d8 7fffd1551e98 
>>> 1001fce8
>>> [  155.240792] GPR28: c0116aada888 c0116aada800 4d43 
>>> c0116aada818
>>> [  155.240885] NIP [c06f1f0c] __asan_load2+0x5c/0xe0
>>> [  155.240898] LR [c00630d0] pseries_errorlog_id+0x20/0x40
>>> [  155.240910] Call Trace:
>>> [  155.240914] [c000ff887b50] [c00630d0] 
>>> pseries_errorlog_id+0x20/0x40
>>> [  155.240925] [c000ff887b80] [c15595c8] 
>>> get_pseries_errorlog+0xa8/0x110
>>   
>> get_pseries_errorlog() is marked noinstr.
>>
>> And pseries_errorlog_id() is:
>>
>> static
>> inline uint16_t pseries_errorlog_id(struct pseries_errorlog *sect)
>> {
>>  return be16_to_cpu(sect->id);
>> }
>>
>> So I guess the compiler has decided not to inline it (why?!), and it is
>> not marked noinstr, so it gets KASAN instrumentation which crashes in
>> real mode.
>>
>> We'll have to make sure everything get_pseries_errorlog() is either
>> forced inline, or marked noinstr.
>
> Making the following functions always_inline and noinstr is fixing the issue.
> __always_inline pseries_errorlog_id()
> __always_inline pseries_errorlog_length()
> __always_inline do_enter_rtas()
> __always_inline srr_regs_clobbered()
> noinstr va_rtas_call_unlocked()

Why do we need it? Because of fwnmi_release_errinfo()?

> Shall I post the patch?

Yeah.

cheers

Re: [PATCH linux-next] powerpc: disable sanitizer in irq_soft_mask_set

[Christophe Leroy]

Le 21/08/2022 à 03:00, Zhouyi Zhou a écrit :
> In ppc, compiler based sanitizer will generate instrument instructions
> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
> 
> 0xc0295cb0 <+0>:  addis   r2,r12,774
> 0xc0295cb4 <+4>:  addir2,r2,16464
> 0xc0295cb8 <+8>:  mflrr0
> 0xc0295cbc <+12>: bl  0xc008bb4c 
> 0xc0295cc0 <+16>: mflrr0
> 0xc0295cc4 <+20>: std r31,-8(r1)
> 0xc0295cc8 <+24>: addir3,r13,2354
> 0xc0295ccc <+28>: mr  r31,r13
> 0xc0295cd0 <+32>: std r0,16(r1)
> 0xc0295cd4 <+36>: stdur1,-48(r1)
> 0xc0295cd8 <+40>: bl  0xc0609b98 <__asan_store1+8>
> 0xc0295cdc <+44>: nop
> 0xc0295ce0 <+48>: li  r9,1
> 0xc0295ce4 <+52>: stb r9,2354(r31)
> 0xc0295ce8 <+56>: addir1,r1,48
> 0xc0295cec <+60>: ld  r0,16(r1)
> 0xc0295cf0 <+64>: ld  r31,-8(r1)
> 0xc0295cf4 <+68>: mtlrr0
> 
> If there is a context switch before "stb r9,2354(r31)", r31 may
> not equal to r13, in such case, irq soft mask will not work.
> 
> This patch disable sanitizer in irq_soft_mask_set.

Well spotted, thanks.

You should add:

Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")

By the way, I think the READ_ONCE() likely has the same issue so you 
should fix irq_soft_mask_return() at the same time.

> 
> Signed-off-by: Zhouyi Zhou 
> ---
> Dear PPC developers
> 
> I found this bug when trying to do rcutorture tests in ppc VM of
> Open Source Lab of Oregon State University following Paul E. McKenny's 
> guidance.
> 
> console.log report following bug:
> 
> [  346.527467][  T100] BUG: using smp_processor_id() in preemptible 
> [] code: rcu_torture_rea/100^M
> [  346.529416][  T100] caller is 
> rcu_preempt_deferred_qs_irqrestore+0x74/0xed0^M
> [  346.531157][  T100] CPU: 4 PID: 100 Comm: rcu_torture_rea Tainted: G   
>  W  5.19.0-rc5-next-20220708-dirty #253^M
> [  346.533620][  T100] Call Trace:^M
> [  346.534449][  T100] [c94876c0] [c0ce2b68] 
> dump_stack_lvl+0xbc/0x108 (unreliable)^M
> [  346.536632][  T100] [c9487710] [c1712954] 
> check_preemption_disabled+0x154/0x160^M
> [  346.538665][  T100] [c94877a0] [c02ce2d4] 
> rcu_preempt_deferred_qs_irqrestore+0x74/0xed0^M
> [  346.540830][  T100] [c94878b0] [c02cf3c0] 
> __rcu_read_unlock+0x290/0x3b0^M
> [  346.542746][  T100] [c9487910] [c02bb330] 
> rcu_torture_read_unlock+0x30/0xb0^M
> [  346.544779][  T100] [c9487930] [c02b7ff8] 
> rcutorture_one_extend+0x198/0x810^M
> [  346.546851][  T100] [c9487a10] [c02b8bfc] 
> rcu_torture_one_read+0x58c/0xc90^M
> [  346.548844][  T100] [c9487ca0] [c02b942c] 
> rcu_torture_reader+0x12c/0x360^M
> [  346.550784][  T100] [c9487db0] [c01de978] 
> kthread+0x1e8/0x220^M
> [  346.552555][  T100] [c9487e10] [c000cd54] 
> ret_from_kernel_thread+0x5c/0x64^M
> 
> After 12 days debugging, I finally narrow the problem to irq_soft_mask_set.
> 
> I am a beginner, hope I can be of some beneficial to the community ;-)
> 
> Thanks
> Zhouyi
> --
>   arch/powerpc/include/asm/hw_irq.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_irq.h 
> b/arch/powerpc/include/asm/hw_irq.h
> index 26ede09c521d..a5ae8d82cc9d 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -121,7 +121,7 @@ static inline notrace unsigned long 
> irq_soft_mask_return(void)
>* for the critical section and as a clobber because
>* we changed paca->irq_soft_mask
>*/
> -static inline notrace void irq_soft_mask_set(unsigned long mask)
> +static inline notrace __no_kcsan __no_sanitize_address void 
> irq_soft_mask_set(unsigned long mask)
>   {
>   /*
>* The irq mask must always include the STD bit if any are set.