[PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Lu Baolu]

An iommu domain is allocated in rproc_enable_iommu() and is attached to
rproc->dev.parent in the same function.

Use iommu_paging_domain_alloc() to make it explicit.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
---
 drivers/remoteproc/remoteproc_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f276956f2c5c..eb66f78ec8b7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
 		return 0;
 	}
 
-	domain = iommu_domain_alloc(dev->bus);
-	if (!domain) {
+	domain = iommu_paging_domain_alloc(dev);
+	if (IS_ERR(domain)) {
 		dev_err(dev, "can't alloc iommu domain\n");
-		return -ENOMEM;
+		return PTR_ERR(domain);
 	}
 
 	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
-- 
2.34.1

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Mathieu Poirier]

Hi Baolu,

Sorry for the late reply, this slipped through the cracks.

On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> An iommu domain is allocated in rproc_enable_iommu() and is attached to
> rproc->dev.parent in the same function.
> 
> Use iommu_paging_domain_alloc() to make it explicit.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
> ---
>  drivers/remoteproc/remoteproc_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f276956f2c5c..eb66f78ec8b7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
>  		return 0;
>  	}
>  
> -	domain = iommu_domain_alloc(dev->bus);
> -	if (!domain) {
> +	domain = iommu_paging_domain_alloc(dev);

I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
with the IOMMU subsystem to confidently more forward.

I am asking the Qualcomm (Bjorn and friends) and TI crew (Beleswar, Andrew,
Nishanth and Hari) to test this patch on their IOMMU devices and get back to me
with a "Tested-by".

Thanks,
Mathieu

> +	if (IS_ERR(domain)) {
>  		dev_err(dev, "can't alloc iommu domain\n");
> -		return -ENOMEM;
> +		return PTR_ERR(domain);
>  	}
>  
>  	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> -- 
> 2.34.1
> 

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 10:17:56AM -0600, Mathieu Poirier wrote:

> > -	domain = iommu_domain_alloc(dev->bus);
> > -	if (!domain) {
> > +	domain = iommu_paging_domain_alloc(dev);
> 
> I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
> second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
> provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
> with the IOMMU subsystem to confidently more forward.

So long as dev is the device that will be initiating DMA this is a
correct change from the iommu subsystem perspective.

Jason

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 01:24:25PM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 22, 2024 at 10:17:56AM -0600, Mathieu Poirier wrote:
> 
> > > -	domain = iommu_domain_alloc(dev->bus);
> > > -	if (!domain) {
> > > +	domain = iommu_paging_domain_alloc(dev);
> > 
> > I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
> > second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
> > provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
> > with the IOMMU subsystem to confidently more forward.
> 
> So long as dev is the device that will be initiating DMA this is a
> correct change from the iommu subsystem perspective.

This is the only call site left for iommu_domain_alloc() - we want to
remove this API on the next cycle. Please take the patch

Thanks,
Jason

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Mathieu Poirier]

On Sun, 15 Sept 2024 at 08:09, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Aug 22, 2024 at 01:24:25PM -0300, Jason Gunthorpe wrote:
> > On Thu, Aug 22, 2024 at 10:17:56AM -0600, Mathieu Poirier wrote:
> >
> > > > - domain = iommu_domain_alloc(dev->bus);
> > > > - if (!domain) {
> > > > + domain = iommu_paging_domain_alloc(dev);
> > >
> > > I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
> > > second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
> > > provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
> > > with the IOMMU subsystem to confidently more forward.
> >
> > So long as dev is the device that will be initiating DMA this is a
> > correct change from the iommu subsystem perspective.
>
> This is the only call site left for iommu_domain_alloc() - we want to
> remove this API on the next cycle. Please take the patch
>

And I have no intentions of delaying things further.  I will discuss
this with Bjorn later this week in Vienna.

> Thanks,
> Jason

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Beleswar Prasad Padhi]

Hi All,

On 22/08/24 21:47, Mathieu Poirier wrote:
> Hi Baolu,
>
> Sorry for the late reply, this slipped through the cracks.
>
> On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> > An iommu domain is allocated in rproc_enable_iommu() and is attached to
> > rproc->dev.parent in the same function.
> > 
> > Use iommu_paging_domain_alloc() to make it explicit.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index f276956f2c5c..eb66f78ec8b7 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
> >  		return 0;
> >  	}
> >  
> > -	domain = iommu_domain_alloc(dev->bus);
> > -	if (!domain) {
> > +	domain = iommu_paging_domain_alloc(dev);
>
> I'm a little hesitant here.  Function iommu_domain_alloc() passes NULL two the
> second argument of __iommu_domain_alloc() while iommu_paging_domain_alloc()
> provides a 'dev'.  I don't have any HW to test on and I am not familiar enough
> with the IOMMU subsystem to confidently more forward.
>
> I am asking the Qualcomm (Bjorn and friends) and TI crew (Beleswar, Andrew,
> Nishanth and Hari) to test this patch on their IOMMU devices and get back to me
> with a "Tested-by".


Just a heads-up. Currently, I am seeing boot failures while booting 
remotecores in TI's IOMMU devices with mainline kernel. Working on the 
the fix, once it is done, will apply the above patch and test it ASAP.

Thanks,
Beleswar

>
> Thanks,
> Mathieu
>
> > +	if (IS_ERR(domain)) {
> >  		dev_err(dev, "can't alloc iommu domain\n");
> > -		return -ENOMEM;
> > +		return PTR_ERR(domain);
> >  	}
> >  
> >  	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> > -- 
> > 2.34.1
> > 
>

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Beleswar Prasad Padhi]

On 29-08-2024 11:47, Beleswar Prasad Padhi wrote:
> Hi All,
>
> On 22/08/24 21:47, Mathieu Poirier wrote:
>> Hi Baolu,
>>
>> Sorry for the late reply, this slipped through the cracks.
>>
>> On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
>> > An iommu domain is allocated in rproc_enable_iommu() and is 
>> attached to
>> > rproc->dev.parent in the same function.
>> > > Use iommu_paging_domain_alloc() to make it explicit.
>> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> > Link: 
>> https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
>> > ---
>> >  drivers/remoteproc/remoteproc_core.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > > diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>> > index f276956f2c5c..eb66f78ec8b7 100644
>> > --- a/drivers/remoteproc/remoteproc_core.c
>> > +++ b/drivers/remoteproc/remoteproc_core.c
>> > @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc 
>> *rproc)
>> >          return 0;
>> >      }
>> >  > -    domain = iommu_domain_alloc(dev->bus);
>> > -    if (!domain) {
>> > +    domain = iommu_paging_domain_alloc(dev);
>>
>> I'm a little hesitant here.  Function iommu_domain_alloc() passes 
>> NULL two the
>> second argument of __iommu_domain_alloc() while 
>> iommu_paging_domain_alloc()
>> provides a 'dev'.  I don't have any HW to test on and I am not 
>> familiar enough
>> with the IOMMU subsystem to confidently more forward.
>>
>> I am asking the Qualcomm (Bjorn and friends) and TI crew (Beleswar, 
>> Andrew,
>> Nishanth and Hari) to test this patch on their IOMMU devices and get 
>> back to me
>> with a "Tested-by".
>
>
> Just a heads-up. Currently, I am seeing boot failures while booting 
> remotecores in TI's IOMMU devices with mainline kernel. Working on the 
> the fix, once it is done, will apply the above patch and test it ASAP.


Since commit 17de3f5fdd35, the IOMMU framework has changed how it 
handles callback ops, moving away from using bus->iommu_ops. The 
omap-iommu driver has not yet been updated to align with these framework 
changes. Therefore, omap-iommu, and hence, omap-remoteproc have been 
broken since 17de3f5fdd35.

This patch is a step in the right direction for adapting the 
remoteproc_core framework to the newer IOMMU framework by looking for 
iommu_ops attached to the dev structure instead of dev->bus. Due to time 
constraints, I was unable to update the omap-iommu driver to accommodate 
these changes, so, I could not test this patch. However, logically, this 
patch LGTM. So,

Acked-by: Beleswar Padhi <b-padhi@ti.com>

Thanks,
Beleswar

>
> Thanks,
> Beleswar
>
>>
>> Thanks,
>> Mathieu
>>
>> > +    if (IS_ERR(domain)) {
>> >          dev_err(dev, "can't alloc iommu domain\n");
>> > -        return -ENOMEM;
>> > +        return PTR_ERR(domain);
>> >      }
>> >  >      iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
>> > -- > 2.34.1
>> >

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Jason Gunthorpe]

On Sun, Sep 22, 2024 at 04:57:07PM +0530, Beleswar Prasad Padhi wrote:

> Since commit 17de3f5fdd35, the IOMMU framework has changed how it handles
> callback ops, moving away from using bus->iommu_ops. The omap-iommu driver
> has not yet been updated to align with these framework changes. Therefore,
> omap-iommu, and hence, omap-remoteproc have been broken since 17de3f5fdd35.

That commit is a year old now, if nobody has cared to fix or report
this can we consider removing some of this old code from upstream?
Usually a year of being broken without anyone caring is a good enough
threshold.

> Acked-by: Beleswar Padhi <b-padhi@ti.com>

Thanks

Jason

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Mathieu Poirier]

On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> An iommu domain is allocated in rproc_enable_iommu() and is attached to
> rproc->dev.parent in the same function.
> 
> Use iommu_paging_domain_alloc() to make it explicit.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
> ---
>  drivers/remoteproc/remoteproc_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>

I have applied this patch.

Thanks,
Mathieu

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f276956f2c5c..eb66f78ec8b7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -109,10 +109,10 @@ static int rproc_enable_iommu(struct rproc *rproc)
>  		return 0;
>  	}
>  
> -	domain = iommu_domain_alloc(dev->bus);
> -	if (!domain) {
> +	domain = iommu_paging_domain_alloc(dev);
> +	if (IS_ERR(domain)) {
>  		dev_err(dev, "can't alloc iommu domain\n");
> -		return -ENOMEM;
> +		return PTR_ERR(domain);
>  	}
>  
>  	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
> -- 
> 2.34.1
> 

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Jason Gunthorpe]

