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

[Peng Fan <peng.fan@xxxxxxx>]

Hi Andy,

> Subject: RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
> 
> > 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%2Flo
> > > re
> > > .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.

Just give a look. But since most scmi firmware drivers are using
devm_x APIs in protocol init. I would follow the style to use
devm_x as of now.

And for pinctrl protocol deinit phase, I will add a comment on why
use kfree and what it is to free.

For the __free macro, people drop all the scmi firmware drivers
using devm_x APIs in init phase in a future patch.

Is this ok?

Thanks,
Peng.

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