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@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. > 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. > > 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. This is/was handy since, till now, all the SCMI querying and resources allocation happened anyway all at once at init time... ...the mess, as you kindly called it, derives from the fact that this specific protocol is the first and only one that does NOT allocate all that it needs during the initialization (to minimize needless allocs for a lot of possibly unused resources) and this lazy-initialization phase, done after init at runtime, must be handled manually since it cannot be managed by the devres group that is open/clsoed around init by the SCMI core. I dont like particularly this split allocation but it has a reason and any other solution seems more messy to me at the moment. And I dont feel like changing all the SCMI protocol initialziation core code (that address a lot more under the hood) is a desirable solution to address a non-existent problem really. > 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. > Initialization order is enforced by SCMI core like this: @driver_probe->get_protocol_ops() @core/get_protocol_ops -> devres_group_open() -> protocol_init->devm_*() -> devres_group_close() -> driver_probing @runtime optional explicit_lazy_kmallocs inside the protocol @driver_remove->put_protocol_ops() @core/put_protocol_ops() -> protocol_denit->optional_explicit_kfree_of_the_above -> devres_group_release() -> driver_removing ... dont think there's an ordering problem. ...note that the ph->dev provided in the protocol_init and used by devm_ is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops, it is an internal SCMI device associated with the core SCMI stack probing and allocations, within which a devres group for the specific protocol is created when that specific protocol is initialized...protocols are not fully fledged drivers are just bits of the SCMI stack that are initialized when needed (and possibly also loaded when needed for vendor protocols) and de-initialzed when no more SCMI driver users exist for that protocol. Thanks, Cristian
