linux-kernel - Re: [PATCH 0/3] dma_mapping: Add auto cleanup support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aPZbuY5bLpkoO2J/@lizhi-Precision-Tower-5810>
Date: Mon, 20 Oct 2025 11:56:41 -0400
From: Frank Li <Frank.li@....com>
To: Leon Romanovsky <leon@...nel.org>
Cc: Marek Szyprowski <m.szyprowski@...sung.com>,
	Robin Murphy <robin.murphy@....com>,
	Dong Aisheng <aisheng.dong@....com>,
	Andi Shyti <andi.shyti@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org,
	iommu@...ts.linux.dev, linux-i2c@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 0/3] dma_mapping: Add auto cleanup support

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@....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@....com>
> > >
> > >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
> >
> >