Hello.

Bartlomiej Zolnierkiewicz wrote:

quoted

quoted

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).

quoted

  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. :-)

quoted

  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.

quoted

quoted

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

quoted

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

quoted

quoted

#
# 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

quoted

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

quoted

quoted

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)

quoted

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

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

quoted

quoted

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

quoted

  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. :-)

quoted

quoted

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

quoted

  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"...

quoted

quoted

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

quoted

  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. :-)

quoted

quoted

quoted

quoted

quoted

quoted

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);

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

This function will always set PIO mode 4. Mess.

quoted

quoted

quoted

quoted

Yep.

quoted

quoted

quoted

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

quoted

quoted

OK

quoted

  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