linux-kernel - Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOmrzkJfQu6PuCpyyRJL8PNvSYxYO42F-k8jEYf2d1-5C28A_A@mail.gmail.com>
Date:   Wed, 11 Jan 2017 09:02:36 +1300
From:   Michael Schmitz <schmitzmic@...il.com>
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     Tejun Heo <tj@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-ide@...r.kernel.org,
        "Linux/m68k" <linux-m68k@...ts.linux-m68k.org>,
        Linux Kernel Development <linux-kernel@...r.kernel.org>,
        Andreas Schwab <schwab@...ux-m68k.org>
Subject: Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

Bartlomiej,


>> How is polling implemented in libata? Sleeping for something
>> approximating the average seek latency shouldn't hurt but spinning
>> wont be acceptable for a low performance single CPU architecture like
>> the Falcon.
>
> You can find actual implementation in libata-sff.c.
>
> Please see ata_sff_pio_task() for the main polling logic:
>
> fsm_start:
>         WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
>
>         /*
>          * This is purely heuristic.  This is a fast path.
>          * Sometimes when we enter, BSY will be cleared in
>          * a chk-status or two.  If not, the drive is probably seeking
>          * or something.  Snooze for a couple msecs, then
>          * chk-status again.  If still busy, queue delayed work.
>          */
>         status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
>         if (status & ATA_BUSY) {
>                 spin_unlock_irq(ap->lock);
>                 ata_msleep(ap, 2);
>                 spin_lock_irq(ap->lock);
>
>                 status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
>                 if (status & ATA_BUSY) {
>                         ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
>                         goto out_unlock;
>                 }
>         }
>
>         /*
>          * hsm_move() may trigger another command to be processed.
>          * clean the link beforehand.
>          */
>         ap->sff_pio_task_link = NULL;
>         /* move the HSM */
>         poll_next = ata_sff_hsm_move(ap, qc, status, 1);
>
>         /* another command or interrupt handler
>          * may be running at this point.
>          */
>         if (poll_next)
>                 goto fsm_start;
> out_unlock:
>
>
> ata_sff_busy_wait()'s last argument is number of 10uS waits so
> first check (50uS) should be quite quick.  If device is still
> busy we sleep for 2ms.  Then we quickly (100uS) busy wait and
> if needed  queue delayed work (with ATA_SHORT_PAUSE == 16 ms
> delay).
>
> Overall the current heuristic looks fine and spinning should be
> minimal.

Thanks, that looks entirely reasonable.

>> > Tested under ARAnyM emulator.
>>
>> Not sure that the emulator is really feature complete enough - I'll
>> get this tested on my Falcon in the next few weeks. I'm a bit worried
>
> Great!
>
>> about IDE still generating interrupts on seek completion (did you spot
>> anything like that, Geert?).
>>
>> Cheers,
>>
>>   Michael
>
> BTW according to comment in arch/m68k/atari/stdma.c:
>
> /* On the Falcon, the IDE bus uses just the ACSI/Floppy interrupt, but */
> /* not the ST-DMA chip itself. So falhd.c needs not to lock the        */
> /* chip. The interrupt is routed to falhd.c if IDE is configured, the  */
> /* model is a Falcon and the interrupt was caused by the HD controller */
> /* (can be determined by looking at its status register).              */

That comment is probably incorrect in part. Blame Linus :-)

Seriously though - that comment dates back to the dark ages (when m68k
was an entirely separate port and the IDE driver was indeed named
falhd.c). That would have been even before 2.2 or 2.4 times. The
comment just never got updated.

What is still correct is that the IDE driver does use the interrupt
only, not the ST-DMA chip. And a single IDE interrupt can be correctly
assigned to IDE by looking at the status register.

With the SCSI (and IIRC also floppy) interrupts, we don't have direct
access to the status registers without disturbing the state of the DMA
though. Unless we know for definite that either chips have raised the
interrupt (and DMA ops are in flight), we must not touch the DMA chip
at all.

The case I'm worried about is both IDE and SCSI raising an interrupt.
We don't currently mask the IDE/ST-DMA interrupt so a stacked
interrupt must be processed in the same pass as the initial interrupt
or it will get dropped. We'd have to peek at the DMA registers to
check the SCSI or floppy interrupt status, and we just can't safely do
that. So races of this kind are currently prevented by including IDE
in the IRQ locking process.

Whether it's possible to mask the interrupt, do one pass, unmask and
process the second interrupt I don't know. Maybe Andreas does?

Cheers,

  Michael


> it should be okay to use IDE at the same time as SCSI/Floppy which
> is what the new driver does (the old one is serialized operations by
> ST-DMA related IRQ handling magic).
>
> Also the comment itself may need some fixups as on Falcon it is SCSI
> not ACSI (according to the earlier comment in same file) and the old
> IDE host driver name is not falhd.c but falconide.c.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>