linux-kernel - Re: [PATCH] staging: unisys: handle major number properly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150324083247.GP10964@mwanda>
Date:	Tue, 24 Mar 2015 11:32:47 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Benjamin Romer <benjamin.romer@...sys.com>,
	David Kershner <david.kershner@...sys.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, sparmaintainer@...sys.com,
	linux-kernel@...r.kernel.org
Subject: Re: [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@...torindia.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

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