[PATCH v2 1/4] staging: unisys: remove unused variables

[PATCH v2 1/4] staging: unisys: remove unused variables

[Sudip Mukherjee]

these variable were being assigned some values but never reused.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: no change

 drivers/staging/unisys/virthba/virthba.c | 18 ++++++------------
 drivers/staging/unisys/virtpci/virtpci.c | 22 +++++++---------------
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/unisys/virthba/virthba.c b/drivers/staging/unisys/virthba/virthba.c
index e5b0dd8..655db2f 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -333,16 +333,14 @@ static inline void
 send_disk_add_remove(struct diskaddremove *dar)
 {
 	struct scsi_device *sdev;
-	int error;
 
 	sdev = scsi_device_lookup(dar->shost, dar->channel, dar->id, dar->lun);
 	if (sdev) {
 		if (!(dar->add))
 			scsi_remove_device(sdev);
 	} else if (dar->add) {
-		error =
-		    scsi_add_device(dar->shost, dar->channel, dar->id,
-				    dar->lun);
+		scsi_add_device(dar->shost, dar->channel, dar->id,
+				dar->lun);
 	}
 	kfree(dar);
 }
@@ -401,7 +399,6 @@ virthba_isr(int irq, void *dev_id)
 	struct channel_header __iomem *channel_header;
 	struct signal_queue_header __iomem *pqhdr;
 	u64 mask;
-	unsigned long long rc1;
 
 	if (virthbainfo == NULL)
 		return IRQ_NONE;
