| Message ID | 20240529053250.91284-7-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Refactoring domain allocation interface | expand |
On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote: > The domain allocated in msm_iommu_new() is for the @dev. Replace > iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. > > Update msm_iommu_new() to always return ERR_PTR in failure cases instead > of NULL. Please don't mix unrelated changes, because ... > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > index d5512037c38b..f7e28d4b5f62 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) > struct msm_iommu *iommu; > int ret; > > - domain = iommu_domain_alloc(dev->bus); > - if (!domain) > - return NULL; > + domain = iommu_paging_domain_alloc(dev); > + if (IS_ERR(domain)) > + return ERR_CAST(domain); > > iommu_set_pgtable_quirks(domain, quirks); > > @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig > struct msm_mmu *mmu; > > mmu = msm_iommu_new(dev, quirks); > - if (IS_ERR_OR_NULL(mmu)) > + if (IS_ERR(mmu)) > return mmu; NAK, not having an IOMMU is a poor but legit usecase for some of devices which don't have IOMMU support yet (for example because of the buggy implementation for which we were not able to get all the hooks in). Please don't break compatibility for existing platforms. > > iommu = to_msm_iommu(mmu); > -- > 2.34.1 >
On 5/29/24 4:21 PM, Dmitry Baryshkov wrote: > On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote: >> The domain allocated in msm_iommu_new() is for the @dev. Replace >> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. >> >> Update msm_iommu_new() to always return ERR_PTR in failure cases instead >> of NULL. > Please don't mix unrelated changes, because ... > >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >> --- >> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c >> index d5512037c38b..f7e28d4b5f62 100644 >> --- a/drivers/gpu/drm/msm/msm_iommu.c >> +++ b/drivers/gpu/drm/msm/msm_iommu.c >> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) >> struct msm_iommu *iommu; >> int ret; >> >> - domain = iommu_domain_alloc(dev->bus); >> - if (!domain) >> - return NULL; >> + domain = iommu_paging_domain_alloc(dev); >> + if (IS_ERR(domain)) >> + return ERR_CAST(domain); >> >> iommu_set_pgtable_quirks(domain, quirks); >> >> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig >> struct msm_mmu *mmu; >> >> mmu = msm_iommu_new(dev, quirks); >> - if (IS_ERR_OR_NULL(mmu)) >> + if (IS_ERR(mmu)) >> return mmu; > NAK, not having an IOMMU is a poor but legit usecase for some of devices > which don't have IOMMU support yet (for example because of the buggy > implementation for which we were not able to get all the hooks in). > > Please don't break compatibility for existing platforms. Sure. I will remove this line of change. Though I have no idea in which case msm_iommu_new() could return NULL after this patch. Best regards, baolu
On Thu, 30 May 2024 at 04:59, Baolu Lu <baolu.lu@linux.intel.com> wrote: > > On 5/29/24 4:21 PM, Dmitry Baryshkov wrote: > > On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote: > >> The domain allocated in msm_iommu_new() is for the @dev. Replace > >> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. > >> > >> Update msm_iommu_new() to always return ERR_PTR in failure cases instead > >> of NULL. > > Please don't mix unrelated changes, because ... > > > >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> > >> --- > >> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > >> index d5512037c38b..f7e28d4b5f62 100644 > >> --- a/drivers/gpu/drm/msm/msm_iommu.c > >> +++ b/drivers/gpu/drm/msm/msm_iommu.c > >> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) > >> struct msm_iommu *iommu; > >> int ret; > >> > >> - domain = iommu_domain_alloc(dev->bus); > >> - if (!domain) > >> - return NULL; > >> + domain = iommu_paging_domain_alloc(dev); > >> + if (IS_ERR(domain)) > >> + return ERR_CAST(domain); > >> > >> iommu_set_pgtable_quirks(domain, quirks); > >> > >> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig > >> struct msm_mmu *mmu; > >> > >> mmu = msm_iommu_new(dev, quirks); > >> - if (IS_ERR_OR_NULL(mmu)) > >> + if (IS_ERR(mmu)) > >> return mmu; > > NAK, not having an IOMMU is a poor but legit usecase for some of devices > > which don't have IOMMU support yet (for example because of the buggy > > implementation for which we were not able to get all the hooks in). > > > > Please don't break compatibility for existing platforms. > > Sure. I will remove this line of change. Though I have no idea in which > case msm_iommu_new() could return NULL after this patch. So, even without this chunk you are going to break the no-IOMMU case. Please don't. This will result in a regression report and a revert. Instead please provide a way for the existing drivers to continue working. For example, something like: if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV)) return NULL;
On 5/30/24 3:58 PM, Dmitry Baryshkov wrote: > On Thu, 30 May 2024 at 04:59, Baolu Lu<baolu.lu@linux.intel.com> wrote: >> On 5/29/24 4:21 PM, Dmitry Baryshkov wrote: >>> On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote: >>>> The domain allocated in msm_iommu_new() is for the @dev. Replace >>>> iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. >>>> >>>> Update msm_iommu_new() to always return ERR_PTR in failure cases instead >>>> of NULL. >>> Please don't mix unrelated changes, because ... >>> >>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c >>>> index d5512037c38b..f7e28d4b5f62 100644 >>>> --- a/drivers/gpu/drm/msm/msm_iommu.c >>>> +++ b/drivers/gpu/drm/msm/msm_iommu.c >>>> @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) >>>> struct msm_iommu *iommu; >>>> int ret; >>>> >>>> - domain = iommu_domain_alloc(dev->bus); >>>> - if (!domain) >>>> - return NULL; >>>> + domain = iommu_paging_domain_alloc(dev); >>>> + if (IS_ERR(domain)) >>>> + return ERR_CAST(domain); >>>> >>>> iommu_set_pgtable_quirks(domain, quirks); >>>> >>>> @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig >>>> struct msm_mmu *mmu; >>>> >>>> mmu = msm_iommu_new(dev, quirks); >>>> - if (IS_ERR_OR_NULL(mmu)) >>>> + if (IS_ERR(mmu)) >>>> return mmu; >>> NAK, not having an IOMMU is a poor but legit usecase for some of devices >>> which don't have IOMMU support yet (for example because of the buggy >>> implementation for which we were not able to get all the hooks in). >>> >>> Please don't break compatibility for existing platforms. >> Sure. I will remove this line of change. Though I have no idea in which >> case msm_iommu_new() could return NULL after this patch. > So, even without this chunk you are going to break the no-IOMMU case. > Please don't. This will result in a regression report and a revert. > > Instead please provide a way for the existing drivers to continue > working. For example, something like: > > if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV)) > return NULL; Oh I see. msm_iommu_new() returning NULL indicates a no-IOMMU case, right? So perhaps we can make it explicit like below? if (!device_iommu_mapped(dev)) return NULL; domain = iommu_paging_domain_alloc(dev); if (IS_ERR(domain)) return ERR_CAST(domain); Best regards, baolu
On Fri, May 31, 2024 at 09:57:54AM +0800, Baolu Lu wrote: > On 5/30/24 3:58 PM, Dmitry Baryshkov wrote: > > On Thu, 30 May 2024 at 04:59, Baolu Lu<baolu.lu@linux.intel.com> wrote: > > > On 5/29/24 4:21 PM, Dmitry Baryshkov wrote: > > > > On Wed, May 29, 2024 at 01:32:36PM +0800, Lu Baolu wrote: > > > > > The domain allocated in msm_iommu_new() is for the @dev. Replace > > > > > iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. > > > > > > > > > > Update msm_iommu_new() to always return ERR_PTR in failure cases instead > > > > > of NULL. > > > > Please don't mix unrelated changes, because ... > > > > > > > > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > > > > > index d5512037c38b..f7e28d4b5f62 100644 > > > > > --- a/drivers/gpu/drm/msm/msm_iommu.c > > > > > +++ b/drivers/gpu/drm/msm/msm_iommu.c > > > > > @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) > > > > > struct msm_iommu *iommu; > > > > > int ret; > > > > > > > > > > - domain = iommu_domain_alloc(dev->bus); > > > > > - if (!domain) > > > > > - return NULL; > > > > > + domain = iommu_paging_domain_alloc(dev); > > > > > + if (IS_ERR(domain)) > > > > > + return ERR_CAST(domain); > > > > > > > > > > iommu_set_pgtable_quirks(domain, quirks); > > > > > > > > > > @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig > > > > > struct msm_mmu *mmu; > > > > > > > > > > mmu = msm_iommu_new(dev, quirks); > > > > > - if (IS_ERR_OR_NULL(mmu)) > > > > > + if (IS_ERR(mmu)) > > > > > return mmu; > > > > NAK, not having an IOMMU is a poor but legit usecase for some of devices > > > > which don't have IOMMU support yet (for example because of the buggy > > > > implementation for which we were not able to get all the hooks in). > > > > > > > > Please don't break compatibility for existing platforms. > > > Sure. I will remove this line of change. Though I have no idea in which > > > case msm_iommu_new() could return NULL after this patch. > > So, even without this chunk you are going to break the no-IOMMU case. > > Please don't. This will result in a regression report and a revert. > > > > Instead please provide a way for the existing drivers to continue > > working. For example, something like: > > > > if (IS_ERR(mmu) && ERR_PTR(mmu) == -ENODEV)) > > return NULL; > > Oh I see. msm_iommu_new() returning NULL indicates a no-IOMMU case, > right? So perhaps we can make it explicit like below? > > if (!device_iommu_mapped(dev)) > return NULL; > > domain = iommu_paging_domain_alloc(dev); > if (IS_ERR(domain)) > return ERR_CAST(domain); Yes, this should work, thank you.
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index d5512037c38b..f7e28d4b5f62 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -407,9 +407,9 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks) struct msm_iommu *iommu; int ret; - domain = iommu_domain_alloc(dev->bus); - if (!domain) - return NULL; + domain = iommu_paging_domain_alloc(dev); + if (IS_ERR(domain)) + return ERR_CAST(domain); iommu_set_pgtable_quirks(domain, quirks); @@ -441,7 +441,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig struct msm_mmu *mmu; mmu = msm_iommu_new(dev, quirks); - if (IS_ERR_OR_NULL(mmu)) + if (IS_ERR(mmu)) return mmu; iommu = to_msm_iommu(mmu);
The domain allocated in msm_iommu_new() is for the @dev. Replace iommu_domain_alloc() with iommu_paging_domain_alloc() to make it explicit. Update msm_iommu_new() to always return ERR_PTR in failure cases instead of NULL. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/gpu/drm/msm/msm_iommu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)