[PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

PIONEER DVD-RW DVRTD08 times out SETXFER if no media is present.  The
device is SATA and simply skipping SETXFER works around the problem.
Implement ATA_HORKAGE_NOSETXFER and apply it to the device.

Reported by Moritz Rigler in the following thread.

  http://thread.gmane.org/gmane.linux.ide/36790

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |   10 ++++++++--
 include/linux/libata.h    |    1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fecca42..eceaace 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3310,14 +3310,17 @@ static int ata_dev_set_mode(struct ata_device *dev)
 	struct ata_eh_context *ehc = &dev->link->eh_context;
 	const char *dev_err_whine = "";
 	int ign_dev_err = 0;
-	unsigned int err_mask;
+	unsigned int err_mask = 0;
 	int rc;
 
 	dev->flags &= ~ATA_DFLAG_PIO;
 	if (dev->xfer_shift == ATA_SHIFT_PIO)
 		dev->flags |= ATA_DFLAG_PIO;
 
-	err_mask = ata_dev_set_xfermode(dev);
+	if (!(dev->horkage & ATA_HORKAGE_NOSETXFER))
+		err_mask = ata_dev_set_xfermode(dev);
+	else
+		dev_err_whine = " (SET_XFERMODE skipped)";
 
 	if (err_mask & ~AC_ERR_DEV)
 		goto fail;
@@ -4207,6 +4210,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },
 
+	/* Devices which choke on SETXFER.  Presumably SATA only. */
+	{ "PIONEER DVD-RW  DVRTD08",	"1.00",	ATA_HORKAGE_NOSETXFER },
+
 	/* End Marker */
 	{ }
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3449de5..e78dc6b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -377,6 +377,7 @@ enum {
 	ATA_HORKAGE_ATAPI_MOD16_DMA = (1 << 11), /* use ATAPI DMA for commands
 						    not multiple of 16 bytes */
 	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firwmare update warning */
+	ATA_HORKAGE_NOSETXFER	= (1 << 13),
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

On Wed, 07 Jan 2009 11:01:05 +0900
Tejun Heo <tj@kernel.org> wrote:

> PIONEER DVD-RW DVRTD08 times out SETXFER if no media is present.  The
> device is SATA and simply skipping SETXFER works around the problem.
> Implement ATA_HORKAGE_NOSETXFER and apply it to the device.

NAK

Sorry you can't just blindly do this because some of the controllers snoop
the SETXFER command to set their timings and whether they expect DMA. Also
we've no idea if this is a bug in a specific firmware revision, a quirky
pata/sata bridge or a timing problem of some sort.

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Alan Cox wrote:
> On Wed, 07 Jan 2009 11:01:05 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> PIONEER DVD-RW DVRTD08 times out SETXFER if no media is present.  The
>> device is SATA and simply skipping SETXFER works around the problem.
>> Implement ATA_HORKAGE_NOSETXFER and apply it to the device.
> 
> NAK
> 
> Sorry you can't just blindly do this because some of the controllers snoop
> the SETXFER command to set their timings and whether they expect DMA. Also
> we've no idea if this is a bug in a specific firmware revision, a quirky
> pata/sata bridge or a timing problem of some sort.

I think it'll generally be okay for SATA unless it's bridged over to
PATA controller.  Any other ideas?

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> > Sorry you can't just blindly do this because some of the controllers snoop
> > the SETXFER command to set their timings and whether they expect DMA. Also
> > we've no idea if this is a bug in a specific firmware revision, a quirky
> > pata/sata bridge or a timing problem of some sort.
> 
> I think it'll generally be okay for SATA unless it's bridged over to
> PATA controller.  Any other ideas?

We seem to have one report, from one user, with one configuration, on one
controller, using one firmware set.

That to me isn't meaningful evidence of anything that needs a workaround,
beyond maybe doing what old IDE does and if a setxfer times out
continuing in hope.

(We should of course then re-read the identify pages and check the mode
in use)

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Alan Cox wrote:
>>> Sorry you can't just blindly do this because some of the controllers snoop
>>> the SETXFER command to set their timings and whether they expect DMA. Also
>>> we've no idea if this is a bug in a specific firmware revision, a quirky
>>> pata/sata bridge or a timing problem of some sort.
>> I think it'll generally be okay for SATA unless it's bridged over to
>> PATA controller.  Any other ideas?
> 
> We seem to have one report, from one user, with one configuration, on one
> controller, using one firmware set.

There are two more reports linked from the thread.

  http://ubuntuforums.org/showthread.php?t=986871
  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/299865

And all of them are showing the same problem.  This being a slim SATA
drive for a laptop, I don't really expect to see the drive in varied
configurations.  The problem might as well be limited to certain OEM
drive(s).

> That to me isn't meaningful evidence of anything that needs a workaround,
> beyond maybe doing what old IDE does and if a setxfer times out
> continuing in hope.

So, as far as the validity of the report goes, I think it's at
reasonable level.  Add to that my general trigger happiness toward
quirks and the machine is a VAIO and to me quirking it doesn't seem
too careless.

> (We should of course then re-read the identify pages and check the mode
> in use)

Of course, whether the workaround is proper is a completely separate
issue but unless other devices with the same problem creep up and the
device is happy with the quirk, I don't really think we should modify
the default configuration sequence for devices like this.

That said, Moritz, can you please post the output of "hdparm -I" with
and without the quirk applied?  And can you please use a preoper mail
address?

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Moritz Rigler]

