| Message ID | 20200728091057.10.Ibe84fae61cd914c116e6d59ffeb644f1cbecd601@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sx9310 iio driver updates | expand |
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Checks for non-zero return values to signal error conditions. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 52 +++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 26 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 31234691a31abf..051c6515e62c18 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -338,7 +338,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data, > int ret; > > ret = regmap_write(data->regmap, SX9310_REG_SENSOR_SEL, chan->channel); > - if (ret < 0) > + if (ret) > return ret; > > return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val)); > @@ -354,7 +354,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > unsigned int val; > > ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &val); > - if (ret < 0) > + if (ret) > return ret; > > val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > static int sx9310_read_proximity(struct sx9310_data *data, > const struct iio_chan_spec *chan, int *val) > { > - int ret = 0; > + int ret; > __be16 rawval; > > mutex_lock(&data->mutex); > > ret = sx9310_get_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > if (data->client->irq) { > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > mutex_lock(&data->mutex); > > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > ret = sx9310_read_prox_data(data, chan, &rawval); > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > *val = sign_extend32(be16_to_cpu(rawval), > @@ -411,7 +411,7 @@ static int sx9310_read_proximity(struct sx9310_data *data, > } > > ret = sx9310_put_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > mutex_unlock(&data->mutex); > @@ -434,7 +434,7 @@ static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2) > unsigned int regval; > int ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, ®val); > > - if (ret < 0) > + if (ret) > return ret; > > regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> > @@ -535,7 +535,7 @@ static void sx9310_push_events(struct iio_dev *indio_dev) > > /* Read proximity state on all channels */ > ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val); > - if (ret < 0) { > + if (ret) { > dev_err(&data->client->dev, "i2c transfer error in irq\n"); > return; > } > @@ -570,7 +570,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private) > mutex_lock(&data->mutex); > > ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); > - if (ret < 0) { > + if (ret) { > dev_err(&data->client->dev, "i2c transfer error in irq\n"); > goto out; > } > @@ -613,20 +613,20 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev, > mutex_lock(&data->mutex); > if (state) { > ret = sx9310_get_event_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out_unlock; > if (!(data->chan_event & ~BIT(chan->channel))) { > ret = sx9310_enable_irq(data, eventirq); > - if (ret < 0) > + if (ret) > sx9310_put_event_channel(data, chan->channel); > } > } else { > ret = sx9310_put_event_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out_unlock; > if (!data->chan_event) { > ret = sx9310_disable_irq(data, eventirq); > - if (ret < 0) > + if (ret) > sx9310_get_event_channel(data, chan->channel); > } > } > @@ -665,7 +665,7 @@ static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state) > ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ); > else if (!data->chan_read) > ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ); > - if (ret < 0) > + if (ret) > goto out; > > data->trigger_enabled = state; > @@ -694,7 +694,7 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private) > indio_dev->masklength) { > ret = sx9310_read_prox_data(data, &indio_dev->channels[bit], > &val); > - if (ret < 0) > + if (ret) > goto out; > > data->buffer[i++] = val; > @@ -801,13 +801,13 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > unsigned int ctrl0; > > ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &ctrl0); > - if (ret < 0) > + if (ret) > return ret; > > /* run the compensation phase on all channels */ > ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, > ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK); > - if (ret < 0) > + if (ret) > return ret; > > ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val, > @@ -833,21 +833,21 @@ static int sx9310_init_device(struct iio_dev *indio_dev) > unsigned int i, val; > > ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET); > - if (ret < 0) > + if (ret) > return ret; > > usleep_range(1000, 2000); /* power-up time is ~1ms. */ > > /* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */ > ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); > - if (ret < 0) > + if (ret) > return ret; > > /* Program some sane defaults. */ > for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) { > initval = &sx9310_default_regs[i]; > ret = regmap_write(data->regmap, initval->reg, initval->def); > - if (ret < 0) > + if (ret) > return ret; > } > > @@ -901,14 +901,14 @@ static int sx9310_probe(struct i2c_client *client) > return PTR_ERR(data->regmap); > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > - if (ret < 0) { > + if (ret) { > dev_err(&client->dev, "error in reading WHOAMI register: %d", > ret); > return ret; > } > > ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami); > - if (ret < 0) > + if (ret) > return ret; > > ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev)); > @@ -920,7 +920,7 @@ static int sx9310_probe(struct i2c_client *client) > i2c_set_clientdata(client, indio_dev); > > ret = sx9310_init_device(indio_dev); > - if (ret < 0) > + if (ret) > return ret; > > if (client->irq) { > @@ -929,7 +929,7 @@ static int sx9310_probe(struct i2c_client *client) > sx9310_irq_thread_handler, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "sx9310_event", indio_dev); > - if (ret < 0) > + if (ret) > return ret; > > data->trig = > @@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client) > iio_pollfunc_store_time, > sx9310_trigger_handler, > &sx9310_buffer_setup_ops); > - if (ret < 0) > + if (ret) > return ret; > > return devm_iio_device_register(&client->dev, indio_dev); > -- > 2.28.0.rc0.142.g3c755180ce-goog >
Quoting Daniel Campello (2020-07-28 08:12:53) > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > static int sx9310_read_proximity(struct sx9310_data *data, > const struct iio_chan_spec *chan, int *val) > { > - int ret = 0; > + int ret; > __be16 rawval; > > mutex_lock(&data->mutex); > > ret = sx9310_get_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > if (data->client->irq) { > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > mutex_lock(&data->mutex); > > - if (ret < 0) > + if (ret) > goto out_disable_irq; Why is this condition checked after grabbing the mutex? Shouldn't it be checked before grabbing the mutex? Or is that supposed to be a mutex_unlock()? > > ret = sx9310_read_prox_data(data, chan, &rawval); > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > *val = sign_extend32(be16_to_cpu(rawval),
On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Daniel Campello (2020-07-28 08:12:53) > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > static int sx9310_read_proximity(struct sx9310_data *data, > > const struct iio_chan_spec *chan, int *val) > > { > > - int ret = 0; > > + int ret; > > __be16 rawval; > > > > mutex_lock(&data->mutex); > > > > ret = sx9310_get_read_channel(data, chan->channel); > > - if (ret < 0) > > + if (ret) > > goto out; > > > > if (data->client->irq) { > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > mutex_lock(&data->mutex); > > > > - if (ret < 0) > > + if (ret) > > goto out_disable_irq; > > Why is this condition checked after grabbing the mutex? Shouldn't it be > checked before grabbing the mutex? Or is that supposed to be a > mutex_unlock()? We acquire the lock before jumping to out_disable_irq which is before a mutex_unlock() > > > > > ret = sx9310_read_prox_data(data, chan, &rawval); > > - if (ret < 0) > > + if (ret) > > goto out_disable_irq; > > > > *val = sign_extend32(be16_to_cpu(rawval),
Quoting Daniel Campello (2020-07-28 14:23:29) > On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Daniel Campello (2020-07-28 08:12:53) > > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > > static int sx9310_read_proximity(struct sx9310_data *data, > > > const struct iio_chan_spec *chan, int *val) > > > { > > > - int ret = 0; > > > + int ret; > > > __be16 rawval; > > > > > > mutex_lock(&data->mutex); > > > > > > ret = sx9310_get_read_channel(data, chan->channel); > > > - if (ret < 0) > > > + if (ret) > > > goto out; > > > > > > if (data->client->irq) { > > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > > > mutex_lock(&data->mutex); > > > > > > - if (ret < 0) > > > + if (ret) > > > goto out_disable_irq; > > > > Why is this condition checked after grabbing the mutex? Shouldn't it be > > checked before grabbing the mutex? Or is that supposed to be a > > mutex_unlock()? > We acquire the lock before jumping to out_disable_irq which is before > a mutex_unlock() Does this function need to hold the mutex lock around get/put_read_channel? It drops the lock while waiting and then regrabs it which seems to imply that another reader could come in and try to get the channel again during the wait. So put another way, it may be simpler to shorten the lock area and then bail out of this function to a place where the lock isn't held already on the return path.
On Tue, Jul 28, 2020 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Daniel Campello (2020-07-28 14:23:29) > > On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Daniel Campello (2020-07-28 08:12:53) > > > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > > > static int sx9310_read_proximity(struct sx9310_data *data, > > > > const struct iio_chan_spec *chan, int *val) > > > > { > > > > - int ret = 0; > > > > + int ret; > > > > __be16 rawval; > > > > > > > > mutex_lock(&data->mutex); > > > > > > > > ret = sx9310_get_read_channel(data, chan->channel); > > > > - if (ret < 0) > > > > + if (ret) > > > > goto out; > > > > > > > > if (data->client->irq) { > > > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > > > > > mutex_lock(&data->mutex); > > > > > > > > - if (ret < 0) > > > > + if (ret) > > > > goto out_disable_irq; > > > > > > Why is this condition checked after grabbing the mutex? Shouldn't it be > > > checked before grabbing the mutex? Or is that supposed to be a > > > mutex_unlock()? > > We acquire the lock before jumping to out_disable_irq which is before > > a mutex_unlock() > > Does this function need to hold the mutex lock around get/put_read_channel? Yes, both get/put_read_channel and get/put_event_channel use sx9310_update_chan_en which is updating data->chan_{read,event} bitmaps. > It drops the lock while waiting and then regrabs it which seems to > imply that another reader could come in and try to get the channel again > during the wait. So put another way, it may be simpler to shorten the > lock area and then bail out of this function to a place where the lock > isn't held already on the return path.
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c index 31234691a31abf..051c6515e62c18 100644 --- a/drivers/iio/proximity/sx9310.c +++ b/drivers/iio/proximity/sx9310.c @@ -338,7 +338,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data, int ret; ret = regmap_write(data->regmap, SX9310_REG_SENSOR_SEL, chan->channel); - if (ret < 0) + if (ret) return ret; return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val)); @@ -354,7 +354,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) unsigned int val; ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &val); - if (ret < 0) + if (ret) return ret; val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) static int sx9310_read_proximity(struct sx9310_data *data, const struct iio_chan_spec *chan, int *val) { - int ret = 0; + int ret; __be16 rawval; mutex_lock(&data->mutex); ret = sx9310_get_read_channel(data, chan->channel); - if (ret < 0) + if (ret) goto out; if (data->client->irq) { @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, mutex_lock(&data->mutex); - if (ret < 0) + if (ret) goto out_disable_irq; ret = sx9310_read_prox_data(data, chan, &rawval); - if (ret < 0) + if (ret) goto out_disable_irq; *val = sign_extend32(be16_to_cpu(rawval), @@ -411,7 +411,7 @@ static int sx9310_read_proximity(struct sx9310_data *data, } ret = sx9310_put_read_channel(data, chan->channel); - if (ret < 0) + if (ret) goto out; mutex_unlock(&data->mutex); @@ -434,7 +434,7 @@ static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2) unsigned int regval; int ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, ®val); - if (ret < 0) + if (ret) return ret; regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> @@ -535,7 +535,7 @@ static void sx9310_push_events(struct iio_dev *indio_dev) /* Read proximity state on all channels */ ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val); - if (ret < 0) { + if (ret) { dev_err(&data->client->dev, "i2c transfer error in irq\n"); return; } @@ -570,7 +570,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private) mutex_lock(&data->mutex); ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); - if (ret < 0) { + if (ret) { dev_err(&data->client->dev, "i2c transfer error in irq\n"); goto out; } @@ -613,20 +613,20 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev, mutex_lock(&data->mutex); if (state) { ret = sx9310_get_event_channel(data, chan->channel); - if (ret < 0) + if (ret) goto out_unlock; if (!(data->chan_event & ~BIT(chan->channel))) { ret = sx9310_enable_irq(data, eventirq); - if (ret < 0) + if (ret) sx9310_put_event_channel(data, chan->channel); } } else { ret = sx9310_put_event_channel(data, chan->channel); - if (ret < 0) + if (ret) goto out_unlock; if (!data->chan_event) { ret = sx9310_disable_irq(data, eventirq); - if (ret < 0) + if (ret) sx9310_get_event_channel(data, chan->channel); } } @@ -665,7 +665,7 @@ static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state) ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ); else if (!data->chan_read) ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ); - if (ret < 0) + if (ret) goto out; data->trigger_enabled = state; @@ -694,7 +694,7 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private) indio_dev->masklength) { ret = sx9310_read_prox_data(data, &indio_dev->channels[bit], &val); - if (ret < 0) + if (ret) goto out; data->buffer[i++] = val; @@ -801,13 +801,13 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) unsigned int ctrl0; ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &ctrl0); - if (ret < 0) + if (ret) return ret; /* run the compensation phase on all channels */ ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK); - if (ret < 0) + if (ret) return ret; ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val, @@ -833,21 +833,21 @@ static int sx9310_init_device(struct iio_dev *indio_dev) unsigned int i, val; ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET); - if (ret < 0) + if (ret) return ret; usleep_range(1000, 2000); /* power-up time is ~1ms. */ /* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */ ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); - if (ret < 0) + if (ret) return ret; /* Program some sane defaults. */ for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) { initval = &sx9310_default_regs[i]; ret = regmap_write(data->regmap, initval->reg, initval->def); - if (ret < 0) + if (ret) return ret; } @@ -901,14 +901,14 @@ static int sx9310_probe(struct i2c_client *client) return PTR_ERR(data->regmap); ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); - if (ret < 0) { + if (ret) { dev_err(&client->dev, "error in reading WHOAMI register: %d", ret); return ret; } ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami); - if (ret < 0) + if (ret) return ret; ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev)); @@ -920,7 +920,7 @@ static int sx9310_probe(struct i2c_client *client) i2c_set_clientdata(client, indio_dev); ret = sx9310_init_device(indio_dev); - if (ret < 0) + if (ret) return ret; if (client->irq) { @@ -929,7 +929,7 @@ static int sx9310_probe(struct i2c_client *client) sx9310_irq_thread_handler, IRQF_TRIGGER_LOW | IRQF_ONESHOT, "sx9310_event", indio_dev); - if (ret < 0) + if (ret) return ret; data->trig = @@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client) iio_pollfunc_store_time, sx9310_trigger_handler, &sx9310_buffer_setup_ops); - if (ret < 0) + if (ret) return ret; return devm_iio_device_register(&client->dev, indio_dev);
Checks for non-zero return values to signal error conditions. Signed-off-by: Daniel Campello <campello@chromium.org> --- drivers/iio/proximity/sx9310.c | 52 +++++++++++++++++----------------- 1 file changed, 26 insertions(+), 26 deletions(-)