@@ -414,7 +411,8 @@ virthba_isr(int irq, void *dev_id)
 		0)) {
 		virthbainfo->interrupts_disabled++;
 		mask = ~ULTRA_CHANNEL_ENABLE_INTS;
-		rc1 = uisqueue_interlocked_and(virthbainfo->flags_addr, mask);
+		uisqueue_interlocked_and(virthbainfo->flags_addr,
+					 mask);
 	}
 	if (spar_signalqueue_empty(channel_header, IOCHAN_FROM_IOPART)) {
 		virthbainfo->interrupts_notme++;
@@ -807,7 +805,6 @@ virthba_queue_command_lck(struct scsi_cmnd *scsicmd,
 {
 	struct scsi_device *scsidev = scsicmd->device;
 	int insert_location;
-	unsigned char op;
 	unsigned char *cdb = scsicmd->cmnd;
 	struct Scsi_Host *scsihost = scsidev->host;
 	struct uiscmdrsp *cmdrsp;
@@ -890,7 +887,6 @@ virthba_queue_command_lck(struct scsi_cmnd *scsicmd,
 		cmdrsp->scsi.guest_phys_entries = scsi_sg_count(scsicmd);
 	}
 
-	op = cdb[0];
 	i = uisqueue_put_cmdrsp_with_lock_client(virthbainfo->chinfo.queueinfo,
 						 cmdrsp, IOCHAN_TO_IOPART,
 						 &virthbainfo->chinfo.
@@ -984,11 +980,9 @@ do_scsi_linuxstat(struct uiscmdrsp *cmdrsp, struct scsi_cmnd *scsicmd)
 {
 	struct virtdisk_info *vdisk;
 	struct scsi_device *scsidev;
-	struct sense_data *sd;
 
 	scsidev = scsicmd->device;
 	memcpy(scsicmd->sense_buffer, cmdrsp->scsi.sensebuf, MAX_SENSE_SIZE);
-	sd = (struct sense_data *)scsicmd->sense_buffer;
 
 	/* Do not log errors for disk-not-present inquiries */
 	if ((cmdrsp->scsi.cmnd[0] == INQUIRY) &&
@@ -1178,7 +1172,6 @@ process_incoming_rsps(void *v)
 	struct uiscmdrsp *cmdrsp = NULL;
 	const int SZ = sizeof(struct uiscmdrsp);
 	u64 mask;
-	unsigned long long rc1;
 
 	UIS_DAEMONIZE("vhba_incoming");
 	/* alloc once and reuse */
@@ -1197,7 +1190,8 @@ process_incoming_rsps(void *v)
 		atomic_set(&virthbainfo->interrupt_rcvd, 0);
 		/* drain queue */
 		drain_queue(virthbainfo, dc, cmdrsp);
-		rc1 = uisqueue_interlocked_or(virthbainfo->flags_addr, mask);
+		uisqueue_interlocked_or(virthbainfo->flags_addr,
+					mask);
 	}
 
 	kfree(cmdrsp);
diff --git a/drivers/staging/unisys/virtpci/virtpci.c b/drivers/staging/unisys/virtpci/virtpci.c
index 43b5736..e0d38d5 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -546,7 +546,6 @@ static void delete_all(void)
 static int delete_all_virt(enum virtpci_dev_type devtype,
 			   struct del_vbus_guestpart *delparams)
 {
-	int i;
 	unsigned char busid[BUS_ID_SIZE];
 	struct device *vbus;
 
@@ -561,7 +560,7 @@ static int delete_all_virt(enum virtpci_dev_type devtype,
 		return 0;
 
 	/* delete all vhbas/vnics */
-	i = virtpci_device_del(vbus, devtype, NULL, NULL);
+	virtpci_device_del(vbus, devtype, NULL, NULL);
 	return 1;
 }
 
@@ -797,7 +796,6 @@ static int virtpci_device_add(struct device *parentbus, int devtype,
 	struct virtpci_dev *tmpvpcidev = NULL, *prev;
 	unsigned long flags;
 	int ret;
-	struct spar_io_channel_protocol __iomem *io_chan = NULL;
 	struct device *dev;
 
 	POSTCODE_LINUX_2(VPCI_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
@@ -831,10 +829,6 @@ static int virtpci_device_add(struct device *parentbus, int devtype,
 	virtpcidev->queueinfo.chan = addparams->chanptr;
 	virtpcidev->queueinfo.send_int_if_needed = NULL;
 
-	/* Set up safe queue... */
-	io_chan = (struct spar_io_channel_protocol __iomem *)
-		virtpcidev->queueinfo.chan;
-
 	virtpcidev->intr = addparams->intr;
 
 	/* initialize stuff in the device portion of the struct */
@@ -946,7 +940,7 @@ static int virtpci_device_serverdown(struct device *parentbus,
 {
 	int pausethisone = 0;
 	bool found = false;
-	struct virtpci_dev *tmpvpcidev, *prevvpcidev;
+	struct virtpci_dev *tmpvpcidev;
 	struct virtpci_driver *vpcidriver;
 	unsigned long flags;
 	int rc = 0;
@@ -957,9 +951,8 @@ static int virtpci_device_serverdown(struct device *parentbus,
 	/* find the vhba or vnic in virtpci device list */
 	write_lock_irqsave(&vpcidev_list_lock, flags);
 
-	for (tmpvpcidev = vpcidev_list_head, prevvpcidev = NULL;
-	     (tmpvpcidev && !found);
-	     prevvpcidev = tmpvpcidev, tmpvpcidev = tmpvpcidev->next) {
+	for (tmpvpcidev = vpcidev_list_head; (tmpvpcidev && !found);
+	     tmpvpcidev = tmpvpcidev->next) {
 		if (tmpvpcidev->devtype != devtype)
 			continue;
 
@@ -1000,7 +993,7 @@ static int virtpci_device_serverup(struct device *parentbus,
 {
 	int resumethisone = 0;
 	bool found = false;
-	struct virtpci_dev *tmpvpcidev, *prevvpcidev;
+	struct virtpci_dev *tmpvpcidev;
 	struct virtpci_driver *vpcidriver;
 	unsigned long flags;
 	int rc = 0;
@@ -1012,9 +1005,8 @@ static int virtpci_device_serverup(struct device *parentbus,
 	/* find the vhba or vnic in virtpci device list */
 	write_lock_irqsave(&vpcidev_list_lock, flags);
 
-	for (tmpvpcidev = vpcidev_list_head, prevvpcidev = NULL;
-	     (tmpvpcidev && !found);
-	     prevvpcidev = tmpvpcidev, tmpvpcidev = tmpvpcidev->next) {
+	for (tmpvpcidev = vpcidev_list_head; (tmpvpcidev && !found);
+	     tmpvpcidev = tmpvpcidev->next) {
 		if (tmpvpcidev->devtype != devtype)
 			continue;
 
-- 
1.8.1.2

[PATCH v2 2/4] staging: unisys: dev_t initialization

[Sudip Mukherjee]

dev_t is defined to be of unsigned int type, no use initializing
it to -1.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
v2: it was not in v1

 drivers/staging/unisys/visorchipset/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index 9ca7f1e..e9459af 100644
--- a/drivers/staging/unisys/visorchipset/file.c
+++ b/drivers/staging/unisys/visorchipset/file.c
@@ -30,7 +30,7 @@
 
 static struct cdev file_cdev;
 static struct visorchannel **file_controlvm_channel;
-static dev_t majordev = -1; /**< indicates major num for device */
+static dev_t majordev; /**< indicates major num for device */
 static BOOL registered = FALSE;
 
 static int visorchipset_open(struct inode *inode, struct file *file);
-- 
1.8.1.2

[PATCH v2 3/4] staging: unisys: remove comparison

[Sudip Mukherjee]

MAJOR(majordev) is 0 when dynamic major number is used, and it is
greater than 0 when static major number is used. Considering both the
cases the expression is always true.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: changed commit log

 drivers/staging/unisys/visorchipset/file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index e9459af..e88a215 100644
--- a/drivers/staging/unisys/visorchipset/file.c
+++ b/drivers/staging/unisys/visorchipset/file.c
@@ -82,10 +82,8 @@ visorchipset_file_cleanup(void)
 		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);
+		majordev = MKDEV(0, 0);
 		registered = FALSE;
 	}
 }
-- 
1.8.1.2

[PATCH v2 4/4] staging: unisys: remove forward declarations

[Sudip Mukherjee]

rearranged the functions to remove the forward declarations.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: it was not in v1

 drivers/staging/unisys/visorchipset/file.c | 78 ++++++++++++++----------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
index e88a215..b74a0f2 100644
--- a/drivers/staging/unisys/visorchipset/file.c
+++ b/drivers/staging/unisys/visorchipset/file.c
@@ -33,48 +33,6 @@ static struct visorchannel **file_controlvm_channel;
 static dev_t majordev; /**< indicates major num for device */
 static BOOL registered = FALSE;
 
-static int visorchipset_open(struct inode *inode, struct file *file);
-static int visorchipset_release(struct inode *inode, struct file *file);
-static int visorchipset_mmap(struct file *file, struct vm_area_struct *vma);
-static long visorchipset_ioctl(struct file *file, unsigned int cmd,
-				unsigned long arg);
-
-static const struct file_operations visorchipset_fops = {
-	.owner = THIS_MODULE,
-	.open = visorchipset_open,
-	.read = NULL,
-	.write = NULL,
-	.unlocked_ioctl = visorchipset_ioctl,
-	.release = visorchipset_release,
-	.mmap = visorchipset_mmap,
-};
-
-int
-visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
-{
-	int rc = 0;
-
-	file_controlvm_channel = controlvm_channel;
-	majordev = major_dev;
-	cdev_init(&file_cdev, &visorchipset_fops);
-	file_cdev.owner = THIS_MODULE;
-	if (MAJOR(majordev) == 0) {
-		/* dynamic major device number registration required */
-		if (alloc_chrdev_region(&majordev, 0, 1, MYDRVNAME) < 0)
-			return -1;
-		registered = TRUE;
-	} else {
-		/* static major device number registration required */
-		if (register_chrdev_region(majordev, 1, MYDRVNAME) < 0)
-			return -1;
-		registered = TRUE;
-	}
-	rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
-	if (rc  < 0)
-		return -1;
-	return 0;
-}
-
 void
 visorchipset_file_cleanup(void)
 {
@@ -169,3 +127,39 @@ static long visorchipset_ioctl(struct file *file, unsigned int cmd,
 		return -EFAULT;
 	}
 }
+
+static const struct file_operations visorchipset_fops = {
+	.owner = THIS_MODULE,
+	.open = visorchipset_open,
+	.read = NULL,
+	.write = NULL,
+	.unlocked_ioctl = visorchipset_ioctl,
+	.release = visorchipset_release,
+	.mmap = visorchipset_mmap,
+};
+
+int
+visorchipset_file_init(dev_t major_dev, struct visorchannel **controlvm_channel)
+{
+	int rc = 0;
+
+	file_controlvm_channel = controlvm_channel;
+	majordev = major_dev;
+	cdev_init(&file_cdev, &visorchipset_fops);
+	file_cdev.owner = THIS_MODULE;
+	if (MAJOR(majordev) == 0) {
+		/* dynamic major device number registration required */
+		if (alloc_chrdev_region(&majordev, 0, 1, MYDRVNAME) < 0)
+			return -1;
+		registered = TRUE;
+	} else {
+		/* static major device number registration required */
+		if (register_chrdev_region(majordev, 1, MYDRVNAME) < 0)
+			return -1;
+		registered = TRUE;
+	}
+	rc = cdev_add(&file_cdev, MKDEV(MAJOR(majordev), 0), 1);
+	if (rc  < 0)
+		return -1;
+	return 0;
+}
-- 
1.8.1.2

Re: [PATCH v2 1/4] staging: unisys: remove unused variables

[Greg Kroah-Hartman]

On Thu, Mar 12, 2015 at 11:00:09PM +0530, Sudip Mukherjee wrote:
> these variable were being assigned some values but never reused.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v2: no change

Then why resend?

confused,

greg k-h

Re: [PATCH v2 2/4] staging: unisys: dev_t initialization

[Greg Kroah-Hartman]

On Thu, Mar 12, 2015 at 11:00:10PM +0530, Sudip Mukherjee wrote:
> dev_t is defined to be of unsigned int type, no use initializing
> it to -1.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> v2: it was not in v1
> 
>  drivers/staging/unisys/visorchipset/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
> index 9ca7f1e..e9459af 100644
> --- a/drivers/staging/unisys/visorchipset/file.c
> +++ b/drivers/staging/unisys/visorchipset/file.c
> @@ -30,7 +30,7 @@
>  
>  static struct cdev file_cdev;
>  static struct visorchannel **file_controlvm_channel;
> -static dev_t majordev = -1; /**< indicates major num for device */
> +static dev_t majordev; /**< indicates major num for device */

I don't like this, please fix this up to handle the major number
properly, no need for this -1 mess.  And you just broke the logic with
this change, which isn't allowed in any patch, sorry.

greg k-h

Re: [PATCH v2 1/4] staging: unisys: remove unused variables

[Sudip Mukherjee]

On Mon, Mar 16, 2015 at 04:35:19PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2015 at 11:00:09PM +0530, Sudip Mukherjee wrote:
> > these variable were being assigned some values but never reused.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > v2: no change
> 
> Then why resend?
> 
> confused,
sorry for the confusion. but i was sending v2 of the series and this
patch from that series had no change. so in that case should i mark
the subject of this patch as [PATCH v2] or should it be just
[PATCH] ?

regards
sudip
> 
> greg k-h

Re: [PATCH v2 2/4] staging: unisys: dev_t initialization

[Dan Carpenter]

On Mon, Mar 16, 2015 at 04:36:13PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 12, 2015 at 11:00:10PM +0530, Sudip Mukherjee wrote:
> > dev_t is defined to be of unsigned int type, no use initializing
> > it to -1.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > v2: it was not in v1
> > 
> >  drivers/staging/unisys/visorchipset/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/unisys/visorchipset/file.c b/drivers/staging/unisys/visorchipset/file.c
> > index 9ca7f1e..e9459af 100644
> > --- a/drivers/staging/unisys/visorchipset/file.c
> > +++ b/drivers/staging/unisys/visorchipset/file.c
> > @@ -30,7 +30,7 @@
> >  
> >  static struct cdev file_cdev;
> >  static struct visorchannel **file_controlvm_channel;
> > -static dev_t majordev = -1; /**< indicates major num for device */
> > +static dev_t majordev; /**< indicates major num for device */
> 
> I don't like this, please fix this up to handle the major number
> properly, no need for this -1 mess.  And you just broke the logic with
> this change, which isn't allowed in any patch, sorry.

The patch description is bad and the patch is not complete, but it
doesn't introduce a bug.

The -1 is never used.  By the time we call visorchipset_file_cleanup()
is called then majordev has been initialized to >= 0 so we can delete
the (MAJOR(majordev) >= 0) check.  We can also delete the "registered"
variable because visorchipset_file_cleanup() is never called unless
"registered" is true.

"registered" is set on successful module_init() and
visorchipset_file_cleanup() is only called from module_exit().

There is the problem that visorchipset_init() doesn't release any
resources on error.  But that's a separate issue and using globals like
"registered" is an ugly thing so it doesn't help.

regards,
dan carpenter