linux-kernel - Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

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] [day] [month] [year] [list]

Message-ID: <8ca189e8-dc68-8438-ab3f-f468e839507a@vaisala.com>
Date:   Mon, 19 Apr 2021 15:48:52 +0300
From:   Tomas Melin <tomas.melin@...sala.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300
 accelerometer


On 4/19/21 3:29 PM, Andy Shevchenko wrote:
> On Mon, Apr 19, 2021 at 2:36 PM Tomas Melin <tomas.melin@...sala.com> wrote:
>> On 4/19/21 2:14 PM, Andy Shevchenko wrote:
>>> On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin <tomas.melin@...sala.com> wrote:
>>>> On 4/17/21 3:39 PM, Andy Shevchenko wrote:
>>>>> On Fri, Apr 16, 2021 at 5:21 PM Tomas Melin <tomas.melin@...sala.com> wrote:
>>>>>> Add initial support for Murata SCA3300 3-axis industrial
>>>>>> accelerometer with digital SPI interface. This device also
>>>>>> provides a temperature measurement.
>>> ...
>>>
>>>>>> +       ret = spi_sync_transfer(sca_data->spi, xfers, ARRAY_SIZE(xfers));
>>>>>> +       if (ret < 0) {
>>>>>> +               dev_err(&sca_data->spi->dev,
>>>>>> +                       "transfer error, error: %d\n", ret);
>>>>>> +               return -EIO;
>>>>> Why shadowing error code?
>>>> Returning EIO here to have full control over the return value from this
>>>> function. As return value of this is used for testing
>>> Care to show what kind of testing requires this?
>>> Also why can't it be refactored to accept all error codes?
>> I was referring to this:
>>
>> +static int sca3300_read_reg(struct sca3300_data *sca_data, u8 reg, int *val)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&sca_data->lock);
>> +       sca_data->txbuf[0] = 0x0 | (reg << 2);
>> +       ret = sca3300_transfer(sca_data, val);
>> +       mutex_unlock(&sca_data->lock);
>> +       if (ret == -EINVAL)
>> +               ret  = sca3300_error_handler(sca_data);
>> +
>> +       return ret;
>> +}
>> +
>> +static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
>> +{
>> +       int reg_val = 0;
>> +       int ret;
>> +
>> +       mutex_lock(&sca_data->lock);
>> +       sca_data->txbuf[0] = BIT(7) | (reg << 2);
>> +       put_unaligned_be16(val, &sca_data->txbuf[1]);
>> +       ret = sca3300_transfer(sca_data, &reg_val);
>> +       mutex_unlock(&sca_data->lock);
>> +       if (ret == -EINVAL)
>> +               ret  = sca3300_error_handler(sca_data);
>> +
>> +       return ret;
>> +}
>>
>> So this goes into error handling only when transfer indicates EINVAL
>> (which happens when
>>
>> transfer otherwise is good, but device return status has error flags set
>> i message).
> In such cases I would recommend introducing your own error space (with
> positive numbers) or playing around with the number of transfers (but
> this usually works only if you anticipate several of them in a row).
>
> Something like
>
> #define SCA3300_ERROR_FLAGS 1
> ...
>
> if (ret > 0)
>    return error_handler(..., ret); // ret in case if you want to
> convert the code to something in Linux error code space.

In version 1 of this patchset, positive return values were used for 
indicating this situation.

One of the comments I got from Jonathan to that version was to change

to use appropriate standard error code rather than positive return value.

I would thus keep this as is, hopefully You are ok with current approach.

Thanks,

Tomas


>
>>>> for possible status error (EINVAL), feels more confident to have it like
>>>> this to avoid any confusion. And atleast spi_sync_transfer() return value
>>>>
>>>> would be visible in error message.
>>>>>> +       }