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.
Responsible Changed From-To: freebsd-i386->freebsd-bugs Fix category and Synopsis to note that it includes a patch.
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; }
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
Responsible Changed From-To: freebsd-bugs->mav This came up one the PFSense dev-list, can you have a look at this please?
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
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
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"
State Changed From-To: open->patched Alternative patch committed to HEAD.
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"
State Changed From-To: patched->closed Patch merged to 8-STABLE.
*** Bug 130794 has been marked as a duplicate of this bug. ***