From: Xiang Chen chenxiang66@hisilicon.com
Right now hisi_sas driver has already supported runtime PM, and it works well on based function. But for some exception situations, there are some issues related to libsas layer: - Remove a directly attached disk when sas host is suspended, a hang will occur in the resume process, patch 1~2 solve the issue; - Insert a new disk (for expander) during suspended, and the disk is not revalidated when resuming sas host, patch 4~7 solve the issue; - SMP IOs from libsas may be sending when sas host is suspended, so resume sas host when sending SMP IOs in patch 9; - New phyup may occur during the process of resuming controller, then work of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of suspended phy are blocked by each other, so defer works of new phys during suspend in patch 10~12; - Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same workqueue, but it is possible that they are blocked by each other, so keep sas host active until finished some work in patch 14.
And patch 3 which is related to scsi/block PM is from Alan Stern (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.e...)
John Garry (3): libsas: Don't always drain event workqueue for HA resume Revert "scsi: hisi_sas: Filter out new PHY up events during suspend" scsi/block PM: Always set request queue runtime active in blk_post_runtime_resume()
Xiang Chen (12): scsi: libsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list scsi: mvsas: Add spin_lock/unlock() to protect asd_sas_port->phy_list scsi: libsas: Send event PORTE_BROADCAST_RCVD for valid ports scsi: hisi_sas: Add more prink for runtime suspend/resume scsi: libsas: Resume sas host before sending SMP IOs scsi: libsas: Add a flag SAS_HA_RESUMING of sas_ha scsi: libsas: Refactor out sas_queue_deferred_work() scsi: libsas: Defer works of new phys during suspend scsi: hisi_sas: Keep controller active between ISR of phyup and the event being processed scsi: libsas: Keep sas host active until finished some work scsi: hisi_sas: Use autosuspend for SAS controller
block/blk-pm.c | 22 +++----- drivers/scsi/hisi_sas/hisi_sas.h | 1 + drivers/scsi/hisi_sas/hisi_sas_main.c | 39 +++++++++---- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 25 +++++++-- drivers/scsi/libsas/sas_event.c | 77 +++++++++++++++++++++----- drivers/scsi/libsas/sas_expander.c | 3 + drivers/scsi/libsas/sas_init.c | 50 ++++++++++++++++- drivers/scsi/libsas/sas_internal.h | 2 + drivers/scsi/mvsas/mv_sas.c | 5 ++ drivers/scsi/scsi_pm.c | 2 +- include/linux/blk-pm.h | 2 +- include/scsi/libsas.h | 2 + 12 files changed, 182 insertions(+), 48 deletions(-)
From: John Garry john.garry@huawei.com
For the hisi_sas driver, if a directly attached disk is removed during suspend, a hang will occur in the resume process:
The background is that in commit 16fd4a7c5917 ("scsi: hisi_sas: Add device link between SCSI devices and hisi_hba"), it is ensured that the HBA device cannot be runtime suspended when any SCSI device associated is active.
Other drivers which use libsas don't worry about this as none support runtime suspend.
The mentioned hang occurs when an disk is removed during suspend. In the removal process - from PHYE_RESUME_TIMEOUT event processing - we call into scsi_remove_device(), which is being processed in the HA event workqueue. Here we wait for all suppliers of the SCSI device to resume, which includes the HBA device (from the above commit). However the HBA device cannot resume, as it is waiting for the PHYE_RESUME_TIMEOUT to be processed (from calling sas_resume_ha() -> sas_drain_work()). This is the deadlock.
There does not appear to be any need for the sas_drain_work() to be called at all in sas_resume_ha() as it is not syncing against anything, so allow LLDDs to avoid this by providing a variant of sas_resume_ha() which does "sync", i.e. doesn't drain the event workqueue.
Signed-off-by: John Garry john.garry@huawei.com --- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 10 +++++++++- drivers/scsi/libsas/sas_init.c | 17 +++++++++++++++-- include/scsi/libsas.h | 1 + 3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index 9ae47d1d9897..5a83373ac0b1 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -4942,7 +4942,15 @@ static int _resume_v3_hw(struct device *device) return rc; } phys_init_v3_hw(hisi_hba); - sas_resume_ha(sha); + + /* + * If a directly-attached disk is removed during suspend, a deadlock + * may occur, as the PHYE_RESUME_TIMEOUT processing will require the + * hisi_hba->device to be active, which can only happen when resume + * completes. So don't wait for the HA event workqueue to drain upon + * resume. + */ + sas_resume_ha_no_sync(sha); clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
return 0; diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c index b640e09af6a4..43509d139241 100644 --- a/drivers/scsi/libsas/sas_init.c +++ b/drivers/scsi/libsas/sas_init.c @@ -387,7 +387,7 @@ static int phys_suspended(struct sas_ha_struct *ha) return rc; }
-void sas_resume_ha(struct sas_ha_struct *ha) +static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) { const unsigned long tmo = msecs_to_jiffies(25000); int i; @@ -417,10 +417,23 @@ void sas_resume_ha(struct sas_ha_struct *ha) * flush out disks that did not return */ scsi_unblock_requests(ha->core.shost); - sas_drain_work(ha); + if (drain) + sas_drain_work(ha); +} + +void sas_resume_ha(struct sas_ha_struct *ha) +{ + _sas_resume_ha(ha, true); } EXPORT_SYMBOL(sas_resume_ha);
+/* A no-sync variant, which does not call sas_drain_ha(). */ +void sas_resume_ha_no_sync(struct sas_ha_struct *ha) +{ + _sas_resume_ha(ha, false); +} +EXPORT_SYMBOL(sas_resume_ha_no_sync); + void sas_suspend_ha(struct sas_ha_struct *ha) { int i; diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index 79e4903bd414..a795a2d9e5b1 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -660,6 +660,7 @@ extern int sas_register_ha(struct sas_ha_struct *); extern int sas_unregister_ha(struct sas_ha_struct *); extern void sas_prep_resume_ha(struct sas_ha_struct *sas_ha); extern void sas_resume_ha(struct sas_ha_struct *sas_ha); +extern void sas_resume_ha_no_sync(struct sas_ha_struct *sas_ha); extern void sas_suspend_ha(struct sas_ha_struct *sas_ha);
int sas_set_phy_speed(struct sas_phy *phy, struct sas_phy_linkrates *rates);
From: John Garry john.garry@huawei.com
This reverts commit b14a37e011d829404c29a5ae17849d7efb034893.
In that commit, we had to filter out phy-up events during suspend, as it work cause a deadlock between processing the phyup event and the resume HA function try to drain the HA event workqueue to complete the resume process.
Now that we no longer try to drain the HA event queue during the HA resume processor, the deadlock would not occur, so remove the special handling for it.
Signed-off-by: John Garry john.garry@huawei.com --- drivers/scsi/hisi_sas/hisi_sas_main.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index f206c433de32..305d6282845a 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -619,12 +619,6 @@ static void hisi_sas_bytes_dmaed(struct hisi_hba *hisi_hba, int phy_no, if (!phy->phy_attached) return;
- if (test_bit(HISI_SAS_PM_BIT, &hisi_hba->flags) && - !sas_phy->suspended) { - dev_warn(hisi_hba->dev, "phy%d during suspend filtered out\n", phy_no); - return; - } - sas_notify_phy_event(sas_phy, PHYE_OOB_DONE, gfp_flags);
if (sas_phy->phy) {
On 16/11/2021 03:10, chenxiang wrote:
From: John Garryjohn.garry@huawei.com
This reverts commit b14a37e011d829404c29a5ae17849d7efb034893.
In that commit, we had to filter out phy-up events during suspend, as it work cause a deadlock between processing the phyup event and the resume HA function try to drain the HA event workqueue to complete the resume process.
Now that we no longer try to drain the HA event queue during the HA resume processor, the deadlock would not occur, so remove the special handling for it.
Signed-off-by: John Garryjohn.garry@huawei.com
This is missing your Signed-off-by
Thanks, John
From: John Garry john.garry@huawei.com
John Garry reported a deadlock that occurs when trying to access a runtime-suspended SATA device. For obscure reasons, the rescan procedure causes the link to be hard-reset, which disconnects the device.
The rescan tries to carry out a runtime resume when accessing the device. scsi_rescan_device() holds the SCSI device lock and won't release it until it can put commands onto the device's block queue. This can't happen until the queue is successfully runtime-resumed or the device is unregistered. But the runtime resume fails because the device is disconnected, and __scsi_remove_device() can't do the unregistration because it can't get the device lock.
The best way to resolve this deadlock appears to be to allow the block queue to start running again even after an unsuccessful runtime resume. The idea is that the driver or the SCSI error handler will need to be able to use the queue to resolve the runtime resume failure.
This patch removes the err argument to blk_post_runtime_resume() and makes the routine act as though the resume was successful always. This fixes the deadlock.
Reported-and-tested-by: John Garry john.garry@huawei.com Signed-off-by: Alan Stern stern@rowland.harvard.edu Fixes: e27829dc92e5 ("scsi: serialize ->rescan against ->remove") --- block/blk-pm.c | 22 +++++++--------------- drivers/scsi/scsi_pm.c | 2 +- include/linux/blk-pm.h | 2 +- 3 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/block/blk-pm.c b/block/blk-pm.c index 17bd020268d4..2dad62cc1572 100644 --- a/block/blk-pm.c +++ b/block/blk-pm.c @@ -163,27 +163,19 @@ EXPORT_SYMBOL(blk_pre_runtime_resume); /** * blk_post_runtime_resume - Post runtime resume processing * @q: the queue of the device - * @err: return value of the device's runtime_resume function * * Description: - * Update the queue's runtime status according to the return value of the - * device's runtime_resume function. If the resume was successful, call - * blk_set_runtime_active() to do the real work of restarting the queue. + * For historical reasons, this routine merely calls blk_set_runtime_active() + * to do the real work of restarting the queue. It does this regardless of + * whether the device's runtime-resume succeeded; even if it failed the + * driver or error handler will need to communicate with the device. * * This function should be called near the end of the device's * runtime_resume callback. */ -void blk_post_runtime_resume(struct request_queue *q, int err) +void blk_post_runtime_resume(struct request_queue *q) { - if (!q->dev) - return; - if (!err) { - blk_set_runtime_active(q); - } else { - spin_lock_irq(&q->queue_lock); - q->rpm_status = RPM_SUSPENDED; - spin_unlock_irq(&q->queue_lock); - } + blk_set_runtime_active(q); } EXPORT_SYMBOL(blk_post_runtime_resume);
@@ -201,7 +193,7 @@ EXPORT_SYMBOL(blk_post_runtime_resume); * runtime PM status and re-enable peeking requests from the queue. It * should be called before first request is added to the queue. * - * This function is also called by blk_post_runtime_resume() for successful + * This function is also called by blk_post_runtime_resume() for * runtime resumes. It does everything necessary to restart the queue. */ void blk_set_runtime_active(struct request_queue *q) diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c index b5a858c29488..f06ca9d2a597 100644 --- a/drivers/scsi/scsi_pm.c +++ b/drivers/scsi/scsi_pm.c @@ -181,7 +181,7 @@ static int sdev_runtime_resume(struct device *dev) blk_pre_runtime_resume(sdev->request_queue); if (pm && pm->runtime_resume) err = pm->runtime_resume(dev); - blk_post_runtime_resume(sdev->request_queue, err); + blk_post_runtime_resume(sdev->request_queue);
return err; } diff --git a/include/linux/blk-pm.h b/include/linux/blk-pm.h index b80c65aba249..2580e05a8ab6 100644 --- a/include/linux/blk-pm.h +++ b/include/linux/blk-pm.h @@ -14,7 +14,7 @@ extern void blk_pm_runtime_init(struct request_queue *q, struct device *dev); extern int blk_pre_runtime_suspend(struct request_queue *q); extern void blk_post_runtime_suspend(struct request_queue *q, int err); extern void blk_pre_runtime_resume(struct request_queue *q); -extern void blk_post_runtime_resume(struct request_queue *q, int err); +extern void blk_post_runtime_resume(struct request_queue *q); extern void blk_set_runtime_active(struct request_queue *q); #else static inline void blk_pm_runtime_init(struct request_queue *q,
On 16/11/2021 03:10, chenxiang wrote:
From: John Garry john.garry@huawei.com
Please fix the author here (to be Alan)
John Garry reported a deadlock that occurs when trying to access a runtime-suspended SATA device. For obscure reasons, the rescan procedure causes the link to be hard-reset, which disconnects the device.
The rescan tries to carry out a runtime resume when accessing the device. scsi_rescan_device() holds the SCSI device lock and won't release it until it can put commands onto the device's block queue. This can't happen until the queue is successfully runtime-resumed or the device is unregistered. But the runtime resume fails because the device is disconnected, and __scsi_remove_device() can't do the unregistration because it can't get the device lock.
The best way to resolve this deadlock appears to be to allow the block queue to start running again even after an unsuccessful runtime resume. The idea is that the driver or the SCSI error handler will need to be able to use the queue to resolve the runtime resume failure.
This patch removes the err argument to blk_post_runtime_resume() and makes the routine act as though the resume was successful always. This fixes the deadlock.
Reported-and-tested-by: John Garry john.garry@huawei.com Signed-off-by: Alan Stern stern@rowland.harvard.edu
You need to add your SoB when you send to the community.
Fixes: e27829dc92e5 ("scsi: serialize ->rescan against ->remove")
Did Alan add this? I didn't think that it was necessary
在 2021/11/16 19:04, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
From: John Garry john.garry@huawei.com
Please fix the author here (to be Alan)
ok
John Garry reported a deadlock that occurs when trying to access a runtime-suspended SATA device. For obscure reasons, the rescan procedure causes the link to be hard-reset, which disconnects the device.
The rescan tries to carry out a runtime resume when accessing the device. scsi_rescan_device() holds the SCSI device lock and won't release it until it can put commands onto the device's block queue. This can't happen until the queue is successfully runtime-resumed or the device is unregistered. But the runtime resume fails because the device is disconnected, and __scsi_remove_device() can't do the unregistration because it can't get the device lock.
The best way to resolve this deadlock appears to be to allow the block queue to start running again even after an unsuccessful runtime resume. The idea is that the driver or the SCSI error handler will need to be able to use the queue to resolve the runtime resume failure.
This patch removes the err argument to blk_post_runtime_resume() and makes the routine act as though the resume was successful always. This fixes the deadlock.
Reported-and-tested-by: John Garry john.garry@huawei.com Signed-off-by: Alan Stern stern@rowland.harvard.edu
You need to add your SoB when you send to the community.
ok
Fixes: e27829dc92e5 ("scsi: serialize ->rescan against ->remove")
Did Alan add this? I didn't think that it was necessary
Yes, he added it in the thread (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.e...).
.
From: Xiang Chen chenxiang66@hisilicon.com
Most places that use asd_sas_port->phy_list in libsas are protected by spinlock asd_sas_port->phy_list_lock, but there are still some places which lack of it. So add it in those places.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_event.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c index f703115e7a25..af605620ea13 100644 --- a/drivers/scsi/libsas/sas_event.c +++ b/drivers/scsi/libsas/sas_event.c @@ -104,11 +104,15 @@ void sas_enable_revalidation(struct sas_ha_struct *ha) if (!test_and_clear_bit(ev, &d->pending)) continue;
- if (list_empty(&port->phy_list)) + spin_lock(&port->phy_list_lock); + if (list_empty(&port->phy_list)) { + spin_unlock(&port->phy_list_lock); continue; + }
sas_phy = container_of(port->phy_list.next, struct asd_sas_phy, port_phy_el); + spin_unlock(&port->phy_list_lock); sas_notify_port_event(sas_phy, PORTE_BROADCAST_RCVD, GFP_KERNEL); }
From: Xiang Chen chenxiang66@hisilicon.com
Need to add spin_lock/unlock when accessing asd_sas_port->phy_list, so add them in function hisi_sas_refresh_port_id(). But it has a risk that list mutates while dropping the lock at the same time in function hisi_sas_send_ata_reset_each_phy(), so read asd_sas_port->phy_mask instead of accessing asd_sas_port->phy_list to avoid the risk.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/hisi_sas/hisi_sas_main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index 305d6282845a..44c888e0afd7 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -1427,11 +1427,13 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba) sas_port = device->port; port = to_hisi_sas_port(sas_port);
+ spin_lock(&sas_port->phy_list_lock); list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) if (state & BIT(sas_phy->id)) { phy = sas_phy->lldd_phy; break; } + spin_unlock(&sas_port->phy_list_lock);
if (phy) { port->id = phy->port_id; @@ -1508,22 +1510,25 @@ static void hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba, struct ata_link *link; u8 fis[20] = {0}; u32 state; + int i;
state = hisi_hba->hw->get_phys_state(hisi_hba); - list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) { + for (i = 0; i < hisi_hba->n_phy; i++) { if (!(state & BIT(sas_phy->id))) continue; + if (!(sas_port->phy_mask & BIT(i))) + continue;
ata_for_each_link(link, ap, EDGE) { int pmp = sata_srst_pmp(link);
- tmf_task.phy_id = sas_phy->id; + tmf_task.phy_id = i; hisi_sas_fill_ata_reset_cmd(link->device, 1, pmp, fis); rc = hisi_sas_exec_internal_tmf_task(device, fis, s, &tmf_task); if (rc != TMF_RESP_FUNC_COMPLETE) { dev_err(dev, "phy%d ata reset failed rc=%d\n", - sas_phy->id, rc); + i, rc); break; } }
On 16/11/2021 03:10, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Need to add spin_lock/unlock when accessing asd_sas_port->phy_list
Please say why
, so add them in function hisi_sas_refresh_port_id(). But it has a risk that list mutates while dropping the lock
nit: it would be nice to mention why we drop the lock
at the same time in function hisi_sas_send_ata_reset_each_phy(), so read asd_sas_port->phy_mask instead of accessing asd_sas_port->phy_list to avoid the risk.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
With issues addressed: Acked-by: John Garry john.garry@huawei.com
drivers/scsi/hisi_sas/hisi_sas_main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index 305d6282845a..44c888e0afd7 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -1427,11 +1427,13 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba) sas_port = device->port; port = to_hisi_sas_port(sas_port);
spin_lock(&sas_port->phy_list_lock);list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) if (state & BIT(sas_phy->id)) { phy = sas_phy->lldd_phy; break; }
spin_unlock(&sas_port->phy_list_lock);if (phy) { port->id = phy->port_id;
@@ -1508,22 +1510,25 @@ static void hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba, struct ata_link *link; u8 fis[20] = {0}; u32 state;
int i;
state = hisi_hba->hw->get_phys_state(hisi_hba);
- list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) {
for (i = 0; i < hisi_hba->n_phy; i++) { if (!(state & BIT(sas_phy->id))) continue;
if (!(sas_port->phy_mask & BIT(i)))continue;ata_for_each_link(link, ap, EDGE) { int pmp = sata_srst_pmp(link);
tmf_task.phy_id = sas_phy->id;
tmf_task.phy_id = i; hisi_sas_fill_ata_reset_cmd(link->device, 1, pmp, fis); rc = hisi_sas_exec_internal_tmf_task(device, fis, s, &tmf_task); if (rc != TMF_RESP_FUNC_COMPLETE) { dev_err(dev, "phy%d ata reset failed rc=%d\n",
sas_phy->id, rc);
i, rc); break; }
在 2021/11/16 19:51, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
From: Xiang Chen chenxiang66@hisilicon.com
Need to add spin_lock/unlock when accessing asd_sas_port->phy_list
Please say why
ok
, so add them in function hisi_sas_refresh_port_id(). But it has a risk that list mutates while dropping the lock
nit: it would be nice to mention why we drop the lock
ok, i will add the reason
at the same time in function hisi_sas_send_ata_reset_each_phy(), so read asd_sas_port->phy_mask instead of accessing asd_sas_port->phy_list to avoid the risk.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com
With issues addressed: Acked-by: John Garry john.garry@huawei.com
ok
drivers/scsi/hisi_sas/hisi_sas_main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index 305d6282845a..44c888e0afd7 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -1427,11 +1427,13 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba) sas_port = device->port; port = to_hisi_sas_port(sas_port);
spin_lock(&sas_port->phy_list_lock); list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) if (state & BIT(sas_phy->id)) { phy = sas_phy->lldd_phy; break; }spin_unlock(&sas_port->phy_list_lock); if (phy) { port->id = phy->port_id;@@ -1508,22 +1510,25 @@ static void hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba, struct ata_link *link; u8 fis[20] = {0}; u32 state;
- int i; state = hisi_hba->hw->get_phys_state(hisi_hba);
- list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) {
- for (i = 0; i < hisi_hba->n_phy; i++) { if (!(state & BIT(sas_phy->id))) continue;
if (!(sas_port->phy_mask & BIT(i)))continue; ata_for_each_link(link, ap, EDGE) { int pmp = sata_srst_pmp(link);
tmf_task.phy_id = sas_phy->id;tmf_task.phy_id = i; hisi_sas_fill_ata_reset_cmd(link->device, 1, pmp, fis); rc = hisi_sas_exec_internal_tmf_task(device, fis, s, &tmf_task); if (rc != TMF_RESP_FUNC_COMPLETE) { dev_err(dev, "phy%d ata reset failed rc=%d\n",
sas_phy->id, rc);
i, rc); break; } }.
From: Xiang Chen chenxiang66@hisilicon.com
It lacks of spinlock phy_list_lock when using asd_sas_port->phy_list in mvsas driver, so add spin_lock/unlock in those places.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com --- drivers/scsi/mvsas/mv_sas.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c index 31d1ea5a5dd2..1e52bc7febfa 100644 --- a/drivers/scsi/mvsas/mv_sas.c +++ b/drivers/scsi/mvsas/mv_sas.c @@ -67,8 +67,10 @@ static struct mvs_info *mvs_find_dev_mvi(struct domain_device *dev)
while (sha->sas_port[i]) { if (sha->sas_port[i] == dev->port) { + spin_lock(&sha->sas_port[i]->phy_list_lock); phy = container_of(sha->sas_port[i]->phy_list.next, struct asd_sas_phy, port_phy_el); + spin_unlock(&sha->sas_port[i]->phy_list_lock); j = 0; while (sha->sas_phy[j]) { if (sha->sas_phy[j] == phy) @@ -96,6 +98,8 @@ static int mvs_find_dev_phyno(struct domain_device *dev, int *phyno) while (sha->sas_port[i]) { if (sha->sas_port[i] == dev->port) { struct asd_sas_phy *phy; + + spin_lock(&sha->sas_port[i]->phy_list_lock); list_for_each_entry(phy, &sha->sas_port[i]->phy_list, port_phy_el) { j = 0; @@ -109,6 +113,7 @@ static int mvs_find_dev_phyno(struct domain_device *dev, int *phyno) num++; n++; } + spin_unlock(&sha->sas_port[i]->phy_list_lock); break; } i++;
From: Xiang Chen chenxiang66@hisilicon.com
If inserting a new disk for expander, the disk will not be revalidated as the topology is not re-scanned during the resume process. So send event PORTE_BROADCAST_RCVD to identify some new inserted disks for expander.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_init.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c index 43509d139241..974c4a305ece 100644 --- a/drivers/scsi/libsas/sas_init.c +++ b/drivers/scsi/libsas/sas_init.c @@ -387,6 +387,30 @@ static int phys_suspended(struct sas_ha_struct *ha) return rc; }
+static void sas_resume_insert_broadcast_ha(struct sas_ha_struct *ha) +{ + int i; + + for (i = 0; i < ha->num_phys; i++) { + struct asd_sas_port *port = ha->sas_port[i]; + struct domain_device *dev = port->port_dev; + + if (dev && dev_is_expander(dev->dev_type)) { + struct asd_sas_phy *first_phy; + + spin_lock(&port->phy_list_lock); + first_phy = list_first_entry_or_null( + &port->phy_list, struct asd_sas_phy, + port_phy_el); + spin_unlock(&port->phy_list_lock); + + if (first_phy) + sas_notify_port_event(first_phy, + PORTE_BROADCAST_RCVD, GFP_KERNEL); + } + } +} + static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) { const unsigned long tmo = msecs_to_jiffies(25000); @@ -419,6 +443,11 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) scsi_unblock_requests(ha->core.shost); if (drain) sas_drain_work(ha); + + /* send event PORTE_BROADCAST_RCVD to identify some new inserted + * disks for expander + */ + sas_resume_insert_broadcast_ha(ha); }
void sas_resume_ha(struct sas_ha_struct *ha)
From: Xiang Chen chenxiang66@hisilicon.com
Add some prink at the beginning and end of suspend/resume.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Signed-off-by: John Garry john.garry@huawei.com --- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index 5a83373ac0b1..4e9dfdb69757 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -4896,6 +4896,8 @@ static int _suspend_v3_hw(struct device *device) if (test_and_set_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags)) return -1;
+ dev_warn(dev, "entering suspend state\n"); + scsi_block_requests(shost); set_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags); flush_workqueue(hisi_hba->wq); @@ -4911,11 +4913,11 @@ static int _suspend_v3_hw(struct device *device)
hisi_sas_init_mem(hisi_hba);
- dev_warn(dev, "entering suspend state\n"); - hisi_sas_release_tasks(hisi_hba);
sas_suspend_ha(sha); + + dev_warn(dev, "end of suspending controller\n"); return 0; }
@@ -4953,6 +4955,8 @@ static int _resume_v3_hw(struct device *device) sas_resume_ha_no_sync(sha); clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
+ dev_warn(dev, "end of resuming controller\n"); + return 0; }
From: Xiang Chen chenxiang66@hisilicon.com
Need to resume sas host before sending SMP IOs to ensure that SMP IOs are sent sucessfully.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_expander.c | 3 +++ drivers/scsi/libsas/sas_internal.h | 1 + 2 files changed, 4 insertions(+)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index c2150a818423..6abce9dfc17b 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -58,7 +58,9 @@ static int smp_execute_task_sg(struct domain_device *dev, struct sas_task *task = NULL; struct sas_internal *i = to_sas_internal(dev->port->ha->core.shost->transportt); + struct sas_ha_struct *ha = dev->port->ha;
+ pm_runtime_get_sync(ha->dev); mutex_lock(&dev->ex_dev.cmd_mutex); for (retry = 0; retry < 3; retry++) { if (test_bit(SAS_DEV_GONE, &dev->state)) { @@ -131,6 +133,7 @@ static int smp_execute_task_sg(struct domain_device *dev, } } mutex_unlock(&dev->ex_dev.cmd_mutex); + pm_runtime_put_sync(ha->dev);
BUG_ON(retry == 3 && task != NULL); sas_free_task(task); diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h index d7a1fb5c10c6..ad9764a976c3 100644 --- a/drivers/scsi/libsas/sas_internal.h +++ b/drivers/scsi/libsas/sas_internal.h @@ -14,6 +14,7 @@ #include <scsi/scsi_transport_sas.h> #include <scsi/libsas.h> #include <scsi/sas_ata.h> +#include <linux/pm_runtime.h>
#ifdef pr_fmt #undef pr_fmt
From: Xiang Chen chenxiang66@hisilicon.com
Add a flag SAS_HA_RESUMING and use it to indicate the process of resuming sas_ha.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_init.c | 3 +++ include/scsi/libsas.h | 1 + 2 files changed, 4 insertions(+)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c index 974c4a305ece..f7c56677da3e 100644 --- a/drivers/scsi/libsas/sas_init.c +++ b/drivers/scsi/libsas/sas_init.c @@ -362,6 +362,7 @@ void sas_prep_resume_ha(struct sas_ha_struct *ha) int i;
set_bit(SAS_HA_REGISTERED, &ha->state); + set_bit(SAS_HA_RESUMING, &ha->state);
/* clear out any stale link events/data from the suspension path */ for (i = 0; i < ha->num_phys; i++) { @@ -443,6 +444,8 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) scsi_unblock_requests(ha->core.shost); if (drain) sas_drain_work(ha); + clear_bit(SAS_HA_RESUMING, &ha->state); +
/* send event PORTE_BROADCAST_RCVD to identify some new inserted * disks for expander diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index a795a2d9e5b1..698f2032807b 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -356,6 +356,7 @@ enum sas_ha_state { SAS_HA_DRAINING, SAS_HA_ATA_EH_ACTIVE, SAS_HA_FROZEN, + SAS_HA_RESUMING, };
struct sas_ha_struct {
On 16/11/2021 03:10, chenxiang wrote:
set_bit(SAS_HA_REGISTERED, &ha->state);
set_bit(SAS_HA_RESUMING, &ha->state);
/* clear out any stale link events/data from the suspension path */ for (i = 0; i < ha->num_phys; i++) {
@@ -443,6 +444,8 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) scsi_unblock_requests(ha->core.shost); if (drain) sas_drain_work(ha);
- clear_bit(SAS_HA_RESUMING, &ha->state);
nit: please remove extra blank line (if it exists)
/* send event PORTE_BROADCAST_RCVD to identify some new inserted * disks for expander diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index a795a2d9e5b1..698f2032807b 100644 --- a/include/scsi/libsas.h
在 2021/11/16 19:20, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
set_bit(SAS_HA_REGISTERED, &ha->state);
- set_bit(SAS_HA_RESUMING, &ha->state); /* clear out any stale link events/data from the suspension
path */ for (i = 0; i < ha->num_phys; i++) { @@ -443,6 +444,8 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) scsi_unblock_requests(ha->core.shost); if (drain) sas_drain_work(ha);
- clear_bit(SAS_HA_RESUMING, &ha->state);
nit: please remove extra blank line (if it exists)
ok
/* send event PORTE_BROADCAST_RCVD to identify some new inserted * disks for expanderdiff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index a795a2d9e5b1..698f2032807b 100644 --- a/include/scsi/libsas.h
.
From: Xiang Chen chenxiang66@hisilicon.com
In the 2rd part of function __sas_drain_work, it queues defer work. And it will be used in other places, so refactor out sas_queue_deferred_work().
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_event.c | 25 ++++++++++++++----------- drivers/scsi/libsas/sas_internal.h | 1 + 2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c index af605620ea13..01e544ca518a 100644 --- a/drivers/scsi/libsas/sas_event.c +++ b/drivers/scsi/libsas/sas_event.c @@ -41,12 +41,23 @@ static int sas_queue_event(int event, struct sas_work *work, return rc; }
- -void __sas_drain_work(struct sas_ha_struct *ha) +void sas_queue_deferred_work(struct sas_ha_struct *ha) { struct sas_work *sw, *_sw; int ret;
+ spin_lock_irq(&ha->lock); + 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) + sas_free_event(to_asd_sas_event(&sw->work)); + } + spin_unlock_irq(&ha->lock); +} + +void __sas_drain_work(struct sas_ha_struct *ha) +{ set_bit(SAS_HA_DRAINING, &ha->state); /* flush submitters */ spin_lock_irq(&ha->lock); @@ -55,16 +66,8 @@ void __sas_drain_work(struct sas_ha_struct *ha) drain_workqueue(ha->event_q); drain_workqueue(ha->disco_q);
- spin_lock_irq(&ha->lock); clear_bit(SAS_HA_DRAINING, &ha->state); - 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) - sas_free_event(to_asd_sas_event(&sw->work)); - - } - spin_unlock_irq(&ha->lock); + sas_queue_deferred_work(ha); }
int sas_drain_work(struct sas_ha_struct *ha) diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h index ad9764a976c3..acd515c01861 100644 --- a/drivers/scsi/libsas/sas_internal.h +++ b/drivers/scsi/libsas/sas_internal.h @@ -57,6 +57,7 @@ void sas_unregister_ports(struct sas_ha_struct *sas_ha);
void sas_disable_revalidation(struct sas_ha_struct *ha); void sas_enable_revalidation(struct sas_ha_struct *ha); +void sas_queue_deferred_work(struct sas_ha_struct *ha); void __sas_drain_work(struct sas_ha_struct *ha);
void sas_deform_port(struct asd_sas_phy *phy, int gone);
From: Xiang Chen chenxiang66@hisilicon.com
During the process of event PORT_BYTES_DMAED, it queues work DISCE_DISCOVER_DOMAIN and then flush workqueue ha->disco_q. If new phyup occurs during resming SAS controller, the work PORTE_BYTES_DMAED of new phys occurs before suspended phys', then work DISCE_DISCOVER_DOMAIN of new phy requires the active of SAS controller (It requires to resume SAS controller by function scsi_sysfs_add_sdev() and some other functions such as fucntion add_device_link()). But the active of SAS controller requires the complete of work PORTE_BYTES_DMAED of suspended phys while it is blocked by new phy's work on ha->event_q. So there is a deadlock and it is released only after resume timeout.
To solve the issue, defer works of new phys during suspend and queue those defer works after SAS controller becomes active.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Reviewed-by: John Garry john.garry@huawei.com --- drivers/scsi/libsas/sas_event.c | 24 ++++++++++++++++++++++++ drivers/scsi/libsas/sas_init.c | 1 + 2 files changed, 25 insertions(+)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c index 01e544ca518a..66eab7694832 100644 --- a/drivers/scsi/libsas/sas_event.c +++ b/drivers/scsi/libsas/sas_event.c @@ -139,6 +139,24 @@ static void sas_phy_event_worker(struct work_struct *work) sas_free_event(ev); }
+/* defer works of new phys during suspend */ +static bool sas_defer_event(struct asd_sas_phy *phy, struct asd_sas_event *ev) +{ + struct sas_ha_struct *ha = phy->ha; + unsigned long flags; + bool deferred = false; + + spin_lock_irqsave(&ha->lock, flags); + if (test_bit(SAS_HA_RESUMING, &ha->state) && !phy->suspended) { + struct sas_work *sw = &ev->work; + + list_add_tail(&sw->drain_node, &ha->defer_q); + deferred = true; + } + spin_unlock_irqrestore(&ha->lock, flags); + return deferred; +} + int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event, gfp_t gfp_flags) { @@ -154,6 +172,9 @@ int sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
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) sas_free_event(ev); @@ -177,6 +198,9 @@ int sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event,
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) sas_free_event(ev); diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c index f7c56677da3e..87ececc31e1b 100644 --- a/drivers/scsi/libsas/sas_init.c +++ b/drivers/scsi/libsas/sas_init.c @@ -446,6 +446,7 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain) sas_drain_work(ha); clear_bit(SAS_HA_RESUMING, &ha->state);
+ sas_queue_deferred_work(ha);
/* send event PORTE_BROADCAST_RCVD to identify some new inserted * disks for expander
On 16/11/2021 03:10, chenxiang wrote:
+/* defer works of new phys during suspend */ +static bool sas_defer_event(struct asd_sas_phy *phy, struct asd_sas_event *ev) +{
- struct sas_ha_struct *ha = phy->ha;
- unsigned long flags;
- bool deferred = false;
- spin_lock_irqsave(&ha->lock, flags);
- if (test_bit(SAS_HA_RESUMING, &ha->state) && !phy->suspended) {
struct sas_work *sw = &ev->work;
nit: double whitespace
list_add_tail(&sw->drain_node, &ha->defer_q);deferred = true;- }
在 2021/11/16 19:21, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
+/* defer works of new phys during suspend */ +static bool sas_defer_event(struct asd_sas_phy *phy, struct asd_sas_event *ev) +{
- struct sas_ha_struct *ha = phy->ha;
- unsigned long flags;
- bool deferred = false;
- spin_lock_irqsave(&ha->lock, flags);
- if (test_bit(SAS_HA_RESUMING, &ha->state) && !phy->suspended) {
struct sas_work *sw = &ev->work;nit: double whitespace
ok, i will remove it.
list_add_tail(&sw->drain_node, &ha->defer_q);deferred = true;- }
.
From: Xiang Chen chenxiang66@hisilicon.com
It is possible that controller may be suspended between ISR of phyup and the event being processed, then it can't ensure controller is active when processing the phyup event which will cause issues. To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup and pm_runtime_put_sync() in the work handler exit of a new event HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is only supported for v3 hw.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Signed-off-by: John Garry john.garry@huawei.com --- drivers/scsi/hisi_sas/hisi_sas.h | 1 + drivers/scsi/hisi_sas/hisi_sas_main.c | 22 ++++++++++++++++++++-- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 5 ++++- 3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h index 2213a91923a5..8269703b978d 100644 --- a/drivers/scsi/hisi_sas/hisi_sas.h +++ b/drivers/scsi/hisi_sas/hisi_sas.h @@ -154,6 +154,7 @@ enum hisi_sas_bit_err_type { enum hisi_sas_phy_event { HISI_PHYE_PHY_UP = 0U, HISI_PHYE_LINK_RESET, + HISI_PHYE_PHY_UP_PM, HISI_PHYES_NUM, };
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index 44c888e0afd7..9255790a8568 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -854,10 +854,11 @@ int hisi_sas_scan_finished(struct Scsi_Host *shost, unsigned long time) } EXPORT_SYMBOL_GPL(hisi_sas_scan_finished);
-static void hisi_sas_phyup_work(struct work_struct *work) +static void hisi_sas_phyup_work_common(struct work_struct *work, + enum hisi_sas_phy_event event) { struct hisi_sas_phy *phy = - container_of(work, typeof(*phy), works[HISI_PHYE_PHY_UP]); + container_of(work, typeof(*phy), works[event]); struct hisi_hba *hisi_hba = phy->hisi_hba; struct asd_sas_phy *sas_phy = &phy->sas_phy; int phy_no = sas_phy->id; @@ -868,6 +869,11 @@ static void hisi_sas_phyup_work(struct work_struct *work) hisi_sas_bytes_dmaed(hisi_hba, phy_no, GFP_KERNEL); }
+static void hisi_sas_phyup_work(struct work_struct *work) +{ + hisi_sas_phyup_work_common(work, HISI_PHYE_PHY_UP); +} + static void hisi_sas_linkreset_work(struct work_struct *work) { struct hisi_sas_phy *phy = @@ -877,9 +883,21 @@ static void hisi_sas_linkreset_work(struct work_struct *work) hisi_sas_control_phy(sas_phy, PHY_FUNC_LINK_RESET, NULL); }
+static void hisi_sas_phyup_pm_work(struct work_struct *work) +{ + struct hisi_sas_phy *phy = + container_of(work, typeof(*phy), works[HISI_PHYE_PHY_UP_PM]); + struct hisi_hba *hisi_hba = phy->hisi_hba; + struct device *dev = hisi_hba->dev; + + hisi_sas_phyup_work_common(work, HISI_PHYE_PHY_UP_PM); + pm_runtime_put_sync(dev); +} + static const work_func_t hisi_sas_phye_fns[HISI_PHYES_NUM] = { [HISI_PHYE_PHY_UP] = hisi_sas_phyup_work, [HISI_PHYE_LINK_RESET] = hisi_sas_linkreset_work, + [HISI_PHYE_PHY_UP_PM] = hisi_sas_phyup_pm_work, };
bool hisi_sas_notify_phy_event(struct hisi_sas_phy *phy, diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index 4e9dfdb69757..a5695ae8b73b 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -1561,7 +1561,10 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
phy->port_id = port_id; phy->phy_attached = 1; - hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP); + + /* Call pm_runtime_put_sync() with pairs in hisi_sas_phyup_pm_work() */ + pm_runtime_get_noresume(dev); + hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP_PM); res = IRQ_HANDLED; end: if (phy->reset_completion)
On 16/11/2021 03:10, chenxiang wrote:
en ISR of phyup and the event being processed, then it can't ensure controller is active when processing the phyup event which will cause issues. To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup and pm_runtime_put_sync() in the work handler exit of a new event HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is only supported for v3 hw.
Signed-off-by: Xiang Chenchenxiang66@hisilicon.com Signed-off-by: John Garryjohn.garry@huawei.com
When you send this, it should be Acked-by for me
在 2021/11/16 19:21, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
en ISR of phyup and the event being processed, then it can't ensure controller is active when processing the phyup event which will cause issues. To avoid the issue, add pm_runtime_get_noresume() in ISR of phyup and pm_runtime_put_sync() in the work handler exit of a new event HISI_PHYE_PHY_UP_PM which is called in v3 hw as runtime PM is only supported for v3 hw.
Signed-off-by: Xiang Chenchenxiang66@hisilicon.com Signed-off-by: John Garryjohn.garry@huawei.com
When you send this, it should be Acked-by for me
OK, i will change it. For patch 8 and 15, do i need to change SoB to Acked-by for you?
.
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(-)
diff --git a/drivers/scsi/libsas/sas_event.c b/drivers/scsi/libsas/sas_event.c index 66eab7694832..a5b4f9435ba6 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; }
From: Xiang Chen chenxiang66@hisilicon.com
For some scenarios, it may send many IOs in a short time, then SAS controller will enter suspend and resume frequently which is invalid. To avoid it, use autosuspend mode for SAS controller and set default autosuspend delay time to 5s.
Signed-off-by: Xiang Chen chenxiang66@hisilicon.com Signed-off-by: John Garry john.garry@huawei.com --- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index a5695ae8b73b..d96fe636e984 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -4777,6 +4777,8 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
scsi_scan_host(shost);
+ pm_runtime_set_autosuspend_delay(dev, 5000); + pm_runtime_use_autosuspend(dev); /* * For the situation that there are ATA disks connected with SAS * controller, it additionally creates ata_port which will affect the
On 16/11/2021 03:10, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
Right now hisi_sas driver has already supported runtime PM, and it works well on based function. But for some exception situations, there are some issues related to libsas layer:
- Remove a directly attached disk when sas host is suspended, a hang will
occur in the resume process, patch 1~2 solve the issue;
- Insert a new disk (for expander) during suspended, and the disk is not
revalidated when resuming sas host, patch 4~7 solve the issue;
- SMP IOs from libsas may be sending when sas host is suspended, so resume
sas host when sending SMP IOs in patch 9;
- New phyup may occur during the process of resuming controller, then work
of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of suspended phy are blocked by each other, so defer works of new phys during suspend in patch 10~12;
- Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same
workqueue, but it is possible that they are blocked by each other, so keep sas host active until finished some work in patch 14.
And patch 3 which is related to scsi/block PM is from Alan Stern (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.e...)
Generally it looks ok.
It would be nice to test on something which is not hisi_sas.
I have a pm8001 card, but it is not setup in a machine. I can try that as I think we may need for some other work, but feel free to send to community in the meantime (with any issues I mentioned addressed, please).
Thanks, john
在 2021/11/16 22:30, John Garry 写道:
On 16/11/2021 03:10, chenxiang wrote:
From: Xiang Chenchenxiang66@hisilicon.com
Right now hisi_sas driver has already supported runtime PM, and it works well on based function. But for some exception situations, there are some issues related to libsas layer:
- Remove a directly attached disk when sas host is suspended, a hang
will occur in the resume process, patch 1~2 solve the issue;
- Insert a new disk (for expander) during suspended, and the disk is not
revalidated when resuming sas host, patch 4~7 solve the issue;
- SMP IOs from libsas may be sending when sas host is suspended, so
resume sas host when sending SMP IOs in patch 9;
- New phyup may occur during the process of resuming controller, then
work of DISCE_DISCOVER_DOMAIN of a new phy and work PORTE_BYTES_DMAED of suspended phy are blocked by each other, so defer works of new phys during suspend in patch 10~12;
- Work PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED are in the same
workqueue, but it is possible that they are blocked by each other, so keep sas host active until finished some work in patch 14.
And patch 3 which is related to scsi/block PM is from Alan Stern (https://lore.kernel.org/linux-scsi/20210714161027.GC380727@rowland.harvard.e...)
Generally it looks ok.
It would be nice to test on something which is not hisi_sas.
I have a pm8001 card, but it is not setup in a machine. I can try that as I think we may need for some other work, but feel free to send to community in the meantime (with any issues I mentioned addressed, please).
Ok, thanks.
Thanks, john
.
-
chenxiang -
chenxiang (M)
-
John Garry