[PATCH WIP] parport: add device model

[PATCH WIP] parport: add device model

[Sudip Mukherjee]

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);
-	return 0;
+	ret = parport_bus_init();
+	if (ret)
+		unregister_sysctl_table(parport_default_sysctl_table.
+					sysctl_header);
+	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();
 }
 
 #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,
+};
+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) {
+		mutex_lock(&registration_lock);
+		list_del_init(&drv->list);
+		list_for_each_entry(port, &portlist, list)
+			drv->detach(port);
+		mutex_unlock(&registration_lock);
+	}
+
+	return ret;
 }
 
 /**
@@ -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) {
+		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)
+		return ret;
 
 	/* Preempt any current device */
 	write_lock_irqsave (&port->cad_lock, flags);
@@ -866,6 +930,7 @@ blocked:
 		spin_unlock (&port->waitlist_lock);
 	}
 	write_unlock_irqrestore (&port->cad_lock, flags);
+	device_unregister(&dev->dev);
 	return -EAGAIN;
 }
 
@@ -971,6 +1036,7 @@ void parport_release(struct pardevice *dev)
 
 	port->cad = NULL;
 	write_unlock_irqrestore(&port->cad_lock, flags);
+	device_unregister(&dev->dev);
 
 	/* Save control registers */
 	port->ops->save_state(port, dev->state);
@@ -1019,7 +1085,7 @@ EXPORT_SYMBOL(parport_release);
 EXPORT_SYMBOL(parport_register_port);
 EXPORT_SYMBOL(parport_announce_port);
 EXPORT_SYMBOL(parport_remove_port);
-EXPORT_SYMBOL(parport_register_driver);
+EXPORT_SYMBOL(__parport_register_driver);
 EXPORT_SYMBOL(parport_unregister_driver);
 EXPORT_SYMBOL(parport_register_device);
 EXPORT_SYMBOL(parport_unregister_device);
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..d7e4451 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -15,6 +15,7 @@
 #include <linux/semaphore.h>
 #include <asm/ptrace.h>
 #include <uapi/linux/parport.h>
+#include <linux/device.h>
 
 /* Define this later. */
 struct parport;
@@ -146,6 +147,7 @@ struct pardevice {
 	struct pardevice *next;
 	struct pardevice *prev;
 	struct parport_state *state;     /* saved status over preemption */
+	struct device dev;
 	wait_queue_head_t wait_q;
 	unsigned long int time;
 	unsigned long int timeslice;
@@ -156,6 +158,11 @@ struct pardevice {
 	void * sysctl_table;
 };
 
+static inline struct pardevice *to_pardevice(struct device *n)
+{
+	return container_of(n, struct pardevice, dev);
+}
+
 /* IEEE1284 information */
 
 /* IEEE1284 phases. These are exposed to userland through ppdev IOCTL
@@ -195,6 +202,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
@@ -251,6 +259,7 @@ struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
 	void (*detach) (struct parport *);
+	struct device_driver driver;
 	struct list_head list;
 };
 
@@ -267,12 +276,22 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
    determining the IEEE 1284.3 topology of the port and collecting
    DeviceIDs). */
 void parport_announce_port (struct parport *port);
+void parport_bus_exit(void);
+extern struct bus_type parport_bus_type;
 
+int parport_bus_init(void);
 /* Unregister a port. */
 extern void parport_remove_port(struct parport *port);
 
 /* Register a new high-level driver. */
-extern int parport_register_driver (struct parport_driver *);
+int __parport_register_driver(struct parport_driver *,
+			      struct module *, const char *);
+/*
+ * parport_register_driver must be a macro so that KBUILD_MODNAME can
+ * be expanded
+ */
+#define parport_register_driver(driver)             \
+	__parport_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
 
 /* Unregister a high-level driver. */
 extern void parport_unregister_driver (struct parport_driver *);
-- 
1.8.1.2

Re: [PATCH WIP] parport: add device model

[Dan Carpenter]

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

Re: [PATCH WIP] parport: add device model

[Ondrej Zary]

On Friday 10 April 2015 16:30:38 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.

Many newer parallel port devices support plug&play (IEEE1284 device ID) but 
Linux never supported it properly. The ID is probed and even the class is 
printed in the kernel log (drivers/parport/probe.c) but there's no support 
for module autoloading based on that.
This could be a good opportunity to add this support. I was thinking about 
this while playing with some parport webcams recently.

-- 
Ondrej Zary

Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee]

