Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
<cristian.marussi@xxxxxxx> wrote:
> On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > <cristian.marussi@xxxxxxx> wrote:
> > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:

...

> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/scmi_protocol.h>
> > > > > > +#include <linux/slab.h>
> > > > >
> > > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > > this page I see bits.h, types.h, and  asm/byteorder.h).
> > > >
> > > > Is there any documentation about this requirement?
> > > > Some headers are already included by others.
> >
> > The documentation here is called "a common sense".
> > The C language is built like this and we expect that nobody will
> > invest into the dependency hell that we have already, that's why IWYU
> > principle, please follow it.
>
> Yes, but given that we have a growing number of SCMI protocols there is a
> common local protocols.h header to group all includes needed by any
> protocols: the idea behind this (and the devm_ saga down below) was to ease
> development of protocols, since there are lots of them and growing, given
> the SCMI spec is extensible.

Yes, and what you are effectively suggesting is: "Technical debt? Oh,
fine, we do not care!" This is not good. I'm in a long term of
cleaning up the dependency hell in the kernel (my main focus is
kernel.h for now) and I am talking from my experience. I do not like
what people are doing in 95% of the code, that's why I really want to
stop the bad practices as soon as possible.

Last to add, but not least is that your code may be used as an example
for others, hence we really have to do our best in order to avoid bad
design, practices, and cargo cults. If this requires more refactoring
of the existing code, then do it sooner than later.

...

> > > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > > this same patch while it was posted by Oleksii.
> > >
> > > And I told that time that most of the remarks around devm_ usage were
> > > wrong due to how the SCMI core handles protocol initialization (using a
> > > devres group transparently).
> > >
> > > This is what I answered that time.
> > >
> > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> > >
> > > I wont repeat myself, but, in a nutshell the memory allocation like it
> > > is now is fine: a bit happens via devm_ at protocol initialization, the
> > > other is doe via explicit kmalloc at runtime and freed via kfree at
> > > remove time (if needed...i.e. checking the present flag of some structs)
> >
> > This sounds like a mess. devm_ is expected to be used only for the
> > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > to have automatic free at the paths where memory is not needed.
>
> Indeed, this protocol_init code is called by the SCMI core once for all when
> an SCMI driver tries at first to use this specific protocol by 'getting' its
> protocol_ops, so it is indeed called inside the probe chain of the driver:
> at this point you *can* decide to use devres to allocate memory and be assured
> that if the init fails, or when the driver cease to use this protocol (calling
> its remove()) and no other driver is using it, all the stuff that have been
> allocated related to this protocol will be released by the core for you.
> (using an internal devres group)
>
> Without this you should handle manually all the deallocation manually on
> the init error-paths AND also provide all the cleanup explicitly when
> the protocol is no more used by any driver (multiple users of the same
> protocol instance are possible)...for all protocols.

Yes. Is it a problem?

> This is/was handy since, till now, all the SCMI querying and resources
> allocation happened anyway all at once at init time...
>
> ...the mess, as you kindly called it, derives from the fact that this specific
> protocol is the first and only one that does NOT allocate all that it needs
> during the initialization (to minimize needless allocs for a lot of possibly
> unused resources) and this lazy-initialization phase, done after init at runtime,
> must be handled manually since it cannot be managed by the devres group that is
> open/clsoed around init by the SCMI core.
>
> I dont like particularly this split allocation but it has a reason and any
> other solution seems more messy to me at the moment.
>
> And I dont feel like changing all the SCMI protocol initialziation core code
> (that address a lot more under the hood) is a desirable solution to address a
> non-existent problem really.
>
> > And the function naming doesn't suggest that you have a probe-remove
> > pair. Moreover, if the init-deinit part is called in the probe-remove,
> > the devm_ must not be mixed with non-devm ones, as it breaks the order
> > and leads to subtle mistakes.
>
> Initialization order is enforced by SCMI core like this:
>
>  @driver_probe->get_protocol_ops()
>   @core/get_protocol_ops
>      -> devres_group_open()
>      -> protocol_init->devm_*()
>      -> devres_group_close()
>      -> driver_probing
>
>    @runtime optional explicit_lazy_kmallocs inside the protocol
>
>  @driver_remove->put_protocol_ops()
>    @core/put_protocol_ops()
>      -> protocol_denit->optional_explicit_kfree_of_the_above
>      -> devres_group_release()
>    -> driver_removing
>
> ... dont think there's an ordering problem.

The mess with devm_ vs. non-devm is quite easy to achieve. You are
probably out of the control of what the protocol driver wants to do in
the init. Is the usage of devm (which APIs and in which order can be
used)  WRT SCMI documented somewhere?

Misuse of devm is a common issue, I'm not surprised it will hit your
subsystem one day with such an approach.

> ...note that the ph->dev provided in the protocol_init and used by devm_
> is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
> it is an internal SCMI device associated with the core SCMI stack probing and
> allocations, within which a devres group for the specific protocol is created
> when that specific protocol is initialized...protocols are not fully
> fledged drivers are just bits of the SCMI stack that are initialized when needed
> (and possibly also loaded when needed for vendor protocols) and
> de-initialzed when no more SCMI driver users exist for that protocol.

P.S. I guess from now on it's your call, but this code and in case
other drivers use similar, is badly written. I hope some documentation
exists to at least justify all this mess and explaining why there no
and (what's really important) will never be a problem.

-- 
With Best Regards,
Andy Shevchenko