| Message ID | 16623de4-351d-135b-f3ff-701a465c5d92@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 14 Mar 2018 16:15:32 +0100 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 14 Mar 2018 16:06:49 +0100 > > * Add jump targets so that a call of the function "mutex_unlock" is stored > only once in these function implementations. > > * Replace 19 calls by goto statements. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Hi Markus, Some of these are good and sensible changes - others break the code. Please be careful to fully check all the resulting paths and ensure we don't change wether the lock is still held in all exit paths. Note a function that isn't lockdep annotated should not be holding any locks, that it took, upon exit. > --- > drivers/iio/gyro/bmg160_core.c | 103 ++++++++++++++++++----------------------- > 1 file changed, 45 insertions(+), 58 deletions(-) > > diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c > index 63ca31628a93..fa367fd7bc8c 100644 > --- a/drivers/iio/gyro/bmg160_core.c > +++ b/drivers/iio/gyro/bmg160_core.c > @@ -499,21 +499,19 @@ static int bmg160_get_temp(struct bmg160_data *data, int *val) > > mutex_lock(&data->mutex); > ret = bmg160_set_power_state(data, true); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > > ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val); > if (ret < 0) { > dev_err(dev, "Error reading reg_temp\n"); > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > > *val = sign_extend32(raw_val, 7); > ret = bmg160_set_power_state(data, false); > +unlock: > mutex_unlock(&data->mutex); > if (ret < 0) > return ret; > @@ -529,22 +527,20 @@ static int bmg160_get_axis(struct bmg160_data *data, int axis, int *val) > > mutex_lock(&data->mutex); > ret = bmg160_set_power_state(data, true); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > > ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val, > sizeof(raw_val)); > if (ret < 0) { > dev_err(dev, "Error reading axis %d\n", axis); > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > > *val = sign_extend32(le16_to_cpu(raw_val), 15); > ret = bmg160_set_power_state(data, false); > +unlock: > mutex_unlock(&data->mutex); > if (ret < 0) > return ret; > @@ -632,19 +628,16 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, > * mode to power on for other writes. > */ > ret = bmg160_set_power_state(data, true); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > + > ret = bmg160_set_bw(data, val); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > - ret = bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + > + goto set_power_state; > case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > if (val2) > return -EINVAL; > @@ -653,18 +646,15 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, > ret = bmg160_set_power_state(data, true); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > ret = bmg160_set_filter(data, val); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > - ret = bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + > + goto set_power_state; > case IIO_CHAN_INFO_SCALE: > if (val) > return -EINVAL; > @@ -672,24 +662,27 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, > mutex_lock(&data->mutex); > /* Refer to comments above for the suspend mode ops */ > ret = bmg160_set_power_state(data, true); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > + > ret = bmg160_set_scale(data, val2); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > - ret = bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); Please keep the mutex_unlock in the same scope as the mutex_lock. I may make sense to take both outside the switch statement but that needs careful consideration. > - return ret; > + > + goto set_power_state; > default: > return -EINVAL; We exit with the mutex locked now and it should not be. > } > > return -EINVAL; Mutex is still locked here and the return is wrong. > + > +set_power_state: > + ret = bmg160_set_power_state(data, false); > +unlock: > + mutex_unlock(&data->mutex); blank line before the return. > + return ret; > } > > static int bmg160_read_event(struct iio_dev *indio_dev, > @@ -763,8 +756,8 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev, > > if (!state && data->motion_trigger_on) { > data->ev_enable_state = 0; > - mutex_unlock(&data->mutex); > - return 0; > + ret = 0; Put this in as the value at instantiation. int ret = 0; > + goto unlock; > } > /* > * We will expect the enable and disable to do operation in > @@ -776,22 +769,19 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev, > * is always on so sequence doesn't matter > */ > ret = bmg160_set_power_state(data, state); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > > ret = bmg160_setup_any_motion_interrupt(data, state); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > > data->ev_enable_state = state; > +unlock: > mutex_unlock(&data->mutex); > - Blank line preferred before the return ret. > - return 0; > + return ret; > } > > static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("100 200 400 1000 2000"); > @@ -919,8 +909,8 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig, > > if (!state && data->ev_enable_state && data->motion_trigger_on) { > data->motion_trigger_on = false; > - mutex_unlock(&data->mutex); > - return 0; > + ret = 0; Setting ret where it is originally defined to 0 would be tidier. int ret = 0; > + goto unlock; > } > > /* > @@ -928,27 +918,24 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig, > * enable/disable operation order > */ > ret = bmg160_set_power_state(data, state); > - if (ret < 0) { > - mutex_unlock(&data->mutex); > - return ret; > - } > + if (ret < 0) > + goto unlock; > + > if (data->motion_trig == trig) > ret = bmg160_setup_any_motion_interrupt(data, state); > else > ret = bmg160_setup_new_data_interrupt(data, state); > if (ret < 0) { > bmg160_set_power_state(data, false); > - mutex_unlock(&data->mutex); > - return ret; > + goto unlock; > } > if (data->motion_trig == trig) > data->motion_trigger_on = state; > else > data->dready_trigger_on = state; > - > +unlock: > mutex_unlock(&data->mutex); > - > - return 0; I would prefer a blank line between the mutex_unlock and the return. > + return ret; > } > > static const struct iio_trigger_ops bmg160_trigger_ops = { -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 17.03.2018 um 20:54 schrieb Jonathan Cameron: > On Wed, 14 Mar 2018 16:15:32 +0100 > SF Markus Elfring <elfring@users.sourceforge.net> wrote: > >> From: Markus Elfring <elfring@users.sourceforge.net> >> Date: Wed, 14 Mar 2018 16:06:49 +0100 >> >> * Add jump targets so that a call of the function "mutex_unlock" is stored >> only once in these function implementations. >> >> * Replace 19 calls by goto statements. >> >> This issue was detected by using the Coccinelle software. >> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > Hi Markus, > > Some of these are good and sensible changes Such feedback is nice. > - others break the code. Which concrete places do you find questionable here? >> - return ret; >> + >> + goto set_power_state; >> default: >> return -EINVAL; > We exit with the mutex locked now and it should not be. I wonder about your source code interpretation here. The mutex was (and is still only) locked within case branches, isn't it? > >> } >> >> return -EINVAL; > Mutex is still locked here and the return is wrong. Should this statement get any more software development attention? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 17, 2018 at 07:54:22PM +0000, Jonathan Cameron wrote: > On Wed, 14 Mar 2018 16:15:32 +0100 > SF Markus Elfring <elfring@users.sourceforge.net> wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Wed, 14 Mar 2018 16:06:49 +0100 > > > > * Add jump targets so that a call of the function "mutex_unlock" is stored > > only once in these function implementations. > > > > * Replace 19 calls by goto statements. > > > > This issue was detected by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > Hi Markus, > > Some of these are good and sensible changes - others break the code. > Please be careful to fully check all the resulting paths and ensure > we don't change wether the lock is still held in all exit paths. > Note a function that isn't lockdep annotated should not be holding > any locks, that it took, upon exit. Please note that you are dealing with someone who is on many kernel maintainer's email-blacklist, including my own. I've found it's just not worth the time and energy in responding to these emails, it's a time-sink you will never find your way out of. good luck! greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 18 Mar 2018 09:19:47 +0100 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > Am 17.03.2018 um 20:54 schrieb Jonathan Cameron: > > On Wed, 14 Mar 2018 16:15:32 +0100 > > SF Markus Elfring <elfring@users.sourceforge.net> wrote: > > > >> From: Markus Elfring <elfring@users.sourceforge.net> > >> Date: Wed, 14 Mar 2018 16:06:49 +0100 > >> > >> * Add jump targets so that a call of the function "mutex_unlock" is stored > >> only once in these function implementations. > >> > >> * Replace 19 calls by goto statements. > >> > >> This issue was detected by using the Coccinelle software. > >> > >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > > > Hi Markus, > > > > Some of these are good and sensible changes > > Such feedback is nice. > > > > - others break the code. > > Which concrete places do you find questionable here? > > > >> - return ret; > >> + > >> + goto set_power_state; > >> default: > >> return -EINVAL; > > We exit with the mutex locked now and it should not be. > > I wonder about your source code interpretation here. > The mutex was (and is still only) locked within case branches, isn't it? > You are correct, this does however reflect the issue with the resulting lack of balance here. I saw the mutex was getting unlocked outside the local scope and so assumed that it was also take outside the local scope. That isn't true, so we have hurt readability. It might make sense to move the lock and unlock outside the switch statement but we certainly don't want to the the confusion that the lack of balance is causing here. I read it quickly and got the wrong idea which generally implies it is not as clear as we would like. Hence this change isn't going anywhere I'm afraid. Jonathan > > > > >> } > >> > >> return -EINVAL; > > Mutex is still locked here and the return is wrong. > > Should this statement get any more software development attention? > > Regards, > Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> The mutex was (and is still only) locked within case branches, isn't it? >> > You are correct, this does however reflect the issue with the resulting > lack of balance here. I suggest to reconsider affected software aspects a bit more. > I saw the mutex was getting unlocked outside the local scope and so assumed > that it was also take outside the local scope. Assumptions and corresponding expectations might need further clarifications. > That isn't true, so we have hurt readability. Does your conclusion need any adjustment? > I read it quickly and got the wrong idea which generally implies it is not > as clear as we would like. > > Hence this change isn't going anywhere I'm afraid. I imagine that more time will be needed then to get used to additional adjustments of implementation details in these functions. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> The mutex was (and is still only) locked within case branches, isn't it? >> > You are correct, this does however reflect the issue with the resulting > lack of balance here. Do you find changes for the other function implementations easier to integrate? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c index 63ca31628a93..fa367fd7bc8c 100644 --- a/drivers/iio/gyro/bmg160_core.c +++ b/drivers/iio/gyro/bmg160_core.c @@ -499,21 +499,19 @@ static int bmg160_get_temp(struct bmg160_data *data, int *val) mutex_lock(&data->mutex); ret = bmg160_set_power_state(data, true); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val); if (ret < 0) { dev_err(dev, "Error reading reg_temp\n"); bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } *val = sign_extend32(raw_val, 7); ret = bmg160_set_power_state(data, false); +unlock: mutex_unlock(&data->mutex); if (ret < 0) return ret; @@ -529,22 +527,20 @@ static int bmg160_get_axis(struct bmg160_data *data, int axis, int *val) mutex_lock(&data->mutex); ret = bmg160_set_power_state(data, true); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val, sizeof(raw_val)); if (ret < 0) { dev_err(dev, "Error reading axis %d\n", axis); bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } *val = sign_extend32(le16_to_cpu(raw_val), 15); ret = bmg160_set_power_state(data, false); +unlock: mutex_unlock(&data->mutex); if (ret < 0) return ret; @@ -632,19 +628,16 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, * mode to power on for other writes. */ ret = bmg160_set_power_state(data, true); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; + ret = bmg160_set_bw(data, val); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } - ret = bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + + goto set_power_state; case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: if (val2) return -EINVAL; @@ -653,18 +646,15 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, ret = bmg160_set_power_state(data, true); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } ret = bmg160_set_filter(data, val); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } - ret = bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + + goto set_power_state; case IIO_CHAN_INFO_SCALE: if (val) return -EINVAL; @@ -672,24 +662,27 @@ static int bmg160_write_raw(struct iio_dev *indio_dev, mutex_lock(&data->mutex); /* Refer to comments above for the suspend mode ops */ ret = bmg160_set_power_state(data, true); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; + ret = bmg160_set_scale(data, val2); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } - ret = bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + + goto set_power_state; default: return -EINVAL; } return -EINVAL; + +set_power_state: + ret = bmg160_set_power_state(data, false); +unlock: + mutex_unlock(&data->mutex); + return ret; } static int bmg160_read_event(struct iio_dev *indio_dev, @@ -763,8 +756,8 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev, if (!state && data->motion_trigger_on) { data->ev_enable_state = 0; - mutex_unlock(&data->mutex); - return 0; + ret = 0; + goto unlock; } /* * We will expect the enable and disable to do operation in @@ -776,22 +769,19 @@ static int bmg160_write_event_config(struct iio_dev *indio_dev, * is always on so sequence doesn't matter */ ret = bmg160_set_power_state(data, state); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; ret = bmg160_setup_any_motion_interrupt(data, state); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } data->ev_enable_state = state; +unlock: mutex_unlock(&data->mutex); - - return 0; + return ret; } static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("100 200 400 1000 2000"); @@ -919,8 +909,8 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig, if (!state && data->ev_enable_state && data->motion_trigger_on) { data->motion_trigger_on = false; - mutex_unlock(&data->mutex); - return 0; + ret = 0; + goto unlock; } /* @@ -928,27 +918,24 @@ static int bmg160_data_rdy_trigger_set_state(struct iio_trigger *trig, * enable/disable operation order */ ret = bmg160_set_power_state(data, state); - if (ret < 0) { - mutex_unlock(&data->mutex); - return ret; - } + if (ret < 0) + goto unlock; + if (data->motion_trig == trig) ret = bmg160_setup_any_motion_interrupt(data, state); else ret = bmg160_setup_new_data_interrupt(data, state); if (ret < 0) { bmg160_set_power_state(data, false); - mutex_unlock(&data->mutex); - return ret; + goto unlock; } if (data->motion_trig == trig) data->motion_trigger_on = state; else data->dready_trigger_on = state; - +unlock: mutex_unlock(&data->mutex); - - return 0; + return ret; } static const struct iio_trigger_ops bmg160_trigger_ops = {