linux-kernel - Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Apr 2023 17:23:05 +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 20.04.23 20:05, Cristian Marussi wrote:
> On Wed, Apr 12, 2023 at 11:04:05PM +0100, 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,
>
> one more thing that I missed in my previous review down below.
>
>>> 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:
>>> https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!0kMLQ5c3tKsMfWCqTKHp6eolY3sTZlyKmAD7B7pbiSESABUUoBzmhgrYdDgWGC_g0vgLE4zwrS4ppeTOD8KizP9fIeJkpg$ [developer[.]arm[.]com]
>>>
> [snip]
>
>>> +static int scmi_pinctrl_request_config(const struct scmi_handle *handle,
>>> +				       u32 selector,
>>> +				       enum scmi_pinctrl_selector_type type,
>>> +				       u32 *config)
>>> +{
>>> +	struct scmi_xfer *t;
>>> +	struct scmi_conf_tx *tx;
>>> +	__le32 *packed_config;
>>> +	u32 attributes = 0;
>>> +	int ret;
>>> +
>>> +	if (!handle || !config || type == FUNCTION_TYPE)
>>> +		return -EINVAL;
>>> +
>>> +	ret = scmi_pinctrl_validate_id(handle, selector, type);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_GET,
>>> +				 SCMI_PROTOCOL_PINCTRL,
>>> +				 sizeof(*tx), sizeof(*packed_config), &t);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	tx = t->tx.buf;
>>> +	packed_config = t->rx.buf;
>>> +	tx->identifier = cpu_to_le32(selector);
>>> +	attributes = SET_TYPE_BITS(attributes, type);
>>> +	attributes = SET_CONFIG(attributes, *config);
>>> +
> Looking at scmi_conf_tx and these pinctrl get/set functions, you do not
> seem to consider the ConfigType field in the SCMI related messages, so
> basically using always the Default 0 Type, and as a consequence you dont
> either expose any way to choose a Different type in the related SCMI
> protocol ops; I imagine this is because the pinctrl driver currently using
> this protocol, at the end, does not need any of the other available types
> (as in Table 23 of the spec).
>
I'm not sure I've understood your point. Pinctrl subsystem pass config 
in so-called Packed format. So this means that config is both input and 
output parameter. Packed format means that u32 config has both config id 
and config value packed inside.

So I receive packed config with both id and value on config_set call and 
then just send it over SCMI, expecting error from server if config is 
invalid. On config_get call I receive config parameter with only id 
packed inside, them pass it to the server and receive packed_config with 
both id and value, which is ready to be returned to the subsystem.

> This is fine for pinctrl driver as it is now, BUT the SCMI pinctrl
> protocol implementation in the core SCMI stack and its related
> protocol_operations as exposed in scmi_protocol.h should be generic
> enough to serve any possible user of the SCMI pinctrl protocol (and there
> is already a request to extend/amend the spec somehow to send multiple pin
> setup of different types in one go as you may have seen), so I'd say it's
> better if you add also a ConfigType param to the get/set_config scmi_pinctrl_ops
> and expose the whole ConfigType enums (Table23) in scmi_protocol.h (like we do for
> sensor classes on scmi_protocol.h) to address this; the pinctrl driver
> can then anyway call such new protocol_ops with a Default type, but at
> least the SCMI protocol_ops interface will remain generic and could be
> reused iin other scenarios.
>
> This is equally true for all the other protocol messages (should I have
> miss something else for now...I'll review again you next V2 anyway).
>
> Thanks,
> Cristian
>