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

From: Sudip Mukherjee
Date: Tue Apr 21 2015 - 09:52:54 EST

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",
+};
+
+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;
+}
+
+/*
+ * __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,
+ 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);
+ 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;
+ 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);
+ par_dev->dev.release = free_pardevice;
+ par_dev->devmodel = true;
+ ret = device_register(&par_dev->dev);
+ if (ret)
+ goto err_free_all;
+
+ /* 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 */
struct parport *physport;
/* If this is a non-default mux
parport, i.e. we're a clone of a real
@@ -245,15 +250,26 @@ struct parport {
struct parport *slaves[3];
};

+#define to_parport_dev(n) container_of(n, struct parport, ddev)
+
#define DEFAULT_SPIN_TIME 500 /* us */

struct parport_driver {
const char *name;
void (*attach) (struct parport *);
void (*detach) (struct parport *);
+ void (*check)(struct parport *);
+ int (*probe)(struct device *);
+ struct device_driver driver;
+ bool devmodel;
struct list_head list;
};

+extern struct bus_type parport_bus_type;
+
+int parport_bus_init(void);
+void parport_bus_exit(void);
+
/* parport_register_port registers a new parallel port at the given
address (if one does not already exist) and returns a pointer to it.
This entails claiming the I/O region, IRQ and DMA. NULL is returned
@@ -274,8 +290,19 @@ extern void parport_remove_port(struct parport *port);
/* Register a new high-level driver. */
extern int parport_register_driver (struct parport_driver *);

+int __must_check __parport_register_drv(struct parport_driver *,
+ struct module *,
+ const char *mod_name);
+/*
+ * parport_register_drv must be a macro so that KBUILD_MODNAME can
+ * be expanded
+ */
+#define parport_register_drv(driver) \
+ __parport_register_drv(driver, THIS_MODULE, KBUILD_MODNAME)
+
/* Unregister a high-level driver. */
extern void parport_unregister_driver (struct parport_driver *);
+void parport_unregister_driver(struct parport_driver *);

/* If parport_register_driver doesn't fit your needs, perhaps
* parport_find_xxx does. */
@@ -289,6 +316,14 @@ extern irqreturn_t parport_irq_handler(int irq, void *dev_id);
extern struct parport *parport_get_port (struct parport *);
extern void parport_put_port (struct parport *);

+struct pardev_cb {
+ int (*preempt)(void *);
+ void (*wakeup)(void *);
+ void *private;
+ void (*irq_func)(void *);
+ unsigned int flags;
+};
+
/* parport_register_device declares that a device is connected to a
port, and tells the kernel all it needs to know.
- pf is the preemption function (may be NULL for no callback)
@@ -301,6 +336,10 @@ struct pardevice *parport_register_device(struct parport *port,
void (*irq_func)(void *),
int flags, void *handle);

+struct pardevice *
+parport_register_dev(struct parport *port, const char *name,
+ struct pardev_cb *par_dev_cb);
+
/* parport_unregister unlinks a device from the chain. */
extern void parport_unregister_device(struct pardevice *dev);

--
1.8.1.2

--
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/