Re: Fixing property memory leaks on device tree overlay removal

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Rob,

On Wed, 3 Jul 2024 15:08:11 -0600
Rob Herring <robh@kernel.org> wrote:

> > > I need to study this more, but a notifier is never a great design so
> > > maybe we can come up with something better.  
> >
> > Do you have specific ideas in mind? I'm very interested in knowing
> > alternative options.  
> 
> Like I said, need to study it more... Perhaps just remove notifying on
> properties. I think properties changing at any point in time is
> difficult to support and we just shouldn't support that other than a
> few exceptions like "status". Or we could have a "node changed"
> notifier and then it's the client's problem to figure out what
> property changed.

At first glance it makes sense. And, reassuring, I'm not using property
notifiers in my code.

> > > > Once all call sites are updated to the new API, the old API can be
> > > > removed entirely along with the deadprops list and the
> > > > CONFIG_EXPORT_UNSAFE_OF_ACCESSORS Kconfig symbol.  
> > >
> > > I don't like the kconfig symbol even if it is temporary. How does it
> > > get configured for a multi-platform kernel?  
> >
> > First of all: this kconfig symbol is useful only if the goal is to
> > remove all property-leaking APIs.
> >
> > The idea is to use it as a guard: if a defconfig builds with it set to
> > 'n', then all the code enabled in that defconfig is not using any
> > "unsafe" accessor. Meaning: we haven't removed all accessors from the
> > whole kernel, but from the subset of the kernel that this defconfig is
> > building.
> >
> > For multi-platform kernels it is not much relevant in the short term.
> > If/when at some point we will be able to set it to 'n' in one of them
> > (e.g. arch/arm64/configs/defconfig) that would mean a large percentage
> > of call sites have been removed, and (even more important) _no_ call
> > sites will be added anymore or the defconfiig will fail immediately.
> >
> > And I think it should be a requirement for any driver loading/unloading
> > overlays, so that one cannot even load an overlay without fixing all
> > the call sites. I think this is the most relevant usefulness in the
> > short term: either you call unsafe accessors or you load overlays, not
> > both.  
> 
> Sure, but that can be runtime. If you load overlays, then you'll get
> warnings for calling unsafe accessors on the nodes associated with the
> overlay.

I got your position, thanks for taking time to provide more details.

> > > > Note there is no "convert all call sites" in the plan. The amount of
> > > > drivers alone is huge, and converting all of them would not be doable
> > > > entirely by us both in terms of amount of work and for lack of hardware
> > > > to test the changes.  
> > >
> > > That would only be necessary if we allow any change everywhere in a DT
> > > on every system. IOW, if we accepted the userspace configfs overlay
> > > applying patchset. If things are constrained to specific systems and
> > > specific parts of a DT (e.g. under a connector node), then it is a lot
> > > fewer pieces to fix. If powerpc is excluded for example, that alone
> > > eliminates a bunch of code and drivers to worry about.  
> >
> > Bottom line, I think the takeaway is that in your opinion removing the
> > unsafe property accessors is not a requirement for accepting a driver
> > that loads/unloads an overlay. Only having zero warnings for the
> > specific overlay is a requirement. Correct?  
> 
> Right, assuming unsafe property accessors warn or we can deem it safe
> in some other way. For example, we've had fixup overlays which fixed
> old bindings to new ones in tree. Those were safe as they are never
> removed and are applied before the affected driver probed.

I see, good to know.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com