[RFC] [PATCH v2 0/3] scatterlist: Add support to clone sg_table

[RFC] [PATCH v2 0/3] scatterlist: Add support to clone sg_table

[Franklin S Cooper Jr]

This patchset creates two new functions within scatterlist that allows a
user to pass in a sg_table and receive a duplicate copy. The sgl in this
copied table are dynamically allocated but its values such as offset,
length and dma_address are the same as its variant within the sgl in the
original sg_table.

This is useful when tweaks to sgl may be needed to alter a future DMA
operation by tweaking the table nents count or the individual sgl
offset/length without changing the original values.

An example use case is also included in this patchset. In the omap2-mcspi
driver on certain OMAP SOCs if certain conditions are met the DMA should
transfer one or two less elements when reading from the SPI. The
remaining bytes are read in CPU mode. To accomplish this the spi's rx_sg
sgl should have the last entry length reduced for the DMA operation.
However, this change should only be done locally so instead of altering
the original sgl this new function allows a clone to be created.

V2 changes:
Save page_link value if page_link doesn't point to chained sg_list

Franklin S Cooper Jr (3):
  scatterlist: Add support to clone scatterlist
  spi: omap2-mcspi: Add comments for RX only DMA buffer workaround
  spi: omap2-mcspi: Use the SPI framework to handle DMA mapping

 drivers/spi/spi-omap2-mcspi.c | 120 +++++++++++++++++-------------------------
 include/linux/scatterlist.h   |   2 +
 lib/scatterlist.c             | 103 ++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 73 deletions(-)

-- 
2.7.0

[RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr]

Occasionally there are times you need to tweak a chained S/G list while
maintaining the original list. This function will duplicate the passed
in chained S/G list and return a pointer to the cloned copy.

The function also supports passing in a length that can be equal or less
than the original chained S/G list length. Reducing the length can result
in less entries of the chained S/G list being created.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 include/linux/scatterlist.h |   2 +
 lib/scatterlist.c           | 103 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index cb3c8fe..9a109da 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -247,6 +247,8 @@ struct scatterlist *sg_next(struct scatterlist *);
 struct scatterlist *sg_last(struct scatterlist *s, unsigned int);
 void sg_init_table(struct scatterlist *, unsigned int);
 void sg_init_one(struct scatterlist *, const void *, unsigned int);
+struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
+				gfp_t gfp_mask);
 int sg_split(struct scatterlist *in, const int in_mapped_nents,
 	     const off_t skip, const int nb_splits,
 	     const size_t *split_sizes,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70..b15f648 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -180,6 +180,109 @@ static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
 		return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
 }
 
