Re: [PATCH 2/8] ide: add ide_set_irq() inline helper

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

Hello, I wrote:

>>>>>> Index: b/drivers/ide/ide-iops.c
>>>>>> ===================================================================
>>>>>> --- a/drivers/ide/ide-iops.c
>>>>>> +++ b/drivers/ide/ide-iops.c
>>>>>> @@ -688,8 +688,7 @@ int ide_driveid_update(ide_drive_t *driv
>>>>>>      */
>>>>>>
>>>>>>     SELECT_MASK(drive, 1);
>>>>>> -    if (IDE_CONTROL_REG)
>>>>>> -        hwif->OUTB(drive->ctl,IDE_CONTROL_REG);
>>>>>> +    ide_set_irq(drive, 1);

>>>>>    If we're going to execute the command using polling, isn't it 
>>>>> logical to *disable* drive's interrupt instead of enabling it which 
>>>>> this code is currently doing?  This looks like it might work only 
>>>>> for the drivers having the maskproc() method (of which hpt366.c is 
>>>>> the only one that I've ever dealt with).

>>>> Yes, this needs fixing.

>>>     But should we honor drive->quirk_list here? What its different 
>>> values mean? I'm seeing either 1 or 2 is used to decide whether to 
>>> set nIEN or not...

>> I did some research on ->quirk_list in the past but I couldn't exactly
>> figure it out.  I also wasn't able to trace this code to its author
>> (not that I tried very hard)...

>> Anyway my current findings/theories are the following:

>> - we should make sure that all SELECT_MASK() users also call ide_set_irq()
>>   (then sgiioc4's ->maskproc can be removed since it is a copy of ->set_irq)

>   I've kind of made sure about that already, 

    After another look, I've indeed correctly described this as "kind of" --
since ide_set_irq() is called only before issuing a command, while
SELECT_MASK() is called both before (but it seems to be always skipped due to 
IDE_TFLAG_NO_SELECT_MASK being set for the commands executed in a normal way, 
i.e. using interrupts) and after that (the latter is done if we are using the 
polling mode). So, those maskproc() methods in hpt366 and sgiioc4 in fact 
inadvertently clear nIEN after the polled command execution -- not that it's 
wrong but why the hell? And those maskproc() methods duplicate nIEN setup 
before the command execution -- except in ide_config_drive_speed() where 
there's an obvious error...

> hence I've already created a patch doing that...

    As I have no idea why sgiioc4 needs to manipulate nIEN, I guess I'll just 
CC the SGI people on that patch...

>> Thanks,
>> Bart

MBR, Sergei