| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8D983423E7EDF846BB3056827B8CC5D14BC26E06@corpmail1.na.ads.idt.com>
Date: Thu, 25 Apr 2013 21:00:22 +0000
From: "Bounine, Alexandre" <Alexandre.Bounine@....com>
To: Andrew Morton <akpm@...ux-foundation.org>
CC: Micha Nelissen <micha.nelissen@...drive.nl>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andre van Herk <andre.van.herk@...drive.nl>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: RE: [PATCH 1/3] rapidio: make enumeration/discovery configurable
On Wednesday, April 24, 2013 5:37 PM Andrew Morton wrote:
>
> On Wed, 24 Apr 2013 10:31:57 -0400 Alexandre Bounine
> <alexandre.bounine@....com> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists