[PATCH] iommu/iova: Fix module config properly

[PATCH] iommu/iova: Fix module config properly

[Robin Murphy]

IOMMU_IOVA is intended to be an optional library for users to select as
and when they desire. Since it can be a module now, this means that
built-in code which has chosen not to select it should not fail to link
if it happens to have selected as a module by someone else. Replace
IS_ENABLED() with IS_REACHABLE() to do the right thing.

CC: Thierry Reding <thierry.reding@gmail.com>
Reported-by: John Garry <john.garry@huawei.com>
Fixes: 15bbdec3931e ("iommu: Make the iova library a module")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Phrased as a fix for the sake of complete honesty, but it seems
everyone's been making do for years already so by now it's really
just more of an enhancement.

 include/linux/iova.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iova.h b/include/linux/iova.h
index 8f97a43be834..fe18c3e6a725 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -75,7 +75,7 @@ static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
 	return iova >> iova_shift(iovad);
 }
 
-#if IS_ENABLED(CONFIG_IOMMU_IOVA)
+#if IS_REACHABLE(CONFIG_IOMMU_IOVA)
 int iova_cache_get(void);
 void iova_cache_put(void);
 
-- 
2.36.1.dirty

Re: [PATCH] iommu/iova: Fix module config properly

[John Garry]

On 13/09/2022 12:47, Robin Murphy wrote:
> IOMMU_IOVA is intended to be an optional library for users to select as
> and when they desire. Since it can be a module now, this means that
> built-in code which has chosen not to select it should not fail to link
> if it happens to have selected as a module by someone else. Replace
> IS_ENABLED() with IS_REACHABLE() to do the right thing.

Hi Robin,

Recently you mentioned "I wonder if we couldn't replace the IS_ENABLED() 
with IS_REACHABLE() and restore some of the previously-conditional 
selects", and pointed me to 84db889e6d82 as an example of when a 
conditional select was made unconditional.

So will you also restore some previously-conditional selects next?

To me, it seems that any user of IOVA API selects IOMMU_IOVA always.

thanks,
John

> 
> CC: Thierry Reding <thierry.reding@gmail.com>
> Reported-by: John Garry <john.garry@huawei.com>
> Fixes: 15bbdec3931e ("iommu: Make the iova library a module")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Phrased as a fix for the sake of complete honesty, but it seems
> everyone's been making do for years already so by now it's really
> just more of an enhancement.
> 
>   include/linux/iova.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/iova.h b/include/linux/iova.h
> index 8f97a43be834..fe18c3e6a725 100644
> --- a/include/linux/iova.h
> +++ b/include/linux/iova.h
> @@ -75,7 +75,7 @@ static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
>   	return iova >> iova_shift(iovad);
>   }
>   
> -#if IS_ENABLED(CONFIG_IOMMU_IOVA)
> +#if IS_REACHABLE(CONFIG_IOMMU_IOVA)
>   int iova_cache_get(void);
>   void iova_cache_put(void);
>   

Re: [PATCH] iommu/iova: Fix module config properly

[Robin Murphy]

On 2022-09-13 14:01, John Garry wrote:
> On 13/09/2022 12:47, Robin Murphy wrote:
>> IOMMU_IOVA is intended to be an optional library for users to select as
>> and when they desire. Since it can be a module now, this means that
>> built-in code which has chosen not to select it should not fail to link
>> if it happens to have selected as a module by someone else. Replace
>> IS_ENABLED() with IS_REACHABLE() to do the right thing.
> 
> Hi Robin,
> 
> Recently you mentioned "I wonder if we couldn't replace the IS_ENABLED() 
> with IS_REACHABLE() and restore some of the previously-conditional 
> selects", and pointed me to 84db889e6d82 as an example of when a 
> conditional select was made unconditional.
> 
> So will you also restore some previously-conditional selects next?

I figured I'd leave that up to Thierry (and/or anyone else with a vested 
interest), but having mulled it over since that previous thread, there's 
really no excuse for the API itself not to do the right thing either 
way, so I felt compelled to write up this much.

Cheers,
Robin.

