[PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[K. Y. Srinivasan]

Windows hosts don't handle the ATA_16 command; don't pass it to the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 44c7a48..1404c9b 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1221,8 +1221,10 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
 	/*
 	 * smartd sends this command and the host does not handle
 	 * this. So, don't send it.
+	 * Host also does not support ATA_16 command.
 	 */
 	case SET_WINDOW:
+	case ATA_16:
 		scmnd->result = ILLEGAL_REQUEST << 16;
 		allowed = false;
 		break;
-- 
1.7.4.1

Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[Greg KH]

On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Should this go to older kernel versions as well?

greg k-h

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, March 02, 2012 4:14 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; jbottomley@parallels.com;
> hch@infradead.org; linux-scsi@vger.kernel.org; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  drivers/scsi/storvsc_drv.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> Should this go to older kernel versions as well?

I think it should. Do you want me to resend this patch with the correct tag?
Also, given that storvsc has changed so much over the last several months,
this patch may or may not apply to earlier versions of this driver even though
this patch itself is quite trivial.

Regards,

K. Y

Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[Greg KH]

On Fri, Mar 02, 2012 at 09:22:38PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Friday, March 02, 2012 4:14 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org; ohering@suse.com; jbottomley@parallels.com;
> > hch@infradead.org; linux-scsi@vger.kernel.org; Haiyang Zhang
> > Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> > the host
> > 
> > On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > > Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > >  drivers/scsi/storvsc_drv.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > Should this go to older kernel versions as well?
> 
> I think it should. Do you want me to resend this patch with the correct tag?
> Also, given that storvsc has changed so much over the last several months,
> this patch may or may not apply to earlier versions of this driver even though
> this patch itself is quite trivial.

I'll tag it for the stable tree, then when it doesn't apply, you will
get an email saying it didn't, so you can then send me the correct one
:)

thanks,

greg k-h

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, March 02, 2012 4:31 PM
> To: KY Srinivasan
> Cc: linux-scsi@vger.kernel.org; Haiyang Zhang; ohering@suse.com;
> jbottomley@parallels.com; linux-kernel@vger.kernel.org; hch@infradead.org;
> virtualization@lists.osdl.org; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Fri, Mar 02, 2012 at 09:22:38PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Friday, March 02, 2012 4:14 PM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org; ohering@suse.com; jbottomley@parallels.com;
> > > hch@infradead.org; linux-scsi@vger.kernel.org; Haiyang Zhang
> > > Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> > > the host
> > >
> > > On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > > > Windows hosts don't handle the ATA_16 command; don't pass it to the
> host.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > >  drivers/scsi/storvsc_drv.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > Should this go to older kernel versions as well?
> >
> > I think it should. Do you want me to resend this patch with the correct tag?
> > Also, given that storvsc has changed so much over the last several months,
> > this patch may or may not apply to earlier versions of this driver even though
> > this patch itself is quite trivial.
> 
> I'll tag it for the stable tree, then when it doesn't apply, you will
> get an email saying it didn't, so you can then send me the correct one
> :)

Thanks Greg.

K. Y

Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[Christoph Hellwig]

On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> Windows hosts don't handle the ATA_16 command; don't pass it to the host.

Most devices don't handle it, and answer with and unsupported opcode
sense reason.  If hyperv iis buggy enough to crap out on it please add
a comment explaining that.

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Sunday, March 04, 2012 4:12 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> Haiyang Zhang
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> 
> Most devices don't handle it, and answer with and unsupported opcode
> sense reason.  If hyperv iis buggy enough to crap out on it please add
> a comment explaining that.

The host does not "crap out", it does return an error code but it is not "unsupported opcode".
The sense reason that comes back is a generic error SRB_STATUS code. It is easier for me to filter the
command on the outgoing side as opposed to dealing with a generic error code that is coming back from
the host.

Regards,

K. Y 

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[James Bottomley]

On Sun, 2012-03-04 at 14:23 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:hch@infradead.org]
> > Sent: Sunday, March 04, 2012 4:12 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> > Haiyang Zhang
> > Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> > the host
> > 
> > On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > > Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> > 
> > Most devices don't handle it, and answer with and unsupported opcode
> > sense reason.  If hyperv iis buggy enough to crap out on it please add
> > a comment explaining that.
> 
> The host does not "crap out", it does return an error code but it is not "unsupported opcode".
> The sense reason that comes back is a generic error SRB_STATUS code. It is easier for me to filter the
> command on the outgoing side as opposed to dealing with a generic error code that is coming back from
> the host.

That's the wrong thing to do ... you need to unwrap the error code.
The reason being I presume it's not impossible for Windows to host a
device supporting ATA_16 and there are signs that this is going to be
necessary to prevent data corruption on some USB devices ... if you just
filter the command without checking if the host supports it, you're
going to end up perpetuating the corruption problem.

The general rule of thumb for avoiding this is to let the lower layers
handle as much as possible, and only begin behaviour alterations in the
upper layers if the lower layers have a provable and usually fatal
failure.

