Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c adapters

From: Wolfram Sang <wsa@the-dreams.de>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Lukas Wunner <lukas@wunner.de>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c adapters
Date: Fri, 21 Dec 2018 15:50:22 +0100	[thread overview]
Message-ID: <20181221145022.umsugdwadw5a5ag6@ninjato> (raw)
In-Reply-To: <e8bfcadc-7c1a-6659-7173-4ce484ceeca3@redhat.com>

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

> > > 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.

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