On Fri, Apr 10, 2015 at 08:24:23PM +0200, Ondrej Zary wrote:
> On Friday 10 April 2015 16:30:38 Sudip Mukherjee wrote:
> 
> Many newer parallel port devices support plug&play (IEEE1284 device ID) but 
> Linux never supported it properly. The ID is probed and even the class is 
> printed in the kernel log (drivers/parport/probe.c) but there's no support 
> for module autoloading based on that.
> This could be a good opportunity to add this support. I was thinking about 
> this while playing with some parport webcams recently.
I think yes, or maybe immediately after this updation is over. But the
main problem for me will be to get hold of a plug&play parallel port
device. is any available now? Can you please suggest one other than
webcam so that you can test on webcam and I can test it on something
else.

regards
sudip

> 
> -- 
> Ondrej Zary

Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee]

On Fri, Apr 10, 2015 at 05:49:55PM +0300, Dan Carpenter wrote:
> On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote:
<snip>
> > +
> >  	parport_default_sysctl_table.sysctl_header =
> >  		register_sysctl_table(parport_default_sysctl_table.dev_dir);
> 
> Should we return an error if this fails?
not sure. but even if it fails it will not affect the normal functioning
of the parallel port. but I will add that in the next WIP patch.
> 
> > -	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;
do we need two returns here? parport_bus_init() will return 0 if it succeeds,
so return ret will return either 0 or the error code whatever the case maybe.	
> 
> 
> > +	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?
no, we dont need. on similar reasoning we also donot need parport_bus_init().
I will remove both. :)
> 
<snip>
> 
> > +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().
ok.
> 
<snip>
> > +	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?
I am not sure what you meant by "free port". attach will claim the port,
and the port will be marked. detach will just remove that connection and
the driver will release the port.
> 
> > +		mutex_unlock(&registration_lock);
> 
> 		return ret;
> > +	}
> > +
> > +	return ret;
> 
> 	return 0;
do we need two returns? as ret will be either 0 or error code.
> 
> >  }
> >  
<snip>	
> 
> Please use "if (ret) " everywhere unless it returns positive on success.
sure.
> 
> I know that I have done a rubbish review.  I'm going to have to review
> this properly later.
main thing i wanted to know is if my approach is correct. since nothing
on that so I hope I am on the correct track. Thanks.
I will send in the next version in a day or two.

regards
sudip
> 
> regards,
> dan carpenter

Re: [PATCH WIP] parport: add device model

[Greg KH]

On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> > I know that I have done a rubbish review.  I'm going to have to review
> > this properly later.
> main thing i wanted to know is if my approach is correct. since nothing
> on that so I hope I am on the correct track. Thanks.
> I will send in the next version in a day or two.

At quick glance, you are on the right track.  Writing a new bus is hard,
I know, the documentation is lacking and it's tricky in places and the
api is horrid in others.  I know this, just never had the time to make
it easier, so if you have any questions about it, please let me know.

thanks,

greg k-h

Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee]

On Sat, Apr 11, 2015 at 09:27:16AM +0200, Greg KH wrote:
> On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> > > I know that I have done a rubbish review.  I'm going to have to review
> > > this properly later.
> > main thing i wanted to know is if my approach is correct. since nothing
> > on that so I hope I am on the correct track. Thanks.
> > I will send in the next version in a day or two.
> 
> At quick glance, you are on the right track.  Writing a new bus is hard,
> I know, the documentation is lacking and it's tricky in places and the
> api is horrid in others.  I know this, just never had the time to make
> it easier, so if you have any questions about it, please let me know.
well, as of now one question. I am planning like this :

                       sys
         _______________|_____________
         |	|	|       |    | 
        bus
________|______
              |
           parport
       _______|_____
       |            |
    devices       drivers- lp, ppdev, panel etc.. 
  _____|________
  |            |
parport0      parport1


I can understand that drivers need to be binded to one device, so
suppose ppdev wants to use parport0, how that binding will be done?
do i need mark the driver in the parport->ddev.driver ?
I think that should have been automatically done if i have a probe
function for the bus...
somehow I am missing this step in the other drivers codes.

regards
sudip
> 
> thanks,
> 
> greg k-h

Re: [PATCH WIP] parport: add device model

[Ondrej Zary]

