diff mbox series

[33/37] iommu/arm-smmu-v3: Use msi_get_virq()

Message ID 20211126230525.885757679@linutronix.de (mailing list archive)
State New, archived
Headers show
Series genirq/msi, PCI/MSI: Spring cleaning - Part 2 | expand

Commit Message

Thomas Gleixner Nov. 27, 2021, 1:20 a.m. UTC
Let the core code fiddle with the MSI descriptor retrieval.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

Comments

Will Deacon Nov. 29, 2021, 10:55 a.m. UTC | #1
Hi Thomas,

On Sat, Nov 27, 2021 at 02:20:59AM +0100, Thomas Gleixner wrote:
> Let the core code fiddle with the MSI descriptor retrieval.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3154,7 +3154,6 @@ static void arm_smmu_write_msi_msg(struc
>  
>  static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  {
> -	struct msi_desc *desc;
>  	int ret, nvec = ARM_SMMU_MAX_MSIS;
>  	struct device *dev = smmu->dev;
>  
> @@ -3182,21 +3181,9 @@ static void arm_smmu_setup_msis(struct a
>  		return;
>  	}
>  
> -	for_each_msi_entry(desc, dev) {
> -		switch (desc->msi_index) {
> -		case EVTQ_MSI_INDEX:
> -			smmu->evtq.q.irq = desc->irq;
> -			break;
> -		case GERROR_MSI_INDEX:
> -			smmu->gerr_irq = desc->irq;
> -			break;
> -		case PRIQ_MSI_INDEX:
> -			smmu->priq.q.irq = desc->irq;
> -			break;
> -		default:	/* Unknown */
> -			continue;
> -		}
> -	}
> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);

Prviously, if retrieval of the MSI failed then we'd fall back to wired
interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
we make the assignments to smmu->*irq here conditional on the MSI being
valid, please?

Cheers,

Will
Thomas Gleixner Nov. 29, 2021, 12:52 p.m. UTC | #2
Will,

On Mon, Nov 29 2021 at 10:55, Will Deacon wrote:
> On Sat, Nov 27, 2021 at 02:20:59AM +0100, Thomas Gleixner wrote:
>> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
>> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
>> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
>
> Prviously, if retrieval of the MSI failed then we'd fall back to wired
> interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
> we make the assignments to smmu->*irq here conditional on the MSI being
> valid, please?

So the wired irq number is in ->irq already and MSI does an override
if available. Not really obvious...

Thanks,

        tglx
Thomas Gleixner Nov. 29, 2021, 12:58 p.m. UTC | #3
On Mon, Nov 29 2021 at 13:52, Thomas Gleixner wrote:
> On Mon, Nov 29 2021 at 10:55, Will Deacon wrote:
>> On Sat, Nov 27, 2021 at 02:20:59AM +0100, Thomas Gleixner wrote:
>>> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
>>> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
>>> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
>>
>> Prviously, if retrieval of the MSI failed then we'd fall back to wired
>> interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
>> we make the assignments to smmu->*irq here conditional on the MSI being
>> valid, please?
>
> So the wired irq number is in ->irq already and MSI does an override
> if available. Not really obvious...

But, this happens right after:

     ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);

So if that succeeded then the descriptors exist and have interrupts
assigned.

Thanks,

        tglx
