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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Jul 18, 2025 at 03:49:37PM +0200, Thierry Reding wrote:
> 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 <treding@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.

Ah, I must have looked at the wrong examples in the patch series, sorry.

> 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.

I agree, I overall like this change, just expected to see more global
structures being able to be removed.

> > 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?

I missed the list_head, so yes, this would be better, but don't pass
back the syscore structure, how about just a void * instead, making the
whole container_of() stuff go away?

thanks,

greg k-h