Bug 123980 - [ata] [patch] Implement ATA UDMA speed limit (hw.ata.ata_dma_limit)
Summary: [ata] [patch] Implement ATA UDMA speed limit (hw.ata.ata_dma_limit)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 7.0-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Alexander Motin
URL:
Keywords:
: 130794 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-05-25 18:10 UTC by Martin Johnson
Modified: 2018-02-18 07:39 UTC (History)
1 user (show)

See Also:


Attachments
file.diff (1.47 KB, patch)
2008-05-25 18:10 UTC, Martin Johnson
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Johnson 2008-05-25 18:10:00 UTC
FreeBSD sometimes chooses an unsafe UDMA speed for IDE hard drives, despite best efforts made in the ATA driver code to prevent this.

An example is the Soekris net5501 embedded computer, using a Travelstar E7K100 2.5" hard drive.  FreeBSD 6.2-RELEASE chooses UDMA33 mode for this hardware - which works OK - but FreeBSD 7.0-RELEASE chooses UDMA100 mode, which is too fast and causes disk errors such as "ad0: WARNING - READ_DMA UDMA ICRC error (retrying request) LBA=3871".

The attached patch allows the user to set a new sysctl, hw.ata.ata_dma_limit, in /boot/loader.conf.  This acts as a speed limiter.  For example on the Soekris net5501 platform, this hw.ata.ata_dma_limit="4" reduces the speed to UDMA66 mode, which works well, giving a small performance increase over UDMA33 mode without the errors caused by UDMA100 mode.

Fix: See attached patch.  

This only implements the change for IDE (PATA) drives, and it's possible that similar code might be sensible for SATA drives.


Patch attached with submission follows:
How-To-Repeat: Clean install (via PXEBOOT and NFS) of FreeBSD 7.0-RELEASE onto Soekris net5501 with E7K100 Travelstar hard drive.   Errors will be reported after installation, particularly when disk activity is heavy.

http://jdc.parodius.com/freebsd/pxeboot_serial_install.html gives some hints including a workaround for a showstopper regarding loading gzip'd mfsroot from pxeboot.
Comment 1 Brad Davis freebsd_committer freebsd_triage 2008-05-25 18:41:09 UTC
Responsible Changed
From-To: freebsd-i386->freebsd-bugs

Fix category and Synopsis to note that it includes a patch.
Comment 2 Martin Johnson 2009-03-15 19:50:49 UTC
The issue is still present in 7.1-RELEASE on the Soekris NET5501 i386  
hardware, for PATA hard disk drives such as the TravelStar E7K100.

The file ata-all.c has changed enough to break my patch, so I have re- 
made the patch to implement the ATA speed limit sysctl for 7.1- 
RELEASE.  The updated patch is pasted below.

-- Martin.

--- ata-all.c.orig	2009-03-15 15:49:30.000000000 +0000
+++ ata-all.c	2009-03-15 15:55:33.000000000 +0000
@@ -78,15 +78,19 @@
  int ata_dma_check_80pin = 1;

  /* local vars */
  static int ata_dma = 1;
+static int ata_dma_limit = 6;
  static int atapi_dma = 1;

  /* sysctl vars */
  SYSCTL_NODE(_hw, OID_AUTO, ata, CTLFLAG_RD, 0, "ATA driver  
parameters");
  TUNABLE_INT("hw.ata.ata_dma", &ata_dma);
  SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma, CTLFLAG_RDTUN, &ata_dma, 0,
  	   "ATA disk DMA mode control");
+TUNABLE_INT("hw.ata.ata_dma_limit", &ata_dma_limit);
+SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma_limit, CTLFLAG_RDTUN,  
&ata_dma_limit, 0,
+           "ATA disk DMA mode limit");
  TUNABLE_INT("hw.ata.ata_dma_check_80pin", &ata_dma_check_80pin);
  SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma_check_80pin,
  	   CTLFLAG_RDTUN, &ata_dma_check_80pin, 1,
  	   "Check for 80pin cable before setting ATA DMA mode");
