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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251019120304.GG6199@unreal>
Date: Sun, 19 Oct 2025 15:03:04 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Marek Szyprowski <m.szyprowski@...sung.com>
Cc: Frank Li <Frank.Li@....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 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.

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
> 
>