[PATCH 0/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[PATCH 0/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

Hi!

There are a couple of users that could benefit from an unlocked variant
of i2c_smbus_xfer. In response to [1] I dusted off an old series that I
have had in a branch for a while that adds such a beast. It also converts
over some users in the i2c-mux department.

There are a couple of remaining candidates in i2c/busses/i2c-amd756-s4882.c
and i2c/busses/i2c-nforce2-s4985.c, but frankly, those are just too nasty.
Why do they unregister the root adapter and then continue to use it? I
dare not touch that code...

Anyway, this series looks like a nice cleanup to me, and the recent sccb
patch from Akinobu Mita could also benefit, as indicated by Wolfram [1].

Cheers,
Peter

[1] https://patchwork.ozlabs.org/patch/928386/#1936016

Peter Rosin (5):
  i2c: smbus: add unlocked __i2c_smbus_xfer variant
  i2c: mux: mlxcpld: make use of __i2c_smbus_xfer
  i2c: mux: pca9541: make use of __i2c_smbus_xfer
  i2c: mux: pca954x: make use of __i2c_smbus_xfer
  i2c: mux: make use of __i2c_smbus_xfer

 drivers/i2c/i2c-core-smbus.c        | 28 ++++++++++-----
 drivers/i2c/i2c-mux.c               |  4 +--
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 28 +++------------
 drivers/i2c/muxes/i2c-mux-pca9541.c | 69 ++++++-------------------------------
 drivers/i2c/muxes/i2c-mux-pca954x.c | 27 +++------------
 include/linux/i2c.h                 | 11 ++++--
 6 files changed, 49 insertions(+), 118 deletions(-)

-- 
2.11.0

[PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

Removes all locking from i2c_smbus_xfer and renames it to __i2c_smbus_xfer,
then adds a new i2c_smbus_xfer function that simply grabs the lock while
calling the unlocked variant.

This is not perfectly equivalent, since i2c_smbus_xfer was callable from
atomic/irq context if you happened to end up emulating SMBus with an I2C
transfer, and that is no longer the case with this patch. It is unknown
(to me) if anything depends on that quirk, but it seems fragile enough to
simply break those cases and require them to call i2c_transfer directly
instead.

While at it, for consistency rename the 2nd to last argument (size) of
the i2c_smbus_xfer declaration to protocol and remove the surplus extern
marker.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/i2c-core-smbus.c | 28 ++++++++++++++++++++--------
 include/linux/i2c.h          | 11 ++++++++---
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index f3f683041e7f..e89956ec5e27 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -463,7 +463,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			msg[num-1].len++;
 	}
 
-	status = i2c_transfer(adapter, msg, num);
+	status = __i2c_transfer(adapter, msg, num);
 	if (status < 0)
 		return status;
 	if (status != num)
@@ -520,9 +520,24 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
  * This executes an SMBus protocol operation, and returns a negative
  * errno code else zero on success.
  */
-s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
-		   char read_write, u8 command, int protocol,
-		   union i2c_smbus_data *data)
+s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
+		   unsigned short flags, char read_write,
+		   u8 command, int protocol, union i2c_smbus_data *data)
+{
+	s32 res;
+
+	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
+	res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
+			       command, protocol, data);
+	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
+
+	return res;
+}
+EXPORT_SYMBOL(i2c_smbus_xfer);
+
+s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
+		     unsigned short flags, char read_write,
+		     u8 command, int protocol, union i2c_smbus_data *data)
 {
 	unsigned long orig_jiffies;
 	int try;
@@ -539,8 +554,6 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 	flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB;
 
 	if (adapter->algo->smbus_xfer) {
-		i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
-
 		/* Retry automatically on arbitration loss */
 		orig_jiffies = jiffies;
 		for (res = 0, try = 0; try <= adapter->retries; try++) {
@@ -553,7 +566,6 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 				       orig_jiffies + adapter->timeout))
 				break;
 		}
-		i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
 
 		if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)
 			goto trace;
@@ -575,7 +587,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 
 	return res;
 }
