Re: [PATCH WIP] parport: add device model

[Dan Carpenter <dan.carpenter@oracle.com>]

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