@@ -910,19 +914,19 @@
  int
  ata_umode(struct ata_params *ap)
  {
      if (ap->atavalid & ATA_FLAG_88) {
-	if (ap->udmamodes & 0x40)
+	if (ap->udmamodes & 0x40  &&  ata_dma_limit >= 6)
  	    return ATA_UDMA6;
-	if (ap->udmamodes & 0x20)
+	if (ap->udmamodes & 0x20  &&  ata_dma_limit >= 5)
  	    return ATA_UDMA5;
-	if (ap->udmamodes & 0x10)
+	if (ap->udmamodes & 0x10  &&  ata_dma_limit >= 4)
  	    return ATA_UDMA4;
-	if (ap->udmamodes & 0x08)
+	if (ap->udmamodes & 0x08  &&  ata_dma_limit >= 3)
  	    return ATA_UDMA3;
-	if (ap->udmamodes & 0x04)
+	if (ap->udmamodes & 0x04  &&  ata_dma_limit >= 2)
  	    return ATA_UDMA2;
-	if (ap->udmamodes & 0x02)
+	if (ap->udmamodes & 0x02  &&  ata_dma_limit >= 1)
  	    return ATA_UDMA1;
  	if (ap->udmamodes & 0x01)
  	    return ATA_UDMA0;
      }
Comment 3 Wesha 2009-12-21 06:21:50 UTC
Hello Bug-followup,

Updated patch for 8.0-RELEASE:

---------------------------------------------------------------

--- ata-all.c.orig      2009-12-20 19:15:15.000000000 -0600
+++ ata-all.c   2009-12-20 19:20:20.000000000 -0600
@@ -79,15 +79,19 @@
 int ata_dma_check_80pin =3D 1;

 /* local vars */
 static int ata_dma =3D 1;
+static int ata_dma_limit =3D 6;
 static int atapi_dma =3D 1;

 /* sysctl vars */
 SYSCTL_NODE(_hw, OID_AUTO, ata, CTLFLAG_RD, 0, "ATA driver parameters");
 TUNABLE_INT("hw.ata.ata_dma", &ata_dma);
 SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma, CTLFLAG_RDTUN, &ata_dma, 0,
           "ATA disk DMA mode control");
+TUNABLE_INT("hw.ata.ata_dma_limit", &ata_dma_limit);
+SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma_limit, CTLFLAG_RDTUN,
+            &ata_dma_limit, 0, "ATA disk DMA mode limit");
 TUNABLE_INT("hw.ata.ata_dma_check_80pin", &ata_dma_check_80pin);
 SYSCTL_INT(_hw_ata, OID_AUTO, ata_dma_check_80pin,
           CTLFLAG_RDTUN, &ata_dma_check_80pin, 1,
           "Check for 80pin cable before setting ATA DMA mode");
