Re: Race in rcar-v4l2.c

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hello Tomi,

Thanks for testing that series.

On 2024-09-06 15:28:21 +0300, Tomi Valkeinen wrote:
> On 06/09/2024 14:27, Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 06/09/2024 14:14, Tomi Valkeinen wrote:
> > > Hi Niklas,
> > > 
> > > On 06/09/2024 13:14, Niklas Söderlund wrote:
> > > > Hi Tomi,
> > > > 
> > > > Thanks for your report.
> > > > 
> > > > I have an on-going series trying to clean this all up [1]. The one
> > > > change in the v4l-async core I proposed was however rejected and I have
> > > > yet to circle back to figure out a different solution.
> > > > 
> > > > Could you give it a try and see if it also solves this issue?
> > > > 
> > > > 1. [PATCH 0/6] media: rcar-vin: Make use of multiple connections
> > > > in v4l-async
> > > >     https://lore.kernel.org/linux-renesas-
> > > > soc/20240129202254.1126012-1-niklas.soderlund+renesas@ragnatech.se/
> > > 
> > > The compilation fails (broken in media: rcar-vin: Simplify remote
> > > source type detection) with:
> > > 
> > > drivers/media/platform/renesas/rcar-vin/rcar-dma.c:767:24: error:
> > > ‘struct rvin_dev’ has no member named ‘is_csi’
> > > 
> > > If I hack past that, I don't see the warnings anymore. But if I'm
> > > not mistaken, rvin_release() is not called at all anymore. I can
> > > also see plenty of leaks with kmemleak. Those seem to originate from
> > > max96712, but... I don't see max96712's remove() getting called
> > > either when I remove the module.

FWIW module unloading is tricky and AFIK not supported upstream. There 
is a reason the VIN capture pipeline have .suppress_bind_attrs = true 
;-)

> > 
> > Sorry, never mind that. I had the debug print in max96714... =). So
> > max96712 remove() gets called, but it's missing:
> > 
> > +       v4l2_subdev_cleanup(&priv->sd);
> > +       media_entity_cleanup(&priv->sd.entity);
> > +       v4l2_ctrl_handler_free(&priv->ctrl_handler);
> > 
> > But now I'm seeing rvin_release() getting called (no warnings). I'm not
> > sure what changed. Maybe I had some extra changes lying around. Let me
> > test the series a bit more...
> 
> I haven't seen the warning anymore, so I believe your series indeed fixes
> the issue.

Awesome! I will try to find some time soon and rebase that series, I 
think I have an idea on how to avoid having to mock around in the 
v4l-async core and still achieve the same thing.

> 
>  Tomi
> 
> > 
> >   Tomi
> > 
> > > I'm testing on Renesas' whitehawk board, with max96712 TPG. If you
> > > have that board, and want to try module loading & unloading, you
> > > also need to fix the max96712 remove function:
> > > 
> > > -       struct max96712_priv *priv = i2c_get_clientdata(client);
> > > +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +       struct max96712_priv *priv = v4l2_get_subdevdata(sd);
> > > 
> > >   Tomi
> > > 
> > > > On 2024-09-06 12:57:50 +0300, Tomi Valkeinen wrote:
> > > > > Hi Niklas,
> > > > > 
> > > > > There seems to be a race in rcar-v4l2.c, causing
> > > > > WARN_ON(entity->use_count < 0) in pipeline_pm_power_one().
> > > > > 
> > > > > If my understanding is correct, the VIN v4l2 nodes are being created
> > > > > (rvin_v4l2_register), meaning they are userspace accessible,
> > > > > but the media
> > > > > pipeline as a whole is not ready yet (e.g. media links).
> > > > > 
> > > > > So what happens is that after some video nodes have been created, the
> > > > > userspace opens them (I think it's udevd checking the new
> > > > > device nodes),
> > > > > causing rvin_open(). rvin_open() goes through the media
> > > > > graph and does some
> > > > > PM enabling (I'm not familiar with the legacy v4l2_pipeline_pm_get()).
> > > > > However, as the links are not there, it doesn't really
> > > > > enable much at all.
> > > > > 
> > > > > Then the driver goes forward and finishes with the media graph.
> > > > > 
> > > > > Then the userspace closes the opened video nodes,
> > > > > rvin_release() gets called
> > > > > and it goes through the media graph, which now contains all
> > > > > the entities,
> > > > > and powers them down. As the entities were never powered up, we hit the
> > > > > use_count warning.
> > > > > 
> > > > > This happens quite often to me when loading the modules, but
> > > > > I think it can
> > > > > be made to happen more often by adding msleep(1000) to the beginning of
> > > > > rvin_release(), thus ensuring that the graph setup is
> > > > > finished before the
> > > > > rvin_release() proceeds (and hoping that the graph setup was
> > > > > not ready when
> > > > > rvin_open() was called).
> > > > > 
> > > > >   Tomi
> > > > > 
> > > > 
> > > 
> > 
> 

-- 
Kind Regards,
Niklas Söderlund