Re: [PATCH] staging: unisys: handle major number properly

[Dan Carpenter]

This patch also doesn't introduce bugs but it's sort of whacky and hard
to understand.  Also the subject and description imply or say "fix" but
it's just a cleanup.  The original code was also proper but just messy.

On Tue, Mar 17, 2015 at 08:31:24PM +0530, Sudip Mukherjee wrote:
> fixed the handling of dev_t and the major number.
> now the major and minor number is passed to the init function.
> similarly in the cleanup function dev_t is passed to unregister it.
> 
> Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> ---
>  drivers/staging/unisys/visorchipset/file.c             | 18 ++++++++----------
>  drivers/staging/unisys/visorchipset/file.h             |  4 ++--
>  .../staging/unisys/visorchipset/visorchipset_main.c    | 10 +++-------
>  3 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
> index 9ca7f1e..224e214 100644
> --- a/drivers/staging/unisys/visorchipset/file.c
> +++ b/drivers/staging/unisys/visorchipset/file.c
> @@ -30,7 +30,6 @@
>  
>  static struct cdev file_cdev;
>  static struct visorchannel **file_controlvm_channel;
> -static dev_t majordev = -1; /**< indicates major num for device */
>  static BOOL registered = FALSE;
>  
>  static int visorchipset_open(struct inode *inode, struct file *file);
> @@ -50,15 +49,17 @@ static const struct file_operations visorchipset_fops = {
>  };
>  
>  int
> -visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
> +visorchipset_file_init(int major, int minor,
> +		       struct visorchannel **controlvm_channel)

Pass the dev_t majordev;

1) Then it's consistent with visorchipset_file_cleanup()
2) You need majordev anyway.

Don't save majordev as a global because globals are bad and you already
have a copy in Visorchipset_platform_device.dev.devt.

>  {
>  	int rc = 0;
> +	dev_t majordev;
>  
>  	file_controlvm_channel = controlvm_channel;
> -	majordev = major_dev;
>  	cdev_init(&file_cdev, &visorchipset_fops);
>  	file_cdev.owner = THIS_MODULE;
> -	if (MAJOR(majordev) == 0) {
> +	majordev = MKDEV(major, minor);
> +	if (major == 0) {
>  		/* dynamic major device number registration required */
>  		if (alloc_chrdev_region(&majordev, 0, 1, MYDRVNAME) < 0)
>  			return -1;
> @@ -69,23 +70,20 @@ visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
>  			return -1;
>  		registered = TRUE;
>  	}
> -	rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
> +	rc = cdev_add(&file_cdev, MKDEV(major, 0), 1);

This should just be:

	rc = cdev_add(&file_cdev, majordev, 1);

So here is my suggestion:

[patch 1] delete dead code I mentioned in my previous email.
	This deletes "registered" and the (MAJOR(majordev) >= 0) check.

[patch 2] pass majordev to visorchipset_file_cleanup()
	This lets you delete the "majordev" global.

[patch 3] small cleanup in visorchipset_file_init()

-	rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
+	rc = cdev_add(&file_cdev, majordev, 1);

There are several other ways you could break it up but do something like
that.

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/