Messages in this thread | | | Date | Tue, 4 Jun 2019 13:07:44 +0200 | From | Thierry Reding <> | Subject | Re: [PATCH v4 07/16] PM / devfreq: tegra: Properly disable interrupts |
| |
On Thu, May 02, 2019 at 02:38:06AM +0300, Dmitry Osipenko wrote: > There is no guarantee that interrupt handling isn't running in parallel > with tegra_actmon_disable_interrupts(), hence it is necessary to protect > DEV_CTRL register accesses and clear IRQ status with ACTMON's IRQ being > disabled in the Interrupt Controller in order to ensure that device > interrupt is indeed being disabled. > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/devfreq/tegra-devfreq.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c > index b65313fe3c2e..ce1eb97a2090 100644 > --- a/drivers/devfreq/tegra-devfreq.c > +++ b/drivers/devfreq/tegra-devfreq.c > @@ -171,6 +171,8 @@ struct tegra_devfreq { > struct notifier_block rate_change_nb; > > struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)]; > + > + int irq;
Interrupts are typically unsigned int.
> }; > > struct tegra_actmon_emc_ratio { > @@ -417,6 +419,8 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra) > u32 val; > unsigned int i; > > + disable_irq(tegra->irq); > + > for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) { > dev = &tegra->devices[i]; > > @@ -427,9 +431,14 @@ static void tegra_actmon_disable_interrupts(struct tegra_devfreq *tegra) > val &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN; > > device_writel(dev, val, ACTMON_DEV_CTRL); > + > + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, > + ACTMON_DEV_INTR_STATUS); > } > > actmon_write_barrier(tegra); > + > + enable_irq(tegra->irq);
Why do we enable interrupts after this? Is there any use in having the top-level interrupt enabled if nothing's going to generate an interrupt anyway?
> } > > static void tegra_actmon_configure_device(struct tegra_devfreq *tegra, > @@ -604,7 +613,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > struct resource *res; > unsigned int i; > unsigned long rate; > - int irq; > int err; > > tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); > @@ -673,15 +681,16 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > dev_pm_opp_add(&pdev->dev, rate, 0); > } > > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", irq); > - return irq; > + tegra->irq = platform_get_irq(pdev, 0); > + if (tegra->irq < 0) { > + err = tegra->irq; > + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err); > + return err; > }
This is very oddly written. tegra->irq should really be an unsigned int since that's the standard type for interrupt numbers. But since you need to be able to detect errors from platform_get_irq() it now becomes natural to write this as:
err = platform_get_irq(pdev, 0); if (err < 0) { dev_err(...); return err; }
tegra->irq = err;
Two birds with one stone. I suppose this could be done in a follow-up patch since it isn't practically wrong in your version, so either way:
Acked-by: Thierry Reding <treding@nvidia.com> [unhandled content-type:application/pgp-signature] | |