lkml.org 
[lkml]   [2015]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] staging: unisys: handle major number properly
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



\
 
 \ /
  Last update: 2015-03-24 09:41    [W:0.047 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site