linux-kernel - Re: [PATCH v2] i2c/muxes/i2c-mux-ltc4306: LTC4306 and LTC4305 I2C multiplexer/switch

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: <b5838bae-8108-1a0e-5234-d14cb5e95e52@axentia.se>
Date:   Tue, 4 Apr 2017 11:28:23 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     <michael.hennerich@...log.com>, <wsa@...-dreams.de>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <linus.walleij@...aro.org>
CC:     <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] i2c/muxes/i2c-mux-ltc4306: LTC4306 and LTC4305 I2C
 multiplexer/switch

*snip* *snip*

>>> +static int ltc4306_gpio_get(struct gpio_chip *chip, unsigned int offset)
>>> +{
>>> +	struct ltc4306 *data = gpiochip_get_data(chip);
>>> +	int ret = 0;
>>> +
>>> +	if (gpiochip_line_is_open_drain(chip, offset) ||
>>> +	    (data->regs[LTC_REG_MODE] & BIT(7 - offset))) {
>>
>> I wonder about this open-coded register cache. So, gpio people, is there
>> a guarantee from gpiolib that only one gpio_chip operation is in flight
>> concurrently? Because I don't see any evidence of that. With that in
>> mind, I think some locking is needed?
> 
> I thought there is a per chip mutex in the gpiolib. But I can't find 
> anything like this either. Since these two gpios can be used from 
> different internal or external users. The locking seem to be needed.
> 
> This gets us back to the regmap option. I did a quick grep, and 9 out of 
> 205 drivers using regmap i2c, also use i2c_smbus... concurrently.
> 
> grep -Rl regmap_init_i2c ./drivers  | xargs grep -l i2c_smbus_ | grep "\.c"
> 
> Mostly to work around non uniform transfer layouts.

I see three options.

1. Go with regmap and convert to mux-locked. Then the unlocked i2c-xfer
becomes an ordinary i2c-xfer (or smbus, whatever). This will result in
the cleanest code.

2. Go with regmap and stay parent-locked. Then hook into the regmap
locking as is done in one of the drivers that have worked around similar
problems with regmap and parent-locked i2c-mux interactions:

drivers/media/dvb-frontends/rtl2830.c
drivers/media/dvb-frontends/m88ds3103.c

This will probably work, but you'd need to add a number of extra helper
functions.

3. Exclude register 3 from regmap and only use regmap for the other
registers. This will be a bit ugly and ad-hoc, will need clear comments
on what is going on and why it is safe etc. And I want to see it before
I accept it. And it might not be my call to begin with, because TBH, it
sounds a bit disgusting...

> I'll check with Mark Brown on this topic.

Ok, might be a good idea...

>>> +
>>> +add_adapter_failed:
>>> +	i2c_mux_del_adapters(muxc);
>>> +gpio_default:
>>> +	gpiod_direction_input(data->en_gpio);
>>
>> This was actually not what I had in mind when I asked about it in v1, and
>> this looks a bit strange. You have no way of knowing if the pin was
>> configured as input when probe was called, and I don't see code like this
>> all over the place. Maybe it's is ok to not disable the chip over
>> suspend/resume, I was just asking because it looked a bit strange to grab
>> a pin and then forget about it. Now that I think about it some more, it's
>> probably ok to do just that since it is perhaps not possible to make the
>> chip draw less power by deasserting enable, but what do I know?
> 
> GPIOs are assumed by default inputs. So if you want to undo the actions 
> in probe. The logical consequence is to move them back to inputs, and 
> let the external PULL-UP or PULL-DOWN on the ENABLE decide what happens. 
> I would also prefer to leave it enabled, so that the GPIOs can retain

My point is that I do not see any probe functions undoing gpio configs.
Why bother in this case? Or are other probe functions really doing this?
I actually didn't check, but I haven't stumbled over it previously and
at least think I would have noticed...

> it's last state. Well I think the device draws a bit less power when 
> disabled. But we don't support runtime PM anyways.

It might not be safe to reset the gpio pins over a suspend/resume depending
on what they are used for, so it is probably a bad idea to go there. Sorry
for bringing the whole issue up and muddying the waters...

Cheers,
peda