| Message ID | 20220922225012.1709173-1-floridsleeves@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v1] drivers/magnetometer/ak8975: check the return value of ak8975_set_mode | expand |
On Thu, Sep 22, 2022 at 3:50 PM Li Zhong <floridsleeves@gmail.com> wrote: > > Check the return value of ak8975_set_mode(). When it fails to write the > register, the error should at least be propagated to the caller. > Currently function ak8975_remove() returns the success value 0 > no matter the execution fails or not, which will silently leave the > error unhandled. > > Signed-off-by: Li Zhong <floridsleeves@gmail.com> > --- > drivers/iio/magnetometer/ak8975.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c > index 2432e697150c..e08f10fe16d0 100644 > --- a/drivers/iio/magnetometer/ak8975.c > +++ b/drivers/iio/magnetometer/ak8975.c > @@ -1022,16 +1022,17 @@ static int ak8975_remove(struct i2c_client *client) > { > struct iio_dev *indio_dev = i2c_get_clientdata(client); > struct ak8975_data *data = iio_priv(indio_dev); > + int err = 0; Not needed to initialize to zero since it will get set below. > > pm_runtime_get_sync(&client->dev); > pm_runtime_put_noidle(&client->dev); > pm_runtime_disable(&client->dev); > iio_device_unregister(indio_dev); > iio_triggered_buffer_cleanup(indio_dev); > - ak8975_set_mode(data, POWER_DOWN); > + err = ak8975_set_mode(data, POWER_DOWN); I am not convinced there is a real reason to report this failure from .remove() since device is still unregistered and at the worst case it isn't in low-power mode. Could you explain if this is an issue that actually happens? If it is, this should be a warning that is displayed and not a failure to remove the driver. - Matt > ak8975_power_off(data); > > - return 0; > + return err; > } > > static int ak8975_runtime_suspend(struct device *dev) > -- > 2.25.1 >
On Thu, 2022-09-22 at 16:50 -0700, Matt Ranostay wrote: > On Thu, Sep 22, 2022 at 3:50 PM Li Zhong <floridsleeves@gmail.com> > wrote: > > > > Check the return value of ak8975_set_mode(). When it fails to write > > the > > register, the error should at least be propagated to the caller. > > Currently function ak8975_remove() returns the success value 0 > > no matter the execution fails or not, which will silently leave the > > error unhandled. > > > > Signed-off-by: Li Zhong <floridsleeves@gmail.com> > > --- > > drivers/iio/magnetometer/ak8975.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/magnetometer/ak8975.c > > b/drivers/iio/magnetometer/ak8975.c > > index 2432e697150c..e08f10fe16d0 100644 > > --- a/drivers/iio/magnetometer/ak8975.c > > +++ b/drivers/iio/magnetometer/ak8975.c > > @@ -1022,16 +1022,17 @@ static int ak8975_remove(struct i2c_client > > *client) > > { > > struct iio_dev *indio_dev = i2c_get_clientdata(client); > > struct ak8975_data *data = iio_priv(indio_dev); > > + int err = 0; > > Not needed to initialize to zero since it will get set below. > > > > > pm_runtime_get_sync(&client->dev); > > pm_runtime_put_noidle(&client->dev); > > pm_runtime_disable(&client->dev); > > iio_device_unregister(indio_dev); > > iio_triggered_buffer_cleanup(indio_dev); > > - ak8975_set_mode(data, POWER_DOWN); > > + err = ak8975_set_mode(data, POWER_DOWN); > > I am not convinced there is a real reason to report this failure from > .remove() since > device is still unregistered and at the worst case it isn't in low- > power mode. > Agreed.. Also look here: https://lore.kernel.org/linux-i2c/20220609091018.q52fhowlsdbdkct5@pengutronix.de/ I'm not sure the status of this but the key point is that returning an error code does not make a difference. - Nuno Sá
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index 2432e697150c..e08f10fe16d0 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -1022,16 +1022,17 @@ static int ak8975_remove(struct i2c_client *client) { struct iio_dev *indio_dev = i2c_get_clientdata(client); struct ak8975_data *data = iio_priv(indio_dev); + int err = 0; pm_runtime_get_sync(&client->dev); pm_runtime_put_noidle(&client->dev); pm_runtime_disable(&client->dev); iio_device_unregister(indio_dev); iio_triggered_buffer_cleanup(indio_dev); - ak8975_set_mode(data, POWER_DOWN); + err = ak8975_set_mode(data, POWER_DOWN); ak8975_power_off(data); - return 0; + return err; } static int ak8975_runtime_suspend(struct device *dev)
Check the return value of ak8975_set_mode(). When it fails to write the register, the error should at least be propagated to the caller. Currently function ak8975_remove() returns the success value 0 no matter the execution fails or not, which will silently leave the error unhandled. Signed-off-by: Li Zhong <floridsleeves@gmail.com> --- drivers/iio/magnetometer/ak8975.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)