Re: [PATCH v2] perf: arm_cspmu: Separate Arm and vendor module

[Robin Murphy]

On 2023-04-28 23:23, Besar Wicaksono wrote:
> Hi Robin and Suzuki,
>
> > -----Original Message-----
> > From: Robin Murphy <robin.murphy@xxxxxxx>
> > Sent: Thursday, April 27, 2023 7:21 AM
> > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; suzuki.poulose@xxxxxxx;
> > catalin.marinas@xxxxxxx; will@xxxxxxxxxx; mark.rutland@xxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > tegra@xxxxxxxxxxxxxxx; Thierry Reding <treding@xxxxxxxxxx>; Jonathan
> > Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>; Richard
> > Wiley <rwiley@xxxxxxxxxx>; Eric Funsten <efunsten@xxxxxxxxxx>
> > Subject: Re: [PATCH v2] perf: arm_cspmu: Separate Arm and vendor module
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On 2023-04-18 07:20, Besar Wicaksono wrote:
> > > Arm Coresight PMU driver consists of main standard code and vendor
> > > backend code. Both are currently built as a single module.
> > > This patch adds vendor registration API to separate the two to
> > > keep things modular. Vendor module shall register to the main
> > > module on loading and trigger device reprobe.
> >
> > I think it might be considerably cleaner and safer if the main driver
> > retained at least some knowledge of the PMIIDR matches and used those to
> > explicity request the relevant module. Otherwise, not only is there an
> > awful lot of fiddly complexity here, but there's also quite a burden on
> > the user to know which modules they have to load to get full
> > functionality on any given system.
>
> Do you mean like keep the existing match table as a whitelist, and associate
> each entry with the backend module name to load it from the main driver ?

It would essentially be a table that matches a PMIIDR filter to a module 
name. Having looked for existing examples of this kind of usage model, 
in terms of the overall shape it might look closest to a very 
stripped-down version of the crypto manager.

> > FYI I've just started working on adding devicetree support, and I do
> > need the generic architectural functionality to keep working in the
> > absence of any imp-def backend.
>
> W.r.t the reprobe discussion with Suzuki, this would mean the expected
> behavior is to attach the device to standard imp as fallback/default.
> Suzuki, my preference is not supporting delayed reprobe on event->destroy
> due to the potential access to stale data. We should just fail the backend
> registration if one of the device is in use.

We shouldn't need to worry about that at all. If requesting an expected 
implementation module fails and those PMUs end up falling back to 
baseline functionality, there's no need to actively deny the backend if 
the module is manually loaded later, it merely won't be used. As far as 
reprobing goes, it seems reasonable for the user to remove/reload the 
main module after fixing the backend module availability if it matters 
to them. Or maybe we could just EPROBE_DEFER instead of falling back to 
baseline if we know the module is enabled and *should* be available; I 
don't have a particular preference either way. The main thing is just to 
have the backend modules work in a simple, intuitive, and mostly 
automatic manner, without all the complexity of effectively 
reimplementing a whole custom driver model in between perf and the real 
driver model.

I think the only really notable thing about this approach is that it 
would probably need to make sure the PMU probe runs asynchronously from 
the main module_init.

Thanks,
Robin.