Patchwork [#upstream-fixes] libata: clear PIO pad area

login
register
mail settings
Submitter Tejun Heo
Date Sept. 6, 2011, 4:09 a.m.
Message ID <20110906040905.GD18425@mtj.dyndns.org>
Download mbox | patch
Permalink /patch/113481/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - Sept. 6, 2011, 4:09 a.m.
ata_sff_data_xfer[32]() use pad area if the transfer size isn't
multiple of transfer size; however, this area wasn't cleared and
garbage data in pad area could be transferred to the device.  Make
sure the pad area is cleared.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Lei Ming <tom.leiming@gmail.com>
---
 drivers/ata/libata-sff.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Sept. 6, 2011, 10:06 a.m.
Hello.

On 06-09-2011 8:09, Tejun Heo wrote:

> ata_sff_data_xfer[32]() use pad area if the transfer size isn't
> multiple of transfer size; however, this area wasn't cleared and
> garbage data in pad area could be transferred to the device.  Make
> sure the pad area is cleared.

> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Lei Ming<tom.leiming@gmail.com>

    And what's the problem with garbage data? Why it's worse than 0s?

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Sept. 6, 2011, 11:16 a.m.
On Tue, 06 Sep 2011 14:06:23 +0400
Sergei Shtylyov <sshtylyov@mvista.com> wrote:

> Hello.
> 
> On 06-09-2011 8:09, Tejun Heo wrote:
> 
> > ata_sff_data_xfer[32]() use pad area if the transfer size isn't
> > multiple of transfer size; however, this area wasn't cleared and
> > garbage data in pad area could be transferred to the device.  Make
> > sure the pad area is cleared.
> 
> > Signed-off-by: Tejun Heo<tj@kernel.org>
> > Cc: Lei Ming<tom.leiming@gmail.com>
> 
>     And what's the problem with garbage data? Why it's worse than 0s?

It's potentially externally observable on things like an eSATA port so
it probably should be cleared just as we do with network padding.

For the CDB case we clear the rest of the CDB anyway I believe because
some old PATA stuff used to freak if the padding wasn't zero.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Sept. 6, 2011, 4:58 p.m.
Hello,

On Tue, Sep 06, 2011 at 02:06:23PM +0400, Sergei Shtylyov wrote:
> On 06-09-2011 8:09, Tejun Heo wrote:
> 
> >ata_sff_data_xfer[32]() use pad area if the transfer size isn't
> >multiple of transfer size; however, this area wasn't cleared and
> >garbage data in pad area could be transferred to the device.  Make
> >sure the pad area is cleared.
> 
> >Signed-off-by: Tejun Heo<tj@kernel.org>
> >Cc: Lei Ming<tom.leiming@gmail.com>
> 
>    And what's the problem with garbage data? Why it's worse than 0s?

There were devices which puke on random garbage at the end of CDB and
in general sending indeterministic garbage to external device is a bad
idea.  It increases chance of problems which can be difficult to
reproduce and track down while gaining nothing.  It really is a stupid
thing to do.

Thanks.
Sergei Shtylyov - Sept. 6, 2011, 5:23 p.m.
Hello.

On 09/06/2011 08:58 PM, Tejun Heo wrote:

>>> ata_sff_data_xfer[32]() use pad area if the transfer size isn't
>>> multiple of transfer size; however, this area wasn't cleared and
>>> garbage data in pad area could be transferred to the device.  Make
>>> sure the pad area is cleared.

>>> Signed-off-by: Tejun Heo<tj@kernel.org>
>>> Cc: Lei Ming<tom.leiming@gmail.com>

>>     And what's the problem with garbage data? Why it's worse than 0s?

> There were devices which puke on random garbage at the end of CDB and

    Note that ATAPI CDBs are always aligned to 4 bytes, so this path should 
never be hit when sending them.

> in general sending indeterministic garbage to external device is a bad
> idea.  It increases chance of problems which can be difficult to
> reproduce and track down while gaining nothing.

    Problems with "don't care" padding bytes? Dunno...

> It really is a stupid thing to do.

    Well, at least the old code was equally stupid before I patched it. :-)

> Thanks.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Sept. 6, 2011, 5:28 p.m.
Hello.

On 09/06/2011 09:23 PM, Sergei Shtylyov wrote:

>>>> ata_sff_data_xfer[32]() use pad area if the transfer size isn't
>>>> multiple of transfer size; however, this area wasn't cleared and
>>>> garbage data in pad area could be transferred to the device. Make
>>>> sure the pad area is cleared.

>>>> Signed-off-by: Tejun Heo<tj@kernel.org>
>>>> Cc: Lei Ming<tom.leiming@gmail.com>

>>> And what's the problem with garbage data? Why it's worse than 0s?

>> There were devices which puke on random garbage at the end of CDB and

> Note that ATAPI CDBs are always aligned to 4 bytes, so this path should never be
> hit when sending them.

>> in general sending indeterministic garbage to external device is a bad
>> idea. It increases chance of problems which can be difficult to
>> reproduce and track down while gaining nothing.

> Problems with "don't care" padding bytes? Dunno...

>> It really is a stupid thing to do.

> Well, at least the old code was equally stupid before I patched it. :-)

    Only in ata_sff_data_xfer32(). It was I who removed the padding buffer 
initialization in ata_sff_data_xfer()...

>> Thanks.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Sept. 6, 2011, 5:30 p.m.
On Tue, Sep 06, 2011 at 09:23:01PM +0400, Sergei Shtylyov wrote:
> >There were devices which puke on random garbage at the end of CDB and
> 
>    Note that ATAPI CDBs are always aligned to 4 bytes, so this path
> should never be hit when sending them.

Ah, okay, but still, let's just clear it.  There's nothing to be
gained by not clearing several bytes there.

> >in general sending indeterministic garbage to external device is a bad
> >idea.  It increases chance of problems which can be difficult to
> >reproduce and track down while gaining nothing.
> 
>    Problems with "don't care" padding bytes? Dunno...

Oh yeah, IIRC there were multiple devices which puked on "don't care"
garbage bytes in CDB.  Don't underestimate the crappiness of cheap
ATAPI devices. :)

Thanks.

Patch

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index c24127d..2487ea7 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -569,7 +569,7 @@  unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
 
 	/* Transfer trailing byte, if any. */
 	if (unlikely(buflen & 0x01)) {
-		unsigned char pad[2];
+		unsigned char pad[2] = { };
 
 		/* Point buf to the tail of buffer */
 		buf += buflen - 1;
@@ -628,7 +628,7 @@  unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
 
 	/* Transfer trailing bytes, if any */
 	if (unlikely(slop)) {
-		unsigned char pad[4];
+		unsigned char pad[4] = { };
 
 		/* Point buf to the tail of buffer */
 		buf += buflen - slop;