linux-kernel - Re: [PATCH] i2c: designware: Fix unbalanced suspended flag

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] [day] [month] [year] [list]

Message-ID: <6338cce6-c13d-bda3-6f0b-06767122076c@redhat.com>
Date:   Fri, 9 Dec 2022 19:16:21 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>, wsa@...nel.org,
        jarkko.nikula@...ux.intel.com, andriy.shevchenko@...ux.intel.com,
        mika.westerberg@...ux.intel.com, jsd@...ihalf.com
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH] i2c: designware: Fix unbalanced suspended flag

Hi,

On 12/9/22 15:22, Richard Fitzgerald wrote:
> On 9/12/22 13:36, Richard Fitzgerald wrote:
>> On 9/12/22 12:15, Hans de Goede wrote:
>>> Hi Richard,
>>>
>>> On 12/9/22 12:40, Richard Fitzgerald wrote:
>>>> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
>>>> i2c_mark_adapter_resumed().
>>
>> <snip>
>>
>>>
>>> It is not entirely clear to me where the unbalance you claim to see comes
>>> from? When runtime-suspended SMART_SUSPEND should keep it suspended at which point
>>> the system suspend callback will never run ?
>>>
>>> Are you sure that you are not maybe seeing a suspend/resume ordering issue?
>>>
>>> Did you add printk messages to the suspend/resume callbacks of
>>> i2c-designware-platdrv.c which show the system suspend callback
>>> being called but not the system resume one ?
>>>
>>
>> With messages in strategic places.
>>
>> [  169.607358] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=0 pm_runtime_status_suspended=1
>> [  169.607361] i2c_designware i2c_designware.2: PM: __device_suspend_late: dev_pm_skip_suspend:false
>> [  169.607364] i2c_designware i2c_designware.2: dw_i2c_plat_suspend
>> ...
>> [  169.702511] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume
>> [  169.706241] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume
>> [  169.706244] i2c_designware i2c_designware.2: PM: device_resume_early: dev_pm_skip_resume:true
>> ...
>> [  175.254832] i2c i2c-2: Transfer while suspended
>>
>> (Just to prove my logging isn't lying, for i2c3 it reports
>> SMART_SUSPEND=1)
>>
> 
> Oh, that's embarrassing. After confidently telling you my logging
> is perfect, actually there was a bug in it...
> 
> New log summary:
> 
> [  162.253431] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=1 pm_runtime_status_suspended=0

Ok, so the device's pm_runtime_get() count is 0 here (otherwise must_resume
should be 1 later on) but the device is not run-time suspended yet. Probably
because of some timeout; or because of runtime pm getting disabled durig suspend
before the count dropped to 0.

And this scenario will indeed cause the system-level suspend callback to
get called, but not the resume one ...

> [  162.253438] i2c_designware i2c_designware.2: PM: __device_suspend_late: dev_pm_skip_suspend:false
> [  162.253445] i2c_designware i2c_designware.2: dw_i2c_plat_suspend
> [  162.273115] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=1 pm_runtime_status_suspended=0
> [  162.362547] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume
> [  162.369216] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume
> [  162.369220] i2c_designware i2c_designware.2: PM: device_resume_early: dev_pm_skip_resume:true
> [  167.901269] i2c i2c-2: Transfer while suspended
> 
> Same result that it doesn't skip suspend but does skip resume.

>From your other email:

> Ok, what do you suggest as the fix?
> If you post an alternate fix I can test it.

I don't really see a better solution, so lets go with your solution, but then:

1. Simply drop the flag but don't add the if (!pm_runtime_suspended(dev))
check. The runtime status is always going to be set to active at this point
so the check does not do anything.

2. Drop the dw_i2c_plat_complete() callback since we now always resume the controller
on system resume.

Regards,

Hans