* Re: media: i2c: add OV02A10 image sensor driver [not found] <9af089ea-2532-68ac-5d22-97a669ccec91@canonical.com> @ 2020-12-03 18:10 ` Andy Shevchenko 2020-12-03 18:24 ` Colin Ian King 2020-12-04 2:46 ` Dongchun Zhu 0 siblings, 2 replies; 6+ messages in thread From: Andy Shevchenko @ 2020-12-03 18:10 UTC (permalink / raw) To: Colin Ian King Cc: Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Sakari Ailus, Matthias Brugger, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > Static analysis on linux-next with Coverity has detected an issue with > the following commit: If you want to fix it properly, see my comments below... > 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > 530 { > 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); > 532 struct i2c_client *client = > v4l2_get_subdevdata(&ov02a10->subdev); > > 1. var_decl: Declaring variable ret without initializer. > > 533 int ret; > 534 > 535 mutex_lock(&ov02a10->mutex); > 536 > > 2. Condition ov02a10->streaming == on, taking true branch. > > 537 if (ov02a10->streaming == on) > > 3. Jumping to label unlock_and_return. > > 538 goto unlock_and_return; > 539 > 540 if (on) { > 541 ret = pm_runtime_get_sync(&client->dev); > 542 if (ret < 0) { > 543 pm_runtime_put_noidle(&client->dev); > 544 goto unlock_and_return; Instead of two above: goto err_rpm_put; > 545 } > 546 > 547 ret = __ov02a10_start_stream(ov02a10); > 548 if (ret) { > 549 __ov02a10_stop_stream(ov02a10); > 550 ov02a10->streaming = !on; > 551 goto err_rpm_put; > 552 } > 553 } else { > 554 __ov02a10_stop_stream(ov02a10); > 555 pm_runtime_put(&client->dev); > 556 } > 557 > 558 ov02a10->streaming = on; (1) > 559 mutex_unlock(&ov02a10->mutex); > 560 > 561 return 0; > 562 > 563 err_rpm_put: > 564 pm_runtime_put(&client->dev); > 565 unlock_and_return: Should be moved to (1). > 566 mutex_unlock(&ov02a10->mutex); > 567 > > Uninitialized scalar variable (UNINIT) > 4. uninit_use: Using uninitialized value ret. > > 568 return ret; > 569 } > > Variable ret has not been initialized, so the error return value is a > garbage value. It should be initialized with some appropriate negative > error code, or ret could be removed and the return should return a > literal value of a error code. > > I was unsure what value is appropriate to fix this, so instead I'm > reporting this issue. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: media: i2c: add OV02A10 image sensor driver 2020-12-03 18:10 ` media: i2c: add OV02A10 image sensor driver Andy Shevchenko @ 2020-12-03 18:24 ` Colin Ian King 2020-12-03 18:30 ` Andy Shevchenko 2020-12-04 2:46 ` Dongchun Zhu 1 sibling, 1 reply; 6+ messages in thread From: Colin Ian King @ 2020-12-03 18:24 UTC (permalink / raw) To: Andy Shevchenko Cc: Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Sakari Ailus, Matthias Brugger, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media On 03/12/2020 18:10, Andy Shevchenko wrote: > On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > >> Static analysis on linux-next with Coverity has detected an issue with >> the following commit: > > If you want to fix it properly, see my comments below... > >> 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) >> 530 { >> 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); >> 532 struct i2c_client *client = >> v4l2_get_subdevdata(&ov02a10->subdev); >> >> 1. var_decl: Declaring variable ret without initializer. >> >> 533 int ret; >> 534 >> 535 mutex_lock(&ov02a10->mutex); >> 536 >> >> 2. Condition ov02a10->streaming == on, taking true branch. >> >> 537 if (ov02a10->streaming == on) >> >> 3. Jumping to label unlock_and_return. >> >> 538 goto unlock_and_return; >> 539 >> 540 if (on) { >> 541 ret = pm_runtime_get_sync(&client->dev); >> 542 if (ret < 0) { > >> 543 pm_runtime_put_noidle(&client->dev); >> 544 goto unlock_and_return; > > Instead of two above: > goto err_rpm_put; > >> 545 } >> 546 >> 547 ret = __ov02a10_start_stream(ov02a10); >> 548 if (ret) { >> 549 __ov02a10_stop_stream(ov02a10); >> 550 ov02a10->streaming = !on; >> 551 goto err_rpm_put; >> 552 } >> 553 } else { >> 554 __ov02a10_stop_stream(ov02a10); >> 555 pm_runtime_put(&client->dev); >> 556 } >> 557 >> 558 ov02a10->streaming = on; > > (1) > >> 559 mutex_unlock(&ov02a10->mutex); >> 560 >> 561 return 0; >> 562 >> 563 err_rpm_put: >> 564 pm_runtime_put(&client->dev); > >> 565 unlock_and_return: > > Should be moved to (1). > >> 566 mutex_unlock(&ov02a10->mutex); >> 567 >> >> Uninitialized scalar variable (UNINIT) >> 4. uninit_use: Using uninitialized value ret. >> >> 568 return ret; >> 569 } >> >> Variable ret has not been initialized, so the error return value is a >> garbage value. It should be initialized with some appropriate negative >> error code, or ret could be removed and the return should return a >> literal value of a error code. >> >> I was unsure what value is appropriate to fix this, so instead I'm >> reporting this issue. > Not sure I fully understand how that fixes it. Given that ret is currently not initialized when we hit: if (ov02a10->streaming == on) goto unlock_and_return; either we initialize ret to 0 at the start of the function, or do: if (ov02a10->streaming == on) { ret = 0; goto unlock_and_return; } (assuming the intention is to return zero for this specific case). Colin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: media: i2c: add OV02A10 image sensor driver 2020-12-03 18:24 ` Colin Ian King @ 2020-12-03 18:30 ` Andy Shevchenko 2020-12-04 11:45 ` Sakari Ailus 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2020-12-03 18:30 UTC (permalink / raw) To: Colin Ian King Cc: Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Sakari Ailus, Matthias Brugger, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media On Thu, Dec 3, 2020 at 8:24 PM Colin Ian King <colin.king@canonical.com> wrote: > On 03/12/2020 18:10, Andy Shevchenko wrote: > > On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > > > >> Static analysis on linux-next with Coverity has detected an issue with > >> the following commit: > > > > If you want to fix it properly, see my comments below... > > > >> 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > >> 530 { > >> 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); > >> 532 struct i2c_client *client = > >> v4l2_get_subdevdata(&ov02a10->subdev); > >> > >> 1. var_decl: Declaring variable ret without initializer. > >> > >> 533 int ret; > >> 534 > >> 535 mutex_lock(&ov02a10->mutex); > >> 536 > >> > >> 2. Condition ov02a10->streaming == on, taking true branch. > >> > >> 537 if (ov02a10->streaming == on) > >> > >> 3. Jumping to label unlock_and_return. > >> > >> 538 goto unlock_and_return; > >> 539 > >> 540 if (on) { > >> 541 ret = pm_runtime_get_sync(&client->dev); > >> 542 if (ret < 0) { > > > >> 543 pm_runtime_put_noidle(&client->dev); > >> 544 goto unlock_and_return; > > > > Instead of two above: > > goto err_rpm_put; > > > >> 545 } > >> 546 > >> 547 ret = __ov02a10_start_stream(ov02a10); > >> 548 if (ret) { > >> 549 __ov02a10_stop_stream(ov02a10); > >> 550 ov02a10->streaming = !on; > >> 551 goto err_rpm_put; > >> 552 } > >> 553 } else { > >> 554 __ov02a10_stop_stream(ov02a10); > >> 555 pm_runtime_put(&client->dev); > >> 556 } > >> 557 > >> 558 ov02a10->streaming = on; > > > > (1) > > > >> 559 mutex_unlock(&ov02a10->mutex); > >> 560 > >> 561 return 0; > >> 562 > >> 563 err_rpm_put: > >> 564 pm_runtime_put(&client->dev); > > > >> 565 unlock_and_return: > > > > Should be moved to (1). > > > >> 566 mutex_unlock(&ov02a10->mutex); > >> 567 > >> > >> Uninitialized scalar variable (UNINIT) > >> 4. uninit_use: Using uninitialized value ret. > >> > >> 568 return ret; > >> 569 } > >> > >> Variable ret has not been initialized, so the error return value is a > >> garbage value. It should be initialized with some appropriate negative > >> error code, or ret could be removed and the return should return a > >> literal value of a error code. > >> > >> I was unsure what value is appropriate to fix this, so instead I'm > >> reporting this issue. > > > Not sure I fully understand how that fixes it. If you are not sure and have no means to test, then don't bother. This is not the priority driver anyway. > Given that ret is > currently not initialized when we hit: > > if (ov02a10->streaming == on) > goto unlock_and_return; > > either we initialize ret to 0 at the start of the function, or do: > > if (ov02a10->streaming == on) { > ret = 0; > goto unlock_and_return; > } > > (assuming the intention is to return zero for this specific case). No, please read my message again. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: media: i2c: add OV02A10 image sensor driver 2020-12-03 18:30 ` Andy Shevchenko @ 2020-12-04 11:45 ` Sakari Ailus 0 siblings, 0 replies; 6+ messages in thread From: Sakari Ailus @ 2020-12-04 11:45 UTC (permalink / raw) To: Andy Shevchenko Cc: Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Matthias Brugger, Colin Ian King, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media On Thu, Dec 03, 2020 at 08:30:03PM +0200, Andy Shevchenko wrote: > On Thu, Dec 3, 2020 at 8:24 PM Colin Ian King <colin.king@canonical.com> wrote: > > On 03/12/2020 18:10, Andy Shevchenko wrote: > > > On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > > > > > >> Static analysis on linux-next with Coverity has detected an issue with > > >> the following commit: > > > > > > If you want to fix it properly, see my comments below... > > > > > >> 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > > >> 530 { > > >> 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); > > >> 532 struct i2c_client *client = > > >> v4l2_get_subdevdata(&ov02a10->subdev); > > >> > > >> 1. var_decl: Declaring variable ret without initializer. > > >> > > >> 533 int ret; > > >> 534 > > >> 535 mutex_lock(&ov02a10->mutex); > > >> 536 > > >> > > >> 2. Condition ov02a10->streaming == on, taking true branch. > > >> > > >> 537 if (ov02a10->streaming == on) > > >> > > >> 3. Jumping to label unlock_and_return. > > >> > > >> 538 goto unlock_and_return; > > >> 539 > > >> 540 if (on) { > > >> 541 ret = pm_runtime_get_sync(&client->dev); > > >> 542 if (ret < 0) { > > > > > >> 543 pm_runtime_put_noidle(&client->dev); > > >> 544 goto unlock_and_return; > > > > > > Instead of two above: > > > goto err_rpm_put; > > > > > >> 545 } > > >> 546 > > >> 547 ret = __ov02a10_start_stream(ov02a10); > > >> 548 if (ret) { > > >> 549 __ov02a10_stop_stream(ov02a10); > > >> 550 ov02a10->streaming = !on; > > >> 551 goto err_rpm_put; > > >> 552 } > > >> 553 } else { > > >> 554 __ov02a10_stop_stream(ov02a10); > > >> 555 pm_runtime_put(&client->dev); > > >> 556 } > > >> 557 > > >> 558 ov02a10->streaming = on; > > > > > > (1) > > > > > >> 559 mutex_unlock(&ov02a10->mutex); > > >> 560 > > >> 561 return 0; > > >> 562 > > >> 563 err_rpm_put: > > >> 564 pm_runtime_put(&client->dev); > > > > > >> 565 unlock_and_return: > > > > > > Should be moved to (1). > > > > > >> 566 mutex_unlock(&ov02a10->mutex); > > >> 567 > > >> > > >> Uninitialized scalar variable (UNINIT) > > >> 4. uninit_use: Using uninitialized value ret. > > >> > > >> 568 return ret; > > >> 569 } > > >> > > >> Variable ret has not been initialized, so the error return value is a > > >> garbage value. It should be initialized with some appropriate negative > > >> error code, or ret could be removed and the return should return a > > >> literal value of a error code. > > >> > > >> I was unsure what value is appropriate to fix this, so instead I'm > > >> reporting this issue. > > > > > Not sure I fully understand how that fixes it. > > If you are not sure and have no means to test, then don't bother. This > is not the priority driver anyway. Arnd sent a patch to address this: <URL:https://patchwork.linuxtv.org/project/linux-media/patch/20201204082037.1658297-1-arnd@kernel.org/> -- Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: media: i2c: add OV02A10 image sensor driver 2020-12-03 18:10 ` media: i2c: add OV02A10 image sensor driver Andy Shevchenko 2020-12-03 18:24 ` Colin Ian King @ 2020-12-04 2:46 ` Dongchun Zhu 2020-12-04 6:23 ` Tomasz Figa 1 sibling, 1 reply; 6+ messages in thread From: Dongchun Zhu @ 2020-12-04 2:46 UTC (permalink / raw) To: Andy Shevchenko Cc: Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, tfiga, sj.huang, moderated list:ARM/Mediatek SoC support, dongchun.zhu, Sakari Ailus, Matthias Brugger, Colin Ian King, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media Hi Andy, On Thu, 2020-12-03 at 20:10 +0200, Andy Shevchenko wrote: > On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > > > Static analysis on linux-next with Coverity has detected an issue with > > the following commit: > > If you want to fix it properly, see my comments below... > > > 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > > 530 { > > 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); > > 532 struct i2c_client *client = > > v4l2_get_subdevdata(&ov02a10->subdev); > > > > 1. var_decl: Declaring variable ret without initializer. > > > > 533 int ret; > > 534 > > 535 mutex_lock(&ov02a10->mutex); > > 536 > > > > 2. Condition ov02a10->streaming == on, taking true branch. > > > > 537 if (ov02a10->streaming == on) > > > > 3. Jumping to label unlock_and_return. > > > > 538 goto unlock_and_return; > > 539 > > 540 if (on) { > > 541 ret = pm_runtime_get_sync(&client->dev); > > 542 if (ret < 0) { > > > 543 pm_runtime_put_noidle(&client->dev); > > 544 goto unlock_and_return; > > Instead of two above: From the document, pm_runtime_put_noidle is to decrease the runtime PM usage counter of a device unless it is 0 already; while pm_runtime_put would additionally run pm_request_idle to turn off the power if usage counter is zero. So here maybe we can really use pm_runtime_put instead of pm_runtime_put_noidle, although it seems that 'pm_runtime_get_sync' and 'pm_runtime_put_noidle' often appear in pairs. > goto err_rpm_put; > > > 545 } > > 546 > > 547 ret = __ov02a10_start_stream(ov02a10); > > 548 if (ret) { > > 549 __ov02a10_stop_stream(ov02a10); > > 550 ov02a10->streaming = !on; > > 551 goto err_rpm_put; > > 552 } > > 553 } else { > > 554 __ov02a10_stop_stream(ov02a10); > > 555 pm_runtime_put(&client->dev); > > 556 } > > 557 > > 558 ov02a10->streaming = on; > > (1) > > > 559 mutex_unlock(&ov02a10->mutex); > > 560 > > 561 return 0; > > 562 > > 563 err_rpm_put: > > 564 pm_runtime_put(&client->dev); > > > 565 unlock_and_return: > > Should be moved to (1). > > > 566 mutex_unlock(&ov02a10->mutex); > > 567 > > > > Uninitialized scalar variable (UNINIT) > > 4. uninit_use: Using uninitialized value ret. > > > > 568 return ret; > > 569 } > > > > Variable ret has not been initialized, so the error return value is a > > garbage value. It should be initialized with some appropriate negative > > error code, or ret could be removed and the return should return a > > literal value of a error code. > > > > I was unsure what value is appropriate to fix this, so instead I'm > > reporting this issue. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: media: i2c: add OV02A10 image sensor driver 2020-12-04 2:46 ` Dongchun Zhu @ 2020-12-04 6:23 ` Tomasz Figa 0 siblings, 0 replies; 6+ messages in thread From: Tomasz Figa @ 2020-12-04 6:23 UTC (permalink / raw) To: Dongchun Zhu Cc: Sj Huang, Mauro Carvalho Chehab, linux-kernel@vger.kernel.org, Andy Shevchenko, moderated list:ARM/Mediatek SoC support, Sakari Ailus, Matthias Brugger, Colin Ian King, Andy Shevchenko, linux-arm-kernel@lists.infradead.org, linux-media On Fri, Dec 4, 2020 at 11:47 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Andy, > > On Thu, 2020-12-03 at 20:10 +0200, Andy Shevchenko wrote: > > On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King <colin.king@canonical.com> wrote: > > > > > Static analysis on linux-next with Coverity has detected an issue with > > > the following commit: > > > > If you want to fix it properly, see my comments below... > > > > > 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > > > 530 { > > > 531 struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > 532 struct i2c_client *client = > > > v4l2_get_subdevdata(&ov02a10->subdev); > > > > > > 1. var_decl: Declaring variable ret without initializer. > > > > > > 533 int ret; > > > 534 > > > 535 mutex_lock(&ov02a10->mutex); > > > 536 > > > > > > 2. Condition ov02a10->streaming == on, taking true branch. > > > > > > 537 if (ov02a10->streaming == on) > > > > > > 3. Jumping to label unlock_and_return. > > > > > > 538 goto unlock_and_return; > > > 539 > > > 540 if (on) { > > > 541 ret = pm_runtime_get_sync(&client->dev); > > > 542 if (ret < 0) { > > > > > 543 pm_runtime_put_noidle(&client->dev); > > > 544 goto unlock_and_return; > > > > Instead of two above: > > From the document, pm_runtime_put_noidle is to decrease the runtime PM > usage counter of a device unless it is 0 already; while pm_runtime_put > would additionally run pm_request_idle to turn off the power if usage > counter is zero. > > So here maybe we can really use pm_runtime_put instead of > pm_runtime_put_noidle, although it seems that 'pm_runtime_get_sync' and > 'pm_runtime_put_noidle' often appear in pairs. > In an error state (e.g. if pm_runtime_get_sync() fails), pm_runtime_put() would decrement the usage counter and call rpm_idle() which would instantly return an error code. The end result would be the same, except that pm_runtime_put() would return a non-zero error code, but we ignore it anyway. However strange it looks, this seems to be an API guarantee, so Andy's suggestion is correct. Best regards, Tomasz > > goto err_rpm_put; > > > > > 545 } > > > 546 > > > 547 ret = __ov02a10_start_stream(ov02a10); > > > 548 if (ret) { > > > 549 __ov02a10_stop_stream(ov02a10); > > > 550 ov02a10->streaming = !on; > > > 551 goto err_rpm_put; > > > 552 } > > > 553 } else { > > > 554 __ov02a10_stop_stream(ov02a10); > > > 555 pm_runtime_put(&client->dev); > > > 556 } > > > 557 > > > 558 ov02a10->streaming = on; > > > > (1) > > > > > 559 mutex_unlock(&ov02a10->mutex); > > > 560 > > > 561 return 0; > > > 562 > > > 563 err_rpm_put: > > > 564 pm_runtime_put(&client->dev); > > > > > 565 unlock_and_return: > > > > Should be moved to (1). > > > > > 566 mutex_unlock(&ov02a10->mutex); > > > 567 > > > > > > Uninitialized scalar variable (UNINIT) > > > 4. uninit_use: Using uninitialized value ret. > > > > > > 568 return ret; > > > 569 } > > > > > > Variable ret has not been initialized, so the error return value is a > > > garbage value. It should be initialized with some appropriate negative > > > error code, or ret could be removed and the return should return a > > > literal value of a error code. > > > > > > I was unsure what value is appropriate to fix this, so instead I'm > > > reporting this issue. > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-04 11:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <9af089ea-2532-68ac-5d22-97a669ccec91@canonical.com>
2020-12-03 18:10 ` media: i2c: add OV02A10 image sensor driver Andy Shevchenko
2020-12-03 18:24 ` Colin Ian King
2020-12-03 18:30 ` Andy Shevchenko
2020-12-04 11:45 ` Sakari Ailus
2020-12-04 2:46 ` Dongchun Zhu
2020-12-04 6:23 ` Tomasz Figa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox