Re: [PATCH] staging: unisys: Add s-Par visorhba

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Benjamin Romer <benjamin.romer@unisys.com>
Cc: gregkh@linuxfoundation.org, sparmaintainer@unisys.com,
	driverdev-devel@linuxdriverproject.org, Jes.Sorensen@redhat.com
Subject: Re: [PATCH] staging: unisys: Add s-Par visorhba
Date: Wed, 15 Jul 2015 15:16:18 +0300	[thread overview]
Message-ID: <20150715121617.GE5835@mwanda> (raw)
In-Reply-To: <1436812703-15262-1-git-send-email-benjamin.romer@unisys.com>

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@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel