| Message ID | c4d2e584-39ca-6e30-43ee-56088905149e@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 18, 2017 at 03:57:21PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 18 Sep 2017 13:50:45 +0200 > > Adjust jump targets so that a bit of exception handling can be better > reused at the end of this function. > > This refactoring might fix also an error situation where the > function "i2c_unregister_device" was not called after a software failure > was noticed by the data member "hdl.error". > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/media/usb/go7007/s2250-board.c | 37 +++++++++++++++++----------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/media/usb/go7007/s2250-board.c b/drivers/media/usb/go7007/s2250-board.c > index 1fd4c09dd516..1bd9a7f2e7a3 100644 > --- a/drivers/media/usb/go7007/s2250-board.c > +++ b/drivers/media/usb/go7007/s2250-board.c > @@ -510,6 +510,7 @@ static int s2250_probe(struct i2c_client *client, > u8 *data; > struct go7007 *go = i2c_get_adapdata(adapter); > struct go7007_usb *usb = go->hpi_context; > + int code; It should be called "int rc" to match the rest of the driver. "ret" or "err" would also have been acceptable. > > audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1); > if (!audio) > @@ -517,8 +518,8 @@ static int s2250_probe(struct i2c_client *client, > > state = kzalloc(sizeof(*state), GFP_KERNEL); > if (!state) { > - i2c_unregister_device(audio); > - return -ENOMEM; > + code = -ENOMEM; > + goto unregister_device; > } > > sd = &state->sd; > @@ -538,11 +539,8 @@ static int s2250_probe(struct i2c_client *client, > V4L2_CID_HUE, -512, 511, 1, 0); > sd->ctrl_handler = &state->hdl; > if (state->hdl.error) { > - int err = state->hdl.error; > - > - v4l2_ctrl_handler_free(&state->hdl); > - kfree(state); > - return err; > + code = state->hdl.error; > + goto free_handler; > } > > state->std = V4L2_STD_NTSC; > @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client, > /* initialize the audio */ > if (write_regs(audio, aud_regs) < 0) { > dev_err(&client->dev, "error initializing audio\n"); > - goto fail; > + goto e_io; Preserve the error code. regards, dan carpenter
>> @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client, >> /* initialize the audio */ >> if (write_regs(audio, aud_regs) < 0) { >> dev_err(&client->dev, "error initializing audio\n"); >> - goto fail; >> + goto e_io; > > Preserve the error code. Do you suggest then to adjust the implementation of the function "write_regs" so that a more meaningful value would be used instead of the failure indication "-1"? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/go7007/s2250-board.c?h=v4.14-rc1#n302 http://elixir.free-electrons.com/linux/v4.14-rc1/source/drivers/media/usb/go7007/s2250-board.c#L298 Regards, Markus
On Wed, Sep 20, 2017 at 09:09:16AM +0200, SF Markus Elfring wrote: > >> @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client, > >> /* initialize the audio */ > >> if (write_regs(audio, aud_regs) < 0) { > >> dev_err(&client->dev, "error initializing audio\n"); > >> - goto fail; > >> + goto e_io; > > > > Preserve the error code. > > Do you suggest then to adjust the implementation of the function "write_regs" > so that a more meaningful value would be used instead of the failure indication "-1"? > If you want to, yeah, that would be good. regards, dan carpenter
diff --git a/drivers/media/usb/go7007/s2250-board.c b/drivers/media/usb/go7007/s2250-board.c index 1fd4c09dd516..1bd9a7f2e7a3 100644 --- a/drivers/media/usb/go7007/s2250-board.c +++ b/drivers/media/usb/go7007/s2250-board.c @@ -510,6 +510,7 @@ static int s2250_probe(struct i2c_client *client, u8 *data; struct go7007 *go = i2c_get_adapdata(adapter); struct go7007_usb *usb = go->hpi_context; + int code; audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1); if (!audio) @@ -517,8 +518,8 @@ static int s2250_probe(struct i2c_client *client, state = kzalloc(sizeof(*state), GFP_KERNEL); if (!state) { - i2c_unregister_device(audio); - return -ENOMEM; + code = -ENOMEM; + goto unregister_device; } sd = &state->sd; @@ -538,11 +539,8 @@ static int s2250_probe(struct i2c_client *client, V4L2_CID_HUE, -512, 511, 1, 0); sd->ctrl_handler = &state->hdl; if (state->hdl.error) { - int err = state->hdl.error; - - v4l2_ctrl_handler_free(&state->hdl); - kfree(state); - return err; + code = state->hdl.error; + goto free_handler; } state->std = V4L2_STD_NTSC; @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client, /* initialize the audio */ if (write_regs(audio, aud_regs) < 0) { dev_err(&client->dev, "error initializing audio\n"); - goto fail; + goto e_io; } - if (write_regs(client, vid_regs) < 0) { - dev_err(&client->dev, "error initializing decoder\n"); - goto fail; - } - if (write_regs_fp(client, vid_regs_fp) < 0) { - dev_err(&client->dev, "error initializing decoder\n"); - goto fail; - } + if (write_regs(client, vid_regs) < 0 || + write_regs_fp(client, vid_regs_fp) < 0) + goto report_write_failure; + /* set default channel */ /* composite */ write_reg_fp(client, 0x20, 0x020 | 1); @@ -602,11 +596,16 @@ static int s2250_probe(struct i2c_client *client, v4l2_info(sd, "initialized successfully\n"); return 0; -fail: - i2c_unregister_device(audio); +report_write_failure: + dev_err(&client->dev, "error initializing decoder\n"); +e_io: + code = -EIO; +free_handler: v4l2_ctrl_handler_free(&state->hdl); kfree(state); - return -EIO; +unregister_device: + i2c_unregister_device(audio); + return code; } static int s2250_remove(struct i2c_client *client)