pata_it821x: Add module param to force UDMA on Vortex86SX
PATCH 1/1 pata_it821x: Add module param to force UDMA on Vortex86SX
This patch adds parameter 'it8212_rdc11_force_udma' to 'pata_it821x' module, which allows users to force UDMA mode on RDC11/Vortex86SX embedded devices.
---
UDMA is disabled by default due to some CRC errors, which i cannot observe when using CompactFlash card with patched driver.
May be there are errors with pure IDE devices or with older revisions of Vortex86SX boards, i do no know. But for my device i have 2x improvement in I/O speed when enabling UDMA/33 instead of MWDMA, so i think this parameter would be useful.
Also fix to initialize default value for parameter 'noraid'.
drivers/ata/pata_it821x.c 2011-03-26 15:10:49.608952573 +0300 renamed from drivers/ata/pata_it821x.c 2011-03-23 23:04:47.000000000 +0300 +7 -3
@@ -111,7 +111,8 @@ struct it821x_dev * device. */ -static int it8212_noraid; +static int it8212_noraid = 0; +static int it8212_rdc11_force_udma = 0; /** * it821x_program - program the PIO/MWDMA registers
@@ -913,7 +914,7 @@ static int it821x_init_one(struct pci_de if (pdev->vendor == PCI_VENDOR_ID_RDC) { /* Deal with Vortex86SX */ - if (pdev->revision == 0x11) + if (pdev->revision == 0x11 && !it8212_rdc11_force_udma) ppi[0] = &info_rdc_11; else ppi[0] = &info_rdc;
@@ -988,9 +989,12 @@ MODULE_LICENSE("GPL"); MODULE_DEVICE_TABLE(pci, it821x); MODULE_VERSION(DRV_VERSION); - module_param_named(noraid, it8212_noraid, int, S_IRUGO); MODULE_PARM_DESC(noraid, "Force card into bypass mode"); +module_param_named(rdc11_force_udma, it8212_rdc11_force_udma, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(rdc11_force_udma, "Force enable UDMA mode RDC11/Vortex86SX boards." + " Use with caution"); + module_init(it821x_init); module_exit(it821x_exit);
Quoted 6 lines
May be there are errors with pure IDE devices or with older revisions of Vortex86SX
boards, i do no know. But for my device i have 2x improvement in I/O speed when
enabling UDMA/33 instead of MWDMA, so i think this parameter would be useful.
"I do not know" is not a good basis for hacking storage code, particularly if it was to turn out that the reason was something like silent data corruption (eg as with UDMA on the old OSB4 controllers in some cases)
The change came via DMP signoff (the manufacturer) so any adjustment of this sort of checking really ought to go via DMP as well.
Quoted 1 line
Also fix to initialize default value for parameter 'noraid'.
This does not need initialising - C guarantees static variables start at zero. If that makes a difference you have other problems.
So NAK this.
Although if you want to take it up with DMP and find out if they have updated rules or checks that want pushing that might be useful.
Alan
Quoted 10 lines · Alan Cox
Quoted 9 lines
May be there are errors with pure IDE devices or with older
revisions
of Vortex86SX
boards, i do no know. But for my device i have 2x improvement in
I/O
speed when
enabling UDMA/33 instead of MWDMA, so i think this parameter would
be
useful.
Quoted 5 lines
"I do not know" is not a good basis for hacking storage code, particularly if it was to turn out that the reason was something like silent data corruption (eg as with UDMA on the old OSB4 controllers in some cases)
Well, by "i do not know" i meant that i do not have other (older) DMP hardware, nor do i have means to connect pure IDE device to my device (DMP-2300).
I just can state that i was having a root FS on a CompactFlash for quite a long time with UDMA enabled, and i have not observed any problems, and have seen no signs of data corruption either.
I am just proposing to give users an option to enable UDMA at their own risk, maybe add comment stating that enabling this option may or may not cause silent data corruption. Or maybe to add this as a sub-option to the driver in Kconfig to switch on/off at compile time with detailed description under ---help---
Quoted 7 lines
The change came via DMP signoff (the manufacturer) so any adjustment of this sort of checking really ought to go via DMP as well.
Quoted 1 line
Also fix to initialize default value for parameter 'noraid'.
This does not need initialising - C guarantees static variables start at zero. If that makes a difference you have other problems.
Sorry, my bad.
Quoted 4 lines
So NAK this.
Although if you want to take it up with DMP and find out if they have updated rules or checks that want pushing that might be useful.
No, i do not intend to contact them, i just wanted people to have an on/off switch in kernel, not to push this patch by all means necessary.
If this option is considered by community unneeded, i'll just have to patch my own kernel with every upgrade :)
Quoted 1 line
Alan
Quoted 6 lines
Quoted 2 lines
Although if you want to take it up with DMP and find out if they have updated rules or checks that want pushing that might be useful.
No, i do not intend to contact them, i just wanted people to have an on/off
switch in kernel, not to push this patch by all means necessary.
If you want a patch in the upstream kernel then you need to do the related work. If you can't even be bothered to follow up why an erratum was added by the manufacturer then you've comprehensively missed the standards we work to.
Quoted 3 lines
If this option is considered by community unneeded, i'll just have to patch my
own kernel with every upgrade :)
By all means. The GPL ensures you can do this and it means you won't leave us with a one off potentially unsafe and unmaintained update.
Alan