From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: "Kershner, David A" Subject: RE: [PATCH] staging: unisys: Add s-Par visorhba Date: Wed, 15 Jul 2015 16:53:24 +0000 Message-ID: <63123ea0c059467c9cb7bd20a6cc2247@US-EXCH13-1.na.uis.unisys.com> References: <1436812703-15262-1-git-send-email-benjamin.romer@unisys.com> <20150715121617.GE5835@mwanda> In-Reply-To: <20150715121617.GE5835@mwanda> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 List-ID: To: Dan Carpenter , "Romer, Benjamin M" Cc: "gregkh@linuxfoundation.org" , "driverdev-devel@linuxdriverproject.org" , "Jes.Sorensen@redhat.com" , *S-Par-Maintainer > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: Wednesday, July 15, 2015 8:16 AM > To: Romer, Benjamin M > Cc: gregkh@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; > Jes.Sorensen@redhat.com; *S-Par-Maintainer > Subject: Re: [PATCH] staging: unisys: Add s-Par visorhba >=20 > Since you are redoing this anyway... >=20 > On Mon, Jul 13, 2015 at 02:38:23PM -0400, Benjamin Romer wrote: > > + for (vdisk =3D &devdata->head; vdisk->next; vdisk =3D vdisk->next) { > > + if ((scsidev->channel =3D=3D vdisk->channel) && > > + (scsidev->id =3D=3D vdisk->id) && > > + (scsidev->lun =3D=3D 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); > > + } > > + } >=20 >=20 > We do this loop all the time, and we're hitting the 80 character > limit. Make it a define. >=20 > #define for_each_vdisk_match(iter, list, match) \ > for (iter =3D &list->head; iter->next; iter =3D iter->next) \ > if (iter->channel =3D=3D match->channel && \ > iter->id =3D=3D match->id && \ > iter->lun =3D=3D match->lun) >=20 > Btw, avoid using too many parenthesis. It makes the code harder to read > and it silences GCC's check for =3D=3D vs =3D typos so it can lead to bug= s. >=20 > Now the loop looks like: >=20 > 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); >=20 > } >=20 > (Written in email client. Caveat emptor.) Thanks for the comments and I really like this idea. When I put it in the c= ode,=20 I get the following from checkpatch:=20 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 =3D &list->head; iter->next; iter =3D iter->next) \ + if (iter->channel =3D=3D match->channel && \ + iter->id =3D=3D match->id && \ + iter->lun =3D=3D 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? >=20 > > +static int > > +visorhba_queue_command_lck(struct scsi_cmnd *scsicmd, > > + void (*visorhba_cmnd_done)(struct scsi_cmnd *)) > > +{ > > + struct scsi_device *scsidev =3D scsicmd->device; > > + int insert_location; > > + unsigned char op; > > + unsigned char *cdb =3D scsicmd->cmnd; > > + struct Scsi_Host *scsihost =3D scsidev->host; > > + struct uiscmdrsp *cmdrsp; > > + unsigned int i; > > + struct visorhba_devdata *devdata =3D > > + (struct visorhba_devdata *)scsihost->hostdata; > > + struct scatterlist *sg =3D NULL; > > + struct scatterlist *sglist =3D NULL; > > + int err =3D 0; > > + > > + if (devdata->serverdown || devdata->serverchangingstate) > > + return SCSI_MLQUEUE_DEVICE_BUSY; > > + > > + cmdrsp =3D 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 =3D CMD_SCSI_TYPE; > > + /* save the pending insertion location. Deletion from pending > > + * will return the scsicmd pointer for completion > > + */ > > + insert_location =3D > > + add_scsipending_entry(devdata, CMD_SCSI_TYPE, (void > *)scsicmd); > > + if (insert_location !=3D -1) { > > + cmdrsp->scsi.scsicmd =3D (void *)(uintptr_t)insert_location; > > + } else { > > + kfree(cmdrsp); >=20 > This kfree in the middle of the function is weird. >=20 > > + return SCSI_MLQUEUE_DEVICE_BUSY; > > + } >=20 > 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. >=20 > https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ >=20 > Instead of trying to match the existing buggy style, just adopt normal > kernel style. >=20 > regards, > dan carpenter