Re: [PATCH linux dev-4.7] drivers: fsi: scom: Zero out user buffer first

[Joel Stanley <joel@jms.id.au>]

On Wed, Feb 22, 2017 at 1:51 PM, Andrew Geissler <geissonator@gmail.com> wrote:
> On Tue, Feb 21, 2017 at 11:02 AM, Eddie James
> <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Scom data is just bit-wise OR'd with the user buffer, so it needs to be
>> set to 0 first.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>  drivers/fsi/fsi-scom.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
>> index 51b3dc3..ed3b98e 100644
>> --- a/drivers/fsi/fsi-scom.c
>> +++ b/drivers/fsi/fsi-scom.c
>> @@ -85,6 +85,7 @@ static int get_scom(struct scom_device *scom_dev, uint64_t *value,
>>         uint32_t result, data;
>>         int rc;
>>
>> +       *value = 0ULL;
>>         data = addr;
>>         rc = fsi_device_write(scom_dev->fsi_dev, SCOM_CMD_REG, &data,
>>                                 sizeof(uint32_t));

We could do something more sensible, like apply the low 32 bits to
value and then OR in the top half:

        rc = fsi_device_read(scom_dev->fsi_dev, SCOM_DATA1_REG, &result,
                                sizeof(uint32_t));
        if (rc)
                return rc;
       *value = result;

       rc = fsi_device_read(scom_dev->fsi_dev, SCOM_DATA0_REG, &result,
                                sizeof(uint32_t));
        if (rc)
                return rc;

        *value |= (uint64_t) result << 32;

Or have result1 and result2, and then only save to value the once:

    *value = (result0 << 32) | result1;

And then there are questions of endianess that must be considered
before upstreaming.

I have applied the fix as-is to dev-4.7.  Please consider a cleaner
fix when applying this patch to the series for upstream.

Thanks for the review everyone. Most reviewed one liner OpenBMC has ever seen :D

In all seriousness, keep it up, it's good to share the load.

Cheers,

Joel