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] [day] [month] [year] [list]

Date:   Wed, 5 Apr 2017 14:21:17 +0200
From:   Michael Hennerich <michael.hennerich@...log.com>
To:     Peter Rosin <peda@...ntia.se>, <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

On 04.04.2017 11:28, Peter Rosin wrote:
> *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.

ok - you convinced me.

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

I restore the original implementation and also pulse the ENABLE low so 
we're always doing a clean reset.

I'll send a new patch shortly.

Thanks!

-- 
Greetings,
Michael

--
Analog Devices GmbH      Otl-Aicher Strasse 60-64      80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne