Message-ID: <45B2624E.4030900@ru.mvista.com>
Date:	Sat, 20 Jan 2007 21:41:18 +0300
From:	Sergei Shtylyov <sshtylyov@...mvista.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/15] ide: disable DMA in ->ide_dma_check for "no IORDY"
 case

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists