Re: [PATCH 8/15] ide: disable DMA in ->ide_dma_check for "no IORDY" case

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

Hello.

Bartlomiej Zolnierkiewicz wrote:

> > > The other advantage of doing cleanups is that code becomes cleaner/simpler
> > > which matters a lot for this codebase, i.e. ide-dma-off-void.patch exposed
> > > (yet to be fixed) bug in set_using_dma() (->ide_dma_off_quietly always returns
> > > 0 which is passed by ->ide_dma_check to set_using_dma() which incorrectly
> > > then calls ->ide_dma_on).

> >   Well, this seems a newly intruduced bug.

> The old code is so convulted that it is hard to see it w/o cleanup. :)

> ->ide_dma_check implementations often do

> 		return hwif->ide_dma_off_quietly(drive);

> so the return value of ide_dma_off_quietly() (which is always 0) is passed to

> 			if (HWIF(drive)->ide_dma_check(drive)) return -EIO;

> in ide.c:set_using_dma() -> as a result the next line is executed

> 			if (HWIF(drive)->ide_dma_on(drive)) return -EIO;

   Ah, indeed! Nice. :-)

> >   It's all fine but goes somewhat against Linus' policy as far as I
> > understnad it: fixes are merged all the time while cleanups (along with new
> > code) are merged mostly duting the merge window.

> > > Moreover I don't find the current tree to be more of cleanups than fixes,
> > > here is the analysis of current series file:

> >   Maybe I slightly exaggerated, being impressed by the volume of your recent
> > changes. :-)
> >   But still...

> > > #
> > > # IDE patches from 2.6.20-rc3-mm1
> > > #
> > > toshiba-tc86c001-ide-driver-take-2.patch
> > > toshiba-tc86c001-ide-driver-take-2-fix.patch
> > > toshiba-tc86c001-ide-driver-take-2-fix-2.patch
> > >      -- new driver

> >    I'd count that as cleanup, since it's definitely not fix. ;-)

> > > hpt3xx-rework-rate-filtering.patch
> > > hpt3xx-rework-rate-filtering-tidy.patch
> > > hpt3xx-print-the-real-chip-name-at-startup.patch
> > > hpt3xx-switch-to-using-pci_get_slot.patch
> > > hpt3xx-cache-channels-mcr-address.patch
> > > hpt3x7-merge-speedproc-handlers.patch
> > > hpt370-clean-up-dma-timeout-handling.patch
> > > hpt3xx-init-code-rewrite.patch
> > > piix-fix-82371mx-enablebits.patch
> > > piix-tuneproc-fixes-cleanups.patch
> > > slc90e66-carry-over-fixes-from-piix-driver.patch
> > > hpt36x-pci-clock-detection-fix.patch
> > > jmicron-warning-fix.patch
> > >      -- fixes (but most have cleanups mixed in)

> >   Yeah, but not that those came in from the -mm tree.

   Oops, "not that" didn't make sense here :-)

> > > ide-mmio-flag.patch
> > >      -- cleanup
> > > hpt34x-tune-chipset-fix.patch
> > >      -- fix
> > > ide-fix-pio-fallback.patch
> > >      -- fix

> >   Those 2 are seem more of a cleanup to me...

> They fix real but quite hard to hit bugs.
> I'll put them at the end of the fixes series.

   Well, most of the recent fixes were for such issues.  Nobody had screamed 
about them, it took a code review to find them. :-)

> > > ide-set-dma-helper.patch
> > > ide-dma-off-void.patch
> > > ide-dma-host-on-void.patch
> > > ide-fix-dma-masks.patch
> > > ide-max-dma-mode.patch
> > > ide-tune-dma-helper.patch
> > >      -- cleanups

> >   Would make sense to keep those last in the tail of queue because of the
> > amount of changes they introduce.  Possibly even IDE subsystem wide cleanups

> They are at the end already - no problem here. :)

   I meant "in the future"...

> > > and if you would like me to shuffle ordering of the patches (but without
> > > need of rewritting them) it also OK

> >   Erm, no talking about the rewrite but that way you may have to rebase
> > cleanups on top of fixes.  This seems unavoidble though due to the way the
> > kernel patch acceptance process is working, as far as I understand it...

> I'll change the ordering of the patches based on your suggestions
> and generally try to keep such order of fixes first and cleanups later.

   Thanks. :-)

> > > > > > > Index: b/drivers/ide/pci/cmd64x.c
> > > > > > > ===================================================================
> > > > > > > --- a/drivers/ide/pci/cmd64x.c
> > > > > > > +++ b/drivers/ide/pci/cmd64x.c
> > > > > > > @@ -479,12 +479,10 @@ static int cmd64x_config_drive_for_dma (
> > > > > > >    if (ide_use_dma(drive) && config_chipset_for_dma(drive))
> > > > > > >            return hwif->ide_dma_on(drive);

> > > > > > > -     if (ide_use_fast_pio(drive)) {
> > > > > > > +     if (ide_use_fast_pio(drive))
> > > > > > >            config_chipset_for_pio(drive, 1);

> > > > > > This function will always set PIO mode 4. Mess.

> > > > > Yep.

> > > >  I'm going to send the patch for both this and siimage.c...

> > > OK

> >   Not sure if I'll be able to find a card to test it soon though (I prefer
> > to test my stuff before submitting, even the simple changes :-).

> Please send it anyway.

   Ugh, this one is more tough than pdc202xx_old.c -- since tuneproc() is 
also borken (doesn't set drive's own transfer mode).
   And... I looked into speedproc() handler, then into PCI0646U datasheet for 
reference and was really terrified: the code for SW/DW DMA setup us utter 
nonsense!  It writes to some reserved bits of BMIDE status reg. instead of 
doinf the real setup, and twiddles the drive 0/1 DMA capable bit which nobody 
asks it to do... Really messy mess. :-(

> Thanks,
> Bart

WBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/