On Fri, Jul 04, 2025 at 06:27:48PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 04, 2025 at 03:18:07AM +0300, Laurent Pinchart wrote:
> > The media bus code passed to the .enum_frame_size() operation for the
> > sink pad is required to be supported by the device, but not to match the
> > current format. All entities that use the vsp1_subdev_enum_frame_size()
> > helper, as well as the SRU and UDS entities that implement the operation
> > manually, perform the check incorrectly.
> >
> > Fix the issue by implementing the correct code check in the
> > vsp1_subdev_enum_frame_size(). For the SRU and UDS, to avoid duplicating
> > code, use the vsp1_subdev_enum_frame_size() as a base and override the
> > enumerated size on the source pad with entity-specific constraints.
> >
> > While at it, include the missing <linux/mutex.h> as the code locks
> > mutexes.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > Changes since v1:
> >
> > - Include <linux/cleanup.h> and <linux/mutex.h>
> > ---
> > .../media/platform/renesas/vsp1/vsp1_entity.c | 49 ++++++++++++-------
> > .../media/platform/renesas/vsp1/vsp1_sru.c | 38 +++++++-------
> > .../media/platform/renesas/vsp1/vsp1_uds.c | 38 +++++++-------
> > 3 files changed, 64 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > index 04b7ae6fb935..892a2adfdf3a 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > @@ -7,8 +7,10 @@
> > * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > */
> >
> > +#include <linux/cleanup.h>
> > #include <linux/device.h>
> > #include <linux/gfp.h>
> > +#include <linux/mutex.h>
> >
> > #include <media/media-entity.h>
> > #include <media/v4l2-ctrls.h>
> > @@ -238,42 +240,51 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_frame_size_enum *fse)
> > {
> > struct vsp1_entity *entity = to_vsp1_entity(subdev);
> > - struct v4l2_subdev_state *state;
> > - struct v4l2_mbus_framefmt *format;
> > - int ret = 0;
> >
> > - state = vsp1_entity_get_state(entity, sd_state, fse->which);
> > - if (!state)
> > + if (fse->index)
> > return -EINVAL;
> >
> > - format = v4l2_subdev_state_get_format(state, fse->pad);
> > -
> > - mutex_lock(&entity->lock);
> > -
> > - if (fse->index || fse->code != format->code) {
> > - ret = -EINVAL;
> > - goto done;
> > - }
> > -
> > if (fse->pad == 0) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < entity->num_codes; ++i) {
> > + if (fse->code == entity->codes[i])
> > + break;
> > + }
> > +
> > + if (i == entity->num_codes)
> > + return -EINVAL;
> > +
> > fse->min_width = entity->min_width;
> > fse->max_width = entity->max_width;
> > fse->min_height = entity->min_height;
> > fse->max_height = entity->max_height;
> > } else {
> > + struct v4l2_subdev_state *state;
> > + struct v4l2_mbus_framefmt *format;
> > +
> > + state = vsp1_entity_get_state(entity, sd_state, fse->which);
> > + if (!state)
> > + return -EINVAL;
> > +
> > /*
> > - * The size on the source pad are fixed and always identical to
> > - * the size on the sink pad.
> > + * The media bus code and size on the source pad are fixed and
> > + * always identical to the sink pad.
> > */
> > + format = v4l2_subdev_state_get_format(state, 0);
> > +
> > + guard(mutex)(&entity->lock);
> > +
> > + if (fse->code != format->code)
> > + return -EINVAL;
> > +
> > fse->min_width = format->width;
> > fse->max_width = format->width;
> > fse->min_height = format->height;
> > fse->max_height = format->height;
> > }
> >
> > -done:
> > - mutex_unlock(&entity->lock);
> > - return ret;
> > + return 0;
> > }
> >
> > /*
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_sru.c b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > index 1dc34e6a510d..37fd36d09045 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > @@ -7,8 +7,10 @@
> > * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > */
> >
> > +#include <linux/cleanup.h>
> > #include <linux/device.h>
> > #include <linux/gfp.h>
> > +#include <linux/mutex.h>
> >
> > #include <media/v4l2-subdev.h>
> >
> > @@ -116,29 +118,25 @@ static int sru_enum_frame_size(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_frame_size_enum *fse)
> > {
> > struct vsp1_sru *sru = to_sru(subdev);
> > - struct v4l2_subdev_state *state;
> > - struct v4l2_mbus_framefmt *format;
> > - int ret = 0;
> > + int ret;
> >
> > - state = vsp1_entity_get_state(&sru->entity, sd_state, fse->which);
> > - if (!state)
> > - return -EINVAL;
> > + ret = vsp1_subdev_enum_frame_size(subdev, sd_state, fse);
> > + if (ret)
> > + return ret;
>
> Here, in case of PAD_SOURCE, we have validated that the fse->code
> matches the format on the sink
>
> >
> > - format = v4l2_subdev_state_get_format(state, SRU_PAD_SINK);
> > + if (fse->pad == SRU_PAD_SOURCE) {
> > + struct v4l2_subdev_state *state;
> > + struct v4l2_mbus_framefmt *format;
> >
> > - mutex_lock(&sru->entity.lock);
> > + state = vsp1_entity_get_state(&sru->entity, sd_state,
> > + fse->which);
> > + if (!state)
> > + return -EINVAL;
> >
> > - if (fse->index || fse->code != format->code) {
> > - ret = -EINVAL;
> > - goto done;
> > - }
> > + format = v4l2_subdev_state_get_format(state, SRU_PAD_SINK);
> > +
> > + guard(mutex)(&sru->entity.lock);
>
> and we get here where we do SRU-specific adjustments to the sizes but
> we don't re-check for the code.
>
> Can the code on the sink change between the above call to
> vsp1_subdev_enum_frame_size() and here ? Is this a concern ?
I guess it could change, but I don't think that's a concern. The only
reason we check the format is to return an error if it doesn't match.
Failure to do so will make enumeration of sizes succeed with an
incorrect format, but will have no other bad effect. As such a case will
only occur when userspace races itself, I don't think we introduce a
problem here. If the change occurs after validation and before the
enum_frame_size call returns to userspace, that won't be distinguishible
in userspace from a case where the enum_frame_size function completes
before the change, followed by the current thread being scheduled out,
and returning from the ioctl after the format change completes.
All this will also get simplified when switching to the subdev active
state API, which I'm working on.
> Same for UDS I guess..
>
> >
> > - if (fse->pad == SRU_PAD_SINK) {
> > - fse->min_width = SRU_MIN_SIZE;
> > - fse->max_width = SRU_MAX_SIZE;
> > - fse->min_height = SRU_MIN_SIZE;
> > - fse->max_height = SRU_MAX_SIZE;
> > - } else {
> > fse->min_width = format->width;
> > fse->min_height = format->height;
> > if (format->width <= SRU_MAX_SIZE / 2 &&
> > @@ -151,9 +149,7 @@ static int sru_enum_frame_size(struct v4l2_subdev *subdev,
> > }
> > }
> >
> > -done:
> > - mutex_unlock(&sru->entity.lock);
> > - return ret;
> > + return 0;
> > }
> >
> > static void sru_try_format(struct vsp1_sru *sru,
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_uds.c b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > index 8006d49ffbea..dd4722315c56 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > @@ -7,8 +7,10 @@
> > * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> > */
> >
> > +#include <linux/cleanup.h>
> > #include <linux/device.h>
> > #include <linux/gfp.h>
> > +#include <linux/mutex.h>
> >
> > #include <media/v4l2-subdev.h>
> >
> > @@ -121,38 +123,32 @@ static int uds_enum_frame_size(struct v4l2_subdev *subdev,
> > struct v4l2_subdev_frame_size_enum *fse)
> > {
> > struct vsp1_uds *uds = to_uds(subdev);
> > - struct v4l2_subdev_state *state;
> > - struct v4l2_mbus_framefmt *format;
> > - int ret = 0;
> > + int ret;
> >
> > - state = vsp1_entity_get_state(&uds->entity, sd_state, fse->which);
> > - if (!state)
> > - return -EINVAL;
> > + ret = vsp1_subdev_enum_frame_size(subdev, sd_state, fse);
> > + if (ret)
> > + return ret;
> >
> > - format = v4l2_subdev_state_get_format(state, UDS_PAD_SINK);
> > + if (fse->pad == UDS_PAD_SOURCE) {
> > + struct v4l2_subdev_state *state;
> > + struct v4l2_mbus_framefmt *format;
> >
> > - mutex_lock(&uds->entity.lock);
> > + state = vsp1_entity_get_state(&uds->entity, sd_state,
> > + fse->which);
> > + if (!state)
> > + return -EINVAL;
> >
> > - if (fse->index || fse->code != format->code) {
> > - ret = -EINVAL;
> > - goto done;
> > - }
> > + format = v4l2_subdev_state_get_format(state, UDS_PAD_SINK);
> > +
> > + guard(mutex)(&uds->entity.lock);
> >
> > - if (fse->pad == UDS_PAD_SINK) {
> > - fse->min_width = UDS_MIN_SIZE;
> > - fse->max_width = UDS_MAX_SIZE;
> > - fse->min_height = UDS_MIN_SIZE;
> > - fse->max_height = UDS_MAX_SIZE;
> > - } else {
> > uds_output_limits(format->width, &fse->min_width,
> > &fse->max_width);
> > uds_output_limits(format->height, &fse->min_height,
> > &fse->max_height);
> > }
> >
> > -done:
> > - mutex_unlock(&uds->entity.lock);
> > - return ret;
> > + return 0;
> > }
> >
> > static void uds_try_format(struct vsp1_uds *uds,
--
Regards,
Laurent Pinchart
