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.)
> +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
