* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-08 14:16 ` Markus Pargmann 0 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-08 14:16 UTC (permalink / raw) To: Linus Walleij Cc: Shawn Guo, linux-arm-kernel, kernel, Markus Pargmann, stable The pins do not have a 1:1 mapping from index to pin_id. Unfortunately the debug output assumes exactly that. The first driver using imx1-core was imx27, which had exactly this 1:1 mapping. It was accidently removed when removing all unused pads which were listed: 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) The patch fixes this issue by printing the pin_id directly and not the pin name. Cc: <stable@vger.kernel.org> Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/pinctrl/freescale/pinctrl-imx1-core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c index 5fd4437cee15..f08858769167 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c @@ -403,14 +403,12 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, unsigned num_configs) { struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); - const struct imx1_pinctrl_soc_info *info = ipctl->info; int i; for (i = 0; i != num_configs; ++i) { imx1_write_bit(ipctl, pin_id, configs[i] & 0x01, MX1_PUEN); - dev_dbg(ipctl->dev, "pinconf set pullup pin %s\n", - info->pins[pin_id].name); + dev_dbg(ipctl->dev, "pinconf set pullup pin %d\n", pin_id); } return 0; -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-08 14:16 ` Markus Pargmann 0 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-08 14:16 UTC (permalink / raw) To: linux-arm-kernel The pins do not have a 1:1 mapping from index to pin_id. Unfortunately the debug output assumes exactly that. The first driver using imx1-core was imx27, which had exactly this 1:1 mapping. It was accidently removed when removing all unused pads which were listed: 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) The patch fixes this issue by printing the pin_id directly and not the pin name. Cc: <stable@vger.kernel.org> Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/pinctrl/freescale/pinctrl-imx1-core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c index 5fd4437cee15..f08858769167 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c @@ -403,14 +403,12 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, unsigned num_configs) { struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); - const struct imx1_pinctrl_soc_info *info = ipctl->info; int i; for (i = 0; i != num_configs; ++i) { imx1_write_bit(ipctl, pin_id, configs[i] & 0x01, MX1_PUEN); - dev_dbg(ipctl->dev, "pinconf set pullup pin %s\n", - info->pins[pin_id].name); + dev_dbg(ipctl->dev, "pinconf set pullup pin %d\n", pin_id); } return 0; -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-08 14:16 ` Markus Pargmann @ 2015-07-09 7:12 ` Uwe Kleine-König -1 siblings, 0 replies; 14+ messages in thread From: Uwe Kleine-König @ 2015-07-09 7:12 UTC (permalink / raw) To: Markus Pargmann Cc: Linus Walleij, stable, Shawn Guo, kernel, linux-arm-kernel, Chris Ruehl Hello Markus, Cc += Chris Ruehl On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: > The pins do not have a 1:1 mapping from index to pin_id. Unfortunately > the debug output assumes exactly that. > > The first driver using imx1-core was imx27, which had exactly this 1:1 > mapping. It was accidently removed when removing all unused pads which > were listed: > 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) > > The patch fixes this issue by printing the pin_id directly and not the > pin name. Knowing a bit about the imx pinctrl drivers I failed to understand what you wrote here. Probably because I first though that "1:1 mapping" is a hardware property. What about: Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback imx1_pinconf_set assumes that the array of pins in struct imx1_pinctrl_soc_info can be indexed by pin id to get the pinctrl_pin_desc for a pin. This used to be correct up to commit 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") which removed some entries from the array and so made it wrong to access the array by pin id. Implement the easiest fix by not resolving the pin id to a name but printing the id instead. Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") Cc: stable@vger.kernel.org Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> Having the pad name in the output is nice, is it worth to search for the right pinctrl_pin_desc in the array? The array is still sorted, so a binary search would do, maybe a function for this already exists? How did Chris notice the error? Just a bogus output, or did it crash the kernel? That would be worth to note in the commit log, too. Otherwise the change looks fine. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K�nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-09 7:12 ` Uwe Kleine-König 0 siblings, 0 replies; 14+ messages in thread From: Uwe Kleine-König @ 2015-07-09 7:12 UTC (permalink / raw) To: linux-arm-kernel Hello Markus, Cc += Chris Ruehl On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: > The pins do not have a 1:1 mapping from index to pin_id. Unfortunately > the debug output assumes exactly that. > > The first driver using imx1-core was imx27, which had exactly this 1:1 > mapping. It was accidently removed when removing all unused pads which > were listed: > 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) > > The patch fixes this issue by printing the pin_id directly and not the > pin name. Knowing a bit about the imx pinctrl drivers I failed to understand what you wrote here. Probably because I first though that "1:1 mapping" is a hardware property. What about: Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback imx1_pinconf_set assumes that the array of pins in struct imx1_pinctrl_soc_info can be indexed by pin id to get the pinctrl_pin_desc for a pin. This used to be correct up to commit 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") which removed some entries from the array and so made it wrong to access the array by pin id. Implement the easiest fix by not resolving the pin id to a name but printing the id instead. Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") Cc: stable at vger.kernel.org Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> Having the pad name in the output is nice, is it worth to search for the right pinctrl_pin_desc in the array? The array is still sorted, so a binary search would do, maybe a function for this already exists? How did Chris notice the error? Just a bogus output, or did it crash the kernel? That would be worth to note in the commit log, too. Otherwise the change looks fine. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-09 7:12 ` Uwe Kleine-König @ 2015-07-09 8:28 ` Chris Ruehl -1 siblings, 0 replies; 14+ messages in thread From: Chris Ruehl @ 2015-07-09 8:28 UTC (permalink / raw) To: Uwe Kleine-König, Markus Pargmann Cc: Linus Walleij, stable, Shawn Guo, kernel, linux-arm-kernel Uwe, On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-K�nig wrote: > Hello Markus, > > Cc += Chris Ruehl > > On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: >> The pins do not have a 1:1 mapping from index to pin_id. Unfortunately >> the debug output assumes exactly that. >> >> The first driver using imx1-core was imx27, which had exactly this 1:1 >> mapping. It was accidently removed when removing all unused pads which >> were listed: >> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) >> >> The patch fixes this issue by printing the pin_id directly and not the >> pin name. > Knowing a bit about the imx pinctrl drivers I failed to understand what > you wrote here. Probably because I first though that "1:1 mapping" is a > hardware property. What about: > > Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback > > imx1_pinconf_set assumes that the array of pins in struct > imx1_pinctrl_soc_info can be indexed by pin id to get the > pinctrl_pin_desc for a pin. This used to be correct up to commit > 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > which removed some entries from the array and so made it wrong to access > the array by pin id. > > Implement the easiest fix by not resolving the pin id to a name but > printing the id instead. > > Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > Cc: stable@vger.kernel.org > Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Having the pad name in the output is nice, is it worth to search for the > right pinctrl_pin_desc in the array? The array is still sorted, so a > binary search would do, maybe a function for this already exists? > > How did Chris notice the error? Just a bogus output, or did it crash the > kernel? That would be worth to note in the commit log, too. > > Otherwise the change looks fine. > > Best regards > Uwe I had a crash on a array overrun. I'd applied a patch to the linux-usb (which was spit out by the maintainer perl script) chris@wheezyvm:~/kernel.d/linux-next$ cat 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 From: Chris Ruehl <chris.ruehl@gtsys.com.hk> Date: Sat, 23 May 2015 15:02:44 +0800 Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving Bug in function imx1_pinconf_set() cause crash when princtrl debug is enabled and the pin_id becomes larger then the info->pins[] contains. imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, direction 0, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, direction 0, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD ... imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 Unable to handle kernel paging request at virtual address 6c61765f pgd = c0004000 6c61765f] *pgd=00000000 Internal error: Oops: 5 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) task: ce832000 ti: ce848000 task.ti: ce848000 PC is at strnlen+0x28/0x3c LR is at string.isra.4+0x34/0xcc pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 sp : ce849a88 ip : ce849a98 fp : ce849a94 r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> --- drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c index 5ac59fb..8408bd8 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, unsigned num_configs) { struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); - const struct imx1_pinctrl_soc_info *info = ipctl->info; + struct pin_desc *desc; int i; + desc = pin_desc_get(pctldev, pin_id); + for (i = 0; i != num_configs; ++i) { imx1_write_bit(ipctl, pin_id, configs[i] & 0x01, MX1_PUEN); dev_dbg(ipctl->dev, "pinconf set pullup pin %s\n", - info->pins[pin_id].name); + desc->name); } return 0; -- 1.7.10.4 -- GTSYS Limited RFID Technology 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2, 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong Tel (852) 9079 9521 Disclaimer: http://www.gtsys.com.hk/email/classified.html ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-09 8:28 ` Chris Ruehl 0 siblings, 0 replies; 14+ messages in thread From: Chris Ruehl @ 2015-07-09 8:28 UTC (permalink / raw) To: linux-arm-kernel Uwe, On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-K?nig wrote: > Hello Markus, > > Cc += Chris Ruehl > > On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: >> The pins do not have a 1:1 mapping from index to pin_id. Unfortunately >> the debug output assumes exactly that. >> >> The first driver using imx1-core was imx27, which had exactly this 1:1 >> mapping. It was accidently removed when removing all unused pads which >> were listed: >> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) >> >> The patch fixes this issue by printing the pin_id directly and not the >> pin name. > Knowing a bit about the imx pinctrl drivers I failed to understand what > you wrote here. Probably because I first though that "1:1 mapping" is a > hardware property. What about: > > Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback > > imx1_pinconf_set assumes that the array of pins in struct > imx1_pinctrl_soc_info can be indexed by pin id to get the > pinctrl_pin_desc for a pin. This used to be correct up to commit > 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > which removed some entries from the array and so made it wrong to access > the array by pin id. > > Implement the easiest fix by not resolving the pin id to a name but > printing the id instead. > > Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > Cc: stable at vger.kernel.org > Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Having the pad name in the output is nice, is it worth to search for the > right pinctrl_pin_desc in the array? The array is still sorted, so a > binary search would do, maybe a function for this already exists? > > How did Chris notice the error? Just a bogus output, or did it crash the > kernel? That would be worth to note in the commit log, too. > > Otherwise the change looks fine. > > Best regards > Uwe I had a crash on a array overrun. I'd applied a patch to the linux-usb (which was spit out by the maintainer perl script) chris at wheezyvm:~/kernel.d/linux-next$ cat 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 From: Chris Ruehl <chris.ruehl@gtsys.com.hk> Date: Sat, 23 May 2015 15:02:44 +0800 Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving Bug in function imx1_pinconf_set() cause crash when princtrl debug is enabled and the pin_id becomes larger then the info->pins[] contains. imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for 1000b000.serial imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, direction 0, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, direction 0, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD ... imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, direction 1, oconf 0, iconfa 0, iconfb 0 imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 Unable to handle kernel paging request at virtual address 6c61765f pgd = c0004000 6c61765f] *pgd=00000000 Internal error: Oops: 5 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) task: ce832000 ti: ce848000 task.ti: ce848000 PC is at strnlen+0x28/0x3c LR is at string.isra.4+0x34/0xcc pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 sp : ce849a88 ip : ce849a98 fp : ce849a94 r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> --- drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c index 5ac59fb..8408bd8 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, unsigned num_configs) { struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); - const struct imx1_pinctrl_soc_info *info = ipctl->info; + struct pin_desc *desc; int i; + desc = pin_desc_get(pctldev, pin_id); + for (i = 0; i != num_configs; ++i) { imx1_write_bit(ipctl, pin_id, configs[i] & 0x01, MX1_PUEN); dev_dbg(ipctl->dev, "pinconf set pullup pin %s\n", - info->pins[pin_id].name); + desc->name); } return 0; -- 1.7.10.4 -- GTSYS Limited RFID Technology 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2, 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong Tel (852) 9079 9521 Disclaimer: http://www.gtsys.com.hk/email/classified.html ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-09 8:28 ` Chris Ruehl @ 2015-07-09 13:30 ` Markus Pargmann -1 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-09 13:30 UTC (permalink / raw) To: Chris Ruehl Cc: Uwe Kleine-König, linux-arm-kernel, Linus Walleij, Shawn Guo, kernel, stable [-- Attachment #1: Type: text/plain, Size: 6955 bytes --] Hi, On Thu, Jul 09, 2015 at 04:28:08PM +0800, Chris Ruehl wrote: > Uwe, > > On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-König wrote: > >Hello Markus, > > > >Cc += Chris Ruehl > > > >On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: > >>The pins do not have a 1:1 mapping from index to pin_id. Unfortunately > >>the debug output assumes exactly that. > >> > >>The first driver using imx1-core was imx27, which had exactly this 1:1 > >>mapping. It was accidently removed when removing all unused pads which > >>were listed: > >> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) > >> > >>The patch fixes this issue by printing the pin_id directly and not the > >>pin name. > >Knowing a bit about the imx pinctrl drivers I failed to understand what > >you wrote here. Probably because I first though that "1:1 mapping" is a > >hardware property. What about: > > > > Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback > > > > imx1_pinconf_set assumes that the array of pins in struct > > imx1_pinctrl_soc_info can be indexed by pin id to get the > > pinctrl_pin_desc for a pin. This used to be correct up to commit > > 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > > which removed some entries from the array and so made it wrong to access > > the array by pin id. > > > > Implement the easiest fix by not resolving the pin id to a name but > > printing the id instead. > > > > Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > > Cc: stable@vger.kernel.org > > Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > >Having the pad name in the output is nice, is it worth to search for the > >right pinctrl_pin_desc in the array? The array is still sorted, so a > >binary search would do, maybe a function for this already exists? > > > >How did Chris notice the error? Just a bogus output, or did it crash the > >kernel? That would be worth to note in the commit log, too. > > > >Otherwise the change looks fine. > > > >Best regards > >Uwe > > I had a crash on a array overrun. > > I'd applied a patch to the linux-usb (which was spit out by the maintainer > perl script) Yes, as there is already a patch from Chris my patch can be dropped. We should continue the discussion on your patch instead. > chris@wheezyvm:~/kernel.d/linux-next$ cat > 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch > From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 > From: Chris Ruehl <chris.ruehl@gtsys.com.hk> > Date: Sat, 23 May 2015 15:02:44 +0800 > Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving > > Bug in function imx1_pinconf_set() cause crash when > princtrl debug is enabled and the pin_id becomes larger > then the info->pins[] contains. > > imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, > direction 0, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, > direction 0, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD > ... > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 > Unable to handle kernel paging request at virtual address 6c61765f > pgd = c0004000 > 6c61765f] *pgd=00000000 > Internal error: Oops: 5 [#1] ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 > Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) > task: ce832000 ti: ce848000 task.ti: ce848000 > PC is at strnlen+0x28/0x3c > LR is at string.isra.4+0x34/0xcc > pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 > sp : ce849a88 ip : ce849a98 fp : ce849a94 > r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 > r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a > r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f > Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel > > > Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > --- > drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > index 5ac59fb..8408bd8 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > unsigned num_configs) > { > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > + struct pin_desc *desc; > int i; > > + desc = pin_desc_get(pctldev, pin_id); For the rest of this function pin_desc_get() is not necessary. This is not a simple function call but a radix tree lookup as far as I can see. I am not sure how important this debugging information is and how expensive this function call is?! Best Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-09 13:30 ` Markus Pargmann 0 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-09 13:30 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, Jul 09, 2015 at 04:28:08PM +0800, Chris Ruehl wrote: > Uwe, > > On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-K?nig wrote: > >Hello Markus, > > > >Cc += Chris Ruehl > > > >On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: > >>The pins do not have a 1:1 mapping from index to pin_id. Unfortunately > >>the debug output assumes exactly that. > >> > >>The first driver using imx1-core was imx27, which had exactly this 1:1 > >>mapping. It was accidently removed when removing all unused pads which > >>were listed: > >> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) > >> > >>The patch fixes this issue by printing the pin_id directly and not the > >>pin name. > >Knowing a bit about the imx pinctrl drivers I failed to understand what > >you wrote here. Probably because I first though that "1:1 mapping" is a > >hardware property. What about: > > > > Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback > > > > imx1_pinconf_set assumes that the array of pins in struct > > imx1_pinctrl_soc_info can be indexed by pin id to get the > > pinctrl_pin_desc for a pin. This used to be correct up to commit > > 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > > which removed some entries from the array and so made it wrong to access > > the array by pin id. > > > > Implement the easiest fix by not resolving the pin id to a name but > > printing the id instead. > > > > Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") > > Cc: stable at vger.kernel.org > > Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > >Having the pad name in the output is nice, is it worth to search for the > >right pinctrl_pin_desc in the array? The array is still sorted, so a > >binary search would do, maybe a function for this already exists? > > > >How did Chris notice the error? Just a bogus output, or did it crash the > >kernel? That would be worth to note in the commit log, too. > > > >Otherwise the change looks fine. > > > >Best regards > >Uwe > > I had a crash on a array overrun. > > I'd applied a patch to the linux-usb (which was spit out by the maintainer > perl script) Yes, as there is already a patch from Chris my patch can be dropped. We should continue the discussion on your patch instead. > chris at wheezyvm:~/kernel.d/linux-next$ cat > 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch > From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 > From: Chris Ruehl <chris.ruehl@gtsys.com.hk> > Date: Sat, 23 May 2015 15:02:44 +0800 > Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving > > Bug in function imx1_pinconf_set() cause crash when > princtrl debug is enabled and the pin_id becomes larger > then the info->pins[] contains. > > imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for > 1000b000.serial > imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, > direction 0, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, > direction 0, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 > imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD > ... > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 > imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 > imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, > direction 1, oconf 0, iconfa 0, iconfb 0 > imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 > Unable to handle kernel paging request at virtual address 6c61765f > pgd = c0004000 > 6c61765f] *pgd=00000000 > Internal error: Oops: 5 [#1] ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 > Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) > task: ce832000 ti: ce848000 task.ti: ce848000 > PC is at strnlen+0x28/0x3c > LR is at string.isra.4+0x34/0xcc > pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 > sp : ce849a88 ip : ce849a98 fp : ce849a94 > r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 > r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a > r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f > Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel > > > Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> > --- > drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > index 5ac59fb..8408bd8 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > unsigned num_configs) > { > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > + struct pin_desc *desc; > int i; > > + desc = pin_desc_get(pctldev, pin_id); For the rest of this function pin_desc_get() is not necessary. This is not a simple function call but a radix tree lookup as far as I can see. I am not sure how important this debugging information is and how expensive this function call is?! Best Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150709/509ef35f/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-09 13:30 ` Markus Pargmann @ 2015-07-09 18:06 ` Uwe Kleine-König -1 siblings, 0 replies; 14+ messages in thread From: Uwe Kleine-König @ 2015-07-09 18:06 UTC (permalink / raw) To: Markus Pargmann Cc: Chris Ruehl, linux-arm-kernel, Linus Walleij, Shawn Guo, kernel, stable Hi, On Thu, Jul 09, 2015 at 03:30:36PM +0200, Markus Pargmann wrote: > > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > index 5ac59fb..8408bd8 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > > unsigned num_configs) > > { > > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > > + struct pin_desc *desc; > > int i; > > > > + desc = pin_desc_get(pctldev, pin_id); > > For the rest of this function pin_desc_get() is not necessary. This is > not a simple function call but a radix tree lookup as far as I can see. > I am not sure how important this debugging information is and how > expensive this function call is?! If it's moved into the call to pr_debug it should be optimized away when debugging is off. That together with a merge of Chris' and mine commit log and I'm happy. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K�nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-09 18:06 ` Uwe Kleine-König 0 siblings, 0 replies; 14+ messages in thread From: Uwe Kleine-König @ 2015-07-09 18:06 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, Jul 09, 2015 at 03:30:36PM +0200, Markus Pargmann wrote: > > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > index 5ac59fb..8408bd8 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > > unsigned num_configs) > > { > > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > > + struct pin_desc *desc; > > int i; > > > > + desc = pin_desc_get(pctldev, pin_id); > > For the rest of this function pin_desc_get() is not necessary. This is > not a simple function call but a radix tree lookup as far as I can see. > I am not sure how important this debugging information is and how > expensive this function call is?! If it's moved into the call to pr_debug it should be optimized away when debugging is off. That together with a merge of Chris' and mine commit log and I'm happy. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-09 18:06 ` Uwe Kleine-König @ 2015-07-10 10:02 ` Markus Pargmann -1 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-10 10:02 UTC (permalink / raw) To: Uwe Kleine-König Cc: Chris Ruehl, linux-arm-kernel, Linus Walleij, Shawn Guo, kernel, stable [-- Attachment #1: Type: text/plain, Size: 1563 bytes --] Hi, On Thu, Jul 09, 2015 at 08:06:52PM +0200, Uwe Kleine-König wrote: > Hi, > > On Thu, Jul 09, 2015 at 03:30:36PM +0200, Markus Pargmann wrote: > > > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > index 5ac59fb..8408bd8 100644 > > > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > > > unsigned num_configs) > > > { > > > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > > > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > > > + struct pin_desc *desc; > > > int i; > > > > > > + desc = pin_desc_get(pctldev, pin_id); > > > > For the rest of this function pin_desc_get() is not necessary. This is > > not a simple function call but a radix tree lookup as far as I can see. > > I am not sure how important this debugging information is and how > > expensive this function call is?! > If it's moved into the call to pr_debug it should be optimized away when > debugging is off. That together with a merge of Chris' and mine commit > log and I'm happy. Ok. If you change that I am happy as well. Best Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-10 10:02 ` Markus Pargmann 0 siblings, 0 replies; 14+ messages in thread From: Markus Pargmann @ 2015-07-10 10:02 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, Jul 09, 2015 at 08:06:52PM +0200, Uwe Kleine-K?nig wrote: > Hi, > > On Thu, Jul 09, 2015 at 03:30:36PM +0200, Markus Pargmann wrote: > > > b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > index 5ac59fb..8408bd8 100644 > > > --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c > > > @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, > > > unsigned num_configs) > > > { > > > struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); > > > - const struct imx1_pinctrl_soc_info *info = ipctl->info; > > > + struct pin_desc *desc; > > > int i; > > > > > > + desc = pin_desc_get(pctldev, pin_id); > > > > For the rest of this function pin_desc_get() is not necessary. This is > > not a simple function call but a radix tree lookup as far as I can see. > > I am not sure how important this debugging information is and how > > expensive this function call is?! > If it's moved into the call to pr_debug it should be optimized away when > debugging is off. That together with a merge of Chris' and mine commit > log and I'm happy. Ok. If you change that I am happy as well. Best Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150710/9ef8930b/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pinctrl: imx1-core: Fix debug output pin array index 2015-07-09 13:30 ` Markus Pargmann @ 2015-07-10 1:24 ` Chris Ruehl -1 siblings, 0 replies; 14+ messages in thread From: Chris Ruehl @ 2015-07-10 1:24 UTC (permalink / raw) To: Markus Pargmann Cc: Uwe Kleine-König, linux-arm-kernel, Linus Walleij, Shawn Guo, kernel, stable On Thursday, July 09, 2015 09:30 PM, Markus Pargmann wrote: > Hi, > > On Thu, Jul 09, 2015 at 04:28:08PM +0800, Chris Ruehl wrote: >> Uwe, >> >> On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-König wrote: >>> Hello Markus, >>> >>> Cc += Chris Ruehl >>> >>> On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: >>>> The pins do not have a 1:1 mapping from index to pin_id. Unfortunately >>>> the debug output assumes exactly that. >>>> >>>> The first driver using imx1-core was imx27, which had exactly this 1:1 >>>> mapping. It was accidently removed when removing all unused pads which >>>> were listed: >>>> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) >>>> >>>> The patch fixes this issue by printing the pin_id directly and not the >>>> pin name. >>> Knowing a bit about the imx pinctrl drivers I failed to understand what >>> you wrote here. Probably because I first though that "1:1 mapping" is a >>> hardware property. What about: >>> >>> Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback >>> >>> imx1_pinconf_set assumes that the array of pins in struct >>> imx1_pinctrl_soc_info can be indexed by pin id to get the >>> pinctrl_pin_desc for a pin. This used to be correct up to commit >>> 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") >>> which removed some entries from the array and so made it wrong to access >>> the array by pin id. >>> >>> Implement the easiest fix by not resolving the pin id to a name but >>> printing the id instead. >>> >>> Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") >>> Cc: stable@vger.kernel.org >>> Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> >>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> >>> >>> Having the pad name in the output is nice, is it worth to search for the >>> right pinctrl_pin_desc in the array? The array is still sorted, so a >>> binary search would do, maybe a function for this already exists? >>> >>> How did Chris notice the error? Just a bogus output, or did it crash the >>> kernel? That would be worth to note in the commit log, too. >>> >>> Otherwise the change looks fine. >>> >>> Best regards >>> Uwe >> >> I had a crash on a array overrun. >> >> I'd applied a patch to the linux-usb (which was spit out by the maintainer >> perl script) > > Yes, as there is already a patch from Chris my patch can be dropped. We > should continue the discussion on your patch instead. > >> chris@wheezyvm:~/kernel.d/linux-next$ cat >> 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch >> From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 >> From: Chris Ruehl <chris.ruehl@gtsys.com.hk> >> Date: Sat, 23 May 2015 15:02:44 +0800 >> Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving >> >> Bug in function imx1_pinconf_set() cause crash when >> princtrl debug is enabled and the pin_id becomes larger >> then the info->pins[] contains. >> >> imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, >> direction 0, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, >> direction 0, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD >> ... >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 >> Unable to handle kernel paging request at virtual address 6c61765f >> pgd = c0004000 >> 6c61765f] *pgd=00000000 >> Internal error: Oops: 5 [#1] ARM >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 >> Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) >> task: ce832000 ti: ce848000 task.ti: ce848000 >> PC is at strnlen+0x28/0x3c >> LR is at string.isra.4+0x34/0xcc >> pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 >> sp : ce849a88 ip : ce849a98 fp : ce849a94 >> r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 >> r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a >> r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f >> Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel >> >> >> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> >> --- >> drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> b/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> index 5ac59fb..8408bd8 100644 >> --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, >> unsigned num_configs) >> { >> struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); >> - const struct imx1_pinctrl_soc_info *info = ipctl->info; >> + struct pin_desc *desc; >> int i; >> >> + desc = pin_desc_get(pctldev, pin_id); > > For the rest of this function pin_desc_get() is not necessary. This is > not a simple function call but a radix tree lookup as far as I can see. > I am not sure how important this debugging information is and how > expensive this function call is?! > > Best Regards, > > Markus > Hi Markus, I found it helpful to debug my boards configuration, and have the pin debug on, then I see the output only while the kernel initiation phase. If you look around, the pin_desc_get() function is used quite often in other parts of the pinctrl section. drivers/pinctrl/pinctrl-falcon.c drivers/pinctrl/pinconf.c drivers/pinctrl/pinmux.c drivers/pinctrl/core.c (me) drivers/pinctrl/freescale/pinctrl-imx1-core.c drivers/pinctrl/pinctrl-single.c Regards Chris -- GTSYS Limited RFID Technology 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2, 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong Tel (852) 9079 9521 Disclaimer: http://www.gtsys.com.hk/email/classified.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] pinctrl: imx1-core: Fix debug output pin array index @ 2015-07-10 1:24 ` Chris Ruehl 0 siblings, 0 replies; 14+ messages in thread From: Chris Ruehl @ 2015-07-10 1:24 UTC (permalink / raw) To: linux-arm-kernel On Thursday, July 09, 2015 09:30 PM, Markus Pargmann wrote: > Hi, > > On Thu, Jul 09, 2015 at 04:28:08PM +0800, Chris Ruehl wrote: >> Uwe, >> >> On Thursday, July 09, 2015 03:12 PM, Uwe Kleine-K?nig wrote: >>> Hello Markus, >>> >>> Cc += Chris Ruehl >>> >>> On Wed, Jul 08, 2015 at 04:16:06PM +0200, Markus Pargmann wrote: >>>> The pins do not have a 1:1 mapping from index to pin_id. Unfortunately >>>> the debug output assumes exactly that. >>>> >>>> The first driver using imx1-core was imx27, which had exactly this 1:1 >>>> mapping. It was accidently removed when removing all unused pads which >>>> were listed: >>>> 607af165c047 (pinctrl: i.MX27: Remove nonexistent pad definitions) >>>> >>>> The patch fixes this issue by printing the pin_id directly and not the >>>> pin name. >>> Knowing a bit about the imx pinctrl drivers I failed to understand what >>> you wrote here. Probably because I first though that "1:1 mapping" is a >>> hardware property. What about: >>> >>> Subject: pinctrl: imx1-core: Fix debug output in .pin_config_set callback >>> >>> imx1_pinconf_set assumes that the array of pins in struct >>> imx1_pinctrl_soc_info can be indexed by pin id to get the >>> pinctrl_pin_desc for a pin. This used to be correct up to commit >>> 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") >>> which removed some entries from the array and so made it wrong to access >>> the array by pin id. >>> >>> Implement the easiest fix by not resolving the pin id to a name but >>> printing the id instead. >>> >>> Fixes: 607af165c047 ("pinctrl: i.MX27: Remove nonexistent pad definitions") >>> Cc: stable at vger.kernel.org >>> Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> >>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> >>> >>> Having the pad name in the output is nice, is it worth to search for the >>> right pinctrl_pin_desc in the array? The array is still sorted, so a >>> binary search would do, maybe a function for this already exists? >>> >>> How did Chris notice the error? Just a bogus output, or did it crash the >>> kernel? That would be worth to note in the commit log, too. >>> >>> Otherwise the change looks fine. >>> >>> Best regards >>> Uwe >> >> I had a crash on a array overrun. >> >> I'd applied a patch to the linux-usb (which was spit out by the maintainer >> perl script) > > Yes, as there is already a patch from Chris my patch can be dropped. We > should continue the discussion on your patch instead. > >> chris at wheezyvm:~/kernel.d/linux-next$ cat >> 0001-Pinctrl-imx1-fix-wrong-pin-name-resolving.patch >> From 50d56e5f626b2ea86211818cf487514c96f60487 Mon Sep 17 00:00:00 2001 >> From: Chris Ruehl <chris.ruehl@gtsys.com.hk> >> Date: Sat, 23 May 2015 15:02:44 +0800 >> Subject: [PATCH] Pinctrl: imx1: fix wrong pin-name resolving >> >> Bug in function imx1_pinconf_set() cause crash when >> princtrl debug is enabled and the pin_id becomes larger >> then the info->pins[] contains. >> >> imx27-pinctrl 10015000.iomuxc: request pin 134 (MX27_PAD_UART2_TXD) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 135 (MX27_PAD_UART2_RXD) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 131 (MX27_PAD_UART2_CTS) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: request pin 132 (MX27_PAD_UART2_RTS) for >> 1000b000.serial >> imx27-pinctrl 10015000.iomuxc: enable function uart group uart2-1 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x86, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x87, function 0, gpio 0, >> direction 0, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x83, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0x84, function 0, gpio 0, >> direction 0, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=134 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RTS >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=135 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_RTCK >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=131 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_TXD >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=132 >> imx27-pinctrl 10015000.iomuxc: pinconf set pullup pin MX27_PAD_UART1_RXD >> ... >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415508 offset 4 value 0x3 >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415510 offset 4 value 0x0 >> imx27-pinctrl 10015000.iomuxc: write: register 0xf4415518 offset 4 value 0x0 >> imx27-pinctrl 10015000.iomuxc: imx1_pmx_set, pin 0xb5, function 0, gpio 0, >> direction 1, oconf 0, iconfa 0, iconfb 0 >> imx27-pinctrl 10015000.iomuxc: num_configs=1 PinID=171 >> Unable to handle kernel paging request at virtual address 6c61765f >> pgd = c0004000 >> 6c61765f] *pgd=00000000 >> Internal error: Oops: 5 [#1] ARM >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.1.0-rc4-next-20150522-dirty #8 >> Hardware name: GTSYS i.MX27GTSIR (Device Tree Support) >> task: ce832000 ti: ce848000 task.ti: ce848000 >> PC is at strnlen+0x28/0x3c >> LR is at string.isra.4+0x34/0xcc >> pc : [<c01ae188>] lr : [<c01af9a4>] psr: 20000093 >> sp : ce849a88 ip : ce849a98 fp : ce849a94 >> r10: c05d7e3a r9 : c05d81e4 r8 : 00000000 >> r7 : 6c61765f r6 : c05d81e4 r5 : ffffffff r4 : c05d7e3a >> r3 : 6c61765f r2 : 6c61765f r1 : 6c61765e r0 : 6c61765f >> Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel >> >> >> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk> >> --- >> drivers/pinctrl/freescale/pinctrl-imx1-core.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> b/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> index 5ac59fb..8408bd8 100644 >> --- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> +++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c >> @@ -403,14 +403,16 @@ static int imx1_pinconf_set(struct pinctrl_dev *pctldev, >> unsigned num_configs) >> { >> struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev); >> - const struct imx1_pinctrl_soc_info *info = ipctl->info; >> + struct pin_desc *desc; >> int i; >> >> + desc = pin_desc_get(pctldev, pin_id); > > For the rest of this function pin_desc_get() is not necessary. This is > not a simple function call but a radix tree lookup as far as I can see. > I am not sure how important this debugging information is and how > expensive this function call is?! > > Best Regards, > > Markus > Hi Markus, I found it helpful to debug my boards configuration, and have the pin debug on, then I see the output only while the kernel initiation phase. If you look around, the pin_desc_get() function is used quite often in other parts of the pinctrl section. drivers/pinctrl/pinctrl-falcon.c drivers/pinctrl/pinconf.c drivers/pinctrl/pinmux.c drivers/pinctrl/core.c (me) drivers/pinctrl/freescale/pinctrl-imx1-core.c drivers/pinctrl/pinctrl-single.c Regards Chris -- GTSYS Limited RFID Technology 9/F, Unit E, R07, Kwai Shing Industrial Building Phase 2, 42-46 Tai Lin Pai Road, Kwai Chung, N.T., Hong Kong Tel (852) 9079 9521 Disclaimer: http://www.gtsys.com.hk/email/classified.html ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-07-10 10:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-08 14:16 [PATCH] pinctrl: imx1-core: Fix debug output pin array index Markus Pargmann 2015-07-08 14:16 ` Markus Pargmann 2015-07-09 7:12 ` Uwe Kleine-König 2015-07-09 7:12 ` Uwe Kleine-König 2015-07-09 8:28 ` Chris Ruehl 2015-07-09 8:28 ` Chris Ruehl 2015-07-09 13:30 ` Markus Pargmann 2015-07-09 13:30 ` Markus Pargmann 2015-07-09 18:06 ` Uwe Kleine-König 2015-07-09 18:06 ` Uwe Kleine-König 2015-07-10 10:02 ` Markus Pargmann 2015-07-10 10:02 ` Markus Pargmann 2015-07-10 1:24 ` Chris Ruehl 2015-07-10 1:24 ` Chris Ruehl
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.