All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Atish Patra <atish.patra@wdc.com>
Cc: devicetree@vger.kernel.org, Albert Ou <aou@eecs.berkeley.edu>,
	Tobias Klauser <tklauser@distanz.ch>,
	Robin Murphy <robin.murphy@arm.com>,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	iommu@lists.linux-foundation.org,
	Guo Ren <guoren@linux.alibaba.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	linux-riscv@lists.infradead.org,
	Frank Rowand <frowand.list@gmail.com>,
	Christoph Hellwig <hch@lst.de>,
	Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions
Date: Mon, 26 Jul 2021 08:56:57 +0200	[thread overview]
Message-ID: <20210726065657.GA9035@lst.de> (raw)
In-Reply-To: <20210723214031.3251801-2-atish.patra@wdc.com>

> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> +struct riscv_dma_cache_sync {
> +	void (*cache_invalidate)(phys_addr_t paddr, size_t size);
> +	void (*cache_clean)(phys_addr_t paddr, size_t size);
> +	void (*cache_flush)(phys_addr_t paddr, size_t size);
> +};
> +
> +void riscv_dma_cache_sync_set(struct riscv_dma_cache_sync *ops);
> +#endif

As told a bunch of times before: doing indirect calls here is a
performance nightmare.  Use something that actually does perform
horribly like alternatives.  Or even delay implementing that until
we need it and do a plain direct call for now.

