[PATCH v2 1/1] iommu/dma: Use DMA ops setter instead of direct assignment

[PATCH v2 1/1] iommu/dma: Use DMA ops setter instead of direct assignment

[Andy Shevchenko]

Use DMA ops setter instead of direct assignment. Even we know that
this module doesn't perform access to the dma_ops member of struct device,
it's better to use setter to avoid potential problems in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased on top of the latest codebase
 drivers/iommu/dma-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d85d54f2b549..b585a3fdbc56 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1482,7 +1482,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
 	if (iommu_is_dma_domain(domain)) {
 		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
 			goto out_err;
-		dev->dma_ops = &iommu_dma_ops;
+		set_dma_ops(dev, &iommu_dma_ops);
 	}
 
 	return;
-- 
2.34.1

Re: [PATCH v2 1/1] iommu/dma: Use DMA ops setter instead of direct assignment

[Robin Murphy]

On 2022-02-07 14:13, Andy Shevchenko wrote:
> Use DMA ops setter instead of direct assignment. Even we know that
> this module doesn't perform access to the dma_ops member of struct device,
> it's better to use setter to avoid potential problems in the future.

What potential problems are you imagining? This whole file is a DMA ops 
implementation, not driver code (and definitely not a module); if anyone 
removes the "select DMA_OPS" from CONFIG_IOMMU_DMA they deserve whatever 
breakage they get.

I concur that there's no major harm in using the helper here, but I also 
see no point in pretending that there's any value to abstracting the 
operation in this particular context.

Thanks,
Robin.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: rebased on top of the latest codebase
>   drivers/iommu/dma-iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index d85d54f2b549..b585a3fdbc56 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1482,7 +1482,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
>   	if (iommu_is_dma_domain(domain)) {
>   		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
>   			goto out_err;
> -		dev->dma_ops = &iommu_dma_ops;
> +		set_dma_ops(dev, &iommu_dma_ops);
>   	}
>   
>   	return;

Re: [PATCH v2 1/1] iommu/dma: Use DMA ops setter instead of direct assignment

[Christoph Hellwig]

On Mon, Feb 07, 2022 at 03:55:32PM +0000, Robin Murphy wrote:
> On 2022-02-07 14:13, Andy Shevchenko wrote:
> > Use DMA ops setter instead of direct assignment. Even we know that
> > this module doesn't perform access to the dma_ops member of struct device,
> > it's better to use setter to avoid potential problems in the future.
> 
> What potential problems are you imagining? This whole file is a DMA ops
> implementation, not driver code (and definitely not a module); if anyone
> removes the "select DMA_OPS" from CONFIG_IOMMU_DMA they deserve whatever
> breakage they get.
> 
> I concur that there's no major harm in using the helper here, but I also see
> no point in pretending that there's any value to abstracting the operation
> in this particular context.

Yeah.  Killing off the the wrapper is actually on my todo list, mostly
because it leads to people doing completely broken things like the VDPA
private dma ops that should not exist.

Re: [PATCH v2 1/1] iommu/dma: Use DMA ops setter instead of direct assignment

[Andy Shevchenko]

On Tue, Feb 08, 2022 at 11:06:56PM -0800, Christoph Hellwig wrote:
> On Mon, Feb 07, 2022 at 03:55:32PM +0000, Robin Murphy wrote:
> > On 2022-02-07 14:13, Andy Shevchenko wrote:
> > > Use DMA ops setter instead of direct assignment. Even we know that
> > > this module doesn't perform access to the dma_ops member of struct device,
> > > it's better to use setter to avoid potential problems in the future.
> > 
> > What potential problems are you imagining? This whole file is a DMA ops
> > implementation, not driver code (and definitely not a module); if anyone
> > removes the "select DMA_OPS" from CONFIG_IOMMU_DMA they deserve whatever
> > breakage they get.
> > 
> > I concur that there's no major harm in using the helper here, but I also see
> > no point in pretending that there's any value to abstracting the operation
> > in this particular context.
> 
> Yeah.  Killing off the the wrapper is actually on my todo list, mostly
> because it leads to people doing completely broken things like the VDPA
> private dma ops that should not exist.

Let's abandon this change. (I see that it's kinda 50/50 of the users
with API and without)

-- 
With Best Regards,
Andy Shevchenko