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

[Peng Fan <peng.fan@xxxxxxx>]

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937-
> lin%2F%2
> >
> 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d
> c5315bed
> >
> 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583
> 40430%7CUn
> >
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1haW
> >
> wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6
> K3HF%2FofD
> > Yu7j0Lrm8dN5k%3D&reserved=0
> >
> > 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 am new to __free() honestly. I'll let Cristian and Sudeep to comment on
what should I do for v8.

Thanks,
Peng.

> 
> > I'll made further remarks on v7 that you just posted.
> 
> --
> With Best Regards,
> Andy Shevchenko