@@ -1001,19 +1005,19 @@
 int
 ata_umode(struct ata_params *ap)
 {
     if (ap->atavalid & ATA_FLAG_88) {
-       if (ap->udmamodes & 0x40)
+       if (ap->udmamodes & 0x40 && ata_dma_limit >=3D 6)
            return ATA_UDMA6;
-       if (ap->udmamodes & 0x20)
+       if (ap->udmamodes & 0x20 && ata_dma_limit >=3D 5)
            return ATA_UDMA5;
-       if (ap->udmamodes & 0x10)
+       if (ap->udmamodes & 0x10 && ata_dma_limit >=3D 4)
            return ATA_UDMA4;
-       if (ap->udmamodes & 0x08)
+       if (ap->udmamodes & 0x08 && ata_dma_limit >=3D 3)
            return ATA_UDMA3;
-       if (ap->udmamodes & 0x04)
+       if (ap->udmamodes & 0x04 && ata_dma_limit >=3D 2)
            return ATA_UDMA2;
-       if (ap->udmamodes & 0x02)
+       if (ap->udmamodes & 0x02 && ata_dma_limit >=3D 1)
            return ATA_UDMA1;
        if (ap->udmamodes & 0x01)
            return ATA_UDMA0;
     }


---------------------------------------------------------------


--=20
Best regards,
 Wesha                          mailto:wesha@wesha.name
Comment 4 Remko Lodder freebsd_committer freebsd_triage 2010-02-12 09:33:26 UTC
Responsible Changed
From-To: freebsd-bugs->mav

This came up one the PFSense dev-list, can you have a look at this 
please?
Comment 5 Alexander Motin freebsd_committer freebsd_triage 2010-02-21 17:32:41 UTC
This patch made in wrong place. Proper place is ata_setmode() function
in ata-all.c. Also this patch looks overstrict, as it limits speed for
all ATA devices in system. I'll work on better implementation for new
CAM ATA.

-- 
Alexander Motin
Comment 6 Martin Johnson 2010-07-03 09:08:23 UTC
The patch for FreeBSD 7.x lost formatting when pasted above.
A clean copy can be found here:
     http://www.net42.co.uk/downloads/ata-all.c.patch
Comment 7 dfilter service freebsd_committer freebsd_triage 2010-07-03 15:14:56 UTC
Author: mav
Date: Sat Jul  3 14:14:42 2010
New Revision: 209664
URL: http://svn.freebsd.org/changeset/base/209664

Log:
  Add ata(4) ability to limit initial ATA mode for devices via device hints.
  After boot this mode can be changed with atacontrol/camcontrol as usual.
  It works for both legacy and ATA_CAM wrapper mode.
  
  PR:		kern/123980

Modified:
  head/share/man/man4/ata.4
  head/sys/dev/ata/ata-all.c
  head/sys/dev/ata/ata-all.h

Modified: head/share/man/man4/ata.4
==============================================================================
--- head/share/man/man4/ata.4	Sat Jul  3 14:03:31 2010	(r209663)
+++ head/share/man/man4/ata.4	Sat Jul  3 14:14:42 2010	(r209664)
@@ -27,7 +27,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd March 3, 2010
+.Dd July 3, 2010
 .Dt ATA 4
 .Os
 .Sh NAME
@@ -103,6 +103,10 @@ can cause data loss on power failures an
 .It Va hint.atapci.X.msi
 set to 1 to allow Message Signalled Interrupts (MSI) to be used by
 specified PCI ATA controller, if supported.
+.It Va hint.ata.X.devX.mode
+limits initial ATA mode for specified device on specified channel.
+.It Va hint.ata.X.mode
+limits initial ATA mode for every device on specified channel.
 .It Va hint.ata.X.pm_level
 controls SATA interface Power Management for specified channel,
 allowing to save some power by the cost of additional command latency.

Modified: head/sys/dev/ata/ata-all.c
==============================================================================
--- head/sys/dev/ata/ata-all.c	Sat Jul  3 14:03:31 2010	(r209663)
+++ head/sys/dev/ata/ata-all.c	Sat Jul  3 14:14:42 2010	(r209664)
@@ -133,7 +133,9 @@ ata_attach(device_t dev)
     int error, rid;
 #ifdef ATA_CAM
     struct cam_devq *devq;
-    int i;
+    const char *res;
+    char buf[64];
+    int i, mode;
 #endif
 
     /* check that we have a virgin channel to attach */
@@ -152,6 +154,17 @@ ata_attach(device_t dev)
 #ifdef ATA_CAM
 	for (i = 0; i < 16; i++) {
 		ch->user[i].mode = 0;
+		snprintf(buf, sizeof(buf), "dev%d.mode", i);
+		if (resource_string_value(device_get_name(dev),
+		    device_get_unit(dev), buf, &res) == 0)
+			mode = ata_str2mode(res);
+		else if (resource_string_value(device_get_name(dev),
+		    device_get_unit(dev), "mode", &res) == 0)
+			mode = ata_str2mode(res);
+		else
+			mode = -1;
+		if (mode >= 0)
+			ch->user[i].mode = mode;
 		if (ch->flags & ATA_SATA)
 			ch->user[i].bytecount = 8192;
 		else
@@ -826,8 +839,10 @@ ata_getparam(struct ata_device *atadev, 
 {
     struct ata_channel *ch = device_get_softc(device_get_parent(atadev->dev));
     struct ata_request *request;
+    const char *res;
+    char buf[64];
     u_int8_t command = 0;
-    int error = ENOMEM, retries = 2;
+    int error = ENOMEM, retries = 2, mode = -1;
 
     if (ch->devices & (ATA_ATA_MASTER << atadev->unit))
 	command = ATA_ATA_IDENTIFY;
@@ -907,6 +922,15 @@ ata_getparam(struct ata_device *atadev, 
 		     ata_wmode(&atadev->param) > 0))
 		    atadev->mode = ATA_DMA_MAX;
 	    }
+	    snprintf(buf, sizeof(buf), "dev%d.mode", atadev->unit);
+	    if (resource_string_value(device_get_name(ch->dev),
+	        device_get_unit(ch->dev), buf, &res) == 0)
+		    mode = ata_str2mode(res);
+	    else if (resource_string_value(device_get_name(ch->dev),
+		device_get_unit(ch->dev), "mode", &res) == 0)
+		    mode = ata_str2mode(res);
+	    if (mode >= 0)
+		    atadev->mode = mode;
 	}
     }
     else {
@@ -1163,6 +1187,35 @@ ata_mode2str(int mode)
     }
 }
 
