On 01/13/14 03:52, Will Deacon wrote:

On Fri, Jan 10, 2014 at 07:36:57PM +0000, Stephen Boyd wrote:

quoted

Passing the hw_events as the pcpu token here is kind of hacky.
The reason is because the token is dereferenced into cpu_pmu in
armv7pmu_handle_irq() like so:

	struct arm_pmu *cpu_pmu = (struct arm_pmu *)dev;

It would be great if we could pass cpu_pmu directly to the
request call like so:

	request_percpu_irq(irq, cpu_pmu->handle_irq, "arm-pmu", &cpu_pmu);

but no. request_percpu_irq() wants a percpu pointer so this won't
work. If cpu_pmu was declared as DEFINE_PER_CPU, this would work
out just fine.

That feels really broken though, since we rely on the cpu_pmu being a
container for the struct pmu that was registered with perf core.

quoted

Should the cpu_pmu become a per-cpu variable? That sounds rather
invasive.

I also don't think that's the right solution, based on the above. It's
actually pretty hard to work out what's the right thing to do here...

Yes it doesn't seem like the right solution.

We *could* have a per-cpu pointer to the cpu_pmu_pointer, but then we'd
need to update the IRQ handlers, including things like the CCI PMU which
really doesn't care about per-cpu stuff. So after all this, the shim we have
around the IRQ handler for the U8500 SPI workarounds might be the right
thing after all -- it allows us to consolidate the conversion of a pcpu
pointer into the relevant instance (actually any instance, since they'd all
point at the same thing) for the current CPU.

What do you think to having that shim throw away the second level pcpu
pointer in the case of a PPI? (probably means we need to revisit that
renaming again).

Ok I think I understand what you're getting at. We pass a per-cpu
pointer to the cpu_pmu pointer as the dev_id argument to the PPI irq
handler, and then we check to see if the irq is per-cpu inside the
armpmu_dispatch_irq() function and throw away the second level of
pointer, i.e.

static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
{
        struct arm_pmu *armpmu;
        struct platform_device *plat_device;
        struct arm_pmu_platdata *plat;

        if (irq_is_percpu(irq))
                dev = *(struct arm_pmu_cpu **)dev;
        armpmu = dev;
        plat_device = armpmu->plat_device;
        plat = dev_get_platdata(&plat_device->dev);

        if (plat && plat->handle_irq)
                return plat->handle_irq(irq, dev, armpmu->handle_irq);
        else
                return armpmu->handle_irq(irq, dev);
}


We still need to make a per-cpu variable to hold the pointer, and assign
it during cpu_pmu_init like this patch does. Hopefully that is ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation