> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Wednesday, July 15, 2015 8:16 AM
> To: Romer, Benjamin M
> Cc: gre...@linuxfoundation.org; driverdev-devel@linuxdriverproject.org;
> jes.soren...@redhat.com; *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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to