From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752180AbbCXIcq (ORCPT ); Tue, 24 Mar 2015 04:32:46 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:44902 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751941AbbCXIcm (ORCPT ); Tue, 24 Mar 2015 04:32:42 -0400 Date: Tue, 24 Mar 2015 11:32:47 +0300 From: Dan Carpenter To: Sudip Mukherjee Cc: Benjamin Romer , David Kershner , Greg Kroah-Hartman , devel@driverdev.osuosl.org, sparmaintainer@unisys.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: unisys: handle major number properly Message-ID: <20150324083247.GP10964@mwanda> References: <1426604484-7770-1-git-send-email-sudipm.mukherjee@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426604484-7770-1-git-send-email-sudipm.mukherjee@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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