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:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 2, 2022 at 11:06 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > On 8/2/22 10:42, Andy Shevchenko wrote:
> > > On Mon, Aug 1, 2022 at 8:35 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > >> On 01/08/2022 16:57, Andy Shevchenko wrote:
> > >>> On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
> > >>> <hljunggr@cisco.com> wrote:
> > >>>> On Fri, 2022-07-29 at 17:51 +0200, Andy Shevchenko wrote:
> > >>>>> On Thu, Jul 28, 2022 at 1:53 PM Erling Ljunggren <hljunggr@cisco.com>
> > >>>>> wrote:
>
> ...
>
> > >>>>>> +       state = kzalloc(sizeof(*state), GFP_KERNEL);
> > >>>>>> +       if (!state)
> > >>>>>> +               return -ENOMEM;
> > >>>>>
> > >>>>> devm_kzalloc() ?
> > >>>>
> > >>>> This will fail if the device is forcibly unloaded while some
> > >>>> application has the device node open.
> > >>>
> > >>> I'm not sure how it's related. Can you elaborate a bit, please?
> > >>>
> > >>> If you try to forcibly unload the device (driver) when it's open and
> > >>> it somehow succeeds, that will be a sign of lifetime issues in the
> > >>> code.
> > >>
> > >> Not with rmmod but using the unbind facility.
> > >
> > > And what is the difference? The device driver core calls the same, no?
> >
> > rmmod when the /dev/videoX is open won't work (device is busy), whereas
> > unbind *will* work, and it is really the same as a USB unplug.
>
> Seems there are no guards against that.
>
> > >> For new media drivers we generally
> > >> want to avoid using devm_*alloc, it causes more problems than it solves.
> > >
> > > I think it's because people don't think much about the lifetime of
> > > objects. I don't think devm is an issue here.
> >
> > Yes, it is: unbind will call the driver remove() function, and after that
> > function all memory allocated with devm_*alloc will be freed immediately.
>
> Yes.
>
> > But if an application still has a filehandle open and was possibly even in
> > the middle of an ioctl call, then having the memory removed instantaneously
> > is a really bad thing.
>
> True.
>
> > Hotpluggable devices in general definitely should not use it. I'm not a fan
> > of devm_*alloc anymore.
>
> 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?


-- 
With Best Regards,
Andy Shevchenko