Re: [PATCH] driver/perf: Add PMU driver for the ARM DMC-620 memory controller.

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Apr 01, 2020 at 11:27:25AM +0100, Will Deacon wrote:
> On Wed, Apr 01, 2020 at 10:52:26AM +0100, Mark Rutland wrote:
> > On Tue, Mar 31, 2020 at 03:14:59PM -0700, Tuan Phan wrote:
> > > > On Mar 20, 2020, at 4:25 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > On Thu, Mar 19, 2020 at 12:03:43PM -0700, Tuan Phan wrote:
> > > >>> On Mar 19, 2020, at 8:16 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >>> On Tue, Mar 17, 2020 at 05:29:38PM -0700, Tuan Phan wrote:
> > > >>>> +static int arm_dmc620_pmu_dev_init(struct arm_dmc620_pmu *dmc620_pmu)
> > > >>>> +{
> > > >>>> +	struct platform_device *pdev = dmc620_pmu->pdev;
> > > >>>> +	int ret;
> > > >>>> +
> > > >>>> +	ret = devm_request_irq(&pdev->dev, dmc620_pmu->irq,
> > > >>>> +				arm_dmc620_pmu_handle_irq,
> > > >>>> +				IRQF_SHARED,
> > > >>>> +				dev_name(&pdev->dev), dmc620_pmu);
> > > >>> 
> > > >>> This should have IRQF_NOBALANCING | IRQF_NO_THREAD. I don't think we
> > > >>> should have IRQF_SHARED.
> > > >> => I agree on having IRQF_NOBALANCING and IRQF_NO_THREAD. But
> > > >> IRQF_SHARED is needed. In our platform all DMC620s share same IRQs and
> > > >> any cpus can access the pmu registers.
> > > > 
> > > > Linux needs to ensure that the same instance is concistently accessed
> > > > from the same CPU, and needs to migrate the IRQ to handle that. Given we
> > > > do that on a per-instance basis, we cannot share the IRQ with another
> > > > instance.
> > > > 
> > > > Please feed back to you HW designers that muxing IRQs like this causes
> > > > significant problems for software.
> > > 
> > > I looked at the SMMUv3 PMU driver and it also uses IRQF_SHARED. SMMUv3
> > > PMU and DMC620 PMU are very much similar in which counters can be
> > > accessed by any cores using memory map. Any special reasons
> > > IRQF_SHARED works with SMMUv3 PMU driver?
> > 
> > No; I believe that is a bug in the SMMUv3 PMU driver. If the IRQ were
> > shared, and another driver that held the IRQ changed the affinity,
> > things would go very wrong.
> 
> I *think* the idea is that the SMMUv3 PMU driver manages multiple PMCG
> devices, which may all share an irq line, rather than the irq line being
> shared by some other driver that might change the affinity. So I suspect
> dropping IRQF_SHARED will break things.

Ok. So long as each of the contexts are migrated before the IRQ is, I
think that's sound. Otherwise there's a small window where the IRQ
handler for an instance won't see the state expected (and could end up
treated as a screaming IRQ).

Otherwise, in that case I think that's not so bad.

> > Note that it's also missing IRQF_NOBALANCING, which is also necessary to
> > avoid such issues.
> 
>   unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;
> 
> so it looks like IRQF_NOBALANCING is already taken care of.

Whoops; I'd misread thhe DMC-620 code above assuming it was the SMMUv3
PMU.

Thanks,
Mark.