RE: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the driver's private data

[Leo Li <leoyang.li@nxp.com>]

> -----Original Message-----
> From: Leo Li
> Sent: Friday, October 16, 2020 4:20 PM
> To: 'Yi Wang' <wang.yi59@zte.com.cn>; Roy Pledge <roy.pledge@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org; xue.zhihong@zte.com.cn;
> jiang.xuexin@zte.com.cn; Hao Si <si.hao@zte.com.cn>; Lin Chen
> <chen.lin5@zte.com.cn>
> Subject: RE: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the
> driver's private data
> 
> 
> 
> > -----Original Message-----
> > From: Yi Wang <wang.yi59@zte.com.cn>
> > Sent: Friday, October 16, 2020 1:49 AM
> > To: Roy Pledge <roy.pledge@nxp.com>; Laurentiu Tudor
> > <laurentiu.tudor@nxp.com>
> > Cc: Leo Li <leoyang.li@nxp.com>; linux-kernel@vger.kernel.org;
> > linuxppc- dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> > xue.zhihong@zte.com.cn; wang.yi59@zte.com.cn;
> jiang.xuexin@zte.com.cn;
> > Hao Si <si.hao@zte.com.cn>; Lin Chen <chen.lin5@zte.com.cn>
> > Subject: [PATCH v2] soc: fsl: dpio: Change 'cpumask_t mask' to the
> > driver's private data
> >
> > From: Hao Si <si.hao@zte.com.cn>
> >
> > The local variable 'cpumask_t mask' is in the stack memory, and its
> > address is assigned to 'desc->affinity' in 'irq_set_affinity_hint()'.
> > But the memory area where this variable is located is at risk of being
> > modified.
> >
> > During LTP testing, the following error was generated:
> >
> > Unable to handle kernel paging request at virtual address
> > ffff000012e9b790 Mem abort info:
> >   ESR = 0x96000007
> >   Exception class = DABT (current EL), IL = 32 bits
> >   SET = 0, FnV = 0
> >   EA = 0, S1PTW = 0
> > Data abort info:
> >   ISV = 0, ISS = 0x00000007
> >   CM = 0, WnR = 0
> > swapper pgtable: 4k pages, 48-bit VAs, pgdp = 0000000075ac5e07
> > [ffff000012e9b790] pgd=00000027dbffe003, pud=00000027dbffd003,
> > pmd=00000027b6d61003, pte=0000000000000000 Internal error: Oops:
> > 96000007 [#1] PREEMPT SMP Modules linked in: xt_conntrack Process
> > read_all (pid: 20171, stack limit = 0x0000000044ea4095)
> > CPU: 14 PID: 20171 Comm: read_all Tainted: G    B   W
> > Hardware name: NXP Layerscape LX2160ARDB (DT)
> > pstate: 80000085 (Nzcv daIf -PAN -UAO) pc :
> > irq_affinity_hint_proc_show+0x54/0xb0
> > lr : irq_affinity_hint_proc_show+0x4c/0xb0
> > sp : ffff00001138bc10
> > x29: ffff00001138bc10 x28: 0000ffffd131d1e0
> > x27: 00000000007000c0 x26: ffff8025b9480dc0
> > x25: ffff8025b9480da8 x24: 00000000000003ff
> > x23: ffff8027334f8300 x22: ffff80272e97d000
> > x21: ffff80272e97d0b0 x20: ffff8025b9480d80
> > x19: ffff000009a49000 x18: 0000000000000000
> > x17: 0000000000000000 x16: 0000000000000000
> > x15: 0000000000000000 x14: 0000000000000000
> > x13: 0000000000000000 x12: 0000000000000040
> > x11: 0000000000000000 x10: ffff802735b79b88
> > x9 : 0000000000000000 x8 : 0000000000000000
> > x7 : ffff000009a49848 x6 : 0000000000000003
> > x5 : 0000000000000000 x4 : ffff000008157d6c
> > x3 : ffff00001138bc10 x2 : ffff000012e9b790
> > x1 : 0000000000000000 x0 : 0000000000000000 Call trace:
> >  irq_affinity_hint_proc_show+0x54/0xb0
> >  seq_read+0x1b0/0x440
> >  proc_reg_read+0x80/0xd8
> >  __vfs_read+0x60/0x178
> >  vfs_read+0x94/0x150
> >  ksys_read+0x74/0xf0
> >  __arm64_sys_read+0x24/0x30
> >  el0_svc_common.constprop.0+0xd8/0x1a0
> >  el0_svc_handler+0x34/0x88
> >  el0_svc+0x10/0x14
> > Code: f9001bbf 943e0732 f94066c2 b4000062 (f9400041) ---[ end trace
> > b495bdcb0b3b732b ]--- Kernel panic - not syncing: Fatal exception
> > SMP: stopping secondary CPUs
> > SMP: failed to stop secondary CPUs 0,2-4,6,8,11,13-15 Kernel Offset:
> > disabled CPU features: 0x0,21006008 Memory Limit: none ---[ end Kernel
> > panic - not syncing: Fatal exception ]---
> >
> > Fix it by changing 'cpumask_t mask' to the driver's private data.
> 
> Thanks for the patch.  Sorry to chime in late.
> 
> Since we are only setting single bit in the cpumask, it is actually not necessary
> to keep the cpumask in private data as we already kept the cpu number in
> desc.cpu.  The better and easier approach is to actually use
> get_cpu_mask(cpu) API to get the pre-defined cpumask in the static
> cpu_bit_bitmap array.  We don't even need to declare the mask/cpu_mask
> in the register_dpio_irq_handlers().

Btw, cpumask_of(cpu) is more commonly used than get_cpu_mask(cpu), although they are essentially the same.

> 
> >
> > Signed-off-by: Hao Si <si.hao@zte.com.cn>
> > Signed-off-by: Lin Chen <chen.lin5@zte.com.cn>
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > ---
> > v2: Place 'cpumask_t mask' in the driver's private data and while at
> > it, rename it to cpu_mask.
> >
> >  drivers/soc/fsl/dpio/dpio-driver.c | 9 +++++----
> >  include/linux/fsl/mc.h             | 2 ++
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soc/fsl/dpio/dpio-driver.c
> > b/drivers/soc/fsl/dpio/dpio- driver.c index 7b642c3..e9d820d 100644
> > --- a/drivers/soc/fsl/dpio/dpio-driver.c
> > +++ b/drivers/soc/fsl/dpio/dpio-driver.c
> > @@ -95,7 +95,7 @@ static int register_dpio_irq_handlers(struct
> > fsl_mc_device *dpio_dev, int cpu)  {
> >  	int error;
> >  	struct fsl_mc_device_irq *irq;
> > -	cpumask_t mask;
> > +	cpumask_t *cpu_mask;
> >
> >  	irq = dpio_dev->irqs[0];
> >  	error = devm_request_irq(&dpio_dev->dev, @@ -112,9 +112,10 @@
> static
> > int register_dpio_irq_handlers(struct fsl_mc_device *dpio_dev, int
> > cpu)
> >  	}
> >
> >  	/* set the affinity hint */
> > -	cpumask_clear(&mask);
> > -	cpumask_set_cpu(cpu, &mask);
> > -	if (irq_set_affinity_hint(irq->msi_desc->irq, &mask))
> > +	cpu_mask = &dpio_dev->mask;
> > +	cpumask_clear(cpu_mask);
> > +	cpumask_set_cpu(cpu, cpu_mask);
> > +	if (irq_set_affinity_hint(irq->msi_desc->irq, cpu_mask))
> >  		dev_err(&dpio_dev->dev,
> >  			"irq_set_affinity failed irq %d cpu %d\n",
> >  			irq->msi_desc->irq, cpu);
> > diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h index
> > a428c61..ebdfb54 100644
> > --- a/include/linux/fsl/mc.h
> > +++ b/include/linux/fsl/mc.h
> > @@ -151,6 +151,7 @@ struct fsl_mc_obj_desc {
> >  /**
> >   * struct fsl_mc_device - MC object device object
> >   * @dev: Linux driver model device object
> > + * @mask: cpu mask for affinity_hint
> >   * @dma_mask: Default DMA mask
> >   * @flags: MC object device flags
> >   * @icid: Isolation context ID for the device @@ -184,6 +185,7 @@
> > struct fsl_mc_obj_desc {
> >   */
> >  struct fsl_mc_device {
> >  	struct device dev;
> > +	cpumask_t mask;
> >  	u64 dma_mask;
> >  	u16 flags;
> >  	u16 icid;
> > --
> > 2.15.2