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: <ZMbgIovV7lxlgd5T@qmqm.qmqm.pl>
Date:   Mon, 31 Jul 2023 00:11:46 +0200
From:   Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:     Andi Shyti <andi.shyti@...nel.org>
Cc:     Svyatoslav Ryhel <clamor95@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wolfram Sang <wsa@...nel.org>, 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 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