On Saturday 11 April 2015 07:05:11 you wrote:
> On Fri, Apr 10, 2015 at 08:24:23PM +0200, Ondrej Zary wrote:
> > On Friday 10 April 2015 16:30:38 Sudip Mukherjee wrote:
> >
> > Many newer parallel port devices support plug&play (IEEE1284 device ID)
> > but Linux never supported it properly. The ID is probed and even the
> > class is printed in the kernel log (drivers/parport/probe.c) but there's
> > no support for module autoloading based on that.
> > This could be a good opportunity to add this support. I was thinking
> > about this while playing with some parport webcams recently.
>
> I think yes, or maybe immediately after this updation is over. But the
> main problem for me will be to get hold of a plug&play parallel port
> device. is any available now? Can you please suggest one other than
> webcam so that you can test on webcam and I can test it on something
> else.

Most printers produced since 1995 or 1996 are plug&play capable (lp module 
could be loaded if a printer-class device is found) and also external drives 
(ZIP, LS-120).

-- 
Ondrej Zary

Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee]

On Sat, Apr 11, 2015 at 01:41:34PM +0530, Sudip Mukherjee wrote:
> On Sat, Apr 11, 2015 at 09:27:16AM +0200, Greg KH wrote:
> > On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> well, as of now one question. I am planning like this :
> 
>                        sys
>          _______________|_____________
>          |	|	|       |    | 
>         bus
> ________|______
>               |
>            parport
>        _______|_____
>        |            |
>     devices       drivers- lp, ppdev, panel etc.. 
>   _____|________
>   |            |
> parport0      parport1
> 
> 
> I can understand that drivers need to be binded to one device, so
> suppose ppdev wants to use parport0, how that binding will be done?
> do i need mark the driver in the parport->ddev.driver ?
> I think that should have been automatically done if i have a probe
> function for the bus...
> somehow I am missing this step in the other drivers codes.

i think i got the missing link, let me try first.

regards
sudip

Re: [PATCH WIP] parport: add device model

[Greg KH]

On Sat, Apr 11, 2015 at 01:41:34PM +0530, Sudip Mukherjee wrote:
> On Sat, Apr 11, 2015 at 09:27:16AM +0200, Greg KH wrote:
> > On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> > > > I know that I have done a rubbish review.  I'm going to have to review
> > > > this properly later.
> > > main thing i wanted to know is if my approach is correct. since nothing
> > > on that so I hope I am on the correct track. Thanks.
> > > I will send in the next version in a day or two.
> > 
> > At quick glance, you are on the right track.  Writing a new bus is hard,
> > I know, the documentation is lacking and it's tricky in places and the
> > api is horrid in others.  I know this, just never had the time to make
> > it easier, so if you have any questions about it, please let me know.
> well, as of now one question. I am planning like this :
> 
>                        sys
>          _______________|_____________
>          |	|	|       |    | 
>         bus
> ________|______
>               |
>            parport
>        _______|_____
>        |            |
>     devices       drivers- lp, ppdev, panel etc.. 
>   _____|________
>   |            |
> parport0      parport1
> 
> 
> I can understand that drivers need to be binded to one device, so
> suppose ppdev wants to use parport0, how that binding will be done?

The driver core does the "binding".

> do i need mark the driver in the parport->ddev.driver ?

No, just have the probe function for the ppdev function return 0,
meaning it successfully bound to the driver that was passed to it and
all will be fine.

> I think that should have been automatically done if i have a probe
> function for the bus...

Yes.

> somehow I am missing this step in the other drivers codes.

Hope this helps,

greg k-h

Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee]

On Mon, Apr 13, 2015 at 10:43:39AM +0200, Greg KH wrote:
> On Sat, Apr 11, 2015 at 01:41:34PM +0530, Sudip Mukherjee wrote:
> > On Sat, Apr 11, 2015 at 09:27:16AM +0200, Greg KH wrote:
> > > On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
<snip>
> > 
> > 
> > I can understand that drivers need to be binded to one device, so
> > suppose ppdev wants to use parport0, how that binding will be done?
> 
> The driver core does the "binding".
> 
> > do i need mark the driver in the parport->ddev.driver ?
> 
> No, just have the probe function for the ppdev function return 0,
> meaning it successfully bound to the driver that was passed to it and
> all will be fine.
well, I was thinking that instead of having probe for individual
drivers (then we need to modify all the drivers), i was planning like:
when the driver calls parport_device_register(), the driver will be
registered under parport and also a subdevice will be created under the
particular parport the driver wants to use and both will have the same
name. The probe of the bus will verify that name comparison and will
return 0 if the name matches. is this plan ok ?
and i wanted the binding to happen when the driver calls parport_claim()
and for that I was thinking of calling device_attach() . is that also ok?

regards
sudip

> 
> > I think that should have been automatically done if i have a probe
> > function for the bus...
> 
> Yes.
> 
> > somehow I am missing this step in the other drivers codes.
> 
> Hope this helps,
> 
> greg k-h

