* [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection()
@ 2024-05-09 17:05 Ricardo Ribalda
2024-05-09 19:39 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Ricardo Ribalda @ 2024-05-09 17:05 UTC (permalink / raw
To: Sakari Ailus, Martin Kepplinger, Mauro Carvalho Chehab,
Pavel Machek
Cc: Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
linux-kernel, Ricardo Ribalda
The current code does not return anything to the user.
Although the code looks a bit dangerous (using a pointer without
checking if it is valid), it should be fine. The code validates that
sel->pad has a valid value.
Fix the following smatch error:
drivers/media/i2c/hi846.c:1854 hi846_get_selection() warn: statement has no effect 31
Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
While running media-ci on the last patches there was a new sparse
warning:
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/58524338/artifacts/external_file/junit/test-smatch.log.txt
---
drivers/media/i2c/hi846.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 9c565ec033d4..52d9ca68a86c 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1851,7 +1851,7 @@ static int hi846_get_selection(struct v4l2_subdev *sd,
mutex_lock(&hi846->mutex);
switch (sel->which) {
case V4L2_SUBDEV_FORMAT_TRY:
- v4l2_subdev_state_get_crop(sd_state, sel->pad);
+ sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
break;
case V4L2_SUBDEV_FORMAT_ACTIVE:
sel->r = hi846->cur_mode->crop;
---
base-commit: 48259b90973718d2277db27b5e510f0fe957eaa0
change-id: 20240509-fix-hi846-c3d77768622e
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection()
2024-05-09 17:05 [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection() Ricardo Ribalda
@ 2024-05-09 19:39 ` Laurent Pinchart
2024-05-11 7:47 ` Ricardo Ribalda
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2024-05-09 19:39 UTC (permalink / raw
To: Ricardo Ribalda
Cc: Sakari Ailus, Martin Kepplinger, Mauro Carvalho Chehab,
Pavel Machek, Mauro Carvalho Chehab, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Thu, May 09, 2024 at 05:05:55PM +0000, Ricardo Ribalda wrote:
> The current code does not return anything to the user.
>
> Although the code looks a bit dangerous (using a pointer without
> checking if it is valid), it should be fine. The code validates that
I think you meant s/code/core/
> sel->pad has a valid value.
>
> Fix the following smatch error:
> drivers/media/i2c/hi846.c:1854 hi846_get_selection() warn: statement has no effect 31
>
> Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> While running media-ci on the last patches there was a new sparse
> warning:
> https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/58524338/artifacts/external_file/junit/test-smatch.log.txt
> ---
> drivers/media/i2c/hi846.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 9c565ec033d4..52d9ca68a86c 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1851,7 +1851,7 @@ static int hi846_get_selection(struct v4l2_subdev *sd,
> mutex_lock(&hi846->mutex);
> switch (sel->which) {
> case V4L2_SUBDEV_FORMAT_TRY:
> - v4l2_subdev_state_get_crop(sd_state, sel->pad);
> + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> break;
> case V4L2_SUBDEV_FORMAT_ACTIVE:
> sel->r = hi846->cur_mode->crop;
>
> ---
> base-commit: 48259b90973718d2277db27b5e510f0fe957eaa0
> change-id: 20240509-fix-hi846-c3d77768622e
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection()
2024-05-09 19:39 ` Laurent Pinchart
@ 2024-05-11 7:47 ` Ricardo Ribalda
2024-05-11 19:09 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Ricardo Ribalda @ 2024-05-11 7:47 UTC (permalink / raw
To: Laurent Pinchart
Cc: Sakari Ailus, Martin Kepplinger, Mauro Carvalho Chehab,
Pavel Machek, Mauro Carvalho Chehab, linux-media, linux-kernel
Hi Laurent
On Thu, 9 May 2024 at 21:39, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, May 09, 2024 at 05:05:55PM +0000, Ricardo Ribalda wrote:
> > The current code does not return anything to the user.
> >
> > Although the code looks a bit dangerous (using a pointer without
> > checking if it is valid), it should be fine. The code validates that
>
> I think you meant s/code/core/
Yes, sorry, fatty fingers :)
If you are planning to send a v5 of this
https://patchwork.linuxtv.org/project/linux-media/patch/20240508214045.24716-4-laurent.pinchart+renesas@ideasonboard.com/
Maybe you could include this patch there?
I found the issue when the CI tested your series (eventhough it is not
caused by your series)
Regards!
>
> > sel->pad has a valid value.
> >
> > Fix the following smatch error:
> > drivers/media/i2c/hi846.c:1854 hi846_get_selection() warn: statement has no effect 31
> >
> > Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> > While running media-ci on the last patches there was a new sparse
> > warning:
> > https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/58524338/artifacts/external_file/junit/test-smatch.log.txt
> > ---
> > drivers/media/i2c/hi846.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > index 9c565ec033d4..52d9ca68a86c 100644
> > --- a/drivers/media/i2c/hi846.c
> > +++ b/drivers/media/i2c/hi846.c
> > @@ -1851,7 +1851,7 @@ static int hi846_get_selection(struct v4l2_subdev *sd,
> > mutex_lock(&hi846->mutex);
> > switch (sel->which) {
> > case V4L2_SUBDEV_FORMAT_TRY:
> > - v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > break;
> > case V4L2_SUBDEV_FORMAT_ACTIVE:
> > sel->r = hi846->cur_mode->crop;
> >
> > ---
> > base-commit: 48259b90973718d2277db27b5e510f0fe957eaa0
> > change-id: 20240509-fix-hi846-c3d77768622e
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection()
2024-05-11 7:47 ` Ricardo Ribalda
@ 2024-05-11 19:09 ` Laurent Pinchart
0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2024-05-11 19:09 UTC (permalink / raw
To: Ricardo Ribalda
Cc: Sakari Ailus, Martin Kepplinger, Mauro Carvalho Chehab,
Pavel Machek, Mauro Carvalho Chehab, linux-media, linux-kernel
Hi Ricardo,
On Sat, May 11, 2024 at 09:47:47AM +0200, Ricardo Ribalda wrote:
> On Thu, 9 May 2024 at 21:39, Laurent Pinchart wrote:
> > On Thu, May 09, 2024 at 05:05:55PM +0000, Ricardo Ribalda wrote:
> > > The current code does not return anything to the user.
> > >
> > > Although the code looks a bit dangerous (using a pointer without
> > > checking if it is valid), it should be fine. The code validates that
> >
> > I think you meant s/code/core/
>
> Yes, sorry, fatty fingers :)
>
> If you are planning to send a v5 of this
> https://patchwork.linuxtv.org/project/linux-media/patch/20240508214045.24716-4-laurent.pinchart+renesas@ideasonboard.com/
>
> Maybe you could include this patch there?
Yes, I've already taken the patch in my tree. If Hans merges it to the
stage tree I'll drop it, otherwise I'll include it in the pull request
to avoid breaking CI.
> I found the issue when the CI tested your series (eventhough it is not
> caused by your series)
>
> > > sel->pad has a valid value.
> > >
> > > Fix the following smatch error:
> > > drivers/media/i2c/hi846.c:1854 hi846_get_selection() warn: statement has no effect 31
> > >
> > > Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > ---
> > > While running media-ci on the last patches there was a new sparse
> > > warning:
> > > https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/58524338/artifacts/external_file/junit/test-smatch.log.txt
> > > ---
> > > drivers/media/i2c/hi846.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> > > index 9c565ec033d4..52d9ca68a86c 100644
> > > --- a/drivers/media/i2c/hi846.c
> > > +++ b/drivers/media/i2c/hi846.c
> > > @@ -1851,7 +1851,7 @@ static int hi846_get_selection(struct v4l2_subdev *sd,
> > > mutex_lock(&hi846->mutex);
> > > switch (sel->which) {
> > > case V4L2_SUBDEV_FORMAT_TRY:
> > > - v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > > + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > > break;
> > > case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > sel->r = hi846->cur_mode->crop;
> > >
> > > ---
> > > base-commit: 48259b90973718d2277db27b5e510f0fe957eaa0
> > > change-id: 20240509-fix-hi846-c3d77768622e
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-11 19:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-09 17:05 [PATCH] media: i2c: hi846: Fix V4L2_SUBDEV_FORMAT_TRY get_selection() Ricardo Ribalda
2024-05-09 19:39 ` Laurent Pinchart
2024-05-11 7:47 ` Ricardo Ribalda
2024-05-11 19:09 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).