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