+/*
+ * sg_clone -	Duplicate an existing chained sgl
+ * @orig_sgl:	Original sg list to be duplicated
+ * @len:	Total length of sg while taking chaining into account
+ * @gfp_mask:	GFP allocation mask
+ *
+ * Description:
+ *   Clone a chained sgl. This cloned copy may be modified in some ways while
+ *   keeping the original sgl in tact. Also allow the cloned copy to have
+ *   a smaller length than the original which may reduce the sgl total
+ *   sg entries.
+ *
+ * Returns:
+ *   Pointer to new kmalloced sg list, ERR_PTR() on error
+ *
+ */
+static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
+				    gfp_t gfp_mask)
+{
+	unsigned int		nents;
+	bool			last_entry;
+	struct scatterlist	*sgl, *head;
+
+	nents = sg_nents_for_len(orig_sgl, len);
+
+	if (nents < 0)
+		return ERR_PTR(-EINVAL);
+
+	head = sg_kmalloc(nents, gfp_mask);
+
+	if (!head)
+		return ERR_PTR(-ENOMEM);
+
+	sgl = head;
+
+	sg_init_table(sgl, nents);
+
+	for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {
+
+		last_entry = sg_is_last(sgl);
+		*sgl = *orig_sgl;
+
+		/*
+		 * If page_link is pointing to a chained sgl then set it to
+		 * zero since we already compensated for chained sgl. If
+		 * page_link is pointing to a page then clear bits 1 and 0.
+		 */
+		if (sg_is_chain(orig_sgl))
+			sgl->page_link = 0x0;
+		else
+			sgl->page_link &= ~0x03;
+
+		if (last_entry) {
+			sg_dma_len(sgl) = len;
+			/* Set bit 1 to indicate end of sgl */
+			sgl->page_link |= 0x02;
+		} else {
+			len -= sg_dma_len(sgl);
+		}
+	}
+
+	return head;
+}
+
+/*
+ * sg_table_clone - Duplicate an existing sg_table including chained sgl
+ * @orig_table:     Original sg_table to be duplicated
+ * @len:            Total length of sg while taking chaining into account
+ * @gfp_mask:       GFP allocation mask
+ *
+ * Description:
+ *   Clone a sg_table along with chained sgl. This cloned copy may be
+ *   modified in some ways while keeping the original table and sgl in tact.
+ *   Also allow the cloned sgl copy to have a smaller length than the original
+ *   which may reduce the sgl total sg entries.
+ *
+ * Returns:
+ *   Pointer to new kmalloced sg_table, ERR_PTR() on error
+ *
+ */
+struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
+				gfp_t gfp_mask)
+{
+	struct sg_table	*table;
+
+	table = kmalloc(sizeof(struct sg_table), gfp_mask);
+
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
+
+	if (IS_ERR(table->sgl)) {
+		kfree(table);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	table->nents = table->orig_nents = sg_nents(table->sgl);
+
+	return table;
+}
+EXPORT_SYMBOL(sg_table_clone);
+
 static void sg_kfree(struct scatterlist *sg, unsigned int nents)
 {
 	if (nents == SG_MAX_SINGLE_ALLOC) {
-- 
2.7.0

[RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr]

Occasionally there are times you need to tweak a chained S/G list while
maintaining the original list. This function will duplicate the passed
in chained S/G list and return a pointer to the cloned copy.

The function also supports passing in a length that can be equal or less
than the original chained S/G list length. Reducing the length can result
in less entries of the chained S/G list being created.

Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
---
 include/linux/scatterlist.h |   2 +
 lib/scatterlist.c           | 103 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index cb3c8fe..9a109da 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -247,6 +247,8 @@ struct scatterlist *sg_next(struct scatterlist *);
 struct scatterlist *sg_last(struct scatterlist *s, unsigned int);
 void sg_init_table(struct scatterlist *, unsigned int);
 void sg_init_one(struct scatterlist *, const void *, unsigned int);
+struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
+				gfp_t gfp_mask);
 int sg_split(struct scatterlist *in, const int in_mapped_nents,
 	     const off_t skip, const int nb_splits,
 	     const size_t *split_sizes,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70..b15f648 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -180,6 +180,109 @@ static struct scatterlist *sg_kmalloc(unsigned int nents, gfp_t gfp_mask)
 		return kmalloc(nents * sizeof(struct scatterlist), gfp_mask);
 }
 
+/*
+ * sg_clone -	Duplicate an existing chained sgl
+ * @orig_sgl:	Original sg list to be duplicated
+ * @len:	Total length of sg while taking chaining into account
+ * @gfp_mask:	GFP allocation mask
+ *
+ * Description:
+ *   Clone a chained sgl. This cloned copy may be modified in some ways while
+ *   keeping the original sgl in tact. Also allow the cloned copy to have
+ *   a smaller length than the original which may reduce the sgl total
+ *   sg entries.
+ *
+ * Returns:
+ *   Pointer to new kmalloced sg list, ERR_PTR() on error
+ *
+ */
+static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
+				    gfp_t gfp_mask)
+{
+	unsigned int		nents;
+	bool			last_entry;
+	struct scatterlist	*sgl, *head;
+
+	nents = sg_nents_for_len(orig_sgl, len);
+
+	if (nents < 0)
+		return ERR_PTR(-EINVAL);
+
+	head = sg_kmalloc(nents, gfp_mask);
+
+	if (!head)
+		return ERR_PTR(-ENOMEM);
+
+	sgl = head;
+
+	sg_init_table(sgl, nents);
+
+	for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {
+
+		last_entry = sg_is_last(sgl);
+		*sgl = *orig_sgl;
+
+		/*
+		 * If page_link is pointing to a chained sgl then set it to
+		 * zero since we already compensated for chained sgl. If
+		 * page_link is pointing to a page then clear bits 1 and 0.
+		 */
+		if (sg_is_chain(orig_sgl))
+			sgl->page_link = 0x0;
+		else
+			sgl->page_link &= ~0x03;
+
+		if (last_entry) {
+			sg_dma_len(sgl) = len;
+			/* Set bit 1 to indicate end of sgl */
+			sgl->page_link |= 0x02;
+		} else {
+			len -= sg_dma_len(sgl);
+		}
+	}
+
+	return head;
+}
+
+/*
+ * sg_table_clone - Duplicate an existing sg_table including chained sgl
+ * @orig_table:     Original sg_table to be duplicated
+ * @len:            Total length of sg while taking chaining into account
+ * @gfp_mask:       GFP allocation mask
+ *
+ * Description:
+ *   Clone a sg_table along with chained sgl. This cloned copy may be
+ *   modified in some ways while keeping the original table and sgl in tact.
+ *   Also allow the cloned sgl copy to have a smaller length than the original
+ *   which may reduce the sgl total sg entries.
+ *
+ * Returns:
+ *   Pointer to new kmalloced sg_table, ERR_PTR() on error
+ *
+ */
+struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
+				gfp_t gfp_mask)
+{
+	struct sg_table	*table;
+
+	table = kmalloc(sizeof(struct sg_table), gfp_mask);
+
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
+
+	if (IS_ERR(table->sgl)) {
+		kfree(table);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	table->nents = table->orig_nents = sg_nents(table->sgl);
+
+	return table;
+}
+EXPORT_SYMBOL(sg_table_clone);
+
 static void sg_kfree(struct scatterlist *sg, unsigned int nents)
 {
 	if (nents == SG_MAX_SINGLE_ALLOC) {
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] [PATCH v2 2/3] spi: omap2-mcspi: Add comments for RX only DMA buffer workaround

[Franklin S Cooper Jr]

OMAP35x and OMAP37x mentions in the McSPI End-of-Transfer Sequences section
that if the McSPI is configured as a Master and only DMA RX is being
performed then the DMA transfer size needs to be reduced by 1 or 2.

This was originally implemented by:
commit 57c5c28dbc83 ("spi: omap2_mcspi rxdma bugfix")

This patch adds comments to clarify what is going on in the code since its
not obvious what problem its addressing.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/spi/spi-omap2-mcspi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 1d237e9..c47f958 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -459,6 +459,11 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	count = xfer->len;
 	dma_count = xfer->len;
 
+	/*
+	 *  In the "End-of-Transfer Procedure" section for DMA RX in OMAP35x TRM
+	 *  it mentions reducing DMA transfer length by one element in master
+	 *  normal mode.
+	 */
 	if (mcspi->fifo_depth == 0)
 		dma_count -= es;
 
@@ -478,6 +483,10 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 
 		dmaengine_slave_config(mcspi_dma->dma_rx, &cfg);
 
+		/*
+		 *  Reduce DMA transfer length by one more if McSPI is
+		 *  configured in turbo mode.
+		 */
 		if ((l & OMAP2_MCSPI_CHCONF_TURBO) && mcspi->fifo_depth == 0)
 			dma_count -= es;
 
@@ -507,6 +516,10 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	if (mcspi->fifo_depth > 0)
 		return count;
 
+	/*
+	 *  Due to the DMA transfer length reduction the missing bytes must
+	 *  be read manually to receive all of the expected data.
+	 */
 	omap2_mcspi_set_enable(spi, 0);
 
 	elements = element_count - 1;
-- 
2.7.0

[RFC] [PATCH v2 2/3] spi: omap2-mcspi: Add comments for RX only DMA buffer workaround

[Franklin S Cooper Jr]

OMAP35x and OMAP37x mentions in the McSPI End-of-Transfer Sequences section
that if the McSPI is configured as a Master and only DMA RX is being
performed then the DMA transfer size needs to be reduced by 1 or 2.

This was originally implemented by:
commit 57c5c28dbc83 ("spi: omap2_mcspi rxdma bugfix")

This patch adds comments to clarify what is going on in the code since its
not obvious what problem its addressing.

Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
---
 drivers/spi/spi-omap2-mcspi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 1d237e9..c47f958 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -459,6 +459,11 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	count = xfer->len;
 	dma_count = xfer->len;
 
+	/*
+	 *  In the "End-of-Transfer Procedure" section for DMA RX in OMAP35x TRM
+	 *  it mentions reducing DMA transfer length by one element in master
+	 *  normal mode.
+	 */
 	if (mcspi->fifo_depth == 0)
 		dma_count -= es;
 
@@ -478,6 +483,10 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 
 		dmaengine_slave_config(mcspi_dma->dma_rx, &cfg);
 
+		/*
+		 *  Reduce DMA transfer length by one more if McSPI is
+		 *  configured in turbo mode.
+		 */
 		if ((l & OMAP2_MCSPI_CHCONF_TURBO) && mcspi->fifo_depth == 0)
 			dma_count -= es;
 
@@ -507,6 +516,10 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	if (mcspi->fifo_depth > 0)
 		return count;
 
+	/*
+	 *  Due to the DMA transfer length reduction the missing bytes must
+	 *  be read manually to receive all of the expected data.
+	 */
 	omap2_mcspi_set_enable(spi, 0);
 
 	elements = element_count - 1;
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] [PATCH v2 3/3] spi: omap2-mcspi: Use the SPI framework to handle DMA mapping

[Franklin S Cooper Jr]

Currently, the driver handles mapping buffers to be used by the DMA.
However, there are times that the current mapping implementation will
fail for certain buffers. Fortunately, the SPI framework can detect
and map buffers so its usable by the DMA.

Update the driver to utilize the SPI framework for buffer
mapping instead. Also incorporate hooks that the framework uses to
determine if the DMA can or can not be used.

This will result in the original omap2_mcspi_transfer_one function being
deleted and omap2_mcspi_work_one being renamed to
omap2_mcspi_transfer_one. Previously transfer_one was only responsible
for mapping and work_one handled the transfer. But now only transferring
needs to be handled by the driver.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/spi/spi-omap2-mcspi.c | 107 ++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 73 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index c47f958..e26dabf 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -419,16 +419,13 @@ static void omap2_mcspi_tx_dma(struct spi_device *spi,
 
 	if (mcspi_dma->dma_tx) {
 		struct dma_async_tx_descriptor *tx;
-		struct scatterlist sg;
 
 		dmaengine_slave_config(mcspi_dma->dma_tx, &cfg);
 
-		sg_init_table(&sg, 1);
-		sg_dma_address(&sg) = xfer->tx_dma;
-		sg_dma_len(&sg) = xfer->len;
-
-		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_tx, &sg, 1,
-		DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_tx, xfer->tx_sg.sgl,
+					     xfer->tx_sg.nents,
+					     DMA_MEM_TO_DEV,
+					     DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 		if (tx) {
 			tx->callback = omap2_mcspi_tx_callback;
 			tx->callback_param = spi;
@@ -449,7 +446,8 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 {
 	struct omap2_mcspi	*mcspi;
 	struct omap2_mcspi_dma  *mcspi_dma;
-	unsigned int		count, dma_count;
+	struct sg_table		*cloned_table = NULL;
+	unsigned int		count, transfer_reduction = 0;
 	u32			l;
 	int			elements = 0;
 	int			word_len, element_count;
@@ -457,7 +455,6 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	mcspi = spi_master_get_devdata(spi->master);
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 	count = xfer->len;
-	dma_count = xfer->len;
 
 	/*
 	 *  In the "End-of-Transfer Procedure" section for DMA RX in OMAP35x TRM
@@ -465,7 +462,7 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	 *  normal mode.
 	 */
 	if (mcspi->fifo_depth == 0)
-		dma_count -= es;
+		transfer_reduction = es;
 
 	word_len = cs->word_len;
 	l = mcspi_cached_chconf0(spi);
@@ -479,7 +476,6 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 
 	if (mcspi_dma->dma_rx) {
 		struct dma_async_tx_descriptor *tx;
-		struct scatterlist sg;
 
 		dmaengine_slave_config(mcspi_dma->dma_rx, &cfg);
 
@@ -488,13 +484,21 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 		 *  configured in turbo mode.
 		 */
 		if ((l & OMAP2_MCSPI_CHCONF_TURBO) && mcspi->fifo_depth == 0)
-			dma_count -= es;
+			transfer_reduction += es;
+
+		/*
+		 * Get a cloned copy of the rx sgl whose transfer count has
+		 * has been reduced by transfer_reduction.
+		 */
+		cloned_table = sg_table_clone(&xfer->rx_sg,
+				count - transfer_reduction, GFP_KERNEL);
 
-		sg_init_table(&sg, 1);
-		sg_dma_address(&sg) = xfer->rx_dma;
-		sg_dma_len(&sg) = dma_count;
+		if (IS_ERR(cloned_table))
+			return 0;
 
-		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_rx, &sg, 1,
+		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_rx,
+				cloned_table->sgl,
+				cloned_table->nents,
 				DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT |
 				DMA_CTRL_ACK);
 		if (tx) {
@@ -510,8 +514,8 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	omap2_mcspi_set_dma_req(spi, 1, 1);
 
 	wait_for_completion(&mcspi_dma->dma_rx_completion);
-	dma_unmap_single(mcspi->dev, xfer->rx_dma, count,
-			 DMA_FROM_DEVICE);
+
+	sg_free_table(cloned_table);
 
 	if (mcspi->fifo_depth > 0)
 		return count;
@@ -628,8 +632,6 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 
 	if (tx != NULL) {
 		wait_for_completion(&mcspi_dma->dma_tx_completion);
-		dma_unmap_single(mcspi->dev, xfer->tx_dma, xfer->len,
-				 DMA_TO_DEVICE);
 
 		if (mcspi->fifo_depth > 0) {
 			irqstat_reg = mcspi->base + OMAP2_MCSPI_IRQSTATUS;
@@ -1087,7 +1089,7 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
 		gpio_free(spi->cs_gpio);
 }
 
-static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
+static int omap2_mcspi_transfer_one(struct spi_master *master,
 		struct spi_device *spi, struct spi_transfer *t)
 {
 
@@ -1098,7 +1100,7 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 	 * chipselect with the FORCE bit ... CS != channel enable.
 	 */
 
-	struct spi_master		*master;
+	struct omap2_mcspi		*mcspi;
 	struct omap2_mcspi_dma		*mcspi_dma;
 	struct omap2_mcspi_cs		*cs;
 	struct omap2_mcspi_device_config *cd;
@@ -1106,7 +1108,7 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 	int				status = 0;
 	u32				chconf;
 
-	master = spi->master;
+	mcspi = spi_master_get_devdata(master);
 	mcspi_dma = mcspi->dma_channels + spi->chip_select;
 	cs = spi->controller_state;
 	cd = spi->controller_data;
@@ -1166,7 +1168,8 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 		unsigned	count;
 
 		if ((mcspi_dma->dma_rx && mcspi_dma->dma_tx) &&
-		    (t->len >= DMA_MIN_BYTES))
+		    master->cur_msg_mapped &&
+		    master->can_dma(master, spi, t))
 			omap2_mcspi_set_fifo(spi, t, 1);
 
 		omap2_mcspi_set_enable(spi, 1);
@@ -1177,7 +1180,8 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 					+ OMAP2_MCSPI_TX0);
 
 		if ((mcspi_dma->dma_rx && mcspi_dma->dma_tx) &&
-		    (t->len >= DMA_MIN_BYTES))
+		    master->cur_msg_mapped &&
+		    master->can_dma(master, spi, t))
 			count = omap2_mcspi_txrx_dma(spi, t);
 		else
 			count = omap2_mcspi_txrx_pio(spi, t);
@@ -1246,55 +1250,11 @@ static int omap2_mcspi_prepare_message(struct spi_master *master,
 	return 0;
 }
 
-static int omap2_mcspi_transfer_one(struct spi_master *master,
-		struct spi_device *spi, struct spi_transfer *t)
+static bool omap2_mcspi_can_dma(struct spi_master *master,
+				struct spi_device *spi,
+				struct spi_transfer *xfer)
 {
-	struct omap2_mcspi	*mcspi;
-	struct omap2_mcspi_dma	*mcspi_dma;
-	const void	*tx_buf = t->tx_buf;
-	void		*rx_buf = t->rx_buf;
-	unsigned	len = t->len;
-
-	mcspi = spi_master_get_devdata(master);
-	mcspi_dma = mcspi->dma_channels + spi->chip_select;
-
-	if ((len && !(rx_buf || tx_buf))) {
-		dev_dbg(mcspi->dev, "transfer: %d Hz, %d %s%s, %d bpw\n",
-				t->speed_hz,
-				len,
-				tx_buf ? "tx" : "",
-				rx_buf ? "rx" : "",
-				t->bits_per_word);
-		return -EINVAL;
-	}
-
-	if (len < DMA_MIN_BYTES)
-		goto skip_dma_map;
-
-	if (mcspi_dma->dma_tx && tx_buf != NULL) {
-		t->tx_dma = dma_map_single(mcspi->dev, (void *) tx_buf,
-				len, DMA_TO_DEVICE);
-		if (dma_mapping_error(mcspi->dev, t->tx_dma)) {
-			dev_dbg(mcspi->dev, "dma %cX %d bytes error\n",
-					'T', len);
-			return -EINVAL;
-		}
-	}
-	if (mcspi_dma->dma_rx && rx_buf != NULL) {
-		t->rx_dma = dma_map_single(mcspi->dev, rx_buf, t->len,
-				DMA_FROM_DEVICE);
-		if (dma_mapping_error(mcspi->dev, t->rx_dma)) {
-			dev_dbg(mcspi->dev, "dma %cX %d bytes error\n",
-					'R', len);
-			if (tx_buf != NULL)
-				dma_unmap_single(mcspi->dev, t->tx_dma,
-						len, DMA_TO_DEVICE);
-			return -EINVAL;
-		}
-	}
-
-skip_dma_map:
-	return omap2_mcspi_work_one(mcspi, spi, t);
+	return (xfer->len >= DMA_MIN_BYTES);
 }
 
 static int omap2_mcspi_master_setup(struct omap2_mcspi *mcspi)
@@ -1374,6 +1334,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	master->setup = omap2_mcspi_setup;
 	master->auto_runtime_pm = true;
 	master->prepare_message = omap2_mcspi_prepare_message;
+	master->can_dma = omap2_mcspi_can_dma;
 	master->transfer_one = omap2_mcspi_transfer_one;
 	master->set_cs = omap2_mcspi_set_cs;
 	master->cleanup = omap2_mcspi_cleanup;
-- 
2.7.0

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On Mon, Jun 27, 2016 at 09:54:07AM -0500, Franklin S Cooper Jr wrote:
> Occasionally there are times you need to tweak a chained S/G list while
> maintaining the original list. This function will duplicate the passed
> in chained S/G list and return a pointer to the cloned copy.

This looks reasonable to me and there's not been any other comment over
a fairly extended period (this wasn't the first posting) so unless
someone screams I'll probably go ahead and apply this soon.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On Mon, Jun 27, 2016 at 09:54:07AM -0500, Franklin S Cooper Jr wrote:
> Occasionally there are times you need to tweak a chained S/G list while
> maintaining the original list. This function will duplicate the passed
> in chained S/G list and return a pointer to the cloned copy.

This looks reasonable to me and there's not been any other comment over
a fairly extended period (this wasn't the first posting) so unless
someone screams I'll probably go ahead and apply this soon.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Andy Shevchenko]

On Tue, Jul 5, 2016 at 5:49 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 27, 2016 at 09:54:07AM -0500, Franklin S Cooper Jr wrote:
>> Occasionally there are times you need to tweak a chained S/G list while
>> maintaining the original list. This function will duplicate the passed
>> in chained S/G list and return a pointer to the cloned copy.
>
> This looks reasonable to me and there's not been any other comment over
> a fairly extended period (this wasn't the first posting) so unless
> someone screams I'll probably go ahead and apply this soon.

It has style issues I suppose. Extra empty lines, for example.

I will comment more.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Andy Shevchenko]

On Mon, Jun 27, 2016 at 5:54 PM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
> Occasionally there are times you need to tweak a chained S/G list while
> maintaining the original list. This function will duplicate the passed
> in chained S/G list and return a pointer to the cloned copy.
>
> The function also supports passing in a length that can be equal or less
> than the original chained S/G list length. Reducing the length can result
> in less entries of the chained S/G list being created.

My comments below.

> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +                               gfp_t gfp_mask);

size_t len ?
Or it would be commented somewhere why u64.

> +/*
> + * sg_clone -  Duplicate an existing chained sgl
> + * @orig_sgl:  Original sg list to be duplicated
> + * @len:       Total length of sg while taking chaining into account
> + * @gfp_mask:  GFP allocation mask
> + *
> + * Description:
> + *   Clone a chained sgl. This cloned copy may be modified in some ways while
> + *   keeping the original sgl in tact. Also allow the cloned copy to have
> + *   a smaller length than the original which may reduce the sgl total
> + *   sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg list, ERR_PTR() on error

Maybe "...new allocated SG list using kmalloc()..." ?

> + *

Extra line.

> + */
> +static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
> +                                   gfp_t gfp_mask)
> +{
> +       unsigned int            nents;
> +       bool                    last_entry;
> +       struct scatterlist      *sgl, *head;
> +
> +       nents = sg_nents_for_len(orig_sgl, len);
> +

Ditto.

> +       if (nents < 0)

> +               return ERR_PTR(-EINVAL);

return ERR_PTR(nents);

> +
> +       head = sg_kmalloc(nents, gfp_mask);
> +

Extra line.

> +       if (!head)
> +               return ERR_PTR(-ENOMEM);
> +

> +       sgl = head;
> +
> +       sg_init_table(sgl, nents);
> +
> +       for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {

Maybe

  sg_init_table(head, nents);

  for (sgl = head; sgl; ...

> +
> +               last_entry = sg_is_last(sgl);
> +               *sgl = *orig_sgl;
> +
> +               /*
> +                * If page_link is pointing to a chained sgl then set it to
> +                * zero since we already compensated for chained sgl. If
> +                * page_link is pointing to a page then clear bits 1 and 0.
> +                */
> +               if (sg_is_chain(orig_sgl))
> +                       sgl->page_link = 0x0;
> +               else
> +                       sgl->page_link &= ~0x03;
> +

> +               if (last_entry) {
> +                       sg_dma_len(sgl) = len;

Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
will set dma_length field and len otherwise. Is it on purpose?

> +                       /* Set bit 1 to indicate end of sgl */
> +                       sgl->page_link |= 0x02;
> +               } else {

> +                       len -= sg_dma_len(sgl);

Ditto.

> +               }
> +       }
> +
> +       return head;
> +}
> +
> +/*
> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
> + * @orig_table:     Original sg_table to be duplicated
> + * @len:            Total length of sg while taking chaining into account
> + * @gfp_mask:       GFP allocation mask
> + *
> + * Description:
> + *   Clone a sg_table along with chained sgl. This cloned copy may be
> + *   modified in some ways while keeping the original table and sgl in tact.
> + *   Also allow the cloned sgl copy to have a smaller length than the original
> + *   which may reduce the sgl total sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
> + *
> + */
> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +                               gfp_t gfp_mask)
> +{
> +       struct sg_table *table;
> +
> +       table = kmalloc(sizeof(struct sg_table), gfp_mask);
> +

Extra line.

> +       if (!table)
> +               return ERR_PTR(-ENOMEM);
> +
> +       table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
> +

Ditto.

> +       if (IS_ERR(table->sgl)) {
> +               kfree(table);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       table->nents = table->orig_nents = sg_nents(table->sgl);
> +
> +       return table;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Andy Shevchenko]

On Mon, Jun 27, 2016 at 5:54 PM, Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org> wrote:
> Occasionally there are times you need to tweak a chained S/G list while
> maintaining the original list. This function will duplicate the passed
> in chained S/G list and return a pointer to the cloned copy.
>
> The function also supports passing in a length that can be equal or less
> than the original chained S/G list length. Reducing the length can result
> in less entries of the chained S/G list being created.

My comments below.

> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +                               gfp_t gfp_mask);

size_t len ?
Or it would be commented somewhere why u64.

> +/*
> + * sg_clone -  Duplicate an existing chained sgl
> + * @orig_sgl:  Original sg list to be duplicated
> + * @len:       Total length of sg while taking chaining into account
> + * @gfp_mask:  GFP allocation mask
> + *
> + * Description:
> + *   Clone a chained sgl. This cloned copy may be modified in some ways while
> + *   keeping the original sgl in tact. Also allow the cloned copy to have
> + *   a smaller length than the original which may reduce the sgl total
> + *   sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg list, ERR_PTR() on error

Maybe "...new allocated SG list using kmalloc()..." ?

> + *

Extra line.

> + */
> +static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
> +                                   gfp_t gfp_mask)
> +{
> +       unsigned int            nents;
> +       bool                    last_entry;
> +       struct scatterlist      *sgl, *head;
> +
> +       nents = sg_nents_for_len(orig_sgl, len);
> +

Ditto.

> +       if (nents < 0)

> +               return ERR_PTR(-EINVAL);

return ERR_PTR(nents);

> +
> +       head = sg_kmalloc(nents, gfp_mask);
> +

Extra line.

> +       if (!head)
> +               return ERR_PTR(-ENOMEM);
> +

> +       sgl = head;
> +
> +       sg_init_table(sgl, nents);
> +
> +       for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {

Maybe

  sg_init_table(head, nents);

  for (sgl = head; sgl; ...

> +
> +               last_entry = sg_is_last(sgl);
> +               *sgl = *orig_sgl;
> +
> +               /*
> +                * If page_link is pointing to a chained sgl then set it to
> +                * zero since we already compensated for chained sgl. If
> +                * page_link is pointing to a page then clear bits 1 and 0.
> +                */
> +               if (sg_is_chain(orig_sgl))
> +                       sgl->page_link = 0x0;
> +               else
> +                       sgl->page_link &= ~0x03;
> +

> +               if (last_entry) {
> +                       sg_dma_len(sgl) = len;

Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
will set dma_length field and len otherwise. Is it on purpose?

> +                       /* Set bit 1 to indicate end of sgl */
> +                       sgl->page_link |= 0x02;
> +               } else {

> +                       len -= sg_dma_len(sgl);

Ditto.

> +               }
> +       }
> +
> +       return head;
> +}
> +
> +/*
> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
> + * @orig_table:     Original sg_table to be duplicated
> + * @len:            Total length of sg while taking chaining into account
> + * @gfp_mask:       GFP allocation mask
> + *
> + * Description:
> + *   Clone a sg_table along with chained sgl. This cloned copy may be
> + *   modified in some ways while keeping the original table and sgl in tact.
> + *   Also allow the cloned sgl copy to have a smaller length than the original
> + *   which may reduce the sgl total sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
> + *
> + */
> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +                               gfp_t gfp_mask)
> +{
> +       struct sg_table *table;
> +
> +       table = kmalloc(sizeof(struct sg_table), gfp_mask);
> +

Extra line.

> +       if (!table)
> +               return ERR_PTR(-ENOMEM);
> +
> +       table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
> +

Ditto.

> +       if (IS_ERR(table->sgl)) {
> +               kfree(table);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       table->nents = table->orig_nents = sg_nents(table->sgl);
> +
> +       return table;
> +}

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Sekhar Nori]

On Monday 27 June 2016 08:24 PM, Franklin S Cooper Jr wrote:

> +/*
> + * sg_table_clone - Duplicate an existing sg_table including chained sgl

This function should probably be called sg_clone_table() to be
consistent with sg_alloc_table(), sg_free_table() etc.

> + * @orig_table:     Original sg_table to be duplicated
> + * @len:            Total length of sg while taking chaining into account
> + * @gfp_mask:       GFP allocation mask
> + *
> + * Description:
> + *   Clone a sg_table along with chained sgl. This cloned copy may be
> + *   modified in some ways while keeping the original table and sgl in tact.
> + *   Also allow the cloned sgl copy to have a smaller length than the original
> + *   which may reduce the sgl total sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
> + *
> + */
> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +				gfp_t gfp_mask)
> +{
> +	struct sg_table	*table;
> +
> +	table = kmalloc(sizeof(struct sg_table), gfp_mask);

Can you use sg_alloc_table() to allocate the new table? The way you have
it now, it looks like users will need to use kfree() to free a cloned
table and use sg_free_table() otherwise. It will be nice if
sg_free_table() can be used consistently.

Regards,
Sekhar

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Sekhar Nori]

On Monday 27 June 2016 08:24 PM, Franklin S Cooper Jr wrote:

> +/*
> + * sg_table_clone - Duplicate an existing sg_table including chained sgl

This function should probably be called sg_clone_table() to be
consistent with sg_alloc_table(), sg_free_table() etc.

> + * @orig_table:     Original sg_table to be duplicated
> + * @len:            Total length of sg while taking chaining into account
> + * @gfp_mask:       GFP allocation mask
> + *
> + * Description:
> + *   Clone a sg_table along with chained sgl. This cloned copy may be
> + *   modified in some ways while keeping the original table and sgl in tact.
> + *   Also allow the cloned sgl copy to have a smaller length than the original
> + *   which may reduce the sgl total sg entries.
> + *
> + * Returns:
> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
> + *
> + */
> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
> +				gfp_t gfp_mask)
> +{
> +	struct sg_table	*table;
> +
> +	table = kmalloc(sizeof(struct sg_table), gfp_mask);

Can you use sg_alloc_table() to allocate the new table? The way you have
it now, it looks like users will need to use kfree() to free a cloned
table and use sg_free_table() otherwise. It will be nice if
sg_free_table() can be used consistently.

Regards,
Sekhar

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/05/2016 12:10 PM, Andy Shevchenko wrote:
> On Mon, Jun 27, 2016 at 5:54 PM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>> Occasionally there are times you need to tweak a chained S/G list while
>> maintaining the original list. This function will duplicate the passed
>> in chained S/G list and return a pointer to the cloned copy.
>>
>> The function also supports passing in a length that can be equal or less
>> than the original chained S/G list length. Reducing the length can result
>> in less entries of the chained S/G list being created.
> 
> My comments below.
> 
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +                               gfp_t gfp_mask);
> 
> size_t len ?
> Or it would be commented somewhere why u64.


No reason other than sg_nents_for_len required u64. But I see other
users of that function using size_t so I'll fix this.

> 
>> +/*
>> + * sg_clone -  Duplicate an existing chained sgl
>> + * @orig_sgl:  Original sg list to be duplicated
>> + * @len:       Total length of sg while taking chaining into account
>> + * @gfp_mask:  GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a chained sgl. This cloned copy may be modified in some ways while
>> + *   keeping the original sgl in tact. Also allow the cloned copy to have
>> + *   a smaller length than the original which may reduce the sgl total
>> + *   sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg list, ERR_PTR() on error
> 
> Maybe "...new allocated SG list using kmalloc()..." ?


Will fix
> 
>> + *
> 
> Extra line.


Will fix along with all the other extra line comments.
> 
>> + */
>> +static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
>> +                                   gfp_t gfp_mask)
>> +{
>> +       unsigned int            nents;
>> +       bool                    last_entry;
>> +       struct scatterlist      *sgl, *head;
>> +
>> +       nents = sg_nents_for_len(orig_sgl, len);
>> +
> 
> Ditto.
> 
>> +       if (nents < 0)
> 
>> +               return ERR_PTR(-EINVAL);
> 
> return ERR_PTR(nents);


Will fix
> 
>> +
>> +       head = sg_kmalloc(nents, gfp_mask);
>> +
> 
> Extra line.
> 
>> +       if (!head)
>> +               return ERR_PTR(-ENOMEM);
>> +
> 
>> +       sgl = head;
>> +
>> +       sg_init_table(sgl, nents);
>> +
>> +       for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {
> 
> Maybe
> 
>   sg_init_table(head, nents);
> 
>   for (sgl = head; sgl; ...


Yeah that will probably look better. Will fix.
> 
>> +
>> +               last_entry = sg_is_last(sgl);
>> +               *sgl = *orig_sgl;
>> +
>> +               /*
>> +                * If page_link is pointing to a chained sgl then set it to
>> +                * zero since we already compensated for chained sgl. If
>> +                * page_link is pointing to a page then clear bits 1 and 0.
>> +                */
>> +               if (sg_is_chain(orig_sgl))
>> +                       sgl->page_link = 0x0;
>> +               else
>> +                       sgl->page_link &= ~0x03;
>> +
> 
>> +               if (last_entry) {
>> +                       sg_dma_len(sgl) = len;
> 
> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
> will set dma_length field and len otherwise. Is it on purpose?

Thanks for pointing this out. I didn't take into consideration the above
scenario.

After looking at this more it seems like on occasion dma_length !=
length. I see this occurring in various map_sg functions for several
architectures.

I'm simply trying to reduce the overall sgl length by a certain amount.
I'm not sure what the proper approach would be since dma_length and
length can be set to different values. Simply setting sgl->dma_length to
sgl->length or vice a versa seems like it can be problematic in some
scenarios. What confuses me even more is if dma_length != length then
how can sg_nents_for_len only use the sgl length property.

Any thoughts on what would be the best way to handle this?


How about something like the following to address your original question?

sgl->length = len

if (sgl->dma_address)
	sg_dma_len(sgl) = sgl->length
> 
>> +                       /* Set bit 1 to indicate end of sgl */
>> +                       sgl->page_link |= 0x02;
>> +               } else {
> 
>> +                       len -= sg_dma_len(sgl);

len -= sgl->length
> 
> Ditto.
> 
>> +               }
>> +       }
>> +
>> +       return head;
>> +}
>> +
>> +/*
>> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
>> + * @orig_table:     Original sg_table to be duplicated
>> + * @len:            Total length of sg while taking chaining into account
>> + * @gfp_mask:       GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a sg_table along with chained sgl. This cloned copy may be
>> + *   modified in some ways while keeping the original table and sgl in tact.
>> + *   Also allow the cloned sgl copy to have a smaller length than the original
>> + *   which may reduce the sgl total sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
>> + *
>> + */
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +                               gfp_t gfp_mask)
>> +{
>> +       struct sg_table *table;
>> +
>> +       table = kmalloc(sizeof(struct sg_table), gfp_mask);
>> +
> 
> Extra line.
> 
>> +       if (!table)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
>> +
> 
> Ditto.
> 
>> +       if (IS_ERR(table->sgl)) {
>> +               kfree(table);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>> +
>> +       table->nents = table->orig_nents = sg_nents(table->sgl);
>> +
>> +       return table;
>> +}
> 

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/05/2016 12:10 PM, Andy Shevchenko wrote:
> On Mon, Jun 27, 2016 at 5:54 PM, Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org> wrote:
>> Occasionally there are times you need to tweak a chained S/G list while
>> maintaining the original list. This function will duplicate the passed
>> in chained S/G list and return a pointer to the cloned copy.
>>
>> The function also supports passing in a length that can be equal or less
>> than the original chained S/G list length. Reducing the length can result
>> in less entries of the chained S/G list being created.
> 
> My comments below.
> 
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +                               gfp_t gfp_mask);
> 
> size_t len ?
> Or it would be commented somewhere why u64.


No reason other than sg_nents_for_len required u64. But I see other
users of that function using size_t so I'll fix this.

> 
>> +/*
>> + * sg_clone -  Duplicate an existing chained sgl
>> + * @orig_sgl:  Original sg list to be duplicated
>> + * @len:       Total length of sg while taking chaining into account
>> + * @gfp_mask:  GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a chained sgl. This cloned copy may be modified in some ways while
>> + *   keeping the original sgl in tact. Also allow the cloned copy to have
>> + *   a smaller length than the original which may reduce the sgl total
>> + *   sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg list, ERR_PTR() on error
> 
> Maybe "...new allocated SG list using kmalloc()..." ?


Will fix
> 
>> + *
> 
> Extra line.


Will fix along with all the other extra line comments.
> 
>> + */
>> +static struct scatterlist *sg_clone(struct scatterlist *orig_sgl, u64 len,
>> +                                   gfp_t gfp_mask)
>> +{
>> +       unsigned int            nents;
>> +       bool                    last_entry;
>> +       struct scatterlist      *sgl, *head;
>> +
>> +       nents = sg_nents_for_len(orig_sgl, len);
>> +
> 
> Ditto.
> 
>> +       if (nents < 0)
> 
>> +               return ERR_PTR(-EINVAL);
> 
> return ERR_PTR(nents);


Will fix
> 
>> +
>> +       head = sg_kmalloc(nents, gfp_mask);
>> +
> 
> Extra line.
> 
>> +       if (!head)
>> +               return ERR_PTR(-ENOMEM);
>> +
> 
>> +       sgl = head;
>> +
>> +       sg_init_table(sgl, nents);
>> +
>> +       for (; sgl; orig_sgl = sg_next(orig_sgl), sgl = sg_next(sgl)) {
> 
> Maybe
> 
>   sg_init_table(head, nents);
> 
>   for (sgl = head; sgl; ...


Yeah that will probably look better. Will fix.
> 
>> +
>> +               last_entry = sg_is_last(sgl);
>> +               *sgl = *orig_sgl;
>> +
>> +               /*
>> +                * If page_link is pointing to a chained sgl then set it to
>> +                * zero since we already compensated for chained sgl. If
>> +                * page_link is pointing to a page then clear bits 1 and 0.
>> +                */
>> +               if (sg_is_chain(orig_sgl))
>> +                       sgl->page_link = 0x0;
>> +               else
>> +                       sgl->page_link &= ~0x03;
>> +
> 
>> +               if (last_entry) {
>> +                       sg_dma_len(sgl) = len;
> 
> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
> will set dma_length field and len otherwise. Is it on purpose?

Thanks for pointing this out. I didn't take into consideration the above
scenario.

After looking at this more it seems like on occasion dma_length !=
length. I see this occurring in various map_sg functions for several
architectures.

I'm simply trying to reduce the overall sgl length by a certain amount.
I'm not sure what the proper approach would be since dma_length and
length can be set to different values. Simply setting sgl->dma_length to
sgl->length or vice a versa seems like it can be problematic in some
scenarios. What confuses me even more is if dma_length != length then
how can sg_nents_for_len only use the sgl length property.

Any thoughts on what would be the best way to handle this?


How about something like the following to address your original question?

sgl->length = len

if (sgl->dma_address)
	sg_dma_len(sgl) = sgl->length
> 
>> +                       /* Set bit 1 to indicate end of sgl */
>> +                       sgl->page_link |= 0x02;
>> +               } else {
> 
>> +                       len -= sg_dma_len(sgl);

len -= sgl->length
> 
> Ditto.
> 
>> +               }
>> +       }
>> +
>> +       return head;
>> +}
>> +
>> +/*
>> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
>> + * @orig_table:     Original sg_table to be duplicated
>> + * @len:            Total length of sg while taking chaining into account
>> + * @gfp_mask:       GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a sg_table along with chained sgl. This cloned copy may be
>> + *   modified in some ways while keeping the original table and sgl in tact.
>> + *   Also allow the cloned sgl copy to have a smaller length than the original
>> + *   which may reduce the sgl total sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
>> + *
>> + */
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +                               gfp_t gfp_mask)
>> +{
>> +       struct sg_table *table;
>> +
>> +       table = kmalloc(sizeof(struct sg_table), gfp_mask);
>> +
> 
> Extra line.
> 
>> +       if (!table)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       table->sgl = sg_clone(orig_table->sgl, len, gfp_mask);
>> +
> 
> Ditto.
> 
>> +       if (IS_ERR(table->sgl)) {
>> +               kfree(table);
>> +               return ERR_PTR(-ENOMEM);
>> +       }
>> +
>> +       table->nents = table->orig_nents = sg_nents(table->sgl);
>> +
>> +       return table;
>> +}
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 05:15 AM, Sekhar Nori wrote:
> On Monday 27 June 2016 08:24 PM, Franklin S Cooper Jr wrote:
> 
>> +/*
>> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
> 
> This function should probably be called sg_clone_table() to be
> consistent with sg_alloc_table(), sg_free_table() etc.


Will fix.
> 
>> + * @orig_table:     Original sg_table to be duplicated
>> + * @len:            Total length of sg while taking chaining into account
>> + * @gfp_mask:       GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a sg_table along with chained sgl. This cloned copy may be
>> + *   modified in some ways while keeping the original table and sgl in tact.
>> + *   Also allow the cloned sgl copy to have a smaller length than the original
>> + *   which may reduce the sgl total sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
>> + *
>> + */
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +				gfp_t gfp_mask)
>> +{
>> +	struct sg_table	*table;
>> +
>> +	table = kmalloc(sizeof(struct sg_table), gfp_mask);
> 
> Can you use sg_alloc_table() to allocate the new table? The way you have
> it now, it looks like users will need to use kfree() to free a cloned
> table and use sg_free_table() otherwise. It will be nice if
> sg_free_table() can be used consistently.


Sg_alloc_table doesn't actually allocate a struct sg_table. It allocates
a single sgl or chained sgl. Drivers that use sg_table manually allocate
it via kmalloc. One change that might make sense is to not kmalloc
sg_table but instead have a user pass a pointer to a preallocated
instance of it. This way it will mimic how sg_table is typically handled.
> 
> Regards,
> Sekhar
> 

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 05:15 AM, Sekhar Nori wrote:
> On Monday 27 June 2016 08:24 PM, Franklin S Cooper Jr wrote:
> 
>> +/*
>> + * sg_table_clone - Duplicate an existing sg_table including chained sgl
> 
> This function should probably be called sg_clone_table() to be
> consistent with sg_alloc_table(), sg_free_table() etc.


Will fix.
> 
>> + * @orig_table:     Original sg_table to be duplicated
>> + * @len:            Total length of sg while taking chaining into account
>> + * @gfp_mask:       GFP allocation mask
>> + *
>> + * Description:
>> + *   Clone a sg_table along with chained sgl. This cloned copy may be
>> + *   modified in some ways while keeping the original table and sgl in tact.
>> + *   Also allow the cloned sgl copy to have a smaller length than the original
>> + *   which may reduce the sgl total sg entries.
>> + *
>> + * Returns:
>> + *   Pointer to new kmalloced sg_table, ERR_PTR() on error
>> + *
>> + */
>> +struct sg_table *sg_table_clone(struct sg_table *orig_table, u64 len,
>> +				gfp_t gfp_mask)
>> +{
>> +	struct sg_table	*table;
>> +
>> +	table = kmalloc(sizeof(struct sg_table), gfp_mask);
> 
> Can you use sg_alloc_table() to allocate the new table? The way you have
> it now, it looks like users will need to use kfree() to free a cloned
> table and use sg_free_table() otherwise. It will be nice if
> sg_free_table() can be used consistently.


Sg_alloc_table doesn't actually allocate a struct sg_table. It allocates
a single sgl or chained sgl. Drivers that use sg_table manually allocate
it via kmalloc. One change that might make sense is to not kmalloc
sg_table but instead have a user pass a pointer to a preallocated
instance of it. This way it will mimic how sg_table is typically handled.
> 
> Regards,
> Sekhar
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Andy Shevchenko]

On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:

>>> +               last_entry = sg_is_last(sgl);
>>> +               *sgl = *orig_sgl;
>>> +
>>> +               /*
>>> +                * If page_link is pointing to a chained sgl then set it to
>>> +                * zero since we already compensated for chained sgl. If
>>> +                * page_link is pointing to a page then clear bits 1 and 0.
>>> +                */
>>> +               if (sg_is_chain(orig_sgl))
>>> +                       sgl->page_link = 0x0;
>>> +               else
>>> +                       sgl->page_link &= ~0x03;
>>> +
>>
>>> +               if (last_entry) {
>>> +                       sg_dma_len(sgl) = len;
>>
>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
>> will set dma_length field and len otherwise. Is it on purpose?
>
> Thanks for pointing this out. I didn't take into consideration the above
> scenario.
>
> After looking at this more it seems like on occasion dma_length !=
> length. I see this occurring in various map_sg functions for several
> architectures.
>
> I'm simply trying to reduce the overall sgl length by a certain amount.
> I'm not sure what the proper approach would be since dma_length and
> length can be set to different values. Simply setting sgl->dma_length to
> sgl->length or vice a versa seems like it can be problematic in some
> scenarios. What confuses me even more is if dma_length != length then
> how can sg_nents_for_len only use the sgl length property.
>
> Any thoughts on what would be the best way to handle this?

First come to my mind is to refuse to clone if SG table is DMA mapped.

Imagine the scenario where you have for example last entry which your
caller asked to split as

total_entry_len = 37235 (input)
dma_length = 65536 (64k alignment)

asked_len = 32768 (Split chunks: 32768 + 4467)
resulting_dma_len = ?

What do you put here? Initially it perhaps means the same 64k. But how
to distinguish?
(I couldn't come with better one, but there are many alike scenarios)

Only hack we have is to supply struct device of the DMA device to this
SG table where you can get max segment size (but I dunno about
alignment).

P.S. I'm not familiar of the details of all these.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 12:46 PM, Andy Shevchenko wrote:
> On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
> 
>>>> +               last_entry = sg_is_last(sgl);
>>>> +               *sgl = *orig_sgl;
>>>> +
>>>> +               /*
>>>> +                * If page_link is pointing to a chained sgl then set it to
>>>> +                * zero since we already compensated for chained sgl. If
>>>> +                * page_link is pointing to a page then clear bits 1 and 0.
>>>> +                */
>>>> +               if (sg_is_chain(orig_sgl))
>>>> +                       sgl->page_link = 0x0;
>>>> +               else
>>>> +                       sgl->page_link &= ~0x03;
>>>> +
>>>
>>>> +               if (last_entry) {
>>>> +                       sg_dma_len(sgl) = len;
>>>
>>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
>>> will set dma_length field and len otherwise. Is it on purpose?
>>
>> Thanks for pointing this out. I didn't take into consideration the above
>> scenario.
>>
>> After looking at this more it seems like on occasion dma_length !=
>> length. I see this occurring in various map_sg functions for several
>> architectures.
>>
>> I'm simply trying to reduce the overall sgl length by a certain amount.
>> I'm not sure what the proper approach would be since dma_length and
>> length can be set to different values. Simply setting sgl->dma_length to
>> sgl->length or vice a versa seems like it can be problematic in some
>> scenarios. What confuses me even more is if dma_length != length then
>> how can sg_nents_for_len only use the sgl length property.
>>
>> Any thoughts on what would be the best way to handle this?
> 
> First come to my mind is to refuse to clone if SG table is DMA mapped.
> 
> Imagine the scenario where you have for example last entry which your
> caller asked to split as
> 
> total_entry_len = 37235 (input)
> dma_length = 65536 (64k alignment)


Do you know of an example where dma_length will be set to some kind of
alignment size rather than based on the number of bytes you want
transfered? Or maybe I'm miss understanding this.

> 
> asked_len = 32768 (Split chunks: 32768 + 4467)
> resulting_dma_len = ?
> 
> What do you put here? Initially it perhaps means the same 64k. But how
> to distinguish?
> (I couldn't come with better one, but there are many alike scenarios)
> 
> Only hack we have is to supply struct device of the DMA device to this
> SG table where you can get max segment size (but I dunno about
> alignment).
> 
> P.S. I'm not familiar of the details of all these.
> 

Unfortunately, for the original purpose of this series the scatterlist I
want to clone is indeed DMA mapped.

The times I've seen dma_length != length is when the dma_map_sg is
trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
So maybe it is better to subtract a value from length and dma_length
rather than explicitly setting it to a value. This way it wouldn't
matter that dma_length and length aren't equal.

This looks like a similar approach used by sg_split_mapped. Although
sg_split skips first x number of bytes while I want to skip the last x
number of bytes.

Maybe Robert can add his thoughts since he created sg_split.

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 12:46 PM, Andy Shevchenko wrote:
> On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. <fcooper-l0cyMroinI0@public.gmane.org> wrote:
> 
>>>> +               last_entry = sg_is_last(sgl);
>>>> +               *sgl = *orig_sgl;
>>>> +
>>>> +               /*
>>>> +                * If page_link is pointing to a chained sgl then set it to
>>>> +                * zero since we already compensated for chained sgl. If
>>>> +                * page_link is pointing to a page then clear bits 1 and 0.
>>>> +                */
>>>> +               if (sg_is_chain(orig_sgl))
>>>> +                       sgl->page_link = 0x0;
>>>> +               else
>>>> +                       sgl->page_link &= ~0x03;
>>>> +
>>>
>>>> +               if (last_entry) {
>>>> +                       sg_dma_len(sgl) = len;
>>>
>>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
>>> will set dma_length field and len otherwise. Is it on purpose?
>>
>> Thanks for pointing this out. I didn't take into consideration the above
>> scenario.
>>
>> After looking at this more it seems like on occasion dma_length !=
>> length. I see this occurring in various map_sg functions for several
>> architectures.
>>
>> I'm simply trying to reduce the overall sgl length by a certain amount.
>> I'm not sure what the proper approach would be since dma_length and
>> length can be set to different values. Simply setting sgl->dma_length to
>> sgl->length or vice a versa seems like it can be problematic in some
>> scenarios. What confuses me even more is if dma_length != length then
>> how can sg_nents_for_len only use the sgl length property.
>>
>> Any thoughts on what would be the best way to handle this?
> 
> First come to my mind is to refuse to clone if SG table is DMA mapped.
> 
> Imagine the scenario where you have for example last entry which your
> caller asked to split as
> 
> total_entry_len = 37235 (input)
> dma_length = 65536 (64k alignment)


Do you know of an example where dma_length will be set to some kind of
alignment size rather than based on the number of bytes you want
transfered? Or maybe I'm miss understanding this.

> 
> asked_len = 32768 (Split chunks: 32768 + 4467)
> resulting_dma_len = ?
> 
> What do you put here? Initially it perhaps means the same 64k. But how
> to distinguish?
> (I couldn't come with better one, but there are many alike scenarios)
> 
> Only hack we have is to supply struct device of the DMA device to this
> SG table where you can get max segment size (but I dunno about
> alignment).
> 
> P.S. I'm not familiar of the details of all these.
> 

Unfortunately, for the original purpose of this series the scatterlist I
want to clone is indeed DMA mapped.

The times I've seen dma_length != length is when the dma_map_sg is
trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
So maybe it is better to subtract a value from length and dma_length
rather than explicitly setting it to a value. This way it wouldn't
matter that dma_length and length aren't equal.

This looks like a similar approach used by sg_split_mapped. Although
sg_split skips first x number of bytes while I want to skip the last x
number of bytes.

Maybe Robert can add his thoughts since he created sg_split.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Robert Jarzmik]

"Franklin S Cooper Jr." <fcooper@ti.com> writes:

> Unfortunately, for the original purpose of this series the scatterlist I
> want to clone is indeed DMA mapped.
>
> The times I've seen dma_length != length is when the dma_map_sg is
> trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
> So maybe it is better to subtract a value from length and dma_length
> rather than explicitly setting it to a value. This way it wouldn't
> matter that dma_length and length aren't equal.
>
> This looks like a similar approach used by sg_split_mapped. Although
> sg_split skips first x number of bytes while I want to skip the last x
> number of bytes.
>
> Maybe Robert can add his thoughts since he created sg_split.

Hi,

I've not read it all, but the above chapter is right : dma_length might be
different from length when a dma mapping operation coallesced 2 sg entries into
a "DMA contiguous one". This coallescing might happen for at least for 2 reasons
as far as I know :
 - 2 sg entries are physically contiguous, ie :
   sg_phys(sga) + sga.length == sg_phys(sgb)
 - 2 sg entries are DMA contiguous when an IOMMU maps them contiguously, ie :
   sg_dma_address(sga) + sga.length == sg_dma_address(sgb)

Moreover, this 2 coallescing cases imply a "shift" between (page_link, length)
and (dma_address and dma_length). For example a mapped sglist might look like :
 -sg0: page_link=>page@0k, length=4096, dma_address=0, dma_length=8192
 -sg1: page_link=>page@4k, length=4096, dma_address=8192, dma_length=16
                                                          \=> see the shift here
                                                              coming from sg2
 -sg2: page_link=>page@8k, length=16, dma_address=0, dma_length=0

For these "tricky" cases, at the time I created sg_split I had done a tester as
well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
you might have a look, and the brain cost you'll pay to adapt it to test what
you want will hopefully pay off by the knowledge gained on scatterlist. It is
appended at the end of the mail.

Cheers.

-- 
Robert

---8>---
#include <assert.h>
#include <linux/scatterlist.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#define NB_MAX_SG 10

static void fatal(const char *fmt, ...)
{
	va_list ap;

	va_start(ap, fmt);
	vprintf(fmt, ap);
	exit(1);
	va_end(ap);
}

static int my_random(int max)
{
	double val = random() * max / RAND_MAX;

	return (int)val;
}

static int is_following(struct scatterlist *sg)
{
	return sg_phys(sg - 1) + sg[-1].length == sg_phys(sg);
}

static void print_sg(const char *prefix, struct scatterlist *sg_in, int nents)
{
	struct scatterlist *sg;
	int i;

	printf("%sPrinting sg %p:%d\n", prefix, sg_in, nents);
	for_each_sg(sg_in, sg, nents, i)
		printf("%s\t%s[%02d] page_link=0x%08x offset=0x%08x length=%6d dma=0x%08x dma_len=%6d\n",
		       prefix, i > 0 && is_following(sg) ? "-->" : "   ",
		       i, sg->page_link, sg->offset, sg->length,
		       sg_dma_address(sg), sg_dma_len(sg));
}

static int fill_random_sg(struct scatterlist *sg_in, int nents)
{
	struct scatterlist *sg;
	unsigned long phys;
	int i, new = 1;

	nents = 1 + my_random(nents - 1);
	for_each_sg(sg_in, sg, nents, i) {
		if (new) {
			/* sg->page_link = my_random(100000) & ~4095; */
			sg->page_link = 4096 * i;
			sg->length = 1024 * my_random(4);
			if (sg->length == 0)
				sg->length = 0x4 + my_random(4000);
			sg->offset = my_random(sg->length);
		} else {
			sg[-1].length &= ~0x3;
			sg[-1].offset &= ~0x3;
			phys = (sg_phys(sg - 1) + sg[-1].length) & ~0x3;
			sg->offset = phys % 4096;
			sg->page_link = phys - sg->offset;
			sg->length = 1024 * my_random(4);
			if (sg->length == 0)
				sg->length = 0x4 + my_random(4000);
		}

		new = (my_random(3) > 1) ? 0 : 1;
	}
	sg_mark_end(sg - 1);
	return nents;
}

static int map_random_sg(struct scatterlist *sg_in, int nents)
{
	struct scatterlist *sg, *sg_out;
	int i, follow, m = 0;
	unsigned long map_offs = 0x80000000;

	sg_out = sg_in;
	for_each_sg(sg_in, sg, nents, i) {
		if (i > 0 && is_following(sg))
			follow = 1;
		else
			follow = 0;

		if (follow) {
			sg_dma_len(sg_out - 1) += sg->length;
		} else {
			sg_dma_address(sg_out) = sg_phys(sg) + map_offs;
			sg_dma_len(sg_out) = sg->length;
			sg_out = sg_next(sg_out);
			m++;
		}
	}
	return m;
}

static int check_sgout_phys_continuity(struct scatterlist *sg_in, off_t skip, size_t size,
				       struct scatterlist *sg_out,
				       struct scatterlist **new_sg_in, off_t *new_skip)
{
	struct scatterlist *sg;
	int i, last_len, total_len = 0;
	unsigned long last_phys;
	int in_idx;

	for_each_sg(sg_out, sg, sg_nents(sg), i) {
		if (in_idx < 0) {
			last_phys = sg_phys(sg);
			last_len = sg->length;
			assert(last_phys == sg_phys(&sg_in[in_idx]));
		} else {
		}
	}
}

static int test_sg_split(struct scatterlist *sg_in, int nents, int mapped)
{
	struct scatterlist *sg, *sg_out[7];
	int ret, i, out_mapped_nents[7], nb_sizes = 7, span = 0, len;
	size_t sizes[7], tot_size;
	int sg_phys_span = 0, sg_map_span = 0;
	off_t skip;

	for_each_sg(sg_in, sg, nents, i)
		sg_phys_span += sg->length;
	for_each_sg(sg_in, sg, mapped, i)
		sg_map_span += sg_dma_len(sg);

	assert(sg_phys_span == sg_map_span);

	skip = my_random(sg_map_span);
	for (i = 0, span = 0; i< nb_sizes; i++) {
		sizes[i] = my_random(sg_phys_span / nb_sizes);
		span += sizes[i];
	}

	ret = sg_split(sg_in, mapped, skip, nb_sizes, sizes, sg_out,
		       out_mapped_nents, 0);
	printf("Test the split of sg(n=%d m=%d span=%d skip=%u) : sizes = [",
	       nents, mapped, sg_phys_span, skip);
	for (i = 0, tot_size = 0; i < nb_sizes; tot_size += sizes[i], i++)
		printf(" %d", sizes[i]);
	printf(" ] <=> [%u..%u] : %d\n", skip, skip + tot_size, ret);

	if (ret < 0)
		return ret;

	for (i = 0; i < nb_sizes; i++) {
		printf("\tsg[%02d] : mapped=%d\n", i, out_mapped_nents[i]);
		print_sg("\t\t", sg_out[i], sg_nents(sg_out[i]));
	}

	for (i = 0, sg = sg_in; i < nb_sizes; i++) {
		check_sgout_phys_continuity(sg, skip, sizes[i], sg_out[i], &sg, &len);
		skip += len;
	}

	for (i = 0; i < nb_sizes; i++)
		free(sg_out[i]);
}

int main(int argc, char **argv)
{
	struct scatterlist *sg_in;
	int m, n;
	unsigned int seed;

	if (argc == 1)
		seed = time(NULL);
	else
		seed = atoi(argv[1]);
	srandom(seed);
	printf("Executing: %s %u\n", argv[0], seed);

	sg_in = calloc(NB_MAX_SG, sizeof(struct scatterlist));
	if (!sg_in)
		fatal("Couldn't allocate sg_in\n");

	n = fill_random_sg(sg_in, NB_MAX_SG);
	m = map_random_sg(sg_in, n);
	print_sg("", sg_in, n);

	test_sg_split(sg_in, n, m);

	free(sg_in);
        return 0;
}

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Thu, Jul 07, 2016 at 12:04:33AM +0200, Robert Jarzmik wrote:

> For these "tricky" cases, at the time I created sg_split I had done a tester as
> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
> you might have a look, and the brain cost you'll pay to adapt it to test what
> you want will hopefully pay off by the knowledge gained on scatterlist. It is
> appended at the end of the mail.

Might be worth getting this into the kernel source, under tools/testing
perhaps?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Thu, Jul 07, 2016 at 12:04:33AM +0200, Robert Jarzmik wrote:

> For these "tricky" cases, at the time I created sg_split I had done a tester as
> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
> you might have a look, and the brain cost you'll pay to adapt it to test what
> you want will hopefully pay off by the knowledge gained on scatterlist. It is
> appended at the end of the mail.

Might be worth getting this into the kernel source, under tools/testing
perhaps?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 05:04 PM, Robert Jarzmik wrote:
> "Franklin S Cooper Jr." <fcooper@ti.com> writes:
> 
>> Unfortunately, for the original purpose of this series the scatterlist I
>> want to clone is indeed DMA mapped.
>>
>> The times I've seen dma_length != length is when the dma_map_sg is
>> trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
>> So maybe it is better to subtract a value from length and dma_length
>> rather than explicitly setting it to a value. This way it wouldn't
>> matter that dma_length and length aren't equal.
>>
>> This looks like a similar approach used by sg_split_mapped. Although
>> sg_split skips first x number of bytes while I want to skip the last x
>> number of bytes.
>>
>> Maybe Robert can add his thoughts since he created sg_split.
> 
> Hi,
> 
> I've not read it all, but the above chapter is right : dma_length might be
> different from length when a dma mapping operation coallesced 2 sg entries into
> a "DMA contiguous one". This coallescing might happen for at least for 2 reasons
> as far as I know :
>  - 2 sg entries are physically contiguous, ie :
>    sg_phys(sga) + sga.length == sg_phys(sgb)
>  - 2 sg entries are DMA contiguous when an IOMMU maps them contiguously, ie :
>    sg_dma_address(sga) + sga.length == sg_dma_address(sgb)
> 
> Moreover, this 2 coallescing cases imply a "shift" between (page_link, length)
> and (dma_address and dma_length). For example a mapped sglist might look like :
>  -sg0: page_link=>page@0k, length=4096, dma_address=0, dma_length=8192
>  -sg1: page_link=>page@4k, length=4096, dma_address=8192, dma_length=16
>                                                           \=> see the shift here
>                                                               coming from sg2
>  -sg2: page_link=>page@8k, length=16, dma_address=0, dma_length=0
> 
> For these "tricky" cases, at the time I created sg_split I had done a tester as
> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
> you might have a look, and the brain cost you'll pay to adapt it to test what
> you want will hopefully pay off by the knowledge gained on scatterlist. It is
> appended at the end of the mail.


Thanks Robert for your input and sharing your test program. After
looking at sg_split and test program I realized that I can use it
instead of creating my own clone function. Seems like you and your patch
reviewers already considered and dealt with the above complex scenario.

I updated my patchset to use sg_split rather than my custom function
since it seems to solve my original problem. I plan on sending out a v3
that incorporates this change.
> 
> Cheers.
> 

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 05:04 PM, Robert Jarzmik wrote:
> "Franklin S Cooper Jr." <fcooper-l0cyMroinI0@public.gmane.org> writes:
> 
>> Unfortunately, for the original purpose of this series the scatterlist I
>> want to clone is indeed DMA mapped.
>>
>> The times I've seen dma_length != length is when the dma_map_sg is
>> trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
>> So maybe it is better to subtract a value from length and dma_length
>> rather than explicitly setting it to a value. This way it wouldn't
>> matter that dma_length and length aren't equal.
>>
>> This looks like a similar approach used by sg_split_mapped. Although
>> sg_split skips first x number of bytes while I want to skip the last x
>> number of bytes.
>>
>> Maybe Robert can add his thoughts since he created sg_split.
> 
> Hi,
> 
> I've not read it all, but the above chapter is right : dma_length might be
> different from length when a dma mapping operation coallesced 2 sg entries into
> a "DMA contiguous one". This coallescing might happen for at least for 2 reasons
> as far as I know :
>  - 2 sg entries are physically contiguous, ie :
>    sg_phys(sga) + sga.length == sg_phys(sgb)
>  - 2 sg entries are DMA contiguous when an IOMMU maps them contiguously, ie :
>    sg_dma_address(sga) + sga.length == sg_dma_address(sgb)
> 
> Moreover, this 2 coallescing cases imply a "shift" between (page_link, length)
> and (dma_address and dma_length). For example a mapped sglist might look like :
>  -sg0: page_link=>page@0k, length=4096, dma_address=0, dma_length=8192
>  -sg1: page_link=>page@4k, length=4096, dma_address=8192, dma_length=16
>                                                           \=> see the shift here
>                                                               coming from sg2
>  -sg2: page_link=>page@8k, length=16, dma_address=0, dma_length=0
> 
> For these "tricky" cases, at the time I created sg_split I had done a tester as
> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
> you might have a look, and the brain cost you'll pay to adapt it to test what
> you want will hopefully pay off by the knowledge gained on scatterlist. It is
> appended at the end of the mail.


Thanks Robert for your input and sharing your test program. After
looking at sg_split and test program I realized that I can use it
instead of creating my own clone function. Seems like you and your patch
reviewers already considered and dealt with the above complex scenario.

I updated my patchset to use sg_split rather than my custom function
since it seems to solve my original problem. I plan on sending out a v3
that incorporates this change.
> 
> Cheers.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Thu, Jul 07, 2016 at 12:04:33AM +0200, Robert Jarzmik wrote:
>
>> For these "tricky" cases, at the time I created sg_split I had done a tester as
>> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
>> you might have a look, and the brain cost you'll pay to adapt it to test what
>> you want will hopefully pay off by the knowledge gained on scatterlist. It is
>> appended at the end of the mail.
>
> Might be worth getting this into the kernel source, under tools/testing
> perhaps?

Maybe so.

I'll try, but I don't trust much my chances of success, given that this tester :
 - should compile and link in $(TOP)/lib/scatterlist.c, as this is where
   sg_split() is defined
 - this implies all its includes
 - this implies at least these ones :
	bug.h
	mm.h
	scatterlist.h
	string.h
	types.h
 - this implies having page_to_phys and co. defined somewhere without
   draining the whole include/linux and include/asm* trees

For the tester, I had created an apart include/linux tree where all the includes
were _manually_ filled in with minimal content.

I don't know if an existing selftest had already this kind of problem,
ie. having to compile and link a kernel .c file, and that makes me feel this
might be difficult to keep a nice standalone tester.

Cheers.

-- 
Robert

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Robert Jarzmik]

Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> On Thu, Jul 07, 2016 at 12:04:33AM +0200, Robert Jarzmik wrote:
>
>> For these "tricky" cases, at the time I created sg_split I had done a tester as
>> well. It's very basic, doesn't cover all the corner cases, is a bit dumb, but
>> you might have a look, and the brain cost you'll pay to adapt it to test what
>> you want will hopefully pay off by the knowledge gained on scatterlist. It is
>> appended at the end of the mail.
>
> Might be worth getting this into the kernel source, under tools/testing
> perhaps?

Maybe so.

I'll try, but I don't trust much my chances of success, given that this tester :
 - should compile and link in $(TOP)/lib/scatterlist.c, as this is where
   sg_split() is defined
 - this implies all its includes
 - this implies at least these ones :
	bug.h
	mm.h
	scatterlist.h
	string.h
	types.h
 - this implies having page_to_phys and co. defined somewhere without
   draining the whole include/linux and include/asm* trees

For the tester, I had created an apart include/linux tree where all the includes
were _manually_ filled in with minimal content.

I don't know if an existing selftest had already this kind of problem,
ie. having to compile and link a kernel .c file, and that makes me feel this
might be difficult to keep a nice standalone tester.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thu, Jul 07, 2016 at 07:43:25PM +0200, Robert Jarzmik wrote:

> I'll try, but I don't trust much my chances of success, given that this tester :
>  - should compile and link in $(TOP)/lib/scatterlist.c, as this is where
>    sg_split() is defined
>  - this implies all its includes
>  - this implies at least these ones :
> 	bug.h
> 	mm.h
> 	scatterlist.h
> 	string.h
> 	types.h
>  - this implies having page_to_phys and co. defined somewhere without
>    draining the whole include/linux and include/asm* trees

> For the tester, I had created an apart include/linux tree where all the includes
> were _manually_ filled in with minimal content.

> I don't know if an existing selftest had already this kind of problem,
> ie. having to compile and link a kernel .c file, and that makes me feel this
> might be difficult to keep a nice standalone tester.

Right, that's messy :(  Could it be refactored as a boot/module load
time test so it could be built in the kernel environment?  Less
convenient to use (though KVM/UML help) but easier to build.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thu, Jul 07, 2016 at 07:43:25PM +0200, Robert Jarzmik wrote:

> I'll try, but I don't trust much my chances of success, given that this tester :
>  - should compile and link in $(TOP)/lib/scatterlist.c, as this is where
>    sg_split() is defined
>  - this implies all its includes
>  - this implies at least these ones :
> 	bug.h
> 	mm.h
> 	scatterlist.h
> 	string.h
> 	types.h
>  - this implies having page_to_phys and co. defined somewhere without
>    draining the whole include/linux and include/asm* trees

> For the tester, I had created an apart include/linux tree where all the includes
> were _manually_ filled in with minimal content.

> I don't know if an existing selftest had already this kind of problem,
> ie. having to compile and link a kernel .c file, and that makes me feel this
> might be difficult to keep a nice standalone tester.

Right, that's messy :(  Could it be refactored as a boot/module load
time test so it could be built in the kernel environment?  Less
convenient to use (though KVM/UML help) but easier to build.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Thu, Jul 07, 2016 at 07:43:25PM +0200, Robert Jarzmik wrote:
>
>> I'll try, but I don't trust much my chances of success, given that this tester :
>>  - should compile and link in $(TOP)/lib/scatterlist.c, as this is where
>>    sg_split() is defined
>>  - this implies all its includes
>>  - this implies at least these ones :
>> 	bug.h
>> 	mm.h
>> 	scatterlist.h
>> 	string.h
>> 	types.h
>>  - this implies having page_to_phys and co. defined somewhere without
>>    draining the whole include/linux and include/asm* trees
>
>> For the tester, I had created an apart include/linux tree where all the includes
>> were _manually_ filled in with minimal content.
>
>> I don't know if an existing selftest had already this kind of problem,
>> ie. having to compile and link a kernel .c file, and that makes me feel this
>> might be difficult to keep a nice standalone tester.
>
> Right, that's messy :(  Could it be refactored as a boot/module load
> time test so it could be built in the kernel environment?  Less
> convenient to use (though KVM/UML help) but easier to build.

Actually I thought a bit about it and I got a "messy" idea.

In order to keep things in userspace (for tests it really is more convenient),
I'm going to try this approach :
 - make tools/testing/selftests/lib/sg_split.c, based on the tester
 - make the Makefile generate a scatterlist_generated.c out of lib/scatterlist.c,
   where all the includes will be 'grepped; out and replaced by a single "#include
   sg_split.h" containing all the necessary defines
 - create the sg_split.h as a concatenation of all the .h files I had to use for
   the tester (I will reuse existing .h if it is applicable)

We'll see if that's feasible or not. As long as the "mess" is contained within
sg_split.h, and if it doesn't become a too ugly beast, it might work.

Cheers.

-- 
Robert