> 
> To me, it seems that any user of IOVA API selects IOMMU_IOVA always.
> 
> thanks,
> John
> 
>>
>> CC: Thierry Reding <thierry.reding@gmail.com>
>> Reported-by: John Garry <john.garry@huawei.com>
>> Fixes: 15bbdec3931e ("iommu: Make the iova library a module")
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Phrased as a fix for the sake of complete honesty, but it seems
>> everyone's been making do for years already so by now it's really
>> just more of an enhancement.
>>
>>   include/linux/iova.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>> index 8f97a43be834..fe18c3e6a725 100644
>> --- a/include/linux/iova.h
>> +++ b/include/linux/iova.h
>> @@ -75,7 +75,7 @@ static inline unsigned long iova_pfn(struct 
>> iova_domain *iovad, dma_addr_t iova)
>>       return iova >> iova_shift(iovad);
>>   }
>> -#if IS_ENABLED(CONFIG_IOMMU_IOVA)
>> +#if IS_REACHABLE(CONFIG_IOMMU_IOVA)
>>   int iova_cache_get(void);
>>   void iova_cache_put(void);
> 

Re: [PATCH] iommu/iova: Fix module config properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

On Tue, Sep 13, 2022 at 03:15:18PM +0100, Robin Murphy wrote:
> On 2022-09-13 14:01, John Garry wrote:
> > On 13/09/2022 12:47, Robin Murphy wrote:
> > > IOMMU_IOVA is intended to be an optional library for users to select as
> > > and when they desire. Since it can be a module now, this means that
> > > built-in code which has chosen not to select it should not fail to link
> > > if it happens to have selected as a module by someone else. Replace
> > > IS_ENABLED() with IS_REACHABLE() to do the right thing.
> > 
> > Hi Robin,
> > 
> > Recently you mentioned "I wonder if we couldn't replace the IS_ENABLED()
> > with IS_REACHABLE() and restore some of the previously-conditional
> > selects", and pointed me to 84db889e6d82 as an example of when a
> > conditional select was made unconditional.
> > 
> > So will you also restore some previously-conditional selects next?
> 
> I figured I'd leave that up to Thierry (and/or anyone else with a vested
> interest), but having mulled it over since that previous thread, there's
> really no excuse for the API itself not to do the right thing either way, so
> I felt compelled to write up this much.

On Tegra specifically, as the commit message says, we don't really care
about the conditional selection because practically we always want IOMMU
support enabled. So instead of adding back the conditional select it
would make more sense to select IOMMU_API instead and then get rid of
the handful of #ifdef blocks we have for that.

On a side note: I'm looking at a subtle regression right now where some
corner case no longer works and that's primarily due to the fact that we
can have four different scenarios: non-IOMMU, IOMMU, DMA/IOMMU and IOMMU
but devices with no iommus property in their DT nodes. This has all
become very unwieldy and becomes increasingly difficult to test. So
anything that we can do to keep down the number of permutations is
welcome.

While that's not directly related to this issue, the lesson learned is
that, at least on the Tegra side, we have in the past made things
unnecessarily difficult for ourselves based on the misguided assumption
that maximum configurability is key.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] iommu/iova: Fix module config properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Tue, Sep 13, 2022 at 12:47:20PM +0100, Robin Murphy wrote:
> IOMMU_IOVA is intended to be an optional library for users to select as
> and when they desire. Since it can be a module now, this means that
> built-in code which has chosen not to select it should not fail to link
> if it happens to have selected as a module by someone else. Replace
> IS_ENABLED() with IS_REACHABLE() to do the right thing.
> 
> CC: Thierry Reding <thierry.reding@gmail.com>
> Reported-by: John Garry <john.garry@huawei.com>
> Fixes: 15bbdec3931e ("iommu: Make the iova library a module")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Phrased as a fix for the sake of complete honesty, but it seems
> everyone's been making do for years already so by now it's really
> just more of an enhancement.
> 
>  include/linux/iova.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] iommu/iova: Fix module config properly

[John Garry]

