Date:   Tue, 30 Nov 2021 10:26:08 +0100
From:   Jean Delvare <jdelvare@suse.de>
To:     Wolfram Sang <wsa@kernel.org>
Cc:     Hector Martin <marcan@marcan.st>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Subject: Re: [PATCH v3] i2c: i801: Safely share SMBus with BIOS/ACPI
Message-ID: <20211130102608.57e2171d@endymion>
In-Reply-To: <YaSWx7ldFfbCmrK3@kunai>
References: <20210626054113.246309-1-marcan@marcan.st>
        <YaSWx7ldFfbCmrK3@kunai>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 29 Nov 2021 10:00:55 +0100, Wolfram Sang wrote:
> On Sat, Jun 26, 2021 at 02:41:13PM +0900, Hector Martin wrote:
> > The i801 controller provides a locking mechanism that the OS is supposed
> > to use to safely share the SMBus with ACPI AML or other firmware.
> > 
> > Previously, Linux attempted to get out of the way of ACPI AML entirely,
> > but left the bus locked if it used it before the first AML access. This
> > causes AML implementations that *do* attempt to safely share the bus
> > to time out if Linux uses it first; notably, this regressed ACPI video
> > backlight controls on 2015 iMacs after 01590f361e started instantiating
> > SPD EEPROMs on boot.
> > 
> > Commit 065b6211a8 fixed the immediate problem of leaving the bus locked,
> > but we can do better. The controller does have a proper locking mechanism,
> > so let's use it as intended. Since we can't rely on the BIOS doing this
> > properly, we implement the following logic:
> > 
> > - If ACPI AML uses the bus at all, we make a note and disable power
> >   management. The latter matches already existing behavior.
> > - When we want to use the bus, we attempt to lock it first. If the
> >   locking attempt times out, *and* ACPI hasn't tried to use the bus at
> >   all yet, we cautiously go ahead and assume the BIOS forgot to unlock
> >   the bus after boot. This preserves existing behavior.
> > - We always unlock the bus after a transfer.
> > - If ACPI AML tries to use the bus (except trying to lock it) while
> >   we're in the middle of a transfer, or after we've determined
> >   locking is broken, we know we cannot safely share the bus and give up.
> > 
> > Upon first usage of SMBus by ACPI AML, if nothing has gone horribly
> > wrong so far, users will see:
> > 
> > i801_smbus 0000:00:1f.4: SMBus controller is shared with ACPI AML. This seems safe so far.
> > 
> > If locking the SMBus times out, users will see:
> > 
> > i801_smbus 0000:00:1f.4: BIOS left SMBus locked
> > 
> > And if ACPI AML tries to use the bus concurrently with Linux, or it
> > previously used the bus and we failed to subsequently lock it as
> > above, the driver will give up and users will get:
> > 
> > i801_smbus 0000:00:1f.4: BIOS uses SMBus unsafely
> > i801_smbus 0000:00:1f.4: Driver SMBus register access inhibited
> > 
> > This fixes the regression introduced by 01590f361e, and further allows
> > safely sharing the SMBus on 2015 iMacs. Tested by running `i2cdump` in a
> > loop while changing backlight levels via the ACPI video device.
> > 
> > Fixes: 01590f361e ("i2c: i801: Instantiate SPD EEPROMs automatically")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Hector Martin <marcan@marcan.st>  
> 
> Jean, Heiner, what do we do with this topic?

I like the idea, I need to give it a try and review the code.

-- 
Jean Delvare
SUSE L3 Support