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-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
