| Message ID | 20230918125138.90002-13-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Small fixes and cleanups (ov2740 and ccs) | expand |
Hi Laurent, Thanks for the review. On Mon, Sep 18, 2023 at 04:48:02PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Mon, Sep 18, 2023 at 03:51:38PM +0300, Sakari Ailus wrote: > > Check the validity of pad flags on entity init. Exactly one of the flags > > must be set. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index 83468d4a440b..38d5bbae33d7 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, > > struct media_pad *pads) > > { > > struct media_device *mdev = entity->graph_obj.mdev; > > - struct media_pad *iter; > > + struct media_pad *iter, *iter2; > > unsigned int i = 0; > > + int ret = 0; > > > > if (num_pads >= MEDIA_ENTITY_MAX_PADS) > > return -E2BIG; > > @@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, > > media_entity_for_each_pad(entity, iter) { > > iter->entity = entity; > > iter->index = i++; > > + > > + if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK | > > + MEDIA_PAD_FL_SOURCE)) != 1) { > > + ret = -EINVAL; > > + break; > > + } > > + > > if (mdev) > > media_gobj_create(mdev, MEDIA_GRAPH_PAD, > > &iter->graph_obj); > > } > > > > + if (ret && mdev) { > > + media_entity_for_each_pad(entity, iter2) { > > + if (iter2 == iter) > > + break; > > + media_gobj_destroy(&iter->graph_obj); > > Wrong iterator. > > Instead of using a second iterator, which can be error-prone as shown > here, how about breaking when !iter->graph_obj.mdev, which indicates an > uninitialized object ? media_gobj_destroy() is actually a no-op if it hasn't been initialised (created). It'd be still cleaner to make the pad flag checks first and only then call media_gobj_create() on them. But if someone needs something more in the future, then that doesn't mix as well anymore. Both are fine IMO. > > > + } > > + } > > + > > if (mdev) > > mutex_unlock(&mdev->graph_mutex); > > > > - return 0; > > + return ret; > > } > > EXPORT_SYMBOL_GPL(media_entity_pads_init); > > >
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 83468d4a440b..38d5bbae33d7 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -195,8 +195,9 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, struct media_pad *pads) { struct media_device *mdev = entity->graph_obj.mdev; - struct media_pad *iter; + struct media_pad *iter, *iter2; unsigned int i = 0; + int ret = 0; if (num_pads >= MEDIA_ENTITY_MAX_PADS) return -E2BIG; @@ -210,15 +211,30 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads, media_entity_for_each_pad(entity, iter) { iter->entity = entity; iter->index = i++; + + if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK | + MEDIA_PAD_FL_SOURCE)) != 1) { + ret = -EINVAL; + break; + } + if (mdev) media_gobj_create(mdev, MEDIA_GRAPH_PAD, &iter->graph_obj); } + if (ret && mdev) { + media_entity_for_each_pad(entity, iter2) { + if (iter2 == iter) + break; + media_gobj_destroy(&iter->graph_obj); + } + } + if (mdev) mutex_unlock(&mdev->graph_mutex); - return 0; + return ret; } EXPORT_SYMBOL_GPL(media_entity_pads_init);
Check the validity of pad flags on entity init. Exactly one of the flags must be set. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/mc/mc-entity.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)