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

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

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