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: <50578b41-8894-24fe-a768-25c55fa38a78@quicinc.com>
Date:   Thu, 1 Dec 2022 20:24:20 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: PM-runtime: supplier looses track of consumer during probe
Content-Language: en-US
To:     Adrian Hunter <adrian.hunter@intel.com>,
        Tushar Nimkar <quic_tnimkar@quicinc.com>,
        <linux-pm@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
CC:     <linux-arm-msm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        <bjorn.andersson@kernel.org>, <quic_mkshah@quicinc.com>,
        <quic_lsrao@quicinc.com>, <bvanassche@acm.org>,
        Peter Wang <peter.wang@mediatek.com>
References: <36aed941-a73e-d937-2721-4f0decd61ce0@quicinc.com>
 <8c0a715a-d626-aa70-15f1-79f1e23fbc67@quicinc.com>
 <a5e2aab6-7f0e-7f3b-f34b-6d222450c97d@intel.com>
 <8ca27fcb-b146-3ea7-a042-55f99e0ae3fb@quicinc.com>
 <2a1047a7-3121-6cbe-d4c5-46bbff0c5cc5@quicinc.com>
 <20aae21e-62d2-8fdb-b57a-7b5a180266d8@intel.com>
From:   Nitin Rawat <quic_nitirawa@quicinc.com>
In-Reply-To: <20aae21e-62d2-8fdb-b57a-7b5a180266d8@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Adrian,
Thanks for the patch.
I Agree, using local variable to store the runtime status before 
dev->power.lock is released and using this variable later in the code 
can meet the intention of code and can help to solve this race.

We will get it tested and update you.


Regards,
Nitin

On 12/1/2022 6:39 PM, Adrian Hunter wrote:
> On 29/11/22 18:56, Nitin Rawat wrote:
>> Hi Adrian,
>>
>> On 11/21/2022 11:38 AM, Tushar Nimkar wrote:
>>> Hi Adrian,
>>>
>>> On 11/18/2022 8:25 PM, Adrian Hunter wrote:
>>>> On 4/11/22 11:19, Tushar Nimkar wrote:
>>>>> Hi linux-pm/linux-scsi,
>>>
>>>>>> Process -1
>>>>>> ufshcd_async_scan context (process 1)
>>>>>> scsi_autopm_put_device() //0:0:0:0
>>>>
>>>> I am having trouble following your description.  What function is calling
>>>> scsi_autopm_put_device() here?
>>>>
>>> Below is flow which calls scsi_autopm_put_device()
>>> Process -1
>>> ufshcd_async_scan()
>>>       scsi_probe_and_add_lun()
>>>           scsi_add_lun()
>>>               slave_configure()
>>>                   scsi_sysfs_add_sdev()
>>>                       scsi_autopm_get_device()
>>>                           device_add()     <- invoked [Process 2] sd_probe()
>>>                               scsi_autopm_put_device()
>>>
>>>>>> pm_runtime_put_sync()
>>>>>> __pm_runtime_idle()
>>>>>> rpm_idle() -- RPM_GET_PUT(4)
>>>>>>        __rpm_callback
>>>>>>            scsi_runtime_idle()
>>>>>>                pm_runtime_mark_last_busy()
>>>>>>                pm_runtime_autosuspend()  --[A]
>>>>>>                    rpm_suspend() -- RPM_AUTO(8)
>>>>>>                        pm_runtime_autosuspend_expiration() use_autosuspend    is false return 0   --- [B]
>>>>>>                            __update_runtime_status to RPM_SUSPENDING
>>>>>>                        __rpm_callback()
>>>>>>                            __rpm_put_suppliers(dev, false)
>>>>>>                        __update_runtime_status to RPM_SUSPENDED
>>>>>>                    rpm_suspend_suppliers()
>>>>>>                        rpm_idle() for supplier -- RPM_ASYNC(1) return (-EAGAIN) [ Other consumer active for supplier]
>>>>>>                    rpm_suspend() – END with return=0
>>>>>>            scsi_runtime_idle() END return (-EBUSY) always.
>>>>
>>>> Not following here either.  Which device is EBUSY and why?
>>>
>>> scsi_runtime_idle() return -EBUSY always [3]
>>> Storage/scsi team can better explain -EBUSY implementation.
>>
>> EBUSY is returned from below code for consumer dev 0:0:0:0.
>> scsi_runtime_idle is called from scsi_autopm_put_device which inturn is called from ufshcd_async_scan (Process 1 as per above call stack)
>> static int scsi_runtime_idle(struct device *dev)
>> {
>>      :
>>
>>      if (scsi_is_sdev_device(dev)) {
>>          pm_runtime_mark_last_busy(dev);
>>          pm_runtime_autosuspend(dev);
>>          return -EBUSY; ---> EBUSY returned from here.
>>      }
>>
>>      
>> }
>>
>>>
>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/scsi/scsi_pm.c?h=next-20221118#n210
>>>
>>>
>>>>>>
>>>>>> [1]: https://lore.kernel.org/lkml/4748074.GXAFRqVoOG@kreacher/T/
>>>>>> [2]: https://lkml.org/lkml/2022/10/12/259
> 
> It looks to me like __rpm_callback() makes assumptions about
> dev->power.runtime_status that are not necessarily true because
> dev->power.lock is dropped.  AFAICT the intention of the code
> would be fulfilled by instead using the status as it was before
> the lock was dropped.
> 
> Consequently, perhaps you could try this:
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b52049098d4e..3cf9abc3b2c2 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -365,6 +365,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>   {
>   	int retval = 0, idx;
>   	bool use_links = dev->power.links_count > 0;
> +	enum rpm_status runtime_status = dev->power.runtime_status;
>   
>   	if (dev->power.irq_safe) {
>   		spin_unlock(&dev->power.lock);
> @@ -378,7 +379,7 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>   		 * routine returns, so it is safe to read the status outside of
>   		 * the lock.
>   		 */
> -		if (use_links && dev->power.runtime_status == RPM_RESUMING) {
> +		if (use_links && runtime_status == RPM_RESUMING) {
>   			idx = device_links_read_lock();
>   
>   			retval = rpm_get_suppliers(dev);
> @@ -405,8 +406,8 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>   		 * Do that if resume fails too.
>   		 */
>   		if (use_links
> -		    && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
> -		    || (dev->power.runtime_status == RPM_RESUMING && retval))) {
> +		    && ((runtime_status == RPM_SUSPENDING && !retval)
> +		    || (runtime_status == RPM_RESUMING && retval))) {
>   			idx = device_links_read_lock();
>   
>   			__rpm_put_suppliers(dev, false);
> 
>