| Message ID | 20171219205957.10933-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client) > if (ret) > gc2235_remove(client); This error handling is probably wrong... > > - if (ACPI_HANDLE(&client->dev)) > - ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); > + return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); In the end this should look something like: ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); if (ret) goto err_free_something; return 0; > > - return ret; > out_free: > v4l2_device_unregister_subdev(&dev->sd); > kfree(dev); regards, dan carpenter
On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) > dev_err(&client->dev, "gpio request/direction_output fail"); > goto fail2; > } > - if (ACPI_HANDLE(&client->dev)) > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > - return 0; > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > fail2: > media_entity_cleanup(&flash->sd.entity); > v4l2_ctrl_handler_free(&flash->ctrl_handler); Actually every place where we directly return a function call is wrong and needs error handling added. I've been meaning to write a Smatch check for this because it's a common anti-pattern we don't check the last function call for errors. Someone could probably do the same in Coccinelle if they want. regards, dan carpenter
On Wed, Dec 20, 2017 at 6:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: >> @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client) >> if (ret) >> gc2235_remove(client); > > This error handling is probably wrong... > Thanks for pointing to this, but I'm not going to fix this by the following reasons: 1. I admit the driver's code is ugly 2. It's staging code 3. My patch does not touch those lines 4. My purpose is to get it working first. Feel free to send a followup with a good clean up which I agree with. >> >> - if (ACPI_HANDLE(&client->dev)) >> - ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); >> + return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); > > In the end this should look something like: > > ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); > if (ret) > goto err_free_something; > > return 0; > >> >> - return ret; >> out_free: >> v4l2_device_unregister_subdev(&dev->sd); >> kfree(dev); > > regards, > dan carpenter >
On Wed, 20 Dec 2017, Dan Carpenter wrote: > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) > > dev_err(&client->dev, "gpio request/direction_output fail"); > > goto fail2; > > } > > - if (ACPI_HANDLE(&client->dev)) > > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > - return 0; > > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > fail2: > > media_entity_cleanup(&flash->sd.entity); > > v4l2_ctrl_handler_free(&flash->ctrl_handler); > > Actually every place where we directly return a function call is wrong > and needs error handling added. I've been meaning to write a Smatch > check for this because it's a common anti-pattern we don't check the > last function call for errors. > > Someone could probably do the same in Coccinelle if they want. I'm not sure what you are suggesting. Is every case of return f(...); for any f wrong? Or is it a particular function that is of concern? Or would it be that every function call that has error handling somewhere should have error handling everywhere? Or is it related to what seems to be the problem in the above code that err is initialized but nothing happens to it? julia
Hi Andy and Dan, On Wed, Dec 20, 2017 at 12:24:36PM +0200, Andy Shevchenko wrote: > On Wed, Dec 20, 2017 at 6:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > >> @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client) > >> if (ret) > >> gc2235_remove(client); > > > > This error handling is probably wrong... > > > > Thanks for pointing to this, but I'm not going to fix this by the > following reasons: > 1. I admit the driver's code is ugly > 2. It's staging code > 3. My patch does not touch those lines > 4. My purpose is to get it working first. > > Feel free to send a followup with a good clean up which I agree with. Yeah, there's a lot of ugly stuff in this driver... I understand Andy's patches address problems with functionality, let's make error handling fixes separately. So I'm applying these now. Thanks!
On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote: > > > On Wed, 20 Dec 2017, Dan Carpenter wrote: > > > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) > > > dev_err(&client->dev, "gpio request/direction_output fail"); > > > goto fail2; > > > } > > > - if (ACPI_HANDLE(&client->dev)) > > > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > > - return 0; > > > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > > fail2: > > > media_entity_cleanup(&flash->sd.entity); > > > v4l2_ctrl_handler_free(&flash->ctrl_handler); > > > > Actually every place where we directly return a function call is wrong > > and needs error handling added. I've been meaning to write a Smatch > > check for this because it's a common anti-pattern we don't check the > > last function call for errors. > > > > Someone could probably do the same in Coccinelle if they want. > > I'm not sure what you are suggesting. Is every case of return f(...); > for any f wrong? Or is it a particular function that is of concern? Or > would it be that every function call that has error handling somewhere > should have error handling everywhere? Or is it related to what seems to > be the problem in the above code that err is initialized but nothing > happens to it? > I was just thinking that it's a common pattern to treat the last function call differently and one mistake I often see looks like this: ret = frob(); if (ret) { cleanup(); return ret; } return another_function(); No error handling for the last function call. regards, dan carpenter
On Tue, 2 Jan 2018, Dan Carpenter wrote: > On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote: > > > > > > On Wed, 20 Dec 2017, Dan Carpenter wrote: > > > > > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > > > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) > > > > dev_err(&client->dev, "gpio request/direction_output fail"); > > > > goto fail2; > > > > } > > > > - if (ACPI_HANDLE(&client->dev)) > > > > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > > > - return 0; > > > > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > > > > fail2: > > > > media_entity_cleanup(&flash->sd.entity); > > > > v4l2_ctrl_handler_free(&flash->ctrl_handler); > > > > > > Actually every place where we directly return a function call is wrong > > > and needs error handling added. I've been meaning to write a Smatch > > > check for this because it's a common anti-pattern we don't check the > > > last function call for errors. > > > > > > Someone could probably do the same in Coccinelle if they want. > > > > I'm not sure what you are suggesting. Is every case of return f(...); > > for any f wrong? Or is it a particular function that is of concern? Or > > would it be that every function call that has error handling somewhere > > should have error handling everywhere? Or is it related to what seems to > > be the problem in the above code that err is initialized but nothing > > happens to it? > > > > I was just thinking that it's a common pattern to treat the last > function call differently and one mistake I often see looks like this: > > ret = frob(); > if (ret) { > cleanup(); > return ret; > } > > return another_function(); > > No error handling for the last function call. OK, I see. When there was error handling code along the way, a direct return of a function that could fail needs error handling code too. Thanks for the clarification, julia
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c index e70d8afcc229..61b7598469eb 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c @@ -1370,13 +1370,9 @@ static int gc0310_probe(struct i2c_client *client) dev->fmt_idx = 0; v4l2_i2c_subdev_init(&(dev->sd), client, &gc0310_ops); - if (ACPI_COMPANION(&client->dev)) - pdata = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_8, - atomisp_bayer_order_grbg); - else - pdata = client->dev.platform_data; - + pdata = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_8, + atomisp_bayer_order_grbg); if (!pdata) { ret = -EINVAL; goto out_free; diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c index 85da5fe24033..d8de46da64ae 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c @@ -1108,9 +1108,7 @@ static int gc2235_probe(struct i2c_client *client) dev->fmt_idx = 0; v4l2_i2c_subdev_init(&(dev->sd), client, &gc2235_ops); - gcpdev = client->dev.platform_data; - if (ACPI_COMPANION(&client->dev)) - gcpdev = gmin_camera_platform_data(&dev->sd, + gcpdev = gmin_camera_platform_data(&dev->sd, ATOMISP_INPUT_FORMAT_RAW_10, atomisp_bayer_order_grbg); @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client) if (ret) gc2235_remove(client); - if (ACPI_HANDLE(&client->dev)) - ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); + return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); - return ret; out_free: v4l2_device_unregister_subdev(&dev->sd); kfree(dev); diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c index 974b6ff50c7a..7098bf317f16 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c @@ -824,22 +824,15 @@ static void *lm3554_platform_data_func(struct i2c_client *client) { static struct lm3554_platform_data platform_data; - if (ACPI_COMPANION(&client->dev)) { - platform_data.gpio_reset = - desc_to_gpio(gpiod_get_index(&(client->dev), + platform_data.gpio_reset = + desc_to_gpio(gpiod_get_index(&client->dev, NULL, 2, GPIOD_OUT_LOW)); - platform_data.gpio_strobe = - desc_to_gpio(gpiod_get_index(&(client->dev), + platform_data.gpio_strobe = + desc_to_gpio(gpiod_get_index(&client->dev, NULL, 0, GPIOD_OUT_LOW)); - platform_data.gpio_torch = - desc_to_gpio(gpiod_get_index(&(client->dev), + platform_data.gpio_torch = + desc_to_gpio(gpiod_get_index(&client->dev, NULL, 1, GPIOD_OUT_LOW)); - } else { - platform_data.gpio_reset = -1; - platform_data.gpio_strobe = -1; - platform_data.gpio_torch = -1; - } - dev_info(&client->dev, "camera pdata: lm3554: reset: %d strobe %d torch %d\n", platform_data.gpio_reset, platform_data.gpio_strobe, platform_data.gpio_torch); @@ -868,10 +861,7 @@ static int lm3554_probe(struct i2c_client *client) if (!flash) return -ENOMEM; - flash->pdata = client->dev.platform_data; - - if (!flash->pdata || ACPI_COMPANION(&client->dev)) - flash->pdata = lm3554_platform_data_func(client); + flash->pdata = lm3554_platform_data_func(client); v4l2_i2c_subdev_init(&flash->sd, client, &lm3554_ops); flash->sd.internal_ops = &lm3554_internal_ops; @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) dev_err(&client->dev, "gpio request/direction_output fail"); goto fail2; } - if (ACPI_HANDLE(&client->dev)) - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); - return 0; + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); fail2: media_entity_cleanup(&flash->sd.entity); v4l2_ctrl_handler_free(&flash->ctrl_handler); diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c index 55882bea2049..df253a557c76 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c @@ -1844,11 +1844,9 @@ static int mt9m114_probe(struct i2c_client *client) return -ENOMEM; v4l2_i2c_subdev_init(&dev->sd, client, &mt9m114_ops); - pdata = client->dev.platform_data; - if (ACPI_COMPANION(&client->dev)) - pdata = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_10, - atomisp_bayer_order_grbg); + pdata = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_10, + atomisp_bayer_order_grbg); if (pdata) ret = mt9m114_s_config(&dev->sd, client->irq, pdata); if (!pdata || ret) { diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c index cd67d38f183a..84f8d33ce2d1 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c @@ -1447,13 +1447,9 @@ static int ov2680_probe(struct i2c_client *client) dev->fmt_idx = 0; v4l2_i2c_subdev_init(&(dev->sd), client, &ov2680_ops); - if (ACPI_COMPANION(&client->dev)) - pdata = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_10, - atomisp_bayer_order_bggr); - else - pdata = client->dev.platform_data; - + pdata = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_10, + atomisp_bayer_order_bggr); if (!pdata) { ret = -EINVAL; goto out_free; diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c index 4df7eba8d375..2b6ae0faf972 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c @@ -1259,7 +1259,6 @@ static int ov2722_probe(struct i2c_client *client) struct ov2722_device *dev; void *ovpdev; int ret; - struct acpi_device *adev; dev = kzalloc(sizeof(*dev), GFP_KERNEL); if (!dev) @@ -1270,14 +1269,9 @@ static int ov2722_probe(struct i2c_client *client) dev->fmt_idx = 0; v4l2_i2c_subdev_init(&(dev->sd), client, &ov2722_ops); - ovpdev = client->dev.platform_data; - adev = ACPI_COMPANION(&client->dev); - if (adev) { - adev->power.flags.power_resources = 0; - ovpdev = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_10, - atomisp_bayer_order_grbg); - } + ovpdev = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_10, + atomisp_bayer_order_grbg); ret = ov2722_s_config(&dev->sd, client->irq, ovpdev); if (ret) @@ -1296,10 +1290,7 @@ static int ov2722_probe(struct i2c_client *client) if (ret) ov2722_remove(client); - if (ACPI_HANDLE(&client->dev)) - ret = atomisp_register_i2c_module(&dev->sd, ovpdev, RAW_CAMERA); - - return ret; + return atomisp_register_i2c_module(&dev->sd, ovpdev, RAW_CAMERA); out_ctrl_handler_free: v4l2_ctrl_handler_free(&dev->ctrl_handler); diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c index 3e7c3851280f..2a680eae20d3 100644 --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c @@ -1928,7 +1928,6 @@ static int ov5693_probe(struct i2c_client *client) int i2c; int ret = 0; void *pdata = client->dev.platform_data; - struct acpi_device *adev; unsigned int i; /* Firmware workaround: Some modules use a "secondary default" @@ -1952,14 +1951,9 @@ static int ov5693_probe(struct i2c_client *client) dev->fmt_idx = 0; v4l2_i2c_subdev_init(&(dev->sd), client, &ov5693_ops); - adev = ACPI_COMPANION(&client->dev); - if (adev) { - adev->power.flags.power_resources = 0; - pdata = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_10, - atomisp_bayer_order_bggr); - } - + pdata = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_10, + atomisp_bayer_order_bggr); if (!pdata) goto out_free; diff --git a/drivers/staging/media/atomisp/i2c/ov8858.c b/drivers/staging/media/atomisp/i2c/ov8858.c index ba147ac2e36f..3cf8c710ac65 100644 --- a/drivers/staging/media/atomisp/i2c/ov8858.c +++ b/drivers/staging/media/atomisp/i2c/ov8858.c @@ -2077,29 +2077,28 @@ static int ov8858_probe(struct i2c_client *client) v4l2_i2c_subdev_init(&(dev->sd), client, &ov8858_ops); - if (ACPI_COMPANION(&client->dev)) { - pdata = gmin_camera_platform_data(&dev->sd, - ATOMISP_INPUT_FORMAT_RAW_10, - atomisp_bayer_order_bggr); - if (!pdata) { - dev_err(&client->dev, - "%s: failed to get acpi platform data\n", - __func__); - goto out_free; - } - ret = ov8858_s_config(&dev->sd, client->irq, pdata); - if (ret) { - dev_err(&client->dev, - "%s: failed to set config\n", __func__); - goto out_free; - } - ret = atomisp_register_i2c_module(&dev->sd, pdata, RAW_CAMERA); - if (ret) { - dev_err(&client->dev, - "%s: failed to register subdev\n", __func__); - goto out_free; - } + pdata = gmin_camera_platform_data(&dev->sd, + ATOMISP_INPUT_FORMAT_RAW_10, + atomisp_bayer_order_bggr); + if (!pdata) { + dev_err(&client->dev, + "%s: failed to get acpi platform data\n", + __func__); + goto out_free; + } + ret = ov8858_s_config(&dev->sd, client->irq, pdata); + if (ret) { + dev_err(&client->dev, + "%s: failed to set config\n", __func__); + goto out_free; } + ret = atomisp_register_i2c_module(&dev->sd, pdata, RAW_CAMERA); + if (ret) { + dev_err(&client->dev, + "%s: failed to register subdev\n", __func__); + goto out_free; + } + /* * sd->name is updated with sensor driver name by the v4l2. * change it to sensor name in this case. diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c index 8fb5147531a5..8dcec0e780a1 100644 --- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c @@ -114,7 +114,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev, struct i2c_board_info *bi; struct gmin_subdev *gs; struct i2c_client *client = v4l2_get_subdevdata(subdev); - struct acpi_device *adev; + struct acpi_device *adev = ACPI_COMPANION(&client->dev); dev_info(&client->dev, "register atomisp i2c module type %d\n", type); @@ -124,9 +124,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev, * tickled during suspend/resume. This has caused power and * performance issues on multiple devices. */ - adev = ACPI_COMPANION(&client->dev); - if (adev) - adev->power.flags.power_resources = 0; + adev->power.flags.power_resources = 0; for (i = 0; i < MAX_SUBDEVS; i++) if (!pdata.subdevs[i].type)
Since all drivers are solely requiring ACPI enumeration, there is no need to additionally check for legacy platform data or ACPI handle. Remove leftovers from the sensors and platform code. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 10 ++--- drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 8 +--- drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 28 ++++---------- .../staging/media/atomisp/i2c/atomisp-mt9m114.c | 8 ++-- drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 10 ++--- drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 17 ++------- .../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 12 ++---- drivers/staging/media/atomisp/i2c/ov8858.c | 43 +++++++++++----------- .../platform/intel-mid/atomisp_gmin_platform.c | 6 +-- 9 files changed, 49 insertions(+), 93 deletions(-)