RE: [PATCH 1/3] rapidio: make enumeration/discovery configurable

[Bounine, Alexandre]

On Wednesday, April 24, 2013 5:37 PM Andrew Morton wrote:
> 
> On Wed, 24 Apr 2013 10:31:57 -0400 Alexandre Bounine
> <alexandre.bounine@xxxxxxx> wrote:
> 
> > Rework to implement RapidIO enumeration/discovery method selection
> > combined with ability to use enumeration/discovery as a kernel module.
> >
> > ...
> >
> > @@ -1421,3 +1295,46 @@ enum_done:
> >  bail:
> >  	return -EBUSY;
> >  }
> > +
> > +struct rio_scan rio_scan_ops = {
> > +	.enumerate = rio_enum_mport,
> > +	.discover = rio_disc_mport,
> > +};
> > +
> > +
> > +#ifdef MODULE
> 
> Why the `ifdef MODULE'?  The module parameters are still accessible if
> the driver is statically linked and we do want the driver to behave in
> the same way regardless of how it was linked and loaded.

Marked for review.

> > +static bool scan;
> > +module_param(scan, bool, 0);
> > +MODULE_PARM_DESC(scan, "Start RapidIO network enumeration/discovery
> "
> > +			"(default = 1)");
> > +
> > +/**
> > + * rio_basic_attach:
> > + *
> > + * When this enumeration/discovery method is loaded as a module this function
> > + * registers its specific enumeration and discover routines for all available
> > + * RapidIO mport devices. The "scan" command line parameter controls ability of
> > + * the module to start RapidIO enumeration/discovery automatically.
> > + *
> > + * Returns 0 for success or -EIO if unable to register itself.
> > + *
> > + * This enumeration/discovery method cannot be unloaded and therefore does not
> > + * provide a matching cleanup_module routine.
> > + */
> > +
> > +int __init rio_basic_attach(void)
> 
> static

My miss. Will fix.

> > +{
> > +	if (rio_register_scan(RIO_MPORT_ANY, &rio_scan_ops))
> > +		return -EIO;
> > +	if (scan)
> > +		rio_init_mports();
> > +	return 0;
> > +}
> > +
> > +module_init(rio_basic_attach);
> > +
> > +MODULE_DESCRIPTION("Basic RapidIO enumeration/discovery");
> > +MODULE_LICENSE("GPL");
> > +
> > +#endif /* MODULE */
> > diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
> > index d553b5d..e36628a 100644
> > --- a/drivers/rapidio/rio.c
> > +++ b/drivers/rapidio/rio.c
> > @@ -31,6 +31,9 @@
> >
> >  #include "rio.h"
> >
> > +LIST_HEAD(rio_devices);
> 
> static?
> 
> > +DEFINE_SPINLOCK(rio_global_list_lock);
> 
> static?

These two have been very tempting for me to make them static.
But because a goal was simple code move and they have been visible
from very beginning of RapidIO code, I decided to keep the scope "as is".
While I am against using the device list directly, I am not sure that this
patch is a good place to change the existing scope.

Because keeping them global prompted your comment I will happily make them
static and see if anyone will complain about it.
 
> > +
> >  static LIST_HEAD(rio_mports);
> >  static unsigned char next_portid;
> >  static DEFINE_SPINLOCK(rio_mmap_lock);
> >
> > ...
> >
> > +/**
> > + * rio_switch_init - Sets switch operations for a particular vendor switch
> > + * @rdev: RIO device
> > + * @do_enum: Enumeration/Discovery mode flag
> > + *
> > + * Searches the RIO switch ops table for known switch types. If the vid
> > + * and did match a switch table entry, then call switch initialization
> > + * routine to setup switch-specific routines.
> > + */
> > +void rio_switch_init(struct rio_dev *rdev, int do_enum)
> > +{
> > +	struct rio_switch_ops *cur = __start_rio_switch_ops;
> > +	struct rio_switch_ops *end = __end_rio_switch_ops;
> 
> huh, I hadn't noticed that RIO has its very own vmlinux section.  How
> peculair.

Yes it is there (since 2.6.15). We will address it at some point later.
At this moment just moving the code from one file to another.
 
> > +	while (cur < end) {
> > +		if ((cur->vid == rdev->vid) && (cur->did == rdev->did)) {
> > +			pr_debug("RIO: calling init routine for %s\n",
> > +				 rio_name(rdev));
> > +			cur->init_hook(rdev, do_enum);
> > +			break;
> > +		}
> > +		cur++;
> > +	}
> > +
> > +	if ((cur >= end) && (rdev->pef & RIO_PEF_STD_RT)) {
> > +		pr_debug("RIO: adding STD routing ops for %s\n",
> > +			rio_name(rdev));
> > +		rdev->rswitch->add_entry = rio_std_route_add_entry;
> > +		rdev->rswitch->get_entry = rio_std_route_get_entry;
> > +		rdev->rswitch->clr_table = rio_std_route_clr_table;
> > +	}
> > +
> > +	if (!rdev->rswitch->add_entry || !rdev->rswitch->get_entry)
> > +		printk(KERN_ERR "RIO: missing routing ops for %s\n",
> > +		       rio_name(rdev));
> > +}
> > +EXPORT_SYMBOL_GPL(rio_switch_init);
> >
> > ...
> >
> > +int rio_register_scan(int mport_id, struct rio_scan *scan_ops)
> > +{
> > +	struct rio_mport *port;
> > +	int rc = -EBUSY;
> > +
> > +	list_for_each_entry(port, &rio_mports, node) {
> 
> How come the driver has no locking for rio_mports?  If a bugfix isn't
> needed here then a code comment is!

Locking is not needed at this moment, but has to be added sooner or later anyway.
I will add it now to avoid fixing it later. 
 
> > +		if (port->id == mport_id || mport_id == RIO_MPORT_ANY) {
> > +			if (port->nscan && mport_id == RIO_MPORT_ANY)
> > +				continue;
> > +			else if (port->nscan)
> > +				break;
> > +
> > +			port->nscan = scan_ops;
> > +			rc = 0;
> > +
> > +			if (mport_id != RIO_MPORT_ANY)
> > +				break;
> > +		}
> > +	}
> > +
> > +	return rc;
> > +}
> >
> > ...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/