LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
To: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 8/15] ide: disable DMA in ->ide_dma_check for "no IORDY" case
Date: Sat, 20 Jan 2007 21:41:18 +0300	[thread overview]
Message-ID: <45B2624E.4030900@ru.mvista.com> (raw)
In-Reply-To: <45B25DB7.8050903@gmail.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

  reply	other threads:[~2007-01-20 18:41 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-19  0:30 [PATCH 0/15] IDE quilt tree updated Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 1/15] ACPI support for IDE devices Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 2/15] via82cxxx/pata_via: correct PCI_DEVICE_ID_VIA_SATA_EIDE ID and add support for CX700 and 8237S Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 3/15] it8213: fix build and ->ultra_mask Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 4/15] ide: convert ide_hwif_t.mmio into flag Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 5/15] hpt34x: hpt34x_tune_chipset() (->speedproc) fix Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 6/15] atiixp/jmicron/triflex: fix PIO fallback Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 7/15] piix: cleanup Bartlomiej Zolnierkiewicz
2007-01-19  0:31 ` [PATCH 8/15] ide: disable DMA in ->ide_dma_check for "no IORDY" case Bartlomiej Zolnierkiewicz
2007-01-19 16:26   ` Sergei Shtylyov
     [not found]     ` <58cb370e0701191047h524434eobdb9d86ed614bc71@mail.gmail.com>
2007-01-19 19:11       ` Bartlomiej Zolnierkiewicz
2007-01-19 19:35         ` Sergei Shtylyov
     [not found]           ` <58cb370e0701191200i10119313i4aacae9c504a02e4@mail.gmail.com>
2007-01-19 21:43             ` Bartlomiej Zolnierkiewicz
2007-01-20 17:09               ` Sergei Shtylyov
     [not found]                 ` <58cb370e0701200947p578f4d74g1fee511ded6c9a77@mail.gmail.com>
2007-01-20 18:21                   ` Bartlomiej Zolnierkiewicz
2007-01-20 18:41                     ` Sergei Shtylyov [this message]
2007-01-25 17:03                     ` Sergei Shtylyov
2007-01-19  0:32 ` [PATCH 9/15] sgiioc4: fix sgiioc4_ide_dma_check() to enable/disable DMA properly Bartlomiej Zolnierkiewicz
2007-01-19  0:32 ` [PATCH 10/15] ide: add ide_set_dma() helper Bartlomiej Zolnierkiewicz
2007-01-20 20:22   ` Sergei Shtylyov
2007-01-19  0:32 ` [PATCH 11/15] ide: make ide_hwif_t.ide_dma_{host_off,off_quietly} void Bartlomiej Zolnierkiewicz
2007-01-20 20:56   ` Sergei Shtylyov
     [not found]     ` <58cb370e0702021206h205ff983r88fb4bdbea42ed9f@mail.gmail.com>
2007-02-02 22:39       ` Bartlomiej Zolnierkiewicz
2007-01-19  0:32 ` [PATCH 12/15] ide: make ide_hwif_t.ide_dma_host_on void Bartlomiej Zolnierkiewicz
2007-01-20 20:46   ` Sergei Shtylyov
     [not found]     ` <58cb370e0702021128g15cac87ar507c20e78ded9464@mail.gmail.com>
2007-02-02 21:16       ` Bartlomiej Zolnierkiewicz
2007-03-26 17:19   ` Sergei Shtylyov
2007-04-20 20:36     ` Sergei Shtylyov
2007-01-19  0:32 ` [PATCH 13/15] ide: fix UDMA/MWDMA/SWDMA masks Bartlomiej Zolnierkiewicz
2007-01-22 16:19   ` Sergei Shtylyov
     [not found]     ` <58cb370e0702021606v4efa6143lf060e6aab9782c35@mail.gmail.com>
2007-02-03  0:39       ` Bartlomiej Zolnierkiewicz
2007-02-03 16:30         ` Sergei Shtylyov
2007-01-22 18:17   ` Sergei Shtylyov
2007-01-22 18:46     ` Alan
2007-01-22 20:28       ` Sergei Shtylyov
2007-01-22 21:31         ` Alan
2007-01-22 22:39           ` Jeff Garzik
2007-01-23 14:51           ` Sergei Shtylyov
2007-01-25 15:40             ` Sergei Shtylyov
2007-01-31 20:38       ` Sergei Shtylyov
2007-01-31 23:24         ` Alan
     [not found]     ` <58cb370e0702021606m4eb6f682xfa4bf769d398cf9@mail.gmail.com>
2007-02-03  0:50       ` Bartlomiej Zolnierkiewicz
2007-01-19  0:32 ` [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode Bartlomiej Zolnierkiewicz
2007-01-22 19:48   ` Sergei Shtylyov
     [not found]     ` <58cb370e0702021207o435f39cdsf3abb0d55829fc45@mail.gmail.com>
2007-02-02 23:57       ` Bartlomiej Zolnierkiewicz
2007-01-19  0:32 ` [PATCH 15/15] ide: add ide_tune_dma() helper Bartlomiej Zolnierkiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45B2624E.4030900@ru.mvista.com \
    --to=sshtylyov@ru.mvista.com \
    --cc=bzolnier@gmail.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 8/15] ide: disable DMA in ->ide_dma_check for "no IORDY" case' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).