Re: [RFC PATCH 1/5] iommu/arm-smmu-v3: Refactor arm_smmu_setup_irqs

[Pranjal Shrivastava <praan@google.com>]

On Thu, Mar 20, 2025 at 10:29:42PM +0000, Mostafa Saleh wrote:
> On Wed, Mar 19, 2025 at 12:42:50AM +0000, Pranjal Shrivastava wrote:
> > Refactor arm_smmu_setup_irqs by splitting it into two parts, one for
> > registering interrupt handlers and the other one for enabling interrupt
> > generation in the hardware. This refactor helps in re-initialization of
> > hardware interrupts as part of a subsequent patch that enables runtime
> > power management for the arm-smmu-v3 driver.
> > 
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 ++++++++++++++++-----
> >  1 file changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 358072b4e293..80362d9f293b 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3985,12 +3985,24 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
> >  	}
> >  }
> >  
> > -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> > +static void arm_smmu_enable_irqs(struct arm_smmu_device *smmu)
> >  {
> > -	int ret, irq;
> > +	int ret;
> >  	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> >  
> > -	/* Disable IRQs first */
> > +	if (smmu->features & ARM_SMMU_FEAT_PRI)
> > +		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> > +
> > +	ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> > +				      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> > +	if (ret)
> > +		dev_warn(smmu->dev, "failed to enable irqs\n");
> > +}
> > +
> > +static int arm_smmu_disable_irqs(struct arm_smmu_device *smmu)
> > +{
> > +	int ret;
> > +
> >  	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> >  				      ARM_SMMU_IRQ_CTRLACK);
> >  	if (ret) {
> > @@ -3998,6 +4010,19 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> >  		return ret;
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> > +{
> > +	int ret, irq;
> > +
> > +	/* Disable IRQs first */
> > +	ret = arm_smmu_disable_irqs(smmu);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> >  	irq = smmu->combined_irq;
> >  	if (irq) {
> >  		/*
> > @@ -4014,15 +4039,6 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> >  	} else
> >  		arm_smmu_setup_unique_irqs(smmu);
> >  
> > -	if (smmu->features & ARM_SMMU_FEAT_PRI)
> > -		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> > -
> > -	/* Enable interrupt generation on the SMMU */
> > -	ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> > -				      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> > -	if (ret)
> > -		dev_warn(smmu->dev, "failed to enable irqs\n");
> > -
> >  	return 0;
> >  }
> >  
> > @@ -4171,6 +4187,9 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable interrupt generation on the SMMU */
> > +	arm_smmu_enable_irqs(smmu);
> > +
> >  	if (is_kdump_kernel())
> >  		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
> >  
> > @@ -4754,6 +4773,13 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >  	/* Check for RMRs and install bypass STEs if any */
> >  	arm_smmu_rmr_install_bypass_ste(smmu);
> >  
> > +	/* Setup interrupt handlers */
> > +	ret = arm_smmu_setup_irqs(smmu);
> > +	if (ret) {
> > +		dev_err(smmu->dev, "failed to setup irqs\n");
> 
> Every path that can fail in this function has a print already, I’d say
> remove the ones inside and keep this one, that also simplifies
> arm_smmu_disable_irqs() to one line.

I guess as per Nicolin's comment, this call would go away and only be
called during probe. But yes the enable_irqs() will be here. I guess I
could move the print statement out of enable irqs and put it here?

The arm_smmu_disable_irqs will be called from the setup_irqs mainly,
during probe, so I think we could leave it's print as is..

> 
> Otherwise, besides Nicolin’s comment, it looks good.
> 
> Thanks,
> Mostafa

Thanks,
Praan