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. 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. 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. 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. -Doug
