| Message ID | 1498758015-7072-1-git-send-email-nwatters@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote: > The shutdown method disables the SMMU and its interrupts to avoid > potentially corrupting a new kernel started with kexec. > > Signed-off-by: Nate Watterson <nwatters@codeaurora.org> > --- > drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) We should update arm-smmu.c as well. > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 380969a..907d576 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev) > struct arm_smmu_device *smmu = platform_get_drvdata(pdev); > > arm_smmu_device_disable(smmu); > + > + /* Disable IRQs */ > + arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, > + ARM_SMMU_IRQ_CTRLACK); > + Can you justify the need for this? If we actually need to disable interrupts, then I'd like to understand why so that we can make sure we get the ordering right with respect to disabling the device. Also, do we need to clear the MSI registers too? My understanding is that kexec will mask irqs at the GIC, so there's not actually an issue here. Will
On 6/29/2017 2:34 PM, Will Deacon wrote: > On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote: >> The shutdown method disables the SMMU and its interrupts to avoid >> potentially corrupting a new kernel started with kexec. >> >> Signed-off-by: Nate Watterson <nwatters@codeaurora.org> >> --- >> drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) > > We should update arm-smmu.c as well. > >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 380969a..907d576 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev) >> struct arm_smmu_device *smmu = platform_get_drvdata(pdev); >> >> arm_smmu_device_disable(smmu); >> + >> + /* Disable IRQs */ >> + arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, >> + ARM_SMMU_IRQ_CTRLACK); >> + > > Can you justify the need for this? If we actually need to disable > interrupts, then I'd like to understand why so that we can make sure we > get the ordering right with respect to disabling the device. Also, do we > need to clear the MSI registers too? I have no justification. Based on the number of drivers that take care to prevent their HW from generating an interrupt, I thought it would be required, but I can't find any such requirement explicitly laid out in the documentation. When you mention the MSI registers do you mean, for instance, SMMU_GERROR_IRQ_CFG0? It looks like those are always cleared while initializing the SMMU so the case where an SMMU transitions from using MSIs to using wired interrupts between kernels will be handled properly. > > My understanding is that kexec will mask irqs at the GIC, so there's not > actually an issue here. > > Will >
On Thu, Jun 29, 2017 at 06:20:00PM -0400, Nate Watterson wrote: > On 6/29/2017 2:34 PM, Will Deacon wrote: > >On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote: > >>The shutdown method disables the SMMU and its interrupts to avoid > >>potentially corrupting a new kernel started with kexec. > >> > >>Signed-off-by: Nate Watterson <nwatters@codeaurora.org> > >>--- > >> drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > > > >We should update arm-smmu.c as well. > > > >>diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >>index 380969a..907d576 100644 > >>--- a/drivers/iommu/arm-smmu-v3.c > >>+++ b/drivers/iommu/arm-smmu-v3.c > >>@@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev) > >> struct arm_smmu_device *smmu = platform_get_drvdata(pdev); > >> arm_smmu_device_disable(smmu); > >>+ > >>+ /* Disable IRQs */ > >>+ arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, > >>+ ARM_SMMU_IRQ_CTRLACK); > >>+ > > > >Can you justify the need for this? If we actually need to disable > >interrupts, then I'd like to understand why so that we can make sure we > >get the ordering right with respect to disabling the device. Also, do we > >need to clear the MSI registers too? > > I have no justification. Based on the number of drivers that take care > to prevent their HW from generating an interrupt, I thought it would be > required, but I can't find any such requirement explicitly laid out in > the documentation. > > When you mention the MSI registers do you mean, for instance, > SMMU_GERROR_IRQ_CFG0? It looks like those are always cleared while > initializing the SMMU so the case where an SMMU transitions from using > MSIs to using wired interrupts between kernels will be handled properly. Sure, but if it's only the re-init path you're concerned about, then I don't think we have a problem for wired interrupts either. They're masked at the GIC until we do request_irq, and our handler can tolerate a spurious IRQ anyway. I assumed the race you were trying to close was an IRQ firing after we've disabled the device, but actually, I think the GIC is the right place to handle that too because propagation delay can cause late IRQs anyway. So unless there's a case I'm missing, let's not confuse the code by disabling IRQs for the sake of it. Will
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 380969a..907d576 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev) struct arm_smmu_device *smmu = platform_get_drvdata(pdev); arm_smmu_device_disable(smmu); + + /* Disable IRQs */ + arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, + ARM_SMMU_IRQ_CTRLACK); + return 0; } +static void arm_smmu_device_shutdown(struct platform_device *pdev) +{ + arm_smmu_device_remove(pdev); +} + static struct of_device_id arm_smmu_of_match[] = { { .compatible = "arm,smmu-v3", }, { }, @@ -2781,6 +2791,7 @@ static int arm_smmu_device_remove(struct platform_device *pdev) }, .probe = arm_smmu_device_probe, .remove = arm_smmu_device_remove, + .shutdown = arm_smmu_device_shutdown, }; module_platform_driver(arm_smmu_driver);
The shutdown method disables the SMMU and its interrupts to avoid potentially corrupting a new kernel started with kexec. Signed-off-by: Nate Watterson <nwatters@codeaurora.org> --- drivers/iommu/arm-smmu-v3.c | 11 +++++++++++ 1 file changed, 11 insertions(+)