[PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley at HansenPartnership.com]
> Sent: Monday, March 19, 2012 12:13 PM
> To: KY Srinivasan
> Cc: gregkh at linuxfoundation.org; linux-kernel at vger.kernel.org;
> devel at linuxdriverproject.org; ohering at suse.com; hch at infradead.org; linux-
> scsi at vger.kernel.org
> Subject: Re: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> when SRB status is INVALID
> 
> On Sun, 2012-03-18 at 17:12 -0700, K. Y. Srinivasan wrote:
> > Currently Windows hosts only support a subset of scsi commands and for
> commands
> > that are not supported, the host returns a generic SRB failure status.
> > However, they have agreed to change the return value to indicate that
> > the command is not supported. In preparation for that, handle the
> > SRB_STATUS_INVALID_REQUEST return value correctly.
> >
> > I would like to thank Jeff Garzik <jgpobox at gmail.com> and
> > Douglas Gilbert <dgilbert at interlog.com> for suggesting the correct approach
> > here.
> >
> > Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz at microsoft.com>
> > ---
> >  drivers/scsi/storvsc_drv.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 44c7a48..018c363 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -202,6 +202,7 @@ enum storvsc_request_type {
> >  #define SRB_STATUS_INVALID_LUN	0x20
> >  #define SRB_STATUS_SUCCESS	0x01
> >  #define SRB_STATUS_ERROR	0x04
> > +#define SRB_STATUS_INVALID_REQUEST 0x06
> 
> I don't really think this is the correct approach.  We already have a
> SCSI error return for this, which you're now translating in the driver
> and hypervisor.  Rather than have a special byte return of
> SRB_STATUS_INVALID_REQUEST, why not have the hypervisor do the right
> thing and fill in the ILLEGAL_REQUEST sense return.  That way you don't
> need a special error code and you don't need to construct the sense
> buffer in the driver.  Now HyperV will be correctly set up for both pass
> through and emulated responses.  It's surely not much work and you
> already process sense data correctly in storvsc_command_completion(), so
> you wouldn't need any patches to the driver for this approach.

James, the issue here is that currently shipping Windows hosts don't even do
what I am handling here. Based on the input I got from you and Christoph,
I convinced the windows team to at least return the SRB status that indicates
an illegal request. I will suggest to them that perhaps they should also set the
correct sense code and so I would not need this patch. However, keep in mind
that there is no current ETA on when Windows will ship with these changes - Windows 8
may ship with code where they would return an invalid SRB status, but they are not 
setting the sense code, hence this patch. When the Window host does the "right thing"
I will clean this up, but I don't know when that will be.

More importantly, the second patch  in this series where I filter out the ATA_16 command
on the guest is really important for us. Without that patch on a range on windows hosts
including the current beta version of windows8 where the host is returning a generic 
error in response to ATA_16 command, we cannot boot many Linux distros. If you
prefer, I can drop the first patch and re-submit the second patch for consideration now.

Regards,

K. Y