linux-kernel - Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common code for command handling

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 25 Jun 2024 17:24:54 +0200
From: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Heikki Krogerus
	<heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>
CC: Neil Armstrong <neil.armstrong@...aro.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Nikita Travkin <nikita@...n.ru>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [Linux-stm32] [PATCH v2 6/7] usb: typec: ucsi: extract common
 code for command handling

On 6/21/24 00:55, Dmitry Baryshkov wrote:
> Extract common functions to handle command sending and to handle events
> from UCSI. This ensures that all UCSI glue drivers handle the ACKs in
> the same way.
> 
> The CCG driver used DEV_CMD_PENDING both for internal
> firmware-related commands and for UCSI control handling. Leave the
> former use case intact.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c           | 43 +++++++++++++++++++++++++++
>  drivers/usb/typec/ucsi/ucsi.h           |  7 +++++
>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 46 ++---------------------------
>  drivers/usb/typec/ucsi/ucsi_ccg.c       | 21 ++-----------
>  drivers/usb/typec/ucsi/ucsi_glink.c     | 47 ++---------------------------
>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   | 44 ++--------------------------
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c | 52 ++-------------------------------
>  7 files changed, 62 insertions(+), 198 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 4ba22323dbf9..691ee0c4ef87 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -36,6 +36,48 @@
>   */
>  #define UCSI_SWAP_TIMEOUT_MS	5000
>  
> +void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
> +{
> +	if (UCSI_CCI_CONNECTOR(cci))
> +		ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
> +
> +	if (cci & UCSI_CCI_ACK_COMPLETE &&
> +	    test_bit(ACK_PENDING, &ucsi->flags))
> +		complete(&ucsi->complete);
> +
> +	if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> +	    test_bit(COMMAND_PENDING, &ucsi->flags))
> +		complete(&ucsi->complete);

Hi Dmitry,

I've recently faced some race with ucsi_stm32g0 driver, and have sent a
fix for it [1], as you've noticed in the cover letter.

To fix that, I've used test_and_clear_bit() in above two cases, instead
of test_bit().

https://lore.kernel.org/linux-usb/20240612124656.2305603-1-fabrice.gasnier@foss.st.com/

> +}
> +EXPORT_SYMBOL_GPL(ucsi_notify_common);
> +
> +int ucsi_sync_control_common(struct ucsi *ucsi, u64 command)
> +{
> +	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
> +	int ret;
> +
> +	if (ack)
> +		set_bit(ACK_PENDING, &ucsi->flags);
> +	else
> +		set_bit(COMMAND_PENDING, &ucsi->flags);
> +
> +	ret = ucsi->ops->async_control(ucsi, command);
> +	if (ret)
> +		goto out_clear_bit;
> +
> +	if (!wait_for_completion_timeout(&ucsi->complete, 5 * HZ))
> +		ret = -ETIMEDOUT;

With test_and_clear_bit(), could return 0, in case of success here.

I'd suggest to use similar approach here, unless you see some drawback?

Best Regards,
Fabrice

> +
> +out_clear_bit:
> +	if (ack)
> +		clear_bit(ACK_PENDING, &ucsi->flags);
> +	else
> +		clear_bit(COMMAND_PENDING, &ucsi->flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_sync_control_common);
> +
>  static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
>  {
>  	u64 ctrl;
> @@ -1883,6 +1925,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>  	INIT_WORK(&ucsi->resume_work, ucsi_resume_work);
>  	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
>  	mutex_init(&ucsi->ppm_lock);
> +	init_completion(&ucsi->complete);
>  	ucsi->dev = dev;
>  	ucsi->ops = ops;

[snip]

>  	ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 14737ca3724c..d948c3f579e1 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -61,11 +61,7 @@ struct ucsi_stm32g0 {

[snip]

> -
> -	ret = ucsi_stm32g0_async_control(ucsi, command);
> -	if (ret)
> -		goto out_clear_bit;
> -
> -	if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000)))
> -		ret = -ETIMEDOUT;
> -	else
> -		return 0;
> -
> -out_clear_bit:
> -	if (ack)
> -		clear_bit(ACK_PENDING, &g0->flags);
> -	else
> -		clear_bit(COMMAND_PENDING, &g0->flags);
> -
> -	return ret;
> -}
> -
>  static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>  {
>  	struct ucsi_stm32g0 *g0 = data;
> @@ -449,13 +416,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
>  	if (ret)
>  		return IRQ_NONE;
>  
> -	if (UCSI_CCI_CONNECTOR(cci))
> -		ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
> -
> -	if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags))
> -		complete(&g0->complete);
> -	if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags))
> -		complete(&g0->complete);
> +	ucsi_notify_common(g0->ucsi, cci);

I can see the fix "test_and_clear_bit()" sent earlier is removed from here.

I'd suggest to use similar approach as here, unless you see some drawback?

Please advise,
Best Regards,
Fabrice