[PATCH v2 1/2] parport: return of attach and parport_register_driver

[PATCH v2 1/2] parport: return of attach and parport_register_driver

[Sudip Mukherjee]

as of now, we are not checking if attach or parport_register_driver
has succeeded or failed. But attach can fail in the places where they
have been used. Lets create an attach_ret where we will check the
return of attach, and if attach fails then parport_register_driver
should also fail. We can have multiple parallel port, like parport0,
parport1 and one driver may decide to only use parport0. so we mark
attach as failed only if it has never returned a 0.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/parport/share.c | 27 ++++++++++++++++++++++-----
 include/linux/parport.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..4aab733 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -143,28 +143,45 @@ static void get_lowlevel_driver (void)
  *	pointer it must call parport_get_port() to do so.  Calling
  *	parport_register_device() on that port will do this for you.
  *
+ *	The attach_ret() function will check for the return value.
+ *
  *	The driver's detach() function may block.  The port that
  *	detach() is given will be valid for the duration of the
  *	callback, but if the driver wants to take a copy of the
  *	pointer it must call parport_get_port() to do so.
  *
- *	Returns 0 on success.  Currently it always succeeds.
+ *	Returns 0 on success.
  **/
 
 int parport_register_driver (struct parport_driver *drv)
 {
 	struct parport *port;
+	bool attached = false;
+	int err, ret = 0;
 
 	if (list_empty(&portlist))
 		get_lowlevel_driver ();
 
 	mutex_lock(&registration_lock);
-	list_for_each_entry(port, &portlist, list)
-		drv->attach(port);
-	list_add(&drv->list, &drivers);
+	list_for_each_entry(port, &portlist, list) {
+		if (drv->attach_ret) {
+			err = drv->attach_ret(port);
+		} else {
+			drv->attach(port);
+			err = 0;
+		}
+		if (err == 0)
+			attached = true;
+		else
+			ret = err;
+	}
+	if (attached) {
+		list_add(&drv->list, &drivers);
+		ret = 0;
+	}
 	mutex_unlock(&registration_lock);
 
-	return 0;
+	return ret;
 }
 
 /**
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..cc869ec 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -250,6 +250,7 @@ struct parport {
 struct parport_driver {
 	const char *name;
 	void (*attach) (struct parport *);
+	int (*attach_ret)(struct parport *);
 	void (*detach) (struct parport *);
 	struct list_head list;
 };
-- 
1.8.1.2

[PATCH v2 2/2] ppdev: return proper error values from attach

[Sudip Mukherjee]

As of now, parport_register_driver() never fails, so even if
device_create() fails for some reason we will still think that the
module has initialized properly.
lets start using attach_ret and return the proper return value from
device_create(), so that if device_create fails for some reason then
module_init will also fail.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/char/ppdev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index ae0b42b..ed38071 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -748,10 +748,14 @@ static const struct file_operations pp_fops = {
 	.release	= pp_release,
 };
 
-static void pp_attach(struct parport *port)
+static int pp_attach(struct parport *port)
 {
-	device_create(ppdev_class, port->dev, MKDEV(PP_MAJOR, port->number),
-		      NULL, "parport%d", port->number);
+	struct device *dev;
+
+	dev = device_create(ppdev_class, port->dev,
+			    MKDEV(PP_MAJOR, port->number), NULL,
+			    "parport%d", port->number);
+	return PTR_ERR_OR_ZERO(dev);
 }
 
 static void pp_detach(struct parport *port)
@@ -761,7 +765,7 @@ static void pp_detach(struct parport *port)
 
 static struct parport_driver pp_driver = {
 	.name		= CHRDEV,
-	.attach		= pp_attach,
+	.attach_ret	= pp_attach,
 	.detach		= pp_detach,
 };
 
-- 
1.8.1.2

Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

[Greg Kroah-Hartman]

On Wed, Apr 08, 2015 at 07:00:16PM +0530, Sudip Mukherjee wrote:
> as of now, we are not checking if attach or parport_register_driver
> has succeeded or failed. But attach can fail in the places where they
> have been used. Lets create an attach_ret where we will check the
> return of attach, and if attach fails then parport_register_driver
> should also fail. We can have multiple parallel port, like parport0,
> parport1 and one driver may decide to only use parport0. so we mark
> attach as failed only if it has never returned a 0.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/parport/share.c | 27 ++++++++++++++++++++++-----
>  include/linux/parport.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index 3fa6624..4aab733 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -143,28 +143,45 @@ static void get_lowlevel_driver (void)
>   *	pointer it must call parport_get_port() to do so.  Calling
>   *	parport_register_device() on that port will do this for you.
>   *
> + *	The attach_ret() function will check for the return value.
> + *
>   *	The driver's detach() function may block.  The port that
>   *	detach() is given will be valid for the duration of the
>   *	callback, but if the driver wants to take a copy of the
>   *	pointer it must call parport_get_port() to do so.
>   *
> - *	Returns 0 on success.  Currently it always succeeds.
> + *	Returns 0 on success.
>   **/
>  
>  int parport_register_driver (struct parport_driver *drv)
>  {
>  	struct parport *port;
> +	bool attached = false;
> +	int err, ret = 0;
>  
>  	if (list_empty(&portlist))
>  		get_lowlevel_driver ();
>  
>  	mutex_lock(&registration_lock);
> -	list_for_each_entry(port, &portlist, list)
> -		drv->attach(port);
> -	list_add(&drv->list, &drivers);
> +	list_for_each_entry(port, &portlist, list) {
> +		if (drv->attach_ret) {
> +			err = drv->attach_ret(port);
> +		} else {
> +			drv->attach(port);
> +			err = 0;
> +		}
> +		if (err == 0)
> +			attached = true;
> +		else
> +			ret = err;
> +	}
> +	if (attached) {
> +		list_add(&drv->list, &drivers);
> +		ret = 0;
> +	}
>  	mutex_unlock(&registration_lock);
>  
> -	return 0;
> +	return ret;
>  }

