Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6338cce6-c13d-bda3-6f0b-06767122076c@redhat.com>
Date:   Fri, 9 Dec 2022 19:16:21 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH] i2c: designware: Fix unbalanced suspended flag
To:     Richard Fitzgerald <rf@opensource.cirrus.com>, wsa@kernel.org,
        jarkko.nikula@linux.intel.com, andriy.shevchenko@linux.intel.com,
        mika.westerberg@linux.intel.com, jsd@semihalf.com
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com
References: <20221209114034.18025-1-rf@opensource.cirrus.com>
 <e9d113fb-5cd1-d93d-3d8f-fa9c1e55a8e2@redhat.com>
 <e8b6733c-33b8-cb28-a62b-21dad9bd6466@opensource.cirrus.com>
 <c210c1d6-7327-d377-22e7-b5a123de5cbb@opensource.cirrus.com>
Content-Language: en-US
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <c210c1d6-7327-d377-22e7-b5a123de5cbb@opensource.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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