Robin Murphy Nov. 29, 2021, 1:13 p.m. UTC | #4
On 2021-11-29 10:55, Will Deacon wrote:
> Hi Thomas,
> 
> On Sat, Nov 27, 2021 at 02:20:59AM +0100, Thomas Gleixner wrote:
>> Let the core code fiddle with the MSI descriptor retrieval.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   19 +++----------------
>>   1 file changed, 3 insertions(+), 16 deletions(-)
>>
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -3154,7 +3154,6 @@ static void arm_smmu_write_msi_msg(struc
>>   
>>   static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>   {
>> -	struct msi_desc *desc;
>>   	int ret, nvec = ARM_SMMU_MAX_MSIS;
>>   	struct device *dev = smmu->dev;
>>   
>> @@ -3182,21 +3181,9 @@ static void arm_smmu_setup_msis(struct a
>>   		return;
>>   	}
>>   
>> -	for_each_msi_entry(desc, dev) {
>> -		switch (desc->msi_index) {
>> -		case EVTQ_MSI_INDEX:
>> -			smmu->evtq.q.irq = desc->irq;
>> -			break;
>> -		case GERROR_MSI_INDEX:
>> -			smmu->gerr_irq = desc->irq;
>> -			break;
>> -		case PRIQ_MSI_INDEX:
>> -			smmu->priq.q.irq = desc->irq;
>> -			break;
>> -		default:	/* Unknown */
>> -			continue;
>> -		}
>> -	}
>> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
>> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
>> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
> 
> Prviously, if retrieval of the MSI failed then we'd fall back to wired
> interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
> we make the assignments to smmu->*irq here conditional on the MSI being
> valid, please?

I was just looking at that too, but reached the conclusion that it's 
probably OK, since consumption of this value later is gated on 
ARM_SMMU_FEAT_PRI, so the fact that it changes from 0 to an error value 
in the absence of PRI should make no practical difference. If we don't 
have MSIs at all, we'd presumably still fail earlier either at the 
dev->msi_domain check or upon trying to allocate the vectors, so we'll 
still fall back to any previously-set wired values before getting here. 
The only remaining case is if we've *successfully* allocated the 
expected number of vectors yet are then somehow unable to retrieve one 
or more of them - presumably the system has to be massively borked for 
that to happen, at which point do we really want to bother trying to 
reason about anything?

Robin.
Robin Murphy Nov. 29, 2021, 1:25 p.m. UTC | #5
On 2021-11-27 01:22, Thomas Gleixner wrote:
> Let the core code fiddle with the MSI descriptor retrieval.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   19 +++----------------
>   1 file changed, 3 insertions(+), 16 deletions(-)
> 
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3154,7 +3154,6 @@ static void arm_smmu_write_msi_msg(struc
>   
>   static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>   {
> -	struct msi_desc *desc;
>   	int ret, nvec = ARM_SMMU_MAX_MSIS;
>   	struct device *dev = smmu->dev;
>   
> @@ -3182,21 +3181,9 @@ static void arm_smmu_setup_msis(struct a
>   		return;
>   	}
>   
> -	for_each_msi_entry(desc, dev) {
> -		switch (desc->msi_index) {
> -		case EVTQ_MSI_INDEX:
> -			smmu->evtq.q.irq = desc->irq;
> -			break;
> -		case GERROR_MSI_INDEX:
> -			smmu->gerr_irq = desc->irq;
> -			break;
> -		case PRIQ_MSI_INDEX:
> -			smmu->priq.q.irq = desc->irq;
> -			break;
> -		default:	/* Unknown */
> -			continue;
> -		}
> -	}
> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);

FWIW I've just quickly booted the msi-v1-part-2 branch on a platform 
with MSIs but no PRI such that this now sets priq.q.irq to an error 
value, and as I predicted it's still happy.

Tested-by: Robin Murphy <robin.murphy@arm.com>

Cheers,
Robin.

>   	/* Add callback to free MSIs on teardown */
>   	devm_add_action(dev, arm_smmu_free_msis, dev);
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
Thomas Gleixner Nov. 29, 2021, 2:42 p.m. UTC | #6
On Mon, Nov 29 2021 at 13:13, Robin Murphy wrote:
> On 2021-11-29 10:55, Will Deacon wrote:
>>> -	}
>>> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
>>> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
>>> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
>> 
>> Prviously, if retrieval of the MSI failed then we'd fall back to wired
>> interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
>> we make the assignments to smmu->*irq here conditional on the MSI being
>> valid, please?
>
> I was just looking at that too, but reached the conclusion that it's 
> probably OK, since consumption of this value later is gated on 
> ARM_SMMU_FEAT_PRI, so the fact that it changes from 0 to an error value 
> in the absence of PRI should make no practical difference.

It's actually 0 when the vector cannot be found.

> If we don't have MSIs at all, we'd presumably still fail earlier
> either at the dev->msi_domain check or upon trying to allocate the
> vectors, so we'll still fall back to any previously-set wired values
> before getting here.  The only remaining case is if we've
> *successfully* allocated the expected number of vectors yet are then
> somehow unable to retrieve one or more of them - presumably the system
> has to be massively borked for that to happen, at which point do we
> really want to bother trying to reason about anything?

Probably not. At that point something is going to explode sooner than
later in colorful ways.

Thanks,

        tglx
Robin Murphy Nov. 29, 2021, 2:54 p.m. UTC | #7
On 2021-11-29 14:42, Thomas Gleixner wrote:
> On Mon, Nov 29 2021 at 13:13, Robin Murphy wrote:
>> On 2021-11-29 10:55, Will Deacon wrote:
>>>> -	}
>>>> +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
>>>> +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
>>>> +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
>>>
>>> Prviously, if retrieval of the MSI failed then we'd fall back to wired
>>> interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
>>> we make the assignments to smmu->*irq here conditional on the MSI being
>>> valid, please?
>>
>> I was just looking at that too, but reached the conclusion that it's
>> probably OK, since consumption of this value later is gated on
>> ARM_SMMU_FEAT_PRI, so the fact that it changes from 0 to an error value
>> in the absence of PRI should make no practical difference.
> 
> It's actually 0 when the vector cannot be found.

Oh, -1 for my reading comprehension but +1 for my confidence in the 
patch then :)

I'll let Will have the final say over how cautious we really want to be 
here, but as far as I'm concerned it's a welcome cleanup as-is. Ditto 
for patch #32 based on the same reasoning, although I don't have a 
suitable test platform on-hand to sanity-check that one.

