| Message ID | 20231011130204.52265-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | i2c: mux: don't access GPIOLIB internal structures | expand |
On Wed, Oct 11, 2023 at 4:59 PM Peter Rosin <peda@axentia.se> wrote: > > Hi! > > 2023-10-11 at 15:02, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Use the relevant API functions to retrieve the address of the > > underlying struct device instead of accessing GPIOLIB private structures > > manually. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/i2c/muxes/i2c-mux-gpio.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c > > index 5d5cbe0130cd..48a872a8196b 100644 > > --- a/drivers/i2c/muxes/i2c-mux-gpio.c > > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c > > @@ -14,8 +14,7 @@ > > #include <linux/slab.h> > > #include <linux/bits.h> > > #include <linux/gpio/consumer.h> > > -/* FIXME: stop poking around inside gpiolib */ > > -#include "../../gpio/gpiolib.h" > > +#include <linux/gpio/driver.h> > > > > struct gpiomux { > > struct i2c_mux_gpio_platform_data data; > > @@ -176,7 +175,8 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev) > > } > > > > for (i = 0; i < ngpios; i++) { > > - struct device *gpio_dev; > > + struct gpio_device *gdev; > > + struct device *dev; > > struct gpio_desc *gpiod; > > enum gpiod_flags flag; > > > > @@ -195,9 +195,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev) > > if (!muxc->mux_locked) > > continue; > > > > - /* FIXME: find a proper way to access the GPIO device */ > > - gpio_dev = &gpiod->gdev->dev; > > - muxc->mux_locked = i2c_root_adapter(gpio_dev) == root; > > + gdev = gpiod_to_gpio_device(gpiod); > > + dev = gpio_device_to_device(gdev); > > + muxc->mux_locked = i2c_root_adapter(dev) == root; > > } > > > > if (muxc->mux_locked) > > Very nice to see that wart gone! The only small question I have > is if these helpers are needed elsewhere, or if a more "direct" > gpiod_to_device() would have been sufficient? That said, I have > zero problem with this new code as-is, and that detail is of > course squarely in gpio-land. > > Acked-by: Peter Rosin <peda@axentia.se> gpiod_to_gpio_device() will be used in at least 10 other places. I haven't identified any other potential user for gpio_device_to_device() yet but I haven't looked hard yet either. Bart > > Cheers, > Peter
On Wed, Oct 11, 2023 at 5:59 PM Peter Rosin <peda@axentia.se> wrote: > 2023-10-11 at 15:02, Bartosz Golaszewski wrote: > > Use the relevant API functions to retrieve the address of the > > underlying struct device instead of accessing GPIOLIB private structures > > manually. > Very nice to see that wart gone! The only small question I have > is if these helpers are needed elsewhere, or if a more "direct" > gpiod_to_device() would have been sufficient? Same concern is here. But I'm fine with the code.
Hi Bartosz, > > > -/* FIXME: stop poking around inside gpiolib */ > > > -#include "../../gpio/gpiolib.h" > > > +#include <linux/gpio/driver.h> Hooray! \o/ > gpiod_to_gpio_device() will be used in at least 10 other places. I > haven't identified any other potential user for > gpio_device_to_device() yet but I haven't looked hard yet either. Same here. Looked a little bit, found nothing. Acked-by: Wolfram Sang <wsa@kernel.org>
Hi Bartosz, On Wed, Oct 11, 2023 at 3:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > The backstory for this short series is that we are identyfing and > removing all unauthorized uses of GPIOLIB structures across the kernel. > > For example: there are many users that access struct gpio_chip when the > only user allowed to safely do this is the provider of that chip. > > We are very close to removing gpiochip_find(). Another function that > poses a similar problem is gpiod_to_chip() which also returns the > address of the underlying gpio_chip without assuring that it will not go > away e.g. due to a hot-unplug event or a device unbind. > > We'll need to replace it with gpiod_to_gpio_device() across the entire > tree. Let's start by actually providing it and adding the first user: > the i2c-mux-gpio driver which dereferences the otherwise opaque struct > gpio_desc. > > Let's also add a helper that allows to retrieve the address of the > struct device backing the GPIO device as this is another valid use-case. > > Finally, let's un-include the GPIO private header and fix the code to > access the device in a safe way. > > As the change is pretty minor, it would be best if patch 3/3 could be > acked by the I2C mux maintainers and went through the GPIO tree. > Otherwise, I can apply patches 1 and 2 and provide an immutable branch. > > Bartosz Golaszewski (3): > gpiolib: provide gpio_device_to_device() > gpiolib: provide gpiod_to_gpio_device() > i2c: mux: gpio: don't fiddle with GPIOLIB internals The series: Acked-by: Linus Walleij <linus.walleij@linaro.org> Good job! Yours, Linus Walleij
On Wed, Oct 11, 2023 at 3:02 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > The backstory for this short series is that we are identyfing and > removing all unauthorized uses of GPIOLIB structures across the kernel. > > For example: there are many users that access struct gpio_chip when the > only user allowed to safely do this is the provider of that chip. > > We are very close to removing gpiochip_find(). Another function that > poses a similar problem is gpiod_to_chip() which also returns the > address of the underlying gpio_chip without assuring that it will not go > away e.g. due to a hot-unplug event or a device unbind. > > We'll need to replace it with gpiod_to_gpio_device() across the entire > tree. Let's start by actually providing it and adding the first user: > the i2c-mux-gpio driver which dereferences the otherwise opaque struct > gpio_desc. > > Let's also add a helper that allows to retrieve the address of the > struct device backing the GPIO device as this is another valid use-case. > > Finally, let's un-include the GPIO private header and fix the code to > access the device in a safe way. > > As the change is pretty minor, it would be best if patch 3/3 could be > acked by the I2C mux maintainers and went through the GPIO tree. > Otherwise, I can apply patches 1 and 2 and provide an immutable branch. > > Bartosz Golaszewski (3): > gpiolib: provide gpio_device_to_device() > gpiolib: provide gpiod_to_gpio_device() > i2c: mux: gpio: don't fiddle with GPIOLIB internals > > drivers/gpio/gpiolib.c | 38 ++++++++++++++++++++++++++++++++ > drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++----- > include/linux/gpio/driver.h | 3 +++ > 3 files changed, 47 insertions(+), 6 deletions(-) > > -- > 2.39.2 > Queued for v6.7. Bart
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> The backstory for this short series is that we are identyfing and removing all unauthorized uses of GPIOLIB structures across the kernel. For example: there are many users that access struct gpio_chip when the only user allowed to safely do this is the provider of that chip. We are very close to removing gpiochip_find(). Another function that poses a similar problem is gpiod_to_chip() which also returns the address of the underlying gpio_chip without assuring that it will not go away e.g. due to a hot-unplug event or a device unbind. We'll need to replace it with gpiod_to_gpio_device() across the entire tree. Let's start by actually providing it and adding the first user: the i2c-mux-gpio driver which dereferences the otherwise opaque struct gpio_desc. Let's also add a helper that allows to retrieve the address of the struct device backing the GPIO device as this is another valid use-case. Finally, let's un-include the GPIO private header and fix the code to access the device in a safe way. As the change is pretty minor, it would be best if patch 3/3 could be acked by the I2C mux maintainers and went through the GPIO tree. Otherwise, I can apply patches 1 and 2 and provide an immutable branch. Bartosz Golaszewski (3): gpiolib: provide gpio_device_to_device() gpiolib: provide gpiod_to_gpio_device() i2c: mux: gpio: don't fiddle with GPIOLIB internals drivers/gpio/gpiolib.c | 38 ++++++++++++++++++++++++++++++++ drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++----- include/linux/gpio/driver.h | 3 +++ 3 files changed, 47 insertions(+), 6 deletions(-)