-EXPORT_SYMBOL(i2c_smbus_xfer);
+EXPORT_SYMBOL(__i2c_smbus_xfer);
 
 /**
  * i2c_smbus_read_i2c_block_data_or_emulated - read block or emulate
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 254cd34eeae2..465afb092fa7 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -140,9 +140,14 @@ extern int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
    and probably just as fast.
    Note that we use i2c_adapter here, because you do not need a specific
    smbus adapter to call this function. */
-extern s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
-			  unsigned short flags, char read_write, u8 command,
-			  int size, union i2c_smbus_data *data);
+s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
+		   unsigned short flags, char read_write, u8 command,
+		   int protocol, union i2c_smbus_data *data);
+
+/* Unlocked flavor */
+s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
+		     unsigned short flags, char read_write, u8 command,
+		     int protocol, union i2c_smbus_data *data);
 
 /* Now follow the 'nice' access routines. These also document the calling
    conventions of i2c_smbus_xfer. */
-- 
2.11.0

[PATCH 2/5] i2c: mux: mlxcpld: make use of __i2c_smbus_xfer

[Peter Rosin]

This simplifies the code, and you get retries for free if the adapter
does not support ->master_xfer.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index 12ad8d65faf6..f2bf3e57ed67 100644
--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
@@ -94,31 +94,11 @@ static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
 				 struct i2c_client *client, u8 val)
 {
 	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client->dev);
-	int ret = -ENODEV;
-
-	if (adap->algo->master_xfer) {
-		struct i2c_msg msg;
-		u8 msgbuf[] = {pdata->sel_reg_addr, val};
-
-		msg.addr = client->addr;
-		msg.flags = 0;
-		msg.len = 2;
-		msg.buf = msgbuf;
-		ret = __i2c_transfer(adap, &msg, 1);
-
-		if (ret >= 0 && ret != 1)
-			ret = -EREMOTEIO;
-	} else if (adap->algo->smbus_xfer) {
-		union i2c_smbus_data data;
-
-		data.byte = val;
-		ret = adap->algo->smbus_xfer(adap, client->addr,
-					     client->flags, I2C_SMBUS_WRITE,
-					     pdata->sel_reg_addr,
-					     I2C_SMBUS_BYTE_DATA, &data);
-	}
+	union i2c_smbus_data data = { .byte = val };
 
-	return ret;
+	return __i2c_smbus_xfer(adap, client->addr, client->flags,
+				I2C_SMBUS_WRITE, pdata->sel_reg_addr,
+				I2C_SMBUS_BYTE_DATA, &data);
 }
 
 static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
-- 
2.11.0

[PATCH 3/5] i2c: mux: pca9541: make use of __i2c_smbus_xfer

[Peter Rosin]