>There are two more reports linked from the thread.

>  http://ubuntuforums.org/showthread.php?t=986871
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/299865

There are also
https://bugs.launchpad.net/ubuntu/+bug/305724
http://ubuntuforums.org/showthread.php?t=1031170
http://forum.ubuntu-it.org/index.php?topic=230995.msg1741414
The problem appears at least on two models (Sony Vaio FW and Sony Vaio VGN-NS11S). So I really don't agree with your "one report, from one user, with one configuration, on one controller, using one firmware set" statement, Alan.

I think that the particular laptop model I have (Sony Vaio VGN-NS11S) has sold in large numbers only recently. So if you decide to do nothing there will likely be more reports soon. If you wait for that to happen a good share of the users encountering the problem will (unjustly) blame it on Linux and just turn back to using preinstalled Vista (where the drive works). I've already thought myself that it was perhaps unwise to run Linux on a newly bought computer (where issues like this have not been fixed yet).

>That said, Moritz, can you please post the output of "hdparm -I" with
>and without the quirk applied?  
unpatched:
http://article.gmane.org/gmane.linux.ide/36819
patched:
http://article.gmane.org/gmane.linux.ide/36840

>And can you please use a preoper mail
>address?
Hope you like this one better.

Regards,
Moritz
-- 
Sensationsangebot verlängert: GMX FreeDSL - Telefonanschluss + DSL 
für nur 16,37 Euro/mtl.!* http://dsl.gmx.de/?ac=OM.AD.PD003K1308T4569a

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Moritz Rigler]

>There are two more reports linked from the thread.

>  http://ubuntuforums.org/showthread.php?t=986871
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/299865

There are also
https://bugs.launchpad.net/ubuntu/+bug/305724
http://ubuntuforums.org/showthread.php?t=1031170
http://forum.ubuntu-it.org/index.php?topic=230995.msg1741414
The problem appears at least on two models (Sony Vaio FW and Sony Vaio VGN-NS11S). So I really don't agree with your "one report, from one user, with one configuration, on one controller, using one firmware set" statement, Alan.

I think that the particular laptop model I have (Sony Vaio VGN-NS11S) has sold in large numbers only recently. So if you decide to do nothing there will likely be more reports soon. If you wait for that to happen a good share of the users encountering the problem will (unjustly) blame it on Linux and just turn back to using preinstalled Vista (where the drive works). I've already thought myself that it was perhaps unwise to run Linux on a newly bought computer (where issues like this have not been fixed yet).

>That said, Moritz, can you please post the output of "hdparm -I" with
>and without the quirk applied?  
unpatched:
http://article.gmane.org/gmane.linux.ide/36819
patched:
http://article.gmane.org/gmane.linux.ide/36840

>And can you please use a preoper mail
>address?
Hope you like this one better.

