Messages in this thread | | | From | Jett Rink <> | Date | Tue, 14 Apr 2020 10:45:47 -0600 | Subject | Re: [PATCH v2 1/2] platform/chrome: cros_ec_ishtp: skip old cros_ec responses |
| |
Thank you!
On Tue, Apr 14, 2020 at 10:24 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > > Hi Jett and Mathew, > > Thank you for the patch > > On 10/4/20 18:23, Mathew King wrote: > > From: Jett Rink <jettrink@chromium.org> > > > > The ISHTP layer can give us old responses that we already gave up on. We > > do not want to interpret these old responses as the current response we > > are waiting for. > > > > The cros_ish should only have one request in flight at a time. We send > > the request and wait for the response from the ISH. If the ISH is too > > slow to respond we give up on that request and we can send a new > > request. The ISH may still send the response to the request that timed > > out and without this we treat the old response as the response to the > > current command. This is a condition that should not normally happen but > > it has been observed with a bad ISH image. So add a token to the request > > header which is copied into the response header when the ISH processes > > the message to ensure that response is for the current request. > > > > Signed-off-by: Jett Rink <jettrink@chromium.org> > > Signed-off-by: Mathew King <mathewk@chromium.org> > > Queued for 5.8 > > > --- > > v2: - Change from using id to token > > - Reword the commit message > > --- > > drivers/platform/chrome/cros_ec_ishtp.c | 32 ++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c > > index 93a71e93a2f1..e673a7f738fc 100644 > > --- a/drivers/platform/chrome/cros_ec_ishtp.c > > +++ b/drivers/platform/chrome/cros_ec_ishtp.c > > @@ -48,7 +48,8 @@ static const guid_t cros_ish_guid = > > struct header { > > u8 channel; > > u8 status; > > - u8 reserved[2]; > > + u8 token; > > + u8 reserved; > > } __packed; > > > > struct cros_ish_out_msg { > > @@ -90,6 +91,7 @@ static DECLARE_RWSEM(init_lock); > > * data exceeds this value, we log an error. > > * @size: Actual size of data received from firmware. > > * @error: 0 for success, negative error code for a failure in process_recv(). > > + * @token: Expected token for response that we are waiting on. > > * @received: Set to true on receiving a valid firmware response to host command > > * @wait_queue: Wait queue for host to wait for firmware response. > > */ > > @@ -98,6 +100,7 @@ struct response_info { > > size_t max_size; > > size_t size; > > int error; > > + u8 token; > > bool received; > > wait_queue_head_t wait_queue; > > }; > > @@ -162,6 +165,7 @@ static int ish_send(struct ishtp_cl_data *client_data, > > u8 *out_msg, size_t out_size, > > u8 *in_msg, size_t in_size) > > { > > + static u8 next_token; > > int rv; > > struct header *out_hdr = (struct header *)out_msg; > > struct ishtp_cl *cros_ish_cl = client_data->cros_ish_cl; > > @@ -174,8 +178,11 @@ static int ish_send(struct ishtp_cl_data *client_data, > > client_data->response.data = in_msg; > > client_data->response.max_size = in_size; > > client_data->response.error = 0; > > + client_data->response.token = next_token++; > > client_data->response.received = false; > > > > + out_hdr->token = client_data->response.token; > > + > > rv = ishtp_cl_send(cros_ish_cl, out_msg, out_size); > > if (rv) { > > dev_err(cl_data_to_dev(client_data), > > @@ -249,17 +256,23 @@ static void process_recv(struct ishtp_cl *cros_ish_cl, > > > > switch (in_msg->hdr.channel) { > > case CROS_EC_COMMAND: > > - /* Sanity check */ > > - if (!client_data->response.data) { > > + if (client_data->response.received) { > > dev_err(dev, > > - "Receiving buffer is null. Should be allocated by calling function\n"); > > - client_data->response.error = -EINVAL; > > - goto error_wake_up; > > + "Previous firmware message not yet processed\n"); > > + goto end_error; > > } > > > > - if (client_data->response.received) { > > + if (client_data->response.token != in_msg->hdr.token) { > > + dev_err_ratelimited(dev, > > + "Dropping old response token %d\n", > > + in_msg->hdr.token); > > + goto end_error; > > + } > > + > > + /* Sanity check */ > > + if (!client_data->response.data) { > > dev_err(dev, > > - "Previous firmware message not yet processed\n"); > > + "Receiving buffer is null. Should be allocated by calling function\n"); > > client_data->response.error = -EINVAL; > > goto error_wake_up; > > } > > @@ -289,9 +302,10 @@ static void process_recv(struct ishtp_cl *cros_ish_cl, > > memcpy(client_data->response.data, > > rb_in_proc->buffer.data, data_len); > > > > +error_wake_up: > > /* Set flag before waking up the caller */ > > client_data->response.received = true; > > -error_wake_up: > > + > > /* Wake the calling thread */ > > wake_up_interruptible(&client_data->response.wait_queue); > > > >
| |