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

[Andy Shevchenko]

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.

> 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