| Message ID | 1562347885-58349-1-git-send-email-cst@phaseone.com |
|---|---|
| State | Rejected |
| Delegated to: | Bartosz Golaszewski |
| Headers | show |
| Series | eeprom: at24: Limit gpio calls to when wp_gpio is defined | expand |
pt., 5 lip 2019 o 19:31 Claus H. Stovgaard <cst@phaseone.com> napisał(a): > > Calling gpiod_set_value_cansleep with no GPIO driver associated result in > the WARN_ON error from consumer.h. So change to only call > gpiod_set_value_cansleep when wp_gpio is defined. > > Signed-off-by: Claus H. Stovgaard <cst@phaseone.com> > --- > drivers/misc/eeprom/at24.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index 35bf247..d17e982 100644 > --- a/drivers/misc/eeprom/at24.c > +++ b/drivers/misc/eeprom/at24.c > @@ -458,12 +458,14 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) > * from this host, but not from other I2C masters. > */ > mutex_lock(&at24->lock); > - gpiod_set_value_cansleep(at24->wp_gpio, 0); > + if (at24->wp_gpio) > + gpiod_set_value_cansleep(at24->wp_gpio, 0); > > while (count) { > ret = at24_regmap_write(at24, buf, off, count); > if (ret < 0) { > - gpiod_set_value_cansleep(at24->wp_gpio, 1); > + if (at24->wp_gpio) > + gpiod_set_value_cansleep(at24->wp_gpio, 1); > mutex_unlock(&at24->lock); > pm_runtime_put(dev); > return ret; > @@ -473,7 +475,8 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) > count -= ret; > } > > - gpiod_set_value_cansleep(at24->wp_gpio, 1); > + if (at24->wp_gpio) > + gpiod_set_value_cansleep(at24->wp_gpio, 1); > mutex_unlock(&at24->lock); > > pm_runtime_put(dev); > -- > 2.7.4 > Hi Claus, gpiod_set_value_cansleep() doesn't complain if the passed descriptor is NULL - it just quietly returns. Could you give me some more info on how you trigger this warning? Bart
On lør, 2019-07-06 at 19:33 +0200, Bartosz Golaszewski wrote: > Hi Claus, > > gpiod_set_value_cansleep() doesn't complain if the passed descriptor > is NULL - it just quietly returns. Could you give me some more info > on > how you trigger this warning? > > Bart Hi Bart If you don't have enabled gpiolib, (E.g. CONFIG_GPIOLIB is not set) gpiod_set_value_cansleep ends in /include/linux/gpio/consumer.h with the following code --- static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value) { /* GPIO can never have been requested */ WARN_ON(1); } --- So we get warnings like this in the log [ 148.508317] WARNING: CPU: 0 PID: 1903 at include/linux/gpio/consumer.h:396 at24_write+0x150/0x260 [ 148.517187] Modules linked in: [ 148.520236] CPU: 0 PID: 1903 Comm: DataObjects Tainted: G W 4.19.0-p1-iq4-05669-g6fe8008-dirty #2 [ 148.530394] Hardware name: P1 IQ4 (DT) [ 148.534129] pstate: 60000005 (nZCv daif -PAN -UAO) [ 148.538914] pc : at24_write+0x150/0x260 [ 148.542741] lr : at24_write+0x11c/0x260 [ 148.546565] sp : ffffff800d5f3c40 [ 148.549864] x29: ffffff800d5f3c40 x28: 0000000000000000 [ 148.555167] x27: 0000000000000001 x26: ffffffc975096304 [ 148.560470] x25: ffffff8008d46980 x24: ffffffc975293020 [ 148.565774] x23: 00000000ffff6c15 x22: ffffffc977484498 [ 148.571077] x21: 00000000000006e2 x20: 0000000000000000 [ 148.576381] x19: 00000000000006e1 x18: 0000000000000400 [ 148.581684] x17: 0000000000000000 x16: ffffffc977008300 [ 148.586988] x15: 0000000000000400 x14: 0000000000000088 [ 148.592291] x13: 0000000000000000 x12: 0000000000000001 [ 148.597595] x11: 0000000000000001 x10: 00000000000007f0 [ 148.602898] x9 : ffffff800d5f37e0 x8 : ffffffc977008b50 [ 148.608202] x7 : ffffffc97ff76800 x6 : 000000001fbe647a [ 148.613505] x5 : 00000000ffff6c0f x4 : ffffffbf211a0d8f [ 148.618809] x3 : ffffffbf211a0d90 x2 : ffffff80084e55c4 [ 148.624113] x1 : 00000000000005dc x0 : 0000000000000001 [ 148.629417] Call trace: [ 148.631852] at24_write+0x150/0x260 [ 148.635335] bin_attr_nvmem_write+0x6c/0xa0 [ 148.639510] sysfs_kf_bin_write+0x64/0x80 [ 148.643510] kernfs_fop_write+0xcc/0x1e0 [ 148.647425] __vfs_write+0x30/0x158 [ 148.650905] vfs_write+0xa4/0x1a8 [ 148.654211] ksys_write+0x5c/0xc0 [ 148.657519] __arm64_sys_write+0x18/0x20 [ 148.661436] el0_svc_common+0x84/0xd8 [ 148.665087] el0_svc_handler+0x68/0x80 [ 148.668828] el0_svc+0x8/0xc [ 148.671699] ---[ end trace f3f414c3b5f66f98 ]---
sob., 6 lip 2019 o 19:57 Claus H. Stovgaard <cst@phaseone.com> napisał(a): > > On lør, 2019-07-06 at 19:33 +0200, Bartosz Golaszewski wrote: > > Hi Claus, > > > > gpiod_set_value_cansleep() doesn't complain if the passed descriptor > > is NULL - it just quietly returns. Could you give me some more info > > on > > how you trigger this warning? > > > > Bart > > Hi Bart > > If you don't have enabled gpiolib, (E.g. CONFIG_GPIOLIB is not set) > gpiod_set_value_cansleep ends in /include/linux/gpio/consumer.h with > the following code > > --- > static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int > value) > { > /* GPIO can never have been requested */ > WARN_ON(1); > } > --- > > So we get warnings like this in the log > > [ 148.508317] WARNING: CPU: 0 PID: 1903 at > include/linux/gpio/consumer.h:396 at24_write+0x150/0x260 > [ 148.517187] Modules linked in: > [ 148.520236] CPU: 0 PID: 1903 Comm: DataObjects Tainted: > G W 4.19.0-p1-iq4-05669-g6fe8008-dirty #2 > [ 148.530394] Hardware name: P1 IQ4 (DT) > [ 148.534129] pstate: 60000005 (nZCv daif -PAN -UAO) > [ 148.538914] pc : at24_write+0x150/0x260 > [ 148.542741] lr : at24_write+0x11c/0x260 > [ 148.546565] sp : ffffff800d5f3c40 > [ 148.549864] x29: ffffff800d5f3c40 x28: 0000000000000000 > [ 148.555167] x27: 0000000000000001 x26: ffffffc975096304 > [ 148.560470] x25: ffffff8008d46980 x24: ffffffc975293020 > [ 148.565774] x23: 00000000ffff6c15 x22: ffffffc977484498 > [ 148.571077] x21: 00000000000006e2 x20: 0000000000000000 > [ 148.576381] x19: 00000000000006e1 x18: 0000000000000400 > [ 148.581684] x17: 0000000000000000 x16: ffffffc977008300 > [ 148.586988] x15: 0000000000000400 x14: 0000000000000088 > [ 148.592291] x13: 0000000000000000 x12: 0000000000000001 > [ 148.597595] x11: 0000000000000001 x10: 00000000000007f0 > [ 148.602898] x9 : ffffff800d5f37e0 x8 : ffffffc977008b50 > [ 148.608202] x7 : ffffffc97ff76800 x6 : 000000001fbe647a > [ 148.613505] x5 : 00000000ffff6c0f x4 : ffffffbf211a0d8f > [ 148.618809] x3 : ffffffbf211a0d90 x2 : ffffff80084e55c4 > [ 148.624113] x1 : 00000000000005dc x0 : 0000000000000001 > [ 148.629417] Call trace: > [ 148.631852] at24_write+0x150/0x260 > [ 148.635335] bin_attr_nvmem_write+0x6c/0xa0 > [ 148.639510] sysfs_kf_bin_write+0x64/0x80 > [ 148.643510] kernfs_fop_write+0xcc/0x1e0 > [ 148.647425] __vfs_write+0x30/0x158 > [ 148.650905] vfs_write+0xa4/0x1a8 > [ 148.654211] ksys_write+0x5c/0xc0 > [ 148.657519] __arm64_sys_write+0x18/0x20 > [ 148.661436] el0_svc_common+0x84/0xd8 > [ 148.665087] el0_svc_handler+0x68/0x80 > [ 148.668828] el0_svc+0x8/0xc > [ 148.671699] ---[ end trace f3f414c3b5f66f98 ]--- > Cc Linus Walleij I see. This isn't a problem with at24 but with the GPIO API. I Cc'ed you on a patch I've just sent. Please take a look a possibly test. Bart
On Mon, 2019-07-08 at 10:26 +0200, Bartosz Golaszewski wrote: > sob., 6 lip 2019 o 19:57 Claus H. Stovgaard <cst@phaseone.com> > napisał(a): > > > > > > On lør, 2019-07-06 at 19:33 +0200, Bartosz Golaszewski wrote: > > > > > > Hi Claus, > > > > > > gpiod_set_value_cansleep() doesn't complain if the passed > > > descriptor > > > is NULL - it just quietly returns. Could you give me some more > > > info > > > on > > > how you trigger this warning? > > > > > > Bart > > Hi Bart > > > > If you don't have enabled gpiolib, (E.g. CONFIG_GPIOLIB is not set) > > gpiod_set_value_cansleep ends in /include/linux/gpio/consumer.h > > > Cc Linus Walleij > > I see. This isn't a problem with at24 but with the GPIO API. I Cc'ed > you on a patch I've just sent. Please take a look a possibly test. > > Bart Hi Bart Have just looked at the patch and it look good to me. Will test it, and answer on the separate patch mail, but think it will work great for me. Claus
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 35bf247..d17e982 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -458,12 +458,14 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) * from this host, but not from other I2C masters. */ mutex_lock(&at24->lock); - gpiod_set_value_cansleep(at24->wp_gpio, 0); + if (at24->wp_gpio) + gpiod_set_value_cansleep(at24->wp_gpio, 0); while (count) { ret = at24_regmap_write(at24, buf, off, count); if (ret < 0) { - gpiod_set_value_cansleep(at24->wp_gpio, 1); + if (at24->wp_gpio) + gpiod_set_value_cansleep(at24->wp_gpio, 1); mutex_unlock(&at24->lock); pm_runtime_put(dev); return ret; @@ -473,7 +475,8 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count) count -= ret; } - gpiod_set_value_cansleep(at24->wp_gpio, 1); + if (at24->wp_gpio) + gpiod_set_value_cansleep(at24->wp_gpio, 1); mutex_unlock(&at24->lock); pm_runtime_put(dev);
Calling gpiod_set_value_cansleep with no GPIO driver associated result in the WARN_ON error from consumer.h. So change to only call gpiod_set_value_cansleep when wp_gpio is defined. Signed-off-by: Claus H. Stovgaard <cst@phaseone.com> --- drivers/misc/eeprom/at24.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)