| Message ID | 20240812072811.9737-1-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] remoteproc: Use iommu_paging_domain_alloc() | expand |
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 >
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
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 > > >
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
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
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);