[14/15] scsi: libsas: Keep sas host active until finished some work

From: Xiang Chen <chenxiang66@hisilicon.com>

For those works from event queue, if executing them such as
PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
firstly as SMP IOs are sent in the process. So phyup will occur and it
will call work PORTE_BYTES_DMAED. But the original work (such as
PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
singlethread workqueue, so the complete of original work waits for
the complete of work PORTE_BYTES_DMAED while the complete of work
PORTE_BYTES_DMAED wait for the complete of original and it is blocked.

So call pm_runtime_get_noresume() to keep sas host active until
finished those works.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Please consider som rewrite:

On 17/11/2021 02:45, chenxiang wrote:

"scsi: libsas: Keep sas host active until finished some work" -> "scsi: 
libsas: Keep host active while processing events"

> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> For those works from event queue, if executing them such as
> PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
> firstly as SMP IOs are sent in the process. So phyup will occur and it
> will call work PORTE_BYTES_DMAED. But the original work (such as
> PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
> singlethread workqueue, so the complete of original work waits for
> the complete of work PORTE_BYTES_DMAED while the complete of work
> PORTE_BYTES_DMAED wait for the complete of original and it is blocked.
> 
> So call pm_runtime_get_noresume() to keep sas host active until
> finished those works.
Processing events such as PORTE_BROADCAST_RCVD may cause dependency 
issues for runtime power management support.

Such a problem would be that handling a PORTE_BROADCAST_RCVD event 
requires that the host is resumed to send SMP commands. However, in 
resuming the host, the phyup events generated from re-enabling the phys 
are processed in the same workqueue as the original PORTE_BROADCAST_RCVD 
event. As such, the host will never finish resuming (as it waits for the 
phyup event processing), and then the PORTE_BROADCAST_RCVD event cannot 
be processed as the SMP commands are blocked, and so we have a deadlock.

Solve this problem by ensuring that libsas keeps the host active until 
completely finished processing phy or port events, such as 
PORTE_BYTES_DMAED. As such, we don't have to worry about resuming the 
host for processing individual SMP commands in this example.

> 
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c
> index 626ef96b9348..3613b9b315bc 100644
> --- a/drivers/scsi/libsas/sas_event.c
> +++ b/drivers/scsi/libsas/sas_event.c
> @@ -50,8 +50,10 @@ void sas_queue_deferred_work(struct sas_ha_struct *ha)
>   	list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>   		list_del_init(&sw->drain_node);
>   		ret = sas_queue_work(ha, sw);
> -		if (ret != 1)
> +		if (ret != 1) {
> +			pm_runtime_put(ha->dev);
>   			sas_free_event(to_asd_sas_event(&sw->work));
> +		}
>   	}
>   	spin_unlock_irq(&ha->lock);
>   }
> @@ -126,16 +128,22 @@ void sas_enable_revalidation(struct sas_ha_struct *ha)
>   static void sas_port_event_worker(struct work_struct *work)
>   {
>   	struct asd_sas_event *ev = to_asd_sas_event(work);
> +	struct asd_sas_phy *phy = ev->phy;
> +	struct sas_ha_struct *ha = phy->ha;
>   
>   	sas_port_event_fns[ev->event](work);
> +	pm_runtime_put(ha->dev);
>   	sas_free_event(ev);
>   }
>   
>   static void sas_phy_event_worker(struct work_struct *work)
>   {
>   	struct asd_sas_event *ev = to_asd_sas_event(work);
> +	struct asd_sas_phy *phy = ev->phy;
> +	struct sas_ha_struct *ha = phy->ha;
>   
>   	sas_phy_event_fns[ev->event](work);
> +	pm_runtime_put(ha->dev);
>   	sas_free_event(ev);
>   }
>   
> @@ -170,14 +178,19 @@ int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
>   	if (!ev)
>   		return -ENOMEM;
>   
> +	/* Call pm_runtime_put() with pairs in sas_port_event_worker() */
> +	pm_runtime_get_noresume(ha->dev);
> +
>   	INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
>   
>   	if (sas_defer_event(phy, ev))
>   		return 0;
>   
>   	ret = sas_queue_event(event, &ev->work, ha);
> -	if (ret != 1)
> +	if (ret != 1) {
> +		pm_runtime_put(ha->dev);
>   		sas_free_event(ev);
> +	}
>   
>   	return ret;
>   }
> @@ -196,14 +209,19 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
>   	if (!ev)
>   		return -ENOMEM;
>   
> +	/* Call pm_runtime_put() with pairs in sas_phy_event_worker() */
> +	pm_runtime_get_noresume(ha->dev);
> +
>   	INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
>   
>   	if (sas_defer_event(phy, ev))
>   		return 0;
>   
>   	ret = sas_queue_event(event, &ev->work, ha);
> -	if (ret != 1)
> +	if (ret != 1) {
> +		pm_runtime_put(ha->dev);
>   		sas_free_event(ev);
> +	}
>   
>   	return ret;
>   }
>

