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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: Peng Fan <peng.fan@nxp.com>,
	"Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
	 Sudeep Holla <sudeep.holla@arm.com>,
	Rob Herring <robh@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 Linus Walleij <linus.walleij@linaro.org>,
	Dan Carpenter <dan.carpenter@linaro.org>,
	 "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	 "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	 "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
Date: Tue, 2 Apr 2024 16:06:06 +0300	[thread overview]
Message-ID: <CAHp75VdAaTeQ_Ag3gd0s9UfT=kAT2hwibeJ9-YFXJx4z=R3e+g@mail.gmail.com> (raw)
In-Reply-To: <Zgu4Tok43W5t8KM0@pluto>

On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
<cristian.marussi@arm.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.

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

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.

> I'll made further remarks on v7 that you just posted.

-- 
With Best Regards,
Andy Shevchenko