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