On Wed, May 06, 2015 at 03:46:13PM +0530, Sudip Mukherjee wrote:
> parport subsystem starts using the device-model. drivers using the
> device-model has to define probe and should register the device with
> parport with parport_register_dev_model()
>
> Signed-off-by: Sudip Mukherjee <su...@vectorindia.org>
> ---
>
> v5:
> a) addition/removal of ports are now handled.
> b) is_parport moved to the core level
> c) common parport_driver_register code
> d) parport_register_dev_model now accepts instance number.
>
> v4: use of is_parport() is introduced to check the type of device
> that has been passed to probe or match_port.
>
> v3: started use of parport_del_port(). previously it was creating
> some ghost parallel ports during port probing.
> parport_del_port() removes registered ports if probing has
> failed.
>
> v2: started using probe function. Without probe,whenever any driver
> is trying to register, it is getting bound to all the available
> parallel ports. To solve that probe was required.
> Now the driver is binding only to the device it has registered.
> And that device will appear as a subdevice of the particular
> parallel port it wants to use.
>
> In v1 Greg mentioned that we do not need to maintain our own
> list. That has been partially done. we are no longer maintaining
> the list of drivers. But we still need to maintain the list of
> ports and devices as that will be used by drivers which are not
> yet converted to device model. When all drivers are converted
> to use the device-model parallel port all these lists can be
> removed.
>
> drivers/parport/parport_pc.c | 4 +-
> drivers/parport/procfs.c | 15 +-
> drivers/parport/share.c | 335
> +++++++++++++++++++++++++++++++++++++++----
> include/linux/parport.h | 43 +++++-
> 4 files changed, 368 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
> index 53d15b3..78530d1 100644
> --- a/drivers/parport/parport_pc.c
> +++ b/drivers/parport/parport_pc.c
> @@ -2255,7 +2255,7 @@ out5:
> release_region(base+0x3, 5);
> release_region(base, 3);
> out4:
> - parport_put_port(p);
> + parport_del_port(p);
> out3:
> kfree(priv);
> out2:
> @@ -2294,7 +2294,7 @@ void parport_pc_unregister_port(struct parport *p)
> priv->dma_handle);
> #endif
> kfree(p->private_data);
> - parport_put_port(p);
> + parport_del_port(p);
> kfree(ops); /* hope no-one cached it */
> }
> EXPORT_SYMBOL(parport_pc_unregister_port);
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 3b47080..c776333 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -21,6 +21,7 @@
> #include <linux/parport.h>
> #include <linux/ctype.h>
> #include <linux/sysctl.h>
> +#include <linux/device.h>
>
> #include <asm/uaccess.h>
>
> @@ -558,8 +559,18 @@ int parport_device_proc_unregister(struct pardevice
> *device)
>
> static int __init parport_default_proc_register(void)
> {
> + int ret;
> +
> parport_default_sysctl_table.sysctl_header =
> register_sysctl_table(parport_default_sysctl_table.dev_dir);
> + if (!parport_default_sysctl_table.sysctl_header)
> + return -ENOMEM;
> + ret = parport_bus_init();
> + if (ret) {
> + unregister_sysctl_table(parport_default_sysctl_table.
> + sysctl_header);
> + return ret;
> + }
> return 0;
> }
>
> @@ -570,6 +581,7 @@ static void __exit parport_default_proc_unregister(void)
> sysctl_header);
> parport_default_sysctl_table.sysctl_header = NULL;
> }
> + parport_bus_exit();
> }
>
> #else /* no sysctl or no procfs*/
> @@ -596,11 +608,12 @@ int parport_device_proc_unregister(struct pardevice
> *device)
>
> static int __init parport_default_proc_register (void)
> {
> - return 0;
> + return parport_bus_init();
> }
>
> static void __exit parport_default_proc_unregister (void)
> {
> + parport_bus_exit();
> }
> #endif
>
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index 3fa6624..eb5d91d 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -29,6 +29,7 @@
> #include <linux/slab.h>
> #include <linux/sched.h>
> #include <linux/kmod.h>
> +#include <linux/device.h>
>
> #include <linux/spinlock.h>
> #include <linux/mutex.h>
> @@ -100,13 +101,79 @@ static struct parport_operations dead_ops = {
> .owner = NULL,
> };
>
> +static struct device_type parport_device_type = {
> + .name = "parport",
> +};
> +
> +static int is_parport(struct device *dev)
> +{
> + return dev->type == &parport_device_type;
> +}
> +
> +static int parport_probe(struct device *dev)
> +{
> + struct parport_driver *drv;
> +
> + if (is_parport(dev))
> + return -ENODEV;
Is this test reversed? You guys have tested probing though so it
doesn't seem like it can be reversed.
> +
> + drv = to_parport_driver(dev->driver);
> + if (!drv->probe)
> + return -ENODEV;
> +
> + return drv->probe(to_pardevice(dev));
> +}
> +
> +static struct bus_type parport_bus_type = {
> + .name = "parport",
> + .probe = parport_probe,
> +};
> +
> +int parport_bus_init(void)
> +{
> + return bus_register(&parport_bus_type);
> +}
> +
> +void parport_bus_exit(void)
> +{
> + bus_unregister(&parport_bus_type);
> +}
> +
> +static int driver_check(struct device_driver *_drv, void *_port)
_drv is a poor name. _port is a good name because it's actually port,
but _drv should be dev_drv or something. The port_check() function has
a nice comment explaining why it exists can you add one here?
> +{
> + struct parport *port = _port;
> + struct parport_driver *drv = to_parport_driver(_drv);
> +
> + if (drv->match_port)
> + drv->match_port(port);
> + return 0;
> +}
> +
> /* Call attach(port) for each registered driver. */
> static void attach_driver_chain(struct parport *port)
> {
> /* caller has exclusive registration_lock */
> struct parport_driver *drv;
> +
> list_for_each_entry(drv, &drivers, list)
> drv->attach(port);
> +
> + /*
> + * call the port_check function of the drivers registered in
s/port_check/driver_check/
> + * new device model
> + */
> +
> + bus_for_each_drv(&parport_bus_type, NULL, port, driver_check);
> +}
> +
> +static int driver_detach(struct device_driver *_drv, void *_port)
> +{
> + struct parport *port = _port;
> + struct parport_driver *drv = to_parport_driver(_drv);
> +
> + if (drv->detach)
> + drv->detach(port);
> + return 0;
> }
>
> /* Call detach(port) for each registered driver. */
> @@ -116,6 +183,13 @@ static void detach_driver_chain(struct parport *port)
> /* caller has exclusive registration_lock */
> list_for_each_entry(drv, &drivers, list)
> drv->detach (port);
> +
> + /*
> + * call the detach function of the drivers registered in
> + * new device model
> + */
> +
> + bus_for_each_drv(&parport_bus_type, NULL, port, driver_detach);
> }
>
> /* Ask kmod for some lowlevel drivers. */
> @@ -126,17 +200,39 @@ static void get_lowlevel_driver (void)
> request_module ("parport_lowlevel");
> }
>
> +/*
> + * iterates through all the devices connected to the bus and sends the device
> + * details to the match_port callback of the driver, so that the driver can
> + * know what are all the ports that are connected to the bus and choose the
> + * port to which it wants to register its device.
> + */
> +static int port_check(struct device *dev, void *_drv)
> +{
> + struct parport_driver *drv = _drv;
> +
> + /* only send ports, do not send other devices connected to bus */
> + if (is_parport(dev))
> + drv->match_port(to_parport_dev(dev));
> + return 0;
> +}
> +
> /**
> * parport_register_driver - register a parallel port device driver
> * @drv: structure describing the driver
> + * @owner: owner module of drv
> + * @mod_name: module name string
> *
> * This can be called by a parallel port device driver in order
> * to receive notifications about ports being found in the
> * system, as well as ports no longer available.
> *
> + * If the probe function is defined then the new device model is used
> + * for registration.
> + *
> * The @drv structure is allocated by the caller and must not be
> * deallocated until after calling parport_unregister_driver().
> *
> + * If using the non device model:
> * The driver's attach() function may block. The port that
> * attach() is given will be valid for the duration of the
> * callback, but if the driver wants to take a copy of the
> @@ -148,21 +244,58 @@ static void get_lowlevel_driver (void)
> * callback, but if the driver wants to take a copy of the
> * pointer it must call parport_get_port() to do so.
> *
> - * Returns 0 on success. Currently it always succeeds.
> + *
> + * Returns 0 on success. The non device model will always succeeds.
> + * but the new device model can fail and will return the error code.
> **/
>
> -int parport_register_driver (struct parport_driver *drv)
> +int __parport_register_driver(struct parport_driver *drv, struct module
> *owner,
> + const char *mod_name)
> {
> - struct parport *port;
> -
> if (list_empty(&portlist))
> get_lowlevel_driver ();
>
> - mutex_lock(®istration_lock);
> - list_for_each_entry(port, &portlist, list)
> - drv->attach(port);
> - list_add(&drv->list, &drivers);
> - mutex_unlock(®istration_lock);
> + if (drv->probe) {
I feel like this is weird. The driver should just set ->devmodel
itself and we test for it here.
> + /* using device model */
> + int ret;
> +
> + /* initialize common driver fields */
> + drv->driver.name = drv->name;
> + drv->driver.bus = &parport_bus_type;
> + drv->driver.owner = owner;
> + drv->driver.mod_name = mod_name;
> + drv->devmodel = true;
> + ret = driver_register(&drv->driver);
> + if (ret)
> + return ret;
> +
> + mutex_lock(®istration_lock);
> + if (drv->match_port)
> + bus_for_each_dev(&parport_bus_type, NULL, drv,
> + port_check);
> + mutex_unlock(®istration_lock);
> + } else {
> + struct parport *port;
> +
> + drv->devmodel = false;
> +
> + mutex_lock(®istration_lock);
> + list_for_each_entry(port, &portlist, list)
> + drv->attach(port);
> + list_add(&drv->list, &drivers);
> + mutex_unlock(®istration_lock);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(__parport_register_driver);
> +
> +static int port_detach(struct device *dev, void *_drv)
> +{
> + struct parport_driver *drv = _drv;
> +
> + if (is_parport(dev) && drv->detach)
> + drv->detach(to_parport_dev(dev));
>
> return 0;
> }
> @@ -189,15 +322,22 @@ void parport_unregister_driver (struct parport_driver
> *drv)
> struct parport *port;
>
> mutex_lock(®istration_lock);
> - list_del_init(&drv->list);
> - list_for_each_entry(port, &portlist, list)
> - drv->detach(port);
> + if (drv->devmodel) {
> + bus_for_each_dev(&parport_bus_type, NULL, drv, port_detach);
> + driver_unregister(&drv->driver);
> + } else {
> + list_del_init(&drv->list);
> + list_for_each_entry(port, &portlist, list)
> + drv->detach(port);
> + }
> mutex_unlock(®istration_lock);
> }
>
> -static void free_port (struct parport *port)
> +static void free_port(struct device *dev)
> {
> int d;
> + struct parport *port = to_parport_dev(dev);
> +
> spin_lock(&full_list_lock);
> list_del(&port->full_list);
> spin_unlock(&full_list_lock);
> @@ -223,9 +363,16 @@ static void free_port (struct parport *port)
>
> struct parport *parport_get_port (struct parport *port)
> {
> - atomic_inc (&port->ref_count);
> - return port;
> + struct device *dev = get_device(&port->bus_dev);
> +
> + return to_parport_dev(dev);
> +}
> +
> +void parport_del_port(struct parport *port)
> +{
> + device_unregister(&port->bus_dev);
> }
> +EXPORT_SYMBOL(parport_del_port);
>
> /**
> * parport_put_port - decrement a port's reference count
> @@ -237,11 +384,7 @@ struct parport *parport_get_port (struct parport *port)
>
> void parport_put_port (struct parport *port)
> {
> - if (atomic_dec_and_test (&port->ref_count))
> - /* Can destroy it now. */
> - free_port (port);
> -
> - return;
> + put_device(&port->bus_dev);
I don't understand this. The comment is out of date now. Part of the
issue I'm having is that we need to support both old and new models for
now and I just get confused. This seems like new model stuff but then
what happened to the old model stuff.
Also I seem to remember that the old model stuff was buggy here? Is
that it? If this is a bug fix then send that separately so we can
merge it right away.
Anyway, it's not your fault I'm confused. But please, if it's a bugfix
send that by itself and also update the comment.
> }
>
> /**
> @@ -281,6 +424,7 @@ struct parport *parport_register_port(unsigned long base,
> int irq, int dma,
> int num;
> int device;
> char *name;
> + int ret;
>
> tmp = kzalloc(sizeof(struct parport), GFP_KERNEL);
> if (!tmp) {
> @@ -333,6 +477,10 @@ struct parport *parport_register_port(unsigned long
> base, int irq, int dma,
> */
> sprintf(name, "parport%d", tmp->portnum = tmp->number);
> tmp->name = name;
> + tmp->bus_dev.bus = &parport_bus_type;
> + tmp->bus_dev.release = free_port;
> + dev_set_name(&tmp->bus_dev, name);
> + tmp->bus_dev.type = &parport_device_type;
>
> for (device = 0; device < 5; device++)
> /* assume the worst */
> @@ -340,6 +488,12 @@ struct parport *parport_register_port(unsigned long
> base, int irq, int dma,
>
> tmp->waithead = tmp->waittail = NULL;
>
> + ret = device_register(&tmp->bus_dev);
> + if (ret) {
> + put_device(&tmp->bus_dev);
> + return NULL;
> + }
> +
> return tmp;
> }
>
> @@ -575,6 +729,7 @@ parport_register_device(struct parport *port, const char
> *name,
> tmp->irq_func = irq_func;
> tmp->waiting = 0;
> tmp->timeout = 5 * HZ;
> + tmp->devmodel = false;
>
> /* Chain this onto the list */
> tmp->prev = NULL;
> @@ -630,6 +785,136 @@ parport_register_device(struct parport *port, const
> char *name,
> return NULL;
> }
>
> +static void free_pardevice(struct device *dev)
> +{
> + struct pardevice *par_dev = to_pardevice(dev);
> +
> + kfree(par_dev->name);
> + kfree(par_dev);
> +}
> +
> +struct pardevice *
> +parport_register_dev_model(struct parport *port, const char *name,
> + struct pardev_cb *par_dev_cb, int cnt)
> +{
> + struct pardevice *par_dev;
> + int ret;
> + char *devname;
> +
> + if (port->physport->flags & PARPORT_FLAG_EXCL) {
> + /* An exclusive device is registered. */
> + pr_err("%s: no more devices allowed\n", port->name);
> + return NULL;
> + }
> +
> + if (par_dev_cb->flags & PARPORT_DEV_LURK) {
> + if (!par_dev_cb->preempt || !par_dev_cb->wakeup) {
> + pr_info("%s: refused to register lurking device (%s)
> without callbacks\n",
> + port->name, name);
> + return NULL;
> + }
> + }
> +
> + if (!try_module_get(port->ops->owner))
> + return NULL;
> +
> + parport_get_port(port);
> +
> + par_dev = kzalloc(sizeof(*par_dev), GFP_KERNEL);
> + if (!par_dev)
> + goto err_par_dev;
Could you rename this to err_put_port? Sorry, for being nit-picky but
naming labels after the goto location is a pet peeve of mine.
> +
> + par_dev->state = kzalloc(sizeof(*par_dev->state), GFP_KERNEL);
> + if (!par_dev->state)
> + goto err_put_par_dev;
> +
> + devname = kstrdup(name, GFP_KERNEL);
> + if (!devname)
> + goto err_free_par_dev;
> +
> + par_dev->name = devname;
> + par_dev->port = port;
> + par_dev->daisy = -1;
> + par_dev->preempt = par_dev_cb->preempt;
> + par_dev->wakeup = par_dev_cb->wakeup;
> + par_dev->private = par_dev_cb->private;
> + par_dev->flags = par_dev_cb->flags;
We have a par_dev->flags, par_dev->port->flags, and
par_dev->port->physport->flags. The are slightly different. Do we need
all three?
> + par_dev->irq_func = par_dev_cb->irq_func;
> + par_dev->waiting = 0;
> + par_dev->timeout = 5 * HZ;
> +
> + par_dev->dev.parent = &port->bus_dev;
> + par_dev->dev.bus = &parport_bus_type;
> + ret = dev_set_name(&par_dev->dev, "%s.%d", devname, cnt);
> + if (ret)
> + goto err_free_devname;
> + par_dev->dev.release = free_pardevice;
> + par_dev->devmodel = true;
> + ret = device_register(&par_dev->dev);
> + if (ret)
> + goto err_put_dev;
> +
> + /* Chain this onto the list */
> + par_dev->prev = NULL;
> + /*
> + * This function must not run from an irq handler so we don' t need
> + * to clear irq on the local CPU. -arca
> + */
> + spin_lock(&port->physport->pardevice_lock);
> +
> + if (par_dev_cb->flags & PARPORT_DEV_EXCL) {
> + if (port->physport->devices) {
> + spin_unlock(&port->physport->pardevice_lock);
> + pr_debug("%s: cannot grant exclusive access for device
> %s\n",
> + port->name, name);
> + goto err_put_dev;
> + }
> + port->flags |= PARPORT_FLAG_EXCL;
> + }
> +
> + par_dev->next = port->physport->devices;
> + wmb(); /*
> + * Make sure that tmp->next is written before it's
> + * added to the list; see comments marked 'no locking
> + * required'
> + */
> + if (port->physport->devices)
> + port->physport->devices->prev = par_dev;
> + port->physport->devices = par_dev;
> + spin_unlock(&port->physport->pardevice_lock);
> +
> + init_waitqueue_head(&par_dev->wait_q);
> + par_dev->timeslice = parport_default_timeslice;
> + par_dev->waitnext = NULL;
> + par_dev->waitprev = NULL;
> +
> + /*
> + * This has to be run as last thing since init_state may need other
> + * pardevice fields. -arca
> + */
> + port->ops->init_state(par_dev, par_dev->state);
> + port->proc_device = par_dev;
> + parport_device_proc_register(par_dev);
> +
> + return par_dev;
> +
> +err_put_dev:
> + put_device(&par_dev->dev);
> +err_free_devname:
> + kfree(devname);
> +err_free_par_dev:
> + kfree(par_dev->state);
> +err_put_par_dev:
> + if (!par_dev->devmodel)
> + kfree(par_dev);
> +err_par_dev:
> + parport_put_port(port);
> + module_put(port->ops->owner);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL(parport_register_dev_model);
> +
> /**
> * parport_unregister_device - deregister a device on a parallel port
> * @dev: pointer to structure representing device
> @@ -691,7 +976,10 @@ void parport_unregister_device(struct pardevice *dev)
> spin_unlock_irq(&port->waitlist_lock);
>
> kfree(dev->state);
> - kfree(dev);
> + if (dev->devmodel)
> + device_unregister(&dev->dev);
> + else
> + kfree(dev);
>
> module_put(port->ops->owner);
> parport_put_port (port);
> @@ -926,8 +1214,8 @@ int parport_claim_or_block(struct pardevice *dev)
> dev->port->physport->cad ?
> dev->port->physport->cad->name:"nobody");
> #endif
> - }
> - dev->waiting = 0;
> + } else if (r == 0)
> + dev->waiting = 0;
What is this change for?
> return r;
> }
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