On Mon, Sep 30, 2024 at 10:40:30AM -0600, Mathieu Poirier wrote:
> On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> > An iommu domain is allocated in rproc_enable_iommu() and is attached to
> > rproc->dev.parent in the same function.
> > 
> > Use iommu_paging_domain_alloc() to make it explicit.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> 
> I have applied this patch.

It is a bit late now. Can we run the stragglers through the iommu
tree? We want to put dependent patches on top of this.

Jason

Re: [PATCH 1/1] remoteproc: Use iommu_paging_domain_alloc()

[Mathieu Poirier]

On Tue, 1 Oct 2024 at 07:35, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Sep 30, 2024 at 10:40:30AM -0600, Mathieu Poirier wrote:
> > On Mon, Aug 12, 2024 at 03:28:11PM +0800, Lu Baolu wrote:
> > > An iommu domain is allocated in rproc_enable_iommu() and is attached to
> > > rproc->dev.parent in the same function.
> > >
> > > Use iommu_paging_domain_alloc() to make it explicit.
> > >
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Link: https://lore.kernel.org/r/20240610085555.88197-13-baolu.lu@linux.intel.com
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> >
> > I have applied this patch.
>
> It is a bit late now. Can we run the stragglers through the iommu
> tree? We want to put dependent patches on top of this.
>

Sure, we can do that:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Please send me a pull request so that I can also carry the patch in my tree.