+int
+ata_str2mode(const char *str)
+{
+
+	if (!strcasecmp(str, "PIO0")) return (ATA_PIO0);
+	if (!strcasecmp(str, "PIO1")) return (ATA_PIO1);
+	if (!strcasecmp(str, "PIO2")) return (ATA_PIO2);
+	if (!strcasecmp(str, "PIO3")) return (ATA_PIO3);
+	if (!strcasecmp(str, "PIO4")) return (ATA_PIO4);
+	if (!strcasecmp(str, "WDMA0")) return (ATA_WDMA0);
+	if (!strcasecmp(str, "WDMA1")) return (ATA_WDMA1);
+	if (!strcasecmp(str, "WDMA2")) return (ATA_WDMA2);
+	if (!strcasecmp(str, "UDMA0")) return (ATA_UDMA0);
+	if (!strcasecmp(str, "UDMA16")) return (ATA_UDMA0);
+	if (!strcasecmp(str, "UDMA1")) return (ATA_UDMA1);
+	if (!strcasecmp(str, "UDMA25")) return (ATA_UDMA1);
+	if (!strcasecmp(str, "UDMA2")) return (ATA_UDMA2);
+	if (!strcasecmp(str, "UDMA33")) return (ATA_UDMA2);
+	if (!strcasecmp(str, "UDMA3")) return (ATA_UDMA3);
+	if (!strcasecmp(str, "UDMA44")) return (ATA_UDMA3);
+	if (!strcasecmp(str, "UDMA4")) return (ATA_UDMA4);
+	if (!strcasecmp(str, "UDMA66")) return (ATA_UDMA4);
+	if (!strcasecmp(str, "UDMA5")) return (ATA_UDMA5);
+	if (!strcasecmp(str, "UDMA100")) return (ATA_UDMA5);
+	if (!strcasecmp(str, "UDMA6")) return (ATA_UDMA6);
+	if (!strcasecmp(str, "UDMA133")) return (ATA_UDMA6);
+	return (-1);
+}
+
 const char *
 ata_satarev2str(int rev)
 {

Modified: head/sys/dev/ata/ata-all.h
==============================================================================
--- head/sys/dev/ata/ata-all.h	Sat Jul  3 14:03:31 2010	(r209663)
+++ head/sys/dev/ata/ata-all.h	Sat Jul  3 14:14:42 2010	(r209664)
@@ -625,6 +625,7 @@ void ata_modify_if_48bit(struct ata_requ
 void ata_udelay(int interval);
 char *ata_unit2str(struct ata_device *atadev);
 const char *ata_mode2str(int mode);
+int ata_str2mode(const char *str);
 const char *ata_satarev2str(int rev);
 int ata_atapi(device_t dev, int target);
 int ata_pmode(struct ata_params *ap);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 8 Alexander Motin freebsd_committer freebsd_triage 2010-07-03 15:42:47 UTC
State Changed
From-To: open->patched

Alternative patch committed to HEAD.
Comment 9 dfilter service freebsd_committer freebsd_triage 2010-07-16 10:12:57 UTC
Author: mav
Date: Fri Jul 16 09:12:47 2010
New Revision: 210164
URL: http://svn.freebsd.org/changeset/base/210164

Log:
  MFC r209664:
  Add ata(4) ability to limit initial ATA mode for devices via device hints.
  After boot this mode can be changed with atacontrol/camcontrol as usual.
  It works for both legacy and ATA_CAM wrapper mode.
  
  PR:             kern/123980

Modified:
  stable/8/sys/dev/ata/ata-all.c
  stable/8/sys/dev/ata/ata-all.h
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)

Modified: stable/8/sys/dev/ata/ata-all.c
==============================================================================
--- stable/8/sys/dev/ata/ata-all.c	Fri Jul 16 09:10:11 2010	(r210163)
+++ stable/8/sys/dev/ata/ata-all.c	Fri Jul 16 09:12:47 2010	(r210164)
@@ -133,7 +133,9 @@ ata_attach(device_t dev)
     int error, rid;
 #ifdef ATA_CAM
     struct cam_devq *devq;
-    int i;
+    const char *res;
+    char buf[64];
+    int i, mode;
 #endif
 
     /* check that we have a virgin channel to attach */
@@ -152,6 +154,17 @@ ata_attach(device_t dev)
 #ifdef ATA_CAM
 	for (i = 0; i < 16; i++) {
 		ch->user[i].mode = 0;
+		snprintf(buf, sizeof(buf), "dev%d.mode", i);
+		if (resource_string_value(device_get_name(dev),
+		    device_get_unit(dev), buf, &res) == 0)
+			mode = ata_str2mode(res);
+		else if (resource_string_value(device_get_name(dev),
+		    device_get_unit(dev), "mode", &res) == 0)
+			mode = ata_str2mode(res);
+		else
+			mode = -1;
+		if (mode >= 0)
+			ch->user[i].mode = mode;
 		if (ch->flags & ATA_SATA)
 			ch->user[i].bytecount = 8192;
 		else
@@ -826,8 +839,10 @@ ata_getparam(struct ata_device *atadev, 
 {
     struct ata_channel *ch = device_get_softc(device_get_parent(atadev->dev));
     struct ata_request *request;
+    const char *res;
+    char buf[64];
     u_int8_t command = 0;
-    int error = ENOMEM, retries = 2;
+    int error = ENOMEM, retries = 2, mode = -1;
 
     if (ch->devices & (ATA_ATA_MASTER << atadev->unit))
 	command = ATA_ATA_IDENTIFY;
@@ -907,6 +922,15 @@ ata_getparam(struct ata_device *atadev, 
 		     ata_wmode(&atadev->param) > 0))
 		    atadev->mode = ATA_DMA_MAX;
 	    }
+	    snprintf(buf, sizeof(buf), "dev%d.mode", atadev->unit);
+	    if (resource_string_value(device_get_name(ch->dev),
+	        device_get_unit(ch->dev), buf, &res) == 0)
+		    mode = ata_str2mode(res);
+	    else if (resource_string_value(device_get_name(ch->dev),
+		device_get_unit(ch->dev), "mode", &res) == 0)
+		    mode = ata_str2mode(res);
+	    if (mode >= 0)
+		    atadev->mode = mode;
 	}
     }
     else {
@@ -1163,6 +1187,35 @@ ata_mode2str(int mode)
     }
 }
 
