| Message ID | 20231117101140.1338288-10-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify sub-device state access functions | expand |
Hi Sakari, Thank you for the patch. On Fri, Nov 17, 2023 at 12:11:39PM +0200, 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+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 36 +++++---------------------- > 1 file changed, 6 insertions(+), 30 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 41fe9fa5c21a..985873b7981e 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1545,16 +1545,8 @@ __v4l2_subdev_state_get_format(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. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].format; > } > @@ -1587,16 +1579,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > 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. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].crop; > } > @@ -1629,16 +1613,8 @@ __v4l2_subdev_state_get_compose(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. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].compose; > }
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 41fe9fa5c21a..985873b7981e 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1545,16 +1545,8 @@ __v4l2_subdev_state_get_format(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. The - * purpose of setting pad to 0 here is to avoid accessing memory - * outside the pads array, but still issuing warning of the - * invalid access while making the caller's error handling - * easier. - */ - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) - pad = 0; + if (pad >= state->sd->entity.num_pads) + return NULL; return &state->pads[pad].format; } @@ -1587,16 +1579,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, 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. The - * purpose of setting pad to 0 here is to avoid accessing memory - * outside the pads array, but still issuing warning of the - * invalid access while making the caller's error handling - * easier. - */ - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) - pad = 0; + if (pad >= state->sd->entity.num_pads) + return NULL; return &state->pads[pad].crop; } @@ -1629,16 +1613,8 @@ __v4l2_subdev_state_get_compose(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. The - * purpose of setting pad to 0 here is to avoid accessing memory - * outside the pads array, but still issuing warning of the - * invalid access while making the caller's error handling - * easier. - */ - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) - pad = 0; + if (pad >= state->sd->entity.num_pads) + return NULL; return &state->pads[pad].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 | 36 +++++---------------------- 1 file changed, 6 insertions(+), 30 deletions(-)