LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
To: Sergei Shtylyov <sshtylyov@ru.mvista.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: Fri, 19 Jan 2007 22:43:41 +0100	[thread overview]
Message-ID: <45B13B8D.2020402@gmail.com> (raw)
In-Reply-To: <58cb370e0701191200i10119313i4aacae9c504a02e4@mail.gmail.com>


Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
>>>> [PATCH] ide: disable DMA in ->ide_dma_check for "no IORDY" case
> 
>>>   I've looked thru the code, and found more issues with the PIO fallback
>>> there. Will try to cook up patches for at least some drivers...
> 
>> Great, if possible please base them on top of the IDE tree...
> 
>    Erm, I had doubts about it (having in mind that all that code is more of a
> cleanups than fixes). Maybe it'd be a good idea to separate the fix and
> cleanup series somehow...

I generally tend do cleanups as a groundwork for the real fixes and separate
cleanups and fixes to have good base for dealing with regressions.  Often all
changes (cleanups/fixes) could be included in one patch but then I would have
had harsh times when debugging the regressions.  It matters a lot if you hit
an unknown (or known but the documentation is covered by NDA) hardware bug
- you can concentrate on a small patch changing the way in which hardware is
accessed instead of that big patch moving code around etc.

Also the thing is that the same bugs are propagated over many drivers so doing
cleanups which merge code before fixing the bug makes sense.  We can then fix
the damn bug once and for all and not worry about somebody copy-n-pasting
the bug from the yet-to-be-fixed driver (i.e. in the next patch IDE update
there will be patch to check return value of ->speedproc in ide_tune_dma(),
without ide-fix-dma-mask/ide-max-dma-mode/ide-tune-dma-helper patches
I would have to go over all drivers to fix this bug and still there won't
be a guarantee that same bug wouldn't be introduced in some new driver).

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

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

#
# -pata extraversion
#
pata-extraversion.patch
	-- n/a
#
# 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
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)
pdc202xx_new-remove-useless-code.patch
pdc202xx_-remove-check_in_drive_lists-abomination.patch
	-- cleanups
#
# IDE patches applied by Andrew (2.6.20-rc4-mm1)
#
atiixpc-remove-unused-code.patch
	-- cleanup
atiixpc-sb600-ide-only-has-one-channel.patch
atiixpc-add-cable-detection-support-for-ati-ide.patch
ide-generic-jmicron-has-its-own-drivers-now.patch
	-- fixes
ide-maintainers-entry.patch
	-- n/a
#
# IT8213
#
it8213-ide-driver.patch
it8213-ide-driver-update.patch
	-- new driver
#
# patches posted on Jan 11 2007
#
ia64-pci_get_legacy_ide_irq.patch
ide-pci-init-tags.patch
	-- fixes
pdc202xx_old-dead-code.patch
au1xxx-dead-code.patch
ide-pio-blacklisted.patch
ide-no-dsc-flag.patch
trm290-dma-ifdefs.patch
ide-pci-device-tables.patch
ide-dev-openers.patch
hpt366-init-dma.patch
cs5530-cleanup.patch
svwks-cleanup.patch
sis5513-config-xfer-rate.patch
ide-set-xfer-rate.patch
ide-use-fast-pio-v2.patch
ide-io-cleanup.patch
	-- cleanups
#
# Delkin CardBus CF driver (Mark Lord <mlord@pobox.com>)
#
delkin_cb-ide-driver.patch
	-- new driver
#
# IDE ACPI support (Hannes Reinecke <hare@suse.de>)
#
ide-acpi-support.patch
	-- new functionality (fixes PM on some machines)
#
# ide-pnp exit fix (Tejun Heo <htejun@gmail.com>)
#
ide-pnp-exit-fix.patch
	-- fix
#
# VIA IDE update (Josepch Chan <josephchan@via.com.tw>)
#
via-ide-update.patch
	-- fix
#
# patches posted on 18 Jan 2007
#
it8213-ide-driver-update-fixes.patch
	-- fix
ide-mmio-flag.patch
	-- cleanup
hpt34x-tune-chipset-fix.patch
	-- fix
ide-fix-pio-fallback.patch
	-- fix
piix-cleanup.patch
	-- cleanup
ide-dma-check-disable-dma-fix.patch
sgiioc4-ide-dma-check-fix.patch
	-- fixes
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

So it looks more like 50-50 with majority of fixes coming from you :)

However I understand that for some applications (stable distro etc) fixes
only tree would be more desired and if somebody would like to maintain
such tree I'm all for it. :)

OTOH getting patches against vanilla or -mm is perfectly fine with me
and if you would like me to shuffle ordering of the patches (but without
need of rewritting them) it also OK

