Hi Linus,
On Wed, Feb 26, 2020 at 09:49:05AM -0800, Linus Torvalds wrote:
> On Wed, Feb 26, 2020 at 6:57 AM Denis Efremov <efremov@xxxxxxxxx> wrote:
> >
> > If Linus has no objections (regarding his review) I would prefer to
> > accept 1-10 patches rather to resend them again. They seems complete
> > to me as the first step.
>
> I have no objections, and the patches 11-16 seem to have addressed all
> my "I wish.." concerns too (except for the "we should rewrite or
> sunset the driver entirely"). Looks fine to me.
So I continued to work on this cleanup and regardless of the side I
attacked this hill, it progressively stalled once I got closer to
the interrupt and delayed work stuff. I understood the root cause of
the problem, it's actually double:
- single interrupt for multiple FDCs : this means we need to have
some global context to know what we're working with. It is not
completely true because we could very well have a list of pending
operations per FDC to scan on interrupt and update them when the
interrupt strikes and the FDC reports a completion. I noticed that
raw_cmd, raw_buffer, inr, current_req, _floppy, plus a number of
function pointers used to handle delayed work should in fact be
per-FDC if we didn't want to change them at run time ;
- single DMA channel for multiple FDCs : regardless of what could
be done for the interrupt above, we're still stuck with a single
DMA setup at once, which requires to issue reset_fdc() here and
there, making it clear we can't work with multiple FDCs in parallel.
Given the number of places where such global variables are set, I'm
not totally confident in the fact that nowhere we keep a setting
that was assigned for the FDC in the previous request. For example
a spurious (or late) interrupt could slightly affect the fdc_state[]
and maybe even emit FD_SENSEI to the current controller. Also this
comment in floppy_interrupt() made me realize the driver pre-dates
SMP support:
/* It is OK to emit floppy commands because we are in an interrupt
* handler here, and thus we have to fear no interference of other
* activity.
*/
It seems that changing the current FDC is still only protected by
the fdc_busy bit, but that the interrupt handler doesn't check
if anything is expected before touching bits related to current_fdc.
All this made me wonder if we really want to continue to maintain the
support for multiple FDCs. I checked all archs, and only x86, alpha
and powerpc support more than one FDC, 2 to be precise (hence up to
8 drives). I have the impression that if we keep a single FDC we'll
have a cleaner code that doesn't need to change global settings under
us when doing resets or scans. So I don't know if that's something
interesting to pursue.
I also noticed that a lot of global variables are inter-dependent and
should probably be moved to fdc_state[] so that what's per-FDC is now
more explicit, except that this struct is exposed to userland via
the FDGETFDCSTAT ioctl (but that could be changed so that the fdc_state
is just a struct inside a per-fdc larger struct).
At least now I get a better picture of the little ROI felt trying to
clean this, and I don't think that even a complete rewrite as you
suggested would address all the issues at all.
So if you or Denis think there's some value in me continuing to explore
one of these areas, I can continue, otherwise I can simply resend the
last part of my series with the few missing Cc and be done with it.
Willy
