Re: [PATCH v3 04/30] staging: unisys: visorbus: remove unused module parameters

[Neil Horman]

On Wed, Jun 08, 2016 at 02:13:47AM +0000, Binder, David Anthony wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@xxxxxxxxxx]
> > Sent: Tuesday, June 07, 2016 9:23 AM
> > To: Kershner, David A <David.Kershner@xxxxxxxxxx>
> > Cc: corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
> > hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Arfvidson, Erik
> > <Erik.Arfvidson@xxxxxxxxxx>; Sell, Timothy C <Timothy.Sell@xxxxxxxxxx>;
> > hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; jes.sorensen@xxxxxxxxxx; Curtin,
> > Alexander Paul <Alexander.Curtin@xxxxxxxxxx>;
> > janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx;
> > prarit@xxxxxxxxxx; Binder, David Anthony <David.Binder@xxxxxxxxxx>;
> > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par-
> > Maintainer <SParMaintainer@xxxxxxxxxx>
> > Subject: Re: [PATCH v3 04/30] staging: unisys: visorbus: remove unused
> > module parameters
> > 
> > On Sat, Jun 04, 2016 at 01:27:04PM -0400, David Kershner wrote:
> > > From: David Binder <david.binder@xxxxxxxxxx>
> > >
> > > Removes unused module parameters from visorbus_main.c, in response to
> > > findings by SonarQube.
> > >
> > > Signed-off-by: David Binder <david.binder@xxxxxxxxxx>
> > > Signed-off-by: David Kershner <david.kershner@xxxxxxxxxx>
> > > Reviewed-by: Tim Sell <Timothy.Sell@xxxxxxxxxx>
> > > ---
> > >  drivers/staging/unisys/visorbus/visorbus_main.c | 9 +--------
> > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > index 2ed9628..71bff07 100644
> > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > @@ -27,10 +27,9 @@
> > >  #define MYDRVNAME "visorbus"
> > >
> > >  /* module parameters */
> > > -static int visorbus_debug;
> > >  static int visorbus_forcematch;
> > >  static int visorbus_forcenomatch;
> > > -static int visorbus_debugref;
> > > +
> > >  #define SERIALLOOPBACKCHANADDR (100 * 1024 * 1024)
> > >
> > >  /* Display string that is guaranteed to be no longer the 99 characters*/
> > > @@ -1332,9 +1331,6 @@ visorbus_exit(void)
> > >  	remove_bus_type();
> > >  }
> > >
> > > -module_param_named(debug, visorbus_debug, int, S_IRUGO);
> > > -MODULE_PARM_DESC(visorbus_debug, "1 to debug");
> > > -
> > >  module_param_named(forcematch, visorbus_forcematch, int, S_IRUGO);
> > >  MODULE_PARM_DESC(visorbus_forcematch,
> > >  		 "1 to force a successful dev <--> drv match");
> > > @@ -1342,6 +1338,3 @@ MODULE_PARM_DESC(visorbus_forcematch,
> > >  module_param_named(forcenomatch, visorbus_forcenomatch, int,
> > S_IRUGO);
> > >  MODULE_PARM_DESC(visorbus_forcenomatch,
> > >  		 "1 to force an UNsuccessful dev <--> drv match");
> > > -
> > > -module_param_named(debugref, visorbus_debugref, int, S_IRUGO);
> > > -MODULE_PARM_DESC(visorbus_debugref, "1 to debug reference
> > counting");
> > 
> > visorbus_forcematch and visorbus_forcenomatch appear to be referenced in
> > visorbus_match (at least in the HEAD of linus' tree).  If you're going to
> > remove
> > the module parameters, why not also remove those references and the
> > force[no]match variables themselves?
> > 
> > Neil
> 
> We presently use visorbus_forcematch and visorbus_forcenomatch for
> debugging purposes, and therefore decided to keep the variables in
> the driver.
> 
Ok, thats fine, but this seems like a half measure then.  This patch removes the
module option for those features, which means that for you to use them, you have
to rebuild the module.  If you have intentions to use it, why not just leave the
module option in place?  If you don't, remove it all the way.

Neil

> David Binder