On Tue, Aug 2, 2022 at 6:26 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Tue, Aug 2, 2022 at 2:58 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > On 8/2/22 14:49, Andy Shevchenko wrote: > > > On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > >> On 8/2/22 14:26, Andy Shevchenko wrote: > > >>> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko > > >>> <andy.shevchenko@xxxxxxxxx> wrote: > > >>>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko > > >>>> <andy.shevchenko@xxxxxxxxx> wrote: > > ... > > > >>>>> You are blaming the wrong man here, i.e. devm. The problem as I stated > > >>>>> above is developers who do not understand (pay attention to) the > > >>>>> lifetime of the objects. > > >>>> > > >>>> That said, the devm has nothing to do with the driver still being > > >>>> problematic for the scenario you described, no? > > >>> > > >>> And the cleanest (at the first glance) solution is to make v4l2 to fix > > >>> this bug by suppressing unbind attributes when the device is opened > > >>> for all v4l2 subdev drivers, and restore it back when it's closed. > > >> > > >> Why would we do that? The patch works in the scenario that I described: > > >> the memory is freed in the struct video_device release() callback, which > > >> is called when the last reference to the video node goes away. This is > > >> standard V4L2 framework behavior that works great in the case of a unbind. > > >> > > >> Without devm_kzalloc it works fine, even when unbind is called. With > > >> devm_kzalloc the unbind attributes would have to be suppressed. I see no > > >> reason for that as media maintainer. > > > > > > I'm not sure anymore that we are talking about the same thing. > > > > > > Your driver allocates memory with kzalloc() in ->probe() and frees it > > > in ->remove(). How is this different from the lifetime of a devm:ed > > > object? If what you said is true, than driver is still problematic, > > > since ->remove() frees this memory the very same way at unbind call. > > > > No, it is not freed in remove(). remove() calls video_unregister_device(), > > and that calls cat24c208_release() when the last user of /dev/videoX closes > > its filehandle. And it is cat24c208_release() that finally frees the memory. > > Okay, I got it. > > So, if you unbind the driver the state will be stale and still being > accessible by the user (with outdated info). Now, what happens if you > unbind and try to bind back, while the user is slow enough to close > the device file? > > Also it's still racy against any i2c calls done via IOCTLs, because > you have unregistered I2C devices. As far as I understand current design, you may not call anything but video_device_unregister() in the subdevice's ->remove(), or be very careful on what resources can be used via IOCTLs. -- With Best Regards, Andy Shevchenko
