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

[Hannes Reinecke <hare@xxxxxxx>]

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@xxxxxxx>
> > ---
> >  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@xxxxxxx			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html