On Fri, Sep 23, 2022 at 12:03 AM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> On Fri, Sep 23, 2022 at 1:45 AM Matt Ranostay
> <matt.ranostay@xxxxxxxxxxxx> wrote:
> >
> > On Wed, Sep 21, 2022 at 10:57 AM Matt Ranostay
> > <matt.ranostay@xxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
> > > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > >
> > > > [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> > > > the series, as you will need ack from us and we don't necessarily monitor
> > > > every single message on linux-input]
> > > >
> > > > On Sep 20 2022, Matt Ranostay wrote:
> > > > > Switch from i2c_add_adapter() to resource managed devm_i2c_add_adapter()
> > > > > for matching rest of driver initialization, and more concise code.
> > > > >
> > > > > Signed-off-by: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/hid/hid-mcp2221.c | 45 +++++++++++++++++----------------------
> > > > > 1 file changed, 19 insertions(+), 26 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > > > > index de52e9f7bb8c..7ba63bcd66de 100644
> > > > > --- a/drivers/hid/hid-mcp2221.c
> > > > > +++ b/drivers/hid/hid-mcp2221.c
> > > > > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > > > > return 1;
> > > > > }
> > > > >
> > > > > +static void mcp2221_hid_remove(void *ptr)
> > > > > +{
> > > > > + struct hid_device *hdev = ptr;
> > > > > +
> > > > > + hid_hw_close(hdev);
> > > > > + hid_hw_stop(hdev);
> > > >
> > > > By default, if you remove the .remove() callback, hid_hw_stop() will get
> > > > automatically called by hid-core.c. So we are now calling it twice,
> > > > which, in a way is not a big deal but it might be an issue in the long
> > > > run.
> > > >
> > > > Generally speaking, in the HID subsystem, that situation doesn't happen
> > > > a lot because hid_hw_start() is usually the last command of probe, and
> > > > we don't need to open the device in the driver itself.
> > > >
> > > > Here, I guess as soon as you add the i2c adapter, you might want to have
> > > > the communication channels ready, and thus you need to have it open
> > > > *before* i2c_add_adapter.
> > > >
> > > > I would suggest the following if you want to keep the devm release of
> > > > stop and close: please put a big fat warning before mcp2221_hid_remove()
> > > > explaining that this is called in devm management, *and* add a function
> > > > that would just return 0 as the .remove() callback with another big fat
> > > > warning explaining that we don't want hid-core.c to call hid_hw_stop()
> > > > because we are doing it ourself through devres.
> > > >
> > >
> > > Yeah maybe best to keep the non-devres if it isn't going to affect how the last
> > > change in this series is trying to implement with iio.
> > >
> > > I'll wait for Jonathan to chime in on this thread.
> > >
> > > > Last, in the HID subsystem, we often interleave non devres with devres
> > > > for resource allocation, given that .remove() will be called before any
> > > > devres release. But that is assuming this ordering is OK, which doesn't
> > > > seem to be the case here. We first need to unregister the i2c adapter
> > > > and then close/stop the HID device.
> >
> > On second thought I2C will be unregistered before the HID calls, since
> > unless I'm totally
> > incorrect device resource management unwinds backwards in the order actions are
> > registered.
>
> Yeah, sorry if it was not clear:
> - .remove() is called *before* any devres action takes place
> - devres action are LIFO, so unwinded backwards as you say
>
> In the general case, a driver does:
> int probe() {
> void *pointer = devm_alloc(...)
> some_more_devm_action(...)
> hid_hw_start()
> return 0;
> }
>
> and so the HID start action is the last one, meaning that .remove will
> first call stop and then devres unwind will get called.
>
> But here, in your case, you need hid_hw_start to be called *before*
> devm_i2c_add_adapter(), meaning that the implicit .remove() will mess
> up with the device, so you are forced to do something about it.
>
> You can either keep a non devm variant, or you can override the
> .remove() of HID to not do anything and do the stop/close in a
> specific devm task, which you did here. You are just missing the
> "let's override .remove() to ensure we keep the device open and
> started while we need it".
Ok, now I understand!
Thanks,
Matt
>
> Cheers,
> Benjamin
>
