Re: [PATCH v3] platform/chrome: Use proper protocol transfer function

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

Hi Shawn,

On 26/09/17 16:40, Jon Hunter wrote:
> On 26/09/17 00:15, Shawn N wrote:

...

>> From: Shawn Nematbakhsh <shawnn-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Date: Mon, 25 Sep 2017 14:32:38 -0700
>> Subject: [PATCH] mfd: cros ec: spi: Fix "in progress" error signaling
>>
>> For host commands that take a long time to process, cros ec can return
>> early by signaling a EC_RES_IN_PROGRESS result. The host must then poll
>> status with EC_CMD_GET_COMMS_STATUS until completion of the command.
>>
>> None of the above applies when data link errors are encountered. When
>> errors such as EC_SPI_PAST_END are encountered during command
>> transmission, it usually means the command was not received by the EC.
>> Treating such errors as if they were 'EC_RES_IN_PROGRESS' results is
>> almost always the wrong decision, and can result in host commands
>> silently being lost.
>>
>> Signed-off-by: Shawn Nematbakhsh <shawnn-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  drivers/mfd/cros_ec_spi.c | 26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
>> index c9714072e224..d33e3847e11e 100644
>> --- a/drivers/mfd/cros_ec_spi.c
>> +++ b/drivers/mfd/cros_ec_spi.c
>> @@ -377,6 +377,7 @@ static int cros_ec_pkt_xfer_spi(struct
>> cros_ec_device *ec_dev,
>>         u8 *ptr;
>>         u8 *rx_buf;
>>         u8 sum;
>> +       u8 rx_byte;
>>         int ret = 0, final_ret;
>>
>>         len = cros_ec_prepare_tx(ec_dev, ec_msg);
>> @@ -421,25 +422,22 @@ static int cros_ec_pkt_xfer_spi(struct
>> cros_ec_device *ec_dev,
>>         if (!ret) {
>>                 /* Verify that EC can process command */
>>                 for (i = 0; i < len; i++) {
>> -                       switch (rx_buf[i]) {
>> -                       case EC_SPI_PAST_END:
>> -                       case EC_SPI_RX_BAD_DATA:
>> -                       case EC_SPI_NOT_READY:
>> -                               ret = -EAGAIN;
>> -                               ec_msg->result = EC_RES_IN_PROGRESS;
>> -                       default:
>> +                       rx_byte = rx_buf[i];
>> +                       if (rx_byte == EC_SPI_PAST_END  ||
>> +                           rx_byte == EC_SPI_RX_BAD_DATA ||
>> +                           rx_byte == EC_SPI_NOT_READY) {
>> +                               ret = -EREMOTEIO;
>>                                 break;
>>                         }
>> -                       if (ret)
>> -                               break;
>>                 }
>> -               if (!ret)
>> -                       ret = cros_ec_spi_receive_packet(ec_dev,
>> -                                       ec_msg->insize + sizeof(*response));
>> -       } else {
>> -               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
>>         }
>>
>> +       if (!ret)
>> +               ret = cros_ec_spi_receive_packet(ec_dev,
>> +                               ec_msg->insize + sizeof(*response));
>> +       else
>> +               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
>> +
>>         final_ret = terminate_request(ec_dev);
>>
>>         spi_bus_unlock(ec_spi->spi->master);
>>
> 
> Thanks! Works for me ...
> 
> Tested-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I can't find the formal patch you sent out for the above, but I have not
seen it picked up yet. I am guess that Lee did not pick it up because
there was still an open question. Anyhow we may want to circle back with
Lee on this so that this does get picked up.

Cheers
Jon

-- 
nvpublic