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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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