Re: [PATCH WIP] parport: add device model

[Dan Carpenter]

On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> On Fri, Apr 10, 2015 at 05:49:55PM +0300, Dan Carpenter wrote:
> > On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote:
> <snip>
> > > +
> > >  	parport_default_sysctl_table.sysctl_header =
> > >  		register_sysctl_table(parport_default_sysctl_table.dev_dir);
> > 
> > Should we return an error if this fails?
> not sure. but even if it fails it will not affect the normal functioning
> of the parallel port. but I will add that in the next WIP patch.

Probably, it's better to leave it as-is if you aren't sure.  I was just
asking because I didn't know myself.

> > 
> > > -	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;
> do we need two returns here? parport_bus_init() will return 0 if it succeeds,
> so return ret will return either 0 or the error code whatever the case maybe.

Yes, they work the same, you're right.  But the other style is better
and more robust.

I have been trying to explain this to people but "return 0;" is
beautiful code.  Functions normally are a list of statements in a row
with exceptions for error handling.  The last statement in the success
path should be "return 0;".

Don't mix error and success paths.  I see a quite a few bugs like this
where the error handling doesn't have a return then later we add some
code at the end of the function and forget to add the return.

	ret = parport_bus_init();
	if (ret)
		unregister_sysctl_table();

	ret = something_else();

	return ret;

> > 
> > 
> > > +	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?
> no, we dont need. on similar reasoning we also donot need parport_bus_init().
> I will remove both. :)
> > 
> <snip>
> > 
> > > +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().
> ok.
> > 
> <snip>
> > > +	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?
> I am not sure what you meant by "free port". attach will claim the port,
> and the port will be marked. detach will just remove that connection and
> the driver will release the port.

My concern is that we dereference port to get the next port.  If it's
freed now it causes a use after free.  It's easier to detect if you have
free poisoning turned on.

> > 
> > > +		mutex_unlock(&registration_lock);
> > 
> > 		return ret;
> > > +	}
> > > +
> > > +	return ret;
> > 
> > 	return 0;
> do we need two returns? as ret will be either 0 or error code.
> > 
> > >  }
> > >  
> <snip>	
> > 
> > Please use "if (ret) " everywhere unless it returns positive on success.
> sure.
> > 
> > I know that I have done a rubbish review.  I'm going to have to review
> > this properly later.
> main thing i wanted to know is if my approach is correct. since nothing
> on that so I hope I am on the correct track. Thanks.
> I will send in the next version in a day or two.

Heh.  No, I really know less than you do about the driver model at this
point.  Sorry.  It's going to take me a bit to get up to speed.

regards,
dan carpenter

Re: [PATCH WIP] parport: add device model

[Greg KH]

On Mon, Apr 13, 2015 at 03:32:44PM +0530, Sudip Mukherjee wrote:
> On Mon, Apr 13, 2015 at 10:43:39AM +0200, Greg KH wrote:
> > On Sat, Apr 11, 2015 at 01:41:34PM +0530, Sudip Mukherjee wrote:
> > > On Sat, Apr 11, 2015 at 09:27:16AM +0200, Greg KH wrote:
> > > > On Sat, Apr 11, 2015 at 10:56:51AM +0530, Sudip Mukherjee wrote:
> <snip>
> > > 
> > > 
> > > I can understand that drivers need to be binded to one device, so
> > > suppose ppdev wants to use parport0, how that binding will be done?
> > 
> > The driver core does the "binding".
> > 
> > > do i need mark the driver in the parport->ddev.driver ?
> > 
> > No, just have the probe function for the ppdev function return 0,
> > meaning it successfully bound to the driver that was passed to it and
> > all will be fine.
> well, I was thinking that instead of having probe for individual
> drivers (then we need to modify all the drivers),

Sorry, you will have to modify all drivers, that's going to be a
requirement.

> i was planning like:
> when the driver calls parport_device_register(), the driver will be
> registered under parport and also a subdevice will be created under the
> particular parport the driver wants to use and both will have the same
> name. The probe of the bus will verify that name comparison and will
> return 0 if the name matches. is this plan ok ?
> and i wanted the binding to happen when the driver calls parport_claim()
> and for that I was thinking of calling device_attach() . is that also ok?

I don't know what parport_claim() does, sorry, but you are going to have
to change all drivers.

I suggest working on adding the new parport bus, and convert one or two
drivers to the new code, to get it working first.  Then you can convert
the other drivers and then delete the old registration code from the
parport core, as it will be not used anymore.

good luck,

greg k-h