linux-kernel - Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

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: <1607326431.25719.33.camel@mhfsdcap03>
Date:   Mon, 7 Dec 2020 15:33:51 +0800
From:   Qii Wang <qii.wang@...iatek.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
CC:     Wolfram Sang <wsa@...-dreams.de>, <matthias.bgg@...il.com>,
        <linux-i2c@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>,
        <srv_heupstream@...iatek.com>, <leilk.liu@...iatek.com>
Subject: Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ
 phase

Hi:
Thank you very much for your patience review.
There are two main purposes of this patch:
1.i2c_mark_adapter_suspended&i2c_mark_adapter_resumed
Avoid accessing the adapter while it is suspended by marking it
suspended during suspend.  This allows the I2C core to catch this, and
print a warning.
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20181219164827.20985-2-wsa+renesas@sang-engineering.com/

2. IRQF_NO_SUSPEND.
Having interrupts disabled means not only that an interrupt will not
occur at an awkward time, but also that using any functionality that
requires interrupts will not work. So if the driver uses an I2C bus or
similar to tell the device to turn off, and if the I2C bus uses
interrupts to indicate completion (which is normal), then either the
device must be powered-off in suspend_late, so the I2C interrupt must be
marked IRQF_NO_SUSPEND.
https://patchwork.kernel.org/project/linux-acpi/patch/20180923135812.29574-8-hdegoede@redhat.com/

On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
> 
> On 03/12/2020 03:25, Qii Wang wrote:
> > On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> >> Hi,
> >>
> >>> Some i2c device driver indirectly uses I2C driver when it is now
> >>> being suspended. The i2c devices driver is suspended during the
> >>> NOIRQ phase and this cannot be changed due to other dependencies.
> >>> Therefore, we also need to move the suspend handling for the I2C
> >>> controller driver to the NOIRQ phase as well.
> >>>
> >>> Signed-off-by: Qii Wang <qii.wang@...iatek.com>
> >>
> >> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> >>
> > 
> > Yes, Can you help to apply it into 5.10? Thanks
> 
> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
> 

At present, it is only a problem caused by missing interrupts,
and .master_xfer_atomic() just a implement in polling mode. Why not set
the interrupt to a state that can always be triggered?