Re: [PATCH 6/7] scsi_dh: Update RDAC device handler

[Hannes Reinecke <hare@suse.de>]

Hi Chandra,

Chandra Seetharaman wrote:
> On Wed, 2008-05-14 at 16:43 +0200, Hannes Reinecke wrote:
>> This patch updates the RDAC device handler to
>> refuse to attach to devices not supporting the
>> RDAC vpd pages.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/device_handler/scsi_dh_rdac.c |   84 +++++++++++++++++----------
>>  1 files changed, 53 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
>> index e61cde6..dd9f515 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
[ .. ]
>>  		/*
>>  		 * If in AVT mode or if the path already owns the LUN,
>>  		 * return RDAC_OWNED;
>>  		 */
> 
> With the code change below the comment above is incorrect, please
> remove.
OK.

>> @@ -478,24 +491,9 @@ static int rdac_activate(struct scsi_device *sdev)
>>  	struct rdac_dh_data *h = get_rdac_data(sdev);
>>  	int err = SCSI_DH_OK;
>>
>> -	if (h->lun == UNINITIALIZED_LUN) {
>> -		err = get_lun(sdev);
>> -		if (err != SCSI_DH_OK)
>> -			goto done;
>> -	}
>> -
>>  	err = check_ownership(sdev);
>> -	switch (err) {
>> -	case RDAC_UNOWNED:
>> -		break;
>> -	case RDAC_OWNED:
>> -		err = SCSI_DH_OK;
>> -		goto done;
>> -	case RDAC_FAILED:
>> -	default:
>> -		err = SCSI_DH_IO;
> 
> What does this change yield ? (under check_ownership)
> 
We're now setting the lun state explicitly, so there's
no need to return different error codes.

>> +	if (err != SCSI_DH_OK)
>>  		goto done;
>> -	}
>>
>>  	if (!h->ctlr) {
>>  		err = initialize_controller(sdev);
>> @@ -508,8 +506,9 @@ static int rdac_activate(struct scsi_device *sdev)
>>  		if (err != SCSI_DH_OK)
>>  			goto done;
>>  	}
>> -
>> -	err = send_mode_select(sdev);
>> +	if (h->lun_state != RDAC_LUN_AVT &&
>> +	    !(h->lun_state & RDAC_LUN_OWNED))
> 
> This can be simplified by (h->lun_state == RDAC_LUN_UNOWNED) ?

Indeed,

>> +		err = send_mode_select(sdev);
>>  done:
>>  	return err;
>>  }
>> @@ -606,6 +605,7 @@ static int rdac_bus_attach(struct scsi_device *sdev)
>>  	struct scsi_dh_data *scsi_dh_data;
>>  	struct rdac_dh_data *h;
>>  	unsigned long flags;
>> +	int err;
>>
>>  	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
>>  			       + sizeof(*h) , GFP_KERNEL);
>> @@ -622,11 +622,33 @@ static int rdac_bus_attach(struct scsi_device *sdev)
>>  	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
>>  	sdev->scsi_dh_data = scsi_dh_data;
>>  	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> 
> need an initialization for lun_state.
> 
No. lun_state is initialized to '0', ie RDAC_LUN_UNOWNED.
I don't think we need a separate 'uninitialized' state here.

>> +
>> +	err = get_lun(sdev);
>> +	if (err != SCSI_DH_OK)
>> +		goto failed;
>> +
>> +	err = check_ownership(sdev);
>> +	if (err != SCSI_DH_OK)
>> +		goto failed;
>> +
>> +	sdev_printk(KERN_NOTICE, sdev,
>> +		    "%s: LUN %d (state %d)\n",
>> +		    RDAC_NAME, h->lun, h->lun_state);
> 
> instead of printing lun_state as %d it would be more readable it is a
> string.
> 
Ok.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)