| Message ID | 815bc07c-20ef-86ba-1492-54c77afc7a1f@xs4all.nl |
|---|---|
| State | Accepted |
| Commit | e39cc4960207a67a26b39ebc8449f15c53da0a99 |
| Headers | show |
| Series | media: sunxi: sun6i_mipi_csi2.c/sun8i_a83t_mipi_csi2.c: clarify error handling | expand |
Hi Hans, On Mon 18 Jul 22, 18:08, Hans Verkuil wrote: > Both sun6i_mipi_csi2.c and sun8i_a83t_mipi_csi2.c have the same issue: > the comment before the ret = 0 assignment is incorrect, drop it and > always assign the result of the v4l2_subdev_call(..., 0) to ret. Not sure what is incorrect about it. I agree that it's good to return the return code of v4l2_subdev_call instead of always 0 on the !on path though. > In the disable label check for !on and set ret to 0 in that case. > > This fixes two smatch warnings: > > drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:193 sun6i_mipi_csi2_s_stream() warn: missing error code 'ret' I don't think this is legit: the initialization of ret is specifically there to avoid an undefined ret there. > drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:225 sun8i_a83t_mipi_csi2_s_stream() warn: missing error code 'ret' > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > .../media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 7 ++++--- > .../sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c | 7 ++++--- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > index 31e12f1506cb..a4e3f9a6b2ff 100644 > --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > @@ -182,14 +182,13 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > unsigned int lanes_count = > csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > unsigned long pixel_rate; > - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > - int ret = 0; > + int ret; > > if (!source_subdev) > return -ENODEV; > > if (!on) { > - v4l2_subdev_call(source_subdev, video, s_stream, 0); > + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); > goto disable; > } > > @@ -281,6 +280,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > return 0; > > disable: > + if (!on) > + ret = 0; Then this will always overwrite the ret value from v4l2_subdev_call with the !on disable path. Looks like this can be removed. What do you think? Paul > phy_power_off(dphy); > sun6i_mipi_csi2_disable(csi2_dev); > > diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > index c82ede3ca0ff..d052ee77ef0a 100644 > --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > @@ -214,14 +214,13 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > unsigned int lanes_count = > csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > unsigned long pixel_rate; > - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > - int ret = 0; > + int ret; > > if (!source_subdev) > return -ENODEV; > > if (!on) { > - v4l2_subdev_call(source_subdev, video, s_stream, 0); > + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); > goto disable; > } > > @@ -313,6 +312,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > return 0; > > disable: > + if (!on) > + ret = 0; > phy_power_off(dphy); > sun8i_a83t_mipi_csi2_disable(csi2_dev); > > -- > 2.35.1 >
On 7/19/22 11:54, Paul Kocialkowski wrote: > Hi Hans, > > On Mon 18 Jul 22, 18:08, Hans Verkuil wrote: >> Both sun6i_mipi_csi2.c and sun8i_a83t_mipi_csi2.c have the same issue: >> the comment before the ret = 0 assignment is incorrect, drop it and >> always assign the result of the v4l2_subdev_call(..., 0) to ret. > > Not sure what is incorrect about it. I agree that it's good to return the > return code of v4l2_subdev_call instead of always 0 on the !on path though. > >> In the disable label check for !on and set ret to 0 in that case. >> >> This fixes two smatch warnings: >> >> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:193 sun6i_mipi_csi2_s_stream() warn: missing error code 'ret' > > I don't think this is legit: the initialization of ret is specifically there > to avoid an undefined ret there. I believe so as well, but I need to get rid of the smatch warning. > >> drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:225 sun8i_a83t_mipi_csi2_s_stream() warn: missing error code 'ret' >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> .../media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 7 ++++--- >> .../sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c | 7 ++++--- >> 2 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c >> index 31e12f1506cb..a4e3f9a6b2ff 100644 >> --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c >> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c >> @@ -182,14 +182,13 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) >> unsigned int lanes_count = >> csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; >> unsigned long pixel_rate; >> - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ >> - int ret = 0; >> + int ret; >> >> if (!source_subdev) >> return -ENODEV; >> >> if (!on) { >> - v4l2_subdev_call(source_subdev, video, s_stream, 0); >> + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); >> goto disable; >> } >> >> @@ -281,6 +280,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) >> return 0; >> >> disable: >> + if (!on) >> + ret = 0; > > Then this will always overwrite the ret value from v4l2_subdev_call with the > !on disable path. Looks like this can be removed. Returning an error when stopping streaming is kind of pointless, since what are you going to do with that error? I assumed that's why the original code did this, and I didn't want to change the behavior. If you think it is better to just return the error, then that can be a patch on top. BTW, I think it would have been better to have separate streamon and streamoff ops instead of a single s_stream. Starting streaming would return an int, while stopping streaming would be a void function. In general when you stop, release, close, etc. something there is no point in an error code since there is nothing you can do with it. Regards, Hans > > What do you think? > > Paul > >> phy_power_off(dphy); >> sun6i_mipi_csi2_disable(csi2_dev); >> >> diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c >> index c82ede3ca0ff..d052ee77ef0a 100644 >> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c >> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c >> @@ -214,14 +214,13 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) >> unsigned int lanes_count = >> csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; >> unsigned long pixel_rate; >> - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ >> - int ret = 0; >> + int ret; >> >> if (!source_subdev) >> return -ENODEV; >> >> if (!on) { >> - v4l2_subdev_call(source_subdev, video, s_stream, 0); >> + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); >> goto disable; >> } >> >> @@ -313,6 +312,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) >> return 0; >> >> disable: >> + if (!on) >> + ret = 0; >> phy_power_off(dphy); >> sun8i_a83t_mipi_csi2_disable(csi2_dev); >> >> -- >> 2.35.1 >> >
Hans, On Tue 19 Jul 22, 12:04, Hans Verkuil wrote: > On 7/19/22 11:54, Paul Kocialkowski wrote: > > Hi Hans, > > > > On Mon 18 Jul 22, 18:08, Hans Verkuil wrote: > >> Both sun6i_mipi_csi2.c and sun8i_a83t_mipi_csi2.c have the same issue: > >> the comment before the ret = 0 assignment is incorrect, drop it and > >> always assign the result of the v4l2_subdev_call(..., 0) to ret. > > > > Not sure what is incorrect about it. I agree that it's good to return the > > return code of v4l2_subdev_call instead of always 0 on the !on path though. > > > >> In the disable label check for !on and set ret to 0 in that case. > >> > >> This fixes two smatch warnings: > >> > >> drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:193 sun6i_mipi_csi2_s_stream() warn: missing error code 'ret' > > > > I don't think this is legit: the initialization of ret is specifically there > > to avoid an undefined ret there. > > I believe so as well, but I need to get rid of the smatch warning. Yes fair enough, I was just a bit confused by the commit message. > >> drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:225 sun8i_a83t_mipi_csi2_s_stream() warn: missing error code 'ret' > >> > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> --- > >> .../media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 7 ++++--- > >> .../sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c | 7 ++++--- > >> 2 files changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > >> index 31e12f1506cb..a4e3f9a6b2ff 100644 > >> --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > >> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c > >> @@ -182,14 +182,13 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > >> unsigned int lanes_count = > >> csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > >> unsigned long pixel_rate; > >> - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > >> - int ret = 0; > >> + int ret; > >> > >> if (!source_subdev) > >> return -ENODEV; > >> > >> if (!on) { > >> - v4l2_subdev_call(source_subdev, video, s_stream, 0); > >> + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); > >> goto disable; > >> } > >> > >> @@ -281,6 +280,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > >> return 0; > >> > >> disable: > >> + if (!on) > >> + ret = 0; > > > > Then this will always overwrite the ret value from v4l2_subdev_call with the > > !on disable path. Looks like this can be removed. > > Returning an error when stopping streaming is kind of pointless, since > what are you going to do with that error? Yeah that's true and it's probably safer to pretend that things went well just in case returning != 0 somehow ends up halting the turn-off sequence. > I assumed that's why the original code did this, and I didn't want to > change the behavior. Yeah I think it was my initial intent to discard the v4l2_subdev_call return code for this reason. > If you think it is better to just return the error, then that can be a > patch on top. Nah I'm fine with returning zero after all. But instead of assigning ret from v4l2_subdev_call and then overwriting it conditionally in the disable label, you could just as well set ret = 0; before goto disable; which seems less confusing to me. I think this should also satisfy the smatch warning. Otherwise it would be nice to at least have a comment about it or to reflect the ret overwrite in the commit message (and maybe add a blank like after the conditional block while at it). > BTW, I think it would have been better to have separate streamon and streamoff > ops instead of a single s_stream. Starting streaming would return an int, while > stopping streaming would be a void function. In general when you stop, release, > close, etc. something there is no point in an error code since there is nothing > you can do with it. Agreed, maybe this can be somehow helpful in sensor drivers where you just have one common bit to start/halt the stream but for more complex subdevs it's not very helpful to have a single op. Cheers, Paul > Regards, > > Hans > > > > > What do you think? > > > > Paul > > > >> phy_power_off(dphy); > >> sun6i_mipi_csi2_disable(csi2_dev); > >> > >> diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > >> index c82ede3ca0ff..d052ee77ef0a 100644 > >> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > >> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c > >> @@ -214,14 +214,13 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > >> unsigned int lanes_count = > >> csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > >> unsigned long pixel_rate; > >> - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > >> - int ret = 0; > >> + int ret; > >> > >> if (!source_subdev) > >> return -ENODEV; > >> > >> if (!on) { > >> - v4l2_subdev_call(source_subdev, video, s_stream, 0); > >> + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); > >> goto disable; > >> } > >> > >> @@ -313,6 +312,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > >> return 0; > >> > >> disable: > >> + if (!on) > >> + ret = 0; > >> phy_power_off(dphy); > >> sun8i_a83t_mipi_csi2_disable(csi2_dev); > >> > >> -- > >> 2.35.1 > >> > >
diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c index 31e12f1506cb..a4e3f9a6b2ff 100644 --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c @@ -182,14 +182,13 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) unsigned int lanes_count = csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; unsigned long pixel_rate; - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ - int ret = 0; + int ret; if (!source_subdev) return -ENODEV; if (!on) { - v4l2_subdev_call(source_subdev, video, s_stream, 0); + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); goto disable; } @@ -281,6 +280,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) return 0; disable: + if (!on) + ret = 0; phy_power_off(dphy); sun6i_mipi_csi2_disable(csi2_dev); diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c index c82ede3ca0ff..d052ee77ef0a 100644 --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c @@ -214,14 +214,13 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) unsigned int lanes_count = csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; unsigned long pixel_rate; - /* Initialize to 0 to use both in disable label (ret != 0) and off. */ - int ret = 0; + int ret; if (!source_subdev) return -ENODEV; if (!on) { - v4l2_subdev_call(source_subdev, video, s_stream, 0); + ret = v4l2_subdev_call(source_subdev, video, s_stream, 0); goto disable; } @@ -313,6 +312,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) return 0; disable: + if (!on) + ret = 0; phy_power_off(dphy); sun8i_a83t_mipi_csi2_disable(csi2_dev);
Both sun6i_mipi_csi2.c and sun8i_a83t_mipi_csi2.c have the same issue: the comment before the ret = 0 assignment is incorrect, drop it and always assign the result of the v4l2_subdev_call(..., 0) to ret. In the disable label check for !on and set ret to 0 in that case. This fixes two smatch warnings: drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c:193 sun6i_mipi_csi2_s_stream() warn: missing error code 'ret' drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c:225 sun8i_a83t_mipi_csi2_s_stream() warn: missing error code 'ret' Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- .../media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 7 ++++--- .../sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-)