Re: [RFC PATCH 0/7] usb: typec: ucsi: Polling the alt modes and PDOs

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2514 bytes --]

On Wed, Jun 09, 2021 at 03:18:04PM +0300, Heikki Krogerus wrote:
> On Wed, Jun 09, 2021 at 02:26:00PM +0300, Heikki Krogerus wrote:
> > On Tue, Jun 08, 2021 at 09:32:01PM +0200, Benjamin Berg wrote:
> > > On Tue, 2021-06-08 at 09:54 +0300, Heikki Krogerus wrote:
> > > > On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> > > > > Please check does the partner device get removed. What do you have
> > > > > under /sys/class/typec after that happens?
> > > > 
> > > > Oh yes. Could you also share the trace output when that happens?
> > > > 
> > > >         cd /sys/kernel/debug/tracing
> > > >         echo 1 > events/ucsi/enable
> > > >         # now reproduce the issue
> > > >         cat trace > ucsi.trace
> > > 
> > > So, the partner device is still there when this happens (see below). I
> > > also only see a single event in the trace for the fast plug/unplug
> > > case:
> > >    kworker/u16:8-1771    [003] .... 18848.872145: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
> > 
> > OK. Sorry I had to double check because you were only talking about
> > the psy online state.
> > 
> > Can you now try this HACK on top of these patches:
> > 
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index bd39fe2cb1d0b..99f072700ce7f 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -843,7 +843,8 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> >  
> >         if (!status.change) {
> >                 dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
> > -               goto out_ack;
> > +               /* XXX Force connection check. */
> > +               status.change = UCSI_CONSTAT_CONNECT_CHANGE;
> >         }
> >  
> >         event = kzalloc(sizeof(*event), GFP_KERNEL);
> 
> No, that's not enough. Sorry.
> 
> I'm trying to get a confirmation on my suspecion that we do always
> actually get an event from the EC firmware, but we just end up
> filtering it out in this case because we are too slow in the driver. I
> have an idea what could be done about that, but I need to test if that
> really is the case.
> 
> I'll prepare a new version out of this entire series.

Actually, it's easier if you could just test this attached patch on
top of this series. It makes sure the every single event is
considered. I'm sorry about the hassle.

thanks,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-ucsi-Test-patch-for-confirming-events.patch --]
[-- Type: text/plain, Size: 4449 bytes --]

From 9a5fc7d8e22ca5d0077a97345b8f079957a465b5 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 9 Jun 2021 15:48:23 +0300
Subject: [PATCH] usb: typec: ucsi: Test patch for confirming events

NOT-Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 80 ++++++++++++-----------------------
 1 file changed, 27 insertions(+), 53 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bd39fe2cb1d0b..53718e655edbf 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -748,16 +748,34 @@ static void ucsi_connector_work(struct work_struct *work)
 {
 	struct ucsi_con_event *event = container_of(work, struct ucsi_con_event, work);
 	struct ucsi_connector *con = event->con;
+	struct ucsi_connector_status status;
 	struct ucsi *ucsi = con->ucsi;
 	enum typec_role role;
 	enum usb_role u_role = USB_ROLE_NONE;
+	u64 command;
 	int ret;
 
+	kfree(event);
+
+	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_send_command(ucsi, command, &status, sizeof(status));
+	if (ret < 0)
+		dev_err(ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret);
+
+	if (!status.change) {
+		dev_dbg(ucsi->dev, "con%d: spurious event\n", con->num);
+		/* XXX: Force connection check. */
+		status.change = UCSI_CONSTAT_CONNECT_CHANGE;
+	}
+
+	ret = ucsi_acknowledge_connector_change(ucsi);
+	if (ret)
+		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
+
 	mutex_lock(&con->lock);
 
-	trace_ucsi_connector_change(con->num, &event->status);
-	con->status = event->status;
-	kfree(event);
+	trace_ucsi_connector_change(con->num, &status);
+	con->status = status;
 
 	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
 
@@ -816,53 +834,6 @@ static void ucsi_connector_work(struct work_struct *work)
 	mutex_unlock(&con->lock);
 }
 
-/*
- * We can not read the connector status in ucsi_connector_change() function
- * below because there may be already a command pending. This work is scheduled
- * separately only because of that.
- *
- * This function must finish fast so we do not loose the next events. Every
- * event will have a separate job queued for it in the connector specific
- * workqueue. That way the next event can be generated safely before the
- * previous ones are fully processed.
- */
-static void ucsi_handle_connector_change(struct work_struct *work)
-{
-	struct ucsi_connector *con = container_of(work, struct ucsi_connector, work);
-	struct ucsi_connector_status status;
-	struct ucsi_con_event *event;
-	u64 command;
-	int ret;
-
-	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
-	ret = ucsi_send_command(con->ucsi, command, &status, sizeof(status));
-	if (ret < 0) {
-		dev_err(con->ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret);
-		goto out_ack;
-	}
-
-	if (!status.change) {
-		dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
-		goto out_ack;
-	}
-
-	event = kzalloc(sizeof(*event), GFP_KERNEL);
-	if (!event)
-		goto out_ack;
-
-	INIT_WORK(&event->work, ucsi_connector_work);
-	event->status = status;
-	event->con = con;
-	queue_work(con->wq, &event->work);
-
-out_ack:
-	clear_bit(EVENT_PENDING, &con->ucsi->flags);
-
-	ret = ucsi_acknowledge_connector_change(con->ucsi);
-	if (ret)
-		dev_err(con->ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
-}
-
 /**
  * ucsi_connector_change - Process Connector Change Event
  * @ucsi: UCSI Interface
@@ -871,16 +842,20 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 {
 	struct ucsi_connector *con = &ucsi->connector[num - 1];
+	struct ucsi_con_event *event;
 
 	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
 		dev_dbg(ucsi->dev, "Bogus connector change event\n");
 		return;
 	}
 
-	if (test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
 		return;
 
-	schedule_work(&con->work);
+	INIT_WORK(&event->work, ucsi_connector_work);
+	event->con = con;
+	queue_work(con->wq, &event->work);
 }
 EXPORT_SYMBOL_GPL(ucsi_connector_change);
 
@@ -1078,7 +1053,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	if (!con->wq)
 		return -ENOMEM;
 
-	INIT_WORK(&con->work, ucsi_handle_connector_change);
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
 	con->num = index + 1;
-- 
2.30.2