| Message ID | 20231026070329.948847-9-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Unify sub-device state access functions | expand |
On 26/10/2023 09:03, Sakari Ailus wrote: > Return NULL from sub-device pad state access functions > (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While > this behaviour differs from older set of pad state information access > functions, we've had a WARN_ON() there for a long time and callers also do > validate the pad index nowadays. Therefore problems are not expected. Huh? Patch 2 adds the WARN_ON, and it is removed in patch 8 again? I'm really confused. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 9d4ff9b4fcec..bd0d89c2996f 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1684,12 +1684,8 @@ __v4l2_subdev_state_get_format_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_fmt; > } > @@ -1722,12 +1718,8 @@ __v4l2_subdev_state_get_crop_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_crop; > } > @@ -1760,12 +1752,8 @@ __v4l2_subdev_state_get_compose_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_compose; > }
Hi Hans, On Thu, Oct 26, 2023 at 09:35:55AM +0200, Hans Verkuil wrote: > On 26/10/2023 09:03, Sakari Ailus wrote: > > Return NULL from sub-device pad state access functions > > (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While > > this behaviour differs from older set of pad state information access > > functions, we've had a WARN_ON() there for a long time and callers also do > > validate the pad index nowadays. Therefore problems are not expected. > > Huh? Patch 2 adds the WARN_ON, and it is removed in patch 8 again? > > I'm really confused. Patch two makes the generic subdev state information access functions do exactly what the subdev state pad-specific information access functions did. As we're merging two classes of users, doing one thing at a time is prudent. This patch aligns the behaviour of the single set of subdev state access functions. Should there a problem there, we can just revert this one.
Hi Sakari, Thank you for the patch. On Thu, Oct 26, 2023 at 10:03:28AM +0300, Sakari Ailus wrote: > Return NULL from sub-device pad state access functions > (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While > this behaviour differs from older set of pad state information access > functions, we've had a WARN_ON() there for a long time and callers also do > validate the pad index nowadays. Therefore problems are not expected. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 9d4ff9b4fcec..bd0d89c2996f 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1684,12 +1684,8 @@ __v4l2_subdev_state_get_format_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_fmt; > } > @@ -1722,12 +1718,8 @@ __v4l2_subdev_state_get_crop_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_crop; > } > @@ -1760,12 +1752,8 @@ __v4l2_subdev_state_get_compose_stream(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. > - */ > if (WARN_ON(pad >= state->sd->entity.num_pads)) > - pad = 0; > + return NULL; > > return &state->pads[pad].try_compose; > }
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 9d4ff9b4fcec..bd0d89c2996f 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1684,12 +1684,8 @@ __v4l2_subdev_state_get_format_stream(struct v4l2_subdev_state *state, if (stream) return NULL; - /* - * Set the pad to 0 on error as this is aligned with the - * behaviour of the pad state information access functions. - */ if (WARN_ON(pad >= state->sd->entity.num_pads)) - pad = 0; + return NULL; return &state->pads[pad].try_fmt; } @@ -1722,12 +1718,8 @@ __v4l2_subdev_state_get_crop_stream(struct v4l2_subdev_state *state, if (stream) return NULL; - /* - * Set the pad to 0 on error as this is aligned with the - * behaviour of the pad state information access functions. - */ if (WARN_ON(pad >= state->sd->entity.num_pads)) - pad = 0; + return NULL; return &state->pads[pad].try_crop; } @@ -1760,12 +1752,8 @@ __v4l2_subdev_state_get_compose_stream(struct v4l2_subdev_state *state, if (stream) return NULL; - /* - * Set the pad to 0 on error as this is aligned with the - * behaviour of the pad state information access functions. - */ if (WARN_ON(pad >= state->sd->entity.num_pads)) - pad = 0; + return NULL; return &state->pads[pad].try_compose; }
Return NULL from sub-device pad state access functions (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While this behaviour differs from older set of pad state information access functions, we've had a WARN_ON() there for a long time and callers also do validate the pad index nowadays. Therefore problems are not expected. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)