James

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Sunday, March 04, 2012 9:49 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; linux-scsi@vger.kernel.org;
> Haiyang Zhang
> Subject: RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Sun, 2012-03-04 at 14:23 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > Sent: Sunday, March 04, 2012 4:12 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> > > Haiyang Zhang
> > > Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> > > the host
> > >
> > > On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > > > Windows hosts don't handle the ATA_16 command; don't pass it to the
> host.
> > >
> > > Most devices don't handle it, and answer with and unsupported opcode
> > > sense reason.  If hyperv iis buggy enough to crap out on it please add
> > > a comment explaining that.
> >
> > The host does not "crap out", it does return an error code but it is not
> "unsupported opcode".
> > The sense reason that comes back is a generic error SRB_STATUS code. It is
> easier for me to filter the
> > command on the outgoing side as opposed to dealing with a generic error code
> that is coming back from
> > the host.
> 
> That's the wrong thing to do ... you need to unwrap the error code.

I will see if this is even possible based on the current error codes I get back.

> The reason being I presume it's not impossible for Windows to host a
> device supporting ATA_16 and there are signs that this is going to be
> necessary to prevent data corruption on some USB devices ... if you just
> filter the command without checking if the host supports it, you're
> going to end up perpetuating the corruption problem.

We are talking of virtual  block devices exposed to Linux guests running on a Windows
hosts. I don't think they will ever need to support ATA_16 command on these virtual block
devices. I will however confirm with the Windows team.


Regards,

K. Y

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Sunday, March 04, 2012 9:49 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; linux-scsi@vger.kernel.org;
> Haiyang Zhang
> Subject: RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Sun, 2012-03-04 at 14:23 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > Sent: Sunday, March 04, 2012 4:12 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> > > Haiyang Zhang
> > > Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> > > the host
> > >
> > > On Fri, Mar 02, 2012 at 12:49:07PM -0800, K. Y. Srinivasan wrote:
> > > > Windows hosts don't handle the ATA_16 command; don't pass it to the
> host.
> > >
> > > Most devices don't handle it, and answer with and unsupported opcode
> > > sense reason.  If hyperv iis buggy enough to crap out on it please add
> > > a comment explaining that.
> >
> > The host does not "crap out", it does return an error code but it is not
> "unsupported opcode".
> > The sense reason that comes back is a generic error SRB_STATUS code. It is
> easier for me to filter the
> > command on the outgoing side as opposed to dealing with a generic error code
> that is coming back from
> > the host.
> 
> That's the wrong thing to do ... you need to unwrap the error code.
> The reason being I presume it's not impossible for Windows to host a
> device supporting ATA_16 and there are signs that this is going to be
> necessary to prevent data corruption on some USB devices ... if you just
> filter the command without checking if the host supports it, you're
> going to end up perpetuating the corruption problem.

James,

Currently, the Windows host does not provide me with granular error codes to 
distinguish between the case where the operation failed and the case where it is
an illegal or unsupported operation. I have asked the windows team to return more
appropriate error codes and they may do that sometime soon. In the interim, filtering
the command on the outgoing path is the best way I can deal with this issue. Currently,
even on the host side they are filtering this command, except they are returning a failure
to the guest instead of a more appropriate error code.
While my proposal has been accepted for windows 8, I don't know when this change will ship.
Also, it is not clear when prior versions of Windows hosts that we care about will pick up this
change. This patch is the only way I know how to deal with this problems within my current
constraints.

Regards,

K. Y


> 
> The general rule of thumb for avoiding this is to let the lower layers
> handle as much as possible, and only begin behaviour alterations in the
> upper layers if the lower layers have a provable and usually fatal
> failure.
> 
> James
> 
> 

Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[James Bottomley]

On Fri, 2012-03-02 at 12:49 -0800, K. Y. Srinivasan wrote:
> Windows hosts don't handle the ATA_16 command; don't pass it to the host.

What do you mean by this?  Most SCSI devices don't handle ATA_16 because
it's only useful if the device is actually an ATA device behind a
bridge.

As a general rule, you shouldn't filter in the driver commands you think
the host won't want to see, you should let the host reply with an error
code.  You should *only* filter commands that cause an actual bug in the
host.  Is the latter the case for this command (if so that needs to be
explained)?

> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

This signoff chain doesn't make sense, since you're sending me the patch
and apparently you authored it.

James

RE: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Sunday, March 04, 2012 9:20 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On Fri, 2012-03-02 at 12:49 -0800, K. Y. Srinivasan wrote:
> > Windows hosts don't handle the ATA_16 command; don't pass it to the host.
> 
> What do you mean by this?  Most SCSI devices don't handle ATA_16 because
> it's only useful if the device is actually an ATA device behind a
> bridge.
> 
> As a general rule, you shouldn't filter in the driver commands you think
> the host won't want to see, you should let the host reply with an error
> code.  You should *only* filter commands that cause an actual bug in the
> host.  Is the latter the case for this command (if so that needs to be
> explained)?

As I explained in my email to Christoph, the Windows host returns error codes that
I cannot properly decode as being "unsupported opcode". Let me investigate a little more
and see if I can properly analyze the failure and return the correct error code up. If the error code
I get back from Windows is such that I cannot deduce the cause of the failure, then apart from filtering
the commands in the outgoing path, I am not sure what my options are.
 
> 
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> This signoff chain doesn't make sense, since you're sending me the patch
> and apparently you authored it.

Good point. Since I came to MSFT, this has been the practice here - 
all the patches were reviewed and "signed-off" by all the relevant developers
independent of the authorship and Greg seemed ok with it and has been ascribing
ownership based on the person sending the patch who is always the author of the
patch. How would you recommend we change this practice. 

Regards,

K. Y