[PATCH] staging: unisys: handle major number properly

[PATCH] staging: unisys: handle major number properly

[Sudip Mukherjee]

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@vectorindia.org>
---
 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)
 {
 	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);
 	if (rc  < 0)
 		return -1;
 	return 0;
 }
 
 void
-visorchipset_file_cleanup(void)
+visorchipset_file_cleanup(dev_t majordev)
 {
 	if (file_cdev.ops != NULL)
 		cdev_del(&file_cdev);
 	file_cdev.ops = NULL;
 	if (registered) {
-		if (MAJOR(majordev) >= 0) {
-			unregister_chrdev_region(majordev, 1);
-			majordev = MKDEV(0, 0);
-		}
+		unregister_chrdev_region(majordev, 1);
 		registered = FALSE;
 	}
 }
diff --git a/drivers/staging/unisys/visorchipset/file.h b/drivers/staging/unisys/visorchipset/file.h
index dc7a195..213b3a3 100644
--- a/drivers/staging/unisys/visorchipset/file.h
+++ b/drivers/staging/unisys/visorchipset/file.h
@@ -20,8 +20,8 @@
 
 #include "globals.h"
 
-int visorchipset_file_init(dev_t majorDev,
+int visorchipset_file_init(int major, int minor,
 			   struct visorchannel **pControlVm_channel);
-void visorchipset_file_cleanup(void);
+void visorchipset_file_cleanup(dev_t majordev);
 
 #endif
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index ec258ae..17d3b3f 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -241,9 +241,6 @@ static struct visorchipset_busdev_responders BusDev_Responders = {
 	.device_resume = device_resume_response,
 };
 
-/* info for /dev/visorchipset */
-static dev_t MajorDev = -1; /**< indicates major num for device */
-
 /* prototypes for attributes */
 static ssize_t toolaction_show(struct device *dev,
 	struct device_attribute *attr, char *buf);
@@ -2225,8 +2222,7 @@ visorchipset_init(void)
 		return -ENODEV;
 	}
 
-	MajorDev = MKDEV(visorchipset_major, 0);
-	rc = visorchipset_file_init(MajorDev, &ControlVm_channel);
+	rc = visorchipset_file_init(visorchipset_major, 0, &ControlVm_channel);
 	if (rc < 0) {
 		POSTCODE_LINUX_2(CHIPSET_INIT_FAILURE_PC, DIAG_SEVERITY_ERR);
 		goto Away;
@@ -2276,7 +2272,7 @@ visorchipset_init(void)
 
 	}
 
-	Visorchipset_platform_device.dev.devt = MajorDev;
+	Visorchipset_platform_device.dev.devt = MKDEV(visorchipset_major, 0);
 	if (platform_device_register(&Visorchipset_platform_device) < 0) {
 		POSTCODE_LINUX_2(DEVICE_REGISTER_FAILURE_PC, DIAG_SEVERITY_ERR);
 		rc = -1;
@@ -2322,7 +2318,7 @@ visorchipset_exit(void)
 
 	visorchannel_destroy(ControlVm_channel);
 
-	visorchipset_file_cleanup();
+	visorchipset_file_cleanup(Visorchipset_platform_device.dev.devt);
 	POSTCODE_LINUX_2(DRIVER_EXIT_PC, POSTCODE_SEVERITY_INFO);
 }
 
-- 
1.8.1.2

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

[Greg Kroah-Hartman]

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@vectorindia.org>
> ---
>  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(-)

This doesn't apply anymore, due to other changes recently to this
driver.

But even if it did, I don't think it is correct.  I really don't
understand what you are trying to do here.  I think you just merged two
different major numbers togther, which isn't good at all.  But if you
didn't, then why is this patch doing different things to different files
(hint, only do one thing per file.)

Also, why does the driver have multiple major numbers?  Isn't a single
major good enough?  How many does it need?  For what does it use them
for?

Totally confused,

greg k-h

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

[Sudip Mukherjee]

On Mon, Mar 23, 2015 at 10:04:40PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 17, 2015 at 08:31:24PM +0530, Sudip Mukherjee wrote:
<snip>
>
> This doesn't apply anymore, due to other changes recently to this
> driver.
>
> But even if it did, I don't think it is correct.  I really don't
> understand what you are trying to do here.  I think you just merged two
> different major numbers togther, which isn't good at all.  But if you
> didn't, then why is this patch doing different things to different files
> (hint, only do one thing per file.)
>
> Also, why does the driver have multiple major numbers?  Isn't a single
> major good enough?  How many does it need?  For what does it use them
> for?

but, according to my understanding the driver is having only one major
number. visorchipset_major is the major number defined in
visorchipset_main.c as a module parameter. The original code in
visorchipset_main.c was creating dev_t from this major number and
calling the function visorchipset_file_init(), which is in file.c
with the dev_t as an argument.

Now visorchipset_file_init(), it is registering that dev_t as a
char driver and  storing it in a static variable so that it can reuse
that dev_t in visorchipset_file_cleanup().

My patch is just passing the major and minor number as argument to
visorchipset_file_init() which is creating that dev_t while registering.
and instead of storing it again as a static variable i am using
Visorchipset_platform_device.dev.devt while calling the cleanup().

now since all are related to only one change so it all came in a single
patch.

is my understanding correct or am i missing something here?

regards
sudip


>
> Totally confused,
>
> greg k-h

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@vectorindia.org>
> ---
>  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

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

[Sudip Mukherjee]

On Tue, Mar 24, 2015 at 11:32:47AM +0300, Dan Carpenter wrote:
<snip>
> >  	}
> > -	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.

that was initially my first patch.

> 
> [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);

and i can also include the removal of that global variable in this
3rd patch.
thanks.. after Greg's review i was thinking i understood the code wrong.
but then will this be a v2 or a whole new series?

regards
sudip

> 
> There are several other ways you could break it up but do something like
> that.
> 
> regards,
> dan carpenter
> 

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

[Dan Carpenter]

On Tue, Mar 24, 2015 at 02:13:16PM +0530, Sudip Mukherjee wrote:
> On Tue, Mar 24, 2015 at 11:32:47AM +0300, Dan Carpenter wrote:
> <snip>
> > >  	}
> > > -	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.
> 
> that was initially my first patch.

Yes, but the patch description wasn't clear.  You were talking saying
the condition was always true because of the types and I am saying the
condition is always true because of the order the functions are called.

> 
> > 
> > [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);
> 
> and i can also include the removal of that global variable in this
> 3rd patch.
> thanks.. after Greg's review i was thinking i understood the code wrong.
> but then will this be a v2 or a whole new series?

I'm pretty sure we're up to v3 at least now.  This means redoing the
whole series, yes.

regards,
dan carpenter