Re: [PATCH 1/4] dma: document dma_{un}map_{single|sg}_attrs() interface
On Tue, Jan 29, 2008 at 09:50:40PM -0800, [EMAIL PROTECTED] wrote:
>
> Document the new dma_{un}map_{single|sg}_attrs() functions.
Thank you!
Looks good to me...so far I've only found one nit.
...
> +struct dma_attrs encapsulates a set of "dma attributes". For the
> +definition of struct dma_attrs see linux/dma-attrs.h.
Because all architectures will share the set of attrs definitions,
would it be reasonable to document the intent of each attr?
Two reasons for doing this:
1) people reading the driver code should understand WHY the dma attr was added.
2) other arch maintainers need to know in order to implement the same attr
for their shiny new toys.
> +The interpretation of dma attributes is architecture-specific.
This statement is really importantbut:
Could we add a reference to arch documentation for each attr?
ie something public (doesn't have to be in linux source tree)
which explains the subtlies of how that DMA attr actually works.
Having worked on HP chipsets for 10+ years, I know releasing
original HW docs is often not possible. I'm not asking for the
impossible. Please don't flame me for that. But if the company
is willing to publish the existance of a feature, a paragraph or
two would be good marketing too. Maybe just include comments in
the arch/ code that implements the feature and reference those
comments in DMA-API.txt.
> +If struct dma_attrs* is NULL, the semantics of each of these
> +functions is identical to those of the corresponding function
> +without the _attrs suffix. As a result dma_map_single_attrs()
> +can generally replace dma_map_single(), etc.
> +
> +As an example of the use of the *_attrs functions, here's how
> +you could pass an attribute DMA_ATTR_FOO when mapping memory
> +for DMA:
> +
> +#include
> +/* DMA_ATTR_FOO should be defined in linux/dma-attrs.h */
> +...
> +
> + DECLARE_DMA_ATTRS(attrs);
> + dma_set_attr(&attrs, DMA_ATTR_FOO);
> +
> + n = dma_map_sg_attrs(dev, sg, nents, DMA_TO_DEVICE, &attr);
> +
> +
> +Architectures that care about DMA_ATTR_FOO would check for its
> +presence in their implementations of the mapping and unmapping
> +routines, e.g.:
> +
> +void whizco_dma_map_sg_attrs(struct device *dev, dma_addr_t dma_addr,
> + size_t size, enum dma_data_direction dir,
> + struct dma_attrs* attrs)
> +{
> +
> + int foo = dma_get_attr(attrs, DMA_ATTR_FOO);
> +
> + if (foo)
> + /* twizzle the frobnozzle */
> +
> +
Excellent example.
thanks again for remembering DMA-API.txt.
cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/20] drivers/net/ethernet/dec/tulip/dmfe.c: fix error return code
On Wed, Oct 3, 2012 at 9:17 AM, Peter Senna Tschudin
wrote:
> From: Peter Senna Tschudin
>
> Convert a nonnegative error return code to a negative one, as returned
> elsewhere in the function.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> //
> (
> if@p1 (\(ret < 0\|ret != 0\))
> { ... return ret; }
> |
> ret@p1 = 0
> )
> ... when != ret = e1
> when != &ret
> *if(...)
> {
> ... when != ret = e2
> when forall
> return ret;
> }
> //
>
> Signed-off-by: Peter Senna Tschudin
Thanks! Looks good to me.
Acked-by: Grant Grundler
cheers,
grant
>
> ---
> drivers/net/ethernet/dec/tulip/dmfe.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c
> b/drivers/net/ethernet/dec/tulip/dmfe.c
> index 4d6fe60..d23755e 100644
> --- a/drivers/net/ethernet/dec/tulip/dmfe.c
> +++ b/drivers/net/ethernet/dec/tulip/dmfe.c
> @@ -446,13 +446,17 @@ static int __devinit dmfe_init_one (struct pci_dev
> *pdev,
> /* Allocate Tx/Rx descriptor memory */
> db->desc_pool_ptr = pci_alloc_consistent(pdev, sizeof(struct tx_desc)
> *
> DESC_ALL_CNT + 0x20, &db->desc_pool_dma_ptr);
> - if (!db->desc_pool_ptr)
> + if (!db->desc_pool_ptr) {
> + err = -ENOMEM;
> goto err_out_res;
> + }
>
> db->buf_pool_ptr = pci_alloc_consistent(pdev, TX_BUF_ALLOC *
> TX_DESC_CNT + 4, &db->buf_pool_dma_ptr);
> - if (!db->buf_pool_ptr)
> + if (!db->buf_pool_ptr) {
> + err = -ENOMEM;
> goto err_out_free_desc;
> + }
>
> db->first_tx_desc = (struct tx_desc *) db->desc_pool_ptr;
> db->first_tx_desc_dma = db->desc_pool_dma_ptr;
> @@ -462,8 +466,10 @@ static int __devinit dmfe_init_one (struct pci_dev *pdev,
> db->chip_id = ent->driver_data;
> /* IO type range. */
> db->ioaddr = pci_iomap(pdev, 0, 0);
> - if (!db->ioaddr)
> + if (!db->ioaddr) {
> + err = -ENOMEM;
> goto err_out_free_buf;
> + }
>
> db->chip_revision = pdev->revision;
> db->wol_mode = 0;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 079/193] drivers/net/ethernet/dec/tulip: remove CONFIG_EXPERIMENTAL
On Tue, Oct 23, 2012 at 1:02 PM, Kees Cook wrote: > This config item has not carried much meaning for a while now and is > almost always enabled by default. As agreed during the Linux kernel > summit, remove it. > > CC: Grant Grundler Acked-by: Grant Grundler It clearly makes no sense for this driver (obsolete HW for the most part). Thanks Kees! grant > Signed-off-by: Kees Cook > --- > drivers/net/ethernet/dec/tulip/Kconfig |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/dec/tulip/Kconfig > b/drivers/net/ethernet/dec/tulip/Kconfig > index 1203be0..0c37fb2 100644 > --- a/drivers/net/ethernet/dec/tulip/Kconfig > +++ b/drivers/net/ethernet/dec/tulip/Kconfig > @@ -57,8 +57,8 @@ config TULIP > be called tulip. > > config TULIP_MWI > - bool "New bus configuration (EXPERIMENTAL)" > - depends on TULIP && EXPERIMENTAL > + bool "New bus configuration" > + depends on TULIP > ---help--- > This configures your Tulip card specifically for the card and > system cache line size type you are using. > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: raw_pci_read in quirk_intel_irqbalance
On Sun, Feb 10, 2008 at 10:04:16PM -0700, Matthew Wilcox wrote:
> > "A disabled or non-existent device's configuration register space is
> > hidden. A disabled or non-existent device will return all ones for reads
> > and will drop writes just as if the cycle terminated with a Master Abort
> > on PCI."
>
> I'd like to thank Grant for pointing out to me that this is exactly what
> the write immediately above this is doing -- enabling device 8 to
> respond to config space cycles.
welcome.
...
> >From f565b65591a3f90a272b1d511e4ab1728861fe77 Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <[EMAIL PROTECTED]>
> Date: Sun, 10 Feb 2008 23:18:15 -0500
> Subject: [PATCH] Use proper abstractions in quirk_intel_irqbalance
>
> Since we may not have a pci_dev for the device we need to access, we can't
> use pci_read_config_word. But raw_pci_read is an internal implementation
> detail; it's better to use the architected pci_bus_read_config_word
> interface. Using PCI_DEVFN instead of a mysterious constant helps
> reassure everyone that we really do intend to access device 8.
>
> Signed-off-by: Matthew Wilcox <[EMAIL PROTECTED]>
> ---
> arch/x86/kernel/quirks.c |9 ++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> index 1941482..c47208f 100644
> --- a/arch/x86/kernel/quirks.c
> +++ b/arch/x86/kernel/quirks.c
> @@ -11,7 +11,7 @@
> static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)
> {
> u8 config, rev;
> - u32 word;
> + u16 word;
>
> /* BIOS may enable hardware IRQ balancing for
>* E7520/E7320/E7525(revision ID 0x9 and below)
> @@ -26,8 +26,11 @@ static void __devinit quirk_intel_irqbalance(struct
> pci_dev *dev)
> pci_read_config_byte(dev, 0xf4, &config);
> pci_write_config_byte(dev, 0xf4, config|0x2);
Can you also add a comment which points at the Intel documentation?
http://download.intel.com/design/chipsets/datashts/30300702.pdf
Page 34 documents 0xf4 register.
And I just doubled checked that the 0xf4 register value is restored later
in the quirk (obvious when you look at the code but not from the patch).
> - /* read xTPR register */
> - raw_pci_read(0, 0, 0x40, 0x4c, 2, &word);
> + /*
> + * read xTPR register. We may not have a pci_dev for device 8
> + * because it might be hidden until the above write.
> + */
> + pci_bus_read_config_word(dev->bus, PCI_DEVFN(8, 0), 0x4c, &word);
Yeah, this should work even though we don't have a dev for it.
Acked-by: Grant Grundler <[EMAIL PROTECTED]>
thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: raw_pci_read in quirk_intel_irqbalance
On Mon, Feb 11, 2008 at 09:18:49AM -0800, Linus Torvalds wrote:
> I put it in the commit message, but it wasn't on page 34 when I checked (I
> changed it to 69),
Sorry - page 34 was just the first reference to "Extended Configuration
Registers" when I originally scrounged up the info for willy.
Page 69 is in fact what I wanted to point at ("DEVPRES1" reg).
> and I added the naem for the datasheet so that if/when
> it moves, maybe google can help.
It should. But doing a quick check now only shows one other copy
(in .es domain :) when searching for "30300702.pdf".
Searching for the full document title results in several intel.com
locations and lots of other misc references that don't look quite right.
Many of those just reference the "product brief" and not the data sheet.
yahoo.com gives similar results.
thanks,
grant
>
> Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] mmc: core: cleanup and locking patches description
Ping? Has anyone had a chance to review/test this series and/or be willing to do so this week? grundler@firesword:~$ pwclient list -w 'grund...@chromium.org' -p LKML Patches submitted by Grant Grundler : ID StateName -- - ... 2950961 New [1/7] mmc: core: rename "data" to saved_areq 2951081 New [2/7] mmc: core: rename local var err to saved_err 2950981 New [3/7] mmc: core: restructure error handling for start req 2951001 New [4/7] mmc: core: use common code path to return error 2951061 New [5/7] mmc: core: handling polling more gracefully 2951041 New [6/7] mmc: core: protect references to host->areq with host->lock 2951021 New [7/7] mmc: core: mmc_start_req is a misnomer -> mmc_process_areq To remind, this is the fio command line that results in mmcqd hangs on the exynos 5250 system (uses dw_mmc storage controller): $FIO --name=short_randwrite --size=2G --time_based --runtime=90 \ --readwrite=randwrite --numjobs=2 --bs=4k --norandommap --ioengine=psync \ --direct=0 --invalidate=1 --filename=/dev/mmcblk0p5 thanks, grant On Thu, Sep 26, 2013 at 12:57 PM, Grant Grundler wrote: > Argh...too much wordsmithing... > > On Thu, Sep 26, 2013 at 12:22 PM, Grant Grundler > wrote: >> Following 7 patches are mostly cleanup with one key patch around host->areq >> locking. The host->areq locking problem description is here: >> http://www.spinics.net/lists/linux-mmc/msg21644.html >> >> I do believe this preposed host->areq locking patch is a complete fix. > > ... is NOT a complete fix. > >> But it appears to fix the problem and is better than nothing. > > Still true. > > cheers, > grant > >> >> This patch sequence applies clean to linus' 3.12-rc2 branch and only compile >> tested in this form. This is a forward port (and cleanup) of the >> same patch I've been testing on ChromeOS-3.4 tree using Exynos5250 chipset. >> >> cheers, >> grant >> >> -mmc-core-cleanups-and-locking-description (this email) >> 0001-mmc-core-rename-data-to-saved_areq.patch >> 0002-mmc-core-rename-local-var-err-to-saved_err.patch >> 0003-mmc-core-restructure-error-handling-for-start-req.patch >> 0004-mmc-core-use-common-code-path-to-return-error.patch >> 0005-mmc-core-handling-polling-more-gracefully.patch >> 0006-mmc-core-protect-references-to-host-areq-with-host-l.patch >> 0007-mmc-core-mmc_start_req-is-a-misnomer-mmc_process_are.patch >> >> >> drivers/mmc/card/block.c| 8 ++-- >> drivers/mmc/card/mmc_test.c | 4 +- >> drivers/mmc/core/core.c | 103 >> +--- >> include/linux/mmc/core.h| 2 +- >> 4 files changed, 66 insertions(+), 51 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] mmc: core: cleanup and locking patches description
On Thu, Oct 3, 2013 at 2:35 PM, Ulf Hansson wrote: > > Den 3 okt 2013 22:35 skrev "Grant Grundler" : > > >> >> Ping? >> >> Has anyone had a chance to review/test this series and/or be willing >> to do so this week? > > Hi Grant, > > It is on my todo list. My plan is to check the behavior on ux500 board, > which uses mmci and which has been one of the first drivers that utilizes > the async request mechanism. Thanks Ulf for the update. dw_mmc is the only driver that I can see that uses a tasklet (Soft Interrupt context) to handle completions. So it's "timing" will be unique and nothing it calls can acquire semaphores in that context. :/ > I am not that keen of the patches that renames stuff though, not sure it > actually improves anything. I do agree the name changes are secondary to the locking changes. Names matter. "data" doesn't say anything about how the variable is expected to be used. And "err" doesn't suggest this variable is ONLY related to the completion, not the starting of a new async request. mmc_start_req() is very confusing since it's very different from mmc_start_request. At a minimum, mmc_start_req() should be renamed to "_areq" to be clear it's not dealing with synchronous requests at all. So yes, renaming stuff can improve what developers expect from the code. > This week is not possible, but likely the next. Ok! Thanks for letting me know you plan on looking at it. I can wait. If necessary, I will poke you again next week. :) thank you! grant > > Kind regards > Ulf Hanson > >> >> grundler@firesword:~$ pwclient list -w 'grund...@chromium.org' -p LKML >> Patches submitted by Grant Grundler : >> ID StateName >> -- - >> ... >> 2950961 New [1/7] mmc: core: rename "data" to saved_areq >> 2951081 New [2/7] mmc: core: rename local var err to saved_err >> 2950981 New [3/7] mmc: core: restructure error handling for start >> req >> 2951001 New [4/7] mmc: core: use common code path to return error >> 2951061 New [5/7] mmc: core: handling polling more gracefully >> 2951041 New [6/7] mmc: core: protect references to host->areq >> with host->lock >> 2951021 New [7/7] mmc: core: mmc_start_req is a misnomer -> >> mmc_process_areq >> >> >> To remind, this is the fio command line that results in mmcqd hangs on >> the exynos 5250 system (uses dw_mmc storage controller): >> >> $FIO --name=short_randwrite --size=2G --time_based --runtime=90 \ >> --readwrite=randwrite --numjobs=2 --bs=4k --norandommap >> --ioengine=psync \ >> --direct=0 --invalidate=1 --filename=/dev/mmcblk0p5 >> >> thanks, >> grant >> >> On Thu, Sep 26, 2013 at 12:57 PM, Grant Grundler >> wrote: >> > Argh...too much wordsmithing... >> > >> > On Thu, Sep 26, 2013 at 12:22 PM, Grant Grundler >> > wrote: >> >> Following 7 patches are mostly cleanup with one key patch around >> >> host->areq >> >> locking. The host->areq locking problem description is here: >> >> http://www.spinics.net/lists/linux-mmc/msg21644.html >> >> >> >> I do believe this preposed host->areq locking patch is a complete fix. >> > >> > ... is NOT a complete fix. >> > >> >> But it appears to fix the problem and is better than nothing. >> > >> > Still true. >> > >> > cheers, >> > grant >> > >> >> >> >> This patch sequence applies clean to linus' 3.12-rc2 branch and only >> >> compile >> >> tested in this form. This is a forward port (and cleanup) of the >> >> same patch I've been testing on ChromeOS-3.4 tree using Exynos5250 >> >> chipset. >> >> >> >> cheers, >> >> grant >> >> >> >> -mmc-core-cleanups-and-locking-description (this email) >> >> 0001-mmc-core-rename-data-to-saved_areq.patch >> >> 0002-mmc-core-rename-local-var-err-to-saved_err.patch >> >> 0003-mmc-core-restructure-error-handling-for-start-req.patch >> >> 0004-mmc-core-use-common-code-path-to-return-error.patch >> >> 0005-mmc-core-handling-polling-more-gracefully.patch >> >> 0006-mmc-core-protect-references-to-host-areq-with-host-l.patch >> >> 0007-mmc-core-mmc_start_req-is-a-misnomer-mmc_process_are.patch >> >> >> >> >> >> drivers/mmc/card/block.c| 8 ++-- >> >> drivers/mmc/card/mmc_test.c | 4 +- >> >> drivers/mmc/core/core.c | 103 >> >> +--- >> >> include/linux/mmc/core.h| 2 +- >> >> 4 files changed, 66 insertions(+), 51 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 01/12] iommu/exynos: add missing cache flush for removed pagetable entries
On Thu, Jul 4, 2013 at 4:20 AM, Cho KyongHo wrote: ... > I am still working on the patch set when I am free. > Implementation of the updated patch set has been finished but not tested yet. > > I will post the patches after simple test :) Ok - thanks Cho! If you'd like me to test, please post a "RFC" (request for comment) version that hasn't yet been tested. You don't have to do everything yourself. :) cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 01/12] iommu/exynos: add missing cache flush for removed pagetable entries
On Mon, Jul 8, 2013 at 11:21 AM, Grant Grundler wrote: > On Thu, Jul 4, 2013 at 4:20 AM, Cho KyongHo wrote: > ... >> I am still working on the patch set when I am free. >> Implementation of the updated patch set has been finished but not tested yet. >> >> I will post the patches after simple test :) > > Ok - thanks Cho! > > If you'd like me to test, please post a "RFC" (request for comment) > version that hasn't yet been tested. You don't have to do everything > yourself. :) Nevermind...I'm catching on email in reverse and see that you've already posted the patches! I'll try to test those today. :) thanks! grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] mmc: dw_mmc: Add exynos resume callback to clear WAKEUP_INT
On Tue, Jul 9, 2013 at 12:09 PM, Doug Anderson wrote: > Hi, > > On Tue, Jul 9, 2013 at 10:31 AM, Doug Anderson wrote: >> If the WAKEUP_INT is asserted at wakeup and not cleared, we'll end up >> looping around forever. >> >> Signed-off-by: Doug Anderson >> --- >> drivers/mmc/host/dw_mmc-exynos.c | 23 +++ >> 1 file changed, 23 insertions(+) > > Grant just pointed out that the WAKEUP_INT is supposed to only be > enabled if bits 8, 9, or 10 are 1. Our driver never sets those so we > _should_ never get a WAKEUP_INT. Bits 8-10 are marked as RESERVED on > the exynos5420 manual, so the current guess is that they're broken on > that silicon but that sometimes the interrupt fires anyway. > > In any case, it is still a reasonable thing to clear this interrupt at > wakeup if it has fired, even if we're on an exynos device without any > problems. I agree. Can add: Reviewed-by: Grant Grundler thanks, grant > > -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 00/12] iommu/exynos: Fixes and Enhancements of System MMU driver with DT
On Fri, Jul 5, 2013 at 5:29 AM, Cho KyongHo wrote: > The current exynos-iommu(System MMU) driver does not work autonomously > since it is lack of support for power management of peripheral blocks. ... > Patch summary: > [PATCH v7 1/9] iommu/exynos: do not include removed header > [PATCH v7 2/9] iommu/exynos: add missing cache flush for removed page table > entries > [PATCH v7 3/9] iommu/exynos: fix page table maintenance > [PATCH v7 4/9] iommu/exynos: allocate lv2 page table from own slab > [PATCH v7 5/9] iommu/exynos: change rwlock to spinlock > [PATCH v7 6/9] clk: exynos5250: add gate clock descriptions of System MMU > [PATCH v7 7/9] ARM: dts: Add description of System MMU of Exynos SoCs > [PATCH v7 8/9] iommu/exynos: support for device tree > [PATCH v7 9/9] iommu/exynos: add bus notifier for registering System MMU Cho, Of the above patches, nearly all have been applied to chromeos-3.8 (kernel-next git tree) by Doug Anderson and others. AFAICT, the only ones not applied are: [v7,3/9] iommu/exynos: fix page table maintenance [v7,6/9] clk: exynos5250: add gate clock descriptions of System MMU (conflicts in this one) [v7,7/9] ARM: dts: Add description of System MMU of Exynos SoCs (depends on 6/9) We also already have parts of: [v7,9/9] iommu/exynos: add bus notifier for registering System MMU Some of those are being further discussed but I've lost track now exactly which ones. I'm telling you about chromeos-3.8 status since the adopted changes have been reviewed (by me and others) are being tested manually here on several different Samsung Exynos platforms (including 5250 which is our "snow" platform). Not sure how you should to mark those patches since they aren't identical to your changes (which apply to post 3.10 kernels, not 3.8). You might consider splitting those patches out from the 4 I've listed above to get that series accepted upstream since the additional review/testing should provide some confidence those patches are good. cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mmc: core: remove issue_fn indirect function call
struct mmc_queue defines issue_fn as an indirect function call.
issue_fn field only gets set to mmc_blk_issue_rq and only gets
invoked immediately after calling blk_fetch_request().
Don't bother with indirect function call - it's pointless and just
obfuscates the code.
Signed-off-by: Grant Grundler
---
drivers/mmc/card/block.c | 1 -
drivers/mmc/card/queue.c | 2 +-
drivers/mmc/card/queue.h | 1 -
3 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1a3163f..b2cdd10 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2072,7 +2072,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct
mmc_card *card,
if (ret)
goto err_putdisk;
- md->queue.issue_fn = mmc_blk_issue_rq;
md->queue.data = md;
md->disk->major = MMC_BLOCK_MAJOR;
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index fa9632e..6f9adc5 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -67,7 +67,7 @@ static int mmc_queue_thread(void *d)
if (req || mq->mqrq_prev->req) {
set_current_state(TASK_RUNNING);
cmd_flags = req ? req->cmd_flags : 0;
- mq->issue_fn(mq, req);
+ mmc_blk_issue_rq(mq, req);
if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
continue; /* fetch again */
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 5752d50..35380015 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -51,7 +51,6 @@ struct mmc_queue {
#define MMC_QUEUE_SUSPENDED(1 << 0)
#define MMC_QUEUE_NEW_REQUEST (1 << 1)
- int (*issue_fn)(struct mmc_queue *, struct request
*);
void*data;
struct request_queue*queue;
struct mmc_queue_reqmqrq[2];
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mmc: core: remove dead function mmc_try_claim_host
cscsope says there are no callers for mmc_try_claim_host in the kernel.
No reason to keep it.
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 25 -
include/linux/mmc/core.h | 1 -
2 files changed, 26 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index bf18b6b..006ead2 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -918,31 +918,6 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t
*abort)
EXPORT_SYMBOL(__mmc_claim_host);
/**
- * mmc_try_claim_host - try exclusively to claim a host
- * @host: mmc host to claim
- *
- * Returns %1 if the host is claimed, %0 otherwise.
- */
-int mmc_try_claim_host(struct mmc_host *host)
-{
- int claimed_host = 0;
- unsigned long flags;
-
- spin_lock_irqsave(&host->lock, flags);
- if (!host->claimed || host->claimer == current) {
- host->claimed = 1;
- host->claimer = current;
- host->claim_cnt += 1;
- claimed_host = 1;
- }
- spin_unlock_irqrestore(&host->lock, flags);
- if (host->ops->enable && claimed_host && host->claim_cnt == 1)
- host->ops->enable(host);
- return claimed_host;
-}
-EXPORT_SYMBOL(mmc_try_claim_host);
-
-/**
* mmc_release_host - release a host
* @host: mmc host to release
*
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index da51bec..a00fc49 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -188,7 +188,6 @@ extern unsigned int mmc_align_data_size(struct mmc_card *,
unsigned int);
extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
extern void mmc_release_host(struct mmc_host *host);
-extern int mmc_try_claim_host(struct mmc_host *host);
extern void mmc_get_card(struct mmc_card *card);
extern void mmc_put_card(struct mmc_card *card);
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: tulip: remove deprecated IRQF_DISABLED
On Wed, Sep 11, 2013 at 9:20 PM, Michael Opdenacker
wrote:
> This patch proposes to remove the IRQF_DISABLED flag from
> drivers/net/ethernet/dec/tulip/de4x5.c
>
> It's a NOOP since 2.6.35 and it will be removed one day.
yup - that was easy to confirm.
> Signed-off-by: Michael Opdenacker
Acked-by: Grant Grundler
thanks,
grant
> ---
> drivers/net/ethernet/dec/tulip/de4x5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/de4x5.c
> b/drivers/net/ethernet/dec/tulip/de4x5.c
> index 2db6c57..263b92c 100644
> --- a/drivers/net/ethernet/dec/tulip/de4x5.c
> +++ b/drivers/net/ethernet/dec/tulip/de4x5.c
> @@ -1321,7 +1321,7 @@ de4x5_open(struct net_device *dev)
> if (request_irq(dev->irq, de4x5_interrupt, IRQF_SHARED,
> lp->adapter_name, dev)) {
> printk("de4x5_open(): Requested IRQ%d is busy - attemping
> FAST/SHARE...", dev->irq);
> - if (request_irq(dev->irq, de4x5_interrupt, IRQF_DISABLED |
> IRQF_SHARED,
> + if (request_irq(dev->irq, de4x5_interrupt, IRQF_SHARED,
> lp->adapter_name, dev)) {
> printk("\n Cannot get IRQ- reconfigure your
> hardware.\n");
> disable_ast(dev);
> --
> 1.8.1.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] mmc: core: protect references to host->areq with host->lock
Ulf,
While this patch might be correct, it's not solving the problem I
claimed and my explanation was wrong. See comments in this code
review:
https://chromium-review.googlesource.com/#/c/170880/1//COMMIT_MSG
While I no longer see the same crash with this change in our "ToT
tree", I'm able to reproduce the original mmcqd crash on a different
kernel variant (also based on chromeos-3.4 kernel).
I think I need to review references to mqrq_prev and mqrq_cur since
those appear to be protected by mq->thread_sem and I suspect
references are happening from dw_mmc tasklet without holding this
semaphore.
apologies,
grant
On Thu, Sep 26, 2013 at 12:22 PM, Grant Grundler wrote:
> Races between host->areq being NULL or not are resulting in mmcqd
> hung_task panics. Like this one:
>
> <3>[ 240.501202] INFO: task mmcqd/1:85 blocked for more than 120 seconds.
> <3>[ 240.501213] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> <6>[ 240.501223] mmcqd/1 D 80528020 085 2 0x
> <5>[ 240.501254] [<80528020>] (__schedule+0x614/0x780) from [<80528550>]
> (schedule+0x94/0x98)
> <5>[ 240.501269] [<80528550>] (schedule+0x94/0x98) from [<80526270>]
> (schedule_timeout+0x38/0x2d0)
> <5>[ 240.501284] [<80526270>] (schedule_timeout+0x38/0x2d0) from
> [<805283a4>] (wait_for_common+0x164/0x1a0)
> <5>[ 240.501298] [<805283a4>] (wait_for_common+0x164/0x1a0) from
> [<805284b8>] (wait_for_completion+0x20/0x24)
> <5>[ 240.501313] [<805284b8>] (wait_for_completion+0x20/0x24) from
> [<803d7068>] (mmc_wait_for_req_done+0x2c/0x84)
> <5>[ 240.501327] [<803d7068>] (mmc_wait_for_req_done+0x2c/0x84) from
> [<803d81c0>] (mmc_start_req+0x60/0x120)
> <5>[ 240.501342] [<803d81c0>] (mmc_start_req+0x60/0x120) from [<803e402c>]
> (mmc_blk_issue_rw_rq+0xa0/0x3a8)
> <5>[ 240.501355] [<803e402c>] (mmc_blk_issue_rw_rq+0xa0/0x3a8) from
> [<803e4758>] (mmc_blk_issue_rq+0x424/0x478)
> <5>[ 240.501368] [<803e4758>] (mmc_blk_issue_rq+0x424/0x478) from
> [<803e587c>] (mmc_queue_thread+0xb0/0x118)
> <5>[ 240.501382] [<803e587c>] (mmc_queue_thread+0xb0/0x118) from
> [<8004d61c>] (kthread+0xa8/0xbc)
> <5>[ 240.501396] [<8004d61c>] (kthread+0xa8/0xbc) from [<8000f1c8>]
> (kernel_thread_exit+0x0/0x8)
> <0>[ 240.501407] Kernel panic - not syncing: hung_task: blocked tasks
> <5>[ 240.501421] [<800150a4>] (unwind_backtrace+0x0/0x114) from [<80521920>]
> (dump_stack+0x20/0x24)
> <5>[ 240.501434] [<80521920>] (dump_stack+0x20/0x24) from [<80521a90>]
> (panic+0xa8/0x1f4)
> <5>[ 240.501447] [<80521a90>] (panic+0xa8/0x1f4) from [<80086d3c>]
> (watchdog+0x1f4/0x25c)
> <5>[ 240.501459] [<80086d3c>] (watchdog+0x1f4/0x25c) from [<8004d61c>]
> (kthread+0xa8/0xbc)
> <5>[ 240.501471] [<8004d61c>] (kthread+0xa8/0xbc) from [<8000f1c8>]
> (kernel_thread_exit+0x0/0x8)
>
> I was able to reproduce the mmcqd "hung task" timeout consistently
> with this fio command line on an Exynos5250 system with Toshiba HS200
> eMMC running in HS200 mode:
> fio --name=short_randwrite --size=2G --time_based --runtime=3m \
> --readwrite=randwrite --numjobs=2 --bs=4k --norandommap \
> --ioengine=psync --direct=0 --filename=/dev/mmcblk0p5
>
> I believe the key parameters are "--numjobs=2" (or more) and "randwrite"
> workload. Then the completions are happening around the same time as
> mmc_start_req() is referencing and/or updating host->areq.
>
> I was NOT able to consistently reproduce the problem on a similar
> Exynos 5250 system which had "engineering samples" of Samsung HS200
> capable eMMC installed. Just my clue that the timing is different
> (and the fio performance numbers are different too).
>
> Signed-off-by: Grant Grundler
> ---
> drivers/mmc/core/core.c | 34 +++---
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 36cfe91..e5a9599 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -529,29 +529,40 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
> *host,
> {
> int saved_err = 0;
> int start_err = 0;
> - struct mmc_async_req *saved_areq = host->areq;
> + struct mmc_async_req *saved_areq;
> + unsigned long flags;
>
> - if (!saved_areq &&a
[PATCH 1/7] mmc: core: rename "data" to saved_areq
Replace "data" with a more descriptive name.
Using a local variable (ie a register) makes explicit what the
compiler is doing under the covers anyway: the function is
dereferencing one pointer value the whole time.
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 006ead2..e5a40ee 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -529,14 +529,14 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
{
int err = 0;
int start_err = 0;
- struct mmc_async_req *data = host->areq;
+ struct mmc_async_req *saved_areq = host->areq;
/* Prepare a new request */
if (areq)
- mmc_pre_req(host, areq->mrq, !host->areq);
+ mmc_pre_req(host, areq->mrq, !saved_areq);
- if (host->areq) {
- err = mmc_wait_for_data_req_done(host, host->areq->mrq, areq);
+ if (saved_areq) {
+ err = mmc_wait_for_data_req_done(host, saved_areq->mrq, areq);
if (err == MMC_BLK_NEW_REQUEST) {
if (error)
*error = err;
@@ -550,17 +550,17 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
* Check BKOPS urgency for each R1 response
*/
if (host->card && mmc_card_mmc(host->card) &&
- ((mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1) ||
-(mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1B)) &&
- (host->areq->mrq->cmd->resp[0] & R1_EXCEPTION_EVENT))
+ ((mmc_resp_type(saved_areq->mrq->cmd) == MMC_RSP_R1) ||
+(mmc_resp_type(saved_areq->mrq->cmd) == MMC_RSP_R1B)) &&
+ (saved_areq->mrq->cmd->resp[0] & R1_EXCEPTION_EVENT))
mmc_start_bkops(host->card, true);
}
if (!err && areq)
start_err = __mmc_start_data_req(host, areq->mrq);
- if (host->areq)
- mmc_post_req(host, host->areq->mrq, 0);
+ if (saved_areq)
+ mmc_post_req(host, saved_areq->mrq, 0);
/* Cancel a prepared request if it was not started. */
if ((err || start_err) && areq)
@@ -573,7 +573,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
if (error)
*error = err;
- return data;
+ return saved_areq;
}
EXPORT_SYMBOL(mmc_start_req);
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/7] mmc: core: cleanup and locking patches description
Following 7 patches are mostly cleanup with one key patch around host->areq locking. The host->areq locking problem description is here: http://www.spinics.net/lists/linux-mmc/msg21644.html I do believe this preposed host->areq locking patch is a complete fix. But it appears to fix the problem and is better than nothing. This patch sequence applies clean to linus' 3.12-rc2 branch and only compile tested in this form. This is a forward port (and cleanup) of the same patch I've been testing on ChromeOS-3.4 tree using Exynos5250 chipset. cheers, grant -mmc-core-cleanups-and-locking-description (this email) 0001-mmc-core-rename-data-to-saved_areq.patch 0002-mmc-core-rename-local-var-err-to-saved_err.patch 0003-mmc-core-restructure-error-handling-for-start-req.patch 0004-mmc-core-use-common-code-path-to-return-error.patch 0005-mmc-core-handling-polling-more-gracefully.patch 0006-mmc-core-protect-references-to-host-areq-with-host-l.patch 0007-mmc-core-mmc_start_req-is-a-misnomer-mmc_process_are.patch drivers/mmc/card/block.c| 8 ++-- drivers/mmc/card/mmc_test.c | 4 +- drivers/mmc/core/core.c | 103 +--- include/linux/mmc/core.h| 2 +- 4 files changed, 66 insertions(+), 51 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/7] mmc: core: restructure error handling for start req
This is an intermediate step to fixing the locking around the
access to host->areq. Reduce the number of "if (saved_areq) vs
"if (areq)" tests in the main code path since I'm going to add
references to "host->lock" in the next patch.
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 675139e..4d5de98 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -556,21 +556,20 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
mmc_start_bkops(host->card, true);
}
- if (!saved_err && areq)
+ /* Don't start something new if previous one failed. */
+ if (!saved_err && areq) {
start_err = __mmc_start_data_req(host, areq->mrq);
+ /* Cancel a prepared request if it was not started. */
+ if (start_err) {
+ mmc_post_req(host, areq->mrq, -EINVAL);
+ host->areq = NULL;
+ } else
+ host->areq = areq;
+ }
if (saved_areq)
mmc_post_req(host, saved_areq->mrq, 0);
-/* Cancel a prepared request if it was not started. */
- if ((saved_err || start_err) && areq)
- mmc_post_req(host, areq->mrq, -EINVAL);
-
- if (saved_err)
- host->areq = NULL;
- else
- host->areq = areq;
-
if (error)
*error = saved_err;
return saved_areq;
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 7/7] mmc: core: mmc_start_req is a misnomer -> mmc_process_areq
mmc_start_req() only handles *asynchronous* requests and is easily
confused with mmc_start_request and __mmc_start_req() (both of which
only handle synchronous requests). Renaming should hopefully make
it clearer this function is used to harvest completions and start
new requests.
Signed-off-by: Grant Grundler
---
drivers/mmc/card/block.c| 8
drivers/mmc/card/mmc_test.c | 4 ++--
drivers/mmc/core/core.c | 23 +++
include/linux/mmc/core.h| 2 +-
4 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1a3163f..91a5dae 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1805,7 +1805,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
areq = &mq->mqrq_cur->mmc_active;
} else
areq = NULL;
- areq = mmc_start_req(card->host, areq, (int *) &status);
+ areq = mmc_process_areq(card->host, areq, (int *) &status);
if (!areq) {
if (status == MMC_BLK_NEW_REQUEST)
mq->flags |= MMC_QUEUE_NEW_REQUEST;
@@ -1902,7 +1902,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
if (!mq_rq->packed->retries)
goto cmd_abort;
mmc_blk_packed_hdr_wrq_prep(mq_rq, card, mq);
- mmc_start_req(card->host,
+ mmc_process_areq(card->host,
&mq_rq->mmc_active, NULL);
} else {
@@ -1912,7 +1912,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
*/
mmc_blk_rw_rq_prep(mq_rq, card,
disable_multi, mq);
- mmc_start_req(card->host,
+ mmc_process_areq(card->host,
&mq_rq->mmc_active, NULL);
}
}
@@ -1944,7 +1944,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
mmc_blk_revert_packed_req(mq, mq->mqrq_cur);
mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
- mmc_start_req(card->host,
+ mmc_process_areq(card->host,
&mq->mqrq_cur->mmc_active, NULL);
}
}
diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 0c0fc52..c1e2e09 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -811,7 +811,7 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card
*test,
for (i = 0; i < count; i++) {
mmc_test_prepare_mrq(test, cur_areq->mrq, sg, sg_len, dev_addr,
blocks, blksz, write);
- done_areq = mmc_start_req(test->card->host, cur_areq, &ret);
+ done_areq = mmc_process_areq(test->card->host, cur_areq, &ret);
if (ret || (!done_areq && i > 0))
goto err;
@@ -830,7 +830,7 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card
*test,
dev_addr += blocks;
}
- done_areq = mmc_start_req(test->card->host, NULL, &ret);
+ done_areq = mmc_process_areq(test->card->host, NULL, &ret);
return ret;
err:
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e5a9599..e824ad9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -475,7 +475,7 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
* @is_first_req: true if there is no previous started request
* that may run in parellel to this call, otherwise false
*
- * mmc_pre_req() is called in prior to mmc_start_req() to let
+ * mmc_pre_req() is called in prior to mmc_process_req() to let
* host prepare for the new request. Preparation of a request may be
* performed while another request is running on the host.
*/
@@ -509,22 +509,21 @@ static void mmc_post_req(struct mmc_host *host, struct
mmc_request *mrq,
}
/**
- * mmc_start_req - start a non-blocking request
+ * mmc_process_areq - start a non-blocking IO and handle completions
* @host: MMC host to start command
* @areq: async request to start
- * @error: out parameter returns 0 for success, otherwise non zero
+ * @error: out parameter returns 0 (success) or non-zero (failure)
+ * of completed IO (not the IO we tried to start).
*
-
[PATCH 5/7] mmc: core: handling polling more gracefully
In case threads "race" to harvest async req completions, just return. Signed-off-by: Grant Grundler --- drivers/mmc/core/core.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index deb0ee5..36cfe91 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -531,6 +531,10 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, int start_err = 0; struct mmc_async_req *saved_areq = host->areq; + if (!saved_areq && !areq) + /* Nothing to do...some code is polling. */ + goto set_error; + /* Prepare a new request */ if (areq) mmc_pre_req(host, areq->mrq, !saved_areq); -- 1.8.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/7] mmc: core: use common code path to return error
Don't replicate how the *error is returned by mmc_start_req().
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4d5de98..deb0ee5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -538,13 +538,12 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
if (saved_areq) {
saved_err = mmc_wait_for_data_req_done(host, saved_areq->mrq,
areq);
if (saved_err == MMC_BLK_NEW_REQUEST) {
- if (error)
- *error = saved_err;
/*
* The previous request was not completed,
* nothing to return
*/
- return NULL;
+ saved_areq = NULL;
+ goto set_error;
}
/*
* Check BKOPS urgency for each R1 response
@@ -570,8 +569,10 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
if (saved_areq)
mmc_post_req(host, saved_areq->mrq, 0);
+set_error:
if (error)
*error = saved_err;
+
return saved_areq;
}
EXPORT_SYMBOL(mmc_start_req);
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/7] mmc: core: protect references to host->areq with host->lock
Races between host->areq being NULL or not are resulting in mmcqd
hung_task panics. Like this one:
<3>[ 240.501202] INFO: task mmcqd/1:85 blocked for more than 120 seconds.
<3>[ 240.501213] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
this message.
<6>[ 240.501223] mmcqd/1 D 80528020 085 2 0x
<5>[ 240.501254] [<80528020>] (__schedule+0x614/0x780) from [<80528550>]
(schedule+0x94/0x98)
<5>[ 240.501269] [<80528550>] (schedule+0x94/0x98) from [<80526270>]
(schedule_timeout+0x38/0x2d0)
<5>[ 240.501284] [<80526270>] (schedule_timeout+0x38/0x2d0) from [<805283a4>]
(wait_for_common+0x164/0x1a0)
<5>[ 240.501298] [<805283a4>] (wait_for_common+0x164/0x1a0) from [<805284b8>]
(wait_for_completion+0x20/0x24)
<5>[ 240.501313] [<805284b8>] (wait_for_completion+0x20/0x24) from
[<803d7068>] (mmc_wait_for_req_done+0x2c/0x84)
<5>[ 240.501327] [<803d7068>] (mmc_wait_for_req_done+0x2c/0x84) from
[<803d81c0>] (mmc_start_req+0x60/0x120)
<5>[ 240.501342] [<803d81c0>] (mmc_start_req+0x60/0x120) from [<803e402c>]
(mmc_blk_issue_rw_rq+0xa0/0x3a8)
<5>[ 240.501355] [<803e402c>] (mmc_blk_issue_rw_rq+0xa0/0x3a8) from
[<803e4758>] (mmc_blk_issue_rq+0x424/0x478)
<5>[ 240.501368] [<803e4758>] (mmc_blk_issue_rq+0x424/0x478) from [<803e587c>]
(mmc_queue_thread+0xb0/0x118)
<5>[ 240.501382] [<803e587c>] (mmc_queue_thread+0xb0/0x118) from [<8004d61c>]
(kthread+0xa8/0xbc)
<5>[ 240.501396] [<8004d61c>] (kthread+0xa8/0xbc) from [<8000f1c8>]
(kernel_thread_exit+0x0/0x8)
<0>[ 240.501407] Kernel panic - not syncing: hung_task: blocked tasks
<5>[ 240.501421] [<800150a4>] (unwind_backtrace+0x0/0x114) from [<80521920>]
(dump_stack+0x20/0x24)
<5>[ 240.501434] [<80521920>] (dump_stack+0x20/0x24) from [<80521a90>]
(panic+0xa8/0x1f4)
<5>[ 240.501447] [<80521a90>] (panic+0xa8/0x1f4) from [<80086d3c>]
(watchdog+0x1f4/0x25c)
<5>[ 240.501459] [<80086d3c>] (watchdog+0x1f4/0x25c) from [<8004d61c>]
(kthread+0xa8/0xbc)
<5>[ 240.501471] [<8004d61c>] (kthread+0xa8/0xbc) from [<8000f1c8>]
(kernel_thread_exit+0x0/0x8)
I was able to reproduce the mmcqd "hung task" timeout consistently
with this fio command line on an Exynos5250 system with Toshiba HS200
eMMC running in HS200 mode:
fio --name=short_randwrite --size=2G --time_based --runtime=3m \
--readwrite=randwrite --numjobs=2 --bs=4k --norandommap \
--ioengine=psync --direct=0 --filename=/dev/mmcblk0p5
I believe the key parameters are "--numjobs=2" (or more) and "randwrite"
workload. Then the completions are happening around the same time as
mmc_start_req() is referencing and/or updating host->areq.
I was NOT able to consistently reproduce the problem on a similar
Exynos 5250 system which had "engineering samples" of Samsung HS200
capable eMMC installed. Just my clue that the timing is different
(and the fio performance numbers are different too).
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 36cfe91..e5a9599 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -529,29 +529,40 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
{
int saved_err = 0;
int start_err = 0;
- struct mmc_async_req *saved_areq = host->areq;
+ struct mmc_async_req *saved_areq;
+ unsigned long flags;
- if (!saved_areq && !areq)
- /* Nothing to do...some code is polling. */
+ spin_lock_irqsave(&host->lock, flags);
+ saved_areq = host->areq;
+ if (!saved_areq && !areq) {
+ /* Nothing? Code is racing to harvest a completion. */
+ spin_unlock_irqrestore(&host->lock, flags);
goto set_error;
+ }
/* Prepare a new request */
if (areq)
mmc_pre_req(host, areq->mrq, !saved_areq);
if (saved_areq) {
+ /* This thread owns this IO (saved_areq) for now. */
+ host->areq = NULL;
+ spin_unlock_irqrestore(&host->lock, flags);
+
saved_err = mmc_wait_for_data_req_done(host, saved_areq->mrq,
areq);
if (saved_err == MMC_BLK_NEW_REQUEST) {
- /*
-* The previous request was not completed,
-* nothing to return
-*/
+ spin_lock_irqsave(&host->lock, flags);
+
[PATCH 2/7] mmc: core: rename local var err to saved_err
Just making it more obvious 'err' refers to the status of the in flight
request (aka saved_areq) and not the new async request we might start.
Signed-off-by: Grant Grundler
---
drivers/mmc/core/core.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e5a40ee..675139e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -527,7 +527,7 @@ static void mmc_post_req(struct mmc_host *host, struct
mmc_request *mrq,
struct mmc_async_req *mmc_start_req(struct mmc_host *host,
struct mmc_async_req *areq, int *error)
{
- int err = 0;
+ int saved_err = 0;
int start_err = 0;
struct mmc_async_req *saved_areq = host->areq;
@@ -536,10 +536,10 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
mmc_pre_req(host, areq->mrq, !saved_areq);
if (saved_areq) {
- err = mmc_wait_for_data_req_done(host, saved_areq->mrq, areq);
- if (err == MMC_BLK_NEW_REQUEST) {
+ saved_err = mmc_wait_for_data_req_done(host, saved_areq->mrq,
areq);
+ if (saved_err == MMC_BLK_NEW_REQUEST) {
if (error)
- *error = err;
+ *error = saved_err;
/*
* The previous request was not completed,
* nothing to return
@@ -556,23 +556,23 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
mmc_start_bkops(host->card, true);
}
- if (!err && areq)
+ if (!saved_err && areq)
start_err = __mmc_start_data_req(host, areq->mrq);
if (saved_areq)
mmc_post_req(host, saved_areq->mrq, 0);
/* Cancel a prepared request if it was not started. */
- if ((err || start_err) && areq)
+ if ((saved_err || start_err) && areq)
mmc_post_req(host, areq->mrq, -EINVAL);
- if (err)
+ if (saved_err)
host->areq = NULL;
else
host->areq = areq;
if (error)
- *error = err;
+ *error = saved_err;
return saved_areq;
}
EXPORT_SYMBOL(mmc_start_req);
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] mmc: core: cleanup and locking patches description
Argh...too much wordsmithing... On Thu, Sep 26, 2013 at 12:22 PM, Grant Grundler wrote: > Following 7 patches are mostly cleanup with one key patch around host->areq > locking. The host->areq locking problem description is here: > http://www.spinics.net/lists/linux-mmc/msg21644.html > > I do believe this preposed host->areq locking patch is a complete fix. ... is NOT a complete fix. > But it appears to fix the problem and is better than nothing. Still true. cheers, grant > > This patch sequence applies clean to linus' 3.12-rc2 branch and only compile > tested in this form. This is a forward port (and cleanup) of the > same patch I've been testing on ChromeOS-3.4 tree using Exynos5250 chipset. > > cheers, > grant > > -mmc-core-cleanups-and-locking-description (this email) > 0001-mmc-core-rename-data-to-saved_areq.patch > 0002-mmc-core-rename-local-var-err-to-saved_err.patch > 0003-mmc-core-restructure-error-handling-for-start-req.patch > 0004-mmc-core-use-common-code-path-to-return-error.patch > 0005-mmc-core-handling-polling-more-gracefully.patch > 0006-mmc-core-protect-references-to-host-areq-with-host-l.patch > 0007-mmc-core-mmc_start_req-is-a-misnomer-mmc_process_are.patch > > > drivers/mmc/card/block.c| 8 ++-- > drivers/mmc/card/mmc_test.c | 4 +- > drivers/mmc/core/core.c | 103 > +--- > include/linux/mmc/core.h| 2 +- > 4 files changed, 66 insertions(+), 51 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: core: remove issue_fn indirect function call
On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball wrote: > Hi, > > On Wed, Sep 25 2013, Chris Ball wrote: >> Hi, >> >> On Fri, Sep 20 2013, Ulf Hansson wrote: >>> On 19 September 2013 19:20, Grant Grundler wrote: >>>> struct mmc_queue defines issue_fn as an indirect function call. >>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>>> invoked immediately after calling blk_fetch_request(). >>>> Don't bother with indirect function call - it's pointless and just >>>> obfuscates the code. >>>> >>>> Signed-off-by: Grant Grundler >>> >>> Acked-by: Ulf Hansson >> >> Thanks, pushed to mmc-next for 3.13. > > Have dropped this, it's breaking my build: > > /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: > ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] The function is declared static. :( Let me respin to remove the static and add a function prototype to a header file. I just got lucky when I built this in an earlier tree. sorry about that... grant > /home/cjb/git/mmc/drivers/mmc/card/queue.c: In function ‘mmc_queue_thread’: > /home/cjb/git/mmc/drivers/mmc/card/queue.c:70:4: error: implicit declaration > of function ‘mmc_blk_issue_rq’ [-Werror=implicit-function-declaration] > > Grant, please could you take a look and resubmit? > > - Chris. > -- > Chris Ball <http://printf.net/> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: core: remove issue_fn indirect function call
On Thu, Sep 26, 2013 at 2:56 PM, Grant Grundler wrote: > On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball wrote: >> Hi, >> >> On Wed, Sep 25 2013, Chris Ball wrote: >>> Hi, >>> >>> On Fri, Sep 20 2013, Ulf Hansson wrote: >>>> On 19 September 2013 19:20, Grant Grundler wrote: >>>>> struct mmc_queue defines issue_fn as an indirect function call. >>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>>>> invoked immediately after calling blk_fetch_request(). >>>>> Don't bother with indirect function call - it's pointless and just >>>>> obfuscates the code. >>>>> >>>>> Signed-off-by: Grant Grundler >>>> >>>> Acked-by: Ulf Hansson >>> >>> Thanks, pushed to mmc-next for 3.13. >> >> Have dropped this, it's breaking my build: >> >> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: >> ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] > > The function is declared static. :( Let me respin to remove the > static and add a function prototype to a header file. block.o and queue.o are linked together into one .ko all the time: obj-$(CONFIG_MMC_BLOCK) += mmc_block.o mmc_block-objs := block.o queue.o Two ways to handle this: I can 1) add a local function prototype of mmc_blk_issue_rq() to queue.c 2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c (2) actually makes sense since both functions are block IO specific. Thoughts? Preference? Other ideas? thanks, grant ps. It's more obvious now that the return value from mmc_blk_issue_rq() is getting ignored. *sigh* -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: core: remove dead function mmc_try_claim_host
On Fri, Sep 20, 2013 at 12:35 AM, Ulf Hansson wrote:
> On 20 September 2013 03:21, Grant Grundler wrote:
>> cscsope says there are no callers for mmc_try_claim_host in the kernel.
>> No reason to keep it.
>>
>> Signed-off-by: Grant Grundler
>
> Acked-by: Ulf Hansson
Thank you Ulf!
In which maintainer's git tree/branch should I expect this patch to land?
Just looking for some confirmation that it was applied.
thanks,
grant
>
>> ---
>> drivers/mmc/core/core.c | 25 -
>> include/linux/mmc/core.h | 1 -
>> 2 files changed, 26 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index bf18b6b..006ead2 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -918,31 +918,6 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t
>> *abort)
>> EXPORT_SYMBOL(__mmc_claim_host);
>>
>> /**
>> - * mmc_try_claim_host - try exclusively to claim a host
>> - * @host: mmc host to claim
>> - *
>> - * Returns %1 if the host is claimed, %0 otherwise.
>> - */
>> -int mmc_try_claim_host(struct mmc_host *host)
>> -{
>> - int claimed_host = 0;
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&host->lock, flags);
>> - if (!host->claimed || host->claimer == current) {
>> - host->claimed = 1;
>> - host->claimer = current;
>> - host->claim_cnt += 1;
>> - claimed_host = 1;
>> - }
>> - spin_unlock_irqrestore(&host->lock, flags);
>> - if (host->ops->enable && claimed_host && host->claim_cnt == 1)
>> - host->ops->enable(host);
>> - return claimed_host;
>> -}
>> -EXPORT_SYMBOL(mmc_try_claim_host);
>> -
>> -/**
>> * mmc_release_host - release a host
>> * @host: mmc host to release
>> *
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index da51bec..a00fc49 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -188,7 +188,6 @@ extern unsigned int mmc_align_data_size(struct mmc_card
>> *, unsigned int);
>>
>> extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
>> extern void mmc_release_host(struct mmc_host *host);
>> -extern int mmc_try_claim_host(struct mmc_host *host);
>>
>> extern void mmc_get_card(struct mmc_card *card);
>> extern void mmc_put_card(struct mmc_card *card);
>> --
>> 1.8.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/7] tulip: remove redundant D0 power state set
On Thu, May 30, 2013 at 3:27 AM, Yijing Wang wrote:
> Pci_enable_device() will set device power state to D0,
> so it's no need to do it again in tulip_init_one().
>
> Signed-off-by: Yijing Wang
Ack-by: Grant Grundler
thanks!
grant
> ---
> drivers/net/ethernet/dec/tulip/tulip_core.c |6 --
> 1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c
> b/drivers/net/ethernet/dec/tulip/tulip_core.c
> index 1e9443d..c94152f 100644
> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c
> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
> @@ -1410,12 +1410,6 @@ static int tulip_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> return i;
> }
>
> - /* The chip will fail to enter a low-power state later unless
> -* first explicitly commanded into D0 */
> - if (pci_set_power_state(pdev, PCI_D0)) {
> - pr_notice("Failed to set power state to D0\n");
> - }
> -
> irq = pdev->irq;
>
> /* alloc_etherdev ensures aligned and zeroed private structures */
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 01/12] iommu/exynos: add missing cache flush for removed pagetable entries
On Tuesday, December 25, 2012 6:00:01 PM UTC-8, Cho KyongHo wrote: > This commit adds cache flush for removed small page and large page > entries in exynos_iommu_unmap(). Missing cache flush of removed > page table entries can cause missing page fault interrupt when a > master IP accesses an unmapped area. KyongHo, It appears this patch was never applied and got caught up in the device tree binding discussion. AFAICT, this patch is still necessary. Can you resubmit this patch separately. Or ok if I do? Original patch is here: https://patchwork.kernel.org/patch/1910261/ thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 01/12] iommu/exynos: add missing cache flush for removed pagetable entries
-linux-arm (wrong email address - sorry) On Mon, Jul 1, 2013 at 6:49 PM, Grant Grundler wrote: > On Tuesday, December 25, 2012 6:00:01 PM UTC-8, Cho KyongHo wrote: >> This commit adds cache flush for removed small page and large page >> entries in exynos_iommu_unmap(). Missing cache flush of removed >> page table entries can cause missing page fault interrupt when a >> master IP accesses an unmapped area. > > KyongHo, > It appears this patch was never applied and got caught up in the > device tree binding discussion. AFAICT, this patch is still necessary. > Can you resubmit this patch separately. Or ok if I do? > > Original patch is here: > https://patchwork.kernel.org/patch/1910261/ > > thanks, > grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iommu/exynos: remove dead code (set_prefbuf)
exynos_sysmmu_set_prefbuf() is not called any where.
Signed-off-by: Grant Grundler
---
drivers/iommu/exynos-iommu.c | 44
1 file changed, 44 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 3f32d64..0740189 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -247,50 +247,6 @@ static void __sysmmu_set_prefbuf(void __iomem *sfrbase,
unsigned long base,
__raw_writel(size - 1 + base, sfrbase + REG_PB0_EADDR + idx * 8);
}
-void exynos_sysmmu_set_prefbuf(struct device *dev,
- unsigned long base0, unsigned long size0,
- unsigned long base1, unsigned long size1)
-{
- struct sysmmu_drvdata *data = dev_get_drvdata(dev->archdata.iommu);
- unsigned long flags;
- int i;
-
- BUG_ON((base0 + size0) <= base0);
- BUG_ON((size1 > 0) && ((base1 + size1) <= base1));
-
- read_lock_irqsave(&data->lock, flags);
- if (!is_sysmmu_active(data))
- goto finish;
-
- for (i = 0; i < data->nsfrs; i++) {
- if ((readl(data->sfrbases[i] + REG_MMU_VERSION) >> 28) == 3) {
- if (!sysmmu_block(data->sfrbases[i]))
- continue;
-
- if (size1 == 0) {
- if (size0 <= SZ_128K) {
- base1 = base0;
- size1 = size0;
- } else {
- size1 = size0 -
- ALIGN(size0 / 2, SZ_64K);
- size0 = size0 - size1;
- base1 = base0 + size0;
- }
- }
-
- __sysmmu_set_prefbuf(
- data->sfrbases[i], base0, size0, 0);
- __sysmmu_set_prefbuf(
- data->sfrbases[i], base1, size1, 1);
-
- sysmmu_unblock(data->sfrbases[i]);
- }
- }
-finish:
- read_unlock_irqrestore(&data->lock, flags);
-}
-
static void __set_fault_handler(struct sysmmu_drvdata *data,
sysmmu_fault_handler_t handler)
{
--
1.8.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 01/12] iommu/exynos: add missing cache flush for removed pagetable entries
Ping? On Mon, Jul 1, 2013 at 6:49 PM, Grant Grundler wrote: > On Tuesday, December 25, 2012 6:00:01 PM UTC-8, Cho KyongHo wrote: >> This commit adds cache flush for removed small page and large page >> entries in exynos_iommu_unmap(). Missing cache flush of removed >> page table entries can cause missing page fault interrupt when a >> master IP accesses an unmapped area. > > KyongHo, > It appears this patch was never applied and got caught up in the > device tree binding discussion. AFAICT, this patch is still necessary. > Can you resubmit this patch separately. Or ok if I do? > > Original patch is here: > https://patchwork.kernel.org/patch/1910261/ > > thanks, > grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 03/16] iommu/exynos: fix page table maintenance
On Wed, Aug 14, 2013 at 3:49 AM, Joerg Roedel wrote: > On Thu, Aug 08, 2013 at 11:28:44AM -0700, Grant Grundler wrote: >> I can't speak to the previous BUG_ON(). I believe the EADDRESSINUSE >> failures could be either WARN_ON or BUG_ON. This condition is >> clearly a bug in the generic IOMMU allocator and I think that's why >> KyongHo Cho used BUG_ON. >> >> Handing out duplicate addresses will generally lead to some sort of >> data corruption or other fault depending on how robust the underlying >> device drivers are written. So my preference is a BUG_ON to >> immediately flag this condition instead of hoping a device driver will >> correctly handling the dma mapping failure (Some do, most currently >> don't). >> >> WARN_ON() + return -EADDRESSINUSE would be a good alternative. > > Even if it is a real BUG condition, I don't think it is worth to stop > execution at this point. It makes debugging harder and the system less > reliable. I prefer to go with the WARN_ON and an error return value. I'm ok with WARN_ON and an error return value. This is "valid" behavior. I expect this bug to never happen but if and when it does, I want a clear symptom (e.g. WARN_ON) that it happened. My concern is that historically, drivers did not get an error return value on failure: ftp://193.166.3.4/pub/linux/kernel/v2.3/patch-html/patch-2.3.47/linux_Documentation_DMA-mapping.txt.html or later: https://www.kernel.org/pub/linux/kernel/people/marcelo/linux-2.4/Documentation/DMA-mapping.txt And thus, some drivers don't check or attempt to handle mapping failures based on this existing code. Here is a recent example: http://comments.gmane.org/gmane.linux.network/272969 I hope very few or none of those exist since Neil Horman demonstrated "dma debugging" can flag this behavior. Just for fun, I'll include this link : (apperently 2003 was a good year for DMA talks :) http://ols.fedoraproject.org/OLS/Reprints-2003/LinuxSymposium2003-2side.pdf (three talks on DMA issues) thanks grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 00/12] iommu/exynos: Fixes and Enhancements of System MMU driver with DT
On Mon, Jul 15, 2013 at 5:20 AM, Cho KyongHo wrote: ... >> > Cho, >> > Of the above patches, nearly all have been applied to chromeos-3.8 >> > (kernel-next git tree) by Doug Anderson and others. >> > >> > AFAICT, the only ones not applied are: >> >[v7,3/9] iommu/exynos: fix page table maintenance >> >[v7,6/9] clk: exynos5250: add gate clock descriptions of System MMU >> > (conflicts in this one) >> >[v7,7/9] ARM: dts: Add description of System MMU of Exynos SoCs >> > (depends on 6/9) >> > >> > We also already have parts of: >> >[v7,9/9] iommu/exynos: add bus notifier for registering System MMU >> > >> > Some of those are being further discussed but I've lost track now >> > exactly which ones. >> > >> > I'm telling you about chromeos-3.8 status since the adopted changes >> > have been reviewed (by me and others) are being tested manually here >> > on several different Samsung Exynos platforms (including 5250 which is >> > our "snow" platform). Not sure how you should to mark those patches >> > since they aren't identical to your changes (which apply to post 3.10 >> > kernels, not 3.8). You might consider splitting those patches out >> > from the 4 I've listed above to get that series accepted upstream >> > since the additional review/testing should provide some confidence >> > those patches are good. >> > >> >> I understand what you are concerning about. >> Have you applied v6 patchset? I did not though it's possible some of the code that was applied to chromeos-3.8 kernel came from v6 (or earlier) patches. I just compared with v7. Since it's a "backport", the code I found in chromeos-3.8 is the same if not identical to v7. >> >> I will try to split the patches and make the changes from v6 >> on top of the v6 patcheset. Ok. > Actually, as you know, the previous patches include setting a System MMU > as the parent device of its master device in probe() of System MMU. > I asked Greg KH about changing device hierarchy in probe() and he answered > that it is not a good idea because it modifies sysfs even though probe() of > System MMU driver is called before sysfs is constructed. SysMMU primarily provides DMA API, right? Can sysfs be initialized before DMA is available? On some (many?) platforms the SysMMU is also responsible for "child" bus controllers and MMIO resource routing, and on ARM platforms, clocks for rest of the IO devices. SysMMU might be needed for early gfx support (or something). Maybe the right answer is to "discover" the SysMMU twice: once very early to get the SysMMU operational and then again later in a "normal" probe sequence to register with the sysfs. Any reason a driver couldn't register two different module_init() routines in different parts of the init call table? > That's why I uses genpd_pm_ops. > It results in big change in the patches after registering device tree. > > I want to ask your opinion about this change :) I have no objection to large changes if they work well and are easy to understand. I personally not happy with the direction the Linux IOMMU "subsystem" has taken. I think the generic IOMMU subsystem should be discarded. Strong words for someone who's not a key contributor but I'll explain. I wrote the IOMMU support for PA-RISC (three different IOMMUs) 10+ years ago. I rewrote the HPUX implementations in the 90's and then around 2000 wrote the implementation for PA-RISC (one of which Alex Williamson used for IA64 IOMMU support - sba_iommu.c). It's pretty straight forward. There is "page allocation policy", code to do TLB shoot down, and code to manage the IO Pdir. This is the same as for CPU MMU. The *efficiency* of the TLB depends on the page allocation (SW) to work together with the HW TLB replacement policy. In other words, the page allocation policy needs to avoid thrashing the IO TLB. A generic IOTLB allocation policy can't do that for every platform. Exynos 5250 has an 8-way associative TLB. That's likely very different from the Intel and TI (omap) IOMMU the IOMMU subsystem code was originally implemented for. The "optimal" size of page to allocate will be different too. Older Exynos products are likely different The "2cd layer" of indirect function calls between the generic IOMMU support and the platform specific code makes it harder to understand the code. While jump tables are good for some things, I don't think this is a good use. We should "flip" this design around. Let the platform specific code "own" the DMA ops API and it can decide which services it should or should not use. E.g. The IOTLB shoot down can be divorced from the "unmap" call. The efficiency of shooting down one IOTLB entry at a time is horrible on most platforms. One answer is to add a new API to shoot down an array of IOVAs. But the tradeoffs in the size of the array and the events that might trigger a TLB shoot down should be left up to the platform/chipset specific code (e.g. "lazy TLB shootdown"). Sorry...this got rant got alot longer than it should h
Re: [PATCH v7 3/9] iommu/exynos: fix page table maintenance
On Mon, Jul 15, 2013 at 4:21 AM, Cho KyongHo wrote: ... >> Maybe you could add LV1TABLE_SIZE define and use it here (there is >> already a LV2TABLE_SIZE define)? > > Yes. But, LV2TABLE_SIZE is used in more places than one. > I do not feel that it is needed to define LV1TABLE_SIZE for the single line. Cho, #define's are part of the code "documentation". Doesn't really matter how often it's used. I think Bartlomiej's suggestion is a good one. Key is to use "consistent" names so they makes sense to someone not familiar with the code. In this case, we want to show this use is the same way LV2TABLE_SIZE is used and serves the same purpose. cheers. grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 03/12] iommu/exynos: fix page table maintenance
On Mon, Jul 29, 2013 at 2:18 AM, Cho KyongHo wrote:
...
>> > @@ -908,7 +916,7 @@ static int lv2set_page(unsigned long *pent,
>> > phys_addr_t paddr, size_t size,
>> > int i;
>> > for (i = 0; i < SPAGES_PER_LPAGE; i++, pent++) {
>> > if (!lv2ent_fault(pent)) {
>> > - memset(pent, 0, sizeof(*pent) * i);
>> > + clear_page_table(pent - i, i);
>> > return -EADDRINUSE;
>>
>> I am wondering about two issues with this error handling:
>> 1) we don't call pgtable_flush() in this case - I think just for
>> consistency we should - don't rush to add since my next comment is to
>> change this error handling completely.
>>
> clear_page_table() is called for the page table entries that are already
> fault pages. That is why it does not contain cache flush.
>
>> 2) If -EADDRINUSE is correct, why does the code clear the IO Page
>> table entries?
>>
>>I think this error path should either
>>(a) BUG_ON (ie panic) since this is an inconsistency between
>> generic IOMMU page allocation and chip specific IOMMU mapping code OR
>>(b) WARN_ON, not clear the entries, and hope whoever was using it
>> can finish using the system before crashing or gracefully shutting
>> down.
>>
>> In any case, I'm pretty sure this code needs to change and it should
>> be in a follow up to this series.
>
> Yes, you're right. But I worried the case that a kernel module calls IOMMU API
> functions directly and does not want to make kernel panic when it tries to map
> a region that is already in use.
Using a DMA address for a different physical page while the current
mapping is still active can only be a bug. I can confidently say there
is no way to map the same DMA address twice (at least not for a single
page table.) We can try to fail the mapping somehow and WARN_ON to
indicate we had a "Re-Use before free" type bug.
> I also wonder if the such kernel module exists.
I believe the kernel will never do this.
> WARN_ON is also a good idea.
After this series goes in, post another patch and I'd be happy to
review that as well.
After thinking about it more, I'm also ok with removing this code.
It's a very "defensive" code to catch errors in the generic IOMMU code
that probably no longer exist. Or maybe just make it
"CONFIG_DEBUG_IOMMU_ALLOC" or something like that.
cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] iommu/exynos: remove dead code (set_prefbuf)
Ping?
Adding linux-iommu ML.
thanks,
grant
On Tue, Jul 2, 2013 at 9:08 AM, Grant Grundler wrote:
> exynos_sysmmu_set_prefbuf() is not called any where.
>
> Signed-off-by: Grant Grundler
> ---
> drivers/iommu/exynos-iommu.c | 44
>
> 1 file changed, 44 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 3f32d64..0740189 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -247,50 +247,6 @@ static void __sysmmu_set_prefbuf(void __iomem *sfrbase,
> unsigned long base,
> __raw_writel(size - 1 + base, sfrbase + REG_PB0_EADDR + idx * 8);
> }
>
> -void exynos_sysmmu_set_prefbuf(struct device *dev,
> - unsigned long base0, unsigned long size0,
> - unsigned long base1, unsigned long size1)
> -{
> - struct sysmmu_drvdata *data = dev_get_drvdata(dev->archdata.iommu);
> - unsigned long flags;
> - int i;
> -
> - BUG_ON((base0 + size0) <= base0);
> - BUG_ON((size1 > 0) && ((base1 + size1) <= base1));
> -
> - read_lock_irqsave(&data->lock, flags);
> - if (!is_sysmmu_active(data))
> - goto finish;
> -
> - for (i = 0; i < data->nsfrs; i++) {
> - if ((readl(data->sfrbases[i] + REG_MMU_VERSION) >> 28) == 3) {
> - if (!sysmmu_block(data->sfrbases[i]))
> - continue;
> -
> - if (size1 == 0) {
> - if (size0 <= SZ_128K) {
> - base1 = base0;
> - size1 = size0;
> - } else {
> - size1 = size0 -
> - ALIGN(size0 / 2, SZ_64K);
> - size0 = size0 - size1;
> - base1 = base0 + size0;
> - }
> - }
> -
> - __sysmmu_set_prefbuf(
> - data->sfrbases[i], base0, size0, 0);
> - __sysmmu_set_prefbuf(
> - data->sfrbases[i], base1, size1, 1);
> -
> - sysmmu_unblock(data->sfrbases[i]);
> - }
> - }
> -finish:
> - read_unlock_irqrestore(&data->lock, flags);
> -}
> -
> static void __set_fault_handler(struct sysmmu_drvdata *data,
> sysmmu_fault_handler_t handler)
> {
> --
> 1.8.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 06/12] ARM: dts: Add description of System MMU of Exynos SoCs
Hi Marek, On Tue, Aug 6, 2013 at 6:17 AM, Marek Szyprowski wrote: ... > IMHO it is much better to have a simple driver, which binds to a single > IOMMU controller and leave it to the driver whether to have a same virtual > address > space for all parts of FIMC-IS or MFC submodules/memory ports or not. I understand this part. I having written the IOMMU support for 4 different IOMMUs, all of which had exactly one IO Page Table and one IOMMU shared by many devices. > Just make sure that it will be possible to attach more than one sysmmu > controller to one iommu domain. I don't understand how this is possible. Can someone explain this better in the IOMMU documentation please? "iommu domain" to me means one virtual IO address space for attached devices that can master DMA transactions. The IOMMU then uses it's IO Page Table to translate the DMA address to the system physical address space and forwards the transaction. What is the role of the sysmmu in all of this? Is the sysmmu just the MMU (or collection of MMU) for host DRAM? Or is sysmmu responsible for "other stuff"? (clocks, power domains, MMU, etc) I can understand we might have multiple MMUs in a system...e.g. every range of memory might have it's own MMU. But they share the same physical address space and generally live under one page table. Because of "one page table" I would consider them one entity from the the IOMMUs perspective. thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 06/12] ARM: dts: Add description of System MMU of Exynos SoCs
On Wed, Aug 7, 2013 at 5:07 AM, Cho KyongHo wrote: ... >> I don't understand how this is possible. Can someone explain this >> better in the IOMMU documentation please? > > System MMU is dedicated to a master H/W such as FIMD and FIMC. Sory - Exynos 5250 documentation I have (confidential version) uses FIMD and FIMC but never explains what they are nor identifies them in a diagram. Based on the references, they are related to the video mixer but I don't know exactly what function FIMD/FIMC serve. > Thus, attaching a master H/W to an iommu domain can be thought as > attaching a System MMU to an iommu domain even though such thinking > is not correct view of the relationship between iommu domain and > System MMU. This almost makes sense. I understand the above to mean the System MMU is a proxy for the FIMD and FIMC. >> I can understand we might have multiple MMUs in a system...e.g. every >> range of memory might have it's own MMU. But they share the same >> physical address space and generally live under one page table. >> Because of "one page table" I would consider them one entity from the >> the IOMMUs perspective. > > Sorry, I don't understand. > Do you mean you are thinking that it is better to share one page table > by all IOMMUs in a system? No. This is how the previous IOMMUs I worked on functioned. It doesn't mean this is how current ones should. My example above was referring to CPU MMUs in the case of NUMA architectures. Each NUMA CPU socket can have it's own MMU (and TLB) and corresponding memory controller. All CPUs in an SMP system map process and kernel virtual addresses to one common "physical" address space. This means allocation and use of "physical address space" has to be managed as one entity (even if several page tables exist in the implementation - e.g. NUMA). Back to the original comment that started my question (pulled out of context now...sorry): "Just make sure that it will be possible to attach more than one sysmmu controller to one iommu domain." Does that mean the IOMMU now has to map to multiple "physical address spaces" or am I completely missing what a SysMMU does? The "SysMMU" is the System Memory Management Unit, right? I still thinking one IOMMU domain maps one (IO) virtual address space to one (common with CPU and other IOMMU) physical address space. cheers, grant > > Thank you, > KyongHo >> >> thanks, >> grant > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 03/16] iommu/exynos: fix page table maintenance
Tomasz,
On Thu, Aug 8, 2013 at 6:54 AM, Tomasz Figa wrote:
...
>> + BUG_ON(lv1ent_section(sent));
>
> Is this condition really a critical one, to the point that the system
> should not continue execution?
>
...
>> if (lv1ent_page(sent)) {
>> - if (*pgcnt != NUM_LV2ENTRIES)
>> - return -EADDRINUSE;
>> -
>> + BUG_ON(*pgcnt != NUM_LV2ENTRIES);
>
> Ditto.
I can't speak to the previous BUG_ON(). I believe the EADDRESSINUSE
failures could be either WARN_ON or BUG_ON. This condition is
clearly a bug in the generic IOMMU allocator and I think that's why
KyongHo Cho used BUG_ON.
Handing out duplicate addresses will generally lead to some sort of
data corruption or other fault depending on how robust the underlying
device drivers are written. So my preference is a BUG_ON to
immediately flag this condition instead of hoping a device driver will
correctly handling the dma mapping failure (Some do, most currently
don't).
WARN_ON() + return -EADDRESSINUSE would be a good alternative.
thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A
On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet wrote: ... > I guess that if a driver does not advertise NETIF_F_SG, this > skb_linearize() call is not needed : All frames reaching your xmit > function should already be linear As Ben Hutchings pointed out, hw_features is still setting this...but I'm not sure how that matters. ax88179_set_features() doesn't allow setting SG or TSO features. But I expect it would be "not too difficult" to add such that ethtool could set those features after boot. Or perhaps add a driver module parameter to set these features. I just guessing the skb_linearize() could be removed until set_features support for SG and/or TSO is added. Is that correct? thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A
On Tue, Jul 23, 2013 at 4:46 PM, David Miller wrote: ... > A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have > this problem. > > Instead of the patch starting this thread, I'd like to see one that > hits all three drivers and removes all SG and TSO features bits from > both the ->features _and_ ->hw_features settings. Since you are asking to remove TSO, do you also want skb_linearize() calls in ax88179_178a.c and smsc75xx.c removed as well? Not part of the original patch - but based on this thread, Eric seems to think calling skb_linearize isn't necessary or helpful either. cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A
On Tue, Jul 23, 2013 at 7:29 PM, Grant Grundler wrote: > On Tue, Jul 23, 2013 at 4:46 PM, David Miller wrote: > ... >> A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have >> this problem. >> >> Instead of the patch starting this thread, I'd like to see one that >> hits all three drivers and removes all SG and TSO features bits from >> both the ->features _and_ ->hw_features settings. > > Since you are asking to remove TSO, do you also want skb_linearize() > calls in ax88179_178a.c and smsc75xx.c removed as well? Nevermind...Eric already removed skb_linearize calls in his patch. cheers, grant > > Not part of the original patch - but based on this thread, Eric seems > to think calling skb_linearize isn't necessary or helpful either. > > cheers, > grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 02/12] iommu/exynos: add missing cache flush for removed page table entries
On Fri, Jul 26, 2013 at 4:26 AM, Cho KyongHo wrote:
> This commit adds cache flush for removed small and large page entries
> in exynos_iommu_unmap(). Missing cache flush of removed page table
> entries can cause missing page fault interrupt when a master IP
> accesses an unmapped area.
>
> Signed-off-by: Cho KyongHo
Tested-by: Grant Grundler
I'm convinced this is a critical fix to apply. Any time we touch the
IOMMU Page Table, we need to flush so the change is immediately
visible to the IOMMU.
thanks,
grant
> ---
> drivers/iommu/exynos-iommu.c |2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 233f382..e3be3e5 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -1002,6 +1002,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain
> *domain,
> if (lv2ent_small(ent)) {
> *ent = 0;
> size = SPAGE_SIZE;
> + pgtable_flush(ent, ent +1);
> priv->lv2entcnt[lv1ent_offset(iova)] += 1;
> goto done;
> }
> @@ -1010,6 +1011,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain
> *domain,
> BUG_ON(size < LPAGE_SIZE);
>
> memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE);
> + pgtable_flush(ent, ent + SPAGES_PER_LPAGE);
>
> size = LPAGE_SIZE;
> priv->lv2entcnt[lv1ent_offset(iova)] += SPAGES_PER_LPAGE;
> --
> 1.7.2.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 03/12] iommu/exynos: fix page table maintenance
On Fri, Jul 26, 2013 at 4:27 AM, Cho KyongHo wrote:
> This prevents allocating lv2 page table for the lv1 page table entry
> that already has 1MB page mapping. In addition some BUG_ON() is
> changed to WARN_ON().
>
> Signed-off-by: Cho KyongHo
Reviewed-by: Grant Grundler
In reviewing this, I noticed another issue that is related, but not
caused by this patch. See below.
> ---
> drivers/iommu/exynos-iommu.c | 52 +
> 1 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index e3be3e5..6c4ecce 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -52,11 +52,11 @@
> #define lv2ent_large(pent) ((*(pent) & 3) == 1)
>
> #define section_phys(sent) (*(sent) & SECT_MASK)
> -#define section_offs(iova) ((iova) & 0xF)
> +#define section_offs(iova) ((iova) & ~SECT_MASK)
> #define lpage_phys(pent) (*(pent) & LPAGE_MASK)
> -#define lpage_offs(iova) ((iova) & 0x)
> +#define lpage_offs(iova) ((iova) & ~LPAGE_MASK)
> #define spage_phys(pent) (*(pent) & SPAGE_MASK)
> -#define spage_offs(iova) ((iova) & 0xFFF)
> +#define spage_offs(iova) ((iova) & ~SPAGE_MASK)
>
> #define lv1ent_offset(iova) ((iova) >> SECT_ORDER)
> #define lv2ent_offset(iova) (((iova) & 0xFF000) >> SPAGE_ORDER)
> @@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long
> *sent, unsigned long iova,
> pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> if (!pent)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> *sent = mk_lv1ent_page(__pa(pent));
> *pgcounter = NUM_LV2ENTRIES;
> pgtable_flush(pent, pent + NUM_LV2ENTRIES);
> pgtable_flush(sent, sent + 1);
> + } else if (lv1ent_section(sent)) {
> + return ERR_PTR(-EADDRINUSE);
> }
>
> return page_entry(sent, iova);
> @@ -894,6 +896,12 @@ static int lv1set_section(unsigned long *sent,
> phys_addr_t paddr, short *pgcnt)
> return 0;
> }
>
> +static void clear_page_table(unsigned long *ent, int n)
> +{
> + if (n > 0)
> + memset(ent, 0, sizeof(*ent) * n);
> +}
> +
> static int lv2set_page(unsigned long *pent, phys_addr_t paddr, size_t size,
> short *pgcnt)
> {
> @@ -908,7 +916,7 @@ static int lv2set_page(unsigned long *pent, phys_addr_t
> paddr, size_t size,
> int i;
> for (i = 0; i < SPAGES_PER_LPAGE; i++, pent++) {
> if (!lv2ent_fault(pent)) {
> - memset(pent, 0, sizeof(*pent) * i);
> + clear_page_table(pent - i, i);
> return -EADDRINUSE;
I am wondering about two issues with this error handling:
1) we don't call pgtable_flush() in this case - I think just for
consistency we should - don't rush to add since my next comment is to
change this error handling completely.
2) If -EADDRINUSE is correct, why does the code clear the IO Page
table entries?
I think this error path should either
(a) BUG_ON (ie panic) since this is an inconsistency between
generic IOMMU page allocation and chip specific IOMMU mapping code OR
(b) WARN_ON, not clear the entries, and hope whoever was using it
can finish using the system before crashing or gracefully shutting
down.
In any case, I'm pretty sure this code needs to change and it should
be in a follow up to this series.
thanks,
grant
> }
>
> @@ -944,17 +952,16 @@ static int exynos_iommu_map(struct iommu_domain
> *domain, unsigned long iova,
> pent = alloc_lv2entry(entry, iova,
>
> &priv->lv2entcnt[lv1ent_offset(iova)]);
>
> - if (!pent)
> - ret = -ENOMEM;
> + if (IS_ERR(pent))
> + ret = PTR_ERR(pent);
> else
> ret = lv2set_page(pent, paddr, size,
>
> &priv->lv2entcnt[lv1ent_offset(iova)]);
> }
>
> - if (ret) {
> - pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n",
> - __func__, iova, size);
> - }
> + if (ret)
> + pr_err("%s: Failed(%d) to map 0x%#x bytes @ %#lx\n",
> +
Re: [PATCH v8 11/12] iommu/exynos: change rwlock to spinlock
On Fri, Jul 26, 2013 at 4:30 AM, Cho KyongHo wrote:
> Since acquiring read_lock is not more frequent than write_lock, it is
> not beneficial to use rwlock, this commit changes rwlock to spinlock.
>
> Signed-off-by: Cho KyongHo
Reviewed-by: Grant Grundler
cheers,
grant
> ---
> drivers/iommu/exynos-iommu.c | 36 ++--
> 1 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index c62c244..51e5b35 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -187,7 +187,7 @@ struct sysmmu_drvdata {
> struct clk *clk;
> struct clk *clk_master;
> int activations;
> - rwlock_t lock;
> + spinlock_t lock;
> struct iommu_domain *domain;
> bool runtime_active;
> unsigned long pgtable;
> @@ -287,7 +287,7 @@ void exynos_sysmmu_set_prefbuf(struct device *dev,
> BUG_ON((base0 + size0) <= base0);
> BUG_ON((size1 > 0) && ((base1 + size1) <= base1));
>
> - read_lock_irqsave(&data->lock, flags);
> + spin_lock_irqsave(&data->lock, flags);
> if (!is_sysmmu_active(data))
> goto finish;
>
> @@ -319,7 +319,7 @@ void exynos_sysmmu_set_prefbuf(struct device *dev,
> }
> clk_disable(data->clk_master);
> finish:
> - read_unlock_irqrestore(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
> }
>
> static void show_fault_information(const char *name,
> @@ -371,7 +371,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void
> *dev_id)
>
> if (client)
> spin_lock(&client->lock);
> - read_lock(&data->lock);
> + spin_lock(&data->lock);
>
> if (i == data->nsfrs) {
> itype = SYSMMU_FAULT_UNKNOWN;
> @@ -402,7 +402,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void
> *dev_id)
> if (itype != SYSMMU_FAULT_UNKNOWN)
> sysmmu_unblock(data->sfrbases[i]);
>
> - read_unlock(&data->lock);
> + spin_unlock(&data->lock);
> if (client)
> spin_unlock(&client->lock);
>
> @@ -429,7 +429,7 @@ static bool __sysmmu_disable(struct sysmmu_drvdata *data)
> bool disabled;
> unsigned long flags;
>
> - write_lock_irqsave(&data->lock, flags);
> + spin_lock_irqsave(&data->lock, flags);
>
> disabled = set_sysmmu_inactive(data);
>
> @@ -446,7 +446,7 @@ static bool __sysmmu_disable(struct sysmmu_drvdata *data)
> data->activations);
> }
>
> - write_unlock_irqrestore(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
>
> return disabled;
> }
> @@ -493,7 +493,7 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data,
> int ret = 0;
> unsigned long flags;
>
> - write_lock_irqsave(&data->lock, flags);
> + spin_lock_irqsave(&data->lock, flags);
> if (set_sysmmu_active(data)) {
> data->pgtable = pgtable;
> data->domain = domain;
> @@ -511,7 +511,7 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data,
> if (WARN_ON(ret < 0))
> set_sysmmu_inactive(data); /* decrement count */
>
> - write_unlock_irqrestore(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
>
> return ret;
> }
> @@ -602,7 +602,7 @@ static void sysmmu_tlb_invalidate_entry(struct device
> *dev, unsigned long iova)
>
> data = dev_get_drvdata(client->sysmmu[i]);
>
> - read_lock_irqsave(&data->lock, flags);
> + spin_lock_irqsave(&data->lock, flags);
> if (is_sysmmu_active(data) && data->runtime_active) {
> int i;
> clk_enable(data->clk_master);
> @@ -615,7 +615,7 @@ static void sysmmu_tlb_invalidate_entry(struct device
> *dev, unsigned long iova)
> "disabled. Skipping TLB invalidation @
> %#lx\n",
> iova);
> }
> - read_unlock_irqrestore(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
> }
> }
>
> @@ -630,7 +630,7 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
>
> data = dev_get_drvdata(client->sysmmu[i]);
&
Re: [PATCH v8 09/12] iommu/exynos: remove custom fault handler
On Fri, Jul 26, 2013 at 4:29 AM, Cho KyongHo wrote:
> This commit removes custom fault handler. The device drivers that
> need to register fault handler can register
> with iommu_set_fault_handler().
>
> Signed-off-by: Cho KyongHo
Reviewed-by: Grant Grundler
cheers
grant
> ---
> drivers/iommu/exynos-iommu.c | 71
> ++
> 1 files changed, 17 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 87f6bb7..f9853fe 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -131,16 +131,6 @@ enum exynos_sysmmu_inttype {
> SYSMMU_FAULTS_NUM
> };
>
> -/*
> - * @itype: type of fault.
> - * @pgtable_base: the physical address of page table base. This is 0 if
> @itype
> - *is SYSMMU_BUSERROR.
> - * @fault_addr: the device (virtual) address that the System MMU tried to
> - * translated. This is 0 if @itype is SYSMMU_BUSERROR.
> - */
> -typedef int (*sysmmu_fault_handler_t)(enum exynos_sysmmu_inttype itype,
> - unsigned long pgtable_base, unsigned long fault_addr);
> -
> static unsigned short fault_reg_offset[SYSMMU_FAULTS_NUM] = {
> REG_PAGE_FAULT_ADDR,
> REG_AR_FAULT_ADDR,
> @@ -181,7 +171,6 @@ struct sysmmu_drvdata {
> int activations;
> rwlock_t lock;
> struct iommu_domain *domain;
> - sysmmu_fault_handler_t fault_handler;
> unsigned long pgtable;
> void __iomem *sfrbases[0];
> };
> @@ -313,34 +302,17 @@ finish:
> read_unlock_irqrestore(&data->lock, flags);
> }
>
> -static void __set_fault_handler(struct sysmmu_drvdata *data,
> - sysmmu_fault_handler_t handler)
> -{
> - unsigned long flags;
> -
> - write_lock_irqsave(&data->lock, flags);
> - data->fault_handler = handler;
> - write_unlock_irqrestore(&data->lock, flags);
> -}
> -
> -void exynos_sysmmu_set_fault_handler(struct device *dev,
> - sysmmu_fault_handler_t handler)
> -{
> - struct sysmmu_drvdata *data = dev_get_drvdata(dev->archdata.iommu);
> -
> - __set_fault_handler(data, handler);
> -}
> -
> -static int default_fault_handler(enum exynos_sysmmu_inttype itype,
> -unsigned long pgtable_base, unsigned long fault_addr)
> +static void show_fault_information(const char *name,
> + enum exynos_sysmmu_inttype itype,
> + unsigned long pgtable_base, unsigned long fault_addr)
> {
> unsigned long *ent;
>
> if ((itype >= SYSMMU_FAULTS_NUM) || (itype < SYSMMU_PAGEFAULT))
> itype = SYSMMU_FAULT_UNKNOWN;
>
> - pr_err("%s occurred at 0x%lx(Page table base: 0x%lx)\n",
> - sysmmu_fault_name[itype], fault_addr, pgtable_base);
> + pr_err("%s occurred at 0x%lx by %s(Page table base: 0x%lx)\n",
> + sysmmu_fault_name[itype], fault_addr, name, pgtable_base);
>
> ent = section_entry(__va(pgtable_base), fault_addr);
> pr_err("\tLv1 entry: 0x%lx\n", *ent);
> @@ -353,16 +325,12 @@ static int default_fault_handler(enum
> exynos_sysmmu_inttype itype,
> pr_err("Generating Kernel OOPS... because it is unrecoverable.\n");
>
> BUG();
> -
> - return 0;
> }
>
> static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
> {
> /* SYSMMU is in blocked when interrupt occurred. */
> struct sysmmu_drvdata *data = dev_id;
> - struct resource *irqres;
> - struct platform_device *pdev;
> enum exynos_sysmmu_inttype itype;
> unsigned long addr = -1;
>
> @@ -372,14 +340,15 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void
> *dev_id)
>
> WARN_ON(!is_sysmmu_active(data));
>
> - pdev = to_platform_device(data->sysmmu);
> - for (i = 0; i < (pdev->num_resources / 2); i++) {
> - irqres = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> + for (i = 0; i < data->nsfrs; i++) {
> + struct resource *irqres;
> + irqres =
> platform_get_resource(to_platform_device(data->sysmmu),
> + IORESOURCE_IRQ, i);
> if (irqres && ((int)irqres->start == irq))
> break;
> }
>
> - if (i == pdev->num_resources) {
> + if (i == data->nsfrs) {
> itype = SYSMMU_FAULT_UNKNOWN;
> } else {
>
Re: [PATCH v8 12/12] iommu/exynos: return 0 if iommu_attach_device() successes
On Fri, Jul 26, 2013 at 4:31 AM, Cho KyongHo wrote:
> iommu_attach_device() against exynos-iommu positive integer on success
> if the caller calls iommu_attach_device() with the same iommu_domain
> multiple times without call to iommu_detach_device() to inform the
> caller how many calls to iommu_detach_device() to really detach iommu.
>
> However the convention of the return value of success of common API is
> zero, this patch makes iommu_attach_device() call against exynos-iommu
> always return zero if the given device is successfully attached to
> the given iommu_domain even though it is already attached to the same
> iommu_domain.
>
> Signed-off-by: Cho KyongHo
Reviewed-by: Grant Grundler
cheers,
grant
> ---
> drivers/iommu/exynos-iommu.c | 13 +++--
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 51e5b35..6eed6d6 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -882,15 +882,16 @@ static int exynos_iommu_attach_device(struct
> iommu_domain *domain,
>
> spin_unlock_irqrestore(&priv->lock, flags);
>
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(dev, "%s: Failed to attach IOMMU with pgtable %#lx\n",
> __func__, __pa(priv->pgtable));
> - else
> - dev_dbg(dev, "%s: Attached IOMMU with pgtable 0x%lx%s\n",
> - __func__, __pa(priv->pgtable),
> - (ret == 0) ? "" : ", again");
> + return ret;
> + }
>
> - return ret;
> + dev_dbg(dev, "%s: Attached IOMMU with pgtable 0x%lx%s\n",
> + __func__, __pa(priv->pgtable), (ret == 0) ? "" : ", again");
> +
> + return 0;
> }
>
> static void exynos_iommu_detach_device(struct iommu_domain *domain,
> --
> 1.7.2.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 08/12] iommu/exynos: remove prefetch buffer setting when enabling System MMU
On Fri, Jul 26, 2013 at 4:28 AM, Cho KyongHo wrote:
> Prefetch buffer must be handled accurately, exact range of a buffer,
> frame by frame manually. Otherwise, it may causes page fault or
> deadlock in System MMU.
> Thus this patch removes prefetch buffer setting when System MMU is
> initialized(enabled).
>
> Signed-off-by: Cho KyongHo
Reviewed-by: Grant Grundler
BTW, cscope doesn't find any callers of exynos_sysmmu_enable().
Want to submit another patch to remove it?
(Note I'm talking about exynos_sysmmu_enable() without "__" prefix).
cheers,
grant
> ---
> drivers/iommu/exynos-iommu.c | 32 +++-
> 1 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index cfc02ed..87f6bb7 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -80,6 +80,8 @@
> #define CTRL_BLOCK 0x7
> #define CTRL_DISABLE 0x0
>
> +#define CFG_FLPDCACHE (1 << 20) /* System MMU 3.2+ only */
> +
> #define REG_MMU_CTRL 0x000
> #define REG_MMU_CFG0x004
> #define REG_MMU_STATUS 0x008
> @@ -96,6 +98,9 @@
>
> #define REG_MMU_VERSION0x034
>
> +#define MMU_MAJ_VER(reg) (reg >> 28)
> +#define MMU_MIN_VER(reg) ((reg >> 21) & 0x7F)
> +
> #define REG_PB0_SADDR 0x04C
> #define REG_PB0_EADDR 0x050
> #define REG_PB1_SADDR 0x054
> @@ -200,6 +205,22 @@ static bool is_sysmmu_active(struct sysmmu_drvdata *data)
> return data->activations > 0;
> }
>
> +static unsigned int __sysmmu_version(struct sysmmu_drvdata *data,
> +int idx, unsigned int *minor)
> +{
> + unsigned long major;
> +
> + major = readl(data->sfrbases[idx] + REG_MMU_VERSION);
> +
> + if (minor)
> + *minor = MMU_MIN_VER(major);
> +
> + if (MMU_MAJ_VER(major) > 3)
> + return 1;
> +
> + return MMU_MAJ_VER(major);
> +}
> +
> static void sysmmu_unblock(void __iomem *sfrbase)
> {
> __raw_writel(CTRL_ENABLE, sfrbase + REG_MMU_CTRL);
> @@ -460,14 +481,15 @@ static int __exynos_sysmmu_enable(struct sysmmu_drvdata
> *data,
> data->pgtable = pgtable;
>
> for (i = 0; i < data->nsfrs; i++) {
> + unsigned int min;
> +
> __sysmmu_set_ptbase(data->sfrbases[i], pgtable);
>
> - if ((readl(data->sfrbases[i] + REG_MMU_VERSION) >> 28) == 3) {
> - /* System MMU version is 3.x */
> - __raw_writel((1 << 12) | (2 << 28),
> + if ((__sysmmu_version(data, i, &min) == 3) && (min > 1)) {
> + unsigned long cfg;
> + cfg = __raw_readl(data->sfrbases[i] + REG_MMU_CFG);
> + __raw_writel(cfg | CFG_FLPDCACHE,
> data->sfrbases[i] + REG_MMU_CFG);
> - __sysmmu_set_prefbuf(data->sfrbases[i], 0, -1, 0);
> - __sysmmu_set_prefbuf(data->sfrbases[i], 0, -1, 1);
> }
>
> __raw_writel(CTRL_ENABLE, data->sfrbases[i] + REG_MMU_CTRL);
> --
> 1.7.2.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 06/12] ARM: dts: Add description of System MMU of Exynos SoCs
On Fri, Jul 26, 2013 at 4:28 AM, Cho KyongHo wrote:
> Signed-off-by: Cho KyongHo
> ---
> .../bindings/iommu/samsung,exynos4210-sysmmu.txt | 103 +++
> arch/arm/boot/dts/exynos4.dtsi | 122
> arch/arm/boot/dts/exynos4210.dtsi | 25 ++
> arch/arm/boot/dts/exynos4x12.dtsi | 76 +
> arch/arm/boot/dts/exynos5250.dtsi | 291
>
> 5 files changed, 617 insertions(+), 0 deletions(-)
> create mode 100644
> Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
>
> diff --git
> a/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> new file mode 100644
> index 000..92f0a33
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/samsung,exynos4210-sysmmu.txt
> @@ -0,0 +1,103 @@
> +Samsung Exynos4210 IOMMU H/W, System MMU (System Memory Management Unit)
> +
> +Samsung's Exynos architecture contains System MMU that enables scattered
Cho,
"MMU" should be plural? "MMUs"?
> +physical memory chunks visible as a contiguous region to DMA-capable
> peripheral
> +devices like MFC, FIMC, FIMD, GScaler, FIMC-IS and so forth.
> +
> +System MMU is a sort of IOMMU and support identical translation table format
> to
s/a sort of/an/ . Or perhaps "is also an". For the purposes of
documenting DMA support, we are talking about the IOMMU functionality
this device provides. It might be better to mention the functionality
the System MMU supports and refer to other subsystem documents (e,g,
clocking and power control) for details.
> +ARMv7 translation tables with minimum set of page properties including access
> +permissions, shareability and security protection. In addition, System MMU
> has
> +another capabilities like L2 TLB or block-fetch buffers to minimize
> translation
> +latency.
> +
> +A System MMU is dedicated to a single master peripheral device. Thus, it is
> +important to specify the correct System MMU in the device node of its master
> +device. Whereas a System MMU is dedicated to a master device, the master
> device
> +may have more than one System MMU.
Can I suggest rewriting the last two sentences to:
The master device node must correctly specify at least one
SystemMMU. A master device may have more than one System MMU.
BTW, is there a difference between "master device" and "master
peripheral device" that I'm not aware of? Perhaps use just one of
those expressions in this document, not both if they are the same
thing (which is what I assumed).
cheers,
grant
> +
> +Required properties:
> +- compatible: Should be "samsung,exynos4210-sysmmu"
> +- reg: A tuple of base address and size of System MMU registers.
> +- interrupt-parent: The phandle of the interrupt controller of System MMU
> +- interrupts: A tuple of numbers that indicates the interrupt source.
> +- clock-names: Should be "sysmmu" if the System MMU is needed to gate its
> clock.
> + Please refer to the following documents:
> + Documentation/devicetree/bindings/clock/clock-bindings.txt
> + Documentation/devicetree/bindings/clock/exynos4-clock.txt
> + Documentation/devicetree/bindings/clock/exynos5250-clock.txt
> + Optional "master" if the clock to the System MMU is gated by
> + another gate clock other than "sysmmu". The System MMU driver
> + sets "master" the parent of "sysmmu".
> + Exynos4 SoCs, there needs no "master" clocks.
> + Exynos5 SoCs, some System MMUs must have "master" clocks.
> +- clocks: Required if the System MMU is needed to gate its clock.
> + Please refer to the documents listed above.
> +- samsung,power-domain: Required if the System MMU is needed to gate its
> power.
> + Please refer to the following document:
> + Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> +
> +Required properties for the master peripheral devices:
> +- iommu: phandles to the System MMUs of the device
> +
> +Examples:
> +A System MMU is dedicated to a single master device.
> + gsc_0: gsc@0x13e0 {
> + compatible = "samsung,exynos5-gsc";
> + reg = <0x13e0 0x1000>;
> + interrupts = <0 85 0>;
> + samsung,power-domain = <&pd_gsc>;
> + clocks = <&clock 256>;
> + clock-names = "gscl";
> + iommu = <&sysmmu_gsc1>;
> + };
> +
> + sysmmu_gsc0: sysmmu@13E8 {
> + compatible = "samsung,exynos4210-sysmmu";
> + reg = <0x13E8 0x1000>;
> + interrupt-parent = <&combiner>;
> + interrupt-names = "sysmmu-gsc0";
> + interrupts = <2 0>;
> + clock-names = "sysmmu", "master";
> + clocks = <&clock 262>, <&clock 256>;
> + samsung,power-domain = <&p
Re: [PATCH 06/19] net/ethernet/dec/tulip/xircom_cb: Use module_pci_driver to register driver
[new gmail editor changed how to send plaint/text email - sorry for
multiple copies]
On Tue, May 21, 2013 at 3:42 PM, Peter Huewe wrote:
> Removing some boilerplate by using module_pci_driver instead of calling
> register and unregister in the otherwise empty init/exit functions.
>
> Signed-off-by: Peter Huewe
LGTM
Reviewed-by: Grant Grundler
thanks,
grant
> ---
> drivers/net/ethernet/dec/tulip/xircom_cb.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/xircom_cb.c
> b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> index cdbcd16..9b84cb0 100644
> --- a/drivers/net/ethernet/dec/tulip/xircom_cb.c
> +++ b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> @@ -1171,16 +1171,4 @@ investigate_write_descriptor(struct net_device *dev,
> }
> }
>
> -static int __init xircom_init(void)
> -{
> - return pci_register_driver(&xircom_ops);
> -}
> -
> -static void __exit xircom_exit(void)
> -{
> - pci_unregister_driver(&xircom_ops);
> -}
> -
> -module_init(xircom_init)
> -module_exit(xircom_exit)
> -
> +module_pci_driver(xircom_ops);
> --
> 1.8.1.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mmc: dw_mmc: rewrite CLKDIV computation
Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
But when debugging a related issue (http://crbug.com/221828) I found
the code unreadable. This rewrite simplifies the computation and
explains each step.
Signed-off-by: Grant Grundler
---
Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
I've written a test program to confirm this patch generates the
same correct values and will share that separately.
drivers/mmc/host/dw_mmc.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 9834221..6fdf55b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot,
bool force_clkinit)
if (slot->clock != host->current_speed || force_clkinit) {
div = host->bus_hz / slot->clock;
- if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
- /*
-* move the + 1 after the divide to prevent
-* over-clocking the card.
+ if (host->bus_hz > slot->clock) {
+ /* don't overclock due to integer math losses */
+ if ((div * slot->clock) < host->bus_hz)
+ div++;
+
+ /* don't overclock due to resolution of HW */
+ if (div & 1)
+ div++;
+
+ /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
+* Look for dwc_mobile_storage_db.pdf from Synopsys.
+* CLKDIV value 0 means divisor 1, val 1 -> 2, ...
*/
- div += 1;
-
- div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
+ div /= 2;
+ } else
+ div = 0;/* use bus_hz */
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual
%dHZ"
--
1.8.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation
I've attached the test program I wrote to compare the different
flavors of CLKDIV computation: old (3.4 kernel), current upstream, and
my rewrite.
thanks
grant
On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler wrote:
> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
> But when debugging a related issue (http://crbug.com/221828) I found
> the code unreadable. This rewrite simplifies the computation and
> explains each step.
>
> Signed-off-by: Grant Grundler
> ---
> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>
> I've written a test program to confirm this patch generates the
> same correct values and will share that separately.
>
> drivers/mmc/host/dw_mmc.c | 22 +++---
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 9834221..6fdf55b 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot,
> bool force_clkinit)
>
> if (slot->clock != host->current_speed || force_clkinit) {
> div = host->bus_hz / slot->clock;
> - if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
> - /*
> -* move the + 1 after the divide to prevent
> -* over-clocking the card.
> + if (host->bus_hz > slot->clock) {
> + /* don't overclock due to integer math losses */
> + if ((div * slot->clock) < host->bus_hz)
> + div++;
> +
> + /* don't overclock due to resolution of HW */
> + if (div & 1)
> + div++;
> +
> + /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
> +* Look for dwc_mobile_storage_db.pdf from Synopsys.
> +* CLKDIV value 0 means divisor 1, val 1 -> 2, ...
> */
> - div += 1;
> -
> - div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) :
> 0;
> + div /= 2;
> + } else
> + div = 0;/* use bus_hz */
>
> dev_info(&slot->mmc->class_dev,
> "Bus speed (slot %d) = %dHz (slot req %dHz, actual
> %dHZ"
> --
> 1.8.1.3
>
#include
/* from include/linux/kernel.h */
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
unsigned int original(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
if (bus_hz % clock)
/*
* move the + 1 after the divide to prevent
* over-clocking the card.
*/
div = ((bus_hz / clock) >> 1) + 1;
else
div = (bus_hz / clock) >> 1;
return div;
}
unsigned int upstream(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
div = bus_hz / clock;
if (bus_hz % clock && bus_hz > clock)
/*
* move the + 1 after the divide to prevent
* over-clocking the card.
*/
div += 1;
div = (bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
return div;
}
unsigned int ggg(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
div = bus_hz / clock;
if (bus_hz > clock) {
/* don't overclock due to integer math losses */
if ((div * clock) < bus_hz)
div++;
/* don't overclock due to resolution of HW */
if (div & 1)
div++;
/* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
* Look for dwc_mobile_storage_db.pdf from Synopsys.
* CLKDIV value 0 means divisor 1, value 1 -> 2, val 2 -> 4 etc.
*/
div /= 2;
} else
div = 0; /* use bus_hz */
return div;
}
unsigned int bus_hz_tbl[] = { 13, 17, 19, 20, 5000, 1};
unsigned int slot_hz_tbl[] = { 10, 13, 21, 40, 57, 40, 784314, 5200};
static unsigned int div_to_hz(unsigned int bus_hz, unsigned int d)
{
return d ? (bus_hz/(d*2)) : bus_hz;
}
static void verify_hz(unsigned int bus_hz, unsigned int requested_hz,
unsigned int divided_hz, const char *algo_name)
{
if (divided_hz > bus_hz)
printf(" [FAIL: %s > bus hz!]", algo_name);
if (divided_hz > requested_hz)
printf(" [FAIL: %s > slot hz!]", algo_name);
}
void main(void)
{
unsigned int i, j;
printf("bus/slot hz Original Upstream GGG\n");
for(i=0; i < sizeof(bus_hz_tbl)/sizeof(unsigned int); i++) {
unsigned int bus_hz = bus_hz_tbl[i];
for (j=0; j < sizeof(slot_hz_tbl)/sizeof(unsigned int); j++) {
unsig
Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation
Sorry - please ignore the previous version. Did not include a
copyright (implied to be mine...but it's not) nor license.
Same thing but with proper attribution.
cheers,
grant
On Tue, Mar 26, 2013 at 3:53 PM, Grant Grundler wrote:
> I've attached the test program I wrote to compare the different
> flavors of CLKDIV computation: old (3.4 kernel), current upstream, and
> my rewrite.
>
> thanks
> grant
>
> On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> But when debugging a related issue (http://crbug.com/221828) I found
>> the code unreadable. This rewrite simplifies the computation and
>> explains each step.
>>
>> Signed-off-by: Grant Grundler
>> ---
>> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
>> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>>
>> I've written a test program to confirm this patch generates the
>> same correct values and will share that separately.
>>
>> drivers/mmc/host/dw_mmc.c | 22 +++---
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 9834221..6fdf55b 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot,
>> bool force_clkinit)
>>
>> if (slot->clock != host->current_speed || force_clkinit) {
>> div = host->bus_hz / slot->clock;
>> - if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
>> - /*
>> -* move the + 1 after the divide to prevent
>> -* over-clocking the card.
>> + if (host->bus_hz > slot->clock) {
>> + /* don't overclock due to integer math losses */
>> + if ((div * slot->clock) < host->bus_hz)
>> + div++;
>> +
>> + /* don't overclock due to resolution of HW */
>> + if (div & 1)
>> + div++;
>> +
>> + /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> +* Look for dwc_mobile_storage_db.pdf from Synopsys.
>> +* CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>> */
>> - div += 1;
>> -
>> - div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) :
>> 0;
>> + div /= 2;
>> + } else
>> + div = 0;/* use bus_hz */
>>
>> dev_info(&slot->mmc->class_dev,
>> "Bus speed (slot %d) = %dHz (slot req %dHz, actual
>> %dHZ"
>> --
>> 1.8.1.3
>>
/* Test dw_mmc driver CLKDIV computation.
* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
#include
/* from include/linux/kernel.h */
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
unsigned int original(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
if (bus_hz % clock)
/*
* move the + 1 after the divide to prevent
* over-clocking the card.
*/
div = ((bus_hz / clock) >> 1) + 1;
else
div = (bus_hz / clock) >> 1;
return div;
}
unsigned int upstream(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
div = bus_hz / clock;
if (bus_hz % clock && bus_hz > clock)
/*
* move the + 1 after the divide to prevent
* over-clocking the card.
*/
div += 1;
div = (bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
return div;
}
unsigned int ggg(unsigned int bus_hz, unsigned int clock)
{
unsigned int div;
div = bus_hz /
Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation
On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon wrote:
> On Wednesday, March 27, 2013, Grant Grundler wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
> For easily identifying, it would be good to point the commit id and subject.
commit id will be different for different git trees and branches - not
as useful as one might think.
I will include the following and update the patch:
Author: Seungwon Jeon
Date: Tue May 22 13:01:21 2012 +0900
mmc: dw_mmc: correct the calculation for CLKDIV
>> But when debugging a related issue (http://crbug.com/221828) I found
>
> It is not easy to catch up issue in your site. What problem is bothering you?
> Could you describe the problem and conditions you have found?
The bug is not relevant to this patch - it's a "related bug". I
mentioned the bug only to explain my motivation for looking at this
code. I will move the bug reference out of the commit message.
To summarize, I was trying to backport "mmc: dw_mmc: correct the
calculation for CLKDIV" patch to 3.4 kernel to support faster eMMC
parts since the original computation in 3.4 kernel was returning wrong
CLKDIV value. But then ran into other problems specific to one
firmware combination breaking the eMMC clock settings.
cheers,
grant
>
> Thanks,
> Seungwon Jeon
>
>> the code unreadable. This rewrite simplifies the computation and
>> explains each step.
>>
>> Signed-off-by: Grant Grundler
>> ---
>> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
>> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>>
>> I've written a test program to confirm this patch generates the
>> same correct values and will share that separately.
>>
>> drivers/mmc/host/dw_mmc.c | 22 +++---
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 9834221..6fdf55b 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot,
>> bool force_clkinit)
>>
>> if (slot->clock != host->current_speed || force_clkinit) {
>> div = host->bus_hz / slot->clock;
>> - if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
>> - /*
>> - * move the + 1 after the divide to prevent
>> - * over-clocking the card.
>> + if (host->bus_hz > slot->clock) {
>> + /* don't overclock due to integer math losses */
>> + if ((div * slot->clock) < host->bus_hz)
>> + div++;
>> +
>> + /* don't overclock due to resolution of HW */
>> + if (div & 1)
>> + div++;
>> +
>> + /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> + * Look for dwc_mobile_storage_db.pdf from Synopsys.
>> + * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>>*/
>> - div += 1;
>> -
>> - div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
>> + div /= 2;
>> + } else
>> + div = 0;/* use bus_hz */
>>
>> dev_info(&slot->mmc->class_dev,
>>"Bus speed (slot %d) = %dHz (slot req %dHz, actual
>> %dHZ"
>> --
>> 1.8.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation
On Wed, Mar 27, 2013 at 8:07 AM, Doug Anderson wrote: > Grant, > > Thanks for posting! See below... thanks for reviewing/feedback! :) > On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler wrote: >> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation. >> But when debugging a related issue (http://crbug.com/221828) I found >> the code unreadable. This rewrite simplifies the computation and >> explains each step. > > The fact that you mention a bug here is confusing. I think this patch > has nothing to do with fixing that bug and is just a readability > patch. Is that correct? Correct. "related" implies "not the same". But you are right - the reference is causing confusion. I will move the reference to the bug out of the commit log to below the '---' area of the patch. > Please add to the description if so and > maybe remove unrelated comment about the bug. Thanks! Will do and repost later today. ... >> + /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook" >> +* Look for dwc_mobile_storage_db.pdf from Synopsys. >> +* CLKDIV value 0 means divisor 1, val 1 -> 2, ... > > You are quoting exynos5250 docs here. This driver is used for more > than just exynos and so this could be confusing to users on other > platforms. I'm quoting Synopsys docs - that's the origin of this HW's ip. You and I looked at exynos5250 docs originally and they say exactly the same thing. But the section numbers are different. > >> */ >> - div += 1; >> - >> - div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : >> 0; >> + div /= 2; > > It does look like you're re-implementing DIV_ROUND_UP. Yes, it does look like that but by breaking it out into simple steps AND explaining why we do each step, the code becomes maintainable by normal developers. The comments are key to *quickly* understanding the code in this case. > Maybe replace your "if" test and division with just a DIV_ROUND_UP? No. I'd rather just drop the patch. This code can and should be stupid simple. DIV_ROUND_UP just makes it harder to understand and impossible to document as clearly. cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation
Hi Chris,
On Wed, Mar 27, 2013 at 10:58 AM, Chris Ball wrote:
> Hi Grant,
...
> Please use the following (standard) syntax in the commit message:
>
> Commit e419990b5e8 ("mmc: dw_mmc: correct the calculation for CLKDIV")
> fixed a bug in CLKDIV computation. [..]
Ok - I didn't know that was "standard". But easy enough to do.
cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2] mmc: dw_mmc: rewrite CLKDIV computation
When backporting Commit e419990b5e8 ("mmc: dw_mmc: correct the
calculation for CLKDIV") to 3.4 kernel and debugging
a FW issue, I found the code unreadable. This rewrite simplifies
the computation and explains each step.
Signed-off-by: Grant Grundler
---
V2: rewrote commit msg per feedback
Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
I've posted the test_dw_mmc.c source to confirm this patch generates
the same correct values.
The CLKDIV issue I was trying to resolve in ChromeOS 3.4 kernel:
http://crbug.com/221828
drivers/mmc/host/dw_mmc.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 9834221..6fdf55b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot,
bool force_clkinit)
if (slot->clock != host->current_speed || force_clkinit) {
div = host->bus_hz / slot->clock;
- if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
- /*
-* move the + 1 after the divide to prevent
-* over-clocking the card.
+ if (host->bus_hz > slot->clock) {
+ /* don't overclock due to integer math losses */
+ if ((div * slot->clock) < host->bus_hz)
+ div++;
+
+ /* don't overclock due to resolution of HW */
+ if (div & 1)
+ div++;
+
+ /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
+* Look for dwc_mobile_storage_db.pdf from Synopsys.
+* CLKDIV value 0 means divisor 1, val 1 -> 2, ...
*/
- div += 1;
-
- div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
+ div /= 2;
+ } else
+ div = 0;/* use bus_hz */
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual
%dHZ"
--
1.8.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x
On Wed, Jan 30, 2008 at 09:23:06PM +0100, Ondrej Zary wrote: > On Saturday 26 January 2008 21:58:10 Ondrej Zary wrote: > > Hello, > > I was having problems with these FreedomLine cards with Linux before but > > tested it thoroughly today. This card uses DEC 21041 chip and has TP and > > BNC connectors: > > > > 00:12.0 Ethernet controller [0200]: Digital Equipment Corporation DECchip > > 21041 [Tulip Pass 3] [1011:0014] (rev 21) > > > > > > de2104x driver was loaded automatically by udev and card seemed to work. > > Until I disconnected the TP cable and putting it back after a while. The > > driver then switched to (non-existing) AUI port and remained there. I tried > > to set media to TP using ethtool - and the whole kernel crashed because of > > BUG_ON(de_is_running(de)); > > in de_set_media(). Seems that the driver is unable to stop the DMA in > > de_stop_rxtx(). The BUG_ON() is probably fine normally. But the media selection sounds broken. It's possible to select the wrong media type with 21040 chip but shouldn't be possible with 21041. For 21040 support, see de21040_get_media_info(). But de21041_get_srom_info() is expected to determine which media types are supported from SEPROM "media blocks". My guess is that code is broken since it seems to work with de405 driver. If you care to work the difference, I'd be happy to make a patch to fix that up. Also, from code review, DE2104X driver still has a few places with potential PCI MMIO Write posting issues. Specifically, I was looking in de_stop_hw() and de_set_media(). Several other bits of code correctly flush MMIO writes: e.g. tulip_read_eeprom(). > > I commented out AUI detection in the driver - this time it switched to BNC > > after unplugging the cable and remained there. I also attempted to reset > > the chip when de_stop_rxtx failed but failed to do it. You'd have to basically hardcode only one media type and it's corresponding parameters. > > Then I found that there's de4x5 driver which supports the same cards as > > de2104x (and some other too) - and this one works fine! I can plug and > > unplug the cable and even change between TP and BNC ports just by > > unplugging one and plugging the other cable in. Unfortunately, this driver > > is blacklisted by default - at least in Slackware and Debian. ISTR there was a time when tulip would compete with de4x5 for devices. tulip is the preferred driver. That's clearly no longer the case and perhaps both distro's need to revisit this. > > The question is: why does de2104x exist? Does it work better with some > > hardware? de2104x is a "work in progress". That's why it's marked "EXPERIMENTAL" in the Kconfig file. > > BTW. Found that the problem exist at least since 2003: > > http://oss.sgi.com/archives/netdev/2003-08/msg00951.html > > Does the de2104x driver work correctly for anyone? No idea. I've only used tulip driver. thanks for the bug report, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: uli526x doesn't get link if no link when interface is set up
On Sat, Feb 02, 2008 at 03:56:38PM +0100, Santiago Garcia Mantinan wrote: > Hi! > > I've been experiencing problems with this (internal) card ever since I > bought this motherboard, lately I've been doing some tests and I found out > some things, maybe not enough to let us debug this, but I'll explain it just > in case. Santiago, Thanks for the excellent bug report. Is this perhaps the same problem as described here? http://bugzilla.kernel.org/show_bug.cgi?id=5839 It sounds similar but I only skimmed over your report. If you think it is, could you add (just cut/paste) you email to that bug for me? Could you also try the patch I attached to that bug report? If it works for you, we can both pester Kyle McMartin to push it upstream. ;) Thanks for the excellent bug report...I'll take a closer look at it later this week or weekend. thanks, grant > > The problem is that if the uli526x card is set up (ifconfig ethX up) when > there is no cable plugged or the cable is plugged to something that is off > at that time, this means, when there is no link for the card to detect, > then, when the cable or the switch or whatever is plugged, the link is not > detected by the uli526x card an thus no link is stablished (led on the > switch remains off). > > This is the status reported by ethtool when the link should be on but the > driver is not detecting this condition and thus it is off: > > Supported ports: [ MII ] > Supported link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Supports auto-negotiation: Yes > Advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Advertised auto-negotiation: Yes > Speed: Unknown! (65535) > Duplex: Unknown! (255) > Port: MII > PHYAD: 1 > Transceiver: external > Auto-negotiation: on > Supports Wake-on: pg > Wake-on: d > Link detected: no > > If at this time we run: "ifconfig ethX down" and after some time: "ifconfig > ethX up" then the link is detected, but if we run this two commands without > waiting a time between them, the link remains undetected. > > In fact, if with an stablished and detected link, we run: "ifconfig ethX > down;ifconfig ethX up" the link is lost again and is not detected till we > run the two commands waiting some time between them. > > Once the link is stablished if we don't touch the interface config (we don't > ifconfig it down) then we can unplug the cable or turn off the switch or > whatever and the card will detect the link whenever it becomes available > again. > > This makes me think that the problem is something related to the way on > which we are setting the card up. > > I'm running 2.6.24 on a amd64 machine, these are the messages I get from the > driver on load: > uli526x: ULi M5261/M5263 net driver, version 0.9.3 (2005-7-29) > ACPI: PCI Interrupt :00:12.0[A] -> GSI 20 (level, low) -> IRQ 20 > eth1: ULi M5263 at pci:00:12.0, 00:13:8f:a7:af:b4, irq 20. > uli526x: eth1 NIC Link is Up 100 Mbps Full duplex > > This is my lspci output: > 00:12.0 0200: 10b9:5263 (rev 60) > Subsystem: 1849:5263 > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr- > Stepping- SERR+ FastB2B- DisINTx- > Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- > SERR- Latency: 32 (5000ns min, 1ns max), Cache Line Size: 64 bytes > Interrupt: pin A routed to IRQ 20 > Region 0: I/O ports at c800 [size=256] > Region 1: Memory at dedefc00 (32-bit, non-prefetchable) [size=256] > Capabilities: [50] 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- > Kernel driver in use: uli526x > Kernel modules: uli526x > > I don't know what else to add but I offer myself to do all the wanted tests. > > Regards... > -- > Manty/BestiaTester -> http://manty.net > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: uli526x doesn't get link if no link when interface is set up
On Wed, Feb 06, 2008 at 07:44:47PM +0100, Santiago Garcia Mantinan wrote: > > Is this perhaps the same problem as described here? > > http://bugzilla.kernel.org/show_bug.cgi?id=5839 > > Seems right. > > > If you think it is, could you add (just cut/paste) > > you email to that bug for me? > > Done Thanks! > > Could you also try the patch I attached to that bug report? > > If it works for you, we can both pester Kyle McMartin to > > push it upstream. ;) > > Yes, I have tried it and seems to solve the problem for me as well. I'd > agree to push it. Excellent. I've asked kyle to push that upstream. He ACKd so it should go in pretty soon. thanks again, grant > > Regards... > -- > Manty/BestiaTester -> http://manty.net -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change pci_raw_ops to pci_raw_read/write
On Thu, Feb 07, 2008 at 11:36:18AM -0500, Tony Camuso wrote: > Arjan van de Ven wrote: >> On Thu, 07 Feb 2008 10:54:05 -0500 >> Tony Camuso <[EMAIL PROTECTED]> wrote: >>> Matthew, >>> >>> Perhaps I missed it, but did you address Yinghai's concerns? >>> >>> Yinghai Lu wrote: On Jan 28, 2008 7:03 PM, Matthew Wilcox <[EMAIL PROTECTED]> wrote: > -int pci_conf1_write(unsigned int seg, unsigned int bus, > +static int pci_conf1_write(unsigned int seg, unsigned int bus, >unsigned int devfn, int reg, int len, > u32 value) any reason to change pci_conf1_read/write to static? >> nothing should use these directly. So static is the right answer ;) > > Agreed. Thanks, Arjan. > > Matthew, > What about the ATA_RAM addition to Kconfig? Was it accidental, > or intended? If intended, how is it related? AFAICT, it looks accidental. I can't see how it's related. He should be back online next week and can answer for himself. hth, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Change pci_raw_ops to pci_raw_read/write
On Sun, Feb 10, 2008 at 07:51:22AM -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox <[EMAIL PROTECTED]>
> Date: Sun, 10 Feb 2008 09:45:28 -0500
> Subject: [PATCH] Change pci_raw_ops to pci_raw_read/write
...
> -static int
> -pci_read (struct pci_bus *bus, unsigned int devfn, int where, int size, u32
> *value)
> +static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 *value)
> {
> - return raw_pci_ops->read(pci_domain_nr(bus), bus->number,
> + return raw_pci_read(pci_domain_nr(bus), bus->number,
>devfn, where, size, value);
Willy,
Just wondering...why don't we just pass "struct bus*" through to the
raw_pci* ops?
My thinking is if a PCI bus controller or bridge is discovered, then we should
always create a matching "struct bus *".
Your patch looks fine to me but if you (and others) agree with the above,
I can make patch to change the internal interface. The pci_*_config API
needs to remain the same.
...
> --- a/arch/x86/kernel/quirks.c
> +++ b/arch/x86/kernel/quirks.c
> @@ -27,7 +27,7 @@ static void __devinit quirk_intel_irqbalance(struct pci_dev
> *dev)
> pci_write_config_byte(dev, 0xf4, config|0x2);
>
> /* read xTPR register */
> - raw_pci_ops->read(0, 0, 0x40, 0x4c, 2, &word);
> + raw_pci_read(0, 0, 0x40, 0x4c, 2, &word);
Why are we using raw_pci_read here instead of pci_read_config_dword()?
If the pci_write_config_byte() above works, then I expect the read
to work too.
To be clear, this is not a problem with this patch...rather a seperate
problem with the original code.
hth,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x
On Mon, Feb 18, 2008 at 05:40:42PM +0100, Ondrej Zary wrote: > On Monday 18 February 2008 04:21:11 Grant Grundler wrote: > > On Wed, Jan 30, 2008 at 09:23:06PM +0100, Ondrej Zary wrote: > > > On Saturday 26 January 2008 21:58:10 Ondrej Zary wrote: > > > > Hello, > > > > I was having problems with these FreedomLine cards with Linux before > > > > but tested it thoroughly today. This card uses DEC 21041 chip and has > > > > TP and BNC connectors: > > > > > > > > 00:12.0 Ethernet controller [0200]: Digital Equipment Corporation > > > > DECchip 21041 [Tulip Pass 3] [1011:0014] (rev 21) > > > > > > > > > > > > de2104x driver was loaded automatically by udev and card seemed to > > > > work. Until I disconnected the TP cable and putting it back after a > > > > while. The driver then switched to (non-existing) AUI port and remained > > > > there. I tried to set media to TP using ethtool - and the whole kernel > > > > crashed because of BUG_ON(de_is_running(de)); > > > > in de_set_media(). Seems that the driver is unable to stop the DMA in > > > > de_stop_rxtx(). > > > > The BUG_ON() is probably fine normally. But the media selection sounds > > broken. It's possible to select the wrong media type with 21040 chip but > > shouldn't be possible with 21041. For 21040 support, see > > de21040_get_media_info(). But de21041_get_srom_info() is expected to > > determine which media > > types are supported from SEPROM "media blocks". My guess is that code > > is broken since it seems to work with de405 driver. If you care to > > work the difference, I'd be happy to make a patch to fix that up. > > I don't think that BUG_ON() should be there. It should probably printk a > warning but certainly not crash the whole machine. Ah ok - I agree. I'll see if we can fail more gracfully there. If you have an idea already, please send me a patch. > > > > Then I found that there's de4x5 driver which supports the same cards as > > > > de2104x (and some other too) - and this one works fine! I can plug and > > > > unplug the cable and even change between TP and BNC ports just by > > > > unplugging one and plugging the other cable in. Unfortunately, this > > > > driver is blacklisted by default - at least in Slackware and Debian. > > > > ISTR there was a time when tulip would compete with de4x5 for devices. > > tulip is the preferred driver. That's clearly no longer the case > > and perhaps both distro's need to revisit this. > > de4x5 has no MODULE_DEVICE_TABLE for PCI devices anymore, so no conflicts. > That's probably good for cards that work with tulip driver but bad for mine > card and also probably for some other cards that (should) work with de2104x. > > > > > > > The question is: why does de2104x exist? Does it work better with some > > > > hardware? > > > > de2104x is a "work in progress". > > That's why it's marked "EXPERIMENTAL" in the Kconfig file. > > Great, it looks to be 6 years old and it's still experimental. Probably > because it never worked properly. Yeah, probably. > I think that de2104x driver should be removed (or at least its > MODULE_DEVICE_TABLE) and MODULE_DEVICE_TABLE with only 21040 and 21041 PCI > IDs added to de4x5. > > I can send a patch if this is acceptable. It's acceptable to me. Jeff? (jgarzik) thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 03/10] PCI: AMD SATA IDE mode quirk
On Mon, Feb 25, 2008 at 09:43:59AM +0800, Cai, Crane wrote:
> > On Fri, Feb 22, 2008 at 01:49:20PM +0800, Cai, Crane wrote:
> > > > On Thu, Feb 21, 2008 at 03:47:33PM -0800, Greg
> > Kroah-Hartman wrote:
> > > > > +static void __devinit quirk_amd_ide_mode(struct pci_dev *pdev)
> > > > > {
> > > > > - /* set sb600 sata to ahci mode */
> > > > > - if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
> > > > > - u8 tmp;
> > > > > + /* set sb600/sb700/sb800 sata to ahci mode */
> > > > > + u8 tmp;
> > > > >
> > > > > + pci_read_config_byte(pdev, PCI_CLASS_DEVICE, &tmp);
> > > > > + if (tmp == 0x01) {
> > > > > pci_read_config_byte(pdev, 0x40, &tmp);
> > > >
> > > > This seems like a dis-improvement. Why are we reading a
> > config byte
> > > > for something we already have in the pci_dev?
> > > > Why are we now checking against 0x01 instead of a
> > symbolic constant?
> > > > Why are we no longer checking that this is PCI_BASE_CLASS_STORAGE?
> > > It is a quirk. In pci_ids.h did have PCI_CLASS_STORAGE_IDE and
> > > PCI_BASE_CLASS_STORAGE, these can not represent the right
> > situation we
> > > want to check. 0x01 represents PCI_CLASS_STORAGE_IDE last 2
> > bit. Also
> > > because it is a quirk, I do not think we need to change
> > pci_ids.h. So
> > > 0x01 used.
> >
> > You haven't explained what is wrong with the original code:
> >
> > if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
> >
>
> When resume, this pdev->class is quirked, however BIOS has
> modified pci configuration too. Inconsistance occurs.
Can you update pdev->class from the quirk?
It would be consistent then.
That would leave the code as-is except it's re-reading the field
from config space.
hth,
grant
> > > > Nothing in the changelog entry suggests why we now need
> > FIXUP_RESUME
> > > > entries when we didn't before.
> > > >
> > > PCI configuration space will be changed by BIOS and then in
> > pci init
> > > and restore. So resume also needed.
> >
> > That information needed to be in the changelog.
>
> This info, is a normal info. If maintainer need us to added in source code. I
> preferred too.
> > --
> > Intel are signing my paycheques ... these opinions are still
> > mine "Bill, look, we understand that you're interested in
> > selling us this operating system, but compare it to ours. We
> > can't possibly take such a retrograde step."
> >
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x
On Mon, Feb 25, 2008 at 02:30:00AM -0500, Jeff Garzik wrote: > Grant Grundler wrote: >> On Mon, Feb 18, 2008 at 05:40:42PM +0100, Ondrej Zary wrote: >>> I think that de2104x driver should be removed (or at least its >>> MODULE_DEVICE_TABLE) and MODULE_DEVICE_TABLE with only 21040 and 21041 >>> PCI IDs added to de4x5. >>> >>> I can send a patch if this is acceptable. >> It's acceptable to me. Jeff? (jgarzik) > > NAK, sorry, for two reasons: > > 1) we don't delete otherwise clean, working drivers Just to be clear - he's not trying to remove the driver. He's just interested in making de4x5 the "default" for this set of boards by doctoring with the pci device ids each driver will claim. > simply because of a bug triggered by unplugging a cable. Ondrej would be happy to test any patches sent. Tracking this sort of bug down usually isn't trivial as the statement above implies. > 2) de4x5 needs to go away. Ok. I'd prefer to wait until someone demonstrates de2104x driver is a fully functional alternative for all the PCI Ids it claims. thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [parisc-linux] Re: OK, let's try cleaning up another nit. Is anyone paying attention?
"Eric S. Raymond" wrote:
> Here's what I have for you guys:
...
> CONFIG_DMB_TRAP: arch/parisc/kernel/sba_iommu.c
> CONFIG_FUNC_SIZE: arch/parisc/kernel/sba_iommu.c
>
> Would you please take these out of the CONFIG_ namespace? Changing the
> prefix to CONFIGURE would do nicely.
As willy noted, both mine. I'll remove or rename them rename them so
they aren't in the CONFIG_ name space. Probably s/CONFIG_/SBA_/ for
those two.
I'm going to submit a "wishlist" bug to our debian BTS
(bugs.parisc-linux.org) for "Data Memory Break Trap" support.
It's a damn good Hammer! :^)
(GDB will probably want to use this too)
I once had a working "Data Memory Break Trap" handler to catch other
parts of the kernel when they corrupted the IO Pdirs. Hooks in sba_ccio.c
helped mark which pages would trap and define which code was allowed to
touch the page. My implementation had issues and I never bothered to
re-implement as suggested by our parisc CPU god, John Marvin.
CONFIG_FUNC_SIZE is just a bad choice of name (asking for trouble).
One might consider this a bug that hasn't happened yet - thanks Eric!
#define CONFIG_FUNC_SIZE 4096 /* SBA configuration function reg set */
> CONFIG_KWDB: arch/parisc/Makefile arch/parisc/config.in arch/parisc/defconfig
>arch/parisc/kernel/entry.S arch/parisc/kernel/traps.c arch/parisc/mm/init.
> c
This ones actually mine too. It could be replaced with the SGI debugger
CONFIG option if/when that ever gets supported. The hooks will have to
be in the same place. I'm pretty sure now the HP KWBD team will never give me
permission to publish KWDB sources (they've had almost a year now).
I sorta almost had the damn thing working too...*sigh*.
Willy should do whatever he thinks is right in this case.
> CONFIG_PCI_LBA: arch/parisc/config.in arch/parisc/defconfig arch/parisc/kerne
> l/Makefile
...
> Looks like these need Configure.help entries.
That's mine too.
We've been lazy about documentation since the getting the code working
has been a higher priority. I think having them documented will be a
prerequisite to merging upstream (either to Alan Cox or Linus).
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] 2.4.2-pre3: parport_pc init_module bug
Philipp Rumpf wrote:
> Jeff Garzik wrote:
> > Looks ok, but I wonder if we should include this list in the docs.
> > These is stuff defined by the PCI spec, and this list could potentially
> > get longer... (opinions either way wanted...)
Having people look things up in the spec isn't very user friendly.
Finding a copy of the PCI 2.1 or 2.2 spec I could pass on to others
(outside of HP) was a problem last year. The best I could do then
(legally) was point them to "PCI Systems Architecture" published
by MindShare.
> I'm not sure whether the
> plan is to have drivers handle MSIs or do it in the generic PCI code.
> Grant ?
Generic PCI code can d very little by itself with MSI since not all
platforms provide support for it - even within the same arch.
Support for MSI is very platform specific.
Eg for parisc: I expect everything but V-class to support MSI.
There are some subtle differences between transaction based
interrupts used by parisc CPU and MSI. But I don't recall what
they are at the moment - I'd have to look it up again.
I thought any x86 with SAPIC (not sure about APIC) could support
MSI as well. But I don't know the x86 arch nearly as well.
It's also possible for the driver to just ignore MSI and not use it.
ie use regular PCI IRQ lines for interrupts.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] 2.4.2-pre3: parport_pc init_module bug
Philipp Rumpf wrote:
> On Wed, 14 Feb 2001, Grant Grundler wrote:
> > Having people look things up in the spec isn't very user friendly.
>
> Having the constants in some well-known header file should be sufficient,
> shouldn't it ?
I would hope anyone bothering to include the constants in a document would
spend a few minutes explaining them as well. Perhaps a bad assumption
on my part...
> It depends on the platform and maybe the exact PCI slot used, but I don't
> think it depends on the driver (unless MSI support is broken in which case
> you would want to fix it up in the driver).
correct.
> At least I can't find
> anything in the PCI 2.2 spec that would suggest we need to consult the
> driver before enabling MSIs with one message only.
I don't know how BIOS's treat this (if at all). Need to know this first.
If they manage this resource and pre-assign everything, ok.
That's how it goes.
But if generic PCI manages this, I prefer to avoid allocating resources
that may not get used. The host platform may have a limited pool of
usable MSI data values (think parisc EIRR bits) and some cards may want
to use more than one MSI.
grant
ps. seems this thread has gotten a bit far off from the original subject. :^/
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: The IO problem on multiple PCI busses
Benjamin Herrenschmidt wrote:
> Hi Grant !
>
> Alan Cox suggested I contact you about this. I'm trying to figure out a
> way to cleanly resolve the problem of doing IO accesses on machines with
> multiple PCI host bridges (and multiple IO bases when IO cycles are not
> generated by the CPU). I'd be glad if you could catch on the
> "The IO problem on multiple PCI busses" thread on linux-kernel list
> and let us share your point of viw.
To l-k, Benjamin wrote:
| I've looked at the parisc code (thanks Alan for pointing that out), and
| it seem they implement all inb/outb as quite big functions that decypher
| the address, retreive the bus, and do the proper IO call. Unfortunately,
| that's a bit bloated, and I don't think I'll ever get other PPC
| maintainers to agree with such a mecanism (everybody seem to be quite
| concerned with IO speed, I admit including me).
Benjamin,
As the main author/maintainer of that code, let me explain why
it's so ugly. Hopefully this will give you insight into a "better"
(arch independent) solution. Apologies for the length.
For IO Port space, I didn't worry about the bloat. A nice side effect of
this bloat is it will discourage use of I/O Port space. That's good for
everyone, AFAICT. (I know some devices *only* support I/O port space and
I personnally don't care about them. If someone who does care about one
wants to talk to me about it...fine...I'll help)
[ Caveat: I've simplified the following *alot* to keep it short. ]
parisc supports two different PCI host bus adapters with each having
variants that behave differently. All work under the model we are using
with one binary. One kernel binary is important since we want to make
install's easy for users.
Under Dino (GSCtoPCI), each PCI HBA has it's own 64K I/O port space.
I/O port space transactions are generated by poking registers on Dino.
Yes - performance sucks - that's why HPUX (almost) exclusively
uses devices which support MMIO.
Under Elroy (aka LBA or RopesToPCI), we have two methods of accessing
I/O port space. One view of I/O space can be shared across all Elroy's
which share the same IOMMU (aka SBA). This method distributes the 64K
I/O space over the 8 (or 16) "ropes" with rope 0 getting the first
8k (or 4k) and so on. The other view is each LBA has it's own 64K
of I/O port space. The second view is mapped above 4GB and requires
64-bit kernel to access. In both cases, processor loads/stores from/to
the region will generate an I/O cycle on the respective PCI bus.
Generally speaking, parisc doesn't support VGA or ISA legacy crud on
it's PCI busses. But I think those are orthogonal issues.
The inb/outb support hings on this definition in include/asm-parisc/pci.h:
struct pci_port_ops {
u8 (*inb) (struct pci_hba_data *hba, u16 port);
u16 (*inw) (struct pci_hba_data *hba, u16 port);
u32 (*inl) (struct pci_hba_data *hba, u16 port);
void (*outb) (struct pci_hba_data *hba, u16 port, u8 data);
void (*outw) (struct pci_hba_data *hba, u16 port, u16 data);
void (*outl) (struct pci_hba_data *hba, u16 port, u32 data);
};
Code which uses this is in arch/parisc/kernel/pci.c at:
http://puffin.external.hp.com/cvs/linux/arch/parisc/kernel/pci.c
(look for PCI_PORT_HBA usage)
In a nut shell, the HBA number is encoded in the upper 16-bits
of the 32-bit I/O port space address. The inb() *function* uses the
decoded HBA number to lookup the matching pci_port_ops function table
and pci_hba_data * to pass in. PCI fixup_bus() code virtualizes the
I/O port addresses found by the generic PCI bus walk. inb() is
function so drivers work under *all* parisc PCI HBAs with one binary.
This scheme allows us to support "PCI-like" busses as well.
Some parisc machines have both PCI and EISA slots which are completely
independent of each other. We'd like to keep the semantics of inb/outb
the same and support both at the same time. It might be possible
to do this by feeding the drivers different versions of inb/outb
definitions at compile time. But initial attempts to do this ran
into problems (which I don't remember the details of).
Last comment is regarding who *configures* the PCI devices. On legacy PDC
(parisc's "BIOS on steriods"), the PDC sets everything up but does
not enable everything (ie pci_enable_device will set bits in PCI_COMMAND
cfg register). On card-mode Dino, (GSC cards plugged in proprietary bus),
the firmware doesn't know *anything* about the PCI devices and the arch
support has to set everything up - PCI MMIO space is not currently
supported there. And new servers (like L2000 or A500) with "PAT PDC" only
initialize PCI devices for boot. OS has to initialize the rest.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To un
Re: IO issues vs. multiple busses
Benjamin, Here are my comments directly responding to your mail. Benjamin Herrenschmidt wrote: > Hi Grant ! > > My original mail is: > > Here's the return of an ld problem for which we really need a > solution asap since it's now biting us in real life configurations... > > So the problem happens when you have a machine with more than one PCI > host bridge. This is typically the case of all new Apple machines as they > have 3 host bridges in one chip (2 of them are relevant: the AGP and the > PCI). I don't think the problem exist on x86 machines with real IO > cycles, at least in that case, the problem is different. Large systems have problems with I/O port space and legacy devices. There just isn't enough I/O port space to support large configs and ISA aliasing and all the other crud. That's why Intel is (a) ditching all the legacy crap in IA64 and (b) strongly encouraging people to use MMIO space on PCI. > In order to generate IO cycles, the bridge provides us with a region in > CPU physical memory space (a 16Mb region in our case) that translates > accesses to IO cycles on the PCI bus. Our implementation of inb/outb > currently relies on the kernel ioremap'ing one of these regions (the PCI > one) and using the ioremap result as a base (offset) inside the inb/outb > functions. If you only support one type of bridge, you could avoid the indirect function call (which parisc-linux uses) and encode the access method directly in the inb/outb macros. Just note that processor speed is so much faster (and getter faster) than the ISA bus (and PCI-1X bus), that CPU overhead is mostly irrelevant to the cost of accessing IO port space. On older x86 boxes it is relevant. > So that mean that the current design won't allow access to IOs located on > any bus but the one we arbitrarily choose (the PCI bus). That's fine in > most case, until you decide to put a 3dfx or nvidia card in the AGP slot. > Those cards require some IO accesses to be done to the legacy VGA > addresses, and of course, our inb/outb functions can't do that. parisc-linux has solved exactly that problem. > Obviously, we can hack some driver specific thing that would use the > arch-specific code to retreive the proper io base address for a given > host bridge, but that's a hack. I'm looking for a solution that would > cleanly apply to all archs that may potentially face this problem. I don't believe such a solution exists which is "cleaner" than what parisc-linux does and meets the same objectives. Right now, it's important the install be easy in order to make it easy for people to migrate from HPUX to parisc-linux. :^) > The problem potentially exist also for any PCI card that has PCI IOs on > anything but the main PCI bus. > > One possibility is to limit our IO space to 64k per bus (to avoid > bloating) and then use a hacked ioremap to create a single virtually > contiguous kernel region that appends all those IO spaces together. > Accessing IOs on bus N would just be the matter of calculating an address > of the type 64k*N+offset and doing normal inb/outb on the result. This might work for other arches. I'm pretty sure it won't for parisc. Again, the issue is the IO port space access method varies by HBA. > The > arch PCI code could then properly fixup PCI IO resources for PCI drivers, > and we could add a function of the kind > > unsigned long pci_bus_io_offset(int busno); > > that would return the offset to add to inb/outb when accessing IOs on the > N'th PCI bus. Basically, parisc-linux does that but the arch support hides that from the device drivers. > If we want to go a bit further, and allow ISA drivers that don't have a > pci_dev structure to work on legacy devices on any bus, we could provide > a set of function of the type > > int isa_get_bus_count(); > unsigned long isa_get_bus_io_offset(int busno); > > and eventually > > int isa_bus_to_pci_bus(int isa_busno); > int pci_bus_to_isa_bus(int pci_busno); I don't like this either. Reserving bus 0 for E/ISA solves the problem. > I'm, of course open to any comments about this (in fact, I'd really like > some feedback). One thing is that we also need to find a way to pass > those infos to userland. Currently, we implement an arch-specific syscall > that allow to retreive the IO physical base of a given PCI bus. That may > be enough, but we may also want something that match more closely what we > do in the kernel. I agree with davem on this. But maybe for different reasons. The issue with exporting IO port regions is the access method. Access method varies by platform (for parisc arch) and I don't want to see user apps encoding the access method for specific
PATCH 2.4.0 parisc PCI support
Hi all,
This patch contains the support parisc-linux needs in PCI generic.
My patch is not as clean as I'd like - but it should work.
Please send changes/feedback directly to me.
Code in parisc-linux CVS (based on 2.4.0) does boot on my OB800
(133Mhz Pentium), C3000, and A500 with PCI-PCI bridge support
working. I'm quite certain PCI-PCI bridge configuration (ie BIOS
didn't configure the bridge) support was broken. I'm not able to
test on alpha though...alpha may want to see #ifdef __hppa__
around some of the code I've changed.
I think the plan is to update the arch/parisc support in the near
future so parisc builds actually work from linus' tree.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
Index: drivers/pci/Makefile
===
RCS file: /home/cvs/parisc/linux/drivers/pci/Makefile,v
retrieving revision 1.1.1.4
retrieving revision 1.6
diff -u -p -r1.1.1.4 -r1.6
--- Makefile2001/01/09 16:57:56 1.1.1.4
+++ Makefile2001/02/02 15:35:25 1.6
@@ -21,6 +21,7 @@ obj-$(CONFIG_PROC_FS) += proc.o
#
obj-$(CONFIG_ALPHA) += setup-bus.o setup-irq.o
obj-$(CONFIG_ARM) += setup-bus.o setup-irq.o
+obj-$(CONFIG_PARISC64) += setup-bus.o
ifndef CONFIG_X86
obj-y += syscall.o
Index: drivers/pci/pci.c
===
RCS file: /home/cvs/parisc/linux/drivers/pci/pci.c,v
retrieving revision 1.1.1.6
diff -u -p -r1.1.1.6 pci.c
--- pci.c 2001/01/09 16:57:56 1.1.1.6
+++ pci.c 2001/03/02 18:44:59
@@ -615,6 +615,7 @@ static void pci_read_bases(struct pci_de
}
}
+
void __init pci_read_bridge_bases(struct pci_bus *child)
{
struct pci_dev *dev = child->self;
@@ -628,7 +629,7 @@ void __init pci_read_bridge_bases(struct
if (!dev) /* It's a host bus, nothing to read */
return;
- for(i=0; i<3; i++)
+ for(i=0; i<4; i++)
child->resource[i] = &dev->resource[PCI_BRIDGE_RESOURCES+i];
res = child->resource[0];
@@ -644,12 +645,16 @@ void __init pci_read_bridge_bases(struct
res->end = limit + 0xfff;
res->name = child->name;
} else {
+
/*
-* Ugh. We don't know enough about this bridge. Just assume
-* that it's entirely transparent.
+* Either this is not a PCI-PCI bridge or it's not
+* configured yet. Since this code only supports PCI-PCI
+* bridge, we better not be called for any other type.
+* Don't muck the resources since it will confuse the
+* platform specific code which does that.
*/
- printk("Unknown bridge resource %d: assuming transparent\n", 0);
- child->resource[0] = child->parent->resource[0];
+ printk("PCI : ignoring %s PCI-PCI bridge (I/O BASE not configured)\n",
+child->self->slot_name);
+ return;
}
res = child->resource[1];
@@ -664,8 +669,8 @@ void __init pci_read_bridge_bases(struct
res->name = child->name;
} else {
/* See comment above. Same thing */
- printk("Unknown bridge resource %d: assuming transparent\n", 1);
- child->resource[1] = child->parent->resource[1];
+ printk("PCI : ignoring %s PCI-PCI bridge (MMIO base not
+configured)\n", child->self->slot_name);
+ return;
}
res = child->resource[2];
@@ -690,11 +695,10 @@ void __init pci_read_bridge_bases(struct
res->end = limit + 0xf;
res->name = child->name;
} else {
- /* See comments above */
- printk("Unknown bridge resource %d: assuming transparent\n", 2);
- child->resource[2] = child->parent->resource[2];
+ /* Base > limit means the prefetchable mem is disabled.*/
}
}
+
static struct pci_bus * __init pci_alloc_bus(void)
{
Index: drivers/pci/setup-bus.c
===
RCS file: /home/cvs/parisc/linux/drivers/pci/setup-bus.c,v
retrieving revision 1.1.1.2
retrieving revision 1.5
diff -u -p -r1.1.1.2 -r1.5
--- setup-bus.c 2001/01/09 16:57:56 1.1.1.2
+++ setup-bus.c 2001/02/22 01:11:47 1.5
@@ -23,7 +23,7 @@
#include
-#define DEBUG_CONFIG 1
+#define DEBUG_CONFIG 0
#if DEBUG_CONFIG
# define DBGC(args) printk args
#else
@@ -32,6 +32,7 @@
#define ROUND_UP(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
static int __init
pbus_assign_resources_sorted(struct pci_bus *bus,
struct pbus_set_ranges_data *ranges)
@@ -4
Re: The IO problem on multiple PCI busses
"David S. Miller" wrote:
> There is another case you are ignoring. Some devices support memory
> space as well as I/O space, but only operate reliably when their
> I/O space window is used to access it.
ok. Those also fall into the category of "I personally don't care" :^)
> It just sounds to me like the hppa pci controllers are crap,
> especially the GSC one.
In defense of the HW designers, Dino operates extremely well
in the environment it was designed for. Principally, workstations
with HP graphics cards (which only use MMIO). Optimizations for
graphics make it one of the fastest PCI-1X (and Cujo is PCI-2X)
HBA's - that's according to a 3rd party graphics card vendor who
has ported to the major high-end platforms.
> At least the rope one does something
> reasonable when you have a 64-bit kernel. The horrors you've told me
> about the IOMMUs and stream-caches on these chips further confirms my
> theory :-)
Yup. *sigh*. Between chip bugs, tradeoffs of performance, time to market,
and simple programming interface, things got pretty ugly (its the
old saying about "Pick any two").
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0 parisc PCI support
Jeff Garzik wrote:
> IIRC these "assuming transparent" lines were put in to -fix- PCI-PCI
> bridges on at least some x86 boxes... I didn't really understand the
> bridge code well enough at the time to comment one way or the other on
> its correctness, but it definitely fixed some problems.
Jeff,
If someone could clarify, I'd be happy to rework/resubmit the patch.
My gut feeling is it was to support something other than a PCI-PCI bridge.
pci_read_bridge_bases() assumes the device is a PCI-PCI Bridge (layout
and interpretation of the window registers). Either the code needs to
be more explicit about the type of bridge being handled or the caller
(arch specific code) should.
Only x86 and parisc PCI support call this code in my 2.4.0 tree.
Maybe the right answer is the "assuming transperent" support in
pci_read_bridge_bases() move to arch/x86.
I'm pretty sure Alpha and parisc/PAT_PDC systems don't use this
code since the registers programmed in pci_setup_bridge().
This makes me think none of the other arches attempt to
support PCI-PCI bridges. Is that correct?
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0 parisc PCI support
Jeff Garzik wrote:
> The patch worked 100% on my laptop, but failed to allocate a PCI memory
> region on my desktop machine. Two attachments... "diff -u" output for
> dmesg before and after your patch, and "diff -u" output for lspci before
> and after your patch.
Jeff,
Thanks for trying. I'll rework and resubmit later.
Can you send me the complete lspci output of your desktop?
(either with or without the patch)
I'd like to pull whatever docs I can find on the offending bridge.
I'll also look at moving "transparent Bridge" support to x86
pci_fixup_bus() code (and see if I can find a machine locally
which has this same "feature").
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: IO issues vs. multiple busses
Benjamin Herrenschmidt wrote:
...
> The reason why I'm getting this problem on the public place (again ?)
> is that we are now faced with people who want to put video cards in both
> AGP & PCI busses, those cards requiring accesses to some legacy VGA IOs
> on each of their busses.
I don't see any way of getting around virtualizing the IO port space.
(by that I mean encoding more info in the upper IO port address bits)
...
> Right. That's my opinion too. But it's difficult to make everybody agree
> on it ;) Even the simple mechanism Paul Mackerras did so that IOs to
> non-existent devices don't kill the kernel (very small overhead) caused
> some barking ;)
Well, make it a CONFIG_XXX option for your arch and the people who
insist on doing complicated things will have to live with complicated
solutions. I don't have a better idea.
> It's my understanding that you use high
> bits of the IO address to store the HBA number and then use that to call
> the proper access functions.
Correct. For some reason, I thought alpha (or sparc? hose number?) did this
as well.
> That would solve the PCI IO problem (PCI cards
> requiring IOs to BAR-mapped regions), but I don't see how it can fix the
> problem of a card accessing legacy VGA addresses, except if you hand-fixed
> the video drivers to fill those high bits before doing IOs.
That's right. That happens after the bus walk but before drivers see
the device in pci_fixup_bus().
> If I understand things correctly, that mean that each card, instead of
> accessing the legacy VGA port 0xpp, would instead access 0x00bb00pp
> (or whatever mangling you use to stuff the HBA number).
right.
> The question is then to decide is all ISA busses are on a matching PCI bus,
> in which case a simple unsigned pci_get_bus_io_base(int bus_no) -like functio
> n
> would be enough, or if we want a scheme that supports other ISA-like busses ?
I don't know enough about your arch to answer that.
> We could eventually decide to support only PCI, and additionally declare a
> fake PCI bus for an ISA bus not matched to a PCI bus, whose config ops would
> return no device in any slot.
> Do we agree on this ?
In theory yes. But davem already wrote:
| There is no 'fake' ISA bus number you need. There is a 'real' one,
| the one on which the PCI-->ISA bridge lives, why not use that one
| :-)
> Well, from the driver point of view, I think it _do_ exist. Basically, the
> driver will do inb/outb & friends. Whatever those function do in reality is
> arch-dependant.
Right. I meant the underlying implementation of inb/outb.
> But we agree on the fact that in order for those functions to know on which
> bus to tap, an additional information must be "cooked" inside the IO address
> passed to them.
yes.
> Additionally, the same problem is true for ISA memory, when it exist
> obviously.
Really? I expected ISA memory to look like reguler uncacheable memory
and the drivers would simply dereference the address. But I'm not an
expert on how ISA busses work...
> With those two simple functions, we could at least
[ deleted list ]
I have to defer to someone like Alan Cox or Davem or someone who has
a clue about VGA. I don't. Like sparc, parisc doesn't support VGA.
Alan is the only person I know of who's even considered plugging a VGA
card into a parisc box.
> The only thing that's annoying me in the fact that we keep tied to PCI
> is that in various embedded system, there is one (or more) ISA-like
> bus hanging around with legacy devices and no PCI, and abstracting a
In short, where the HW doesn't route transactions down the right bus
adapter, the SW has too.
> bit the notion of IO bus would have helped. But it seem that now, more
> embedded systems are also going toward in-CPU IOs anyway, along with
> eventually a PCI bus for the most expensive ones, so it may finally
> not be an issue in the long term.
It sounds like the HW will do (some of?) the routing.
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0 parisc PCI support
Ivan Kokshaysky wrote: > On Fri, Mar 02, 2001 at 11:32:35AM -0800, Grant Grundler wrote: > > Code in parisc-linux CVS (based on 2.4.0) does boot on my OB800 > > (133Mhz Pentium), C3000, and A500 with PCI-PCI bridge support > > working. I'm quite certain PCI-PCI bridge configuration (ie BIOS > > didn't configure the bridge) support was broken. > > I believe it isn't. ;-) It works on various alphas including > configurations with chained PCI-PCI bridges. Ok. I overlooked the changes in arch/alpha/kernel/pci.c:pcibios_fixup_bus(). (As you know, things changed alot between -test10 and -test12) > Some comments on the patch: > > > +** If I/O or MEM ranges are overlapping, that's a BIOS bug. > > No. As we reallocate everything, it is quite possible that we'll > have temporary overlaps during setup with resources allocated > by BIOS. I'm not sure if it is harmful though. The other part of the comment I added was: +** Disabling *all* devices is bad. Console, root, etc get +** disabled this way. I can't debug with *all* devices disabled. Normally, the whole point of resource mgt is to (a) avoid overlaps and (b) reflect the state of the HW. I thought the arch specific code was responsible for setting the HW state and resource mgt state congruent. If the arch/alpha code wants everything reallocated anyway, why not have the arch code disable the HW during the bus walk in pcibios_fixup_bus() before calling pci_assign_unassigned_resources()? (I'm looking at 2.4.0 linux/arch/alpha/kernel/pci.c:common_init_pci() ) FYI, under PDC PAT (eg A500), unused devices are left in the "power on" state (which AFAIK, implies disabled). > > +#ifdef __hppa__ > > +/* XXX FIXME > > +** PCI_BRIDGE_CONTROL and PCI_COMMAND programming need to be revisited > > +** to support FBB. Make all this crud "configurable" by the arch specific > > +** (ie "PCI BIOS") support and the ifdef __hppa__ crap can go away then. > > +*/ > > Agreed. Something like pcibios_set_bridge_control(). possibly...I have another problem I wanted to solve - FBB support. I think generic Fast Back-Back support wants a new field in struct pci_bus (u32 bridge_ctl) to save and manage the FBB state (and SERR/PERR). Arch support would need a way to initialize bridge_ctl *before* pci_do_scan_bus() is called to indicate FBB is or is not supported by the parent PCI bus controller (either HBA or PCI-PCI Bridge). Originally I was thinking of seperating the "root" bus allocation from pci_scan_bus(). But calling pcibios_set_bridge_control() before the bus walk would work too if it passes struct pci_bus * as the parameter. And that could allow arch specific control over SERR/PERR bits as well. In pcibios_fixup_bus(), the arch code could check FBB state to see if it should be enabled on that HBA or not. Ideally, generic code would fully handle FBB for PCI-PCI secondary busses. Perhaps the FBB test could be in pci_setup_bridge() but I'm not sure if that would work for all arches (ie not sure off hand which uses pci_setup_bridge()). [ deleted code changes in drivers/pci/setup-bus.c:pbus_assign_resources() driver/pci/setup-res.c:pdev_sort_resources() ] > This change totally breaks PCI allocation logic. > Probably you assign PCI-PCI bridge windows in arch specific code - why? I think my change in pdev_sort_resources() permitted it to occur in generic code. parisc HBA code only calls request_resources for resources assigned by firmware to the HBA. > The only thing you need is to set up the root bus resources > properly and generic code will do the rest. hmmm...Code in alpha's pcibios_fixup_bus() modifies PCI-PCI Bridge resources. It wouldn't if your statement were literally true. I reversed the two changes in my tree to see what breaks on A500: | lba version TR4.0 (0x5) found at 0xfed3c000 | lba_fixup_bus(0x18b4b780) bus 48 sysdata 0x18b4a800 | lba_fixup_bus() LBA I/O Port [3/3]/100 | lba_fixup_bus() LBA LMMIO [fb00/fb7f]/200 | lba_fixup_bus(0x18b4b880) bus 49 sysdata 0x18b4a800 | lba_fixup_bus(0x18b4b980) bus 50 sysdata 0x18b4a800 | PCI: Failed to allocate resource 0 for 31:04.0 | PCI: Failed to allocate resource 0 for 31:04.1 [ I have a 4-port 100BT card and a 2-port 100BT/896 SCSI "combo" card installed in bus 48 - both have PCI-PCI bridges. No resources are available for any devices under either PPB. ] ... | tulip: eth1: I/O region (0xfffd@0x31000) unavailable, aborting ... | sym53c896-6: rev 0x5 on pci bus 49 device 4 function 0 irq 320 | sym53c896-6: ID 7, Fast-40, Parity Checking | sym53c896-6: on-chip RAM at 0xfb10 | CACHE TEST FAILED: reg dstat-sstat2 readback . | CACHE INCORR
[PATCH] 2.2.18pre21 ide-disk.c for OB800
hi folks,
I've been playing with hdparms on the OB800 (running debian 2.2 -
that 2.2.18pre21 based) as shown on:
http://www.phys.unsw.edu.au/~mcba/hp800ct.html
The appended patch attemps to fix the following errors seen when the IDE
drive spins up after a sleep mode:
hda: multwrite_intr: status=0x51 { DriveReady SeekComplete Error }
hda: multwrite_intr: error=0x04 { DriveStatusError }
*or*
hda: read_intr: status=0x59 { DriveReady SeekComplete DataReady Error }
hda: read_intr: error=0x04 { DriveStatusError }
After several retries, both result in "ide0: reset: success" and life
is good again. The patch avoids the reset.
I have two theories on what's wrong: drive forgets multmode was enabled
*or* drive doesn't spin up media when poked. The web page above claims
the drive forgets the hdparm -S (idle time before sleep) and provides a
script to reset that. I've wondered if the same might be true for Multi-sector
mode. Anyway, resetting MultMode seems to fix the problem.
I've learned the drive will spin down in four different cases:
1) hdparm -S10 (50 seconds) and let the machine idle that long
2) BIOS APM has been idle and decides it time to sleep (I have
mine set to 3 minutes right now)
3) "on/off" button on keyboard
4) hdparm -Y /dev/hda (this case still generates similar errors.)
This patch does NOT fix another symptom:
o "irq timeout: status=0xd0 { Busy }" followed by "ide0: reset: success".
enjoy!
grant
grundler at puffin.external.hp.com
parisc-linux I/O hacker
--- kernel-source-2.2.18pre21-2.2.18pre21.ORIG/drivers/block/ide-disk.c Wed Jun 7
14:26:42 2000
+++ linux/drivers/block/ide-disk.c Wed Jan 10 15:03:15 2001
@@ -123,6 +123,23 @@ static int lba_capacity_is_ok (struct hd
}
/*
+ * set_multmode_intr() is invoked on completion of a WIN_SETMULT cmd.
+ */
+static ide_startstop_t set_multmode_intr (ide_drive_t *drive)
+{
+ byte stat = GET_STAT();
+
+ if (OK_STAT(stat,READY_STAT,BAD_STAT)) {
+ drive->mult_count = drive->mult_req;
+ } else {
+ drive->mult_req = drive->mult_count = 0;
+ drive->special.b.recalibrate = 1;
+ (void) ide_dump_status(drive, "set_multmode", stat);
+ }
+ return ide_stopped;
+}
+
+/*
* read_intr() is the handler for disk read/multread interrupts
*/
static ide_startstop_t read_intr (ide_drive_t *drive)
@@ -145,6 +162,32 @@ static ide_startstop_t read_intr (ide_dr
return ide_started;
}
#endif
+ else if ((0x59 == stat) && drive->mult_count) {
+ /*
+ ** HP OB800 laptop HD (IBM-DMCA-21440) will spin down
+ ** and *forget* multi-mode parms when it spins up on
+ ** the next access. The following fixes the stat 0x51
+ ** w/error 0x4 messages and reset after spinning up.
+ ** ggg 9.1.2001
+ **
+ ** If first request which triggers resume is a write,
+ ** stat == 0x51 (vs. 0x59 for read).
+ **
+ ** 1) Check if HD forgot.
+ ** 2) If so, set them again, otherwise report error.
+ ** 3) I/O is still queued - will get retried (I hope).
+ **
+ ** REVISIT: Don't know how to easily check HD settings.
+ **Not sure it's possible since it doesn't just seem to
+ **be a simple register read. Not checking HD settings
+ **risks an infinite loop/wedged system. I would expect
+ **to get a different error for the SETMULT cmd.
+ */
+ {
+ ide_cmd(drive, WIN_SETMULT, drive->mult_req,
+&set_multmode_intr);
+ return ide_started;
+ }
+ }
msect = drive->mult_count;
read_next:
@@ -331,24 +374,17 @@ static ide_startstop_t multwrite_intr (i
}
return ide_stopped; /* the original code did this here (?) */
}
- return ide_error(drive, "multwrite_intr", stat);
-}
-
-/*
- * set_multmode_intr() is invoked on completion of a WIN_SETMULT cmd.
- */
-static ide_startstop_t set_multmode_intr (ide_drive_t *drive)
-{
- byte stat = GET_STAT();
-
- if (OK_STAT(stat,READY_STAT,BAD_STAT)) {
- drive->mult_count = drive->mult_req;
- } else {
- drive->mult_req = drive->mult_count = 0;
- drive->special.b.recalibrate = 1;
- (void) ide_dump_status(drive, "set_multmode", stat);
+ else if (0x51 == stat) {
+ /*
+ ** HP OB800 laptop HD (IBM-DMCA-21440) fix.
+ ** See comments in read_intr().
+ */
+ {
+
Re: [PATCH] 2.2.18pre21 ide-disk.c for OB800
Andre, Alan, My grand total experience with IDE drivers is now around 4 hours. I have no clue what's right or wrong and am quite clueless what the role of apmd is wrt ide-disk driver. I'm open to testing other fixes for this problem. AFAIK, this could be a BIOS bug since no one else seems to have run into on other laptops and it's reproduce with two different makes of drives. The reason I put the fix in the read/multwrite_intr path is the recovery has to occur before the I/O is retried and before accessing the disk. If multmode can be set before even trying the I/O, then that's definitely a better solution. I just have no clue how to implement it. thanks, grant Grant Grundler Unix Systems Enablement Lab +1.408.447.7253 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
PATCH: remove dead code resource_fixup()
ooops...
--- Forwarded Message
<[EMAIL PROTECTED]>: host vger.rutgers.edu[128.6.14.121] said: 550
<[EMAIL PROTECTED]>... User unknown
Date: Fri, 3 Nov 2000 09:54:19 -0800 (PST)
From: Grant Grundler <[EMAIL PROTECTED]>
Message-Id: <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Subject: PATCH: remove dead code resource_fixup()
Cc: [EMAIL PROTECTED]
Hi Linus,
The following patch against linux-2.4.0-test10 removes resource_free() code.
"David S. Miller" <[EMAIL PROTECTED]> wrote:
| Yeah, it's dead code, feel free to send Linus a patch which kills
| it off :-)
thanks,
grant
diff -uNpr linux/arch/ppc/kernel/pci.c linux.patch/arch/ppc/kernel/pci.c
- --- linux/arch/ppc/kernel/pci.c Sun Sep 17 09:48:07 2000
+++ linux.patch/arch/ppc/kernel/pci.c Fri Nov 3 09:37:25 2000
@@ -344,11 +344,6 @@ pcibios_fixup_pbus_ranges(struct pci_bus
ranges->mem_end -= bus->resource[1]->start;
}
- -unsigned long resource_fixup(struct pci_dev * dev, struct resource * res,
- - unsigned long start, unsigned long size)
- -{
- - return start;
- -}
void __init pcibios_fixup_bus(struct pci_bus *bus)
{
diff -uNpr linux/arch/sparc/kernel/pcic.c linux.patch/arch/sparc/kernel/pcic.c
- --- linux/arch/sparc/kernel/pcic.cTue Oct 3 09:24:41 2000
+++ linux.patch/arch/sparc/kernel/pcic.cFri Nov 3 09:37:13 2000
@@ -866,23 +866,6 @@ void pcibios_update_resource(struct pci_
{
}
- -#if 0
- -void pcibios_update_irq(struct pci_dev *pdev, int irq)
- -{
- -}
- -
- -unsigned long resource_fixup(struct pci_dev *pdev, struct resource *res,
- - unsigned long start, unsigned long size)
- -{
- - return start;
- -}
- -
- -void pcibios_fixup_pbus_ranges(struct pci_bus *pbus,
- -struct pbus_set_ranges_data *pranges)
- -{
- -}
- -#endif
- -
void pcibios_align_resource(void *data, struct resource *res, unsigned long size)
{
}
diff -uNpr linux/arch/sparc64/kernel/pci.c linux.patch/arch/sparc64/kernel/pci.c
- --- linux/arch/sparc64/kernel/pci.c Tue Oct 10 10:33:51 2000
+++ linux.patch/arch/sparc64/kernel/pci.c Fri Nov 3 09:37:42 2000
@@ -202,12 +202,6 @@ void pcibios_update_irq(struct pci_dev *
{
}
- -unsigned long resource_fixup(struct pci_dev *pdev, struct resource *res,
- - unsigned long start, unsigned long size)
- -{
- - return start;
- -}
- -
void pcibios_fixup_pbus_ranges(struct pci_bus *pbus,
struct pbus_set_ranges_data *pranges)
{
--- End of Forwarded Message
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] move xchg/cmpxchg to atomic.h
On parisc-linux mailing list, Grant Grundler wrote:
> After surveying all the arches that define __HAVE_ARCH_CMPXCHG:
>
> ./include/asm-alpha/system.h:#define __HAVE_ARCH_CMPXCHG 1
> ./include/asm-i386/system.h:#define __HAVE_ARCH_CMPXCHG 1
> ./include/asm-ia64/system.h:#define __HAVE_ARCH_CMPXCHG 1
> ./include/asm-ppc/system.h:#define __HAVE_ARCH_CMPXCHG 1
> ./include/asm-sparc64/system.h:#define __HAVE_ARCH_CMPXCHG 1
>
> I've come to the conclusion xchg/cmpxchg definitions do NOT
> belong in system.h. AFAICT, all the above use Load Linked semantics
> (or in the i386 case, operation is atomic). In other words, xchg/cmpxchg
> are atomic operations. Shouldn't xchg/cmpxchg definitions live
> with other atomic operations - asm/atomic.h?
On Sat, 30 Dec 2000 16:46:57 + (GMT), Alan Cox replied:
| Seems a reasonable thing to try and move to atomic.h yes
Fundemental problem is parisc only supports one atomic operation
(LDCW/LDCD) and uses spinlocks for all atomic operations including
xchg/cmpxchg. Issue is dependencies between system.h, atomic.h
and spinlock.h are *really* ugly and prevented parisc port from
inlining xchg/cmpxchg definitions. This is a first step in fixing
that problem.
I've already made this change to the parisc-linux source tree for parisc
and parisc64 builds. Below is the i386 patch for linux-2.4.0-prerelease.
This is a simple cut/paste.
thanks,
grant
diff -ruNp linux/include/asm-i386/atomic.h linux.patch/include/asm-i386/atomic.h
--- linux/include/asm-i386/atomic.h Sun Dec 31 11:10:16 2000
+++ linux.patch/include/asm-i386/atomic.h Mon Jan 1 23:28:08 2001
@@ -2,6 +2,7 @@
#define __ARCH_I386_ATOMIC__
#include
+#include /* for LOCK_PREFIX */
/*
* Atomic operations that C can't guarantee us. Useful for
@@ -111,4 +112,136 @@ __asm__ __volatile__(LOCK "andl %0,%1" \
__asm__ __volatile__(LOCK "orl %0,%1" \
: : "r" (mask),"m" (*addr) : "memory")
+
+/* xchg/cmpxchg moved from asm/system.h */
+#define xchg(ptr,v) ((__typeof__(*(ptr)))__xchg((unsigned
+long)(v),(ptr),sizeof(*(ptr
+
+#define tas(ptr) (xchg((ptr),1))
+
+struct __xchg_dummy { unsigned long a[100]; };
+#define __xg(x) ((struct __xchg_dummy *)(x))
+
+
+/*
+ * The semantics of XCHGCMP8B are a bit strange, this is why
+ * there is a loop and the loading of %%eax and %%edx has to
+ * be inside. This inlines well in most cases, the cached
+ * cost is around ~38 cycles. (in the future we might want
+ * to do an SIMD/3DNOW!/MMX/FPU 64-bit store here, but that
+ * might have an implicit FPU-save as a cost, so it's not
+ * clear which path to go.)
+ */
+extern inline void __set_64bit (unsigned long long * ptr,
+ unsigned int low, unsigned int high)
+{
+__asm__ __volatile__ (
+ "1: movl (%0), %%eax;
+ movl 4(%0), %%edx;
+ cmpxchg8b (%0);
+ jnz 1b"
+ :: "D"(ptr),
+ "b"(low),
+ "c"(high)
+ :
+ "ax","dx","memory");
+}
+
+extern void inline __set_64bit_constant (unsigned long long *ptr,
+unsigned long long value)
+{
+ __set_64bit(ptr,(unsigned int)(value), (unsigned int)((value)>>32ULL));
+}
+#define ll_low(x) *(((unsigned int*)&(x))+0)
+#define ll_high(x) *(((unsigned int*)&(x))+1)
+
+extern void inline __set_64bit_var (unsigned long long *ptr,
+unsigned long long value)
+{
+ __set_64bit(ptr,ll_low(value), ll_high(value));
+}
+
+#define set_64bit(ptr,value) \
+(__builtin_constant_p(value) ? \
+ __set_64bit_constant(ptr, value) : \
+ __set_64bit_var(ptr, value) )
+
+#define _set_64bit(ptr,value) \
+(__builtin_constant_p(value) ? \
+ __set_64bit(ptr, (unsigned int)(value), (unsigned int)((value)>>32ULL) ) : \
+ __set_64bit(ptr, ll_low(value), ll_high(value)) )
+
+/*
+ * Note: no "lock" prefix even on SMP: xchg always implies lock anyway
+ * Note 2: xchg has side effect, so that attribute volatile is necessary,
+ * but generally the primitive is invalid, *ptr is output argument. --ANK
+ */
+static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int size)
+{
+ switch (size) {
+ case 1:
+ __asm__ __volatile__("xchgb %b0,%1"
+ :"=q" (x)
+ :"m" (*__xg(ptr)), "0" (x)
+ :"memory");
+ break;
+ case 2:
+ __asm__ __volatile__("xchgw %w0,%1"
+ :"=r" (x)
+ :"m" (*__xg(ptr)), "0"
Re: [PATCH] move xchg/cmpxchg to atomic.h
David,
Sorry for being dense - but I don't see the problem in using
a spinlock to implement xchg(). The example algorithm looks broken.
Or am I missing something obvious here?
"David S. Miller" wrote:
> It is very common to do things like:
>
> producer(elem)
> {
> elem->next = list->head;
> xchg(&list->head, elem);
> }
>
> consumer()
> {
> local_list = xchg(&list->head, NULL);
> for_each(elem, local_list)
> do_something(elem);
> }
producer() looks broken. The problem is two producers can race and
one will put the wrong value of list->head in elem->next.
I think prepending to list->head needs to either be protected by a spinlock
or be a per-cpu data structure. consumer() should be ok assuming the code
can tolerate picking up "late arrivals" in the next pass.
Or am I missing something obvious here?
It's worse if producer were inlined: the arch specific optimisers might
re-order the "elem->next = list->head" statement to be quite a bit more
than 1 or 2 cycles from the xchg() operation.
thanks,
grant
Grant Grundler
Unix Systems Enablement Lab
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0 parisc PCI support
b->resource[i]->name = bus->name;
> + }
> + b->resource[0]->flags |= pci_bridge_check_io(dev);
> + b->resource[1]->flags |= IORESOURCE_MEM;
> b->resource[0]->start = ranges->io_start = ranges->io_end;
> b->resource[1]->start = ranges->mem_start = ranges->mem_end;
> + b->resource[0]->end = bus->resource[0]->end;
> + b->resource[1]->end = bus->resource[1]->end;
> + /* Turn off downstream PF memory address range */
> + bus->resource[2]->start = 1024*1024;
> + bus->resource[2]->end = 0;
>
> pbus_assign_resources(b, ranges);
>
Yes, sort of. If my patch to pdev_sort_resources() makes sense now,
I'm not sure this is needed either.
My first reaction was initialization of b->resource pointer would have
to happen earlier in order to match arch code in pcibios_fixup_bus().
The idea being generic PCI code *in general* do the same things for
primary bus resources as secondary bus resources (ie window registers).
> > I would prefer this but it doesn't matter ATM; just needs to work.
>
> Certainly all this should be cleaned up in 2.5; I'm not sure for 2.4.
I don't think existing PCI code is very "dirty". Understanding
the interactions between generic and arch PCI code is non-trivial.
But it's not that bad. Understanding how various arches use the
code is the hard part.
parisc support is mostly in place in 2.4.0. It would be quite
frustrating to not have it fully working in a 2.4.x release because
of 10 or 20 lines of code change. FBB support will cause more change
than what I've proposed for in the patch parisc support.
thanks again,
grant
ps. Ivan - this has been a good exchange since it's forcing me to revisit
code I haven't looked at in a few monthes with a fresh perspective.
This (and my previous) reply took me about 4 hours to write.
I have to keep looking at code. :^)
>
> Ivan.
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: PATCH 2.4.0 parisc PCI support
Ivan Kokshaysky wrote:
...
> Well, it seems that I finally see what is wrong with your code, and
> why it worked in your case. You assume that "window" resources of
> the bridge are already known when we call pbus_assign_resources_sorted().
> This is incorrect.
Oh...do we know the "sizes" of all child resources from the bus walk?
I'll check that and see if it can be safely changed.
> Probably you rely on pci_read_bridge_bases() doing
> something meaningful (I looked at the parisc pci code in current 2.4.x,
> don't know about your CVS tree).
Nope - don't call that for A500 (machines with PDC PAT)...that might in
fact be another problem later related to some PDC (aka BIOS) changes.
> Yes, at least some of the DEC bridges
> after power-up/reset have 0s in base/limit registers. This means
> that you have ranges -0fff (4K) for IO and -000f (1M)
> for MEM. Obviously it's enough to hold all resources on the
> cards you've tested, but it won't work in common case. There is
> a lot of reasons why; just a couple of them:
> - according to PPB specification, base/limits registers of the bridge
> after reset are *undefined*, so you'll probably have troubles
> with non-DEC bridges.
> - there is a number of alpha systems with a built-in PCI-PCI bridge
> and real PCI slots behind it. Obviously 4K/1M isn't enough for
> these systems, and it was the main reason of rewriting that code.
> etc etc etc.
Yup - I think you are right on all counts here.
I'll rework the parisc code tonight/tomorrow and see if I can get rid
of the contentious generic PCI changes. I should be able to.
> Basically, you won't know bridge "window" size for a given bus until
> you'll have allocated *all* devices on *all* its child busses.
Linux doesn't. It's possible to deal with window register size in
the initial bus walk (where BAR sizes are determined).
> Besides, including bridge resources in the "sort lists" is meaningless,
> since these resources have fixed alignment - 4K for IO and 1M for MEM,
> unlike "regular" ones, which alignment == size.
The alignment would have to be handled correctly and I thought
pcibios_align_resource() did that. I see now the arch/parisc one doesn't
and others probably don't either. Let me think about this more...
> Unfortunately I haven't anything with a bridge handy at the moment
> to test that patch. Besides, we'll have here a sort of holidays till
> Sunday. So maybe next week...
np. thanks.
>
> > I don't think existing PCI code is very "dirty".
>
> I hope so. :-)
:^)
> However, some problems need to be worked out:
> 1. generic vs. arch code - we've already discussed some of these
> 2. Prefetchable Memory - do we need to deal with it? Though looking
>at modern x86 systems I tend to keep it disabled :-)
Ditto for parisc.
> 3. pdev_enable_device() - it's a bit ugly, confuses people and
>possibly is not needed at all.
Agreed.
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
2.4.0 pdev_enable_device() call in setup-bus.c
Hi Ivan,
Can you explain why pci_assign_unassigned_resources()
calls pdev_enable_device() for every PCI device instead
of having each PCI *driver* call pci_enable_device()
as part of driver initialization?
I'm thinking I missed something that a comment in the code
should have explained.
After having written the bulk of PCI support for parisc port,
I was clearly under the impression the PCI driver was
supposed to call pci_enable_device(). IMHO, it's a *bad* idea
to enable a device when it's driver might not be present.
thanks,
grant
Code from drivers/pci/setup-bus.c:
void __init
pci_assign_unassigned_resources(void)
{
...
pci_for_each_dev(dev) {
pdev_enable_device(dev);
}
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: 2.4.0 pdev_enable_device() call in setup-bus.c
Ivan Kokshaysky wrote:
> Mainly because there are driverless devices like display adapters,
> PCI bridges, or PCI devices with legacy drivers (IDE, for example).
Ok. It seems that all of those would have to interact with
the PCI code someplace. And that's were pci_enable_device()
could be called. eg. PCI Bridges could be handled in it's
"driver": pci_setup_bridge().
> OTOH, pdev_enable_device() most likely will be removed, but
> it's 2.5 material.
Agreed - standardizing the enable path would be good for above devices.
I'd like to see arch hooks added for stuff like default latency and
PCI_BRIDGE_CONTROL. My gut feeling is the "root" struct pci_bus
allocation and initialization should be done by arch specific code
*before* pci_scan_bus() is called. That would allow cfg space defaults
to be set to arch specific values on a per bus basis *before* doing
the bus walks instead of hacking this in pci_bus_fixup() later.
thanks,
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] add a clear_pages function to clear pages of higher order
On Tue, Apr 05, 2005 at 10:15:18PM -0700, Gerrit Huizenga wrote: > SpecSDET, Aim7 or ReAim from OSDL are probably what you are thinking of. SDET isn't publicly available. I hope by now osdl-reaim is called "osdl-aim7": http://lkml.org/lkml/2003/8/1/172 grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] parisc: kfree cleanup in arch/parisc/
On Mon, Apr 11, 2005 at 10:54:25PM +0200, Jesper Juhl wrote: > Get rid of redundant NULL pointer checks before kfree() in arch/parisc/ as > well as a few blank lines. > > Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]> thanks! Commited to cvs.parisc-linux.org: http://lists.parisc-linux.org/pipermail/parisc-linux-cvs/2005-April/035542.html I expect Matthew Wilcox will submit to linus with other parisc code changes in the next RC release. thanks, grant > > diff -upr linux-2.6.12-rc2-mm3-orig/arch/parisc/kernel/ioctl32.c > linux-2.6.12-rc2-mm3/arch/parisc/kernel/ioctl32.c > --- linux-2.6.12-rc2-mm3-orig/arch/parisc/kernel/ioctl32.c2005-04-05 > 21:21:08.0 +0200 > +++ linux-2.6.12-rc2-mm3/arch/parisc/kernel/ioctl32.c 2005-04-11 > 22:48:03.0 +0200 > @@ -104,12 +104,9 @@ static int drm32_version(unsigned int fd > } > > out: > - if (kversion.name) > - kfree(kversion.name); > - if (kversion.date) > - kfree(kversion.date); > - if (kversion.desc) > - kfree(kversion.desc); > + kfree(kversion.name); > + kfree(kversion.date); > + kfree(kversion.desc); > return ret; > } > > @@ -166,9 +163,7 @@ static int drm32_getsetunique(unsigned i > ret = -EFAULT; > } > > - if (karg.unique != NULL) > - kfree(karg.unique); > - > + kfree(karg.unique); > return ret; > } > > @@ -265,7 +260,6 @@ static int drm32_info_bufs(unsigned int > } > > kfree(karg.list); > - > return ret; > } > > @@ -305,7 +299,6 @@ static int drm32_free_bufs(unsigned int > > out: > kfree(karg.list); > - > return ret; > } > > @@ -494,15 +487,10 @@ static int drm32_dma(unsigned int fd, un > } > > out: > - if (karg.send_indices) > - kfree(karg.send_indices); > - if (karg.send_sizes) > - kfree(karg.send_sizes); > - if (karg.request_indices) > - kfree(karg.request_indices); > - if (karg.request_sizes) > - kfree(karg.request_sizes); > - > + kfree(karg.send_indices); > + kfree(karg.send_sizes); > + kfree(karg.request_indices); > + kfree(karg.request_sizes); > return ret; > } > > @@ -555,9 +543,7 @@ static int drm32_res_ctx(unsigned int fd > ret = -EFAULT; > } > > - if (karg.contexts) > - kfree(karg.contexts); > - > + kfree(karg.contexts); > return ret; > } > > > > > > PS. If you reply to lists other than Linux-kernel, then please keep me on CC: > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[BUG] 2.4.4 PCI /proc output
Hi, The /proc/bus/pci/devices data looks correct. /proc/bus/pci/0[01]/* entries look correct. The /proc/bus/pci/0[23]/* entries don't match "devices" data and looks wrong. The host machine is a HP LXR8000 (4x 500Mhz PIII, 2GB RAM, ~8 PCI slots). Eg for 02/6.0 lspci -v says: 02:06.0 Non-VGA unclassified device: Digital Equipment Corporation DECchip 21154 Flags: fast devsel I/O ports at [disabled] Memory at (type 3, non-prefetchable) [disabled] Memory at (type 3, non-prefetchable) [disabled] Memory at (low-1M, non-prefetchable) [disabled] Memory at (32-bit, prefetchable) [disabled] (This is a 64-bit PCI-PCI bridge) od -Ax -x /proc/bus/pci/02/06.0 00 1000 1000 0040 10 020b 4011 1000 000b 0157 0210 20 0007 0100 8008 0080 4001 4004 fe40 30 0004 fe40 40 * 000100 /proc/bus/pci/devices for 02/06.0 says: 0230101100260 Full output for lspci -t, lspci -v, /proc/bus/pci/0?/*, and devices is available at ftp://gsyprf10.external.hp.com/pub/244_pci/. If more info is desired, send me mail. I didn't see anything obviously wrong with proc_bus_pci_read() in drivers/pci/proc.c. My first guess is the *ppos parameter is fubar but I'm not able to test this theory. My excuse is the LXR8000 doesn't reboot reliably and is 1km away (I'm in Germany instead of California). If this isn't already fixed in 2.4.5 (or .6), I'll look at it in July when I get back. grant Grant Grundler parisc PCI|IOMMU|SMP hacker +1 408-447-7253 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars
On Tue, Jul 05, 2005 at 01:46:20PM -0400, John W. Linville wrote: > On Sat, Jul 02, 2005 at 01:29:54AM -0600, Grant Grundler wrote: > > On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote: > > > > + /* Some devices lose PCI config header data during D3hot->D0 > > > > Can you name some of those devices here? > > I just want to know what sort of devices need to be tested > > if this code changes in the future. > > I don't really have a list. The devices that brought this issue to > my attention are a 3c905B and a 3c556B, both covered by the 3c59x > driver. John, apologies for the late reply - been offline the past two weeks on holiday. Just listing the two devices in a comment would be sufficient. > According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT INTERFACE > SPECIFICATION, REV. 1.2", a device transitioning from D3hot to D0 > _may_ perform an internal reset, thereby going to "D0 Uninitialized" > rather than "D0 Initialized". Including the above paragraph in a comment would be a good thing. I don't know if this spec is publicly available. But even if it is, typically only a handful of people will be familiar enough with it to know where to look in it. > Since this behaviour is ratified by > the spec, I think we need to accomodate it. Yes - sounds reasonable to me too. > A bit in the PMCSR register indicates how a device will behave in > this regard. We could have a test to only execute the BAR restoration > for those devices that seem to need it. I left that out because it > seemed to add needless complexity. In the meantime the patch has > gotten bigger and more complex, so maybe that code doesn't make it > any worse. Do you want me to add that? I think I'd keep it simpler until someone proves we need it. I've read the rest of the thread and don't recall any such proof. > > > > > > +transition. Since some firmware leaves devices in D3hot > > > +state at boot, this information needs to be restored. > > > > Again, which firmware? > > Examples are good since it makes it possible to track down > > the offending devices for testing. > > The Thinkpad T21 does this. I don't know of any others specifically, > but it seems like something laptop BIOSes would be likely to do. That's fine - just listing the Thinkpad T21 in a comment is helpful. If you happen to know the firmware version too, that would be even better. > > The following chunk looks like it will have issues with 64-bit BARs: > > As RMK pointed-out, this code is inspired by setup-res.c; specifically > that in pci_update_resource. I'd prefer not to blaze any new trails > regarding 64-bit BAR support ATM... :-) After thinking about this more, I'm convinced it's broken if a 64-bit BAR is present on the PCI device. It doesn't matter if the MMIO value is greater than 4GB or not. The problem is pci_dev->resource[i] does NOT map 1:1 with PCI_BASE_ADDRESS_0+(i*4). > So, is the current patch acceptable? I don't think so. 64-bit BARs are just too common today. One solution is to use a seperate variable to track the offset into PCI config space. ie use "i" to walk through pci_dev->resource[] and add "unsigned int pcibar_offset" to keep track of 32 vs 64-bit BARs. > Or shall I add the check for the "no soft reset" bit in the PMCSR register? I don't see why that's necessary. thanks, grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8] pci and yenta: pcibios_bus_to_resource
On Tue, Jul 12, 2005 at 12:21:38AM +0200, Dominik Brodowski wrote: > In yenta_socket, we default to using the resource setting of the CardBus > bridge. However, this is a PCI-bus-centric view of resources and thus > needs to be converted to generic resources first. Therefore, add a call > to pcibios_bus_to_resource() call in between. This function is a mere > wrapper on x86 and friends, however on some others it already exists, is > added in this patch (alpha, arm, ppc, ppc64) or still needs to be > provided (parisc -- where is its pcibios_resource_to_bus() ?). in arch/parisc/kernel/pci.c? At least, it seems to be present in the 2.6.13-rc1 tree on cvs.parisc-linux.org tree. Arnaldo De Carmelo had add-on pci-pcmcia cards working in his a500 (64-bit w/IOMMU PA-RISC) last year. ISTR a few other people have similar cards working for on B180 workstation (32-bit w/o IOMMU PARISC). grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c
On Thu, Jul 14, 2005 at 02:53:27PM +0100, Russell King wrote: > On Thu, Jul 14, 2005 at 03:53:44PM +0400, Ivan Kokshaysky wrote: > > The setup-bus code doesn't work correctly for configurations > > with more than one display adapter in the same PCI domain. > > This stuff actually is a leftover of an early 2.4 PCI setup code > > and apparently it stopped working after some "bridge_ctl" changes. > > So the best thing we can do is just to remove it and rely on the fact > > that any firmware *has* to configure VGA port forwarding for the boot > > display device properly. > > What happens when there is no firmware? I helped test/add bridge_ctl patch but PARISC general does NOT support VGA at this time. > I'm sure this code would not have been added had there not been a reason > for it. Do we know why it was added? It was a replacement for the previous hacks and should represent essentially the same functionality. I suspect we just didn't care about (or test) multiheaded gfx at the time. Certainly not on parisc. This was in 2000/2001 timeframe originally. grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.13-rc1 05/10] IOCHK interface for I/O error handling/detecting
On Wed, Jul 06, 2005 at 02:11:42PM +0900, Hidetoshi Seto wrote: > [This is 5 of 10 patches, "iochk-05-check_bridge.patch"] ... > It means that A or B hits a bus error, but there is no data > which one actually hits the error. So, C should notify the > error to both of A and B, and clear the H's status to start > its own I/Os. > > If there are only two devices, it become more simple. It is > clear if one find a bridge error while another is check-in, > the error is nothing except for another's. Sorry, I don't understand this last paragraph. I don't see how it's more simple with two devices (vs three) if we don't exactly know which device caused the error. I thought one still needed to reset/restart both devices. Is that correct? The devices operate asyncronously from the drivers. Only the driver can tell us for sure if IO was in flight for a particular device and decide that a device could NOT have generated an error. Otherwise, so far, the patches look fine to me. thanks, grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pcibios_bus_to_resource for parisc [Was: Re: [PATCH 8/8] pci and yenta: pcibios_bus_to_resource]
On Sat, Jul 23, 2005 at 09:54:11PM +0200, Dominik Brodowski wrote:
> Oh, yes, I seem to have missed it. Sorry. Does this patch look good?
Yes.
Acked-by: Grant Grundler <[EMAIL PROTECTED]>
I'll commit this to the cvs.parisc-linux.org tree as well.
Willy can let me deal with the collision if it's not trivial
on his next merge.
thanks,
grant
>
>
> Add pcibios_bus_to_resource for parisc.
>
> Signed-off-by: Dominik Brodowski <[EMAIL PROTECTED]>
>
> Index: 2.6.13-rc3-git2/arch/parisc/kernel/pci.c
> ===
> --- 2.6.13-rc3-git2.orig/arch/parisc/kernel/pci.c
> +++ 2.6.13-rc3-git2/arch/parisc/kernel/pci.c
> @@ -255,8 +255,26 @@ void __devinit pcibios_resource_to_bus(s
> pcibios_link_hba_resources(&hba->lmmio_space, bus->resource[1]);
> }
>
> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> + struct pci_bus_region *region)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pci_hba_data *hba = HBA_DATA(bus->bridge->platform_data);
> +
> + if (res->flags & IORESOURCE_MEM) {
> + res->start = PCI_HOST_ADDR(hba, region->start);
> + res->end = PCI_HOST_ADDR(hba, region->end);
> + }
> +
> + if (res->flags & IORESOURCE_IO) {
> + res->start = region->start;
> + res->end = region->end;
> + }
> +}
> +
> #ifdef CONFIG_HOTPLUG
> EXPORT_SYMBOL(pcibios_resource_to_bus);
> +EXPORT_SYMBOL(pcibios_bus_to_resource);
> #endif
>
> /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] Re: mthca and LinuxBIOS
On Fri, Aug 05, 2005 at 04:06:06PM -0700, Linus Torvalds wrote: > > > On Fri, 5 Aug 2005, Greg KH wrote: > > On Fri, Aug 05, 2005 at 01:38:37PM -0700, Linus Torvalds wrote: > > > > But what's the real problem we are trying to fix here? > > We're screwing up the top 32 bits of the BAR when you resume it. Look at > the patch, you'll see the fix (the other part of the patch looks fine, but > then in order to not overwrite the upper bits with zero again when doing > the _next_ - nonexistent - BAR update, we need to have something that > avoids writing the next BAR). ISTR making comments before about the offending patch on linux-pci mailing list. Is this the same patch that assumes pci_dev->resource[i] == BAR[i] ? That's not true for 64-bit bars. > Remember: a 64-bit BAR puts the upper 32 bits in what would otherwise be > the low 32 bits of the next BAR. Which is why we need to mark the next BAR > resource as _not_ being valid some way - so that we don't try to > (incorrectly) "restore" it and overwrite the high bits of the previous > BAR. > Of course, this only hits the (very few) people who not only have 64-bit > PCI devices, but literally have them mapped in the 4GB+ region. *lots* of PCI device now have 64-bit BAR. The first I'm aware of was LSI 53c896 card (Ultra 2 SCSI). > Quite uncommon. Assigning 4GB+ regions is uncommon because too often either the HW, the OS, or the driver would break. firmware keeps having to worry about legacy OSs. grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] Re: mthca and LinuxBIOS
On Fri, Aug 05, 2005 at 04:59:37PM -0700, Grant Grundler wrote: > ISTR making comments before about the offending patch on linux-pci mailing > list. Is this the same patch that assumes pci_dev->resource[i] == BAR[i] ? I meant the patch assume 1:1 for pci_dev->resource[i] and BAR[i]. not that the two are equivalent. grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] Re: mthca and LinuxBIOS
On Fri, Aug 05, 2005 at 04:03:00PM -0700, Greg KH wrote: ... > > yesterday, someone add pci_restore_bars, that will call > > pci_update_resource, and it will overwirte upper 32 bit of BAR2 and > > BAR4 of IB card. > > Hm, perhaps that change should not do this? > > Dominik, care to weigh in here? That was your patch... Was the origin of this patch the following thread started by John Linville: http://lkml.org/lkml/2005/6/23/257 I pointed out it would have issues with 64-bit BARs. And I suggested some solutions to JohnL's patch here: http://lkml.org/lkml/2005/7/2/14 In any case same issues apply to pci_update_resource(). grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] Re: [PATCH 1/2] [IB/cm]: Correct CM port redirect reject codes
On Wed, Aug 03, 2005 at 05:58:11PM -0700, yhlu wrote: > Roland, > > In LinuxBIOS, If I enable the prefmem64 to use real 64 range. the IB > driver in Kernel can not be loaded. Can you provide a few more details about the configuration? o kernel version o architecture (i386 or x86-64) o post the full console output from power up? Recent email on linux-pci raised awareness that 32-bit kernel can not support 64-bit PCI MMIO addresses. struct resource (defined in include/linux/ioport.h) defines the start/end field as "unsigned long". That's only 32-bit on i386 kernels. > PCI: 04:00.0 18 <- [0xfcf000 - 0xfcf07f] prefmem64 > PCI: 04:00.0 20 <- [0xfce000 - 0xfcefff] prefmem64 > I have to wonder if those BARs are truly prefetchable. Does Mellanox assume CPU is the only one to write the 3rd BAR (RAM) and the CPU implements a write-through cache (vs write back)? I'm just guessing because I don't understand exactly how the 256MB of onboard RAM is accessed. hth, grant > > ib_mthca: Mellanox InfiniBand HCA driver v0.06 (June 23, 2005) > ib_mthca: Initializing Mellanox Technologies MT25208 InfiniHost III Ex (Tavor > c) > ib_mthca :04:00.0: Failed to initialize queue pair table, aborting. > ib_mthca: probe of :04:00.0 failed with error -16 > ___ > openib-general mailing list > openib-general@openib.org > http://openib.org/mailman/listinfo/openib-general > > To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general And I have to wonder if those BARs truly are prefetchable. It would imply only the CPU writes them and - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] [RFC] Move InfiniBand .h files
On Thu, Aug 04, 2005 at 10:32:14AM -0700, Roland Dreier wrote: ... I agree with the rename/relocation of the header files for the reasons you mentioned. > - It makes it a little harder to pull the OpenIB svn tree into a > kernel tree, since one would have to link both drivers/infiniband > and include/linux/rdma instead of just drivers/infiniband. This > problem goes away if/when OpenIB shifts over to a new source code > control system. Any thoughts on renaming drivers/infiniband to drivers/rdma at the same time? If you are going to churn...don't be shy about it :^) grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [openib-general] Re: [RFC] Move InfiniBand .h files
On Thu, Aug 04, 2005 at 07:53:58PM +0200, Arjan van de Ven wrote: > On Thu, 2005-08-04 at 10:32 -0700, Roland Dreier wrote: > > I would like to get people's reactions to moving the InfiniBand .h > > files from their current location in drivers/infiniband/include/ to > > include/linux/rdma/. If we agree that this is a good idea then I'll > > push this change as soon as 2.6.14 starts. > > please only put userspace clean headers here; the rest is more or less > private headers for your subsystem. Sorry...this smells like a rathole...but does this mean linus agrees the kernel subsystems should export headers suitable for both user space and kernel driver modules? Historical, I thought glibc and other user space libs were expected to maintain their own set of header files. Maybe I'm just confused... thanks, grant - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
