[v3,2/2] i2c: Add GPIO-based hotplug gate

Message ID 20230729160857.6332-3-clamor95@gmail.com
State New
Headers
Series GPIO-based hotplug i2c bus |

Commit Message

Svyatoslav Ryhel July 29, 2023, 4:08 p.m. UTC
  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.

This feature is mainly used by the ASUS Transformers family. The
Transformers have a connector that's used for USB, charging or for
attaching a dock-keyboard (which also has a battery and a touchpad).
This connector probably (can't be verified since no datasheets or
special equipment is available) has an I2C bus lines and a "detect"
line (pulled low on the dock side) among the pins. I guess there
is either no additional chip or a transparent bridge/buffer chip,
but nothing that could be controlled by software. For DT this setup
could be modelled like an I2C gate or 2-port mux with enable joining
two I2C buses (one "closer" to the CPU as a parent).

Co-developed-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Ion Agorria <ion@agorria.com>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/i2c/Kconfig            |  11 ++
 drivers/i2c/Makefile           |   1 +
 drivers/i2c/i2c-hotplug-gpio.c | 266 +++++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/i2c/i2c-hotplug-gpio.c
  

Comments

Andi Shyti July 30, 2023, 8:25 p.m. UTC | #1
Hi Svyatoslav,

(I'm not going to comment at this stage on some coding issues)

On Sat, Jul 29, 2023 at 07:08:57PM +0300, 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.
> 
> This feature is mainly used by the ASUS Transformers family. The

But not just Asus, right?

> Transformers have a connector that's used for USB, charging or for
> attaching a dock-keyboard (which also has a battery and a touchpad).
> This connector probably (can't be verified since no datasheets or
> special equipment is available) has an I2C bus lines and a "detect"
> line (pulled low on the dock side) among the pins. I guess there
> is either no additional chip or a transparent bridge/buffer chip,
> but nothing that could be controlled by software. For DT this setup
> could be modelled like an I2C gate or 2-port mux with enable joining
> two I2C buses (one "closer" to the CPU as a parent).

the description looks like it's hiding many doubts for a commit
log :)

In the commit log we want to be sure on what we are doing.

[...]

> +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)

there is no point for this to be "integer".

> +{
> +	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;

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?

> +	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);

can you explain this waiting and why 20ms?

Andi

> +	if (gpiod_get_value_cansleep(priv->gpio))
> +		i2c_hotplug_activate(priv);
> +	else
> +		i2c_hotplug_deactivate(priv);
> +
> +	return IRQ_HANDLED;
> +}
  
Krzysztof Kozlowski July 30, 2023, 8:30 p.m. UTC | #2
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.
> 
> This feature is mainly used by the ASUS Transformers family. The
> Transformers have a connector that's used for USB, charging or for
> attaching a dock-keyboard (which also has a battery and a touchpad).
> This connector probably (can't be verified since no datasheets or
> special equipment is available) has an I2C bus lines and a "detect"
> line (pulled low on the dock side) among the pins. I guess there
> is either no additional chip or a transparent bridge/buffer chip,
> but nothing that could be controlled by software. For DT this setup
> could be modelled like an I2C gate or 2-port mux with enable joining
> two I2C buses (one "closer" to the CPU as a parent).
> 
> Co-developed-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

...

> +	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,

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)
2. drop devm
3. drop shared flag.



Best regards,
Krzysztof
  
Michał Mirosław July 30, 2023, 9:55 p.m. UTC | #3
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.

Best Regards
Michał Mirosław
  
Michał Mirosław July 30, 2023, 10:11 p.m. UTC | #4
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.

> > +static irqreturn_t i2c_hotplug_interrupt(int irq, void *dev_id)
> > +{
> > +	struct i2c_hotplug_priv *priv = dev_id;
> > +
> > +	/* debounce */
> > +	msleep(20);
> can you explain this waiting and why 20ms?

It's an arbitrary time long enough to avoid having to handle multiple
on/off events that could happen when the dock is being inserted (ringing)
and short enough to not have to worry about the user getting impatient.

Best Regards
Michał Mirosław
  
Krzysztof Kozlowski July 31, 2023, 6:58 a.m. UTC | #5
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).

Best regards,
Krzysztof
  
Michał Mirosław July 31, 2023, 8:49 a.m. UTC | #6
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.)

Best Regards
Michał Mirosław
  
Krzysztof Kozlowski July 31, 2023, 12:59 p.m. UTC | #7
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.

Best regards,
Krzysztof
  
Michał Mirosław July 31, 2023, 10:50 p.m. UTC | #8
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
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

Best Regards
Michał Mirosław
  
Michał Mirosław July 31, 2023, 11:01 p.m. UTC | #9
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.

Best Regards
Michał Mirosław
  
Andi Shyti Aug. 4, 2023, 11:45 p.m. UTC | #10
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
  
Krzysztof Kozlowski Aug. 5, 2023, 7:17 p.m. UTC | #11
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
  
Michał Mirosław Aug. 10, 2023, 9:52 p.m. UTC | #12
On Sat, Aug 05, 2023 at 09:17:50PM +0200, Krzysztof Kozlowski wrote:
> 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)
[...]
> >> 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?

  3. TODO: DT-binding needs an expanded example and fixing some schema issues;

  4. question from Andi in another thread: I'll answer shortly.

Please correct me if I missed something.

Best Regards
Michał Mirosław

[1] https://en.wikipedia.org/wiki/Proof_by_example
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa11e38ce6fe8846fec046a95cecd5d4690c48cd
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f8a3e7fd5bd08e3fd9847c04a5a445e2994f6b3
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df0a2fdab0068f7452bf0a97ea9ba0ad69d49a1f
  
Michał Mirosław Aug. 10, 2023, 10:55 p.m. UTC | #13
On Sat, Aug 05, 2023 at 01:45:53AM +0200, Andi Shyti wrote:
> 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?
[...]

Not exactly.  There is an 'algorithm for this driver just forwards the calls to
the parent bus and has no use of the algo_data field.  The field is thus
used to store a flag noting whether the child bus was registered or not.

> And... thinking aloud... are there race conditions here? I
> mean... you can't attach two docking stations, but are there
> other scenarios?

The driver depends on I2C core code synchronization (e.g. i2c_del_adapter()
waiting for ongoing transfers). Outside of probe/remove there is only
a single thread used by the driver: the interrupt handler.

While reading to answer your question I noticed that IRQF_ONESHOT can be
removed: if the thread picks up the signal then it atomically clears the
trigger flag; if another signal arrives before the handler is done,
handler will be called again.

Best Regards,
Michał Mirosław
  

Patch

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");