iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] dma_mapping: Add auto cleanup support
@ 2025-10-10 18:50 ` Frank Li
  2025-10-10 18:50   ` [PATCH 1/3] cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments Frank Li
                     ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Frank Li @ 2025-10-10 18:50 UTC (permalink / raw)
  To: Marek Szyprowski, Robin Murphy, Dong Aisheng, Andi Shyti,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
  Cc: linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel, Frank Li

There are many below pattern

	fun()
	{
		...
		dma_map_single();
		if (dma_mapping_error)
			goto err1;

		dmaengine_prep_slave_single()
		if (...)
			goto err2

		dmaengine_submit()
		if (...)
			goto err3

		wait_for_completion_timeout()
		if (...)
			goto err4

	err4:
	err3:
	err2:
		dma_umap_single();
	err1:
	}

Use cleanup can simple error handle like guard(), such as guard(mutex).
or __free(kfree) = kmalloc.

But dma_umap_single() need more argurements. So situation below complex.

It need pack argurments list into structure.

	#define __DEFINE_GUARD_CLASS(_name, _return_type, _list_class_fields)   \
	typedef struct {                                                \
	       	 _return_type ret;                                       \
	        bool    okay;                                           \
	        struct  {                                               \
	                __REMOVE(_list_class_fields);                   \
	        } args;                                                 \
	} class_##_name##_t;

So save all arugments to it.

