Message-ID: <CAHp75Vf+eVgTbemaE4az8s50GgrTyZ40Z=4BX46gseRTSyjzqg@mail.gmail.com>
Date: Wed, 3 Apr 2024 11:44:25 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: Peng Fan <peng.fan@....com>, "Peng Fan (OSS)" <peng.fan@....nxp.com>, 
	Sudeep Holla <sudeep.holla@....com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Dan Carpenter <dan.carpenter@...aro.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>, Oleksii Moisieiev <oleksii_moisieiev@...m.com>
Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support

On Wed, Apr 3, 2024 at 11:06 AM Cristian Marussi
<cristian.marussi@....com> wrote:
> On Tue, Apr 02, 2024 at 07:39:44PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
> > <cristian.marussi@....com> 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@....com> 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.
>
> Not at all, the aim was exactly the opposite, avoiding that some protocol
> could have been written without all the needed includes: since a basic set
> of headers is definitely common to any protocol you may think to write,
> grouping all there was meant to avoid this...I thought that by moving the
> problem away in one single internal common header was easier to monitor.

Which may or may not be okay. It plays too smart, so the end developer
won't care about real headers they need as they are the only ones who
know the source code in the best possible way.

> I certainly maybe wrong, but I dont see how you can deduce I dont care...

See above, the protocols.h it's a reincarnation (much less twisted and
ugly, though) of something like kernel.h. Do you know why we have a
split to headers instead of having everything in one? It speeds up a
build, it separates namespace (to the extent of what we call
"namespace" in C language), it makes the modularization (of the source
code) better (easier to avoid considering what's not needed), and so
on. I don't think making a "common" header is a good idea.

> ...and maybe, only maybe, what that 95% of people is trying to do in their
> horrible code is to deliver the best reasonably possible thing within their
> timeline while you are barking at them in chase of never to be released utter
> perfection.

Yeah. That's why it's good to teach people about many aspects of the C
language (which is not popular, in particular due to these nuances,
nowadays and we are starving for good developers).

> > 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?
>
> Well, no, but is it not a repetitive and error-prone process ?

Yes. That's why we now have cleanup.h. Please, consider using it instead.

> Is it not the exact reason why devres management exist in first place, to avoid
> repetitive manual alloc/free of resources and related pitfalls ? (even though
> certainly it is normally used in a more conventional and straightforward way)

No. Its scope is to clean the probe-remove parts, one should not
spread it otherwise and one definitely should know its limitations and
corner cases (I even gave some examples back in ca. 2017 in one of my
presentation WRT typical mistakes the developers make

> The idea was to give some sort of aid in the SCMI stack for writing protocols,
> so regarding mem_mgmt, I just built on top of devres facilities, not invented
> anything, to try to avoid repetitions and let the core handle mem allocs/free
> during the probe phases as much as possible: in pinctrl case would be
> particularly trivial to instead manually allocate stuff at init (due to many
> lazy delayed allocations) but other protocols need a lot more to be done at init,
> frequently in a loop to allocate multiple resources descriptors, and manually
> undoing all of that on each error-path and on cleanup is definitely error-prone
> and a pain.

I understand the motivation, but again, devm is a beast in the corner
cases. I believe Laurent Pinchart gave a presentation about how bad
devm can hit you if you don't know what you are doing (OTOH it's hard
to know with devm).

> Last but not least, this whole thing was designed to address the needs of the
> protocols that existed at that time....it is only now with pinctrl lazy-allocations
> at runtime that the ugly cohexistence of devm_ and non-devm allocations became a
> thing....so clearly the thing needs to be improved/rethinked...even dropped if no
> more fitting...
>
> ... or alternatively since devres allocations are anyway optional, you could just
> use regular kmalloc/kfree for this protocol and avoid this dual handling..

Probably, just have a look at cleanup.h.

> ...this was just to put things in context...and I'll happily let Sudeep decide
> what he prefers in the immediate for pinctrl or more in general about all the
> scmi devres, that I've got enough of these pleasant interactions for now..

As I said, it's your call, I'm not preventing you from applying
whatever is going on in the SCMI subsystem. Just tried to point out
the problem(s).

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists