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

[Krzysztof Kozlowski]

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.
> 
> 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@xxxxxxxxxxx>
> Signed-off-by: Ion Agorria <ion@xxxxxxxxxxx>
> Signed-off-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>

...

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