Message-ID: <850d286c-3f30-0209-7bf2-e8fcf400f8ae@epam.com>
Date:   Wed, 26 Apr 2023 13:28:22 +0000
From:   Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     "sudeep.holla@....com" <sudeep.holla@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "michal.simek@....com" <michal.simek@....com>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "souvik.chakravarty@....com" <souvik.chakravarty@....com>
Subject: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver

Hi Cristian,

On 26.04.23 14:19, Cristian Marussi wrote:
> On Tue, Apr 25, 2023 at 07:51:34PM +0000, Oleksii Moisieiev wrote:
>>     Hi Cristian,
>>
>>     On 13.04.23 01:04, Cristian Marussi wrote:
>>
>> On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote:
>>
>> Implementation of the SCMI client driver, which implements
>> PINCTRL_PROTOCOL. This protocol has ID 19 and is described
>> in the latest DEN0056 document.
>>
>> Hi,
>>
> 
> Hi Oleksii,
> 
> please do NOT post mail with html on the mailing list, it is very hard to
> read and comment while useing text-only mail-reader (and not so much
> appreciated :D) ; even readers with an UI can be setup to properly avoid
> html stuff and properly format for Kernel upstream work:
> 
> https://urldefense.com/v3/__https://www.kernel.org/doc/html/v4.10/process/email-clients.html__;!!GF_29dbcQIUBPA!w2wEJQrXQbVSfn4q6_Sot9NQhyl_GTC4UNjmHUcubxpIHDt5dPU6emDEkfOvtSSvjjzZR2J-VQDTOovNXDto8RP_FSJatQ$ [kernel[.]org]
> 
> Indeed your original message now cannot even be found on lore, probably
> discarded for the same reason ? (not sure).
> 
> My comments inline.
> 
I'm sorry for that. Somehow Thunderbird managed to switch to HTML after 
update. I've sent v2.
>>
>> This protocol is part of the feature that was designed to
>> separate the pinctrl subsystem from the SCP firmware.
>> The idea is to separate communication of the pin control
>> subsystem with the hardware to SCP firmware
>> (or a similar system, such as ATF), which provides an interface
>> to give the OS ability to control the hardware through SCMI protocol.
>> This is a generic driver that implements SCMI protocol,
>> independent of the platform type.
>>
>> DEN0056 document:
>> [1]https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/l
>> atest__;!!GF_29dbcQIUBPA!wnEVmBR_V-0llnzQFsQfeZq5Ov7xJ87H364gqo1_UvsilzKNfJoy81u
>> 5GR1f0EBIyXOGyesjURGdxT_U5tzLvqT5lgjNpw$ [developer[.]arm[.]com]
>>
>>
>> No need to specify all of this in the commit message, just a note that
>> you are adding a new SCMIv3.2 Pincontrol protocol, highlighting anything
>> that has been left out in this patch (if any) will be enough.
>> You can look at the very first commit logs of existing protos as an
>> example like: drivers/firmware/arm_scmi/powercap.c
>>
>> Some more comments down below, I'll mostly skip anything related to the
>> SCMI API change I mentioned before...
>>
>> I'll also wont comment on more trivial stuff related to style, BUT there
>> are lots of them: you should run
>>
>> ./scripts/checkpatch.pl --strict <your-git-format-patch-file>
>>
>> for each patch in the series. (and fix accordingly..spacing, brackets...etc)
>>
>>     Done.
>>
>> +static int scmi_pinctrl_list_associations(const struct scmi_handle *handle,
> 
> [snip]
> 
>> +                                         u32 selector,
>> +                                         enum scmi_pinctrl_selector_type type,
>> +                                         uint16_t size, unsigned int *array)
>> +{
>>
>> This is the other functionalities you could implement straight away using
>> ph->hops helpers (iterators) but just leave it this way, and I'll port it later
>> (once we retested all of this as working with the new API but without any
>> ph->hops usage..I think it is safer to change one bit at time... :P)
>>
>>     Rewritten using iterators and checked with the unittests.
> 
> Good, thanks.
> 
> [snip]
> 
>> +static int scmi_pinctrl_get_group_info(const struct scmi_handle *handle,
>> +                                      u32 selector,
>> +                                      struct scmi_group_info *group)
>> +{
>> +       int ret = 0;
>> +       struct scmi_pinctrl_info *pi;
>> +
>> +       if (!handle || !handle->pinctrl_priv || !group)
>> +               return -EINVAL;
>> +
>> +       pi = handle->pinctrl_priv;
>> +
>> +       ret = scmi_pinctrl_attributes(handle, GROUP_TYPE, selector,
>> +                                     &group->name,
>> +                                     &group->nr_pins);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (!group->nr_pins) {
>> +               dev_err(handle->dev, "Group %d has 0 elements", selector);
>> +               return -ENODATA;
>> +       }
>> +
>> +       group->group_pins = devm_kmalloc_array(handle->dev, group->nr_pins,
>> +                                              sizeof(*group->group_pins),
>> +                                              GFP_KERNEL);
>>
>> I think you can just use for the array allocation
>>
>>          devm_kcalloc(dev, n, size, flags)
>>
>> and it will add also __GFP_ZERO internally to clear it.
>> (indeed it calls in turn devm_kmalloc_array)
>>
>> ...BUT I think there is a further tricky issue here related to memory allocation
>> ...
>>
>> You call this and others function of this kind from some scmi_pinctrl_ops,
>> like in scmi_pinctrl_get_group_pins (scmi_pinctrl_ops->get_group_pins),
>> and then this is in turn called by the SCMI Pinctrl driver via
>> pinctrl_ops->get_group_pins AND you set a present flag so that you issue a
>> PINCTRL_LIST_ASSOCIATIONS and allocate here a new group_pins array just
>> the first time: but these are never released anywhere, since, even though
>> lazily dynamically allocated when asked for, these are static data that
>> you pass to the caller/user of this protocol and so you cannot release
>> them anytime soon, indeed.
>>
>> The core SCMI stack usually takes care to track and release all the devm_
>> resources allocated by the protocol ONLY if they were allocated with devres
>> while inside scmi_pinctrl_protocol_init() function.
>> (see drivers/firmware/arm-scmi/driver.c:scmi_alloc_init_protocol_instance()
>>   and scmi_protocol_release)
>>
>> BUT you do not allocate these arrays inside the protocol-init function,
>> you allocate them the first time these ops are called at runtime.
>>
>> If you unbind/unload all the drivers using this protocol and then reload
>> them, all the devm_ allocations in protocol_init will be freed and
>> reallocated BUT these arrays will never be freed (they are boudn to handle->dev)
>> and instead they will be reallocated multiple times (present flag will be cleare
>> d
>> on unload), remained unused and freed finally only when the whole SCMI stack is
>> unbind/unloaded.
>>
>> You use a present flag to avoid reissuing the same query and
>> reallocating all the arrays each time a driver calls these
>> protocol_ops one, but really all these data is available early on at
>> protocol init time and they are not supposed to change at runtime, dont they ?
>>
>> Even in a virtualized environment, you boot an agent and the SCMI
>> platform server provides to the agent the list of associations when
>> queried but then this does not change until the next reboot right ?
>> (indeed you do not query more than once...)
>>
>> The agent can only change the PIN status with CONFIG_SET or
>> FUNCTION_SELECT or REQUEST the exclusive use of a pin/group, but it is
>> not that the platform can change the pin/groups associaion for the same
>> agent at run time, this are static data for the whole life of the agent.
>>
>> Am I right ?
>>
>> IOW I think there is some potential memory leak on unbind/bind and it would
>> be better to query and allocate all of these resources at init time and keep
>> them ready to be retrieved by subsequent operations, since the lifetime
>> of these resources is pretty long and they are basically representing
>> static data that does not change after the init/probe phases.
>>
>> Indeed, all the other protocols usually allocate all the needed
>> resources and query all the available SCMI resources once for all during
>> the protocol_init, storing all the retrieved info in some struct *_info
>> exposed in scmi_protocol.h and then provide some related protocol_ops to
>> get the number of resources and to retrieve specific domain info descriptors.
>> (voltage.c is an example and more on this down below...)
>>
>> This way, any dynamic allocation is done during protocol_init, so
>> it can be automatically freed by the SCMI core once there are no more
>> users of that protocol, and all of this static info data is queried
>> and retrieved once for all at protocol initialization time, avoiding
>> unneeded message exchanges to retrieve always the same data.
>> (which you avoid anyway with the present flag)
>>
>> If you have a good reason to instead perform this sort of lazy
>> allocation/query performed only at the last minute when someone ask for
>> that specific resource, you will  have to provide also a .instance_deinit
>> function to clean anything you allocated out of the .instance_init
>> routine; but this would seem strange to me since any resource that is
>> discovered at init will be eventually immediately queried by a driver
>> which uses this protocol...am I missing something ?
>>
>>
>>     This is a good point. But there is some reason why I've made such lazy
>>     allocations:
>>
>>     I agree that we have all data on the early stage, but we probably do
>>     not want to request all associations.
>>
>>     Let's assume we have partial pinctrl configuration with 2-3 groups, 2
>>     functions and 10-15 pins involved.
>>
>>     We don't want to request all 250 groups and 32 functions info during
>>     init because this will impact boottime and memory consumption.
> 
> Yes I supposed this was the reason, and it could be reasonable to just
> query the associations last minute when you need them for the stuff you
> need which was put in the DT...I want to think a bit more about this,
> being the only protocol that needs this behaviour. Good for now.
> 
>>
>>     Pinctrl subsystem will request all needed data during device-tree node
>>     parsing.
>>
>>     I have an idea to implement .instance_deinit callback from
>>     scmi_protocol, which will cleanup all allocated data.
>>
>>     What do you think about that? If it is ok for you - I'll push v2.
>>
> 
> I'd say do the cleanup with the available .instance_deinit as proposed
> in the meantime, so we can see how all the changes and the update to
> mainline kernel pans out...then we can discuss this further down the
> line, maybe finding a better way to serve you lazy allocation from the
> SCMI core (or not) and also see what Sudeep thinks abou these lazy
> allocations.
> 
> [snip]
> 
>> + * struct scmi_pinctrl_ops - represents the various operations provided
>> + * by SCMI Pinctrl Protocol
>> + *
>> + * @get_groups_count: returns count of the registered groups
>> + * @get_group_name: returns group name by index
>> + * @get_group_pins: returns the set of pins, assigned to the specified group
>> + * @get_functions_count: returns count of the registered fucntions
>> + * @get_function_name: returns function name by indes
>> + * @get_function_groups: returns the set of groups, assigned to the specified
>> + *     function
>> + * @set_mux: set muxing function for groups of pins
>> + * @get_pins: returns the set of pins, registered in driver
>> + * @get_config: returns configuration parameter for pin
>> + * @set_config: sets the configuration parameter for pin
>> + * @get_config_group: returns the configuration parameter for a group of pins
>> + * @set_config_group: sets the configuration parameter for a groups of pins
>> + * @request_pin: aquire pin before selecting mux setting
>> + * @free_pin: frees pin, acquired by request_pin call
>> + */
>> +struct scmi_pinctrl_ops {
>> +       int (*get_groups_count)(const struct scmi_handle *handle);
>> +       int (*get_group_name)(const struct scmi_handle *handles, u32 selector,
>> +                             const char **name);
>> +       int (*get_group_pins)(const struct scmi_handle *handle, u32 selector,
>> +                             const unsigned int **pins, unsigned int *nr_pins);
>> +       int (*get_functions_count)(const struct scmi_handle *handle);
>> +       int (*get_function_name)(const struct scmi_handle *handle, u32 selector,
>> +                                const char **name);
>> +       int (*get_function_groups)(const struct scmi_handle *handle,
>> +                                  u32 selector, unsigned int *nr_groups,
>> +                                  const unsigned int **groups);
>> +       int (*set_mux)(const struct scmi_handle *handle, u32 selector,
>> +                      u32 group);
>> +       int (*get_pin_name)(const struct scmi_handle *handle, u32 selector,
>> +                           const char **name);
>> +       int (*get_pins_count)(const struct scmi_handle *handle);
>> +       int (*get_config)(const struct scmi_handle *handle, u32 pin,
>> +                         u32 *config);
>> +       int (*set_config)(const struct scmi_handle *handle, u32 pin,
>> +                         u32 config);
>> +       int (*get_config_group)(const struct scmi_handle *handle, u32 pin,
>> +                         u32 *config);
>> +       int (*set_config_group)(const struct scmi_handle *handle, u32 pin,
>> +                         u32 config);
>> +       int (*request_pin)(const struct scmi_handle *handle, u32 pin);
>> +       int (*free_pin)(const struct scmi_handle *handle, u32 pin);
>> +};
>> +
>>
>> As mentioned above, here you could drop a lot of this get_X_count/name/pins
>> and instead expose a few of the internal proocol struct scmi__X_info and then
>> provide just a mean to query how many resource are there and then get the info
>> descriptor you want for the specific domain_id, i.e.:
>>
>>      int (*num_domains_get)(ph, type)
>>      void *(*info_get)(ph, type, domain_id);
>>
>> Thanks,
>> Cristian
>>
>>     Updated. Exposed selector_type to scmi_protocol, which helped me to
>>     reduce number of call.
>>
>>     Looking forward for your thoughts about .instance_deinit callback
>>     implementation I've mentioned above and I will be ready to push v2.
>>
> 
> As said, let'see V2 with cleanups in .instance_deinit and lazy
> allocations as they are now and move on from there.
> 
> Thanks,
> Cristian

Powered by blists - more mailing lists