From: Guenter Roeck <linux@roeck-us.net>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Xu Yang <xu.yang_2@nxp.com>
Cc: jun.li@nxp.com, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-imx@nxp.com
Subject: Re: [PATCH v2] usb: typec: tcpm: fix issue of multiple tcpm_set_state
Date: Mon, 11 Oct 2021 07:11:38 -0700 [thread overview]
Message-ID: <e98640b8-e5d8-b2e2-f9e0-bc1f6a0debcb@roeck-us.net> (raw)
In-Reply-To: <YWPaPfzlnWdEsk21@kuha.fi.intel.com>
On 10/10/21 11:31 PM, Heikki Krogerus wrote:
> Guenter, can you check this?
>
I tried quite some time ago, but was concerned if it really solves
the problem or just the symptom, and never had the time for a deep
dive so far.
Guenter
> On Fri, Aug 27, 2021 at 07:48:09PM +0800, Xu Yang wrote:
>> There are potential problems when states are set as following:
>>
>> tcpm_set_state(A, 0)
>> tcpm_set_state(B, X)
>>
>> As long as the state A is set and the state_machine work is queued
>> successfully, state_machine work will be scheduled soon after. Before
>> running into tcpm_state_machine_work(), there is a chance to set state
>> B again. If it does occur:
>>
>> either (X = 0)
>> port->state = B and state_machine work is queued again, then work
>> will be executed twice.
>> or (X != 0)
>> port->state = A and port->delayed_state = B, then work will be
>> executed once but timer is still running.
>>
>> For this situation, tcpm should only handle the most recent state change
>> as if only one state is set just now. Therefore, if the state_machine work
>> has already been queued, it can't be queued again before running into
>> tcpm_state_machine_work().
>>
>> The state_machine_running flag already prevents from queuing the work, so
>> we can make it contain the pending stage (after work be queued and before
>> running into tcpm_state_machine_work). The state_machine_pending_or_running
>> flag can be used to indicate that a state can be handled without queuing
>> the work again.
>>
>> Because the state_machine work has been queued for state A, there is no
>> way to cancel as it may be already dequeued later, and then will run into
>> tcpm_state_machine_work() certainly. To handle the delayed state B, such
>> an abnormal work should be skiped. If port->delayed_state != INVALID_STATE
>> and timer is still running, it's time to skip.
>>
>> Fixes: 4b4e02c83167 ("typec: tcpm: Move out of staging")
>> cc: <stable@vger.kernel.org>
>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> What changed since v1?
>
> thanks,
>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 049f4c61ee82..a913bc620e88 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -371,7 +371,7 @@ struct tcpm_port {
>> struct kthread_work enable_frs;
>> struct hrtimer send_discover_timer;
>> struct kthread_work send_discover_work;
>> - bool state_machine_running;
>> + bool state_machine_pending_or_running;
>> bool vdm_sm_running;
>>
>> struct completion tx_complete;
>> @@ -1192,6 +1192,7 @@ static void mod_tcpm_delayed_work(struct tcpm_port *port, unsigned int delay_ms)
>> } else {
>> hrtimer_cancel(&port->state_machine_timer);
>> kthread_queue_work(port->wq, &port->state_machine);
>> + port->state_machine_pending_or_running = true;
>> }
>> }
>>
>> @@ -1250,7 +1251,7 @@ static void tcpm_set_state(struct tcpm_port *port, enum tcpm_state state,
>> * tcpm_state_machine_work() will continue running the state
>> * machine.
>> */
>> - if (!port->state_machine_running)
>> + if (!port->state_machine_pending_or_running)
>> mod_tcpm_delayed_work(port, 0);
>> }
>> }
>> @@ -4810,13 +4811,15 @@ static void tcpm_state_machine_work(struct kthread_work *work)
>> enum tcpm_state prev_state;
>>
>> mutex_lock(&port->lock);
>> - port->state_machine_running = true;
>>
>> if (port->queued_message && tcpm_send_queued_message(port))
>> goto done;
>>
>> /* If we were queued due to a delayed state change, update it now */
>> if (port->delayed_state) {
>> + if (ktime_before(ktime_get(), port->delayed_runtime))
>> + goto done;
>> +
>> tcpm_log(port, "state change %s -> %s [delayed %ld ms]",
>> tcpm_states[port->state],
>> tcpm_states[port->delayed_state], port->delay_ms);
>> @@ -4837,7 +4840,7 @@ static void tcpm_state_machine_work(struct kthread_work *work)
>> } while (port->state != prev_state && !port->delayed_state);
>>
>> done:
>> - port->state_machine_running = false;
>> + port->state_machine_pending_or_running = false;
>> mutex_unlock(&port->lock);
>> }
>>
>> @@ -6300,6 +6303,7 @@ static enum hrtimer_restart state_machine_timer_handler(struct hrtimer *timer)
>> struct tcpm_port *port = container_of(timer, struct tcpm_port, state_machine_timer);
>>
>> kthread_queue_work(port->wq, &port->state_machine);
>> + port->state_machine_pending_or_running = true;
>> return HRTIMER_NORESTART;
>> }
>>
>> --
>> 2.25.1
>
next prev parent reply other threads:[~2021-10-11 14:29 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-27 11:48 [PATCH v2] usb: typec: tcpm: fix issue of multiple tcpm_set_state Xu Yang
2021-09-13 10:42 ` Xu Yang
2021-10-09 2:28 ` Xu Yang
2021-10-11 6:31 ` Heikki Krogerus
2021-10-11 9:17 ` [EXT] " Xu Yang
2021-10-11 9:52 ` Heikki Krogerus
2021-10-11 14:11 ` Guenter Roeck [this message]
2021-10-11 10:32 Xu Yang
2021-10-11 11:25 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e98640b8-e5d8-b2e2-f9e0-bc1f6a0debcb@roeck-us.net \
--to=linux@roeck-us.net \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=jun.li@nxp.com \
--cc=linux-imx@nxp.com \
--cc=linux-usb@vger.kernel.org \
--cc=xu.yang_2@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).