static void __dma_sync(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> +{
> +	if ((dir == DMA_FROM_DEVICE) && (dma_cache_sync->cache_invalidate))
> +		dma_cache_sync->cache_invalidate(paddr, size);
> +	else if ((dir == DMA_TO_DEVICE) && (dma_cache_sync->cache_clean))
> +		dma_cache_sync->cache_clean(paddr, size);
> +	else if ((dir == DMA_BIDIRECTIONAL) && dma_cache_sync->cache_flush)
> +		dma_cache_sync->cache_flush(paddr, size);
> +}

The seletion of flush types is completely broken.  Take a look at the
comment in arch/arc/mm/dma.c above arch_sync_dma_for_device for a good
explanation.

> +void arch_dma_prep_coherent(struct page *page, size_t size)
> +{
> +	void *flush_addr = page_address(page);
> +
> +	memset(flush_addr, 0, size);

arch_dma_prep_coherent is not supposed to modify the content of
the data.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Atish Patra <atish.patra@wdc.com>
Cc: linux-kernel@vger.kernel.org, Albert Ou <aou@eecs.berkeley.edu>,
	Christoph Hellwig <hch@lst.de>,
	devicetree@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Guo Ren <guoren@linux.alibaba.com>,
	iommu@lists.linux-foundation.org,
	linux-riscv@lists.infradead.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Rob Herring <robh+dt@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Tobias Klauser <tklauser@distanz.ch>
Subject: Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions
Date: Mon, 26 Jul 2021 08:56:57 +0200	[thread overview]
Message-ID: <20210726065657.GA9035@lst.de> (raw)
In-Reply-To: <20210723214031.3251801-2-atish.patra@wdc.com>

> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> +struct riscv_dma_cache_sync {
> +	void (*cache_invalidate)(phys_addr_t paddr, size_t size);
> +	void (*cache_clean)(phys_addr_t paddr, size_t size);
> +	void (*cache_flush)(phys_addr_t paddr, size_t size);
> +};
> +
> +void riscv_dma_cache_sync_set(struct riscv_dma_cache_sync *ops);
> +#endif

As told a bunch of times before: doing indirect calls here is a
performance nightmare.  Use something that actually does perform
horribly like alternatives.  Or even delay implementing that until
we need it and do a plain direct call for now.

static void __dma_sync(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> +{
> +	if ((dir == DMA_FROM_DEVICE) && (dma_cache_sync->cache_invalidate))
> +		dma_cache_sync->cache_invalidate(paddr, size);
> +	else if ((dir == DMA_TO_DEVICE) && (dma_cache_sync->cache_clean))
> +		dma_cache_sync->cache_clean(paddr, size);
> +	else if ((dir == DMA_BIDIRECTIONAL) && dma_cache_sync->cache_flush)
> +		dma_cache_sync->cache_flush(paddr, size);
> +}

The seletion of flush types is completely broken.  Take a look at the
comment in arch/arc/mm/dma.c above arch_sync_dma_for_device for a good
explanation.

> +void arch_dma_prep_coherent(struct page *page, size_t size)
> +{
> +	void *flush_addr = page_address(page);
> +
> +	memset(flush_addr, 0, size);

arch_dma_prep_coherent is not supposed to modify the content of
the data.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Atish Patra <atish.patra@wdc.com>
Cc: linux-kernel@vger.kernel.org, Albert Ou <aou@eecs.berkeley.edu>,
	Christoph Hellwig <hch@lst.de>,
	devicetree@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Guo Ren <guoren@linux.alibaba.com>,
	iommu@lists.linux-foundation.org,
	linux-riscv@lists.infradead.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Rob Herring <robh+dt@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Tobias Klauser <tklauser@distanz.ch>
Subject: Re: [RFC 1/5] RISC-V: Implement arch_sync_dma* functions
Date: Mon, 26 Jul 2021 08:56:57 +0200	[thread overview]
Message-ID: <20210726065657.GA9035@lst.de> (raw)
In-Reply-To: <20210723214031.3251801-2-atish.patra@wdc.com>

> +#ifdef CONFIG_RISCV_DMA_NONCOHERENT
> +struct riscv_dma_cache_sync {
> +	void (*cache_invalidate)(phys_addr_t paddr, size_t size);
> +	void (*cache_clean)(phys_addr_t paddr, size_t size);
> +	void (*cache_flush)(phys_addr_t paddr, size_t size);
> +};
> +
> +void riscv_dma_cache_sync_set(struct riscv_dma_cache_sync *ops);
> +#endif

As told a bunch of times before: doing indirect calls here is a
performance nightmare.  Use something that actually does perform
horribly like alternatives.  Or even delay implementing that until
we need it and do a plain direct call for now.

static void __dma_sync(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> +{
> +	if ((dir == DMA_FROM_DEVICE) && (dma_cache_sync->cache_invalidate))
> +		dma_cache_sync->cache_invalidate(paddr, size);
> +	else if ((dir == DMA_TO_DEVICE) && (dma_cache_sync->cache_clean))
> +		dma_cache_sync->cache_clean(paddr, size);
> +	else if ((dir == DMA_BIDIRECTIONAL) && dma_cache_sync->cache_flush)
> +		dma_cache_sync->cache_flush(paddr, size);
> +}

The seletion of flush types is completely broken.  Take a look at the
comment in arch/arc/mm/dma.c above arch_sync_dma_for_device for a good
explanation.

> +void arch_dma_prep_coherent(struct page *page, size_t size)
> +{
> +	void *flush_addr = page_address(page);
> +
> +	memset(flush_addr, 0, size);

arch_dma_prep_coherent is not supposed to modify the content of
the data.

  reply	other threads:[~2021-07-26  6:57 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 21:40 [RFC 0/5] Support non-coherent DMA on RISC-V using a global pool Atish Patra
2021-07-23 21:40 ` Atish Patra
2021-07-23 21:40 ` Atish Patra
2021-07-23 21:40 ` [RFC 1/5] RISC-V: Implement arch_sync_dma* functions Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-26  6:56   ` Christoph Hellwig [this message]
2021-07-26  6:56     ` Christoph Hellwig
2021-07-26  6:56     ` Christoph Hellwig
2021-07-26 21:52     ` Atish Patra
2021-07-26 21:52       ` Atish Patra
2021-07-26 21:52       ` Atish Patra
2021-09-11  9:37       ` Guo Ren
2021-09-11  9:37         ` Guo Ren
2021-09-11  9:37         ` Guo Ren
2021-08-17  1:48   ` Guo Ren
2021-08-17  1:48     ` Guo Ren
2021-08-17  1:48     ` Guo Ren
2021-08-17  3:24     ` Atish Patra
2021-08-17  3:24       ` Atish Patra
2021-08-17  3:24       ` Atish Patra
2021-08-17  6:28       ` Guo Ren
2021-08-17  6:28         ` Guo Ren
2021-08-17  6:28         ` Guo Ren
2021-07-23 21:40 ` [RFC 2/5] of: Move of_dma_get_range to of_address.h Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40 ` [RFC 3/5] dma-mapping: Enable global non-coherent pool support for RISC-V Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-25 22:29   ` Rob Herring
2021-07-25 22:29     ` Rob Herring
2021-07-25 22:29     ` Rob Herring
2021-07-26  7:00   ` Christoph Hellwig
2021-07-26  7:00     ` Christoph Hellwig
2021-07-26  7:00     ` Christoph Hellwig
2021-07-26 22:47     ` Atish Patra
2021-07-26 22:47       ` Atish Patra
2021-07-26 22:47       ` Atish Patra
2021-07-27  8:52       ` Christoph Hellwig
2021-07-27  8:52         ` Christoph Hellwig
2021-07-27  8:52         ` Christoph Hellwig
2021-08-02 18:22         ` Atish Patra
2021-08-02 18:22           ` Atish Patra
2021-08-02 18:22           ` Atish Patra
2021-07-23 21:40 ` [RFC 4/5] dma-direct: Allocate dma pages directly if global pool allocation fails Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-26  7:01   ` Christoph Hellwig
2021-07-26  7:01     ` Christoph Hellwig
2021-07-26  7:01     ` Christoph Hellwig
2021-07-23 21:40 ` [RFC 5/5] RISC-V: Support a new config option for non-coherent DMA Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-23 21:40   ` Atish Patra
2021-07-29  4:30 ` [RFC 0/5] Support non-coherent DMA on RISC-V using a global pool Palmer Dabbelt
2021-07-29  4:30   ` Palmer Dabbelt
2021-07-29  4:30   ` Palmer Dabbelt
2021-07-29  6:19   ` Atish Patra
2021-07-29  6:19     ` Atish Patra
2021-07-29  6:19     ` Atish Patra
2021-08-17  1:37     ` Guo Ren
2021-08-17  1:37       ` Guo Ren
2021-08-17  1:37       ` Guo Ren
2021-08-17  3:28       ` Atish Patra
2021-08-17  3:28         ` Atish Patra
2021-08-17  3:28         ` Atish Patra
2021-08-17  6:42         ` Guo Ren
2021-08-17  6:42           ` Guo Ren
2021-08-17  6:42           ` Guo Ren

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20210726065657.GA9035@lst.de \
    --to=hch@lst.de \
    --cc=aou@eecs.berkeley.edu \
    --cc=atish.patra@wdc.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dvyukov@google.com \
    --cc=frowand.list@gmail.com \
    --cc=guoren@linux.alibaba.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=tklauser@distanz.ch \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.