Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Dan Carpenter]

On Wed, May 06, 2015 at 03:46:17PM +0530, Sudip Mukherjee wrote:
> +void *pi_register_driver(char *name)
> +{
> +	struct parport_driver *parp_drv;
> +	int ret;
> +
> +	parp_drv = kzalloc(sizeof(*parp_drv), GFP_KERNEL);
> +	if (!parp_drv)
> +		return NULL;
> +
> +	parp_drv->name = name;
> +	parp_drv->probe = pi_probe;
> +
> +	ret = parport_register_driver(parp_drv);
> +
> +	if (ret)
> +		return NULL;

Remove the blank line between the call and the error handling.  We
should probably free parp_drv on error.

> +	return (void *)parp_drv;
> +}
> +EXPORT_SYMBOL(pi_register_driver);
> +
> +void pi_unregister_driver(void *_drv)
> +{
> +	struct parport_driver *drv = _drv;
> +
> +	parport_unregister_driver(drv);
> +	kfree(drv);
> +}
> +EXPORT_SYMBOL(pi_unregister_driver);
> diff --git a/drivers/block/paride/paride.h b/drivers/block/paride/paride.h
> index 2bddbf4..ddb9e58 100644
> --- a/drivers/block/paride/paride.h
> +++ b/drivers/block/paride/paride.h
> @@ -165,6 +165,8 @@ typedef struct pi_protocol PIP;
>  
>  extern int paride_register( PIP * );
>  extern void paride_unregister ( PIP * );
> +void *pi_register_driver(char *);
> +void pi_unregister_driver(void *);
>  
>  #endif /* __DRIVERS_PARIDE_H__ */
>  /* end of paride.h */
> diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
> index 3b7c9f1..9336236 100644
> --- a/drivers/block/paride/pcd.c
> +++ b/drivers/block/paride/pcd.c
> @@ -221,6 +221,7 @@ static int pcd_busy;		/* request being processed ? */
>  static int pcd_sector;		/* address of next requested sector */
>  static int pcd_count;		/* number of blocks still to do */
>  static char *pcd_buf;		/* buffer for request in progress */
> +static void *par_drv;		/* reference of parport driver */
>  
>  /* kernel glue structures */
>  
> @@ -690,6 +691,12 @@ static int pcd_detect(void)
>  	printk("%s: %s version %s, major %d, nice %d\n",
>  	       name, name, PCD_VERSION, major, nice);
>  
> +	par_drv = pi_register_driver(name);
> +	if (!par_drv) {
> +		pr_err("failed to register %s driver\n", name);
> +		return -1;
> +	}
> +
>  	k = 0;
>  	if (pcd_drive_count == 0) { /* nothing spec'd - so autoprobe for 1 */
>  		cd = pcd;
> @@ -723,6 +730,7 @@ static int pcd_detect(void)
>  	printk("%s: No CD-ROM drive found\n", name);
>  	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++)
>  		put_disk(cd->disk);
> +	pi_unregister_driver(par_drv);

You didn't introduce this, but I think the error handling here is broken
for the autoprobe case.  The autoprobe case doesn't have a for loop.

>  	return -1;
>  }
>  

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/