lkml.org 
[lkml]   [2008]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl
    > Why the *hell* doesn't it just fix "ata_sff_altstatus()" instead? Why does 
    > it introduce a ludicrously named stupid "maybe" version of it that doesn't
    > oops?

    Ok the problem is we have three cases to distinguish

    - altstatus must be used - in which case we want to blow up anyway if we
    touch it (the usual case)
    - altstatus should be used if available - shared IRQ check
    - altstatus being used for flushing - altstatus irrelevant, it just has
    to flush somehow.

    So the _maybe naming sucks, but the reasoning I think is sound. The other
    way to do it would be to replace it and the bit of irq handler logic with
    an ata_sff_busy() that did the status checks correctly for both ctl and
    non-ctl capable devices.

    > It may be that you meant to make it an "else if" case, ie if there was no
    > IO-read, then you do a ndelay(400) as a last desperate case, but that's
    > not how your ata_sdd_sync() is actually written.

    The ndelay(400) is correct. The IO-read is Jeff being paranoid and
    actually hurts us materially for the usual PIO case (bus PIO not disk
    PIO) to the tune of about 1mS a command in many cases, but is needed for
    MMIO (which we almost never do for any SFF hardware). That itself is a
    different problem that can be fixed later (and not in -rc5). It wants
    fixing as its a key reason that old IDE is still faster for PATA.

    maybe_altstatus is crap naming but simply making ata_sff_altstatus fake a
    reply in arbitary cases risks not catching mistakes and could mean we
    don't catch corrupting mistakes which would be very bad indeed.



    \
     
     \ /
      Last update: 2008-05-29 20:21    [W:2.347 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site