RE: [PATCH] staging: unisys: Add s-Par visorhba

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
> Sent: Wednesday, July 15, 2015 8:16 AM
> To: Romer, Benjamin M
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx;
> Jes.Sorensen@xxxxxxxxxx; *S-Par-Maintainer
> Subject: Re: [PATCH] staging: unisys: Add s-Par visorhba
> 
> Since you are redoing this anyway...
> 
> On Mon, Jul 13, 2015 at 02:38:23PM -0400, Benjamin Romer wrote:
> > +	for (vdisk = &devdata->head; vdisk->next; vdisk = vdisk->next) {
> > +		if ((scsidev->channel == vdisk->channel) &&
> > +		    (scsidev->id == vdisk->id) &&
> > +		    (scsidev->lun == vdisk->lun)) {
> > +			if (atomic_read(&vdisk->error_count) <
> > +			    VISORHBA_ERROR_COUNT)
> > +				atomic_inc(&vdisk->error_count);
> > +			else
> > +				atomic_set(&vdisk->ios_threshold,
> > +					   IOS_ERROR_THRESHOLD);
> > +		}
> > +	}
> 
> 
> We do this loop all the time, and we're hitting the 80 character
> limit.  Make it a define.
> 
> #define for_each_vdisk_match(iter, list, match)                 \
> 	for (iter = &list->head; iter->next; iter = iter->next) \
> 		if (iter->channel == match->channel &&          \
> 		    iter->id == match->id &&                    \
> 		    iter->lun == match->lun)
> 
> Btw, avoid using too many parenthesis.  It makes the code harder to read
> and it silences GCC's check for == vs = typos so it can lead to bugs.
> 
> Now the loop looks like:
> 
> 	for_each_vdisk_match(vdisk, devdata, scsidev) {
> 		if (atomic_read(&vdisk->error_count) <
> VISORHBA_ERROR_COUNT)
> 			atomic_inc(&vdisk->error_count);
> 		else
> 			atomic_set(&vdisk->ios_threshold,
> IOS_ERROR_THRESHOLD);
> 
> 	}
> 
> (Written in email client.  Caveat emptor.)

Thanks for the comments and I really like this idea. When I put it in the code, 
I get the following from checkpatch: 

ERROR: Macros with complex values should be enclosed in parentheses
#31: FILE: drivers/staging/unisys/visorhba/visorhba_main.c:157:
+#define for_each_vdisk_match(iter, list, match)                          \
+       for (iter = &list->head; iter->next; iter = iter->next) \
+               if (iter->channel == match->channel &&            \
+                   iter->id == match->id &&                      \
+                   iter->lun == match->lun)

total: 1 errors, 0 warnings, 0 checks, 275 lines checked

Your patch has style problems, please review.

Any ideas what needs to be wrapped to resolved the checkpatch error?

> 
> > +static int
> > +visorhba_queue_command_lck(struct scsi_cmnd *scsicmd,
> > +			   void (*visorhba_cmnd_done)(struct scsi_cmnd *))
> > +{
> > +	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;
> > +	unsigned int i;
> > +	struct visorhba_devdata *devdata =
> > +		(struct visorhba_devdata *)scsihost->hostdata;
> > +	struct scatterlist *sg = NULL;
> > +	struct scatterlist *sglist = NULL;
> > +	int err = 0;
> > +
> > +	if (devdata->serverdown || devdata->serverchangingstate)
> > +		return SCSI_MLQUEUE_DEVICE_BUSY;
> > +
> > +	cmdrsp = kzalloc(sizeof(*cmdrsp), GFP_ATOMIC);
> > +	if (!cmdrsp)
> > +		return -ENOMEM;
> > +
> > +	/* now saving everything we need from scsi_cmd into cmdrsp
> > +	 * before we queue cmdrsp set type to command - as opposed to
> > +	 * task mgmt
> > +	 */
> > +	cmdrsp->cmdtype = CMD_SCSI_TYPE;
> > +	/* save the pending insertion location. Deletion from pending
> > +	 * will return the scsicmd pointer for completion
> > +	 */
> > +	insert_location =
> > +		add_scsipending_entry(devdata, CMD_SCSI_TYPE, (void
> *)scsicmd);
> > +	if (insert_location != -1) {
> > +		cmdrsp->scsi.scsicmd = (void *)(uintptr_t)insert_location;
> > +	} else {
> > +		kfree(cmdrsp);
> 
> This kfree in the middle of the function is weird.
> 
> > +		return SCSI_MLQUEUE_DEVICE_BUSY;
> > +	}
> 
> The Spar driver tends to have one error label on the end of each
> function and it has had very buggy error handling...  I wrote a google
> plus post on how to do error handling.
> 
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> Instead of trying to match the existing buggy style, just adopt normal
> kernel style.
> 
> regards,
> dan carpenter

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux