| Message ID | 20170529212142.25572-2-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Borislav, On 30/05/17 09:21, Chris Packham wrote: > Check the return status of platform_driver_register() in > mv64x60_edac_init(). Only output messages and initialise the > edac_op_state if the registration is successful. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > Changes in v3: > - catch the retval of platform_register_drivers and return early on failure > (thanks Borislav). > > drivers/edac/mv64x60_edac.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c > index 14b7e7b71eaa..172081551a70 100644 > --- a/drivers/edac/mv64x60_edac.c > +++ b/drivers/edac/mv64x60_edac.c > @@ -853,7 +853,11 @@ static struct platform_driver * const drivers[] = { > > static int __init mv64x60_edac_init(void) > { > - int ret = 0; > + int ret; > + > + ret = platform_register_drivers(drivers, ARRAY_SIZE(drivers)); > + if (ret) > + return ret; I actually think this was wrong. The edac_op_state is set as a module param and affects the behaviour of the driver probe function which on success could be called before we sanity check it below. I'm back to thinking that my original v1 of just removing the unused ret was appropriate. If we still consider the driver too noisy we could just remove the printks. What do you think? > > printk(KERN_INFO "Marvell MV64x60 EDAC driver " MV64x60_REVISION "\n"); > printk(KERN_INFO "\t(C) 2006-2007 MontaVista Software\n"); > @@ -867,7 +871,7 @@ static int __init mv64x60_edac_init(void) > break; > } > > - return platform_register_drivers(drivers, ARRAY_SIZE(drivers)); > + return 0; > } > module_init(mv64x60_edac_init); > >
On Wed, Jun 07, 2017 at 05:29:55AM +0000, Chris Packham wrote:
> What do you think?
Just send me a fix ontop, moving the edac_op_state assignment before the
platform_register_drivers() call and write in the commit message why
we're doing that.
Thanks.
diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c index 14b7e7b71eaa..172081551a70 100644 --- a/drivers/edac/mv64x60_edac.c +++ b/drivers/edac/mv64x60_edac.c @@ -853,7 +853,11 @@ static struct platform_driver * const drivers[] = { static int __init mv64x60_edac_init(void) { - int ret = 0; + int ret; + + ret = platform_register_drivers(drivers, ARRAY_SIZE(drivers)); + if (ret) + return ret; printk(KERN_INFO "Marvell MV64x60 EDAC driver " MV64x60_REVISION "\n"); printk(KERN_INFO "\t(C) 2006-2007 MontaVista Software\n"); @@ -867,7 +871,7 @@ static int __init mv64x60_edac_init(void) break; } - return platform_register_drivers(drivers, ARRAY_SIZE(drivers)); + return 0; } module_init(mv64x60_edac_init);
Check the return status of platform_driver_register() in mv64x60_edac_init(). Only output messages and initialise the edac_op_state if the registration is successful. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Changes in v3: - catch the retval of platform_register_drivers and return early on failure (thanks Borislav). drivers/edac/mv64x60_edac.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)