| Message ID | 20220610053012.27279-1-xiaohuizhang@ruc.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] iio:proximity:sx9360: Fix hardware gain read/write | expand |
On Fri, Jun 10, 2022 at 7:53 AM Xiaohui Zhang <xiaohuizhang@ruc.edu.cn> wrote: > > Similar to the handling of read/write in commit 108e4d4de2b5 > ("iio:proximity:sx9324: Fix hardware gain read/write"), we thought > a patch might be needed here as well. > > There are four possible gain values according to 'sx9360_gain_vals[]': > > 1, 2, 4, and 8 > > The values are off by one when writing and reading the register. The > bits should be set according to this equation: > > ilog2(<gain>) + 1 > > so that a gain of 8 is 0x4 in the register field and a gain of 4 is 0x3 > in the register field, etc. Note that a gain of 0 is reserved per the > datasheet. The default gain (SX9360_REG_PROX_CTRL0_GAIN_1) is also > wrong. It should be 0x1 << 3, i.e. 0x8, not 0x80 which is setting the > reserved bit 7. > > Fix this all up to properly handle the hardware gain and return errors > for invalid settings. ... > + regval = FIELD_GET(SX9360_REG_PROX_CTRL0_GAIN_MASK, regval); > + if (regval) > + regval--; > + else if (regval == SX9360_REG_PROX_CTRL0_GAIN_RSVD || > + regval > SX9360_REG_PROX_CTRL0_GAIN_8) else?! Isn't it a dead code? How has it been tested? > + return -EINVAL; > + *val = 1 << regval; Even in the original code this is wrong in accordance with C standard. It might have potentially UB. BIT(), for example, solves this issue. You may do what it does under the hood.
On Fri, 10 Jun 2022 16:37:05 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 10, 2022 at 7:53 AM Xiaohui Zhang <xiaohuizhang@ruc.edu.cn> wrote: > > > > Similar to the handling of read/write in commit 108e4d4de2b5 > > ("iio:proximity:sx9324: Fix hardware gain read/write"), we thought > > a patch might be needed here as well. > > > > There are four possible gain values according to 'sx9360_gain_vals[]': > > > > 1, 2, 4, and 8 > > > > The values are off by one when writing and reading the register. The > > bits should be set according to this equation: > > > > ilog2(<gain>) + 1 > > > > so that a gain of 8 is 0x4 in the register field and a gain of 4 is 0x3 > > in the register field, etc. Note that a gain of 0 is reserved per the > > datasheet. The default gain (SX9360_REG_PROX_CTRL0_GAIN_1) is also > > wrong. It should be 0x1 << 3, i.e. 0x8, not 0x80 which is setting the > > reserved bit 7. > > > > Fix this all up to properly handle the hardware gain and return errors > > for invalid settings. > > ... > > > + regval = FIELD_GET(SX9360_REG_PROX_CTRL0_GAIN_MASK, regval); > > + if (regval) > > + regval--; > > + else if (regval == SX9360_REG_PROX_CTRL0_GAIN_RSVD || > > + regval > SX9360_REG_PROX_CTRL0_GAIN_8) > > else?! Isn't it a dead code? How has it been tested? Gah. Missed this in review of sx9324 change. First check is fine because GAIN_RSVD is 0 though not a lot of point in the if. Second one is intended as hardening against malicious / broken hardware only so you would never see that value except via emulation or a unit test. So test wouldn't have spotted this as far as I can see. Needs good old eyeballs. :) > > > + return -EINVAL; > > > + *val = 1 << regval; > > Even in the original code this is wrong in accordance with C standard. > It might have potentially UB. BIT(), for example, solves this issue. > You may do what it does under the hood. >
diff --git a/drivers/iio/proximity/sx9360.c b/drivers/iio/proximity/sx9360.c index 3ebb30c8a4f6..f231929debb7 100644 --- a/drivers/iio/proximity/sx9360.c +++ b/drivers/iio/proximity/sx9360.c @@ -64,7 +64,10 @@ #define SX9360_REG_PROX_CTRL0_PHR 0x40 #define SX9360_REG_PROX_CTRL0_PHM 0x41 #define SX9360_REG_PROX_CTRL0_GAIN_MASK GENMASK(5, 3) -#define SX9360_REG_PROX_CTRL0_GAIN_1 0x80 +#define SX9360_REG_PROX_CTRL0_GAIN_SHIFT 3 +#define SX9360_REG_PROX_CTRL0_GAIN_RSVD 0x0 +#define SX9360_REG_PROX_CTRL0_GAIN_1 0x1 +#define SX9360_REG_PROX_CTRL0_GAIN_8 0x4 #define SX9360_REG_PROX_CTRL0_RAWFILT_MASK GENMASK(2, 0) #define SX9360_REG_PROX_CTRL0_RAWFILT_1P50 0x01 #define SX9360_REG_PROX_CTRL1 0x42 @@ -288,7 +291,14 @@ static int sx9360_read_gain(struct sx_common_data *data, if (ret) return ret; - *val = 1 << FIELD_GET(SX9360_REG_PROX_CTRL0_GAIN_MASK, regval); + regval = FIELD_GET(SX9360_REG_PROX_CTRL0_GAIN_MASK, regval); + if (regval) + regval--; + else if (regval == SX9360_REG_PROX_CTRL0_GAIN_RSVD || + regval > SX9360_REG_PROX_CTRL0_GAIN_8) + return -EINVAL; + + *val = 1 << regval; return IIO_VAL_INT; } @@ -630,8 +640,12 @@ static int sx9360_write_gain(struct sx_common_data *data, unsigned int gain, reg; int ret; - gain = ilog2(val); reg = SX9360_REG_PROX_CTRL0_PHR + chan->channel; + + gain = ilog2(val) + 1; + if (val <= 0 || gain > SX9360_REG_PROX_CTRL0_GAIN_8) + return -EINVAL; + gain = FIELD_PREP(SX9360_REG_PROX_CTRL0_GAIN_MASK, gain); mutex_lock(&data->mutex); @@ -681,9 +695,11 @@ static const struct sx_common_reg_default sx9360_default_regs[] = { { SX9360_REG_AFE_PARAM1_PHM, SX9360_REG_AFE_PARAM1_AGAIN_PHM_6PF | SX9360_REG_AFE_PARAM1_FREQ_83_33HZ }, - { SX9360_REG_PROX_CTRL0_PHR, SX9360_REG_PROX_CTRL0_GAIN_1 | + { SX9360_REG_PROX_CTRL0_PHR, + SX9360_REG_PROX_CTRL0_GAIN_1 << SX9360_REG_PROX_CTRL0_GAIN_SHIFT | SX9360_REG_PROX_CTRL0_RAWFILT_1P50 }, - { SX9360_REG_PROX_CTRL0_PHM, SX9360_REG_PROX_CTRL0_GAIN_1 | + { SX9360_REG_PROX_CTRL0_PHM, + SX9360_REG_PROX_CTRL0_GAIN_1 << SX9360_REG_PROX_CTRL0_GAIN_SHIFT | SX9360_REG_PROX_CTRL0_RAWFILT_1P50 }, { SX9360_REG_PROX_CTRL1, SX9360_REG_PROX_CTRL1_AVGNEG_THRESH_16K }, { SX9360_REG_PROX_CTRL2, SX9360_REG_PROX_CTRL2_AVGDEB_2SAMPLES |
Similar to the handling of read/write in commit 108e4d4de2b5 ("iio:proximity:sx9324: Fix hardware gain read/write"), we thought a patch might be needed here as well. There are four possible gain values according to 'sx9360_gain_vals[]': 1, 2, 4, and 8 The values are off by one when writing and reading the register. The bits should be set according to this equation: ilog2(<gain>) + 1 so that a gain of 8 is 0x4 in the register field and a gain of 4 is 0x3 in the register field, etc. Note that a gain of 0 is reserved per the datasheet. The default gain (SX9360_REG_PROX_CTRL0_GAIN_1) is also wrong. It should be 0x1 << 3, i.e. 0x8, not 0x80 which is setting the reserved bit 7. Fix this all up to properly handle the hardware gain and return errors for invalid settings. Signed-off-by: Xiaohui Zhang <xiaohuizhang@ruc.edu.cn> --- drivers/iio/proximity/sx9360.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)