__DEFINE_GUARD_CLASS(dma_map_single, dma_addr_t, (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir)
will expand to

	struct {
		dma_addr_t ret;
		bool	okay;
		struct {
			struct device *dev;
			void *ptr;
			size_t size;
			enum dma_data_direction dir;
		}
	}

So cleanup function can use saved argurement.

The above fun will be

	fun()
	{
		CLASS(dma_map_single, dma)(dev, ...);

		...
		if (...)
			return err;
	}

if funtion return, which need keep map,

	submit()
	{
		dma_map_single();
		...
		dmaengine_submit();
		if (...)
			goto err1
		return;

	goto err1:
		dma_umap_single();
	}

Macro retain_and_empty() will clean varible to avoid unmap.

        ({                                  \
                __auto_type __ptr = &(t); typeof(t) empty= {};   \
                __auto_type __val = *__ptr; \
                __ptr->okay = 0;        \
                __val.ret;              \
        })

So

	submit()
	{
       		CLASS(dma_map_single, dma)(dev,...;

	        ...
	        dmaengine_submit();
		if (...)
			return err;

		//before return;

		retain_and_empty(dma)
	}

This series just show how to hanndle many agurement at resource alloc/free
functions. Only show dma_map_single. If the over all method is acceptable.
I will more define for dma mapping functions.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (3):
      cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
      dma-mapping: Add auto cleanup support dma_map_single()
      i2c: lpi2c: Use auto cleanup for dma_map_single()

 drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++---
 include/linux/cleanup.h            | 73 ++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h        |  8 +++++
 3 files changed, 87 insertions(+), 7 deletions(-)
---
base-commit: 7c3ba4249a3604477ea9c077e10089ba7ddcaa03
change-id: 20251008-dmamap_cleanup-d0a7f0525a3d

Best regards,
--
Frank Li <Frank.Li@nxp.com>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
  2025-10-10 18:50 ` [PATCH 0/3] dma_mapping: Add auto cleanup support Frank Li
@ 2025-10-10 18:50   ` Frank Li
  2025-10-10 18:50   ` [PATCH 2/3] dma-mapping: Add auto cleanup support dma_map_single() Frank Li
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2025-10-10 18:50 UTC (permalink / raw)
  To: Marek Szyprowski, Robin Murphy, Dong Aisheng, Andi Shyti,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
  Cc: linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel, Frank Li

Some resource alloc/free functions require passing more than one argument,
such as dma_map_single() and dma_unmap_single(). Add
DEFINE_GUARD_ARGS_CLASS() to support these cases by introducing a helper
structure to store all arguments for the resource cleanup function.

DEFINE_GUARD_ARGS_CLASS(..., _list_class_fields, _list_func_args, _list_args)

 @_list_class_fields: argument list with types, separated by ';'
        e.g. (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir; int attr)
 @_list_func_args: argument list with types, separated by ','
        e.g. (struct device *dev, void *ptr, size_t size, enum dma_data_direction dir, int attr)
 @_list_args: argument list without types, separated by ','
        e.g. (dev, ptr, size, dir, attr)

Three lists are needed because C syntax differs between struct fields (';'),
function parameters (','), and variable arguments (no types).

Example usage for dma_map_single():

DEFINE_GUARD_ARGS_CLASS(dma_map_single, dma_addr_t,
                        dma_mapping_error(_T.args.dev, _T.dma_addr),
                        dma_unmap_single(_T->args.dev, _T->ret,
                                         _T->args.size, _T->args.dir),
                        dma_map_single,
                        (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir),
                        (struct device *dev, void *ptr, size_t size, enum dma_data_direction dir),
                        (dev, ptr, size, dir))

Example:

    fun()
    {
        CLASS(dma_map_single, dma)(dev, ...);
        ...
        if (error)
                return -EIO; // dma_unmap_single() will be auto-called
    }

Add retain_and_empty() to keep resource when functions.

Example:
    fun()
    {
        CLASS(dma_map_single, dma)(dev, ...);
        ...
        if (error)
                return -EIO; // dma_unmap_single() will be auto-called

	retain_and_empty(dma); // dma_umap_single() will NOT called.

	return 0;
    }

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
There are some checkpatch.pl error, but these is correct and better
readablity.

ERROR: Macros with complex values should be enclosed in parentheses
+#define DEFINE_GUARD_ARGS_CLASS(_name, _return_type, _is_err, _exit, _init, _list_class_fields, _list_func_args, _list_args) \
+__DEFINE_GUARD_ARGS_CLASS(_name, _return_type, _list_class_fields)                   \
+__DEFINE_GUARD_ARGS_ENTRY(_name, _is_err, _init, _list_func_args, _list_args)        \
+__DEFINE_GUARD_ARGS_EXIT(_name, _exit)

ERROR: Macros with complex values should be enclosed in parentheses
+#define __REMOVE_PAREN(x) __REMOVE_PAREN_HELP x
---
 include/linux/cleanup.h | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 2573585b7f068abe992af1ac05f478fef7b34306..84b27dd374f2200382b6dc1c450320b79406fd87 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -518,4 +518,77 @@ __DEFINE_LOCK_GUARD_0(_name, _lock)
 
 #define DEFINE_LOCK_GUARD_1_COND(X...) CONCATENATE(DEFINE_LOCK_GUARD_1_COND_, COUNT_ARGS(X))(X)
 
+/*
+ * Many resource allocation/free function pairs require passing multiple
+ * arguments — for example, dma_map_single() and dma_unmap_single()
+ *
+ * DEFINE_GUARD_ARGS_CLASS(dma_map_single, dma_addr_t,
+ *			   dma_mapping_error(_T.args.dev, _T.ret),
+ *			   dma_unmap_single(_T->args.dev, _T->ret, _T->args.size, _T->args.dir),
+ *			   dma_map_single,
+ *			   (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir),
+ *			   (struct device *dev, void *ptr, size_t size, enum dma_data_direction dir),
+ *			   (dev, ptr, size, dir))
+ * Example usage:
+ *
+ * int fun() {
+ *	CLASS(dma_map_single, dma)(dev, ptr, size, dir);
+ *	if (!dma.okay)
+ *		return -EIO
+ *	...
+ *	if (condition)
+ *		return -EIO // cleanup will auto unmap
+ *
+ *	req->dma_addr = retain_and_empty(dma); //keep mapping
+ *	return 0;
+ * }
+ */
+#define __REMOVE_PAREN_HELP(...) __VA_ARGS__
+#define __REMOVE_PAREN(x) __REMOVE_PAREN_HELP x
+
+#define __DEFINE_GUARD_ARGS_CLASS(_name, _return_type, _list_class_fields)	\
+typedef struct {								\
+	_return_type ret;							\
+	bool okay;								\
+	struct {								\
+		__REMOVE_PAREN(_list_class_fields);				\
+	} args;									\
+} class_##_name##_t;
+
+#define __DEFINE_GUARD_ARGS_EXIT(_name, _exit)					\
+static inline void class_##_name##_destructor(class_##_name##_t *_T)		\
+{       if (_T->okay) {  _exit; } }
+
+#define __DEFINE_GUARD_ARGS_ENTRY(_name, _is_err, _init, _list_func_args, _list__args)	\
+static inline class_##_name##_t class_##_name##_constructor(__REMOVE_PAREN(_list_func_args))	\
+{											\
+	class_##_name##_t _T = { .args = { __REMOVE_PAREN(_list__args) } };		\
+	_T.ret = _init _list__args;							\
+	_T.okay = !(_is_err);								\
+	return _T;									\
+}
+
+/**
+ * @_name: class name
+ * @_return_type: return data type
+ * @_is_err: macro to check init function return value
+ * @_exit: macro to do resource clean up
+ * @_init: macro/func name to alloc resource
+ * @_list_class_fields: (args list with type, use ; as split)
+ * @_list_func_args: (args list with type, use , as split)
+ * @_list_args: (args list without type, use , as split)
+ */
+#define DEFINE_GUARD_ARGS_CLASS(_name, _return_type, _is_err, _exit, _init, _list_class_fields, _list_func_args, _list_args) \
+__DEFINE_GUARD_ARGS_CLASS(_name, _return_type, _list_class_fields)                   \
+__DEFINE_GUARD_ARGS_ENTRY(_name, _is_err, _init, _list_func_args, _list_args)        \
+__DEFINE_GUARD_ARGS_EXIT(_name, _exit)
+
+#define retain_and_empty(t)							\
+	({									\
+		__auto_type __ptr = &(t); typeof(t) empty = {};			\
+		__auto_type __val = *__ptr;					\
+		*__ptr = empty;							\
+		__val.ret;							\
+	})
+
 #endif /* _LINUX_CLEANUP_H */

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] dma-mapping: Add auto cleanup support dma_map_single()
  2025-10-10 18:50 ` [PATCH 0/3] dma_mapping: Add auto cleanup support Frank Li
  2025-10-10 18:50   ` [PATCH 1/3] cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments Frank Li
@ 2025-10-10 18:50   ` Frank Li
  2025-10-10 18:50   ` [PATCH 3/3] i2c: lpi2c: Use auto cleanup for dma_map_single() Frank Li
  2025-10-14  5:54   ` [PATCH 0/3] dma_mapping: Add auto cleanup support Marek Szyprowski
  3 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2025-10-10 18:50 UTC (permalink / raw)
  To: Marek Szyprowski, Robin Murphy, Dong Aisheng, Andi Shyti,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
  Cc: linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel, Frank Li

Add auto cleanup support for dma_map_single().

    fun()
    {
        CLASS(dma_map_single, dma)(dev, ...);
        ...
        if (error)
            return -EIO; // dma_unmap_single() will be auto-called
    }

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 include/linux/dma-mapping.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 8248ff9363eed2ae5dc18e9eedb711b299201bea..89fc80e7cd85ba5a4068c61f58f3a578ab0a7a01 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -757,4 +757,12 @@ static inline int dma_mmap_wc(struct device *dev,
 	do { typeof(PTR) __p __maybe_unused = PTR; } while (0)
 #endif
 
+DEFINE_GUARD_ARGS_CLASS(dma_map_single, dma_addr_t,
+			dma_mapping_error(_T.args.dev, _T.ret),
+			dma_unmap_single(_T->args.dev, _T->ret, _T->args.size, _T->args.dir),
+			dma_map_single,
+			(struct device *dev; void *ptr; size_t size; enum dma_data_direction dir),
+			(struct device *dev, void *ptr, size_t size, enum dma_data_direction dir),
+			(dev, ptr, size, dir))
+
 #endif /* _LINUX_DMA_MAPPING_H */

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] i2c: lpi2c: Use auto cleanup for dma_map_single()
  2025-10-10 18:50 ` [PATCH 0/3] dma_mapping: Add auto cleanup support Frank Li
  2025-10-10 18:50   ` [PATCH 1/3] cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments Frank Li
  2025-10-10 18:50   ` [PATCH 2/3] dma-mapping: Add auto cleanup support dma_map_single() Frank Li
@ 2025-10-10 18:50   ` Frank Li
  2025-10-14  5:54   ` [PATCH 0/3] dma_mapping: Add auto cleanup support Marek Szyprowski
  3 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2025-10-10 18:50 UTC (permalink / raw)
  To: Marek Szyprowski, Robin Murphy, Dong Aisheng, Andi Shyti,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
  Cc: linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel, Frank Li

Use auto cleanup for dma_map_single() to simple code.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Demostrate to how to use auto cleanup for dma map functiongs. It will
simple more if need map multi buffers at a functions.
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 03b5a7e8c361abe1d75fb4d31f9614bbc6387d93..2c89d441ab1b3e607b8a2b0a6589e5909fa39ef4 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -727,9 +727,11 @@ static int lpi2c_dma_rx_cmd_submit(struct lpi2c_imx_struct *lpi2c_imx)
 	struct dma_chan *txchan = dma->chan_tx;
 	dma_cookie_t cookie;
 
-	dma->dma_tx_addr = dma_map_single(txchan->device->dev,
-					  dma->rx_cmd_buf, dma->rx_cmd_buf_len,
-					  DMA_TO_DEVICE);
+	CLASS(dma_map_single, dma_addr)(txchan->device->dev,
+					dma->rx_cmd_buf, dma->rx_cmd_buf_len,
+					DMA_TO_DEVICE);
+	dma->dma_tx_addr = dma_addr.ret;
+
 	if (dma_mapping_error(txchan->device->dev, dma->dma_tx_addr)) {
 		dev_err(&lpi2c_imx->adapter.dev, "DMA map failed, use pio\n");
 		return -EINVAL;
@@ -749,18 +751,15 @@ static int lpi2c_dma_rx_cmd_submit(struct lpi2c_imx_struct *lpi2c_imx)
 		goto submit_err_exit;
 	}
 
+	retain_and_empty(dma_addr);
 	dma_async_issue_pending(txchan);
 
 	return 0;
 
 desc_prepare_err_exit:
-	dma_unmap_single(txchan->device->dev, dma->dma_tx_addr,
-			 dma->rx_cmd_buf_len, DMA_TO_DEVICE);
 	return -EINVAL;
 
 submit_err_exit:
-	dma_unmap_single(txchan->device->dev, dma->dma_tx_addr,
-			 dma->rx_cmd_buf_len, DMA_TO_DEVICE);
 	dmaengine_desc_free(rx_cmd_desc);
 	return -EINVAL;
 }

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] dma_mapping: Add auto cleanup support
  2025-10-10 18:50 ` [PATCH 0/3] dma_mapping: Add auto cleanup support Frank Li
                     ` (2 preceding siblings ...)
  2025-10-10 18:50   ` [PATCH 3/3] i2c: lpi2c: Use auto cleanup for dma_map_single() Frank Li
@ 2025-10-14  5:54   ` Marek Szyprowski
  2025-10-19 12:03     ` Leon Romanovsky
  2025-11-06 17:27     ` Frank Li
  3 siblings, 2 replies; 8+ messages in thread
From: Marek Szyprowski @ 2025-10-14  5:54 UTC (permalink / raw)
  To: Frank Li, Robin Murphy, Dong Aisheng, Andi Shyti, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	Peter Zijlstra
  Cc: linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel,
	Peter Zijlstra

Hi

On 10.10.2025 20:50, Frank Li wrote:
> There are many below pattern
>
> 	fun()
> 	{
> 		...
> 		dma_map_single();
> 		if (dma_mapping_error)
> 			goto err1;
>
> 		dmaengine_prep_slave_single()
> 		if (...)
> 			goto err2
>
> 		dmaengine_submit()
> 		if (...)
> 			goto err3
>
> 		wait_for_completion_timeout()
> 		if (...)
> 			goto err4
>
> 	err4:
> 	err3:
> 	err2:
> 		dma_umap_single();
> 	err1:
> 	}
>
> Use cleanup can simple error handle like guard(), such as guard(mutex).
> or __free(kfree) = kmalloc.
>
> But dma_umap_single() need more argurements. So situation below complex.
>
> It need pack argurments list into structure.
>
> 	#define __DEFINE_GUARD_CLASS(_name, _return_type, _list_class_fields)   \
> 	typedef struct {                                                \
> 	       	 _return_type ret;                                       \
> 	        bool    okay;                                           \
> 	        struct  {                                               \
> 	                __REMOVE(_list_class_fields);                   \
> 	        } args;                                                 \
> 	} class_##_name##_t;
>
> So save all arugments to it.
>
> __DEFINE_GUARD_CLASS(dma_map_single, dma_addr_t, (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir)
> will expand to
>
> 	struct {
> 		dma_addr_t ret;
> 		bool	okay;
> 		struct {
> 			struct device *dev;
> 			void *ptr;
> 			size_t size;
> 			enum dma_data_direction dir;
> 		}
> 	}
>
> So cleanup function can use saved argurement.
>
> The above fun will be
>
> 	fun()
> 	{
> 		CLASS(dma_map_single, dma)(dev, ...);
>
> 		...
> 		if (...)
> 			return err;
> 	}
>
> if funtion return, which need keep map,
>
> 	submit()
> 	{
> 		dma_map_single();
> 		...
> 		dmaengine_submit();
> 		if (...)
> 			goto err1
> 		return;
>
> 	goto err1:
> 		dma_umap_single();
> 	}
>
> Macro retain_and_empty() will clean varible to avoid unmap.
>
>          ({                                  \
>                  __auto_type __ptr = &(t); typeof(t) empty= {};   \
>                  __auto_type __val = *__ptr; \
>                  __ptr->okay = 0;        \
>                  __val.ret;              \
>          })
>
> So
>
> 	submit()
> 	{
>         		CLASS(dma_map_single, dma)(dev,...;
>
> 	        ...
> 	        dmaengine_submit();
> 		if (...)
> 			return err;
>
> 		//before return;
>
> 		retain_and_empty(dma)
> 	}
>
> This series just show how to hanndle many agurement at resource alloc/free
> functions. Only show dma_map_single. If the over all method is acceptable.
> I will more define for dma mapping functions.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

This looks fine from the DMA-mapping API perspective. I think we should 
also get some feedback from Peter, who authored most of the __cleanup() 
based infrastructure, so I've added him to the recipients list.


> ---
> Frank Li (3):
>        cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
>        dma-mapping: Add auto cleanup support dma_map_single()
>        i2c: lpi2c: Use auto cleanup for dma_map_single()
>
>   drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++---
>   include/linux/cleanup.h            | 73 ++++++++++++++++++++++++++++++++++++++
>   include/linux/dma-mapping.h        |  8 +++++
>   3 files changed, 87 insertions(+), 7 deletions(-)
> ---
> base-commit: 7c3ba4249a3604477ea9c077e10089ba7ddcaa03
> change-id: 20251008-dmamap_cleanup-d0a7f0525a3d
>
> Best regards,
> --
> Frank Li <Frank.Li@nxp.com>
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] dma_mapping: Add auto cleanup support
  2025-10-14  5:54   ` [PATCH 0/3] dma_mapping: Add auto cleanup support Marek Szyprowski
@ 2025-10-19 12:03     ` Leon Romanovsky
  2025-10-20 15:56       ` Frank Li
  2025-11-06 17:27     ` Frank Li
  1 sibling, 1 reply; 8+ messages in thread
From: Leon Romanovsky @ 2025-10-19 12:03 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Frank Li, Robin Murphy, Dong Aisheng, Andi Shyti, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	Peter Zijlstra, linux-kernel, iommu, linux-i2c, imx,
	linux-arm-kernel

On Tue, Oct 14, 2025 at 07:54:46AM +0200, Marek Szyprowski wrote:
> Hi
> 
> On 10.10.2025 20:50, Frank Li wrote:
> > There are many below pattern
> >
> > 	fun()
> > 	{
> > 		...
> > 		dma_map_single();
> > 		if (dma_mapping_error)
> > 			goto err1;
> >
> > 		dmaengine_prep_slave_single()
> > 		if (...)
> > 			goto err2
> >
> > 		dmaengine_submit()
> > 		if (...)
> > 			goto err3
> >
> > 		wait_for_completion_timeout()
> > 		if (...)
> > 			goto err4
> >
> > 	err4:
> > 	err3:
> > 	err2:
> > 		dma_umap_single();
> > 	err1:
> > 	}
> >
> > Use cleanup can simple error handle like guard(), such as guard(mutex).
> > or __free(kfree) = kmalloc.
> >
> > But dma_umap_single() need more argurements. So situation below complex.
> >
> > It need pack argurments list into structure.
> >
> > 	#define __DEFINE_GUARD_CLASS(_name, _return_type, _list_class_fields)   \
> > 	typedef struct {                                                \
> > 	       	 _return_type ret;                                       \
> > 	        bool    okay;                                           \
> > 	        struct  {                                               \
> > 	                __REMOVE(_list_class_fields);                   \
> > 	        } args;                                                 \
> > 	} class_##_name##_t;
> >
> > So save all arugments to it.
> >
> > __DEFINE_GUARD_CLASS(dma_map_single, dma_addr_t, (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir)
> > will expand to
> >
> > 	struct {
> > 		dma_addr_t ret;
> > 		bool	okay;
> > 		struct {
> > 			struct device *dev;
> > 			void *ptr;
> > 			size_t size;
> > 			enum dma_data_direction dir;
> > 		}
> > 	}
> >
> > So cleanup function can use saved argurement.
> >
> > The above fun will be
> >
> > 	fun()
> > 	{
> > 		CLASS(dma_map_single, dma)(dev, ...);
> >
> > 		...
> > 		if (...)
> > 			return err;
> > 	}
> >
> > if funtion return, which need keep map,
> >
> > 	submit()
> > 	{
> > 		dma_map_single();
> > 		...
> > 		dmaengine_submit();
> > 		if (...)
> > 			goto err1
> > 		return;
> >
> > 	goto err1:
> > 		dma_umap_single();
> > 	}
> >
> > Macro retain_and_empty() will clean varible to avoid unmap.
> >
> >          ({                                  \
> >                  __auto_type __ptr = &(t); typeof(t) empty= {};   \
> >                  __auto_type __val = *__ptr; \
> >                  __ptr->okay = 0;        \
> >                  __val.ret;              \
> >          })
> >
> > So
> >
> > 	submit()
> > 	{
> >         		CLASS(dma_map_single, dma)(dev,...;
> >
> > 	        ...
> > 	        dmaengine_submit();
> > 		if (...)
> > 			return err;
> >
> > 		//before return;
> >
> > 		retain_and_empty(dma)
> > 	}
> >
> > This series just show how to hanndle many agurement at resource alloc/free
> > functions. Only show dma_map_single. If the over all method is acceptable.
> > I will more define for dma mapping functions.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> 
> This looks fine from the DMA-mapping API perspective. I think we should 
> also get some feedback from Peter, who authored most of the __cleanup() 
> based infrastructure, so I've added him to the recipients list.

I may represent minority here, but patch "i2c: lpi2c: Use auto cleanup
for dma_map_single()" looks completely unreadable after this change.

It is perfectly valid to use __cleanup() for simple and scoped things
like kmalloc, but DMA API is much complicated than that.

Thanks

> 
> 
> > ---
> > Frank Li (3):
> >        cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
> >        dma-mapping: Add auto cleanup support dma_map_single()
> >        i2c: lpi2c: Use auto cleanup for dma_map_single()
> >
> >   drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++---
> >   include/linux/cleanup.h            | 73 ++++++++++++++++++++++++++++++++++++++
> >   include/linux/dma-mapping.h        |  8 +++++
> >   3 files changed, 87 insertions(+), 7 deletions(-)
> > ---
> > base-commit: 7c3ba4249a3604477ea9c077e10089ba7ddcaa03
> > change-id: 20251008-dmamap_cleanup-d0a7f0525a3d
> >
> > Best regards,
> > --
> > Frank Li <Frank.Li@nxp.com>
> >
> >
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] dma_mapping: Add auto cleanup support
  2025-10-19 12:03     ` Leon Romanovsky
@ 2025-10-20 15:56       ` Frank Li
  0 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2025-10-20 15:56 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Marek Szyprowski, Robin Murphy, Dong Aisheng, Andi Shyti,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	Peter Zijlstra, linux-kernel, iommu, linux-i2c, imx,
	linux-arm-kernel

On Sun, Oct 19, 2025 at 03:03:04PM +0300, Leon Romanovsky wrote:
> On Tue, Oct 14, 2025 at 07:54:46AM +0200, Marek Szyprowski wrote:
> > Hi
> >
> > On 10.10.2025 20:50, Frank Li wrote:
> > > There are many below pattern
> > >
> > > 	fun()
> > > 	{
> > > 		...
> > > 		dma_map_single();
> > > 		if (dma_mapping_error)
> > > 			goto err1;
> > >
> > > 		dmaengine_prep_slave_single()
> > > 		if (...)
> > > 			goto err2
> > >
> > > 		dmaengine_submit()
> > > 		if (...)
> > > 			goto err3
> > >
> > > 		wait_for_completion_timeout()
> > > 		if (...)
> > > 			goto err4
> > >
> > > 	err4:
> > > 	err3:
> > > 	err2:
> > > 		dma_umap_single();
> > > 	err1:
> > > 	}
> > >
> > > Use cleanup can simple error handle like guard(), such as guard(mutex).
> > > or __free(kfree) = kmalloc.
> > >
> > > But dma_umap_single() need more argurements. So situation below complex.
> > >
> > > It need pack argurments list into structure.
> > >
> > > 	#define __DEFINE_GUARD_CLASS(_name, _return_type, _list_class_fields)   \
> > > 	typedef struct {                                                \
> > > 	       	 _return_type ret;                                       \
> > > 	        bool    okay;                                           \
> > > 	        struct  {                                               \
> > > 	                __REMOVE(_list_class_fields);                   \
> > > 	        } args;                                                 \
> > > 	} class_##_name##_t;
> > >
> > > So save all arugments to it.
> > >
> > > __DEFINE_GUARD_CLASS(dma_map_single, dma_addr_t, (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir)
> > > will expand to
> > >
> > > 	struct {
> > > 		dma_addr_t ret;
> > > 		bool	okay;
> > > 		struct {
> > > 			struct device *dev;
> > > 			void *ptr;
> > > 			size_t size;
> > > 			enum dma_data_direction dir;
> > > 		}
> > > 	}
> > >
> > > So cleanup function can use saved argurement.
> > >
> > > The above fun will be
> > >
> > > 	fun()
> > > 	{
> > > 		CLASS(dma_map_single, dma)(dev, ...);
> > >
> > > 		...
> > > 		if (...)
> > > 			return err;
> > > 	}
> > >
> > > if funtion return, which need keep map,
> > >
> > > 	submit()
> > > 	{
> > > 		dma_map_single();
> > > 		...
> > > 		dmaengine_submit();
> > > 		if (...)
> > > 			goto err1
> > > 		return;
> > >
> > > 	goto err1:
> > > 		dma_umap_single();
> > > 	}
> > >
> > > Macro retain_and_empty() will clean varible to avoid unmap.
> > >
> > >          ({                                  \
> > >                  __auto_type __ptr = &(t); typeof(t) empty= {};   \
> > >                  __auto_type __val = *__ptr; \
> > >                  __ptr->okay = 0;        \
> > >                  __val.ret;              \
> > >          })
> > >
> > > So
> > >
> > > 	submit()
> > > 	{
> > >         		CLASS(dma_map_single, dma)(dev,...;
> > >
> > > 	        ...
> > > 	        dmaengine_submit();
> > > 		if (...)
> > > 			return err;
> > >
> > > 		//before return;
> > >
> > > 		retain_and_empty(dma)
> > > 	}
> > >
> > > This series just show how to hanndle many agurement at resource alloc/free
> > > functions. Only show dma_map_single. If the over all method is acceptable.
> > > I will more define for dma mapping functions.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> >
> > This looks fine from the DMA-mapping API perspective. I think we should
> > also get some feedback from Peter, who authored most of the __cleanup()
> > based infrastructure, so I've added him to the recipients list.
>
> I may represent minority here, but patch "i2c: lpi2c: Use auto cleanup
> for dma_map_single()" looks completely unreadable after this change.
>
> It is perfectly valid to use __cleanup() for simple and scoped things
> like kmalloc, but DMA API is much complicated than that.

Yes, DMA API is much complicated. Actually macro CLASS() is using
__cleanup().


#define CLASS(_name, var)						\
	class_##_name##_t var __cleanup(class_##_name##_destructor) =	\
		class_##_name##_constructor

Frank

>
> Thanks
>
> >
> >
> > > ---
> > > Frank Li (3):
> > >        cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
> > >        dma-mapping: Add auto cleanup support dma_map_single()
> > >        i2c: lpi2c: Use auto cleanup for dma_map_single()
> > >
> > >   drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++---
> > >   include/linux/cleanup.h            | 73 ++++++++++++++++++++++++++++++++++++++
> > >   include/linux/dma-mapping.h        |  8 +++++
> > >   3 files changed, 87 insertions(+), 7 deletions(-)
> > > ---
> > > base-commit: 7c3ba4249a3604477ea9c077e10089ba7ddcaa03
> > > change-id: 20251008-dmamap_cleanup-d0a7f0525a3d
> > >
> > > Best regards,
> > > --
> > > Frank Li <Frank.Li@nxp.com>
> > >
> > >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
> >
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] dma_mapping: Add auto cleanup support
  2025-10-14  5:54   ` [PATCH 0/3] dma_mapping: Add auto cleanup support Marek Szyprowski
  2025-10-19 12:03     ` Leon Romanovsky
@ 2025-11-06 17:27     ` Frank Li
  1 sibling, 0 replies; 8+ messages in thread
From: Frank Li @ 2025-11-06 17:27 UTC (permalink / raw)
  To: Marek Szyprowski, Peter Zijlstra
  Cc: Robin Murphy, Dong Aisheng, Andi Shyti, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Peter Zijlstra,
	linux-kernel, iommu, linux-i2c, imx, linux-arm-kernel

On Tue, Oct 14, 2025 at 07:54:46AM +0200, Marek Szyprowski wrote:
> Hi
>
> On 10.10.2025 20:50, Frank Li wrote:
> > There are many below pattern
> >
> > 	fun()
> > 	{
> > 		...
> > 		dma_map_single();
> > 		if (dma_mapping_error)
> > 			goto err1;
> >
> > 		dmaengine_prep_slave_single()
> > 		if (...)
> > 			goto err2
> >
> > 		dmaengine_submit()
> > 		if (...)
> > 			goto err3
> >
> > 		wait_for_completion_timeout()
> > 		if (...)
> > 			goto err4
> >
> > 	err4:
> > 	err3:
> > 	err2:
> > 		dma_umap_single();
> > 	err1:
> > 	}
> >
> > Use cleanup can simple error handle like guard(), such as guard(mutex).
> > or __free(kfree) = kmalloc.
> >
> > But dma_umap_single() need more argurements. So situation below complex.
> >
> > It need pack argurments list into structure.
> >
> > 	#define __DEFINE_GUARD_CLASS(_name, _return_type, _list_class_fields)   \
> > 	typedef struct {                                                \
> > 	       	 _return_type ret;                                       \
> > 	        bool    okay;                                           \
> > 	        struct  {                                               \
> > 	                __REMOVE(_list_class_fields);                   \
> > 	        } args;                                                 \
> > 	} class_##_name##_t;
> >
> > So save all arugments to it.
> >
> > __DEFINE_GUARD_CLASS(dma_map_single, dma_addr_t, (struct device *dev; void *ptr; size_t size; enum dma_data_direction dir)
> > will expand to
> >
> > 	struct {
> > 		dma_addr_t ret;
> > 		bool	okay;
> > 		struct {
> > 			struct device *dev;
> > 			void *ptr;
> > 			size_t size;
> > 			enum dma_data_direction dir;
> > 		}
> > 	}
> >
> > So cleanup function can use saved argurement.
> >
> > The above fun will be
> >
> > 	fun()
> > 	{
> > 		CLASS(dma_map_single, dma)(dev, ...);
> >
> > 		...
> > 		if (...)
> > 			return err;
> > 	}
> >
> > if funtion return, which need keep map,
> >
> > 	submit()
> > 	{
> > 		dma_map_single();
> > 		...
> > 		dmaengine_submit();
> > 		if (...)
> > 			goto err1
> > 		return;
> >
> > 	goto err1:
> > 		dma_umap_single();
> > 	}
> >
> > Macro retain_and_empty() will clean varible to avoid unmap.
> >
> >          ({                                  \
> >                  __auto_type __ptr = &(t); typeof(t) empty= {};   \
> >                  __auto_type __val = *__ptr; \
> >                  __ptr->okay = 0;        \
> >                  __val.ret;              \
> >          })
> >
> > So
> >
> > 	submit()
> > 	{
> >         		CLASS(dma_map_single, dma)(dev,...;
> >
> > 	        ...
> > 	        dmaengine_submit();
> > 		if (...)
> > 			return err;
> >
> > 		//before return;
> >
> > 		retain_and_empty(dma)
> > 	}
> >
> > This series just show how to hanndle many agurement at resource alloc/free
> > functions. Only show dma_map_single. If the over all method is acceptable.
> > I will more define for dma mapping functions.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
>
> This looks fine from the DMA-mapping API perspective. I think we should
> also get some feedback from Peter, who authored most of the __cleanup()
> based infrastructure, so I've added him to the recipients list.

Peter Zijlstra:

	Do you have chance to check this?

Frank

>
>
> > ---
> > Frank Li (3):
> >        cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments
> >        dma-mapping: Add auto cleanup support dma_map_single()
> >        i2c: lpi2c: Use auto cleanup for dma_map_single()
> >
> >   drivers/i2c/busses/i2c-imx-lpi2c.c | 13 ++++---
> >   include/linux/cleanup.h            | 73 ++++++++++++++++++++++++++++++++++++++
> >   include/linux/dma-mapping.h        |  8 +++++
> >   3 files changed, 87 insertions(+), 7 deletions(-)
> > ---
> > base-commit: 7c3ba4249a3604477ea9c077e10089ba7ddcaa03
> > change-id: 20251008-dmamap_cleanup-d0a7f0525a3d
> >
> > Best regards,
> > --
> > Frank Li <Frank.Li@nxp.com>
> >
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-11-06 17:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20251010185046eucas1p26868b540b74a96e36943066216525bed@eucas1p2.samsung.com>
2025-10-10 18:50 ` [PATCH 0/3] dma_mapping: Add auto cleanup support Frank Li
2025-10-10 18:50   ` [PATCH 1/3] cleanup: Add DEFINE_GUARD_ARGS_CLASS macro for resource alloc/free functions with multiple arguments Frank Li
2025-10-10 18:50   ` [PATCH 2/3] dma-mapping: Add auto cleanup support dma_map_single() Frank Li
2025-10-10 18:50   ` [PATCH 3/3] i2c: lpi2c: Use auto cleanup for dma_map_single() Frank Li
2025-10-14  5:54   ` [PATCH 0/3] dma_mapping: Add auto cleanup support Marek Szyprowski
2025-10-19 12:03     ` Leon Romanovsky
2025-10-20 15:56       ` Frank Li
2025-11-06 17:27     ` Frank Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).