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 PHC | |
Open Source and information security mailing list archives
| ||
|
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
Powered by blists - more mailing lists