Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello, I wrote:

>>>>>> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
>>>>>>  pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

>>>>> Before using pci_resource_start(), the code should check 
>>>>> pci_resource_len() to ensure it is the appropriate size.

>>>>> It would be very wise to ensure that 
>>>>> pci_resource_flags()&IORESOURCE_IO is true, too.

>>>> Sure but since the old code hasn't been doing these checks (old code
>>>> just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
>>>> this is a separate issue from what the patch tries to achieve.

>>> Nonetheless this is duplicating an existing bug $N times... not ideal :)

>> The existing bug is that the methods converted to use 
>> pci_resource_start()
>> may be set in hwif instance and used even if the BAR4 is invalid.  
>> Therefore
>> the above patch is correct and it isn't duplicating the existing bug. :)

>> However I agree that all these pci_resource_start()s could be a bit 
>> confusing
>> so here is 'take 2'.

>    Yeah, they are.  Actually, I was thinking about such patch ~1.5 years 
> ago *but* using a different approach -- namely, hwif->extra_base.

    Yes, the code introducing this field was written with replacing dma_master 
by it in mind... It's a pity I haven't gotten around to it.

[...]

>> Index: b/drivers/ide/pci/hpt366.c
>> ===================================================================
>> --- a/drivers/ide/pci/hpt366.c
>> +++ b/drivers/ide/pci/hpt366.c
>> @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

>    This just asks to be:

    No, it doesn't cause that way one would only break it.

>>  static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
>>  {
>     unsigned long base = hwif->extra_base;

    Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...

>     u8 scr2 = inb(base + 0x5b);
> 
>      if ((scr2 & 0x7f) == mode)
>          return;
> 
>      /* Tristate the bus */
>     outb(0x80, base + 0x53);
>     outb(0x80, base + 0x57);
> 
>      /* Switch clock and reset channels */
>     outb(mode, base + 0x5b);
>     outb(0xc0, base + 0x59);
> 
>      /*
>       * Reset the state machines.
>        * NOTE: avoid accidentally enabling the disabled channels.
>       */
>     outb(inb(base + 0x50) | 0x32, base + 0x50);
>     outb(inb(base + 0x54) | 0x32, base + 0x54);
> 
>      /* Complete reset */
>     outb(0x00, base + 0x59);
> 
>     /* Reconnect channels to bus */
>     outb(0x00, base + 0x53);
>     outb(0x00, base + 0x57);
> }

MBR, Sergei