This simplifies the code, and you get retries for free if the adapter
does not support ->master_xfer.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 69 ++++++-------------------------------
 1 file changed, 11 insertions(+), 58 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6a39adaf433f..3d8c938e99e7 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -99,31 +99,11 @@ MODULE_DEVICE_TABLE(of, pca9541_of_match);
 static int pca9541_reg_write(struct i2c_client *client, u8 command, u8 val)
 {
 	struct i2c_adapter *adap = client->adapter;
-	int ret;
-
-	if (adap->algo->master_xfer) {
-		struct i2c_msg msg;
-		char buf[2];
-
-		msg.addr = client->addr;
-		msg.flags = 0;
-		msg.len = 2;
-		buf[0] = command;
-		buf[1] = val;
-		msg.buf = buf;
-		ret = __i2c_transfer(adap, &msg, 1);
-	} else {
-		union i2c_smbus_data data;
-
-		data.byte = val;
-		ret = adap->algo->smbus_xfer(adap, client->addr,
-					     client->flags,
-					     I2C_SMBUS_WRITE,
-					     command,
-					     I2C_SMBUS_BYTE_DATA, &data);
-	}
+	union i2c_smbus_data data = { .byte = val };
 
-	return ret;
+	return __i2c_smbus_xfer(adap, client->addr, client->flags,
+				I2C_SMBUS_WRITE, command,
+				I2C_SMBUS_BYTE_DATA, &data);
 }
 
 /*
@@ -133,41 +113,14 @@ static int pca9541_reg_write(struct i2c_client *client, u8 command, u8 val)
 static int pca9541_reg_read(struct i2c_client *client, u8 command)
 {
 	struct i2c_adapter *adap = client->adapter;
+	union i2c_smbus_data data;
 	int ret;
-	u8 val;
-
-	if (adap->algo->master_xfer) {
-		struct i2c_msg msg[2] = {
-			{
-				.addr = client->addr,
-				.flags = 0,
-				.len = 1,
-				.buf = &command
-			},
-			{
-				.addr = client->addr,
-				.flags = I2C_M_RD,
-				.len = 1,
-				.buf = &val
-			}
-		};
-		ret = __i2c_transfer(adap, msg, 2);
-		if (ret == 2)
-			ret = val;
-		else if (ret >= 0)
-			ret = -EIO;
-	} else {
-		union i2c_smbus_data data;
-
-		ret = adap->algo->smbus_xfer(adap, client->addr,
-					     client->flags,
-					     I2C_SMBUS_READ,
-					     command,
-					     I2C_SMBUS_BYTE_DATA, &data);
-		if (!ret)
-			ret = data.byte;
-	}
-	return ret;
+
+	ret = __i2c_smbus_xfer(adap, client->addr, client->flags,
+			       I2C_SMBUS_READ, command,
+			       I2C_SMBUS_BYTE_DATA, &data);
+
+	return ret ?: data.byte;
 }
 
 /*
-- 
2.11.0

[PATCH 4/5] i2c: mux: pca954x: make use of __i2c_smbus_xfer

[Peter Rosin]

This simplifies the code, and you get retries for free if the adapter
does not support ->master_xfer.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index fbc748027087..0e9a0e66e92f 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -220,30 +220,11 @@ MODULE_DEVICE_TABLE(of, pca954x_of_match);
 static int pca954x_reg_write(struct i2c_adapter *adap,
 			     struct i2c_client *client, u8 val)
 {
-	int ret = -ENODEV;
-
-	if (adap->algo->master_xfer) {
-		struct i2c_msg msg;
-		char buf[1];
-
-		msg.addr = client->addr;
-		msg.flags = 0;
-		msg.len = 1;
-		buf[0] = val;
-		msg.buf = buf;
-		ret = __i2c_transfer(adap, &msg, 1);
-
-		if (ret >= 0 && ret != 1)
-			ret = -EREMOTEIO;
-	} else {
-		union i2c_smbus_data data;
-		ret = adap->algo->smbus_xfer(adap, client->addr,
-					     client->flags,
-					     I2C_SMBUS_WRITE,
-					     val, I2C_SMBUS_BYTE, &data);
-	}
+	union i2c_smbus_data dummy;
 
-	return ret;
+	return __i2c_smbus_xfer(adap, client->addr, client->flags,
+				I2C_SMBUS_WRITE, val,
+				I2C_SMBUS_BYTE, &dummy);
 }
 
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
-- 
2.11.0

[PATCH 5/5] i2c: mux: make use of __i2c_smbus_xfer

[Peter Rosin]

Calling the __i2c_smbus_xfer wrapper in __i2c_mux_smbus_xfer provides
retries and thus makes the parent-locked case consistent with the both
mux-locked (i2c_mux_smbus_xfer) and the I2C transfer cases.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/i2c-mux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 300ab4b672e4..221365eb7c05 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -87,8 +87,8 @@ static int __i2c_mux_smbus_xfer(struct i2c_adapter *adap,
 
 	ret = muxc->select(muxc, priv->chan_id);
 	if (ret >= 0)
-		ret = parent->algo->smbus_xfer(parent, addr, flags,
-					read_write, command, size, data);
+		ret = __i2c_smbus_xfer(parent, addr, flags,
+				       read_write, command, size, data);
 	if (muxc->deselect)
 		muxc->deselect(muxc, priv->chan_id);
 
-- 
2.11.0

RE: [PATCH 2/5] i2c: mux: mlxcpld: make use of __i2c_smbus_xfer

[Michael Shych]

> -----Original Message-----
> From: Peter Rosin [mailto:peda@axentia.se]
> Sent: Wednesday, June 20, 2018 11:52 AM
> To: linux-kernel@vger.kernel.org
> Cc: Peter Rosin <peda@axentia.se>; Wolfram Sang <wsa@the-dreams.de>;
> Vadim Pasternak <vadimp@mellanox.com>; Michael Shych
> <michaelsh@mellanox.com>; Guenter Roeck <linux@roeck-us.net>;
> Akinobu Mita <akinobu.mita@gmail.com>; Jean Delvare
> <jdelvare@suse.com>; linux-i2c@vger.kernel.org
> Subject: [PATCH 2/5] i2c: mux: mlxcpld: make use of __i2c_smbus_xfer
> 
> This simplifies the code, and you get retries for free if the adapter
> does not support ->master_xfer.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Acked-by: Michael Shych <michaelsh@mellanox.com>

> ---
>  drivers/i2c/muxes/i2c-mux-mlxcpld.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-
> mux-mlxcpld.c
> index 12ad8d65faf6..f2bf3e57ed67 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -94,31 +94,11 @@ static int mlxcpld_mux_reg_write(struct i2c_adapter
> *adap,
>  				 struct i2c_client *client, u8 val)
>  {
>  	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client-
> >dev);
> -	int ret = -ENODEV;
> -
> -	if (adap->algo->master_xfer) {
> -		struct i2c_msg msg;
> -		u8 msgbuf[] = {pdata->sel_reg_addr, val};
> -
> -		msg.addr = client->addr;
> -		msg.flags = 0;
> -		msg.len = 2;
> -		msg.buf = msgbuf;
> -		ret = __i2c_transfer(adap, &msg, 1);
> -
> -		if (ret >= 0 && ret != 1)
> -			ret = -EREMOTEIO;
> -	} else if (adap->algo->smbus_xfer) {
> -		union i2c_smbus_data data;
> -
> -		data.byte = val;
> -		ret = adap->algo->smbus_xfer(adap, client->addr,
> -					     client->flags, I2C_SMBUS_WRITE,
> -					     pdata->sel_reg_addr,
> -					     I2C_SMBUS_BYTE_DATA, &data);
> -	}
> +	union i2c_smbus_data data = { .byte = val };
> 
> -	return ret;
> +	return __i2c_smbus_xfer(adap, client->addr, client->flags,
> +				I2C_SMBUS_WRITE, pdata->sel_reg_addr,
> +				I2C_SMBUS_BYTE_DATA, &data);
>  }
> 
>  static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
> --
> 2.11.0

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Wolfram Sang]

> This is not perfectly equivalent, since i2c_smbus_xfer was callable from
> atomic/irq context if you happened to end up emulating SMBus with an I2C
> transfer, and that is no longer the case with this patch. It is unknown
> (to me) if anything depends on that quirk, but it seems fragile enough to
> simply break those cases and require them to call i2c_transfer directly
> instead.

Couldn't we just add the same trylock-code path here as well? I always
wondered why I2C and SMBus were not in sync when it came to that. Yet, I
didn't want to change the code for no reason, but it seems we now have
one?

Rest of the series looks good to me, very nice diffstat!

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

On June 26, 2018 4:37:58 AM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> This is not perfectly equivalent, since i2c_smbus_xfer was callable
>from
>> atomic/irq context if you happened to end up emulating SMBus with an
>I2C
>> transfer, and that is no longer the case with this patch. It is
>unknown
>> (to me) if anything depends on that quirk, but it seems fragile
>enough to
>> simply break those cases and require them to call i2c_transfer
>directly
>> instead.
>
>Couldn't we just add the same trylock-code path here as well? I always
>wondered why I2C and SMBus were not in sync when it came to that. Yet,
>I
>didn't want to change the code for no reason, but it seems we now have
>one?
>
>Rest of the series looks good to me, very nice diffstat!

I don't think it's that easy as I just thought about another problem with lifting the locking from the emulation function. It calls kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a very good idea from atomic/irq context...

Cheers,
Peter

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]


> I don't think it's that easy as I just thought about another problem
> with lifting the locking from the emulation function. It calls
> kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a
> very good idea from atomic/irq context...

Right. However, we could simply bail out early when we are in atomic
context. Simply no DMA then...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

On June 26, 2018 3:46:07 PM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote:
>> I don't think it's that easy as I just thought about another problem
>> with lifting the locking from the emulation function. It calls
>> kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a
>> very good idea from atomic/irq context...
>
>Right. However, we could simply bail out early when we are in atomic
>context. Simply no DMA then...

Yeah, we could bail out early, and perhaps we should. But we risk regressions, see below...

And that should probably be fixed before we add the unlocked __i2c_smbus_xfer function.

Because, thinking more about it, the problem with those allocs are not related to the locking details; adding another trylock to the mix just makes it so much more obvious. I mean, first we would specifically handle atomic/irq context with a trylock "documenting" that atomic/irq users are welcome to at least try xfers, and then we blattantly break the rulez with a GFP_KERNEL alloc...

Also, the fact that the buffer is DMA-friendly does not mean that DMA is actually going to be used, so the patch that introduced those allocs might have regressed for this case:
- SMBus user from atomic/irq context
- SMBus emulated
- emulation triggering a GFP_KERNEL alloc
- the existing trylock in i2c_transfer succeeding
- driver not ending up doing DMA

Bailing out would break these users, always. Currently, I assume they are only broken when the alloc happens to need to do more than is allowed from the given context. Something which might or might not be common?

But as usual, I might be missing something...

Cheers,
Peter

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]


> Because, thinking more about it, the problem with those allocs are not
> related to the locking details; adding another trylock to the mix just
> makes it so much more obvious. I mean, first we would specifically
> handle atomic/irq context with a trylock "documenting" that atomic/irq
> users are welcome to at least try xfers, and then we blattantly break
> the rulez with a GFP_KERNEL alloc...

Yes, thinking more about it, I came to the conclusion that we should not
add trylock to SMBus and keep the requirement to allow sleeping.

True, SMBus is not consistent with I2C then, but actually, I'd prefer
the consistency the other way around: I wish we had a clear statement
that i2c_transfer may sleep. And have a dedicated irqless, non-sleeping
callback for handling the atomic case instead.

I really don't like the commit which introduced the trylock
into i2c_transfer[1]. Its commit message even says: "It is the
reponsability of the caller to ensure that the underlying i2c bus driver
will not sleep either." Which seems broken to me because I can't see how
the caller should do that? And most bus drivers will sleep. But that
commit is upstream for 10 years now, so there are probably users. Which
also are very hard to spot, I am afraid. I wouldn't see a way to convert
them off the top of my head.

[1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts")

> Currently, I assume they are only broken when the alloc happens to
> need to do more than is allowed from the given context. Something
> which might or might not be common?

The only regression now would be using smbus_emulated from atomic
context. Which is a bug on the caller side because it cannot know if
smbus_emulated will be used or not. For the non-emulated case, it must
not be atomic anyhow.

So, unless I overlooked something, if we decide to not add trylock to
smbus_xfer, we are all fine?

And I think we should really keep this clean rule of smbus functions
being non-atomic.

D'accord?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On Wed, Jun 27, 2018 at 09:08:18AM +0200, Wolfram Sang wrote:
> 
> > Because, thinking more about it, the problem with those allocs are not
> > related to the locking details; adding another trylock to the mix just
> > makes it so much more obvious. I mean, first we would specifically
> > handle atomic/irq context with a trylock "documenting" that atomic/irq
> > users are welcome to at least try xfers, and then we blattantly break
> > the rulez with a GFP_KERNEL alloc...
> 
> Yes, thinking more about it, I came to the conclusion that we should not
> add trylock to SMBus and keep the requirement to allow sleeping.
> 
> True, SMBus is not consistent with I2C then, but actually, I'd prefer
> the consistency the other way around: I wish we had a clear statement
> that i2c_transfer may sleep. And have a dedicated irqless, non-sleeping
> callback for handling the atomic case instead.
> 
> I really don't like the commit which introduced the trylock
> into i2c_transfer[1]. Its commit message even says: "It is the
> reponsability of the caller to ensure that the underlying i2c bus driver
> will not sleep either." Which seems broken to me because I can't see how
> the caller should do that? And most bus drivers will sleep. But that
> commit is upstream for 10 years now, so there are probably users. Which
> also are very hard to spot, I am afraid. I wouldn't see a way to convert
> them off the top of my head.
> 
> [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts")
> 
> > Currently, I assume they are only broken when the alloc happens to
> > need to do more than is allowed from the given context. Something
> > which might or might not be common?
> 
> The only regression now would be using smbus_emulated from atomic
> context. Which is a bug on the caller side because it cannot know if
> smbus_emulated will be used or not. For the non-emulated case, it must
> not be atomic anyhow.
> 
> So, unless I overlooked something, if we decide to not add trylock to
> smbus_xfer, we are all fine?
> 
> And I think we should really keep this clean rule of smbus functions
> being non-atomic.
> 
> D'accord?

So, if no other arguments drop in, I'll apply this series as is next
week.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

On July 1, 2018 2:13:20 PM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote:
>On Wed, Jun 27, 2018 at 09:08:18AM +0200, Wolfram Sang wrote:
>> 
>> > Because, thinking more about it, the problem with those allocs are
>not
>> > related to the locking details; adding another trylock to the mix
>just
>> > makes it so much more obvious. I mean, first we would specifically
>> > handle atomic/irq context with a trylock "documenting" that
>atomic/irq
>> > users are welcome to at least try xfers, and then we blattantly
>break
>> > the rulez with a GFP_KERNEL alloc...
>> 
>> Yes, thinking more about it, I came to the conclusion that we should
>not
>> add trylock to SMBus and keep the requirement to allow sleeping.
>> 
>> True, SMBus is not consistent with I2C then, but actually, I'd prefer
>> the consistency the other way around: I wish we had a clear statement
>> that i2c_transfer may sleep. And have a dedicated irqless,
>non-sleeping
>> callback for handling the atomic case instead.
>> 
>> I really don't like the commit which introduced the trylock
>> into i2c_transfer[1]. Its commit message even says: "It is the
>> reponsability of the caller to ensure that the underlying i2c bus
>driver
>> will not sleep either." Which seems broken to me because I can't see
>how
>> the caller should do that? And most bus drivers will sleep. But that
>> commit is upstream for 10 years now, so there are probably users.
>Which
>> also are very hard to spot, I am afraid. I wouldn't see a way to
>convert
>> them off the top of my head.
>> 
>> [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts")
>> 
>> > Currently, I assume they are only broken when the alloc happens to
>> > need to do more than is allowed from the given context. Something
>> > which might or might not be common?
>> 
>> The only regression now would be using smbus_emulated from atomic
>> context. Which is a bug on the caller side because it cannot know if
>> smbus_emulated will be used or not. For the non-emulated case, it
>must
>> not be atomic anyhow.
>> 
>> So, unless I overlooked something, if we decide to not add trylock to
>> smbus_xfer, we are all fine?
>> 
>> And I think we should really keep this clean rule of smbus functions
>> being non-atomic.
>> 
>> D'accord?
>
>So, if no other arguments drop in, I'll apply this series as is next
>week.

Right, I had the below response sitting in my drafts folder.  I thought I had sent it, but apparently I didn't...



Well, IF there are SMBus users that in fact do rely on the emulation allowing calls from atomic/irq context then it will be a regression even if those users are "buggy". But if someone complains, I think the correct response is to open-code the trylock dance and call the new unlocked __i2c_smbus_xfer at the affected location. So, I think we have a contingency plan.

Other than that, we are in violent agreement, and I think you also agree with the above.

I guess that also means the series is fine as-is (modulo the fixes recently made in the tail of the first hunk of patch 1/5 that causes a trivial but annoying conflict when applying it, i.e. below the i2c_transfer -> __i2c_transfer update in the emulation function).

As far as I'm concerned, you can take this whole series directly even if most patches are i2c-mux patches. I don't have anything in my tree yet so I'll simply base any other stuff on this once I can fetch it from you...

Ok?

Cheers,
Peter

Re: [PATCH 0/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Wed, Jun 20, 2018 at 10:51:52AM +0200, Peter Rosin wrote:
> Hi!
> 
> There are a couple of users that could benefit from an unlocked variant
> of i2c_smbus_xfer. In response to [1] I dusted off an old series that I
> have had in a branch for a while that adds such a beast. It also converts
> over some users in the i2c-mux department.
> 
> There are a couple of remaining candidates in i2c/busses/i2c-amd756-s4882.c
> and i2c/busses/i2c-nforce2-s4985.c, but frankly, those are just too nasty.
> Why do they unregister the root adapter and then continue to use it? I
> dare not touch that code...
> 
> Anyway, this series looks like a nice cleanup to me, and the recent sccb
> patch from Akinobu Mita could also benefit, as indicated by Wolfram [1].

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]