Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

From: Dan Carpenter
Date: Thu Apr 23 2015 - 11:18:52 EST

On Tue, Apr 21, 2015 at 07:22:34PM +0530, Sudip Mukherjee wrote:
> This is again another WIP for your review.
> almost all the points raised by Greg and Dan has been covered in this
> patch.
>
> apart from those points, I found another problem with my previous code,
> that whenever any driver is trying to register, it is getting bound to
> all the available parallel ports. To solve that probe was required and
> it has been used in this version. 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 the previous version Greg mentioned that we do not need to maintain
> our own list now. That has been partially done in this version, we are
> no longet 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.
>
> Just a little off-topic: I am using the staging/panel to test this code,
> and last few days i had a very tough time finding a stackdump. I was
> always thinking that it is happening because of the changes I have done.
> But it turned out to be a bug in the panel code. And incidentally, with
> the changes done in this version that bug is also resolved.
>
> Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> ---
> drivers/parport/procfs.c | 15 ++-
> drivers/parport/share.c | 243 ++++++++++++++++++++++++++++++++++++++++++++---
> include/linux/parport.h | 41 +++++++-
> 3 files changed, 283 insertions(+), 16 deletions(-)
>
> 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..8a28c54 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,6 +101,20 @@ static struct parport_operations dead_ops = {
> .owner = NULL,
> };
>
> +struct bus_type parport_bus_type = {
> + .name = "parport",
> +};

Can you make this static? The indenting is a bit off.

