[RFC,4/4] i2c: core: introduce master_xfer_irqless callback

Message ID 20180920161423.13990-5-wsa+renesas@sang-engineering.com
State New
Headers show
Series
  • i2c: core: introduce master_xfer_irqless
Related show

Commit Message

Wolfram Sang Sept. 20, 2018, 4:14 p.m.
We had the request to access devices very late when interrupts are not
available anymore multiple times now. Mostly to prepare shutdown or
reboot. Allow adapters to specify a specific callback for this case.
Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  6 +++++-
 include/linux/i2c.h         | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Peter Rosin Sept. 20, 2018, 5:41 p.m. | #1
On 2018-09-20 18:14, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or

The first sentence is a bit backwards, I'd rephrase like so:

Multiple times now we've had the request to access devices very late, when
interrupts are no longer available.

> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

accidentally

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts

"Same" (with capital 'S') to match the other entries. Also, should it
not be @master_xfer to help the tools do the right thing?

> + *   so e.g. PMICs can be accessed very late before shutdown

Trailing period.

I'm fine with this change, but should it not wait until there is a user?
(I think there is one in the wings, so that's a very weak objection...)

Acked-by: Peter Rosin <peda@axentia.se>


>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
>
Wolfram Sang Sept. 20, 2018, 10:55 p.m. | #2
On Thu, Sep 20, 2018 at 07:41:05PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > We had the request to access devices very late when interrupts are not
> > available anymore multiple times now. Mostly to prepare shutdown or
> 
> The first sentence is a bit backwards, I'd rephrase like so:
> 
> Multiple times now we've had the request to access devices very late, when
> interrupts are no longer available.

Ok. Don't see much difference, but I don't mind.

> > reboot. Allow adapters to specify a specific callback for this case.
> > Note that we fall back to the generic master_xfer callback if this new
> > irqless one is not present. This is intentional to preserve the previous
> > behaviour and avoid regressions. Because there are drivers not using
> > interrupts or because it might have worked "accidently" before.
> 
> accidentally

Thanks.

> > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> >   *   defined by the msgs array, with num messages available to transfer via
> >   *   the adapter specified by adap.
> > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> 
> "Same" (with capital 'S') to match the other entries. Also, should it
> not be @master_xfer to help the tools do the right thing?

I'll check. You are probably right.

> > + *   so e.g. PMICs can be accessed very late before shutdown
> 
> Trailing period.
> 
> I'm fine with this change, but should it not wait until there is a user?
> (I think there is one in the wings, so that's a very weak objection...)

As I mentioned in the cover-letter, this series is RFC because it is
mainly meant as assistance for Stefan, so he could base his imx patches
on top of it. Or the TI folks for their omap driver.

I somewhen need to implement irqless transfers for the i2c-sh_mobile
driver. But this may take a while, so I hope the others are first. And
yes, I won't apply this series without a user and proper testing.

Thanks for the review, Peter!
Russell King - ARM Linux Oct. 18, 2018, 10:44 a.m. | #3
On Thu, Sep 20, 2018 at 06:14:23PM +0200, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

It's not really about "irqless", it's about being in atomic context.
irqless makes it sound like you may sleep, but the reality is sleeping
is also out (the scheduler needs IRQs to do it's job.)

So, it may be tempting to use things like msleep() in "irqless" but
that's not permissable.  So surely "atomic" would be a better name for
this?

Also, how does this get around the issue which I pointed out with (eg)
an oops occuring, which leads to a panic followed by an attempt to
reboot if the I2C bus in question is already mid-transaction?  Won't
we deadlock?

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> + *   so e.g. PMICs can be accessed very late before shutdown
>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 904b4d2ebefa..f827446c3089 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1887,7 +1887,11 @@  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Retry automatically on arbitration loss */
 	orig_jiffies = jiffies;
 	for (ret = 0, try = 0; try <= adap->retries; try++) {
-		ret = adap->algo->master_xfer(adap, msgs, num);
+		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
+			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
+		else
+			ret = adap->algo->master_xfer(adap, msgs, num);
+
 		if (ret != -EAGAIN)
 			break;
 		if (time_after(jiffies, orig_jiffies + adap->timeout))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..11e615123bd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -498,6 +498,8 @@  i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
  *   defined by the msgs array, with num messages available to transfer via
  *   the adapter specified by adap.
+ * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
+ *   so e.g. PMICs can be accessed very late before shutdown
  * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
  *   is not present, then the bus layer will try and convert the SMBus calls
  *   into I2C transfers instead.
@@ -511,9 +513,9 @@  i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
  * to name two of the most common.
  *
- * The return codes from the @master_xfer field should indicate the type of
- * error code that occurred during the transfer, as documented in the kernel
- * Documentation file Documentation/i2c/fault-codes.
+ * The return codes from the @master_xfer{_irqless} field should indicate the
+ * type of error code that occurred during the transfer, as documented in the
+ * Kernel Documentation file Documentation/i2c/fault-codes.
  */
 struct i2c_algorithm {
 	/* If an adapter algorithm can't do I2C-level access, set master_xfer
@@ -524,6 +526,8 @@  struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_xfer_irqless)(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);