Hello. On 10/06/2011 03:53 PM, Ming Lei wrote:
This quirk patch fixes one kind of bug inside some Intel Sandybridge chipsets, see reports from
https://bugzilla.kernel.org/show_bug.cgi?id=40592.
Many guys also have reported the problem before:
https://bugs.launchpad.net/bugs/737388 https://bugs.launchpad.net/bugs/794642 https://bugs.launchpad.net/bugs/782389 ......
With help from Tejun, the problem is found to be caused by 32bit PIO mode, so introduce the quirk patch to disable 32bit PIO on SATA piix for some Sandybridge CPT chipsets.
Seth also tested the patch on all five affected chipsets (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found the patch does fix the problem.
Tested-by: Heasley, Seth<seth.heasley@xxxxxxxxx> Cc: Alan Cox<alan@xxxxxxxxxxxxxxx> Signed-off-by: Ming Lei<ming.lei@xxxxxxxxxxxxx> Signed-off-by: Tejun Heo<htejun@xxxxxxxxx> --- drivers/ata/ata_piix.c | 37 ++++++++++++++++++++++++++++++++----- include/linux/libata.h | 1 + 2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c index 43107e9..70095be 100644 --- a/drivers/ata/ata_piix.c +++ b/drivers/ata/ata_piix.c @@ -113,6 +113,8 @@ enum { PIIX_PATA_FLAGS = ATA_FLAG_SLAVE_POSS, PIIX_SATA_FLAGS = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR, + PIIX_FLAG_PIO16 = ATA_FLAG_PIO16,
It's not clear why are you declaring a ganeric flag and then add a local name for it...
It is just same with other flags, isn't it?
No. Read the the ata_piix.c source attentively, looking for PIIX_FLAG_* definitions.
diff --git a/include/linux/libata.h b/include/linux/libata.h index efd6f98..dc68de5 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -207,6 +207,7 @@ enum { ATA_FLAG_SW_ACTIVITY = (1<< 22), /* driver supports sw activity * led */ ATA_FLAG_NO_DIPM = (1<< 23), /* host not happy with DIPM */ + ATA_FLAG_PIO16 = (1<< 24), /*16bit PIO */
Please, fix the formatting. Or better totally remove this.
It is used to describe if the controller only supports 16bit PIO, and it is introduced to fix the problem on SNB chips.
I don't yet see why it should be a generic flag: we have ata_bmdma_port_ops and ata_bmdma32_port_ops to be used for 16-bit and 32-bit PIO. You're only using this flag locally to ata_piix.c, hence it should be local to that file.
This flag is to stored into ata_port flags, so it is better to define a global flag. Otherwise, you have to select a value carefully which must not be defined in ata_port flags already, which way is very error-prone and not friendly.
If everyone starts defining global flags for their local cases, we'll run out of flags rather quickly. :-)
Anyway, I have not see obvious drawbacks to define a global ata_port flag.
Then at least there's no need to define a local alias for the global flag, don't you think?
thanks, -- Ming Lei
WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
