From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marin Mitov Subject: Re: [PATCH] pci_try_set_mwi() in sata_promise Date: Wed, 23 Apr 2008 16:33:24 +0300 Message-ID: <200804231633.24603.mitov@issp.bas.bg> References: <200804222126.56321.mitov@issp.bas.bg> <18446.60365.156326.246875@harpo.it.uu.se> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.issp.bas.bg ([195.96.236.10]:42426 "EHLO mail.issp.bas.bg" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752810AbYDWN3N (ORCPT ); Wed, 23 Apr 2008 09:29:13 -0400 In-Reply-To: <18446.60365.156326.246875@harpo.it.uu.se> Content-Disposition: inline Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Mikael Pettersson Cc: linux-kernel@vger.kernel.org, Jeff Garzik , linux-ide@vger.kernel.org On 23.4.2008, Mikael Pettersson wrote: > Marin Mitov writes: > > Hi all, > > > > The BIOS (Asus A8V Deluxe) is setting incorrectly PCI > > Cache Line Size Register (as seen in lspci -vvv output), > > so try to correct it by pci_try_set_mwi(pdev). > > > > Marin Mitov > > ---------------------------------------------------------- > > Enable Memory-Write-and-Invalidate in sata_promise driver and > > get rid of strange BIOS-set value for cache line size register. > > > > According to Documentation/pci.txt: "This enables... > > ...and also ensures that the cache line size register > > is set correctly". > > > > Signed-off-by: Marin Mitov > > ---------------------------------------------------------- > > --- a/drivers/ata/sata_promise.c 2008-04-22 13:09:15.000000000 +0300 > > +++ b/drivers/ata/sata_promise.c 2008-04-22 13:11:01.000000000 +0300 > > @@ -1114,6 +1114,7 @@ static int pdc_ata_init_one(struct pci_d > > > > /* start host, request IRQ and attach */ > > pci_set_master(pdev); > > + pci_try_set_mwi(pdev); > > return ata_host_activate(host, pdev->irq, pdc_interrupt, IRQF_SHARED, > > &pdc_ata_sht); > > } > > > > Not enough information here. Sorry. > > 0. Please post lspci -vvvx including all PCI devices. Here it is: 00:00.0 Host bridge: VIA Technologies, Inc. K8T800Pro Host Bridge Subsystem: ASUSTeK Computer Inc. A8V Deluxe Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- Status: Cap+ 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- Reset- FastB2B- Capabilities: [80] Power Management version 2 Flags: PMEClk- DSI- D1+ D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 PME-Enable- DSel=0 DScale=0 PME- 00: 06 11 88 b1 07 01 30 02 00 00 04 06 00 00 01 00 10: 00 00 00 00 00 00 00 00 00 01 01 00 f0 00 20 22 20: 00 fb f0 fb 00 f8 f0 f9 00 00 00 00 00 00 00 00 30: 00 00 00 00 80 00 00 00 00 00 00 00 00 00 0a 00 00:08.0 RAID bus controller: Promise Technology, Inc. PDC20378 (FastTrak 378/SATA 378) (rev 02) Subsystem: ASUSTeK Computer Inc. K8V Deluxe/PC-DL Deluxe motherboard Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr- Stepping- SERR+ FastB2B- Status: Cap+ 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- TAbort- SERR- 1. What consequence does the incorrect cache line size setting have? None if MWI is not enabled AFAIK. > 2. What improvement does that pci_try_set_mwi() cause? Speed? Fewer errors? May be speed, but practically hardly observable. It is described in PCI specs. My point supplying the patch was: If the hardware has this property why not using it? In any case that does not hurt (if it is correctly implemented in the hardware).. I am testing it since few days - it works for me. > 3. Why call pci_try_set_mwi()? Can't you set the cache line size directly? pci_set_cacheline_size() is NOT exported, while pci_try_set_mwi() IS and sets (as a side effect) the cache line size. > 4. You write "try to correct it". So the "fix" might not work? Then what? It is not really a fix for a bug. If a bug it is in the BIOS, not in the driver. pci_set_mwi() fails only if cache line size cannot be set. Then MWI is not enabled. pci_try_set_mwi() just try to set it. If not the device continues to work (nicely) as it did up to now. > 5. Is the problem specific for the Promise chip? That is: > a) are any other built-in PCI devices affected by this BIOS bug? No any other built-in PCI devices are affected as seen from the lspci outpot. > b) if you add a PCI card (of any kind), does it also > get the incorrect cache line size setting? There is one: 00:0d.0 Multimedia video controller: Intel Corporation SAA7116, but it is not MWI capable. So I do not know. > 6. Are you running the latest BIOS? Surely not. > > To me it sounds like this is a generic PCI bug on that > machine, and so should be handled by the kernel's PCI layer. It is not a bug, because this "strange" value for the cache line size does not matter if MWI is not enabled, but when it is enabled by pci_set_mwi() or pci_try_set_mwi() - it is already corrected. > But if the problem really is specific to the built-in Promise > chip on that machine, then I can see why a workaround in > sata_promise could be appropriate. I do not insist the patch to be included. It is just "nice to have" it. The hardware has the property, theoretically it should be speedy (otherwise it would not be implemented in the hardware, nor included in the PCI specs), why not using it :-) But you are the maintainer - you decide. Regards. Marin Mitov > > /Mikael >