| Message ID | 20210313191236.4366-1-jeff@labundy.com |
|---|---|
| Headers | show |
| Series | Input: iqs5xx - more enhancements and optimizations | expand |
Hi Jeff, On Sat, Mar 13, 2021 at 01:12:33PM -0600, Jeff LaBundy wrote: > Unsolicited I2C communication causes the device to assert an interrupt; as > such the IRQ is disabled before any registers are written in iqs5xx_open() > and iqs5xx_close(). > > After the driver is unloaded, however, i2c_device_remove() sets the IRQ to > zero before any handlers may call input_close_device() while the device is > unregistered. This keeps iqs5xx_close() from disabling the IRQ, leading to > an interrupt storm during removal. > > Placing input_register_device() in front of devm_request_threaded_irq() to > free the IRQ before iqs5xx_close() is called does not cover the case where > firmware is updated at the factory and the input device is registered well > after the driver has already probed. > > The solution, therefore, is to remove the open and close callbacks as they > do not buy much in the first place. The device already starts in an active > state, then drops into a low-power mode based on activity. No, this is not the proper solution. We should rather fix i2c bus (and really all the other buses with non-trivial probe and remove) so that it is compatible with devres/devm. I wanted to do this for a while and I guess we really need this. Could you please try the patch below and see if it fixes your issue? Thanks. -- Dmitry i2c: ensure timely release of driver-allocated resources From: Dmitry Torokhov <dmitry.torokhov@gmail.com> More and more drivers rely on devres to manage their resources, however if bus' probe() and release() are not trivial and control some of resources as well (for example enable or disable clocks, or attach device to a power domain, we need to make sure that driver-allocated resources are released immediately after driver's remove() method returns, and not postponed until driver core gets around to releasing resources. To fix that we open a new devres group before calling driver's probe() and explicitly release it when we return from driver's remove(). Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/i2c/i2c-core-base.c | 19 ++++++++++++++++++- include/linux/i2c.h | 3 +++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 63ebf722a424..b8a96db2c191 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -518,6 +518,11 @@ static int i2c_device_probe(struct device *dev) if (status) goto err_clear_wakeup_irq; + client->devres_group_id = devres_open_group(&client->dev, NULL, + GFP_KERNEL); + if (!client->devres_group_id) + goto err_detach_pm_domain; + /* * When there are no more users of probe(), * rename probe_new to probe. @@ -530,11 +535,21 @@ static int i2c_device_probe(struct device *dev) else status = -EINVAL; + /* + * Note that we are not closing the devres group opened above so + * even resources that were attached to the device after probe is + * run are released when i2c_device_remove() is executed. This is + * needed as some drivers would allocate additional resources, + * for example when updating firmware. + */ + if (status) - goto err_detach_pm_domain; + goto err_release_driver_resources; return 0; +err_release_driver_resources: + devres_release_group(&client->dev, client->devres_group_id); err_detach_pm_domain: dev_pm_domain_detach(&client->dev, true); err_clear_wakeup_irq: @@ -563,6 +578,8 @@ static int i2c_device_remove(struct device *dev) dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status)); } + devres_release_group(&client->dev, client->devres_group_id); + dev_pm_domain_detach(&client->dev, true); dev_pm_clear_wake_irq(&client->dev); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 56622658b215..5d1f11c0deaa 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -306,6 +306,8 @@ struct i2c_driver { * userspace_devices list * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter * calls it to pass on slave events to the slave driver. + * @devres_group_id: id of the devres group that will be created for resources + * acquired when probing this device. * * An i2c_client identifies a single device (i.e. chip) connected to an * i2c bus. The behaviour exposed to Linux is defined by the driver @@ -334,6 +336,7 @@ struct i2c_client { #if IS_ENABLED(CONFIG_I2C_SLAVE) i2c_slave_cb_t slave_cb; /* callback for slave mode */ #endif + void *devres_group_id; /* ID of probe devres group */ }; #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
Hi Dmitry, On Sat, Mar 13, 2021 at 10:21:27PM -0800, Dmitry Torokhov wrote: > Hi Jeff, > > On Sat, Mar 13, 2021 at 01:12:33PM -0600, Jeff LaBundy wrote: > > Unsolicited I2C communication causes the device to assert an interrupt; as > > such the IRQ is disabled before any registers are written in iqs5xx_open() > > and iqs5xx_close(). > > > > After the driver is unloaded, however, i2c_device_remove() sets the IRQ to > > zero before any handlers may call input_close_device() while the device is > > unregistered. This keeps iqs5xx_close() from disabling the IRQ, leading to > > an interrupt storm during removal. > > > > Placing input_register_device() in front of devm_request_threaded_irq() to > > free the IRQ before iqs5xx_close() is called does not cover the case where > > firmware is updated at the factory and the input device is registered well > > after the driver has already probed. > > > > The solution, therefore, is to remove the open and close callbacks as they > > do not buy much in the first place. The device already starts in an active > > state, then drops into a low-power mode based on activity. > > No, this is not the proper solution. We should rather fix i2c bus (and > really all the other buses with non-trivial probe and remove) so that it > is compatible with devres/devm. I wanted to do this for a while and I > guess we really need this. Could you please try the patch below and see > if it fixes your issue? Thank you for this suggestion; to be honest I had not considered how other drivers may suffer a similar fate and I agree with your approach. I tested your patch and it addresses my issue. That being said, I would still advocate for this patch because of the other reasons mentioned: the open/close callbacks do not happen to buy much since the device effectively "opens" and (almost) "closes" automatically based on touch events, and getting rid of the callbacks lets probe finish faster and cleans up the code a bit. Perhaps as a compromise, I can squash this and the next patch, and speak to these points in a consolidated commit message? > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy > > > i2c: ensure timely release of driver-allocated resources > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > More and more drivers rely on devres to manage their resources, however if > bus' probe() and release() are not trivial and control some of resources as > well (for example enable or disable clocks, or attach device to a power > domain, we need to make sure that driver-allocated resources are released > immediately after driver's remove() method returns, and not postponed until > driver core gets around to releasing resources. To fix that we open a new > devres group before calling driver's probe() and explicitly release it when > we return from driver's remove(). > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Tested-by: Jeff LaBundy <jeff@labundy.com> > --- > drivers/i2c/i2c-core-base.c | 19 ++++++++++++++++++- > include/linux/i2c.h | 3 +++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 63ebf722a424..b8a96db2c191 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -518,6 +518,11 @@ static int i2c_device_probe(struct device *dev) > if (status) > goto err_clear_wakeup_irq; > > + client->devres_group_id = devres_open_group(&client->dev, NULL, > + GFP_KERNEL); > + if (!client->devres_group_id) > + goto err_detach_pm_domain; > + > /* > * When there are no more users of probe(), > * rename probe_new to probe. > @@ -530,11 +535,21 @@ static int i2c_device_probe(struct device *dev) > else > status = -EINVAL; > > + /* > + * Note that we are not closing the devres group opened above so > + * even resources that were attached to the device after probe is > + * run are released when i2c_device_remove() is executed. This is > + * needed as some drivers would allocate additional resources, > + * for example when updating firmware. > + */ > + > if (status) > - goto err_detach_pm_domain; > + goto err_release_driver_resources; > > return 0; > > +err_release_driver_resources: > + devres_release_group(&client->dev, client->devres_group_id); > err_detach_pm_domain: > dev_pm_domain_detach(&client->dev, true); > err_clear_wakeup_irq: > @@ -563,6 +578,8 @@ static int i2c_device_remove(struct device *dev) > dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status)); > } > > + devres_release_group(&client->dev, client->devres_group_id); > + > dev_pm_domain_detach(&client->dev, true); > > dev_pm_clear_wake_irq(&client->dev); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 56622658b215..5d1f11c0deaa 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -306,6 +306,8 @@ struct i2c_driver { > * userspace_devices list > * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter > * calls it to pass on slave events to the slave driver. > + * @devres_group_id: id of the devres group that will be created for resources > + * acquired when probing this device. > * > * An i2c_client identifies a single device (i.e. chip) connected to an > * i2c bus. The behaviour exposed to Linux is defined by the driver > @@ -334,6 +336,7 @@ struct i2c_client { > #if IS_ENABLED(CONFIG_I2C_SLAVE) > i2c_slave_cb_t slave_cb; /* callback for slave mode */ > #endif > + void *devres_group_id; /* ID of probe devres group */ > }; > #define to_i2c_client(d) container_of(d, struct i2c_client, dev) >
On Sat, Mar 13, 2021 at 01:12:28PM -0600, Jeff LaBundy wrote: > Replace 'http' with 'https' and correct the spelling of the nearby > word 'datasheet'. > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you. -- Dmitry
On Sat, Mar 13, 2021 at 01:12:30PM -0600, Jeff LaBundy wrote: > Add the read-only 'fw_info' attribute which reports information > about the device's firmware in the following format: > > a.b.c.d:e.f > > Where: > > a = Product number (e.g. 40 for IQS550) > b = Project number (e.g. 15) > c = Firmware revision (major) > d = Firmware revision (minor) > e = Customer-assigned exported file version (major) > f = Customer-assigned exported file version (minor) > > As part of the corresponding rework to uses of 'bl_status', the > IQS5XX_BL_STATUS_RESET definition is dropped with 0 used in its > place instead. > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you. -- Dmitry
On Sat, Mar 13, 2021 at 01:12:31PM -0600, Jeff LaBundy wrote: > The vendor-assigned firmware project number is restricted to the > generic project number (15); however the vendor may assign other > project numbers to specific applications and customers. > > These custom project numbers may be based on forwards-compatible > firmware revision 1.x. However, the driver unnecessarily rejects > anything older than firmware revision 2.0. > > To support other applications, remove these unnecessarily strict > checks and enter the bootloader only for truly incompatible A000 > devices. > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you.