lkml.org 
[lkml]   [2015]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH WIP] parport: add device model
On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote:
> This is work-in-progree, not for applying to any tree. Posting now for
> your comments so that I know if I am in the proper track.
>
> in parport_register_driver() driver is registered but i am not linking
> anywhere the device with the driver, but yet when I am testing this
> patch I am seeing in sys tree that parport0 is linked with
> the lp driver. Is it done in the device core? I am missing this step
> somewhere.
>
> In parport_claim() the attach is unchecked as of now, I think we will
> need my initial patch series of monitoring the attach return value along
> with it.
>
> while testing I am getting NULL dereference with daisy.c, and after
> disabling PARPORT_1284 , I am getting some new errors. so if you are
> testing this patch please keep in mind that still lots of work is
> pending.
> My main intention to post it now is to know if my approach is correct.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> drivers/parport/procfs.c | 12 ++++++--
> drivers/parport/share.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/parport.h | 21 +++++++++++++-
> 3 files changed, 99 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 3b47080..1c49540 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -558,9 +558,15 @@ 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);

Should we return an error if this fails?

> - return 0;
> + ret = parport_bus_init();
> + if (ret)
> + unregister_sysctl_table(parport_default_sysctl_table.
> + sysctl_header);


ret = parport_bus_init();
if (ret) {
unregister_sysctl_table(
parport_default_sysctl_table.sysctl_header);
return ret;
}

return 0;


> + return ret;
> }
>
> static void __exit parport_default_proc_unregister(void)
> @@ -570,6 +576,7 @@ static void __exit parport_default_proc_unregister(void)
> sysctl_header);
> parport_default_sysctl_table.sysctl_header = NULL;
> }
> + parport_bus_exit();

Do we need this function? Can't we call bus_unregister() directly?



> }
>
> #else /* no sysctl or no procfs*/
> @@ -596,11 +603,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..042863a 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,33 @@ static struct parport_operations dead_ops = {
> .owner = NULL,
> };
>
> +/*
> + * This currently matches any parport driver to any parport device
> + * drivers themselves make the decision whether to drive this device
> + * in their probe method.
> + */
> +
> +static int parport_match(struct device *dev, struct device_driver *drv)
> +{
> + return 1;
> +}
> +
> +struct bus_type parport_bus_type = {
> + .name = "parport",
> + .match = parport_match,

There is no need for a match function. If it's NULL that's the same a
"return 1" fuction. This is called from driver_match_device().

> +};
> +EXPORT_SYMBOL(parport_bus_type);
> +
> +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)
> {
> @@ -151,9 +179,11 @@ static void get_lowlevel_driver (void)
> * Returns 0 on success. Currently it always succeeds.
> **/
>
> -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;
> + int ret;
>
> if (list_empty(&portlist))
> get_lowlevel_driver ();
> @@ -164,7 +194,22 @@ int parport_register_driver (struct parport_driver *drv)
> list_add(&drv->list, &drivers);
> mutex_unlock(&registration_lock);
>
> - return 0;
> + drv->driver.name = drv->name;
> + drv->driver.bus = &parport_bus_type;
> + drv->driver.owner = owner;
> + drv->driver.mod_name = mod_name;
> +
> + /* register with core */
> + ret = driver_register(&drv->driver);
> + if (ret < 0) {

if (ret) {

> + mutex_lock(&registration_lock);
> + list_del_init(&drv->list);
> + list_for_each_entry(port, &portlist, list)
> + drv->detach(port);

Does this free port? Should this be list_for_each_entry_safe?

> + mutex_unlock(&registration_lock);

return ret;
> + }
> +
> + return ret;

return 0;

> }
> /**
> @@ -188,6 +233,7 @@ void parport_unregister_driver (struct parport_driver *drv)
> {
> struct parport *port;
>
> + driver_unregister(&drv->driver);
> mutex_lock(&registration_lock);
> list_del_init(&drv->list);
> list_for_each_entry(port, &portlist, list)
> @@ -281,6 +327,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 +380,8 @@ 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.init_name = name;
>
> for (device = 0; device < 5; device++)
> /* assume the worst */
> @@ -340,6 +389,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 < 0) {

if (ret) {
> + kfree(tmp);
> + return NULL;
> + }
> +
> return tmp;
> }
>
> @@ -442,6 +497,8 @@ void parport_remove_port(struct parport *port)
>
> mutex_unlock(&registration_lock);
>
> + device_unregister(&port->ddev);
> +
> parport_proc_unregister(port);
>
> for (i = 1; i < 3; i++) {
> @@ -774,12 +831,19 @@ int parport_claim(struct pardevice *dev)
> struct pardevice *oldcad;
> struct parport *port = dev->port->physport;
> unsigned long flags;
> + int ret;
>
> if (port->cad == dev) {
> printk(KERN_INFO "%s: %s already owner\n",
> dev->port->name,dev->name);
> return 0;
> }
> + dev->dev.bus = &parport_bus_type;
> + dev->dev.parent = &port->ddev;
> + dev->dev.init_name = dev->name;
> + ret = device_register(&dev->dev);
> + if (ret < 0)

Please use "if (ret) " everywhere unless it returns positive on success.
I know that I have done a rubbish review. I'm going to have to review
this properly later.

regards,
dan carpenter


\
 
 \ /
  Last update: 2015-04-10 17:21    [W:0.114 / U:5.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site