Regards,
Moritz


-- 
Sensationsangebot verlängert: GMX FreeDSL - Telefonanschluss + DSL 
für nur 16,37 Euro/mtl.!* http://dsl.gmx.de/?ac=OM.AD.PD003K1308T4569a

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> The problem appears at least on two models (Sony Vaio FW and Sony Vaio VGN-NS11S). So I really don't agree with you "one report, from one user, with one configuration, on one controller, using one firmware set" statement, Alan.

Thanks - yours was the only one I had seen references too. I don't spend
my days reading the Ubuntu forums.

> (where issues like this have not been fixed yet).

Its probably a better idea to buy a computer that appears to work in the
way it is expected yes. However there is more to making a good quality
OS than running around flapping at every warped piece of hardware we
meet. It would be good if this works but it would also be good to know
therefore why it happens to work in Vista - are the delays longer, are
they clearing some other undefined command bits that the spec says don't
matter etc.

Simply saying 'oh it didn't work lets just pretend we don't care' isn't
always a good idea, particuarly when the worst case failure for it on
other devices where it indicates a real failing might be corruption of
user data.

Yes - I'd like it to just work, but I'd like it to just work without
breaking anything else, without adding device specific special cases and
ideally in a way that makes other stuff that is similarly odd just work
too.

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Robert Hancock]

Alan Cox wrote:
>> The problem appears at least on two models (Sony Vaio FW and Sony Vaio VGN-NS11S). So I really don't agree with you "one report, from one user, with one configuration, on one controller, using one firmware set" statement, Alan.
> 
> Thanks - yours was the only one I had seen references too. I don't spend
> my days reading the Ubuntu forums.
> 
>> (where issues like this have not been fixed yet).
> 
> Its probably a better idea to buy a computer that appears to work in the
> way it is expected yes. However there is more to making a good quality
> OS than running around flapping at every warped piece of hardware we
> meet. It would be good if this works but it would also be good to know
> therefore why it happens to work in Vista - are the delays longer, are
> they clearing some other undefined command bits that the spec says don't
> matter etc.
> 
> Simply saying 'oh it didn't work lets just pretend we don't care' isn't
> always a good idea, particuarly when the worst case failure for it on
> other devices where it indicates a real failing might be corruption of
> user data.
> 
> Yes - I'd like it to just work, but I'd like it to just work without
> breaking anything else, without adding device specific special cases and
> ideally in a way that makes other stuff that is similarly odd just work
> too.

I wonder if whatever storage driver Sony has it set up to run with in 
Vista is just skipping the set transfer mode command on SATA devices?

If you guys still have the Vista install, can you tell us what driver 
it's using in Windows (the Intel one, standard Microsoft AHCI, etc?)

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Hello, Alan, Moritz.

Alan Cox wrote:
>> The problem appears at least on two models (Sony Vaio FW and Sony
>> Vaio VGN-NS11S). So I really don't agree with you "one report, from
>> one user, with one configuration, on one controller, using one
>> firmware set" statement, Alan.
> Thanks - yours was the only one I had seen references too. I don't
> spend my days reading the Ubuntu forums.
> 
>> (where issues like this have not been fixed yet).
> 
> Its probably a better idea to buy a computer that appears to work in the
> way it is expected yes. However there is more to making a good quality
> OS than running around flapping at every warped piece of hardware we
> meet. It would be good if this works but it would also be good to know
> therefore why it happens to work in Vista - are the delays longer, are
> they clearing some other undefined command bits that the spec says don't
> matter etc.
> 
> Simply saying 'oh it didn't work lets just pretend we don't care' isn't
> always a good idea, particuarly when the worst case failure for it on
> other devices where it indicates a real failing might be corruption of
> user data.
>
> Yes - I'd like it to just work, but I'd like it to just work without
> breaking anything else, without adding device specific special cases and
> ideally in a way that makes other stuff that is similarly odd just work
> too.

Then, how do you suggest proceeding on it?  Without a bus tracer, we
can't easily tell what Windows is doing and on native SATA skipping
SETXFER isn't likely to cause any serious problem.  It's basically
noop.