> +
> +int parport_bus_init(void)
> +{
> + return bus_register(&parport_bus_type);
> +}
> +
> +void parport_bus_exit(void)
> +{
> + bus_unregister(&parport_bus_type);
> +}
> +
> /* Call attach(port) for each registered driver. */
> static void attach_driver_chain(struct parport *port)
> {
> @@ -157,6 +172,7 @@ int parport_register_driver (struct parport_driver *drv)
>
> if (list_empty(&portlist))
> get_lowlevel_driver ();
> + drv->devmodel = false;
>
> mutex_lock(&registration_lock);
> list_for_each_entry(port, &portlist, list)
> @@ -167,6 +183,61 @@ int parport_register_driver (struct parport_driver *drv)
> return 0;
> }
>
> +/*
> + * iterates through all the devices connected to the bus and sends the device
> + * details to the check 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)->check(to_parport_dev(dev));
> + return 0;
> +}

The cast isn't beautiful. Do this:

static int port_check(struct device *dev, void *_drv)
{
struct parport_driver *drv = _drv;

drv->check(to_parport_dev(dev));
return 0;
}

What is the point of the check function really? The name isn't clear.

Since it always returns zero that means we loop through all the devices
and then returns NULL. It feels like a function called
bus_find_device() should find something. We have bus_for_each_dev() if
we just want to iterate.

> +
> +/*
> + * __parport_register_drv - register a new parport driver
> + * @drv: the driver structure to register
> + * @owner: owner module of drv
> + * @mod_name: module name string
> + *
> + * Adds the driver structure to the list of registered drivers.
> + * Returns a negative value on error, otherwise 0.
> + * If no error occurred, the driver remains registered even if
> + * no device was claimed during registration.
> + */
> +int __parport_register_drv(struct parport_driver *drv,
> + struct module *owner, const char *mod_name)
> +{
> + int ret;
> +
> + if (!drv->probe) {
> + pr_err("please use probe in %s\n", drv->name);
> + return -ENODEV;
> + }
> +
> + if (list_empty(&portlist))
> + get_lowlevel_driver();
> +
> + /* 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->driver.probe = drv->probe;
> + drv->devmodel = true;
> + ret = driver_register(&drv->driver);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&registration_lock);
> + bus_find_device(&parport_bus_type, NULL, drv, port_check);
> + mutex_unlock(&registration_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(__parport_register_drv);
> +
> /**
> * parport_unregister_driver - deregister a parallel port device driver
> * @drv: structure describing the driver that was given to
> @@ -189,15 +260,21 @@ void parport_unregister_driver (struct parport_driver *drv)
> struct parport *port;
>
> mutex_lock(&registration_lock);
> - list_del_init(&drv->list);
> - list_for_each_entry(port, &portlist, list)
> - drv->detach(port);
> + if (drv->devmodel) {
> + driver_unregister(&drv->driver);
> + } else {
> + list_del_init(&drv->list);
> + list_for_each_entry(port, &portlist, list)
> + drv->detach(port);
> + }
> mutex_unlock(&registration_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,8 +300,9 @@ 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->ddev);
> +
> + return to_parport_dev(dev);
> }
>
> /**
> @@ -237,11 +315,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->ddev);
> }
>
> /**
> @@ -281,6 +355,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 +408,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
> */
> sprintf(name, "parport%d", tmp->portnum = tmp->number);
> tmp->name = name;
> + tmp->ddev.bus = &parport_bus_type;
> + tmp->ddev.release = free_port;
> + dev_set_name(&tmp->ddev, name);
>
> for (device = 0; device < 5; device++)
> /* assume the worst */
> @@ -340,6 +418,12 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
>
> tmp->waithead = tmp->waittail = NULL;
>
> + ret = device_register(&tmp->ddev);
> + if (ret) {
> + put_device(&tmp->ddev);
> + return NULL;
> + }
> +
> return tmp;
> }
>
> @@ -575,6 +659,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 +715,133 @@ 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(struct parport *port, const char *name,

Please choose a better name like parport_register_dev_model() or
something.

> + struct pardev_cb *par_dev_cb)
> +{
> + struct pardevice *par_dev;
> + int ret;
> + char *devname;
> +
> + if (port->physport->flags & PARPORT_FLAG_EXCL) {
> + /* An exclusive device is registered. */
> + pr_debug("%s: no more devices allowed\n",
> + port->name);

This should probably be a pr_err().

> + 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;
> +
> + 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;

The existing code is buggy here as we discussed previously. Could you
just fix that before we do anything else? It's freaking me out.

> + 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;
> + par_dev->irq_func = par_dev_cb->irq_func;
> + par_dev->waiting = 0;
> + par_dev->timeout = 5 * HZ;
> +
> + par_dev->dev.parent = &port->ddev;
> + par_dev->dev.bus = &parport_bus_type;
> + dev_set_name(&par_dev->dev, "%s", devname);

dev_set_name() allocates a copy of "devname". It can fail with -ENOMEM.
It's not likely but we check all the other allocations so we may as
well check here.

> + par_dev->dev.release = free_pardevice;
> + par_dev->devmodel = true;
> + ret = device_register(&par_dev->dev);
> + if (ret)
> + goto err_free_all;

This is a bad name because as soon as you add a new allocation you will
have to change the name. Use something like 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_free_all;
> + }
> + 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_free_all:
> + put_device(&par_dev->dev);
> +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);
> +
> /**
> * parport_unregister_device - deregister a device on a parallel port
> * @dev: pointer to structure representing device
> @@ -691,7 +903,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 +1141,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;
> return r;
> }
>
> diff --git a/include/linux/parport.h b/include/linux/parport.h
> index c22f125..83e1a6e 100644
> --- a/include/linux/parport.h
> +++ b/include/linux/parport.h
> @@ -13,6 +13,7 @@
> #include <linux/wait.h>
> #include <linux/irqreturn.h>
> #include <linux/semaphore.h>
> +#include <linux/device.h>
> #include <asm/ptrace.h>
> #include <uapi/linux/parport.h>
>
> @@ -145,6 +146,8 @@ struct pardevice {
> unsigned int flags;
> struct pardevice *next;
> struct pardevice *prev;
> + struct device dev;
> + bool devmodel;
> struct parport_state *state; /* saved status over preemption */
> wait_queue_head_t wait_q;
> unsigned long int time;
> @@ -156,6 +159,8 @@ struct pardevice {
> void * sysctl_table;
> };
>
> +#define to_pardevice(n) container_of(n, struct pardevice, dev)
> +
> /* IEEE1284 information */
>
> /* IEEE1284 phases. These are exposed to userland through ppdev IOCTL
> @@ -195,7 +200,7 @@ struct parport {
> * This may unfortulately be null if the
> * port has a legacy driver.
> */
> -
> + struct device ddev; /* to link with the bus */

What does ddev stand for? Anyway, it's probably better to call it
bus_dev?


regards,
dan carpenter
--
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/