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: <c210c1d6-7327-d377-22e7-b5a123de5cbb@opensource.cirrus.com>
Date:   Fri, 9 Dec 2022 14:22:14 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH] i2c: designware: Fix unbalanced suspended flag
Content-Language: en-US
From:   Richard Fitzgerald <rf@opensource.cirrus.com>
To:     Hans de Goede <hdegoede@redhat.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>
In-Reply-To: <e8b6733c-33b8-cb28-a62b-21dad9bd6466@opensource.cirrus.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
[  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.