Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Karl Hiramoto <karl@hiramoto.org>]

Sergei Shtylyov wrote:
> Hello, I wrote:
>
>>>> +
>>>> +    reset = hwif->channel ? 0x80 : 0x40;
>
>>    These HighPoint's "soft reset" (WTF does that mean I wonder?) bits 
>> are not the same between HPT366 and HPT370, so this code doesn't look 
>> right...
>
>    Ah, these "soft" reset bits correspond to PRST_/SRST_ signals which 
> are really primary/secondary channel hard resets (RESET- signal). This 
> explains everything; and this patch is a pure hack then.
>
>>>> +
>>>> +    pci_read_config_byte(dev, 0x59, &reg59h);
>>>> +    pci_write_config_byte(dev, 0x59, reg59h|reset);
>>>> +    pci_write_config_byte(dev, 0x59, reg59h);
>>>> +}
>>>> +
>>>> static void __devinit init_hwif_hpt366(ide_hwif_t *hwif)
>>>> {
>>>>     struct hpt_info *info    = hpt3xx_get_info(hwif->dev);
>>>> @@ -1406,6 +1431,7 @@ static const struct ide_port_ops 
>>>> hpt3xx_port_ops = {
>>>>     .set_pio_mode        = hpt3xx_set_pio_mode,
>>>>     .set_dma_mode        = hpt3xx_set_mode,
>>>>     .quirkproc        = hpt3xx_quirkproc,
>>>> +    .resetproc        = hpt3xx_reset,
>>>>     .maskproc        = hpt3xx_maskproc,
>>>>     .mdma_filter        = hpt3xx_mdma_filter,
>>>>     .udma_filter        = hpt3xx_udma_filter,
>>>> diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
>>>> index 7f264ed..09295b4 100644
>>>> --- a/drivers/ide/ide-probe.c
>>>> +++ b/drivers/ide/ide-probe.c
>>>> @@ -367,6 +367,7 @@ static int do_probe (ide_drive_t *drive, u8 cmd)
>>>> {
>>>>     ide_hwif_t *hwif = drive->hwif;
>>>>     const struct ide_tp_ops *tp_ops = hwif->tp_ops;
>>>> +    const struct ide_port_ops *port_ops = hwif->port_ops;
>>>>     u16 *id = drive->id;
>>>>     int rc;
>>>>     u8 present = !!(drive->dev_flags & IDE_DFLAG_PRESENT), stat;
>>>> @@ -427,9 +428,20 @@ static int do_probe (ide_drive_t *drive, u8 cmd)
>>>>         /* ensure drive IRQ is clear */
>>>>         stat = tp_ops->read_status(hwif);
>>>>
>>>> -        if (rc == 1)
>>>> +        if (rc == 1) {
>>>>             printk(KERN_ERR "%s: no response (status = 0x%02x)\n",
>>>>                     drive->name, stat);
>>>> +
>>>> +            if (port_ops->resetproc) {
>>>> +                port_ops->resetproc(drive);
>>>> +                msleep(50);
>
>>    Karl, why are you calling resetproc() without doing a reset? What 
>> this achieves?
>
>    To reset the drives, apparently. But it's not the function of 
> resetproc(). We should try soft-reset instead.
>
Yes, to reset the drive that does not respond.

>>>> +            }
>>>> +            tp_ops->dev_select(drive);
>>>> +            msleep(50);
>>>> +            tp_ops->exec_command(hwif, ATA_CMD_DEV_RESET);
>>>> +            (void)ide_busy_sleep(hwif, WAIT_WORSTCASE, 0);
>>>> +            rc = ide_dev_read_id(drive, cmd, id);
>>>> +        }
>>>>     } else {
>>>>         /* not present or maybe ATAPI */
>>>>         rc = 3;
>
>>> Since the current code in ide-probe.c looks like this:
>
>>>         stat = tp_ops->read_status(hwif);
>>>
>>>         if (stat == (ATA_BUSY | ATA_DRDY))
>>>             return 4;
>>>
>>>         if (rc == 1 && cmd == ATA_CMD_ID_ATAPI) {
>>>             printk(KERN_ERR "%s: no response (status = 0x%02x), "
>>>                     "resetting drive\n", drive->name, stat);
>>>             msleep(50);
>>>             tp_ops->dev_select(drive);
>>>             msleep(50);
>>>             tp_ops->exec_command(hwif, ATA_CMD_DEV_RESET);
>>>             (void)ide_busy_sleep(hwif, WAIT_WORSTCASE, 0);
>>>             rc = ide_dev_read_id(drive, cmd, id);
>>>         }
>>>
>>>         /* ensure drive IRQ is clear */
>>>         stat = tp_ops->read_status(hwif);
>>>
>>>         if (rc == 1)
>>>             printk(KERN_ERR "%s: no response (status = 0x%02x)\n",
>>>                     drive->name, stat);
>
>>> I would really prefer fixing ATAPI case while we are at it
>>> (+ this would also get rid of code duplication).
>
>>> IOW:
>
>>> - in patch #1 we would add ->resetproc call
>
>>    I would first like to hear an explanation of what it does...
>
>    I have to NAK this resetproc() addition. It tries to do things that 
> it's not designed to do. We should instead try soft-resetting the 
> channel...

Where should the soft reset be done, for this to not be called a hack?   
should it be in ide_port_ops->reset_poll?
>
>>> Care to revise your patch?
>
>    I see no point in "revising" this hack now...

Why not reset the drive if it does not respond?  OK, you could blame 
this on a bugy board, or redboot that does not reset the drive on warm 
boot if it is busy.


--
Karl