| Message ID | kb9pa2$mi8$1@ger.gmane.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 12/24/2012 08:37 AM, bl0 wrote: > On Tuesday 18 December 2012 16:23, bl0 wrote: > >> On Monday 17 December 2012 06:44, Robert Hancock wrote: >> >>> But it seems quite likely that >>> whatever magic numbers this code is picking don't work on your system >>> for some reason. It appears the root cause is likely a bug in the SiI >>> chip. There shouldn't be any region why messing around with these values >>> should cause data corruption other than that. >> >> Do you think something should be done about it in the linux sata_sil >> driver? For a lack of a better solution, here is my suggestion. There is >> already one option 'slow_down' for problematic disks. Another option, for >> example 'cache_line_workaround', could be added for problematic >> motherboards. If enabled, the most straightforward way is to set cache >> line size to 0 and not worry about the fifo_cfg register. > > Here is the code I currently have, attached as a diff. (This diff is not > against the latest git tree, it's against an older linux version which I > use.) I wouldn't mind something like this as an option, anyway. Jeff, Tejun, thoughts? -- 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
Hello, Robert, bl0 (would be cool if this were your real name :) On Tue, Jan 08, 2013 at 10:48:43PM -0600, Robert Hancock wrote: > >>Do you think something should be done about it in the linux sata_sil > >>driver? For a lack of a better solution, here is my suggestion. There is > >>already one option 'slow_down' for problematic disks. Another option, for > >>example 'cache_line_workaround', could be added for problematic > >>motherboards. If enabled, the most straightforward way is to set cache > >>line size to 0 and not worry about the fifo_cfg register. > > > >Here is the code I currently have, attached as a diff. (This diff is not > >against the latest git tree, it's against an older linux version which I > >use.) > > I wouldn't mind something like this as an option, anyway. Jeff, > Tejun, thoughts? I don't know. This thread makes me want to eat obsessivly, cry in fetal position and then puke and wet my pants. The issue has been there from the beginning. The current code seems to cope with, I hope, majority of configurations; unfortunately, for the problematic ones, there hasn't been any sensible explanation or reliable way to detect what is causing the problem. I don't even know how we should blacklist them. Is it chipset-specific or system-specific? ie. Should we blacklist north/south bridges or DMI system identifiers? Having a module option is easy and can be helpful for the very rare cases where the admin is aware of the issue, but, in general, it doesn't really help that much. So, let's throw in a module option. Other than that, no idea. \pukes
On Wednesday 09 January 2013 20:17:53 Tejun Heo wrote: > Hello, Robert, bl0 (would be cool if this were your real name :) > > On Tue, Jan 08, 2013 at 10:48:43PM -0600, Robert Hancock wrote: > > I wouldn't mind something like this as an option, anyway. Jeff, > > Tejun, thoughts? > > I don't know. This thread makes me want to eat obsessivly, cry in > fetal position and then puke and wet my pants. > > The issue has been there from the beginning. The current code seems > to cope with, I hope, majority of configurations; unfortunately, for > the problematic ones, there hasn't been any sensible explanation or > reliable way to detect what is causing the problem. I don't even know > how we should blacklist them. Is it chipset-specific or > system-specific? ie. Should we blacklist north/south bridges or DMI > system identifiers? > > Having a module option is easy and can be helpful for the very rare > cases where the admin is aware of the issue, but, in general, it > doesn't really help that much. > > So, let's throw in a module option. Other than that, no idea. > > \pukes Thanks for bringing much needed enthusiasm into this thread. I too would like to know the root cause of this problem. Since the chances of finding it are very low, the only thing that remains is to deal with data corruption problem using the means available. Some people asked for help and posted relevant entries from their logs. Now they would be able to find a hint about the module option in the logs. Some people have set the slow_down option. The code i posted enables the workaround for them too. As for blacklisting, I was thinking about maybe looking at the vendor and device ID of the PCI bridge under which the sata_sil device is found. -- 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
On 13-01-09 02:17 PM, Tejun Heo wrote: > > Having a module option is easy and can be helpful for the very rare > cases where the admin is aware of the issue, but, in general, it > doesn't really help that much. How about a sysfs attribute, rather than a boot option? -- 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
On 13-01-11 08:53 AM, Mark Lord wrote: > On 13-01-09 02:17 PM, Tejun Heo wrote: >> >> Having a module option is easy and can be helpful for the very rare >> cases where the admin is aware of the issue, but, in general, it >> doesn't really help that much. > > How about a sysfs attribute, rather than a boot option? NAK that idea.. a boot-time module flag is the best way. -- 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
On 01/11/2013 08:54 AM, Mark Lord wrote: > On 13-01-11 08:53 AM, Mark Lord wrote: >> On 13-01-09 02:17 PM, Tejun Heo wrote: >>> >>> Having a module option is easy and can be helpful for the very rare >>> cases where the admin is aware of the issue, but, in general, it >>> doesn't really help that much. >> >> How about a sysfs attribute, rather than a boot option? > > NAK that idea.. a boot-time module flag is the best way. Module options appear in sysfs anyway. If the issue continues to be seen, a knob to enable an ugly solution is better than nothing, from the user's perspective. Jeff -- 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
On Monday 14 January 2013 18:58:55 Jeff Garzik wrote: > If the issue continues to be seen, You mean, more than it has been seen already? > a knob to enable an ugly solution is > better than nothing, from the user's perspective. Also, I fail to see how this is any more ugly than what the current code does. -- 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
--- linux-2.6.27.27/drivers/ata/sata_sil.0.c 2009-07-20 05:45:22.000000000 +0200 +++ linux-2.6.27.27/drivers/ata/sata_sil.c 2012-12-24 10:09:51.000000000 +0100 @@ -246,17 +246,25 @@ static int slow_down; module_param(slow_down, int, 0444); MODULE_PARM_DESC(slow_down, "Sledgehammer used to work around random problems, by limiting commands to 15 sectors (0=off, 1=on)"); +static int cache_line_workaround = -1; +module_param(cache_line_workaround, int, 0444); +MODULE_PARM_DESC(cache_line_workaround, "Work around data corruption problem on some motherboards (0 to disable, 1 or 2 to enable different workarounds)"); + static unsigned char sil_get_device_cache_line(struct pci_dev *pdev) { u8 cache_line = 0; pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cache_line); return cache_line; } +static void sil_set_device_cache_line(struct pci_dev *pdev, u8 val) +{ + pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, val); +} /** * sil_set_mode - wrap set_mode functions * @link: link to set up * @r_failed: returned device when we fail @@ -555,30 +563,100 @@ dev->udma_mask &= ATA_UDMA5; return; } } +static int sil_detect_problematic_chipset(void) +{ + /* it could be added later, i wouldn't worry about it now */ + return 0; +} + +static int sil_get_cache_line_workaround(struct pci_dev *pdev) +{ + if (cache_line_workaround != -1) { /* set by user */ + dev_printk(KERN_INFO, &pdev->dev, + "cache_line_workaround=%d given\n", cache_line_workaround); + return cache_line_workaround; + } else if (slow_down) { + /* users have set slow_down because they have experienced problems. lower performance is expected. maybe it makes sense to enable it for them. */ + dev_printk(KERN_INFO, &pdev->dev, + "slow_down given, using cache_line_workaround=1\n"); + return 1; + } else if (sil_detect_problematic_chipset()) { + dev_printk(KERN_INFO, &pdev->dev, + "problematic motherboard chipset detected, using cache_line_workaround=1\n"); + return 1; + } else { + /* default to old behavior */ + return 0; + } +} + static void sil_init_controller(struct ata_host *host) { struct pci_dev *pdev = to_pci_dev(host->dev); void __iomem *mmio_base = host->iomap[SIL_MMIO_BAR]; + int clw; u8 cls; + u8 fifo_cfg_val; u32 tmp; int i; /* Initialize FIFO PCI bus arbitration */ - cls = sil_get_device_cache_line(pdev); - if (cls) { - cls >>= 3; - cls++; /* cls = (line_size/8)+1 */ - for (i = 0; i < host->n_ports; i++) - writew(cls << 8 | cls, - mmio_base + sil_port[i].fifo_cfg); - } else - dev_printk(KERN_WARNING, &pdev->dev, - "cache line size not set. Driver may not function\n"); - + clw = sil_get_cache_line_workaround(pdev); + switch (clw) { + case 0: + dev_printk(KERN_WARNING, &pdev->dev, + "Data corruption is known to occur in combination with certain motherboards. If you encounter it, try cache_line_workaround=1 parameter.\n"); + cls = sil_get_device_cache_line(pdev); + if (cls) { + dev_printk(KERN_DEBUG, &pdev->dev, + "cache line size: %d bytes\n", cls*4); + fifo_cfg_val = cls/8 + 1; + if (fifo_cfg_val > 7) fifo_cfg_val = 7; /* don't go over the maximum value */ + dev_printk(KERN_DEBUG, &pdev->dev, + "setting fifo_cfg to %d\n", fifo_cfg_val); + for (i = 0; i < host->n_ports; i++) { + writew(fifo_cfg_val << 8 | fifo_cfg_val, + mmio_base + sil_port[i].fifo_cfg); + } + } else { + dev_printk(KERN_INFO, &pdev->dev, + "cache line size not set\n"); + } + break; + case 1: + dev_printk(KERN_INFO, &pdev->dev, + "cache_line_workaround=1, setting cache line size to 0\n"); + sil_set_device_cache_line(pdev, 0); + break; + case 2: + dev_printk(KERN_INFO, &pdev->dev, + "cache_line_workaround=2. Increasing cache line size to 64 bytes, if necessary, and setting fifo_cfg to 0.\n"); + cls = sil_get_device_cache_line(pdev); + dev_printk(KERN_DEBUG, &pdev->dev, + "current CLS: %d bytes\n", cls*4); + if (cls < 0x10) { + dev_printk(KERN_DEBUG, &pdev->dev, + "increasing CLS to 64 bytes\n"); + sil_set_device_cache_line(pdev, 0x10); + } else { + dev_printk(KERN_DEBUG, &pdev->dev, + "keeping current CLS\n"); + } + dev_printk(KERN_DEBUG, &pdev->dev, + "setting fifo_cfg to 0\n"); + for (i = 0; i < host->n_ports; i++) { + writew(0, mmio_base + sil_port[i].fifo_cfg); + } + break; + default: + dev_printk(KERN_ERR, &pdev->dev, + "invalid cache_line_workaround: %d\n", clw); + } + /* Apply R_ERR on DMA activate FIS errata workaround */ if (host->ports[0]->flags & SIL_FLAG_RERR_ON_DMA_ACT) { int cnt; for (i = 0, cnt = 0; i < host->n_ports; i++) {