| Message ID | 20231027095913.1010187-9-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, Oct 27, 2023 at 12:59:13PM +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 | 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 bae932134cfc..923a221955a9 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1684,16 +1684,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].try_fmt; > } > @@ -1726,16 +1718,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].try_crop; > } > @@ -1768,16 +1752,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].try_compose; > }
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index bae932134cfc..923a221955a9 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1684,16 +1684,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].try_fmt; } @@ -1726,16 +1718,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].try_crop; } @@ -1768,16 +1752,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].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 | 36 +++++---------------------- 1 file changed, 6 insertions(+), 30 deletions(-)