| Message ID | 20230729160857.6332-3-clamor95@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | GPIO-based hotplug i2c bus | expand |
On Tue, Aug 01, 2023 at 01:01:43AM +0200, Michał Mirosław wrote: > On Mon, Jul 31, 2023 at 12:11:47AM +0200, Michał Mirosław wrote: > > On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote: > > > On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote: > > > > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv) > > [...] > > > > +{ > > > > + int ret; > > > > + > > > > + if (priv->adap.algo_data) > > > > + return 0; > > [...] > > > > + ret = i2c_add_adapter(&priv->adap); > > > > + if (!ret) > > > > + priv->adap.algo_data = (void *)1; > > > > > > You want to set algo_data to "1" in order to keep the > > > activate/deactivate ordering. > > > > > > But if we fail to add the adapter, what's the point to keep it > > > active? > > > > The code above does "if we added the adapter, remember we did so". > > IOW, if we failed to add the adapter we don't set the mark so that > > the next interrupt edge can trigger another try. Also we prevent > > trying to remove an adapter we didn't successfully add. > > Maybe the function's name is misleading? We could find a better one. > Activation/deactivation in this driver means "initialize/shutdown the > hotplugged bus" and is done in response to an edge (triggering an > interrupt) of the hotplug-detect signal. So that algo_data is randomly chosen as a boolean value given the fact that this particular driver doesn't have its own algorithms but it's using the ones from the parent. Right? If so, can we have a different and more meaningful boolean value for this? And... thinking aloud... are there race conditions here? I mean... you can't attach two docking stations, but are there other scenarios? Thanks, Andi
On 01/08/2023 00:50, Michał Mirosław wrote: > On Mon, Jul 31, 2023 at 02:59:41PM +0200, Krzysztof Kozlowski wrote: >> On 31/07/2023 10:49, Michał Mirosław wrote: >>> On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote: >>>> On 30/07/2023 23:55, Michał Mirosław wrote: >>>>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote: >>>>>> On 29/07/2023 18:08, Svyatoslav Ryhel wrote: >>>>>>> From: Michał Mirosław <mirq-linux@rere.qmqm.pl> >>>>>>> >>>>>>> Implement driver for hot-plugged I2C busses, where some devices on >>>>>>> a bus are hot-pluggable and their presence is indicated by GPIO line. >>>>> [...] >>>>>>> + priv->irq = platform_get_irq(pdev, 0); >>>>>>> + if (priv->irq < 0) >>>>>>> + return dev_err_probe(&pdev->dev, priv->irq, >>>>>>> + "failed to get IRQ %d\n", priv->irq); >>>>>>> + >>>>>>> + ret = devm_request_threaded_irq(&pdev->dev, priv->irq, NULL, >>>>>>> + i2c_hotplug_interrupt, >>>>>>> + IRQF_ONESHOT | IRQF_SHARED, >>>>>> >>>>>> Shared IRQ with devm is a recipe for disaster. Are you sure this is a >>>>>> shared one? You have a remove() function which also points that it is >>>>>> not safe. You can: >>>>>> 1. investigate to be sure it is 100% safe (please document why do you >>>>>> think it is safe) >>>>> >>>>> Could you elaborate on what is unsafe in using devm with shared >>>>> interrupts (as compared to non-shared or not devm-managed)? >>>>> >>>>> The remove function is indeed reversing the order of cleanup. The >>>>> shutdown path can be fixed by removing `remove()` and adding >>>>> `devm_add_action_or_reset(...deactivate)` before the IRQ is registered. >>>> Shared interrupt might be triggered easily by other device between >>>> remove() and irq release function (devm_free_irq() or whatever it is >>>> called). >>> >>> This is no different tham a non-shared interrupt that can be triggered >>> by the device being removed. Since devres will release the IRQ first, >>> before freeing the driver data, the interrupt hander will see consistent >>> driver-internal state. (The difference between remove() and devres >>> release phase is that for the latter sysfs files are already removed.) >> >> True, therefore non-devm interrupts are recommended also in such case. >> Maybe one of my solutions is actually not recommended. >> >> However if done right, driver with non-shared interrupts, is expected to >> disable interrupts in remove(), thus there is no risk. We have big >> discussions in the past about it, so feel free to dig through LKML to >> read more about. Anyway shared and devm is a clear no go. > > Can you share pointers to some of those discussions? Quick search > about devm_request_irq() and friends found only a thread from 2013 Just look at CONFIG_DEBUG_SHIRQ. Some things lore points: https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/ https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/ I think pretty clear: https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/ https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/ Also: https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/ https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/ > about conversions of RTC drivers to use devres. [1] IIRC the issue was > then that the drivers requested IRQs before fully initializing the state > (as many still do). Back to the original question: what is the risk > in using devres with shared interrupts? (Let's assume the probe() is already > fixed and remove() removed.) > > BTW, We have devres doc [2] in the kernel tree that, among other things, > lists IRQs as a managed resource and mentions no warnings nor restictions > for driver authors. I'd expect that if devm_request_threaded_irq() for > shared iterrupts was indeed deprecated, it should be documented in a way > easy to refer to. > > [1] https://groups.google.com/g/linux.kernel/c/yi2ueo-sNJs > [2] Documentation/udriver-api/driver-model/devres.rst That's not really an argument. For some reason we have CONFIG_DEBUG_SHIRQ, right? If you think documentation is missing, everyone is encouraged to fix it, but lack of documentation is not a proof of some correct code pattern. Best regards, Krzysztof
On 10/08/2023 23:52, Michał Mirosław wrote: >>>>>>>> Shared IRQ with devm is a recipe for disaster. Are you sure this is a >>>>>>>> shared one? You have a remove() function which also points that it is >>>>>>>> not safe. You can: >>>>>>>> 1. investigate to be sure it is 100% safe (please document why do you >>>>>>>> think it is safe) > [...] >>>> True, therefore non-devm interrupts are recommended also in such case. >>>> Maybe one of my solutions is actually not recommended. >>>> >>>> However if done right, driver with non-shared interrupts, is expected to >>>> disable interrupts in remove(), thus there is no risk. We have big >>>> discussions in the past about it, so feel free to dig through LKML to >>>> read more about. Anyway shared and devm is a clear no go. >>> >>> Can you share pointers to some of those discussions? Quick search >>> about devm_request_irq() and friends found only a thread from 2013 >> >> Just look at CONFIG_DEBUG_SHIRQ. Some things lore points: >> https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/ >> https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/ >> >> I think pretty clear: >> https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/ >> https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/ >> >> Also: >> https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/ >> https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/ > [...] > > Thanks! It all looks like a proof by example [1]: a broken driver [2] > was converted to devres [3] and allowed a shared interrupt [4] and now is > used to back an argument that devres and/or shared IRQs are bad. I have > a hard time accepting this line of reasoning. > > So: sure, if you disable device's clock, you should first disable the > interrupt handler one way or another, and if you request a shared interrupt > then you have to write the handler expecting spurious invocations anytime > between entry to register_irq() and return from free_irq() (BTW, DEBUG_SHIRQ > is here to help test exactly this). And, when used correctly, devres can > release you from having to write remove() and error paths (but I guess it > might be a challenge to find a single driver that is a complete, good and > complex-enough example). > > Coming back from the digression: I gathered following items from the > review of the i2c-hotplug-gpio driver: > > 1. TODO: register i2c_hotplug_deactivate(priv) using > devm_add_action_or_reset() before registering the IRQ handler > and remove remove(); > > 2. shared IRQ: it is expected to be an edge-triggered, rarely > signalled interrupt and the handler will work fine if called > spuriously; it is not required to be shared for my Transformer, > but I can't say much about other hardware. Would a comment help? We have way too lengthy discussion and now we are circling back. Can you refer to the first email I wrote? "You can: 1. investigate to be sure it is 100% safe (please document why do you think it is safe) 2. drop devm 3. drop shared flag." Best regards, Krzysztof
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 438905e2a1d0..3e3f7675ea4a 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -98,6 +98,17 @@ config I2C_SMBUS source "drivers/i2c/algos/Kconfig" source "drivers/i2c/busses/Kconfig" +config I2C_HOTPLUG_GPIO + tristate "Hot-plugged I2C bus detected by GPIO" + depends on GPIOLIB + depends on OF + help + If you say yes to this option, support will be included for + hot-plugging I2C devices with presence detected by GPIO pin value. + + This driver can also be built as a module. If so, the module + will be called i2c-hotplug-gpio. + config I2C_STUB tristate "I2C/SMBus Test Stub" depends on m diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index c1d493dc9bac..9fd44310835a 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o obj-$(CONFIG_I2C_MUX) += i2c-mux.o obj-y += algos/ busses/ muxes/ +obj-$(CONFIG_I2C_HOTPLUG_GPIO) += i2c-hotplug-gpio.o obj-$(CONFIG_I2C_STUB) += i2c-stub.o obj-$(CONFIG_I2C_SLAVE_EEPROM) += i2c-slave-eeprom.o obj-$(CONFIG_I2C_SLAVE_TESTUNIT) += i2c-slave-testunit.o diff --git a/drivers/i2c/i2c-hotplug-gpio.c b/drivers/i2c/i2c-hotplug-gpio.c new file mode 100644 index 000000000000..18c2d7f44d29 --- /dev/null +++ b/drivers/i2c/i2c-hotplug-gpio.c @@ -0,0 +1,266 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * I2C hotplug gate controlled by GPIO + */ + +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +struct i2c_hotplug_priv { + struct i2c_adapter adap; + struct i2c_adapter *parent; + struct device *dev; + struct gpio_desc *gpio; + int irq; +}; + +static inline struct i2c_adapter *i2c_hotplug_parent(struct i2c_adapter *adap) +{ + struct i2c_hotplug_priv *priv = container_of(adap, struct i2c_hotplug_priv, adap); + + return priv->parent; +} + +static int i2c_hotplug_master_xfer(struct i2c_adapter *adap, + struct i2c_msg msgs[], int num) +{ + struct i2c_adapter *parent = i2c_hotplug_parent(adap); + + return parent->algo->master_xfer(parent, msgs, num); +} + +static int i2c_hotplug_smbus_xfer(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int protocol, + union i2c_smbus_data *data) +{ + struct i2c_adapter *parent = i2c_hotplug_parent(adap); + + return parent->algo->smbus_xfer(parent, addr, flags, read_write, + command, protocol, data); +} + +static u32 i2c_hotplug_functionality(struct i2c_adapter *adap) +{ + u32 parent_func = i2c_get_functionality(i2c_hotplug_parent(adap)); + + return parent_func & ~I2C_FUNC_SLAVE; +} + +static const struct i2c_algorithm i2c_hotplug_algo_i2c = { + .master_xfer = i2c_hotplug_master_xfer, + .functionality = i2c_hotplug_functionality, +}; + +static const struct i2c_algorithm i2c_hotplug_algo_smbus = { + .smbus_xfer = i2c_hotplug_smbus_xfer, + .functionality = i2c_hotplug_functionality, +}; + +static const struct i2c_algorithm i2c_hotplug_algo_both = { + .master_xfer = i2c_hotplug_master_xfer, + .smbus_xfer = i2c_hotplug_smbus_xfer, + .functionality = i2c_hotplug_functionality, +}; + +static const struct i2c_algorithm *const i2c_hotplug_algo[2][2] = { + /* non-I2C */ + { NULL, &i2c_hotplug_algo_smbus }, + /* I2C */ + { &i2c_hotplug_algo_i2c, &i2c_hotplug_algo_both } +}; + +static void i2c_hotplug_lock_bus(struct i2c_adapter *adap, unsigned int flags) +{ + i2c_lock_bus(i2c_hotplug_parent(adap), flags); +} + +static int i2c_hotplug_trylock_bus(struct i2c_adapter *adap, + unsigned int flags) +{ + return i2c_trylock_bus(i2c_hotplug_parent(adap), flags); +} + +static void i2c_hotplug_unlock_bus(struct i2c_adapter *adap, + unsigned int flags) +{ + i2c_unlock_bus(i2c_hotplug_parent(adap), flags); +} + +static const struct i2c_lock_operations i2c_hotplug_lock_ops = { + .lock_bus = i2c_hotplug_lock_bus, + .trylock_bus = i2c_hotplug_trylock_bus, + .unlock_bus = i2c_hotplug_unlock_bus, +}; + +static int i2c_hotplug_recover_bus(struct i2c_adapter *adap) +{ + return i2c_recover_bus(i2c_hotplug_parent(adap)); +} + +static struct i2c_bus_recovery_info i2c_hotplug_recovery_info = { + .recover_bus = i2c_hotplug_recover_bus, +}; + +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv) +{ + int ret; + + if (priv->adap.algo_data) + return 0; + + /* + * Store the dev data in adapter dev, since + * previous i2c_del_adapter might have wiped it. + */ + priv->adap.dev.parent = priv->dev; + priv->adap.dev.of_node = priv->dev->of_node; + + dev_dbg(priv->adap.dev.parent, "connection detected"); + + ret = i2c_add_adapter(&priv->adap); + if (!ret) + priv->adap.algo_data = (void *)1; + + return ret; +} + +static void i2c_hotplug_deactivate(struct i2c_hotplug_priv *priv) +{ + if (!priv->adap.algo_data) + return; + + dev_dbg(priv->adap.dev.parent, "disconnection detected"); + + i2c_del_adapter(&priv->adap); + priv->adap.algo_data = NULL; +} + +static irqreturn_t i2c_hotplug_interrupt(int irq, void *dev_id) +{ + struct i2c_hotplug_priv *priv = dev_id; + + /* debounce */ + msleep(20); + + if (gpiod_get_value_cansleep(priv->gpio)) + i2c_hotplug_activate(priv); + else + i2c_hotplug_deactivate(priv); + + return IRQ_HANDLED; +} + +static void devm_i2c_put_adapter(void *adapter) +{ + i2c_put_adapter(adapter); +} + +static int i2c_hotplug_gpio_probe(struct platform_device *pdev) +{ + struct device_node *parent_np; + struct i2c_adapter *parent; + struct i2c_hotplug_priv *priv; + bool is_i2c, is_smbus; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + platform_set_drvdata(pdev, priv); + priv->dev = &pdev->dev; + + parent_np = of_parse_phandle(pdev->dev.of_node, "i2c-parent", 0); + if (IS_ERR(parent_np)) + return dev_err_probe(&pdev->dev, PTR_ERR(parent_np), + "cannot parse i2c-parent\n"); + + parent = of_find_i2c_adapter_by_node(parent_np); + of_node_put(parent_np); + if (IS_ERR(parent)) + return dev_err_probe(&pdev->dev, PTR_ERR(parent), + "failed to get parent I2C adapter\n"); + priv->parent = parent; + + ret = devm_add_action_or_reset(&pdev->dev, devm_i2c_put_adapter, + parent); + if (ret) + return ret; + + priv->gpio = devm_gpiod_get(&pdev->dev, "detect", GPIOD_IN); + if (IS_ERR(priv->gpio)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->gpio), + "failed to get detect GPIO\n"); + + is_i2c = parent->algo->master_xfer; + is_smbus = parent->algo->smbus_xfer; + + snprintf(priv->adap.name, sizeof(priv->adap.name), + "i2c-hotplug (master i2c-%d)", i2c_adapter_id(parent)); + priv->adap.owner = THIS_MODULE; + priv->adap.algo = i2c_hotplug_algo[is_i2c][is_smbus]; + priv->adap.algo_data = NULL; + priv->adap.lock_ops = &i2c_hotplug_lock_ops; + priv->adap.class = parent->class; + priv->adap.retries = parent->retries; + priv->adap.timeout = parent->timeout; + priv->adap.quirks = parent->quirks; + if (parent->bus_recovery_info) + priv->adap.bus_recovery_info = &i2c_hotplug_recovery_info; + + if (!priv->adap.algo) + return -EINVAL; + + priv->irq = platform_get_irq(pdev, 0); + if (priv->irq < 0) + return dev_err_probe(&pdev->dev, priv->irq, + "failed to get IRQ %d\n", priv->irq); + + ret = devm_request_threaded_irq(&pdev->dev, priv->irq, NULL, + i2c_hotplug_interrupt, + IRQF_ONESHOT | IRQF_SHARED, + "i2c-hotplug", priv); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "failed to register IRQ %d\n", priv->irq); + + irq_wake_thread(priv->irq, priv); + + return 0; +} + +static int i2c_hotplug_gpio_remove(struct platform_device *pdev) +{ + struct i2c_hotplug_priv *priv = platform_get_drvdata(pdev); + + i2c_hotplug_deactivate(priv); + + return 0; +} + +static const struct of_device_id i2c_hotplug_gpio_of_match[] = { + { .compatible = "i2c-hotplug-gpio" }, + {}, +}; +MODULE_DEVICE_TABLE(of, i2c_hotplug_gpio_of_match); + +static struct platform_driver i2c_hotplug_gpio_driver = { + .probe = i2c_hotplug_gpio_probe, + .remove = i2c_hotplug_gpio_remove, + .driver = { + .name = "i2c-hotplug-gpio", + .of_match_table = i2c_hotplug_gpio_of_match, + }, +}; + +module_platform_driver(i2c_hotplug_gpio_driver); + +MODULE_DESCRIPTION("Hot-plugged I2C bus detected by GPIO"); +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>"); +MODULE_LICENSE("GPL");