Skip to contentSkip to conversation

dmaengine: memset clarifications and fixes

PATCH Applied Ben Walker 3 participants 8 messages Mar 1, 2022 – Aug 2, 2022

Subsystems (5)
lore
Share & tools
lore

Ben Walker

author
#0 lore raw

v3 cover dmaengine: memset clarifications and fixes

The following contains a clarification for the behavior of the 'value' parameter in the memset operation. It is intended to be a single byte pattern as laid out here:

https://lore.kernel.org/dmaengine/YejrA5ZWZ3lTRO%2F1@matsya/

Then I'm attempting to fix all places it is currently used. But note that I do not have access to this hardware and cannot test it. We'll really need a maintainer to take a look at each of these to verify that the changes are correct.

v3:
- Using signed byte for pattern everywhere

Ben Walker (4):
dmaengine: Document dmaengine_prep_dma_memset
dmaengine: at_hdmac: In atc_prep_dma_memset, treat value as a single
byte
dmaengine: at_xdmac: In at_xdmac_prep_dma_memset, treat value as a
single byte
dmaengine: hidma: In hidma_prep_dma_memset treat value as a single
byte

 drivers/dma/at_hdmac.c    | 10 +++++++++-
 drivers/dma/at_xdmac.c    |  9 ++++++++-
 drivers/dma/qcom/hidma.c  | 13 ++++++++++++-
 include/linux/dmaengine.h |  8 ++++++++
 4 files changed, 37 insertions(+), 3 deletions(-)
Signature · 1 line
2.33.1
author
#1 lore raw

PATCH v3 1/4 dmaengine: Document dmaengine_prep_dma_memset

Document this function to make clear the expected behavior of the 'value' parameter. It was intended to match the behavior of POSIX memset as laid out here:

https://lore.kernel.org/dmaengine/YejrA5ZWZ3lTRO%2F1@matsya/

---
 Documentation/driver-api/dmaengine/provider.rst | 6 ++++++
 include/linux/dmaengine.h                       | 8 ++++++++
 2 files changed, 14 insertions(+)
Documentation/driver-api/dmaengine/provider.rst +6 -0
index 0072c9c7efd34..4f99f0e9bb81d 100644
@@ -206,6 +206,12 @@ Currently, the types available are:
   - The device is able to perform parity check using RAID6 P+Q
     algorithm against a memory buffer.
 
+- DMA_MEMSET
+
+  - The device is able to fill memory with the provided pattern
+
+  - The pattern is treated as a single byte signed value.
+
 - DMA_INTERRUPT
 
   - The device is able to trigger a dummy transfer that will
include/linux/dmaengine.h +8 -0
index 842d4f7ca752d..3d3e663e17ac7 100644
@@ -1031,6 +1031,14 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_interleaved_dma(
 	return chan->device->device_prep_interleaved_dma(chan, xt, flags);
 }
 
+/**
+ * dmaengine_prep_dma_memset() - Prepare a DMA memset descriptor.
+ * @chan: The channel to be used for this descriptor
+ * @dest: Address of buffer to be set
+ * @value: Treated as a single byte value that fills the destination buffer
+ * @len: The total size of dest
+ * @flags: DMA engine flags
+ */
 static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memset(
 		struct dma_chan *chan, dma_addr_t dest, int value, size_t len,
 		unsigned long flags)
Signature · 1 line
2.35.1
author
#2 lore raw

PATCH v3 2/4 dmaengine: at_hdmac: In atc_prep_dma_memset, treat value as a single byte

In reply to Ben Walker

The value passed in to .prep_dma_memset is to be treated as a single byte repeating pattern.

Cc Ludovic Desroches <[email protected]>
Cc Tudor Ambarus <[email protected]>
---
 drivers/dma/at_hdmac.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/at_hdmac.c +9 -1
index 30ae36124b1db..5a50423b7378e 100644
@@ -942,6 +942,7 @@ atc_prep_dma_memset(struct dma_chan *chan, dma_addr_t dest, int value,
 	struct at_desc		*desc;
 	void __iomem		*vaddr;
 	dma_addr_t		paddr;
+	char			fill_pattern;
 
 	dev_vdbg(chan2dev(chan), "%s: d%pad v0x%x l0x%zx f0x%lx\n", __func__,
 		&dest, value, len, flags);
@@ -963,7 +964,14 @@ atc_prep_dma_memset(struct dma_chan *chan, dma_addr_t dest, int value,
 			__func__);
 		return NULL;
 	}
