Re: [PATCH] ata_piix: make DVD Drive recognisable on systems with Intel Sandybridge chipsets(v1)

[Sergei Shtylyov <sshtylyov@mvista.com>]

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@intel.com>
>>>>> Cc: Alan Cox<alan@linux.intel.com>
>>>>> Signed-off-by: Ming Lei<ming.lei@canonical.com>
>>>>> Signed-off-by: Tejun Heo<htejun@gmail.com>
>>>>> ---
>>>>>   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