| Message ID | 20211026194010.109029-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [media] tua9001: Improve messages in .remove's error path | expand |
Quoting Uwe Kleine-König (2021-10-26 20:40:10) > If disabling the hardware fails the driver propagates the error code to > the i2c core. However this only results in a generic error message; the > device still disappears. > > So instead emit a message that better describes the actual problem than > the i2c core's "remove failed (ESOMETHING), will be ignored" and return > 0 to suppress the generic message. You almost caught me out. I was going to say, this changes the behaviour of the return code. But It's intentional ;-) It's still a bit concerning, /not/ returning an error on an error - but as it's not going to prevent removal, and it won't add further (helpful) diagnosticts), maybe it makes sense. My only concern would be if it might be better to keep the return code, so that the core frameworks know about the issue at least. The error message is better IMO though so : Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/media/tuners/tua9001.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/media/tuners/tua9001.c b/drivers/media/tuners/tua9001.c > index 5e3625e75620..af7d5ea1f77e 100644 > --- a/drivers/media/tuners/tua9001.c > +++ b/drivers/media/tuners/tua9001.c > @@ -240,14 +240,10 @@ static int tua9001_remove(struct i2c_client *client) > DVB_FRONTEND_COMPONENT_TUNER, > TUA9001_CMD_CEN, 0); > if (ret) > - goto err_kfree; > + dev_err(&client->dev, "Tuner disable failed (%pe)\n", ERR_PTR(ret)); > } > kfree(dev); > return 0; > -err_kfree: > - kfree(dev); > - dev_dbg(&client->dev, "failed=%d\n", ret); > - return ret; > } > > static const struct i2c_device_id tua9001_id_table[] = { > -- > 2.30.2 >
On Mon, Nov 01, 2021 at 02:22:33PM +0000, Kieran Bingham wrote: > Quoting Uwe Kleine-König (2021-10-26 20:40:10) > > If disabling the hardware fails the driver propagates the error code to > > the i2c core. However this only results in a generic error message; the > > device still disappears. > > > > So instead emit a message that better describes the actual problem than > > the i2c core's "remove failed (ESOMETHING), will be ignored" and return > > 0 to suppress the generic message. > > You almost caught me out. I was going to say, this changes the > behaviour of the return code. But It's intentional ;-) > > It's still a bit concerning, /not/ returning an error on an error - but > as it's not going to prevent removal, and it won't add further (helpful) > diagnosticts), maybe it makes sense. > > My only concern would be if it might be better to keep the return code, > so that the core frameworks know about the issue at least. The long term goal is to make the remove callback return void. For that change I want all implementations to remove 0 to make the change easy to review. Best regards and thanks for your thoughts, Uwe
Quoting Uwe Kleine-König (2021-11-01 15:02:09) > On Mon, Nov 01, 2021 at 02:22:33PM +0000, Kieran Bingham wrote: > > Quoting Uwe Kleine-K�nig (2021-10-26 20:40:10) > > > If disabling the hardware fails the driver propagates the error code to > > > the i2c core. However this only results in a generic error message; the > > > device still disappears. > > > > > > So instead emit a message that better describes the actual problem than > > > the i2c core's "remove failed (ESOMETHING), will be ignored" and return > > > 0 to suppress the generic message. > > > > You almost caught me out. I was going to say, this changes the > > behaviour of the return code. But It's intentional ;-) > > > > It's still a bit concerning, /not/ returning an error on an error - but > > as it's not going to prevent removal, and it won't add further (helpful) > > diagnosticts), maybe it makes sense. > > > > My only concern would be if it might be better to keep the return code, > > so that the core frameworks know about the issue at least. > > The long term goal is to make the remove callback return void. For that > change I want all implementations to remove 0 to make the change easy to > review. Thanks for the update, that makes it much clearer indeed. And will make it clear that there's no use for a return code. -- Kieran > Best regards and thanks for your thoughts, > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-K�nig | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/media/tuners/tua9001.c b/drivers/media/tuners/tua9001.c index 5e3625e75620..af7d5ea1f77e 100644 --- a/drivers/media/tuners/tua9001.c +++ b/drivers/media/tuners/tua9001.c @@ -240,14 +240,10 @@ static int tua9001_remove(struct i2c_client *client) DVB_FRONTEND_COMPONENT_TUNER, TUA9001_CMD_CEN, 0); if (ret) - goto err_kfree; + dev_err(&client->dev, "Tuner disable failed (%pe)\n", ERR_PTR(ret)); } kfree(dev); return 0; -err_kfree: - kfree(dev); - dev_dbg(&client->dev, "failed=%d\n", ret); - return ret; } static const struct i2c_device_id tua9001_id_table[] = {
If disabling the hardware fails the driver propagates the error code to the i2c core. However this only results in a generic error message; the device still disappears. So instead emit a message that better describes the actual problem than the i2c core's "remove failed (ESOMETHING), will be ignored" and return 0 to suppress the generic message. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/media/tuners/tua9001.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)