If you're worried about the match being triggered on unaffected
configurations, how about doing a DMI & device ID match, so we flip
the stupid quirk only on the stupid machine?  It's not like we're
adding a lot of complexities and it's not likely that someone would
invest the time and resource to root-cause problem this specific
unless it begins to appear on other places.  So, let's get the thing
working and if similar problems creep up on other machines, deal with
them then.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> > Yes - I'd like it to just work, but I'd like it to just work without
> > breaking anything else, without adding device specific special cases and
> > ideally in a way that makes other stuff that is similarly odd just work
> > too.
> 
> Then, how do you suggest proceeding on it?  Without a bus tracer, we
> can't easily tell what Windows is doing and on native SATA skipping
> SETXFER isn't likely to cause any serious problem.  It's basically
> noop.

I would suggest we issue the SETXFER always. If it times out then we will
revalidate the identify data so we can use that to see what mode we are
actually in.

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Alan Cox wrote:
>>> Yes - I'd like it to just work, but I'd like it to just work without
>>> breaking anything else, without adding device specific special cases and
>>> ideally in a way that makes other stuff that is similarly odd just work
>>> too.
>> Then, how do you suggest proceeding on it?  Without a bus tracer, we
>> can't easily tell what Windows is doing and on native SATA skipping
>> SETXFER isn't likely to cause any serious problem.  It's basically
>> noop.
> 
> I would suggest we issue the SETXFER always. If it times out then we will
> revalidate the identify data so we can use that to see what mode we are
> actually in.

Hmmm... our current timeout for SETXFER is 5s, so it wouldn't be nice
but not too bad either.  Alright, I'll brew up something.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Tejun Heo wrote:
> Alan Cox wrote:
>>>> Yes - I'd like it to just work, but I'd like it to just work without
>>>> breaking anything else, without adding device specific special cases and
>>>> ideally in a way that makes other stuff that is similarly odd just work
>>>> too.
>>> Then, how do you suggest proceeding on it?  Without a bus tracer, we
>>> can't easily tell what Windows is doing and on native SATA skipping
>>> SETXFER isn't likely to cause any serious problem.  It's basically
>>> noop.
>> I would suggest we issue the SETXFER always. If it times out then we will
>> revalidate the identify data so we can use that to see what mode we are
>> actually in.
> 
> Hmmm... our current timeout for SETXFER is 5s, so it wouldn't be nice
> but not too bad either.  Alright, I'll brew up something.

Hmm... There's a problem.  After a timeout, libata EH always resets
and reset can reset transfer mode settings and stuff, so the whole
dancing becomes a bit pointless.  The device in question in this case
seems to retain (either that or udma/33 is the default which is
unlikely) the configuration over resets but I'm skeptical how useful
such logic would be generally.

We can change EH logic such that it does its best to issue IDENTIFY
after a SETXFER timeout without reset but that's likely to bring more
problem than it solves.  Some LLDs might be okay with it but there
simply is no guarantee.  Given the rarity, I don't think the problem
warrants such major behavior change.

Any more thoughts?

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

Reviving an old thread and quoting whole body for new reporter.

Tejun Heo wrote:
> Alan Cox wrote:
>>>> Sorry you can't just blindly do this because some of the controllers snoop
>>>> the SETXFER command to set their timings and whether they expect DMA. Also
>>>> we've no idea if this is a bug in a specific firmware revision, a quirky
>>>> pata/sata bridge or a timing problem of some sort.
>>> I think it'll generally be okay for SATA unless it's bridged over to
>>> PATA controller.  Any other ideas?
>> We seem to have one report, from one user, with one configuration, on one
>> controller, using one firmware set.
> 
> There are two more reports linked from the thread.
> 
>   http://ubuntuforums.org/showthread.php?t=986871
>   https://bugs.launchpad.net/ubuntu/+source/linux/+bug/299865
> 
> And all of them are showing the same problem.  This being a slim SATA
> drive for a laptop, I don't really expect to see the drive in varied
> configurations.  The problem might as well be limited to certain OEM
> drive(s).
> 
>> That to me isn't meaningful evidence of anything that needs a workaround,
>> beyond maybe doing what old IDE does and if a setxfer times out
>> continuing in hope.
> 
> So, as far as the validity of the report goes, I think it's at
> reasonable level.  Add to that my general trigger happiness toward
> quirks and the machine is a VAIO and to me quirking it doesn't seem
> too careless.
> 
>> (We should of course then re-read the identify pages and check the mode
>> in use)
> 
> Of course, whether the workaround is proper is a completely separate
> issue but unless other devices with the same problem creep up and the
> device is happy with the quirk, I don't really think we should modify
> the default configuration sequence for devices like this.
> 
> That said, Moritz, can you please post the output of "hdparm -I" with
> and without the quirk applied?  And can you please use a preoper mail
> address?

