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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25858c22-ef92-2136-67ef-0d27364c1600@linaro.org>
Date:   Sun, 30 Jul 2023 22:30:56 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Svyatoslav Ryhel <clamor95@...il.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate

On 29/07/2023 18:08, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@...e.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@...rria.com>
> Signed-off-by: Ion Agorria <ion@...rria.com>
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.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