> -----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