>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
>>>> Index: b/drivers/ide/pci/aec62xx.c
>>>> ===================================================================
>>>> --- a/drivers/ide/pci/aec62xx.c
>>>> +++ b/drivers/ide/pci/aec62xx.c
>>>> @@ -214,12 +214,10 @@ static int aec62xx_config_drive_xfer_rat
>>>>      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))
>>>>              aec62xx_tune_drive(drive, 5);
>>>
>>>   This function looks like it's working (thouugh having the wrong
>>> limit of
>>> PIO5 on auto-tuning) but is unnecassary complex.
> 
>> Yes, it seems that there are actually two bugs here:
>> * the maximum allowed PIO mode should be PIO4 not PIO5
>> * for auto-tuning ("pio" == 255) it incorrectly sets PIO0
>>   (255 fails to the default case in the switch statement)
> 
>    Yeah, you if you pass 255, it won't work (so, drive->autotune must be
> broken). But the driver itself have the wrong idea of 5 meaning auto-tune, so
> fallback should still work.

Yep.

>>>   Heh, the driver is certainly a rip-off form hpt366.c. What a doubtful
>>> example they have chosen... :-)
> 
>> hehe
> 
>    The driver's authorship explains it all. :-)
> 
>>>> Index: b/drivers/ide/pci/atiixp.c
>>>> ===================================================================
>>>> --- a/drivers/ide/pci/atiixp.c
>>>> +++ b/drivers/ide/pci/atiixp.c
>>>> @@ -264,10 +264,9 @@ static int atiixp_dma_check(ide_drive_t
>>>>              tspeed = ide_get_best_pio_mode(drive, 255, 5, NULL);
>>>>              speed = atiixp_dma_2_pio(XFER_PIO_0 + tspeed) +
>>>> XFER_PIO_0;
> 
>>>   It's simply stupid to convert PIO mode to PIO mode. The whole idea is
>>> doubtful as well..
> 
>> It is side-effect of basing atiixp on piix driver.  Fixing it will allow PIO1
>> to be used (good) because atiixp_dma_2_pio() always downgrades PIO1 to PIO0
>> (leftover from piix - on Intel chipsets same timings are used for PIO0/1).
> 
>>>>              hwif->speedproc(drive, speed);
> 
>>>   Well, well, the tuneproc() method can't ahndle auto-tunuing here
>>> (255)...
> 
>> Yes, definitely a bug.
> 
>    Ugh... don't expect patches form me soon though. My first priority is the
> drivers that we support here...

OK, added at the end of my TODO (just in case)

>>> And it also doesn't set up drive's own speed. The code seem to be another
>>> rip-off from piix.c, repeating all its mistakes... :-)
> 
>> :)
> 
>>>> 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

>>>> Index: b/drivers/ide/pci/cs5535.c
>>>> ===================================================================
>>>> --- a/drivers/ide/pci/cs5535.c
>>>> +++ b/drivers/ide/pci/cs5535.c
>>>> @@ -206,10 +206,9 @@ static int cs5535_dma_check(ide_drive_t
>>>>      if (ide_use_fast_pio(drive)) {
>>>>              speed = ide_get_best_pio_mode(drive, 255, 4, NULL);
>>>>              cs5535_set_drive(drive, speed);
> 
>>>   Could be folded into tuneproc() method call.
> 
>> Using ->tuneproc() will also set the PIO mode on the drive
>> which is not done currently...
> 
>    Hm, ide_config_drive_speed() is called by both tuneproc() method and
> cs5535_set_drive(), so I saw no issue there...

indeed

>>>> Index: b/drivers/ide/pci/sis5513.c
>>>> ===================================================================
>>>> --- a/drivers/ide/pci/sis5513.c
>>>> +++ b/drivers/ide/pci/sis5513.c
>>>> @@ -678,12 +678,10 @@ static int sis5513_config_xfer_rate(ide_
>>>>      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))
>>>>              sis5513_tune_drive(drive, 5);
> 
>>>    Ugh, PIO fallback effectively always tries to set mode 4 here (thanks
>>> it's not 5). Mess.
> 
>> Yep, but it seems to be even more complicated since config_art_rwp_pio()
>> is a mess^2 - chipset is programmed to the best PIO mode while the
>> device is set to PIO4... *sigh*...
> 
>    Sorry, this one is low prio for me... :-)

OK

Bart


  parent reply	other threads:[~2007-01-19 21:39 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 [this message]
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
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=45B13B8D.2020402@gmail.com \
    --to=bzolnier@gmail.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sshtylyov@ru.mvista.com \
    --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).