Martin Novak (cc'd) is reporting the same problem.  I really think
doing nothing is the worst choice here.  Given the rarity of the
issue, I don't think we need worry too much about long term impact of
the quirk.

Anyways, Martin, can you please post what I asked Moritz - the output
of "hdparm -I" with and without the quirk applied?  Quirk patch is
attached.

Thanks.

-- 
tejun

[-- Attachment #2: horkage-notsetxfer.patch --]
[-- Type: text/x-patch, Size: 2101 bytes --]

libata: implement and use HORKAGE_NOSETXFER

PIONEER DVD-RW DVRTD08 times out SETXFER if no media is present.  The
device is SATA and simply skipping SETXFER works around the problem.
Implement ATA_HORKAGE_NOSETXFER and apply it to the device.

Reported by Moritz Rigler in the following thread.

  http://thread.gmane.org/gmane.linux.ide/36790

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |   10 ++++++++--
 include/linux/libata.h    |    1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c924230..5dcdcb4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3390,14 +3390,17 @@ static int ata_dev_set_mode(struct ata_device *dev)
 	struct ata_eh_context *ehc = &dev->link->eh_context;
 	const char *dev_err_whine = "";
 	int ign_dev_err = 0;
-	unsigned int err_mask;
+	unsigned int err_mask = 0;
 	int rc;
 
 	dev->flags &= ~ATA_DFLAG_PIO;
 	if (dev->xfer_shift == ATA_SHIFT_PIO)
 		dev->flags |= ATA_DFLAG_PIO;
 
-	err_mask = ata_dev_set_xfermode(dev);
+	if (!(dev->horkage & ATA_HORKAGE_NOSETXFER))
+		err_mask = ata_dev_set_xfermode(dev);
+	else
+		dev_err_whine = " (SET_XFERMODE skipped)";
 
 	if (err_mask & ~AC_ERR_DEV)
 		goto fail;
@@ -4292,6 +4295,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	/* Devices which aren't very happy with higher link speeds */
 	{ "WD My Book",			NULL,	ATA_HORKAGE_1_5_GBPS, },
 
+	/* Devices which choke on SETXFER.  Presumably SATA only. */
+	{ "PIONEER DVD-RW  DVRTD08",	"1.00",	ATA_HORKAGE_NOSETXFER },
+
 	/* End Marker */
 	{ }
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..2b641af 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -385,6 +385,7 @@ enum {
 						    not multiple of 16 bytes */
 	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firmware update warning */
 	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
+	ATA_HORKAGE_NOSETXFER	= (1 << 14),
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

On Mon, 25 May 2009 12:10:19 +0900
Tejun Heo <tj@kernel.org> wrote:

> Reviving an old thread and quoting whole body for new reporter.

I've not changed my view btw - which is that the quirk should match what
old IDE does here - issue the command and if it fails carry on regardless.


Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

cc'ing Lars.  New bug reporter on this problem.

Alan Cox wrote:
> On Mon, 25 May 2009 12:10:19 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> Reviving an old thread and quoting whole body for new reporter.
> 
> I've not changed my view btw - which is that the quirk should match what
> old IDE does here - issue the command and if it fails carry on regardless.

That isn't possible because the host machine state is unknown after a
timeout.  The problem is not confined to TF based controllers and even
on TF based controllers, it's far too dangerous to continue as nothing
happened.  That can easily lead to complete system lock up on quite a
few controllers due to fragile TF emulation layer.

The drive is full SATA.  SETXFER doesn't mean anything to it.  Let's
just skip it.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> That isn't possible because the host machine state is unknown after a
> timeout.  The problem is not confined to TF based controllers and even
> on TF based controllers, it's far too dangerous to continue as nothing
> happened.  That can easily lead to complete system lock up on quite a
> few controllers due to fragile TF emulation layer.

It works fine on old style taskfile controllers, and the old IDE layer
has done it for years

> The drive is full SATA.  SETXFER doesn't mean anything to it.  Let's
> just skip it.

What if there is a bridge, what if the controller needs SETXFER for
internal use (HPT PATA with on card SATA bridges) ?

I have a feeling this is one case where there isn't a generic solution
for all devices

For PATA we need SETXFER to be issued
For SATA we can maybe avoid it sometimes but its getting into deeply
undefined territory and will break the early HPT at least.

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Alan Cox wrote:
>> That isn't possible because the host machine state is unknown after a
>> timeout.  The problem is not confined to TF based controllers and even
>> on TF based controllers, it's far too dangerous to continue as nothing
>> happened.  That can easily lead to complete system lock up on quite a
>> few controllers due to fragile TF emulation layer.
> 
> It works fine on old style taskfile controllers, and the old IDE layer
> has done it for years

Yeah, exactly.  libata covers more diverse set of controllers and
nothing has ever been tested for issuing commands without full reset
after exceptions which can put HSM into unknown state.  The whole EH
is designed and implemented that way.  So, many reset methods reset
not only the ATA bus but also the HSM.  This also coincides with
hardware design.  So, if the problem is confined to TF based old IDE,
issuing commands after timeout could work fine but I don't think
that's a viable option here.

>> The drive is full SATA.  SETXFER doesn't mean anything to it.  Let's
>> just skip it.
> 
> What if there is a bridge, what if the controller needs SETXFER for
> internal use (HPT PATA with on card SATA bridges) ?

The device is horridly broken.  I don't think we can please everyone
here.  Also, it being a slim oem dvd which gets shipped with laptops,
I don't think we need to worry too much about highly varied
configurations.

> I have a feeling this is one case where there isn't a generic solution
> for all devices
> 
> For PATA we need SETXFER to be issued
> For SATA we can maybe avoid it sometimes but its getting into deeply
> undefined territory and will break the early HPT at least.

Sure, fully agreed.  It's a trade off.  There are three options.

1. Don't do anything.  The device is broken everywhere.

2. Skip SETXFER.  Any direct SATA connection to the device would work
   fine.

2. Ignore SETXFER timeout.  Probably would work for TF based
   controllers but it's largely untested for sata controllers and the
   behavior is likely undefined for more advanced controllers.

So, #2 seems like the logical choice here.  If worst comes to worst
and some idiot decides to ship the drive with SATA bridge (but really
how many of those configurations do we see anymore?), we can blacklist
that particular machine using dmi or whatnot and craft workaround such
that timeout is ignored for that particular case if it works.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> > What if there is a bridge, what if the controller needs SETXFER for
> > internal use (HPT PATA with on card SATA bridges) ?
> 
> The device is horridly broken. 

I know 8(

> 1. Don't do anything.  The device is broken everywhere.
> 
> 2. Skip SETXFER.  Any direct SATA connection to the device would work
>    fine.
> 
> 2. Ignore SETXFER timeout.  Probably would work for TF based
>    controllers but it's largely untested for sata controllers and the
>    behavior is likely undefined for more advanced controllers.

> So, #2 seems like the logical choice here.  If worst comes to worst

You have two #2's but I agree that providing its for this specific drive
and we document it carefully so people don't add it to stuff that is
wrong then your first #2 is probably safest.

Possibly  HORKAGE_NO_SETXFER_SATA and check word 93 ?


Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>That isn't possible because the host machine state is unknown after a
>>timeout.  The problem is not confined to TF based controllers and even
>>on TF based controllers, it's far too dangerous to continue as nothing
>>happened.  That can easily lead to complete system lock up on quite a
>>few controllers due to fragile TF emulation layer.

> It works fine on old style taskfile controllers, and the old IDE layer
> has done it for years

>>The drive is full SATA.  SETXFER doesn't mean anything to it.  Let's
>>just skip it.

> What if there is a bridge, what if the controller needs SETXFER for
> internal use (HPT PATA with on card SATA bridges) ?

    Do you mean HPT36x/37x here? If so, what internal use the controller has 
for this command?

> I have a feeling this is one case where there isn't a generic solution
> for all devices

> For PATA we need SETXFER to be issued
> For SATA we can maybe avoid it sometimes but its getting into deeply
> undefined territory and will break the early HPT at least.

    Hm...

> Alan

MBR, Sergei

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Alan Cox]

> >>The drive is full SATA.  SETXFER doesn't mean anything to it.  Let's
> >>just skip it.
> 
> > What if there is a bridge, what if the controller needs SETXFER for
> > internal use (HPT PATA with on card SATA bridges) ?
> 
>     Do you mean HPT36x/37x here? If so, what internal use the controller has 
> for this command?

HPT37x - if you don't issue a SETXFER it doesn't work with the SATA
bridge (it also blows up if you don't select certain modes). All very
weird and I've no idea why it does that. Tejun is right however that we
don't need to care for the DVD case in question

Alan

Re: [PATCH #upstream-fixes] libata: implement and use HORKAGE_NOSETXFER

[Tejun Heo]

Hello,

Alan Cox wrote:
>> 2. Skip SETXFER.  Any direct SATA connection to the device would work
>>    fine.
>>
>> 2. Ignore SETXFER timeout.  Probably would work for TF based
>>    controllers but it's largely untested for sata controllers and the
>>    behavior is likely undefined for more advanced controllers.
> 
>> So, #2 seems like the logical choice here.  If worst comes to worst
> 
> You have two #2's

Oops.

> but I agree that providing its for this specific drive
> and we document it carefully so people don't add it to stuff that is
> wrong then your first #2 is probably safest.
> 
> Possibly  HORKAGE_NO_SETXFER_SATA and check word 93 ?

How about something like the following?

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 045a486..2c6aeda 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3392,17 +3392,27 @@ int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel)
 
 static int ata_dev_set_mode(struct ata_device *dev)
 {
+	struct ata_port *ap = dev->link->ap;
 	struct ata_eh_context *ehc = &dev->link->eh_context;
+	const bool nosetxfer = dev->horkage & ATA_HORKAGE_NOSETXFER;
 	const char *dev_err_whine = "";
 	int ign_dev_err = 0;
-	unsigned int err_mask;
+	unsigned int err_mask = 0;
 	int rc;
 
 	dev->flags &= ~ATA_DFLAG_PIO;
 	if (dev->xfer_shift == ATA_SHIFT_PIO)
 		dev->flags |= ATA_DFLAG_PIO;
 
-	err_mask = ata_dev_set_xfermode(dev);
+	if (nosetxfer && ap->flags & ATA_FLAG_SATA && ata_id_is_sata(dev->id))
+		dev_err_whine = " (SET_XFERMODE skipped)";
+	else {
+		if (nosetxfer)
+			ata_dev_printk(dev, KERN_WARNING,
+				       "NOSETXFER but PATA detected - can't "
+				       "skip SETXFER, might malfunction\n");
+		err_mask = ata_dev_set_xfermode(dev);
+	}
 
 	if (err_mask & ~AC_ERR_DEV)
 		goto fail;
@@ -4297,6 +4307,12 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	/* Devices which aren't very happy with higher link speeds */
 	{ "WD My Book",			NULL,	ATA_HORKAGE_1_5_GBPS, },
 
+	/*
+	 * Devices which choke on SETXFER.  Applies only if both the
+	 * device and controller are SATA.
+	 */
+	{ "PIONEER DVD-RW  DVRTD08",	"1.00",	ATA_HORKAGE_NOSETXFER },
+
 	/* End Marker */
 	{ }
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..2b641af 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -385,6 +385,7 @@ enum {
 						    not multiple of 16 bytes */
 	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firmware update warning */
 	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
+	ATA_HORKAGE_NOSETXFER	= (1 << 14),
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

-- 
tejun