Cheers,
Robin.

>> If we don't have MSIs at all, we'd presumably still fail earlier
>> either at the dev->msi_domain check or upon trying to allocate the
>> vectors, so we'll still fall back to any previously-set wired values
>> before getting here.  The only remaining case is if we've
>> *successfully* allocated the expected number of vectors yet are then
>> somehow unable to retrieve one or more of them - presumably the system
>> has to be massively borked for that to happen, at which point do we
>> really want to bother trying to reason about anything?
> 
> Probably not. At that point something is going to explode sooner than
> later in colorful ways.
> 
> Thanks,
> 
>          tglx
>
Will Deacon Nov. 30, 2021, 9:36 a.m. UTC | #8
On Mon, Nov 29, 2021 at 02:54:18PM +0000, Robin Murphy wrote:
> On 2021-11-29 14:42, Thomas Gleixner wrote:
> > On Mon, Nov 29 2021 at 13:13, Robin Murphy wrote:
> > > On 2021-11-29 10:55, Will Deacon wrote:
> > > > > -	}
> > > > > +	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
> > > > > +	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
> > > > > +	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
> > > > 
> > > > Prviously, if retrieval of the MSI failed then we'd fall back to wired
> > > > interrupts. Now, I think we'll clobber the interrupt with 0 instead. Can
> > > > we make the assignments to smmu->*irq here conditional on the MSI being
> > > > valid, please?
> > > 
> > > I was just looking at that too, but reached the conclusion that it's
> > > probably OK, since consumption of this value later is gated on
> > > ARM_SMMU_FEAT_PRI, so the fact that it changes from 0 to an error value
> > > in the absence of PRI should make no practical difference.
> > 
> > It's actually 0 when the vector cannot be found.
> 
> Oh, -1 for my reading comprehension but +1 for my confidence in the patch
> then :)
> 
> I'll let Will have the final say over how cautious we really want to be
> here, but as far as I'm concerned it's a welcome cleanup as-is. Ditto for
> patch #32 based on the same reasoning, although I don't have a suitable test
> platform on-hand to sanity-check that one.

If, as it appears, msi_get_virq() isn't going to fail meaningfully after
we've successfully called platform_msi_domain_alloc_irqs() then it sounds
like the patch is fine. Just wanted to check though, as Spring cleaning at
the end of November raised an eyebrow over here :)

Will
Thomas Gleixner Nov. 30, 2021, 12:30 p.m. UTC | #9
On Tue, Nov 30 2021 at 09:36, Will Deacon wrote:
> On Mon, Nov 29, 2021 at 02:54:18PM +0000, Robin Murphy wrote:
>> On 2021-11-29 14:42, Thomas Gleixner wrote:
>> > It's actually 0 when the vector cannot be found.
>> 
>> Oh, -1 for my reading comprehension but +1 for my confidence in the patch
>> then :)
>> 
>> I'll let Will have the final say over how cautious we really want to be
>> here, but as far as I'm concerned it's a welcome cleanup as-is. Ditto for
>> patch #32 based on the same reasoning, although I don't have a suitable test
>> platform on-hand to sanity-check that one.
>
> If, as it appears, msi_get_virq() isn't going to fail meaningfully after
> we've successfully called platform_msi_domain_alloc_irqs() then it sounds
> like the patch is fine. Just wanted to check though, as Spring cleaning at
> the end of November raised an eyebrow over here :)

Fair enough. Next time I'll name it 'Cleaning the Augean stables' when
it's the wrong season.

Thanks,

        tglx
diff mbox series

Patch

--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3154,7 +3154,6 @@  static void arm_smmu_write_msi_msg(struc
 
 static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 {
-	struct msi_desc *desc;
 	int ret, nvec = ARM_SMMU_MAX_MSIS;
 	struct device *dev = smmu->dev;
 
@@ -3182,21 +3181,9 @@  static void arm_smmu_setup_msis(struct a
 		return;
 	}
 
-	for_each_msi_entry(desc, dev) {
-		switch (desc->msi_index) {
-		case EVTQ_MSI_INDEX:
-			smmu->evtq.q.irq = desc->irq;
-			break;
-		case GERROR_MSI_INDEX:
-			smmu->gerr_irq = desc->irq;
-			break;
-		case PRIQ_MSI_INDEX:
-			smmu->priq.q.irq = desc->irq;
-			break;
-		default:	/* Unknown */
-			continue;
-		}
-	}
+	smmu->evtq.q.irq = msi_get_virq(dev, EVTQ_MSI_INDEX);
+	smmu->gerr_irq = msi_get_virq(dev, GERROR_MSI_INDEX);
+	smmu->priq.q.irq = msi_get_virq(dev, PRIQ_MSI_INDEX);
 
 	/* Add callback to free MSIs on teardown */
 	devm_add_action(dev, arm_smmu_free_msis, dev);