+int
+ata_str2mode(const char *str)
+{
+
+	if (!strcasecmp(str, "PIO0")) return (ATA_PIO0);
+	if (!strcasecmp(str, "PIO1")) return (ATA_PIO1);
+	if (!strcasecmp(str, "PIO2")) return (ATA_PIO2);
+	if (!strcasecmp(str, "PIO3")) return (ATA_PIO3);
+	if (!strcasecmp(str, "PIO4")) return (ATA_PIO4);
+	if (!strcasecmp(str, "WDMA0")) return (ATA_WDMA0);
+	if (!strcasecmp(str, "WDMA1")) return (ATA_WDMA1);
+	if (!strcasecmp(str, "WDMA2")) return (ATA_WDMA2);
+	if (!strcasecmp(str, "UDMA0")) return (ATA_UDMA0);
+	if (!strcasecmp(str, "UDMA16")) return (ATA_UDMA0);
+	if (!strcasecmp(str, "UDMA1")) return (ATA_UDMA1);
+	if (!strcasecmp(str, "UDMA25")) return (ATA_UDMA1);
+	if (!strcasecmp(str, "UDMA2")) return (ATA_UDMA2);
+	if (!strcasecmp(str, "UDMA33")) return (ATA_UDMA2);
+	if (!strcasecmp(str, "UDMA3")) return (ATA_UDMA3);
+	if (!strcasecmp(str, "UDMA44")) return (ATA_UDMA3);
+	if (!strcasecmp(str, "UDMA4")) return (ATA_UDMA4);
+	if (!strcasecmp(str, "UDMA66")) return (ATA_UDMA4);
+	if (!strcasecmp(str, "UDMA5")) return (ATA_UDMA5);
+	if (!strcasecmp(str, "UDMA100")) return (ATA_UDMA5);
+	if (!strcasecmp(str, "UDMA6")) return (ATA_UDMA6);
+	if (!strcasecmp(str, "UDMA133")) return (ATA_UDMA6);
+	return (-1);
+}
+
 const char *
 ata_satarev2str(int rev)
 {

Modified: stable/8/sys/dev/ata/ata-all.h
==============================================================================
--- stable/8/sys/dev/ata/ata-all.h	Fri Jul 16 09:10:11 2010	(r210163)
+++ stable/8/sys/dev/ata/ata-all.h	Fri Jul 16 09:12:47 2010	(r210164)
@@ -625,6 +625,7 @@ void ata_modify_if_48bit(struct ata_requ
 void ata_udelay(int interval);
 char *ata_unit2str(struct ata_device *atadev);
 const char *ata_mode2str(int mode);
+int ata_str2mode(const char *str);
 const char *ata_satarev2str(int rev);
 int ata_atapi(device_t dev, int target);
 int ata_pmode(struct ata_params *ap);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 10 Alexander Motin freebsd_committer freebsd_triage 2010-07-16 10:15:58 UTC
State Changed
From-To: patched->closed

Patch merged to 8-STABLE.
Comment 11 Alex Kozlov freebsd_committer freebsd_triage 2018-02-18 07:39:49 UTC
*** Bug 130794 has been marked as a duplicate of this bug. ***