在 2021/12/14 20:34, John Garry 写道:
> Please consider som rewrite:
>
> On 17/11/2021 02:45, chenxiang wrote:
>
> "scsi: libsas: Keep sas host active until finished some work" -> 
> "scsi: libsas: Keep host active while processing events"
>
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> For those works from event queue, if executing them such as
>> PORTE_BROADCAST_RCVD when sas host is suspended, it will resume sas host
>> firstly as SMP IOs are sent in the process. So phyup will occur and it
>> will call work PORTE_BYTES_DMAED. But the original work (such as
>> PORTE_BROADCAST_RCVD) and the work PORTE_BYTES_DMAED are in the same
>> singlethread workqueue, so the complete of original work waits for
>> the complete of work PORTE_BYTES_DMAED while the complete of work
>> PORTE_BYTES_DMAED wait for the complete of original and it is blocked.
>>
>> So call pm_runtime_get_noresume() to keep sas host active until
>> finished those works.
> Processing events such as PORTE_BROADCAST_RCVD may cause dependency 
> issues for runtime power management support.
>
> Such a problem would be that handling a PORTE_BROADCAST_RCVD event 
> requires that the host is resumed to send SMP commands. However, in 
> resuming the host, the phyup events generated from re-enabling the 
> phys are processed in the same workqueue as the original 
> PORTE_BROADCAST_RCVD event. As such, the host will never finish 
> resuming (as it waits for the phyup event processing), and then the 
> PORTE_BROADCAST_RCVD event cannot be processed as the SMP commands are 
> blocked, and so we have a deadlock.
>
> Solve this problem by ensuring that libsas keeps the host active until 
> completely finished processing phy or port events, such as 
> PORTE_BYTES_DMAED. As such, we don't have to worry about resuming the 
> host for processing individual SMP commands in this example.
>

ok, i will consider to rewrite them.

>>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> Reviewed-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_event.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_event.c 
>> b/drivers/scsi/libsas/sas_event.c
>> index 626ef96b9348..3613b9b315bc 100644
>> --- a/drivers/scsi/libsas/sas_event.c
>> +++ b/drivers/scsi/libsas/sas_event.c
>> @@ -50,8 +50,10 @@ void sas_queue_deferred_work(struct sas_ha_struct 
>> *ha)
>>       list_for_each_entry_safe(sw, _sw, &ha->defer_q, drain_node) {
>>           list_del_init(&sw->drain_node);
>>           ret = sas_queue_work(ha, sw);
>> -        if (ret != 1)
>> +        if (ret != 1) {
>> +            pm_runtime_put(ha->dev);
>> sas_free_event(to_asd_sas_event(&sw->work));
>> +        }
>>       }
>>       spin_unlock_irq(&ha->lock);
>>   }
>> @@ -126,16 +128,22 @@ void sas_enable_revalidation(struct 
>> sas_ha_struct *ha)
>>   static void sas_port_event_worker(struct work_struct *work)
>>   {
>>       struct asd_sas_event *ev = to_asd_sas_event(work);
>> +    struct asd_sas_phy *phy = ev->phy;
>> +    struct sas_ha_struct *ha = phy->ha;
>>         sas_port_event_fns[ev->event](work);
>> +    pm_runtime_put(ha->dev);
>>       sas_free_event(ev);
>>   }
>>     static void sas_phy_event_worker(struct work_struct *work)
>>   {
>>       struct asd_sas_event *ev = to_asd_sas_event(work);
>> +    struct asd_sas_phy *phy = ev->phy;
>> +    struct sas_ha_struct *ha = phy->ha;
>>         sas_phy_event_fns[ev->event](work);
>> +    pm_runtime_put(ha->dev);
>>       sas_free_event(ev);
>>   }
>>   @@ -170,14 +178,19 @@ int sas_notify_port_event(struct asd_sas_phy 
>> *phy, enum port_event event,
>>       if (!ev)
>>           return -ENOMEM;
>>   +    /* Call pm_runtime_put() with pairs in sas_port_event_worker() */
>> +    pm_runtime_get_noresume(ha->dev);
>> +
>>       INIT_SAS_EVENT(ev, sas_port_event_worker, phy, event);
>>         if (sas_defer_event(phy, ev))
>>           return 0;
>>         ret = sas_queue_event(event, &ev->work, ha);
>> -    if (ret != 1)
>> +    if (ret != 1) {
>> +        pm_runtime_put(ha->dev);
>>           sas_free_event(ev);
>> +    }
>>         return ret;
>>   }
>> @@ -196,14 +209,19 @@ int sas_notify_phy_event(struct asd_sas_phy 
>> *phy, enum phy_event event,
>>       if (!ev)
>>           return -ENOMEM;
>>   +    /* Call pm_runtime_put() with pairs in sas_phy_event_worker() */
>> +    pm_runtime_get_noresume(ha->dev);
>> +
>>       INIT_SAS_EVENT(ev, sas_phy_event_worker, phy, event);
>>         if (sas_defer_event(phy, ev))
>>           return 0;
>>         ret = sas_queue_event(event, &ev->work, ha);
>> -    if (ret != 1)
>> +    if (ret != 1) {
>> +        pm_runtime_put(ha->dev);
>>           sas_free_event(ev);
>> +    }
>>         return ret;
>>   }
>>
>
>
> .
>