On Jul 26 2023, Doug Anderson wrote: > Hi, > > On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote: > > > > > @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client) > > > struct i2c_hid *ihid = i2c_get_clientdata(client); > > > struct hid_device *hid; > > > > > > - i2c_hid_core_power_down(ihid); > > > + /* > > > + * If we're a follower, the act of unfollowing will cause us to be > > > + * powered down. Otherwise we need to manually do it. > > > + */ > > > + if (ihid->is_panel_follower) > > > + drm_panel_remove_follower(&ihid->panel_follower); > > > > That part is concerning, as we are now calling hid_drv->suspend() when removing > > the device. It might or not have an impact (I'm not sure of it), but we > > are effectively changing the path of commands sent to the device. > > > > hid-multitouch might call a feature in ->suspend, but the remove makes > > that the physical is actually disconnected, so the function will fail, > > and I'm not sure what is happening then. > > It's not too hard to change this if we're sure we want to. I could > change how the panel follower API works, though I'd rather keep it how > it is now for symmetry. Thus, if we want to do this I'd probably just > set a boolean at the beginning of i2c_hid_core_remove() to avoid the > suspend when the panel follower API calls us back. I was more thinking on a boolean. No need to overload the API. > > That being said, are you sure you want me to do that? > > 1. My patch doesn't change the behavior of any existing hardware. It > will only do anything for hardware that indicates it needs the panel > follower logic. Presumably these people could confirm that the logic > is OK for them, though I'll also admit that it's likely not many of > them will test the remove() case. Isn't trogdor (patch 10/10) already supported? Though you should be the one making tests, so it should be fine ;) > > 2. Can you give more details about why you say that the function will > fail? The first thing that the remove() function will do is to > unfollow the panel and that can cause the suspend to happen. At the > time this code runs all the normal communications should work and so > there should be no problems calling into the suspend code. Now that I think about it more, maybe I am too biased by USB where the device remove would happened *after* the device has been physically unplugged. And this doesn't apply of course in the I2C world. > > 3. You can correct me if I'm wrong, but I'd actually argue that > calling the suspend code during remove actually fixes issues and we > should probably do it for the non-panel-follower case as well. I think > there are at least two benefits. One benefit is that if the i2c-hid > device is on a power rail that can't turn off (either an always-on or > a shared power rail) that we'll at least get the device in a low power > state before we stop managing it with this driver. The second benefit > is that it implicitly disables the interrupt and that fixes a > potential crash at remove time(). The crash in the old code I'm > imagining is: > > a) i2c_hid_core_remove() is called. > > b) We try to power down the i2c hid device, which might not do > anything if the device is on an always-on rail. > > c) We call hid_destroy_device(), which frees the hid device. > > d) An interrupt comes in before the call to free_irq() and we try to > dispatch it to the already freed hid device and crash. > > > If you agree that my reasoning makes sense, I can add a separate patch > before this one to suspend during remove. Yep, I agree with you :) Adding a separate patch would be nice, yes. Thanks! Cheers, Benjamin
