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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4cf2fa5b-f909-1ab1-f743-e1cb9a66c604@nvidia.com>
Date:   Tue, 14 Nov 2017 15:56:10 +0000
From:   Jon Hunter <jonathanh@...dia.com>
To:     Shawn N <shawnn@...omium.org>
CC:     Olof Johansson <olof@...om.net>,
        Benson Leung <bleung@...omium.org>,
        "Lee Jones" <lee.jones@...aro.org>, <linux-kernel@...r.kernel.org>,
        Doug Anderson <dianders@...omium.org>,
        Brian Norris <computersforpeace@...il.com>,
        "Brian Norris" <briannorris@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Enric Balletbo <enric.balletbo@...labora.co.uk>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3] platform/chrome: Use proper protocol transfer function

Hi Shawn,

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

...

>> From: Shawn Nematbakhsh <shawnn@...omium.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@...omium.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@...dia.com>

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