[01/10] i2c: add suspended flag and accessors for i2c adapters
diff mbox series

Message ID 20181219164827.20985-2-wsa+renesas@sang-engineering.com
State Superseded
Headers show
Series
  • i2c: move handling of suspended adapters to the core
Related show

Commit Message

Wolfram Sang Dec. 19, 2018, 4:48 p.m. UTC
A few drivers open code handling of suspended adapters. It could be
handled by the core, though, to ensure generic handling. This patch adds
the flag and accessor functions. The usage of these helpers is optional,
though. See the kerneldoc in this patch.

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

Comments

Lukas Wunner Dec. 19, 2018, 5:22 p.m. UTC | #1
On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> +{
> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}

This looks like a duplication of the is_suspended flag in struct dev_pm_info.
Any reason why you can't use that?  If so, it would be good to document the
reason in the commit message.

If the point is to constrain refusal of transfers in suspended state to
certain drivers, those drivers could opt in to that functionality by
setting a flag, and the i2c core could then gate refusal based on
that flag and the is_suspended flag in struct dev_pm_info.

Also, why is it necessary to take a lock to perform an atomic bitop?
(Sorry if that's a dumb question, seems non-obvious to me.)

Thanks,

Lukas
Hans de Goede Dec. 19, 2018, 6:36 p.m. UTC | #2
Hi,

On 19-12-18 18:22, Lukas Wunner wrote:
> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>> +{
>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>> +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>> +}
> 
> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> Any reason why you can't use that?  If so, it would be good to document the
> reason in the commit message.

Oh, that is a very good point and that one only gets set on system suspend
and not on resume suspend, working around the problems with the i2c-designware
driver.

I think this might be as simple as adding:

	if (WARN_ON(adap->dev.parent->power.is_suspended))
		return -ESHUTDOWN;

To __i2c_transfer (and document the -ESHUTDOWN) combined with removing
all the now no longer DIY code from various bus drivers.

Regards,

Hans




> 
> If the point is to constrain refusal of transfers in suspended state to
> certain drivers, those drivers could opt in to that functionality by
> setting a flag, and the i2c core could then gate refusal based on
> that flag and the is_suspended flag in struct dev_pm_info.
> 
> Also, why is it necessary to take a lock to perform an atomic bitop?
> (Sorry if that's a dumb question, seems non-obvious to me.)
> 
> Thanks,
> 
> Lukas
>
Wolfram Sang Dec. 19, 2018, 10:33 p.m. UTC | #3
Hi Lukas, Hans,

On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 19-12-18 18:22, Lukas Wunner wrote:
> > On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> > > +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> > > +{
> > > +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> > > +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > +}
> > 
> > This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> > Any reason why you can't use that?  If so, it would be good to document the
> > reason in the commit message.
> 
> Oh, that is a very good point and that one only gets set on system suspend
> and not on resume suspend, working around the problems with the i2c-designware

Just to make it clear: you mean runtime suspend, not resume suspend, or?

> driver.
> 
> I think this might be as simple as adding:
> 
> 	if (WARN_ON(adap->dev.parent->power.is_suspended))
> 		return -ESHUTDOWN;

I have seen this flag but decided against it. One reason is because it
is marked as "PM core only". The other reason is that it doesn't know
about the adapter lock. It might get set while a transfer is on going.
Or even right after the suggested if-block above. The code from this
series gets the mutex first which ensures that on going transfers are
completed and no new ones are started in parallel.

Unless I am totally overlooking something...

Thanks,

   Wolfram
Hans de Goede Dec. 20, 2018, 10 a.m. UTC | #4
Hi,

On 19-12-18 23:33, Wolfram Sang wrote:
> Hi Lukas, Hans,
> 
> On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 19-12-18 18:22, Lukas Wunner wrote:
>>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>>>> +{
>>>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>> +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>>>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>> +}
>>>
>>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
>>> Any reason why you can't use that?  If so, it would be good to document the
>>> reason in the commit message.
>>
>> Oh, that is a very good point and that one only gets set on system suspend
>> and not on resume suspend, working around the problems with the i2c-designware
> 
> Just to make it clear: you mean runtime suspend, not resume suspend, or?

Yes I mean runtime-suspend, sorry.

>> driver.
>>
>> I think this might be as simple as adding:
>>
>> 	if (WARN_ON(adap->dev.parent->power.is_suspended))
>> 		return -ESHUTDOWN;
> 
> I have seen this flag but decided against it. One reason is because it
> is marked as "PM core only".

Right and we definitely should not be touching it, but reading it should
be fine.

> The other reason is that it doesn't know
> about the adapter lock. It might get set while a transfer is on going.
> Or even right after the suggested if-block above. The code from this
> series gets the mutex first which ensures that on going transfers are
> completed and no new ones are started in parallel.
> 
> Unless I am totally overlooking something...

No you are right, there is a race here, but I don't think we are likely to
hit that race. Normally there won't be any ongoing i2c-transfers during
a system suspend and more over, the goal of adding this check is to help
find problems, so even if the check sometimes does not trigger because
of the race that is not really a big deal.

I think we need to get really unlucky to have both a suspend ordering
problem in the first case (already a somewhat rare thing) combined with
hitting this race in such a way *each time* that we don't trigger the
WARN_ON.

To me this seems a case of perfect being the enemy of good. When we
first started discussing this you wanted to not have to modify the
adapter/bus drivers for the check, using adap->dev.parent->power.is_suspended
gives us that and it will also work for complex cases like
the i2c-designware case, so I believe the benefits outway the downsides.

Regards,

Hans
Rafael J. Wysocki Dec. 20, 2018, 9:09 p.m. UTC | #5
On Thursday, December 20, 2018 11:00:29 AM CET Hans de Goede wrote:
> Hi,
> 
> On 19-12-18 23:33, Wolfram Sang wrote:
> > Hi Lukas, Hans,
> > 
> > On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 19-12-18 18:22, Lukas Wunner wrote:
> >>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> >>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> >>>> +{
> >>>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> >>>> +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> >>>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> >>>> +}
> >>>
> >>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> >>> Any reason why you can't use that?  If so, it would be good to document the
> >>> reason in the commit message.
> >>
> >> Oh, that is a very good point and that one only gets set on system suspend
> >> and not on resume suspend, working around the problems with the i2c-designware
> > 
> > Just to make it clear: you mean runtime suspend, not resume suspend, or?
> 
> Yes I mean runtime-suspend, sorry.

The power.is_suspended flag is about system-wide suspend, however.
Hans de Goede Dec. 21, 2018, 10:43 a.m. UTC | #6
Hi,

On 20-12-18 22:09, Rafael J. Wysocki wrote:
> On Thursday, December 20, 2018 11:00:29 AM CET Hans de Goede wrote:
>> Hi,
>>
>> On 19-12-18 23:33, Wolfram Sang wrote:
>>> Hi Lukas, Hans,
>>>
>>> On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 19-12-18 18:22, Lukas Wunner wrote:
>>>>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>>>>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>>>>>> +{
>>>>>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>>>> +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>>>>>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>>>> +}
>>>>>
>>>>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
>>>>> Any reason why you can't use that?  If so, it would be good to document the
>>>>> reason in the commit message.
>>>>
>>>> Oh, that is a very good point and that one only gets set on system suspend
>>>> and not on resume suspend, working around the problems with the i2c-designware
>>>
>>> Just to make it clear: you mean runtime suspend, not resume suspend, or?
>>
>> Yes I mean runtime-suspend, sorry.
> 
> The power.is_suspended flag is about system-wide suspend, however.

Right, which is why it is good for us to use, when runtime-suspend the
i2c-adapter drivers transfer function will do a runtime_pm_get and all
is well, we want to check for someone trying to do i2c-transfers on
the adapter while it is system-suspended, since then the runtime_pm_get
is a no-op and things fail.

So for this use case it is a good thing that power.is_suspended flag is about
system-wide suspend (which is what I was trying to say in the first place).

Regards,

Hans
Wolfram Sang Dec. 21, 2018, 2:50 p.m. UTC | #7
> > > I think this might be as simple as adding:
> > > 
> > > 	if (WARN_ON(adap->dev.parent->power.is_suspended))
> > > 		return -ESHUTDOWN;

Peter, I think this should work for muxes, too, or? The i2c_transfer()
call to the mux will not be rejected, but it will be later when we reach
the root adapter. And then the error code will be pushed down the tree
until we arrive at the mux again. So, the rejection will not happen at
the earliest time, but it will happen. Is my understanding correct?

> > I have seen this flag but decided against it. One reason is because it
> > is marked as "PM core only".
> 
> Right and we definitely should not be touching it, but reading it should
> be fine.

Seems like it. So far, no rejection from the other PM people :)

> No you are right, there is a race here, but I don't think we are likely to
> hit that race. Normally there won't be any ongoing i2c-transfers during
> a system suspend and more over, the goal of adding this check is to help
> find problems, so even if the check sometimes does not trigger because
> of the race that is not really a big deal.

You are right that the impact of a missed detection is not fatal. That
helps. The low likeliness was not an argument for me, though, because
detecting rare things is very important IMO. Because, well, they are
rare and especially hard to debug.

> I think we need to get really unlucky to have both a suspend ordering
> problem in the first case (already a somewhat rare thing) combined with
> hitting this race in such a way *each time* that we don't trigger the
> WARN_ON.

And here you convinced me: even if we miss a detection once, I agree it
is super super unlikely to hit the race every time.

> To me this seems a case of perfect being the enemy of good. When we
> first started discussing this you wanted to not have to modify the
> adapter/bus drivers for the check, using adap->dev.parent->power.is_suspended
> gives us that and it will also work for complex cases like
> the i2c-designware case, so I believe the benefits outway the downsides.

I'll try it.
Peter Rosin Dec. 21, 2018, 6:40 p.m. UTC | #8
On 2018-12-21 15:50, Wolfram Sang wrote:
> 
>>>> I think this might be as simple as adding:
>>>>
>>>> 	if (WARN_ON(adap->dev.parent->power.is_suspended))
>>>> 		return -ESHUTDOWN;
> 
> Peter, I think this should work for muxes, too, or? The i2c_transfer()
> call to the mux will not be rejected, but it will be later when we reach
> the root adapter. And then the error code will be pushed down the tree
> until we arrive at the mux again. So, the rejection will not happen at
> the earliest time, but it will happen. Is my understanding correct?

Yes, I agree with that analysis. All mux actions eventually end up with
an __i2c_transfer() call on the relevant root adapter. Hmm, but not *all*
calls. How about SMBus adapters? Should there not be a similar WARN_ON
in __i2c_smbus_xfer?

But maybe that's not applicable for some reason? Just asking...

Cheers,
Peter
Wolfram Sang Dec. 21, 2018, 6:50 p.m. UTC | #9
Hi Peter,

> Yes, I agree with that analysis. All mux actions eventually end up with

Good!

> an __i2c_transfer() call on the relevant root adapter. Hmm, but not *all*
> calls. How about SMBus adapters? Should there not be a similar WARN_ON
> in __i2c_smbus_xfer?

Yes, there should. Yeah, that's what I like about our way of hacking; I
somewhen thought to not forget about SMBus, nevertheless forgot about it
again, and then there is someone else to remind me \o/

Thanks,

   Wolfram

Patch
diff mbox series

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..2c465455b2f6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1232,6 +1232,7 @@  static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (!adap->lock_ops)
 		adap->lock_ops = &i2c_adapter_lock_ops;
 
+	adap->locked_flags = 0;
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
 	mutex_init(&adap->userspace_clients_lock);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..b7401c55dc83 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -680,6 +680,8 @@  struct i2c_adapter {
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
+	unsigned long locked_flags;	/* owned by the I2C core */
+#define I2C_ALF_IS_SUSPENDED	0
 
 	int nr;
 	char name[48];
@@ -762,6 +764,38 @@  i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
 	adapter->lock_ops->unlock_bus(adapter, flags);
 }
 
+/**
+ * i2c_mark_adapter_suspended - Report suspended state of the adapter to the core
+ * @adap: Adapter to mark as suspended
+ *
+ * When using this helper to mark an adapter as suspended, the core will reject
+ * further transfers to this adapter. The usage of this helper is optional but
+ * recommended for devices having distinct handlers for systems suspend and
+ * runtime suspend. More complex devices are free to implement custom solutions
+ * to reject transfers when suspended.
+ */
+static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
+{
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
+/**
+ * i2c_mark_adapter_resumed - Report resumed state of the adapter to the core
+ * @adap: Adapter to mark as resumed
+ *
+ * When using this helper to mark an adapter as resumed, the core will allow
+ * further transfers to this adapter. See also further notes to
+ * @i2c_mark_adapter_suspended().
+ */
+static inline void i2c_mark_adapter_resumed(struct i2c_adapter *adap)
+{
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	clear_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
 /*flags for the client struct: */
 #define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
 #define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */