| Message ID | 20181111185809.mqi5wev4cdzilghd@raspberrypi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: matrix_keypad - check for errors from of_get_named_gpio() | expand |
Hi Christian, On Sun, Nov 11, 2018 at 07:58:19PM +0100, Christian Hoff wrote: > "of_get_named_gpio()" returns a negative error value if it fails > and drivers should check for this. This missing check was now > added to the matrix_keypad driver. > > In my case "of_get_named_gpio()" returned -EPROBE_DEFER because > the referenced GPIOs belong to an I/O expander, which was not yet > probed at the point in time when the matrix_keypad driver was > loading. Because the driver did not check for errors from the > "of_get_named_gpio()" routine, it was assuming that "-EPROBE_DEFER" > is actually a GPIO number and continued as usual, which led to further > errors like this later on: > > WARNING: CPU: 3 PID: 167 at drivers/gpio/gpiolib.c:114 > gpio_to_desc+0xc8/0xd0 > invalid GPIO -517 > > Note that the "GPIO number" -517 in the error message above is > actually "-EPROBE_DEFER". > > As part of the patch a misleading error message "no platform data defined" > was also removed. You should mention, that there is no information loss, since the other error paths in matrix_keypad_parse_dt() already print an error. > Thanks a lot to Sebastian Reichl for his analysis of the problem > and for pointing me into the right direction on how to fix this issue! Your are welcome! FWIW my lastname is spelled "Reichel" and you can shorten this to: Suggested-by: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Christian Hoff <christian_hoff@gmx.net> > Cc: stable@vger.kernel.org > > --- > drivers/input/keyboard/matrix_keypad.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c > index f51ae09596ef..8a6be94b7838 100644 > --- a/drivers/input/keyboard/matrix_keypad.c > +++ b/drivers/input/keyboard/matrix_keypad.c > @@ -407,7 +407,7 @@ matrix_keypad_parse_dt(struct device *dev) > struct matrix_keypad_platform_data *pdata; > struct device_node *np = dev->of_node; > unsigned int *gpios; > - int i, nrow, ncol; > + int ret, i, nrow, ncol; > > if (!np) { > dev_err(dev, "device lacks DT data\n"); > @@ -444,7 +444,7 @@ matrix_keypad_parse_dt(struct device *dev) > &pdata->col_scan_delay_us); > > gpios = devm_kcalloc(dev, > - pdata->num_row_gpios + pdata->num_col_gpios, > + nrow + ncol, Unrelated change. We have a "Solve only one problem per patch" policy in the kernel. > sizeof(unsigned int), > GFP_KERNEL); > if (!gpios) { > @@ -452,12 +452,19 @@ matrix_keypad_parse_dt(struct device *dev) > return ERR_PTR(-ENOMEM); > } > > - for (i = 0; i < pdata->num_row_gpios; i++) > - gpios[i] = of_get_named_gpio(np, "row-gpios", i); > + for (i = 0; i < nrow; i++) { > + ret = of_get_named_gpio(np, "row-gpios", i); > + if (ret < 0) > + return ERR_PTR(ret); > + gpios[i] = ret; > + } > > - for (i = 0; i < pdata->num_col_gpios; i++) > - gpios[pdata->num_row_gpios + i] = > - of_get_named_gpio(np, "col-gpios", i); > + for (i = 0; i < ncol; i++) { > + ret = of_get_named_gpio(np, "col-gpios", i); > + if (ret < 0) > + return ERR_PTR(ret); > + gpios[nrow + i] = ret; > + } > > pdata->row_gpios = gpios; > pdata->col_gpios = &gpios[pdata->num_row_gpios]; > @@ -485,7 +492,6 @@ static int matrix_keypad_probe(struct platform_device *pdev) > if (!pdata) { > pdata = matrix_keypad_parse_dt(&pdev->dev); > if (IS_ERR(pdata)) { > - dev_err(&pdev->dev, "no platform data defined\n"); > return PTR_ERR(pdata); > } The curly brackets can be removed. > } else if (!pdata->keymap_data) { > -- > 2.16.1.2.g9d582f143 > Otherwise looks good to me! P.S.: It would be nice to have this driver converted to gpiod, but that can be done on top of this and is not suitable for stable. -- Sebastian
Hi Sebastian, On Sun, Nov 11, 2018 at 08:47:53PM +0100, Sebastian Reichel wrote: > Hi Christian, > > On Sun, Nov 11, 2018 at 07:58:19PM +0100, Christian Hoff wrote: > > > > As part of the patch a misleading error message "no platform data defined" > > was also removed. > > You should mention, that there is no information loss, since the > other error paths in matrix_keypad_parse_dt() already print an > error. I have updated the commit message accordingly. > > > Thanks a lot to Sebastian Reichl for his analysis of the problem > > and for pointing me into the right direction on how to fix this issue! > > Your are welcome! FWIW my lastname is spelled "Reichel" and you can > shorten this to: > > Suggested-by: Sebastian Reichel <sre@kernel.org> I have done that now. > > > Signed-off-by: Christian Hoff <christian_hoff@gmx.net> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/input/keyboard/matrix_keypad.c | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c > > index f51ae09596ef..8a6be94b7838 100644 > > --- a/drivers/input/keyboard/matrix_keypad.c > > +++ b/drivers/input/keyboard/matrix_keypad.c > > @@ -407,7 +407,7 @@ matrix_keypad_parse_dt(struct device *dev) > > struct matrix_keypad_platform_data *pdata; > > struct device_node *np = dev->of_node; > > unsigned int *gpios; > > - int i, nrow, ncol; > > + int ret, i, nrow, ncol; > > > > if (!np) { > > dev_err(dev, "device lacks DT data\n"); > > @@ -444,7 +444,7 @@ matrix_keypad_parse_dt(struct device *dev) > > &pdata->col_scan_delay_us); > > > > gpios = devm_kcalloc(dev, > > - pdata->num_row_gpios + pdata->num_col_gpios, > > + nrow + ncol, > > Unrelated change. We have a "Solve only one problem per patch" > policy in the kernel. Alright, I have thrown this line out. > > > sizeof(unsigned int), > > GFP_KERNEL); > > if (!gpios) { > > @@ -452,12 +452,19 @@ matrix_keypad_parse_dt(struct device *dev) > > return ERR_PTR(-ENOMEM); > > } > > > > - for (i = 0; i < pdata->num_row_gpios; i++) > > - gpios[i] = of_get_named_gpio(np, "row-gpios", i); > > + for (i = 0; i < nrow; i++) { > > + ret = of_get_named_gpio(np, "row-gpios", i); > > + if (ret < 0) > > + return ERR_PTR(ret); > > + gpios[i] = ret; > > + } > > > > - for (i = 0; i < pdata->num_col_gpios; i++) > > - gpios[pdata->num_row_gpios + i] = > > - of_get_named_gpio(np, "col-gpios", i); > > + for (i = 0; i < ncol; i++) { > > + ret = of_get_named_gpio(np, "col-gpios", i); > > + if (ret < 0) > > + return ERR_PTR(ret); > > + gpios[nrow + i] = ret; > > + } > > > > pdata->row_gpios = gpios; > > pdata->col_gpios = &gpios[pdata->num_row_gpios]; > > @@ -485,7 +492,6 @@ static int matrix_keypad_probe(struct platform_device *pdev) > > if (!pdata) { > > pdata = matrix_keypad_parse_dt(&pdev->dev); > > if (IS_ERR(pdata)) { > > - dev_err(&pdev->dev, "no platform data defined\n"); > > return PTR_ERR(pdata); > > } > > The curly brackets can be removed. Done. > > > } else if (!pdata->keymap_data) { > > -- > > 2.16.1.2.g9d582f143 > > > > Otherwise looks good to me!# I am happy to hear that! I have just submitted a new second version of this patch for review based on your suggestions. > > P.S.: It would be nice to have this driver converted to gpiod, but > that can be done on top of this and is not suitable for stable. I fully agree. I hope that I will find time to do that in the next months. I already had the same idea, but somehow there were always more urgent things to be done... Best regards, Christian
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c index f51ae09596ef..8a6be94b7838 100644 --- a/drivers/input/keyboard/matrix_keypad.c +++ b/drivers/input/keyboard/matrix_keypad.c @@ -407,7 +407,7 @@ matrix_keypad_parse_dt(struct device *dev) struct matrix_keypad_platform_data *pdata; struct device_node *np = dev->of_node; unsigned int *gpios; - int i, nrow, ncol; + int ret, i, nrow, ncol; if (!np) { dev_err(dev, "device lacks DT data\n"); @@ -444,7 +444,7 @@ matrix_keypad_parse_dt(struct device *dev) &pdata->col_scan_delay_us); gpios = devm_kcalloc(dev, - pdata->num_row_gpios + pdata->num_col_gpios, + nrow + ncol, sizeof(unsigned int), GFP_KERNEL); if (!gpios) { @@ -452,12 +452,19 @@ matrix_keypad_parse_dt(struct device *dev) return ERR_PTR(-ENOMEM); } - for (i = 0; i < pdata->num_row_gpios; i++) - gpios[i] = of_get_named_gpio(np, "row-gpios", i); + for (i = 0; i < nrow; i++) { + ret = of_get_named_gpio(np, "row-gpios", i); + if (ret < 0) + return ERR_PTR(ret); + gpios[i] = ret; + } - for (i = 0; i < pdata->num_col_gpios; i++) - gpios[pdata->num_row_gpios + i] = - of_get_named_gpio(np, "col-gpios", i); + for (i = 0; i < ncol; i++) { + ret = of_get_named_gpio(np, "col-gpios", i); + if (ret < 0) + return ERR_PTR(ret); + gpios[nrow + i] = ret; + } pdata->row_gpios = gpios; pdata->col_gpios = &gpios[pdata->num_row_gpios]; @@ -485,7 +492,6 @@ static int matrix_keypad_probe(struct platform_device *pdev) if (!pdata) { pdata = matrix_keypad_parse_dt(&pdev->dev); if (IS_ERR(pdata)) { - dev_err(&pdev->dev, "no platform data defined\n"); return PTR_ERR(pdata); } } else if (!pdata->keymap_data) {
"of_get_named_gpio()" returns a negative error value if it fails and drivers should check for this. This missing check was now added to the matrix_keypad driver. In my case "of_get_named_gpio()" returned -EPROBE_DEFER because the referenced GPIOs belong to an I/O expander, which was not yet probed at the point in time when the matrix_keypad driver was loading. Because the driver did not check for errors from the "of_get_named_gpio()" routine, it was assuming that "-EPROBE_DEFER" is actually a GPIO number and continued as usual, which led to further errors like this later on: WARNING: CPU: 3 PID: 167 at drivers/gpio/gpiolib.c:114 gpio_to_desc+0xc8/0xd0 invalid GPIO -517 Note that the "GPIO number" -517 in the error message above is actually "-EPROBE_DEFER". As part of the patch a misleading error message "no platform data defined" was also removed. Thanks a lot to Sebastian Reichl for his analysis of the problem and for pointing me into the right direction on how to fix this issue! Signed-off-by: Christian Hoff <christian_hoff@gmx.net> Cc: stable@vger.kernel.org --- drivers/input/keyboard/matrix_keypad.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)