What really needs to happen is for the parport subsystem to move to the
driver model, which I don't think ever happened way back during the 2.5
development cycle.  When that happens, then you will not have code like
this, and the drivers will work "properly"...

That's a much larger task, especially trying to do it without breaking
anything along the way, but it's the "correct" thing to do.

thanks,

greg k-h

Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

[Sudip Mukherjee]

On Wed, Apr 08, 2015 at 03:48:57PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 08, 2015 at 07:00:16PM +0530, Sudip Mukherjee wrote:
> >  }
> 
> What really needs to happen is for the parport subsystem to move to the
> driver model, which I don't think ever happened way back during the 2.5
> development cycle.  When that happens, then you will not have code like
> this, and the drivers will work "properly"...
> 
> That's a much larger task, especially trying to do it without breaking
> anything along the way, but it's the "correct" thing to do.

and i guess since parport is orphan so no one is working on that
also, and maybe one more problem you all will be facing is lack
of systems with a parport. I do have systems with parallel port, so
let me try and see if i can do something about it. To start with,
I think I should go back to your LDD3-ch.14 for rest of the day.

regards
sudip

> 
> thanks,
> 
> greg k-h

Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

[Dan Carpenter]

On Wed, Apr 08, 2015 at 07:00:16PM +0530, Sudip Mukherjee wrote:
> as of now, we are not checking if attach or parport_register_driver
> has succeeded or failed. But attach can fail in the places where they
> have been used. Lets create an attach_ret where we will check the
> return of attach, and if attach fails then parport_register_driver
> should also fail. We can have multiple parallel port, like parport0,
> parport1 and one driver may decide to only use parport0. so we mark
> attach as failed only if it has never returned a 0.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/parport/share.c | 27 ++++++++++++++++++++++-----
>  include/linux/parport.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index 3fa6624..4aab733 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -143,28 +143,45 @@ static void get_lowlevel_driver (void)
>   *	pointer it must call parport_get_port() to do so.  Calling
>   *	parport_register_device() on that port will do this for you.
>   *
> + *	The attach_ret() function will check for the return value.
> + *
>   *	The driver's detach() function may block.  The port that
>   *	detach() is given will be valid for the duration of the
>   *	callback, but if the driver wants to take a copy of the
>   *	pointer it must call parport_get_port() to do so.
>   *
> - *	Returns 0 on success.  Currently it always succeeds.
> + *	Returns 0 on success.
>   **/
>  
>  int parport_register_driver (struct parport_driver *drv)
>  {
>  	struct parport *port;
> +	bool attached = false;
> +	int err, ret = 0;
>  
>  	if (list_empty(&portlist))
>  		get_lowlevel_driver ();
>  
>  	mutex_lock(&registration_lock);
> -	list_for_each_entry(port, &portlist, list)
> -		drv->attach(port);
> -	list_add(&drv->list, &drivers);
> +	list_for_each_entry(port, &portlist, list) {
> +		if (drv->attach_ret) {
> +			err = drv->attach_ret(port);
> +		} else {
> +			drv->attach(port);
> +			err = 0;
> +		}
> +		if (err == 0)
> +			attached = true;
> +		else
> +			ret = err;
> +	}
> +	if (attached) {
> +		list_add(&drv->list, &drivers);
> +		ret = 0;
> +	}

I still think it would be nicer to create a do_attach() wrapper like I
said earlier.

	list_for_each_entry(port, &portlist, list) {
		ret = do_attach();
		if (ret)
			continue;
		attached = true;
	}

	if (attached)
		list_add(&drv->list, &drivers);

	mutex_unlock(&registration_lock);

	if (!attached)
		return -ENODEV;
	return 0;

The attach_driver_chain() function needs to be updated as well or it
will break.

regards,
dan carpenter

Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

[Sudip Mukherjee]

On Wed, Apr 08, 2015 at 05:17:54PM +0300, Dan Carpenter wrote:
> On Wed, Apr 08, 2015 at 07:00:16PM +0530, Sudip Mukherjee wrote:
> > +	if (attached) {
> > +		list_add(&drv->list, &drivers);
> > +		ret = 0;
> > +	}
> 
> I still think it would be nicer to create a do_attach() wrapper like I
> said earlier.
yes, after seeing your this code beside mine, your code looks more tidy
than mine. sorry, for not listening to you on the first place.
> 
<snip>
> 	mutex_unlock(&registration_lock);
> 
> 	if (!attached)
> 		return -ENODEV;
> 	return 0;
> 
> The attach_driver_chain() function needs to be updated as well or it
> will break.
oops, i missed that function. but i didn't get any error?
ok, this is for those parallel ports which registers after our driver
registers.
But Greg wants to have parallel port changed into the device model,
so should i start with that task or shall we finalize this series so
that it can be applied for the timebeing?

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH v2 1/2] parport: return of attach and parport_register_driver

[Dan Carpenter]

Greg is coming at this with a lot more experience as a driver author.  I
am a newbie and don't really know where to start when it comes to doing
the things he mentioned.  I can't advise you.

I guess take some time and to look at what moving to the device model
means.  It's not like we're in a rush.

regards,
dan carpenter