-	*(u32*)vaddr = value;
+
+	/* Only the first byte of value is to be used according to dmaengine */
+	fill_pattern = (char)value;
+
+	*(u32*)vaddr = (fill_pattern << 24) |
+		       (fill_pattern << 16) |
+		       (fill_pattern << 8) |
+		       fill_pattern;
 
 	desc = atc_create_memset_desc(chan, paddr, dest, len);
 	if (!desc) {
Signature · 1 line
2.35.1
author
#3 lore raw

PATCH v3 3/4 dmaengine: at_xdmac: In at_xdmac_prep_dma_memset, treat value as a single byte

In reply to Ben Walker

The value passed in to .prep_dma_memset is to be treated as a single byte repeating pattern.

Cc Ludovic Desroches <[email protected]>
Cc Tudor Ambarus <[email protected]>
---
 drivers/dma/at_xdmac.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/dma/at_xdmac.c +8 -1
index a1da2b4b6d732..fb9df3a69babd 100644
@@ -1202,6 +1202,7 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan,
 	unsigned long		flags;
 	size_t			ublen;
 	u32			dwidth;
+	char			pattern;
 	/*
 	 * WARNING: The channel configuration is set here since there is no
 	 * dmaengine_slave_config call in this case. Moreover we don't know the
@@ -1244,10 +1245,16 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan,
 
 	chan_cc |= AT_XDMAC_CC_DWIDTH(dwidth);
 
+	/* Only the first byte of value is to be used according to dmaengine */
+	pattern = (char)value;
+
 	ublen = len >> dwidth;
 
 	desc->lld.mbr_da = dst_addr;
-	desc->lld.mbr_ds = value;
+	desc->lld.mbr_ds = (pattern << 24) |
+			   (pattern << 16) |
+			   (pattern << 8) |
+			   pattern;
 	desc->lld.mbr_ubc = AT_XDMAC_MBR_UBC_NDV3
 		| AT_XDMAC_MBR_UBC_NDEN
 		| AT_XDMAC_MBR_UBC_NSEN
Signature · 1 line
2.35.1
#4 lore raw

Re: [PATCH v3 3/4] dmaengine: at_xdmac: In at_xdmac_prep_dma_memset, treat value as a single byte

Sorry for the delay.
Just wanted to inform you that I plan to test this patch set this week.

Cheers,
ta

author
#5 lore raw

PATCH v3 4/4 dmaengine: hidma: In hidma_prep_dma_memset treat value as a single byte

In reply to Ben Walker

The value parameter is a single byte, so duplicate it to the 8 byte range that is used as the pattern.

---
 drivers/dma/qcom/hidma.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/dma/qcom/hidma.c +12 -1
index 65d054bb11aaa..e455957e3ff98 100644
@@ -431,6 +431,7 @@ hidma_prep_dma_memset(struct dma_chan *dmach, dma_addr_t dest, int value,
 	struct hidma_desc *mdesc = NULL;
 	struct hidma_dev *mdma = mchan->dmadev;
 	unsigned long irqflags;
+	u64 byte_pattern, fill_pattern;
 
 	/* Get free descriptor */
 	spin_lock_irqsave(&mchan->lock, irqflags);
@@ -443,9 +444,19 @@ hidma_prep_dma_memset(struct dma_chan *dmach, dma_addr_t dest, int value,
 	if (!mdesc)
 		return NULL;
 
+	byte_pattern = (char)value;
+	fill_pattern =	(byte_pattern << 56) |
+			(byte_pattern << 48) |
+			(byte_pattern << 40) |
+			(byte_pattern << 32) |
+			(byte_pattern << 24) |
+			(byte_pattern << 16) |
+			(byte_pattern << 8) |
+			byte_pattern;
+
 	mdesc->desc.flags = flags;
 	hidma_ll_set_transfer_params(mdma->lldev, mdesc->tre_ch,
-				     value, dest, len, flags,
+				     fill_pattern, dest, len, flags,
 				     HIDMA_TRE_MEMSET);
 
 	/* Place descriptor in prepared list */
Signature · 1 line
2.35.1
maintainer Applied
#6 lore raw
In reply to Ben Walker
Quoted 10 lines · Ben Walker

The following contains a clarification for the behavior of the 'value' parameter in the memset operation. It is intended to be a single byte pattern as laid out here:

https://lore.kernel.org/dmaengine/YejrA5ZWZ3lTRO%2F1@matsya/

Then I'm attempting to fix all places it is currently used. But note that I do not have access to this hardware and cannot test it. We'll really need a maintainer to take a look at each of these to verify that the changes are correct.

Applied, thanks

Signature · 1 line
~Vinod
#7 lore raw
In reply to Ben Walker

+ Maxime.

Hi!

Quoted 7 lines · Ben Walker

The following contains a clarification for the behavior of the 'value' parameter in the memset operation. It is intended to be a single byte pattern as laid out here:

https://lore.kernel.org/dmaengine/YejrA5ZWZ3lTRO%2F1@matsya/

Then I'm attempting to fix all places it is currently used. But note

Sorry for being late. I see there are no in kernel users of dma_memset/dma_memset_sg methods (other than dmatest.c). Any idea why do we still keep them?

Signature · 2 lines
Cheers,
ta