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

[Cristian Marussi <cristian.marussi@xxxxxxx>]

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.

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

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.

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

Thanks,
Cristian