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

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

[Eddie James]

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));
-- 
1.8.3.1

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

[Andrew Geissler]

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));
> --
> 1.8.3.1
>

Looks good!

Acked-by: Andrew Geissler <geissonator@gmail.com>
Tested-by: Andrew Geissler <geissonator@gmail.com>

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

[Christopher Bostic]

On 2/21/17 11:02 AM, Eddie James 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));
Reviewed-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>

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

[Joel Stanley]

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