| Message ID | 1398735103-16513-1-git-send-email-scottwood@freescale.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Scott Wood |
| Headers | show |
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, April 29, 2014 9:32 AM > To: linuxppc-dev@lists.ozlabs.org > Cc: Wood Scott-B07421; Liu Gang-B34182 > Subject: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use- > after-unmap > > Several of the error paths from fsl_rio_setup are missing error messages. > > Worse, fsl_rio_setup initializes several global pointers and does not > NULL them out after freeing/unmapping on error. This caused > fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which was > non-NULL but had been unmapped. > > Signed-off-by: Scott Wood <scottwood@freescale.com> > Cc: Liu Gang <Gang.Liu@freescale.com> > --- > Liu Gang, are you sure all of these error conditions are fatal? Why does > the rio driver fail if rmu is not present (e.g. on t4240)? Hi Scott, I think the errors you modified in the patch are serious and should be fixed. Thanks very much! And in fact, the rio driver can be used just for the submodule of the SRIO: RMU. It should be used with arch/powerpc/sysdev/fsl_rmu.c and there should have the RMU module. The fsl_rio.c implements some basic and needed works to support the RMU running well. In addition, could you please help to apply the following patch: http://patchwork.ozlabs.org/patch/337810/ Thanks! Liu Gang > --- > arch/powerpc/sysdev/fsl_rio.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-)
On Mon, 2014-04-28 at 23:04 -0500, Liu Gang-B34182 wrote: > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Tuesday, April 29, 2014 9:32 AM > > To: linuxppc-dev@lists.ozlabs.org > > Cc: Wood Scott-B07421; Liu Gang-B34182 > > Subject: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use- > > after-unmap > > > > Several of the error paths from fsl_rio_setup are missing error messages. > > > > Worse, fsl_rio_setup initializes several global pointers and does not > > NULL them out after freeing/unmapping on error. This caused > > fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which was > > non-NULL but had been unmapped. > > > > Signed-off-by: Scott Wood <scottwood@freescale.com> > > Cc: Liu Gang <Gang.Liu@freescale.com> > > --- > > Liu Gang, are you sure all of these error conditions are fatal? Why does > > the rio driver fail if rmu is not present (e.g. on t4240)? > > Hi Scott, I think the errors you modified in the patch are serious and should > be fixed. Thanks very much! > And in fact, the rio driver can be used just for the submodule of the SRIO: RMU. > It should be used with arch/powerpc/sysdev/fsl_rmu.c and there should have the > RMU module. > The fsl_rio.c implements some basic and needed works to support the RMU running well. I don't quite follow -- is it expected that rio can work without rmu, or not? As is, fsl_rio_setup() will error out if it doesn't find an fsl,srio-rmu-handle property. > In addition, could you please help to apply the following patch: > > http://patchwork.ozlabs.org/patch/337810/ Yes, I'll apply it when I do my next batch of patch applications. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, April 30, 2014 1:13 AM > To: Liu Gang-B34182 > Cc: linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and > use-after-unmap > > On Mon, 2014-04-28 at 23:04 -0500, Liu Gang-B34182 wrote: > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, April 29, 2014 9:32 AM > > > To: linuxppc-dev@lists.ozlabs.org > > > Cc: Wood Scott-B07421; Liu Gang-B34182 > > > Subject: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and > > > use- after-unmap > > > > > > Several of the error paths from fsl_rio_setup are missing error > messages. > > > > > > Worse, fsl_rio_setup initializes several global pointers and does > > > not NULL them out after freeing/unmapping on error. This caused > > > fsl_rio_mcheck_exception() to crash when accessing rio_regs_win > > > which was non-NULL but had been unmapped. > > > > > > Signed-off-by: Scott Wood <scottwood@freescale.com> > > > Cc: Liu Gang <Gang.Liu@freescale.com> > > > --- > > > Liu Gang, are you sure all of these error conditions are fatal? Why > > > does the rio driver fail if rmu is not present (e.g. on t4240)? > > > > Hi Scott, I think the errors you modified in the patch are serious and > > should be fixed. Thanks very much! > > And in fact, the rio driver can be used just for the submodule of the > SRIO: RMU. > > It should be used with arch/powerpc/sysdev/fsl_rmu.c and there should > > have the RMU module. > > The fsl_rio.c implements some basic and needed works to support the RMU > running well. > > I don't quite follow -- is it expected that rio can work without rmu, or > not? As is, fsl_rio_setup() will error out if it doesn't find an > fsl,srio-rmu-handle property. fsl_rio_setup() doesn't expect that rio can work without rmu. All the rio drivers just has one purpose, it's rmu. But rmu is a submodule of the rio, so the driver should parse rio in dtb and finish some initial works first, and then to setup rmu. That's why the drivers cannot just have a such as rmu_setup() to parse fsl,srio-rmu-handle property. Thanks, Liu Gang
diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c index cf2b084..c04b718 100644 --- a/arch/powerpc/sysdev/fsl_rio.c +++ b/arch/powerpc/sysdev/fsl_rio.c @@ -391,8 +391,10 @@ int fsl_rio_setup(struct platform_device *dev) ops->get_inb_message = fsl_get_inb_message; rmu_node = of_parse_phandle(dev->dev.of_node, "fsl,srio-rmu-handle", 0); - if (!rmu_node) + if (!rmu_node) { + dev_err(&dev->dev, "No valid fsl,srio-rmu-handle property\n"); goto err_rmu; + } rc = of_address_to_resource(rmu_node, 0, &rmu_regs); if (rc) { dev_err(&dev->dev, "Can't get %s property 'reg'\n", @@ -413,6 +415,7 @@ int fsl_rio_setup(struct platform_device *dev) /*set up doobell node*/ np = of_find_compatible_node(NULL, NULL, "fsl,srio-dbell-unit"); if (!np) { + dev_err(&dev->dev, "No fsl,srio-dbell-unit node\n"); rc = -ENODEV; goto err_dbell; } @@ -441,6 +444,7 @@ int fsl_rio_setup(struct platform_device *dev) /*set up port write node*/ np = of_find_compatible_node(NULL, NULL, "fsl,srio-port-write-unit"); if (!np) { + dev_err(&dev->dev, "No fsl,srio-port-write-unit node\n"); rc = -ENODEV; goto err_pw; } @@ -633,14 +637,18 @@ int fsl_rio_setup(struct platform_device *dev) return 0; err: kfree(pw); + pw = NULL; err_pw: kfree(dbell); + dbell = NULL; err_dbell: iounmap(rmu_regs_win); + rmu_regs_win = NULL; err_rmu: kfree(ops); err_ops: iounmap(rio_regs_win); + rio_regs_win = NULL; err_rio_regs: return rc; }
Several of the error paths from fsl_rio_setup are missing error messages. Worse, fsl_rio_setup initializes several global pointers and does not NULL them out after freeing/unmapping on error. This caused fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which was non-NULL but had been unmapped. Signed-off-by: Scott Wood <scottwood@freescale.com> Cc: Liu Gang <Gang.Liu@freescale.com> --- Liu Gang, are you sure all of these error conditions are fatal? Why does the rio driver fail if rmu is not present (e.g. on t4240)? --- arch/powerpc/sysdev/fsl_rio.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)