Re: usb: typec: ucsi: Clear EVENT_PENDING bit if ucsi_send_command fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11-09-23 06:19 pm, Heikki Krogerus wrote:
On Mon, Sep 11, 2023 at 02:34:15PM +0530, Prashanth K wrote:
Currently if ucsi_send_command() fails, then we bail out without
clearing EVENT_PENDING flag. So when the next connector change
event comes, ucsi_connector_change() won't queue the con->work,
because of which none of the new events will be processed.

Fix this by clearing EVENT_PENDING flag if ucsi_send_command()
fails.

Cc: <stable@xxxxxxxxxxxxxxx> # 5.16
Fixes: 512df95b9432 ("usb: typec: ucsi: Better fix for missing unplug events issue")
Signed-off-by: Prashanth K <quic_prashk@xxxxxxxxxxx>
---
  drivers/usb/typec/ucsi/ucsi.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index c6dfe3d..509c67c 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -884,6 +884,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
  	if (ret < 0) {
  		dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
  			__func__, ret);
+		clear_bit(EVENT_PENDING, &con->ucsi->flags);
  		goto out_unlock;
  	}

I think it would be better to just move that label (out_unlock) above
the point where clear_bit() is already called instead of separately
calling it like that. That way the Connector Change Event will
also get acknowledged.
Do we really need to ACK in this case since we didn't process the current connector change event

If this really can happen, then I think it would be good to also
schedule a task for ucsi_check_connection():

         if (ret < 0) {
                 dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
                         __func__, ret);
+               ucsi_partner_task(con, ucsi_check_connection, 1, HZ);
                 goto out_unlock;
         }

thanks,

Retrying is a good idea, but ucsi_check_connection() doesn't have the full functionality compared to handle_connector_change. I guess ucsi_check_connection() will send a set_role, but won't handle the connector_change scenarios happening due to PR/DR swap, which will lead to deadlocks (due to wait_for_completion). This is just an example. So its better to bail out and process the next events, because the failure here is from the glink layer.

Thanks
Prashanth K



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux