Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Bart Van Assche <bart.vanassche@sandisk.com>]

On 05/04/15 14:42, Hannes Reinecke wrote:
> +static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
> +{
> +	int retval, err = SCSI_DH_RETRY;
> +	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> +	struct scsi_sense_hdr sense_hdr;
> +
> +	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
> +		/* Only implicit ALUA supported, retry */
> +		return SCSI_DH_RETRY;
> +	}
> +	switch (h->state) {
> +	case TPGS_STATE_OPTIMIZED:
> +		return SCSI_DH_OK;
> +	case TPGS_STATE_NONOPTIMIZED:
> +		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
> +		    (!h->pref) &&
> +		    (h->tpgs & TPGS_MODE_IMPLICIT))
> +			return SCSI_DH_OK;
> +		break;
> +	case TPGS_STATE_STANDBY:
> +	case TPGS_STATE_UNAVAILABLE:
> +		break;
> +	case TPGS_STATE_OFFLINE:
> +		return SCSI_DH_IO;
> +		break;
> +	case TPGS_STATE_TRANSITIONING:
> +		break;
> +	default:
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: stpg failed, unhandled TPGS state %d",
> +			    ALUA_DH_NAME, pg->state);
> +		return SCSI_DH_NOSYS;
> +		break;
> +	}
> +	/* Set state to transitioning */
> +	h->state = TPGS_STATE_TRANSITIONING;
> +	retval = submit_stpg(sdev, h->group_id, sense);
> +
> +	if (retval) {
> +		if (!(driver_byte(retval) & DRIVER_SENSE) ||
> +		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
> +					  &sense_hdr)) {
> +			sdev_printk(KERN_INFO, sdev,
> +				    "%s: stpg failed, result %d",
> +				    ALUA_DH_NAME, retval);
> +			/* Retry RTPG */
> +			return err;
> +		}
> +		err = alua_check_sense(h->sdev, &sense_hdr);
> +		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
> +			    ALUA_DH_NAME);
> +		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
> +		err = SCSI_DH_RETRY;
> +	}
> +	return err;
> +}

The value returned by alua_check_sense() is assigned to the variable 
'err' but that value is not used. Otherwise in this function the 
variable 'err' always has the value SCSI_DH_RETRY. Does that mean that 
that variable can be removed ? Additionally, why is 'retval' only 
printed if normalizing the sense data succeeds and not if normalizing 
the sense data fails ? Has it been considered to print the ASC and ASCQ 
values upon STPG failure ? Having these values available can be a big 
help during debugging.

Thanks,

Bart.