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

[Krzysztof Kozlowski]

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@xxxxxxxxxxxx>
>>>>>
>>>>> 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