eeprom: at24: Limit gpio calls to when wp_gpio is defined
diff mbox series

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
Related show

Commit Message

Claus H. Stovgaard July 5, 2019, 5:24 p.m. UTC
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(-)

Comments

Bartosz Golaszewski July 6, 2019, 5:33 p.m. UTC | #1
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
Claus H. Stovgaard July 6, 2019, 5:57 p.m. UTC | #2
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 ]---
Bartosz Golaszewski July 8, 2019, 8:26 a.m. UTC | #3
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
Claus H. Stovgaard July 8, 2019, 8:53 a.m. UTC | #4
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

Patch
diff mbox series

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);