On Thu, Oct 6, 2011 at 6:39 PM, Sergei Shtylyov <sshtylyov@xxxxxxxxxx> wrote: > Hello. > > On 06-10-2011 13:27, 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. Anyway, I have not see obvious drawbacks to define a global ata_port flag. thanks, -- Ming Lei -- 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
