Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964984AbbDJOuR (ORCPT ); Fri, 10 Apr 2015 10:50:17 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:45298 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933560AbbDJOuO (ORCPT ); Fri, 10 Apr 2015 10:50:14 -0400 Date: Fri, 10 Apr 2015 17:49:55 +0300 From: Dan Carpenter To: Sudip Mukherjee Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH WIP] parport: add device model Message-ID: <20150410144955.GJ16501@mwanda> References: <1428676238-17141-1-git-send-email-sudipm.mukherjee@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428676238-17141-1-git-send-email-sudipm.mukherjee@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7470 Lines: 267 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 > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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(®istration_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(®istration_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(®istration_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(®istration_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(®istration_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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/