Let the core code fiddle with the MSI descriptor retrieval.
Signed-off-by: Thomas Gleixner <[email protected]>
---
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);
/* Add callback to free MSIs on teardown */
devm_add_action(dev, arm_smmu_free_msis, dev);
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
>> ---
>> 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.
On 2021-11-27 01:22, Thomas Gleixner wrote:
> Let the core code fiddle with the MSI descriptor retrieval.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> 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 <[email protected]>
Cheers,
Robin.
> /* Add callback to free MSIs on teardown */
> devm_add_action(dev, arm_smmu_free_msis, dev);
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
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
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
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
>
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
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