linux-kernel - Re: [PATCH v4] i2c/pasemi: PASemi I2C controller IRQ enablement

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eec45f54-097b-1f16-f572-44d9499a51c0@marcan.st>
Date:   Mon, 7 Nov 2022 14:14:32 +0900
From:   Hector Martin <marcan@...can.st>
To:     Arminder Singh <arminders208@...look.com>,
        linux-kernel@...r.kernel.org
Cc:     linux-i2c@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-arm-kernel@...ts.infradead.org,
        Paul Mackerras <paulus@...ba.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Sven Peter <sven@...npeter.dev>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        asahi@...ts.linux.dev
Subject: Re: [PATCH v4] i2c/pasemi: PASemi I2C controller IRQ enablement

On 05/11/2022 20.56, Arminder Singh wrote:
> This patch adds IRQ support to the PASemi I2C controller driver to
> increase the performace of I2C transactions on platforms with PASemi I2C
> controllers. While primarily intended for Apple silicon platforms, this
> patch should also help in enabling IRQ support for older PASemi hardware
> as well should the need arise.
> 
> This version of the patch has been tested on an M1 Ultra Mac Studio,
> as well as an M1 MacBook Pro, and userspace launches successfully
> while using the IRQ path for I2C transactions.
> 
> Signed-off-by: Arminder Singh <arminders208@...look.com>
> ---
> This version of the patch fixes some reliability issues brought up by
> Hector and Sven in the v3 patch email thread. First, this patch
> increases the timeout value in pasemi_smb_waitready to 100ms from 10ms,
> as the original 10ms timeout in the driver was incorrect according to the
> controller's datasheet as Hector pointed out in the v3 patch email thread.
> This incorrect timeout had caused some issues with the tps6598x controller
> on Apple silicon platforms.
> 
> This version of the patch also adds a reg_write to REG_IMASK in the IRQ
> handler, because as Sven pointed out in the previous thread, the I2C
> transaction interrupt is level sensitive so not masking the interrupt in
> REG_IMASK will cause the interrupt to trigger again when it leaves the IRQ
> handler until it reaches the call to reg_write after the completion expires.
> 
> Patch changelog:
> 
> v3 to v4 changes:
>  - Increased the timeout value for I2C transactions to 100ms, as the original
>    10ms timeout in the driver was incorrect according to the I2C chip's
>    datasheet. Mitigates an issue with the tps6598x controller on Apple
>    silicon platforms.
>  - Added a reg_write to REG_IMASK inside the IRQ handler, which prevents
>    the IRQ from triggering again after leaving the IRQ handler, as the
>    IRQ is level-sensitive.
> 
> v2 to v3 changes:
>  - Fixed some whitespace and alignment issues found in v2 of the patch
> 
> v1 to v2 changes:
>  - moved completion setup from pasemi_platform_i2c_probe to
>    pasemi_i2c_common_probe to allow PASemi and Apple platforms to share
>    common completion setup code in case PASemi hardware gets IRQ support
>    added
>  - initialized the status variable in pasemi_smb_waitready when going down
>    the non-IRQ path
>  - removed an unnecessary cast of dev_id in the IRQ handler
>  - fixed alignment of struct member names in i2c-pasemi-core.h
>    (addresses Christophe's feedback in the original submission)
>  - IRQs are now disabled after the wait_for_completion_timeout call
>    instead of inside the IRQ handler
>    (prevents the IRQ from going off after the completion times out)
>  - changed the request_irq call to a devm_request_irq call to obviate
>    the need for a remove function and a free_irq call
>    (thanks to Sven for pointing this out in the original submission)
>  - added a reinit_completion call to pasemi_reset 
>    as a failsafe to prevent missed interrupts from causing the completion
>    to never complete (thanks to Arnd Bergmann for pointing this out)
>  - removed the bitmask variable in favor of just using the value
>    directly (it wasn't used anywhere else)
> 
> v3: https://lore.kernel.org/linux-i2c/MN2PR01MB5358ED8FC32C0CFAEBD4A0E19F5F9@MN2PR01MB5358.prod.exchangelabs.com/T/
> 
> v2: https://lore.kernel.org/linux-i2c/MN2PR01MB535821C8058C7814B2F8EEDF9F599@MN2PR01MB5358.prod.exchangelabs.com/T/
> 
> v1: https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@MN2PR01MB5358.prod.exchangelabs.com/T/
> 
>  drivers/i2c/busses/i2c-pasemi-core.c     | 32 ++++++++++++++++++++----
>  drivers/i2c/busses/i2c-pasemi-core.h     |  5 ++++
>  drivers/i2c/busses/i2c-pasemi-platform.c |  6 +++++
>  3 files changed, 38 insertions(+), 5 deletions(-)
> 

Reviewed-by: Hector Martin <marcan@...can.st>

- Hector