Re: [PATCH 4/5] media: i2c: cat24c208: driver for the cat24c208 EDID EEPROM

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Aug 2, 2022 at 2:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 8/2/22 14:49, Andy Shevchenko wrote:
> > On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 8/2/22 14:26, Andy Shevchenko wrote:
> >>> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
> >>> <andy.shevchenko@gmail.com> wrote:
> >>>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
> >>>> <andy.shevchenko@gmail.com> 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.

-- 
With Best Regards,
Andy Shevchenko