Re: [PATCH v2 0/7] syscore: Pass context data to callbacks

[Thierry Reding]

On Thu, Jul 17, 2025 at 02:11:41PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 12:32:34PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <tred...@nvidia.com>
> > 
> > Hi,
> > 
> > Something that's been bugging me over the years is how some drivers have
> > had to adopt file-scoped variables to pass data into something like the
> > syscore operations. This is often harmless, but usually leads to drivers
> > not being able to deal with multiple instances, or additional frameworks
> > or data structures needing to be created to handle multiple instances.
> > 
> > This series proposes to "objectify" struct syscore_ops by passing a
> > pointer to struct syscore_ops to the syscore callbacks. Implementations
> > of these callbacks can then make use of container_of() to get access to
> > contextual data that struct syscore_ops was embedded in. This elegantly
> > avoids the need for file-scoped, singleton variables, by tying syscore
> > to individual instances.
> > 
> > Patch 1 contains the bulk of these changes. It's fairly intrusive
> > because it does the conversion of the function signature all in one
> > patch. An alternative would've been to introduce new callbacks such that
> > these changes could be staged in. However, the amount of changes here
> > are not quite numerous enough to justify that, in my opinion, and
> > syscore isn't very frequently used, so the risk of another user getting
> > added while this is merged is rather small. All in all I think merging
> > this in one go is the simplest way.
> 
> All at once is good, I like the idea, but:
> 
> > Patches 2-7 are conversions of some existing drivers to take advantage
> > of this new parameter and tie the code to per-instance data.
> 
> That's great, but none of these conversions actually get rid of the
> global structure, so what actually was helped here other than the churn
> of this "potentially" allowing the global data variables from being
> removed in the future?
> 
> So how does this actually help?

Thanks for pointing this out and letting me look at it again. Most of
these actually do get rid of the global data variables. The MIPS patch
doesn't because I forgot, but the __alchemy_pci_ctx is no longer used
after the patch (except where it's initialized to the ctx variable, but
that's no longer needed now). I've updated that patch.

The Ingenic TCU patch gets rid of it, and so do the clk/mvebu and
irq-imx-gpcv2 patches. The two exceptions where it wasn't possible to
get rid of the global data variables are mvebu-mbus and Tegra PMC, in
both cases because there is other functionality that relies on the
global variable. The bits that make it very difficult to remove these
entirely is that they export functions that are called without context
from other parts of code.

I have a fairly large series on top of this that converts the Tegra PMC
driver to move away from this as much as possible. It's not possible to
do on 32-bit ARM because there is some low-level CPU code that needs to
call into this function. However, the goal is to at least make the PMC
driver data completely instance-specific on 64-bit ARM so that we can
support multiple instances eventually.

Maybe something similar could be done for mvebu-bus, but I'm not sure
it's worth it. Typically for these cases you need some form of context
in order to replace the global data. On Tegra we do have that in many
cases (via DT phandle references), but I'm not familiar enough with
mvebu to know if something similar exists.

My goal with this series is to get this a bit more established so that
people don't use the lack of context in syscore as an excuse for not
properly encapsulating things. These usually tend to go hand in hand,
where people end up using a global data variable for syscore and since
they can't get around that one, they keep using it for a bunch of other
shortcuts.

> Also, small nit, make the function pointers const please :)

I originally tried that. Unfortunately, the struct syscore_ops contains
a struct list_head to add it to the global list of structures. I suppose
I could move the function pointers into a different structure and make
pointers to that const, something like this:

        struct syscore;

        struct syscore_ops {
                int (*suspend)(struct syscore *syscore);
                void (*resume)(struct syscore *syscore);
                void (*shutdown)(struct syscore *syscore);
        };

        struct syscore {
                const struct syscore_ops *ops;
                struct list_head node;
        };

Is that what you had in mind?

Thanks,
Thierry

signature.asc
Description: PGP signature