| Message ID | 9b0375fd-235f-4ee1-a7fa-daca296ef6bf@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-pci-idio-16 regression after LTS upgrade | expand |
On Mon, Oct 06, 2025 at 09:37:14AM +0100, Mark Cave-Ayland wrote: > root@debian12:~# gpioget 0 0 > gpioget: error reading GPIO values: Input/output error > > which also output: > > [ 329.529321] gpio-512 (gpioget): gpiod_direction_input: missing > direction_input() operation and line is output > > My guess is that this is because > drivers/gpio/gpio-regmap.c::gpio_regmap_get_direction() isn't able to > can't handle the situation where lines 0-15 are outputs and lines 16-31 > are inputs, compared with the old idio_16_gpio_get_direction() function > it replaced. > > What would be the best way forward? Possibly add the .get_direction > callback to the gpio_regmap_config? Or is there another way to have > mixed inputs and outputs with the gpio_regmap API? So the intention I had with gpio-idio-16 was to provide reg_dat_base and reg_set_base to define the input and output bases, and then reg_mask_xlate would do the translation between input and outputs. I think this design is allowed by gpio-regmap, is it not Michael? In theory, gpio_regmap_get_direction should call gpio->reg_mask_xlate() which is mapped to idio_16_reg_mask_xlate(), and thus set reg and mask which then is evaluated at the end of gpio_regmap_get_direction() to determine which direction to return. Is it possible idio_16_reg_mask_xlate() is returning the wrong values for reg and mask? William Breathitt Gray
On 07/10/2025 09:16, William Breathitt Gray wrote: > On Mon, Oct 06, 2025 at 09:37:14AM +0100, Mark Cave-Ayland wrote: >> root@debian12:~# gpioget 0 0 >> gpioget: error reading GPIO values: Input/output error >> >> which also output: >> >> [ 329.529321] gpio-512 (gpioget): gpiod_direction_input: missing >> direction_input() operation and line is output >> >> My guess is that this is because >> drivers/gpio/gpio-regmap.c::gpio_regmap_get_direction() isn't able to >> can't handle the situation where lines 0-15 are outputs and lines 16-31 >> are inputs, compared with the old idio_16_gpio_get_direction() function >> it replaced. >> >> What would be the best way forward? Possibly add the .get_direction >> callback to the gpio_regmap_config? Or is there another way to have >> mixed inputs and outputs with the gpio_regmap API? > > So the intention I had with gpio-idio-16 was to provide reg_dat_base and > reg_set_base to define the input and output bases, and then > reg_mask_xlate would do the translation between input and outputs. I > think this design is allowed by gpio-regmap, is it not Michael? > > In theory, gpio_regmap_get_direction should call gpio->reg_mask_xlate() > which is mapped to idio_16_reg_mask_xlate(), and thus set reg and mask > which then is evaluated at the end of gpio_regmap_get_direction() to > determine which direction to return. > > Is it possible idio_16_reg_mask_xlate() is returning the wrong values > for reg and mask? > > William Breathitt Gray The only logic around .reg_dat_base and .reg_set_base in gpio_regmap_get_direction() is this: if (gpio->reg_dat_base && !gpio->reg_set_base) return GPIO_LINE_DIRECTION_IN; if (gpio->reg_set_base && !gpio->reg_dat_base) return GPIO_LINE_DIRECTION_OUT; Otherwise it attempts to use .reg_dir_out_base and .reg_dir_in_base which are not set for gpio-idio-16 because the GPIO directions are fixed and not controlled via a data-direction register. And as these are not set, gpio_regmap_get_direction() returns -ENOTSUPP. Were you thinking that gpio_regmap_get_direction() should have some additional direction logic if both .reg_dat_base and .reg_set_base are set, based upon their comparative values? ATB, Mark.
On Thu, Oct 09, 2025 at 10:05:58AM +0100, Mark Cave-Ayland wrote: > On 07/10/2025 09:16, William Breathitt Gray wrote: > > So the intention I had with gpio-idio-16 was to provide reg_dat_base and > > reg_set_base to define the input and output bases, and then > > reg_mask_xlate would do the translation between input and outputs. I > > think this design is allowed by gpio-regmap, is it not Michael? > > > > In theory, gpio_regmap_get_direction should call gpio->reg_mask_xlate() > > which is mapped to idio_16_reg_mask_xlate(), and thus set reg and mask > > which then is evaluated at the end of gpio_regmap_get_direction() to > > determine which direction to return. > > > > Is it possible idio_16_reg_mask_xlate() is returning the wrong values > > for reg and mask? > > > > William Breathitt Gray > > The only logic around .reg_dat_base and .reg_set_base in > gpio_regmap_get_direction() is this: > > if (gpio->reg_dat_base && !gpio->reg_set_base) > return GPIO_LINE_DIRECTION_IN; > if (gpio->reg_set_base && !gpio->reg_dat_base) > return GPIO_LINE_DIRECTION_OUT; > > Otherwise it attempts to use .reg_dir_out_base and .reg_dir_in_base > which are not set for gpio-idio-16 because the GPIO directions are fixed > and not controlled via a data-direction register. And as these are not > set, gpio_regmap_get_direction() returns -ENOTSUPP. > > Were you thinking that gpio_regmap_get_direction() should have some > additional direction logic if both .reg_dat_base and .reg_set_base are > set, based upon their comparative values? Ah you're right, I misunderstood the logic in gpio_regmap_get_direction. So essentially the problem is that gpio-idio-16 has no way of indicating the direction of its I/O because it's mixed. The IDIO-16 series lacks a direction setting register, so I think the proper solution is as you suggest: add support for a get_direction callback to struct gpio_regmap_config in the same vein as the existing reg_mask_xlate callback. Then in gpio_regmap_register you can set gpio->get_direction = config->get_direction in the same way config->reg_mask_xlate is handled. William Breathitt Gray
On Mon, Oct 06, 2025 at 09:37:14AM +0100, Mark Cave-Ayland wrote: > After some more debugging I was able to determine that the failure was > due to the regmap cache failing initialisation in > drivers/base/regmap/regcache-flat.c::regcache_flat_init() because > max_register wasn't set on the regmap. I was able to fix that fairly > easily with this: > > > diff --git a/drivers/gpio/gpio-pci-idio-16.c > b/drivers/gpio/gpio-pci-idio-16.c > index 44c0a21b1d1d..55be571b5cca 100644 > --- a/drivers/gpio/gpio-pci-idio-16.c > +++ b/drivers/gpio/gpio-pci-idio-16.c > @@ -41,6 +41,7 @@ static const struct regmap_config > idio_16_regmap_config = { > .reg_stride = 1, > .val_bits = 8, > .io_port = true, > + .max_register = 0x7, > .wr_table = &idio_16_wr_table, > .rd_table = &idio_16_rd_table, > .volatile_table = &idio_16_rd_table, This particular failure is separate from the get_direction issue discovered after. It would be good to have this fix as its own patch so we can keep each solution dedicated to their respective failures and streamline any necessary backports to the stable trees. I have some travel scheduled in the next couple days, but I can probably get a patch series addressing both issues ready by the end of the week. I'll also CC the stable mailing list so we can get the fixes picked up for the LTS kernel they are affecting. William Breathitt Gray
On 12/10/2025 15:22, William Breathitt Gray wrote: > On Mon, Oct 06, 2025 at 09:37:14AM +0100, Mark Cave-Ayland wrote: >> After some more debugging I was able to determine that the failure was >> due to the regmap cache failing initialisation in >> drivers/base/regmap/regcache-flat.c::regcache_flat_init() because >> max_register wasn't set on the regmap. I was able to fix that fairly >> easily with this: >> >> >> diff --git a/drivers/gpio/gpio-pci-idio-16.c >> b/drivers/gpio/gpio-pci-idio-16.c >> index 44c0a21b1d1d..55be571b5cca 100644 >> --- a/drivers/gpio/gpio-pci-idio-16.c >> +++ b/drivers/gpio/gpio-pci-idio-16.c >> @@ -41,6 +41,7 @@ static const struct regmap_config >> idio_16_regmap_config = { >> .reg_stride = 1, >> .val_bits = 8, >> .io_port = true, >> + .max_register = 0x7, >> .wr_table = &idio_16_wr_table, >> .rd_table = &idio_16_rd_table, >> .volatile_table = &idio_16_rd_table, > > This particular failure is separate from the get_direction issue > discovered after. It would be good to have this fix as its own patch so > we can keep each solution dedicated to their respective failures and > streamline any necessary backports to the stable trees. Agreed. > I have some travel scheduled in the next couple days, but I can > probably get a patch series addressing both issues ready by the end of > the week. I'll also CC the stable mailing list so we can get the fixes > picked up for the LTS kernel they are affecting. That would be fantastic! I was hoping to try and look at this over the past week, but it got delayed due to other work. Regardless of this I've still got access to the debugging environment to test any patches if you add me on CC. If it helps the implementation we're using is part of the libvfio-user test suite (see https://github.com/nutanix/libvfio-user and samples/gpio-pci-idio-16.c). Internally we run a script that launches a QEMU VM, reads and writes a few sample gpios and then checks the logging to ensure that the basic vfio-user protocol is working as expected. ATB, Mark.
Hi, On Thu Oct 9, 2025 at 11:49 AM CEST, William Breathitt Gray wrote: > On Thu, Oct 09, 2025 at 10:05:58AM +0100, Mark Cave-Ayland wrote: >> On 07/10/2025 09:16, William Breathitt Gray wrote: >> > So the intention I had with gpio-idio-16 was to provide reg_dat_base and >> > reg_set_base to define the input and output bases, and then >> > reg_mask_xlate would do the translation between input and outputs. I >> > think this design is allowed by gpio-regmap, is it not Michael? >> > >> > In theory, gpio_regmap_get_direction should call gpio->reg_mask_xlate() >> > which is mapped to idio_16_reg_mask_xlate(), and thus set reg and mask >> > which then is evaluated at the end of gpio_regmap_get_direction() to >> > determine which direction to return. >> > >> > Is it possible idio_16_reg_mask_xlate() is returning the wrong values >> > for reg and mask? >> > >> > William Breathitt Gray >> >> The only logic around .reg_dat_base and .reg_set_base in >> gpio_regmap_get_direction() is this: >> >> if (gpio->reg_dat_base && !gpio->reg_set_base) >> return GPIO_LINE_DIRECTION_IN; >> if (gpio->reg_set_base && !gpio->reg_dat_base) >> return GPIO_LINE_DIRECTION_OUT; >> >> Otherwise it attempts to use .reg_dir_out_base and .reg_dir_in_base >> which are not set for gpio-idio-16 because the GPIO directions are fixed >> and not controlled via a data-direction register. And as these are not >> set, gpio_regmap_get_direction() returns -ENOTSUPP. >> >> Were you thinking that gpio_regmap_get_direction() should have some >> additional direction logic if both .reg_dat_base and .reg_set_base are >> set, based upon their comparative values? > > Ah you're right, I misunderstood the logic in gpio_regmap_get_direction. > So essentially the problem is that gpio-idio-16 has no way of indicating > the direction of its I/O because it's mixed. > > The IDIO-16 series lacks a direction setting register, so I think the > proper solution is as you suggest: add support for a get_direction > callback to struct gpio_regmap_config in the same vein as the existing > reg_mask_xlate callback. Then in gpio_regmap_register you can set > gpio->get_direction = config->get_direction in the same way > config->reg_mask_xlate is handled. IIUC the chip has fixed input and outputs. In that case this should help: https://lore.kernel.org/all/20250922142427.3310221-8-ioana.ciornei@nxp.com/ I guess Ioana will resubmit the series for this cycle. To fix the regression, I guess the patch can then be picked up by the stable team along with the driver patch which will set the .fixed_direction_output config field. -michael
On Tue, Oct 14, 2025 at 11:03:25AM +0200, Michael Walle wrote: > Hi, > > On Thu Oct 9, 2025 at 11:49 AM CEST, William Breathitt Gray wrote: > > On Thu, Oct 09, 2025 at 10:05:58AM +0100, Mark Cave-Ayland wrote: > >> On 07/10/2025 09:16, William Breathitt Gray wrote: > >> > So the intention I had with gpio-idio-16 was to provide reg_dat_base and > >> > reg_set_base to define the input and output bases, and then > >> > reg_mask_xlate would do the translation between input and outputs. I > >> > think this design is allowed by gpio-regmap, is it not Michael? > >> > > >> > In theory, gpio_regmap_get_direction should call gpio->reg_mask_xlate() > >> > which is mapped to idio_16_reg_mask_xlate(), and thus set reg and mask > >> > which then is evaluated at the end of gpio_regmap_get_direction() to > >> > determine which direction to return. > >> > > >> > Is it possible idio_16_reg_mask_xlate() is returning the wrong values > >> > for reg and mask? > >> > > >> > William Breathitt Gray > >> > >> The only logic around .reg_dat_base and .reg_set_base in > >> gpio_regmap_get_direction() is this: > >> > >> if (gpio->reg_dat_base && !gpio->reg_set_base) > >> return GPIO_LINE_DIRECTION_IN; > >> if (gpio->reg_set_base && !gpio->reg_dat_base) > >> return GPIO_LINE_DIRECTION_OUT; > >> > >> Otherwise it attempts to use .reg_dir_out_base and .reg_dir_in_base > >> which are not set for gpio-idio-16 because the GPIO directions are fixed > >> and not controlled via a data-direction register. And as these are not > >> set, gpio_regmap_get_direction() returns -ENOTSUPP. > >> > >> Were you thinking that gpio_regmap_get_direction() should have some > >> additional direction logic if both .reg_dat_base and .reg_set_base are > >> set, based upon their comparative values? > > > > Ah you're right, I misunderstood the logic in gpio_regmap_get_direction. > > So essentially the problem is that gpio-idio-16 has no way of indicating > > the direction of its I/O because it's mixed. > > > > The IDIO-16 series lacks a direction setting register, so I think the > > proper solution is as you suggest: add support for a get_direction > > callback to struct gpio_regmap_config in the same vein as the existing > > reg_mask_xlate callback. Then in gpio_regmap_register you can set > > gpio->get_direction = config->get_direction in the same way > > config->reg_mask_xlate is handled. > > IIUC the chip has fixed input and outputs. In that case this should > help: > https://lore.kernel.org/all/20250922142427.3310221-8-ioana.ciornei@nxp.com/ > > I guess Ioana will resubmit the series for this cycle. Yes, I will resubmit shortly. > To fix the regression, I guess the patch can then be picked up by the > stable team along with the driver patch which will set the > .fixed_direction_output config field. Just to make sure that I understand, do you want me to add another patch that adds the new driver user or that will be handled separately? Ioana
On Tue Oct 14, 2025 at 1:21 PM CEST, Ioana Ciornei wrote: >> To fix the regression, I guess the patch can then be picked up by the >> stable team along with the driver patch which will set the >> .fixed_direction_output config field. > > Just to make sure that I understand, do you want me to add another patch > that adds the new driver user or that will be handled separately? No, no. No new patch. It's just that your patch (just the one which has that property added to gpio-regmap) has to be submitted to the stable kernels (by the one who is fixing the regression) I guess. -michael
On Tue, Oct 14, 2025 at 01:25:24PM +0200, Michael Walle wrote: > On Tue Oct 14, 2025 at 1:21 PM CEST, Ioana Ciornei wrote: > >> To fix the regression, I guess the patch can then be picked up by the > >> stable team along with the driver patch which will set the > >> .fixed_direction_output config field. > > > > Just to make sure that I understand, do you want me to add another patch > > that adds the new driver user or that will be handled separately? > > No, no. No new patch. It's just that your patch (just the one which > has that property added to gpio-regmap) has to be submitted to the stable > kernels (by the one who is fixing the regression) I guess. Ok, understood. Ioana
diff --git a/drivers/gpio/gpio-pci-idio-16.c b/drivers/gpio/gpio-pci-idio-16.c index 44c0a21b1d1d..55be571b5cca 100644 --- a/drivers/gpio/gpio-pci-idio-16.c +++ b/drivers/gpio/gpio-pci-idio-16.c @@ -41,6 +41,7 @@ static const struct regmap_config idio_16_regmap_config = { .reg_stride = 1, .val_bits = 8, .io_port = true, + .max_register = 0x7, .wr_table = &idio_16_wr_table, .rd_table = &idio_16_rd_table,
Hi all, As part of our internal vfio testing here at Nutanix we make use of an emulated gpio-pci-idio-16 device. Recently we've upgraded our LTS kernel and found that the device stops working which I've bisected down to this commit: $ git bisect bad 73d8f3efc5c2b757ab06685741df01eaed8090c4 is the first bad commit commit 73d8f3efc5c2b757ab06685741df01eaed8090c4 Author: William Breathitt Gray <william.gray@linaro.org> Date: Thu Aug 10 18:00:40 2023 -0400 gpio: pci-idio-16: Migrate to the regmap API The regmap API supports IO port accessors so we can take advantage of regmap abstractions rather than handling access to the device registers directly in the driver. Migrate the pci-idio-16 module to the new idio-16 library interface leveraging the gpio-regmap API. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Link: https://lore.kernel.org/r/5ba5405c64aca984d5cf3bdbdffa04c325e5a147.1680618405.git.william.gray@linaro.org/ Signed-off-by: William Breathitt Gray <william.gray@linaro.org> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> drivers/gpio/Kconfig | 2 +- drivers/gpio/gpio-pci-idio-16.c | 294 +++++++++------------------------------- 2 files changed, 62 insertions(+), 234 deletions(-) With this commit when the gpio-pci-idio16 module fails upon load as below: [ 0.266606] pci-idio-16 0000:00:03.0: error -EINVAL: Unable to initialize register map [ 0.266867] pci-idio-16: probe of 0000:00:03.0 failed with error -22 After some more debugging I was able to determine that the failure was due to the regmap cache failing initialisation in drivers/base/regmap/regcache-flat.c::regcache_flat_init() because max_register wasn't set on the regmap. I was able to fix that fairly easily with this: .volatile_table = &idio_16_rd_table, Okay so the module now loads, but all of my gpios are now inputs! root@debian12:~# gpioinfo gpiochip0 - 32 lines: line 0: "OUT0" unused input active-high line 1: "OUT1" unused input active-high line 2: "OUT2" unused input active-high line 3: "OUT3" unused input active-high line 4: "OUT4" unused input active-high line 5: "OUT5" unused input active-high line 6: "OUT6" unused input active-high line 7: "OUT7" unused input active-high line 8: "OUT8" unused input active-high line 9: "OUT9" unused input active-high line 10: "OUT10" unused input active-high line 11: "OUT11" unused input active-high line 12: "OUT12" unused input active-high line 13: "OUT13" unused input active-high line 14: "OUT14" unused input active-high line 15: "OUT15" unused input active-high line 16: "IIN0" unused input active-high line 17: "IIN1" unused input active-high line 18: "IIN2" unused input active-high line 19: "IIN3" unused input active-high line 20: "IIN4" unused input active-high line 21: "IIN5" unused input active-high line 22: "IIN6" unused input active-high line 23: "IIN7" unused input active-high line 24: "IIN8" unused input active-high line 25: "IIN9" unused input active-high line 26: "IIN10" unused input active-high line 27: "IIN11" unused input active-high line 28: "IIN12" unused input active-high line 29: "IIN13" unused input active-high line 30: "IIN14" unused input active-high line 31: "IIN15" unused input active-high root@debian12:~# gpioget 0 0 gpioget: error reading GPIO values: Input/output error which also output: [ 329.529321] gpio-512 (gpioget): gpiod_direction_input: missing direction_input() operation and line is output My guess is that this is because drivers/gpio/gpio-regmap.c::gpio_regmap_get_direction() isn't able to can't handle the situation where lines 0-15 are outputs and lines 16-31 are inputs, compared with the old idio_16_gpio_get_direction() function it replaced. What would be the best way forward? Possibly add the .get_direction callback to the gpio_regmap_config? Or is there another way to have mixed inputs and outputs with the gpio_regmap API? Many thanks, Mark.