Hi Sakari!
Thanks for the review. See my reply inline.
On Thu, Apr 14, 2022 at 3:19 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Yunke,
>
> Thanks for the patch.
>
> On Tue, Apr 12, 2022 at 03:23:13PM +0900, Yunke Cao wrote:
> > The original implementation removes reverse links for any input link and
> > assumes the presense of sink/source.
> > It fails when the link is a not a data link.
> > media_entity_remove_links when there's an ancillary link can also fail.
>
> The function's return type is void. Are there other adverse effects from
> this? Looking at the function, it would seem like that the reverse link
> simply isn't found in this case, and so not removed.
>
The function dereferences without any check link->source and link->sink
("link->source->entity == entity" etc.), which is in union.
Ancillary links populate gobj0/gobj1 instead of source/sink.
Calling this function on ancillary links can cause crashes.
> >
> > We only need to remove reverse links for a data link.
>
> Ideally this would not be based on the link flags as it's not a very robust
> way to test whather a backlink needs to be removed.
>
I was mainly trying to make sure link->source and link->sink are
populated by checking the link type.
Currently, only data links need to run this part of the code to remove
reverse links so I feel this is
the easiest way. Let me know if there's any better alternative.
> >
> > Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> > ---
> > drivers/media/mc/mc-entity.c | 34 +++++++++++++++++++---------------
> > 1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 1ff60d411ea9..11f5207f73aa 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -597,26 +597,30 @@ static void __media_entity_remove_link(struct media_entity *entity,
> > struct media_link *rlink, *tmp;
> > struct media_entity *remote;
> >
> > - if (link->source->entity == entity)
> > - remote = link->sink->entity;
> > - else
> > - remote = link->source->entity;
> > + /* Remove the reverse links for a data link. */
> > + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == MEDIA_LNK_FL_DATA_LINK) {
> > + if (link->source->entity == entity)
> > + remote = link->sink->entity;
> > + else
> > + remote = link->source->entity;
> >
> > - list_for_each_entry_safe(rlink, tmp, &remote->links, list) {
> > - if (rlink != link->reverse)
> > - continue;
> > + list_for_each_entry_safe(rlink, tmp, &remote->links, list) {
> > + if (rlink != link->reverse)
> > + continue;
> >
> > - if (link->source->entity == entity)
> > - remote->num_backlinks--;
> > + if (link->source->entity == entity)
> > + remote->num_backlinks--;
> >
> > - /* Remove the remote link */
> > - list_del(&rlink->list);
> > - media_gobj_destroy(&rlink->graph_obj);
> > - kfree(rlink);
> > + /* Remove the remote link */
> > + list_del(&rlink->list);
> > + media_gobj_destroy(&rlink->graph_obj);
> > + kfree(rlink);
> >
> > - if (--remote->num_links == 0)
> > - break;
> > + if (--remote->num_links == 0)
> > + break;
> > + }
> > }
> > +
> > list_del(&link->list);
> > media_gobj_destroy(&link->graph_obj);
> > kfree(link);
>
> --
> Kind regards,
>
> Sakari Ailus
Thanks!
Yunke