On 14/09/2022 10:47, Thierry Reding wrote:
> On Tue, Sep 13, 2022 at 03:15:18PM +0100, Robin Murphy wrote:
>> On 2022-09-13 14:01, John Garry wrote:
>>> On 13/09/2022 12:47, Robin Murphy wrote:
>>>> IOMMU_IOVA is intended to be an optional library for users to select as
>>>> and when they desire. Since it can be a module now, this means that
>>>> built-in code which has chosen not to select it should not fail to link
>>>> if it happens to have selected as a module by someone else. Replace
>>>> IS_ENABLED() with IS_REACHABLE() to do the right thing.
>>>
>>> Hi Robin,
>>>
>>> Recently you mentioned "I wonder if we couldn't replace the IS_ENABLED()
>>> with IS_REACHABLE() and restore some of the previously-conditional
>>> selects", and pointed me to 84db889e6d82 as an example of when a
>>> conditional select was made unconditional.
>>>
>>> So will you also restore some previously-conditional selects next?
>>
>> I figured I'd leave that up to Thierry (and/or anyone else with a vested
>> interest), but having mulled it over since that previous thread, there's
>> really no excuse for the API itself not to do the right thing either way, so
>> I felt compelled to write up this much.
> 
> On Tegra specifically, as the commit message says, we don't really care
> about the conditional selection because practically we always want IOMMU
> support enabled. So instead of adding back the conditional select it
> would make more sense to select IOMMU_API instead and then get rid of
> the handful of #ifdef blocks we have for that.

Out of curiosity, does the same go to host1x, whose kconfig got the same 
treatment as tegra with regards to selecting IOMMU_IOVA? I mean, will 
you not go back to conditionally selecting IOMMU_IOVA, and instead 
select IOMMU_API and IOMMU_IOVA always?

Thanks,
John

> 
> On a side note: I'm looking at a subtle regression right now where some
> corner case no longer works and that's primarily due to the fact that we
> can have four different scenarios: non-IOMMU, IOMMU, DMA/IOMMU and IOMMU
> but devices with no iommus property in their DT nodes. This has all
> become very unwieldy and becomes increasingly difficult to test. So
> anything that we can do to keep down the number of permutations is
> welcome.
> 
> While that's not directly related to this issue, the lesson learned is
> that, at least on the Tegra side, we have in the past made things
> unnecessarily difficult for ourselves based on the misguided assumption
> that maximum configurability is key.
> 
> Thierry

Re: [PATCH] iommu/iova: Fix module config properly

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

On Thu, Sep 15, 2022 at 12:45:43PM +0100, John Garry wrote:
> On 14/09/2022 10:47, Thierry Reding wrote:
> > On Tue, Sep 13, 2022 at 03:15:18PM +0100, Robin Murphy wrote:
> > > On 2022-09-13 14:01, John Garry wrote:
> > > > On 13/09/2022 12:47, Robin Murphy wrote:
> > > > > IOMMU_IOVA is intended to be an optional library for users to select as
> > > > > and when they desire. Since it can be a module now, this means that
> > > > > built-in code which has chosen not to select it should not fail to link
> > > > > if it happens to have selected as a module by someone else. Replace
> > > > > IS_ENABLED() with IS_REACHABLE() to do the right thing.
> > > > 
> > > > Hi Robin,
> > > > 
> > > > Recently you mentioned "I wonder if we couldn't replace the IS_ENABLED()
> > > > with IS_REACHABLE() and restore some of the previously-conditional
> > > > selects", and pointed me to 84db889e6d82 as an example of when a
> > > > conditional select was made unconditional.
> > > > 
> > > > So will you also restore some previously-conditional selects next?
> > > 
> > > I figured I'd leave that up to Thierry (and/or anyone else with a vested
> > > interest), but having mulled it over since that previous thread, there's
> > > really no excuse for the API itself not to do the right thing either way, so
> > > I felt compelled to write up this much.
> > 
> > On Tegra specifically, as the commit message says, we don't really care
> > about the conditional selection because practically we always want IOMMU
> > support enabled. So instead of adding back the conditional select it
> > would make more sense to select IOMMU_API instead and then get rid of
> > the handful of #ifdef blocks we have for that.
> 
> Out of curiosity, does the same go to host1x, whose kconfig got the same
> treatment as tegra with regards to selecting IOMMU_IOVA? I mean, will you
> not go back to conditionally selecting IOMMU_IOVA, and instead select
> IOMMU_API and IOMMU_IOVA always?

Yeah, I suspect that that will happen eventually. People would still
have the option of disabling runtime IOMMU support by disabling the
IOMMU via DT, for example, so if they really care about that last bit
of performance, they do have that option.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] iommu/iova: Fix module config properly

[Joerg Roedel]

On Tue, Sep 13, 2022 at 12:47:20PM +0100, Robin Murphy wrote:
>  include/linux/iova.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.