| Message ID | 20190722172632.4402-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ir-kbd-i2c: fix potential OOPS & minor cleanup | expand |
On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote: > i2c_new_dummy() can fail returning a NULL pointer. The code does not > bail out in this case and the returned pointer is blindly used. I don't see how. The existing code tries to set up the tx part; if i2c_new_dummy() return NULL then the rcdev is registered without tx, and tx_c is never used. > Convert > to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail > out when failing the validity check. Possibly I was being overly cautious with not bailing out if tx can't be registered; moving to devm is probably a good idea. However the commit message is misleading, because the existing code has no NULL pointer access. Sean > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/media/i2c/ir-kbd-i2c.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c > index 876d7587a1da..f46717052efc 100644 > --- a/drivers/media/i2c/ir-kbd-i2c.c > +++ b/drivers/media/i2c/ir-kbd-i2c.c > @@ -885,9 +885,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id) > INIT_DELAYED_WORK(&ir->work, ir_work); > > if (probe_tx) { > - ir->tx_c = i2c_new_dummy(client->adapter, 0x70); > - if (!ir->tx_c) { > + ir->tx_c = devm_i2c_new_dummy_device(&client->dev, > + client->adapter, 0x70); > + if (IS_ERR(ir->tx_c)) { > dev_err(&client->dev, "failed to setup tx i2c address"); > + err = PTR_ERR(ir->tx_c); > + goto err_out_free; > } else if (!zilog_init(ir)) { > ir->carrier = 38000; > ir->duty_cycle = 40; > @@ -904,9 +907,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id) > return 0; > > err_out_free: > - if (ir->tx_c) > - i2c_unregister_device(ir->tx_c); > - > /* Only frees rc if it were allocated internally */ > rc_free_device(rc); > return err; > @@ -919,9 +919,6 @@ static int ir_remove(struct i2c_client *client) > /* kill outstanding polls */ > cancel_delayed_work_sync(&ir->work); > > - if (ir->tx_c) > - i2c_unregister_device(ir->tx_c); > - > /* unregister device */ > rc_unregister_device(ir->rc); > > -- > 2.20.1
Hi Sean, thanks for the review! On Thu, Jul 25, 2019 at 06:12:02AM +0100, Sean Young wrote: > On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote: > > i2c_new_dummy() can fail returning a NULL pointer. The code does not > > bail out in this case and the returned pointer is blindly used. > > I don't see how. The existing code tries to set up the tx part; if > i2c_new_dummy() return NULL then the rcdev is registered without tx, > and tx_c is never used. Yes, you are totally right. I missed that the send_block function is also only called iff zilog_init succeeded. Thanks for the heads up and sorry for the noise. > > > Convert > > to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail > > out when failing the validity check. > > Possibly I was being overly cautious with not bailing out if tx can't > be registered; moving to devm is probably a good idea. However the > commit message is misleading, because the existing code has no > NULL pointer access. Yep, I will resend with a proper commit message. Technically, there is no need to bail out anymore because there is no NULL pointer access. My tendency is now to not bail out and keep the old behaviour (registering without tx). What do you think? Regards, Wolfram
Hi Wolfram, On Thu, Jul 25, 2019 at 09:55:38AM +0200, Wolfram Sang wrote: > Hi Sean, > > thanks for the review! > > On Thu, Jul 25, 2019 at 06:12:02AM +0100, Sean Young wrote: > > On Mon, Jul 22, 2019 at 07:26:31PM +0200, Wolfram Sang wrote: > > > i2c_new_dummy() can fail returning a NULL pointer. The code does not > > > bail out in this case and the returned pointer is blindly used. > > > > I don't see how. The existing code tries to set up the tx part; if > > i2c_new_dummy() return NULL then the rcdev is registered without tx, > > and tx_c is never used. > > Yes, you are totally right. I missed that the send_block function is > also only called iff zilog_init succeeded. Thanks for the heads up and > sorry for the noise. Not at all, thank you for the patch. > > > Convert > > > to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail > > > out when failing the validity check. > > > > Possibly I was being overly cautious with not bailing out if tx can't > > be registered; moving to devm is probably a good idea. However the > > commit message is misleading, because the existing code has no > > NULL pointer access. > > Yep, I will resend with a proper commit message. Technically, there is > no need to bail out anymore because there is no NULL pointer access. My > tendency is now to not bail out and keep the old behaviour (registering > without tx). What do you think? Since I write this code I've got pretty much every model with this zilog transmitter/receiver, and they all work fine, including different firmware versions. If there is a problem it would be nice to hear about it, and not silently swallow the error. I think. Thanks, Sean
diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c index 876d7587a1da..f46717052efc 100644 --- a/drivers/media/i2c/ir-kbd-i2c.c +++ b/drivers/media/i2c/ir-kbd-i2c.c @@ -885,9 +885,12 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id) INIT_DELAYED_WORK(&ir->work, ir_work); if (probe_tx) { - ir->tx_c = i2c_new_dummy(client->adapter, 0x70); - if (!ir->tx_c) { + ir->tx_c = devm_i2c_new_dummy_device(&client->dev, + client->adapter, 0x70); + if (IS_ERR(ir->tx_c)) { dev_err(&client->dev, "failed to setup tx i2c address"); + err = PTR_ERR(ir->tx_c); + goto err_out_free; } else if (!zilog_init(ir)) { ir->carrier = 38000; ir->duty_cycle = 40; @@ -904,9 +907,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id) return 0; err_out_free: - if (ir->tx_c) - i2c_unregister_device(ir->tx_c); - /* Only frees rc if it were allocated internally */ rc_free_device(rc); return err; @@ -919,9 +919,6 @@ static int ir_remove(struct i2c_client *client) /* kill outstanding polls */ cancel_delayed_work_sync(&ir->work); - if (ir->tx_c) - i2c_unregister_device(ir->tx_c); - /* unregister device */ rc_unregister_device(ir->rc);
i2c_new_dummy() can fail returning a NULL pointer. The code does not bail out in this case and the returned pointer is blindly used. Convert to devm_i2c_new_dummy_device() which returns an ERR_PTR and also bail out when failing the validity check. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/media/i2c/ir-kbd-i2c.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)