Re: [RFC PATCH v7 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni]

On 4/15/20 9:54 AM, Sowjanya Komatineni wrote:
> On 4/15/20 7:22 AM, Dmitry Osipenko wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > 15.04.2020 05:57, Sowjanya Komatineni ÐÐÑÐÑ:
> > > +static int tegra_csi_remove(struct platform_device *pdev)
> > > +{
> > > +ÂÂÂÂ struct tegra_csi *csi = platform_get_drvdata(pdev);
> > > +ÂÂÂÂ int err;
> > > +
> > > +ÂÂÂÂ err = host1x_client_unregister(&csi->client);
> > > +ÂÂÂÂ if (err < 0) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(csi->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "failed to unregister host1x client: %d\n", err);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ return err;
> > > +ÂÂÂÂ }
> > > +
> > > +ÂÂÂÂ pm_runtime_disable(csi->dev);
> > > +ÂÂÂÂ kfree(csi);
> > IIRC, the driver removal is invoked on the unbinding. Hence, I'm not
> > sure how moving away from the resource-managed API helps here. Could you
> > please explain in a more details?
> >
> > Have you tried to test this driver under KASAN? I suspect that you just
> > masked the problem, instead of fixing it.
> Using devm_kzalloc for vi/csi structures based on prior feedback 
> request to switch to use kzalloc all over this driver.
>
> Hi Hans,
>
> video devices lifetime is till video device nodes are released. So, 
> v4l2 device release callback does the release of tegra channel 
> allocation which hold video device.
>
> Below are the 3 possible cases of unbind/unload,
>
> 1. during tegra-video module unload, if v4l2 device refcnt is not 0 
> which is the case when any of video device node handle is kept opened 
> then unloading module will not happen and module refcnt is also 
> non-zero and unloading tegra-video module reports module in use.
>
> 2. during tegra-video driver unbind, tegra-video driver removal will 
> do vi/csi clients exit ops which unregisters video device allocated 
> memory during release callback of v4l2 device. vi/csi structure 
> allocation remains same as vi/csi driver removal will not happen in 
> this case.
>
>
> 3. during direct host1x client drivers vi/csi unbind, both 
> host1x_clients vi/csi gets unregistered, deletes host1x logical device 
> which executes tegra-video driver removal() -> vi/csi exit() before 
> vi/csi memory gets freed in vi/csi driver remove().
>
> So, any active streaming will stop and video devices are unregistered 
> during direct client driver unbind prior to freeing vi/csi memory.
>
> Also vi/csi driver remove does explicit free vi/csi as its allocated 
> with kzalloc. So not sure how using kzalloc is different to 
> devm_kzalloc for vi/csi structure in terms of when vi/csi memory gets 
> freed?
>
> Except for channel allocation which holds video device and as video 
> device life time is beyond tegra-video module unbind->vi exit(), looks 
> like we can use devm_kzalloc for vi/csi.
>
>
> Can you please comment if you still think we need to use kzalloc 
> rather than devm_kzalloc for vi/csi structure allocation?
>
> Thanks
>
> Sowjanya
One more case is when video device node is kept opened with v4l2-ctl 
sleep (rather than streaming), where it will keep device node open for 
specified time and if direct vi client driver unbind happens then vi 
driver remove() will free vi memory before v4l2 device release happens.

But I don't see any crash or errors with this case.

Also if we allow direct client driver unbind, then vi structure memory 
lifetime should also be till v4l2 device release happens.

But we can free vi in v4l2 device release callback as in case when 
device node is not kept opened, video device release happens immediate 
and we cant free vi that early.

Hans/Thierry, Can you please comment on this case?

Thanks

Sowjanya