| Message ID | 1637117108-230103-1-git-send-email-chenxiang66@hisilicon.com |
|---|---|
| Headers | show |
| Series | Add runtime PM support for libsas | expand |
On 17/11/2021 02:44, chenxiang wrote: > 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> This is missing your Signed-off-by Thanks, John
On 17/11/2021 02:45, chenxiang wrote: > From: Xiang Chen<chenxiang66@hisilicon.com> > > Add some prink at the beginning and end of suspend/resume. > Same as subject, it should be /s/prink/logs or prints/. I assume. > Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> > Signed-off-by: John Garry<john.garry@huawei.com>
On 17/11/2021 02:45, chenxiang wrote: > Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> > Signed-off-by: John Garry<john.garry@huawei.com> And please change this to Acked-by for my tag Thanks, John
On 17/11/2021 02:45, chenxiang wrote: > 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. Please consider this rewrite: It is possible that the controller may become suspended between processing a phyup interrupt and the event being processed by libsas. As such, we can't ensure that the controller is active when processing the phyup event - this may cause the phyup event to be lost or other issues. To avoid any possible issues, add pm_runtime_get_noresume() in the phyup interrupt handler and pm_runtime_put_sync() in the work handler exit to ensure that we stay always active. Since we only want pm_runtime_get_noresume() called for v3 hw, signal this will a new event, HISI_PHYE_PHY_UP_PM. Thanks, John
Please consider this rewrite: On 17/11/2021 02:45, chenxiang wrote: scsi: hisi_sas: Use autosuspend for the host controller > 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. The controller may frequently enter and exit suspend for each IO which we need to deal with. This is inefficient and may cause too much suspend+resume activity for the controller. To avoid this, use a default 5s autosuspend for the controller to stop frequently suspending and resuming. This value may still be still modified via sysfs. > > Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> > Signed-off-by: John Garry<john.garry@huawei.com> Acked-by please Thanks, John
Please consider these points: About "scsi: libsas: Resume sas host before sending SMP IOs", just have "Resume host while sending SMP IOs" On 17/11/2021 02:45, chenxiang wrote: > From: Xiang Chen<chenxiang66@hisilicon.com> > > Need to resume sas host before sending SMP IOs to ensure that > SMP IOs are sent sucessfully. successfully > When sending SMP IOs to the host we need to ensure that that host is not suspended and may handle the commands. This is a better approach than relying on the host to resume itself to handle such commands. So use pm_runtime_get_sync() and pm_runtime_get_sync() calls for the host when executing SMP tasks. > > Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> > Reviewed-by: John Garry<john.garry@huawei.com>
Please consider these: On 17/11/2021 02:45, chenxiang wrote: I'd have "scsi: libsas: Refactor sas_queue_deferred_work()" > 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(). The second part of the function __sas_drain_work() queues the deferred work. This functionality would be duplicated in other places, so factor out into sas_queue_deferred_work(). Thanks, John > > 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); >
On 17/11/2021 02:44, chenxiang wrote: > From: Xiang Chen<chenxiang66@hisilicon.com> > Please consider this rewrite: Currently the HiSilicon SAS controller v3 hw driver supports runtime PM. However a number of corner-case bugs have been reported for this feature. These include: a. If a directly-attached disk is removed when the host is suspended a system hang may occur during resume. libsas drains events after resuming the host. Draining the events causes a deadlock as we need to ensure that the host is resumed for some libsas events processing, however the resume process will not complete until all events are processed. b. If a disk is attached to an expander when the host is suspended then this new disk will not be detected when active again. c. The host controller may not resume from suspension when sending SMP IOs. d. If a phy comes up when resuming the host controller then we may get a deadlock from processing of events DISCE_DISCOVER_DOMAIN and PORTE_BYTES_DMAED. e. Similar to d., the work of PORTE_BROADCAST_RCVD and PORTE_BYTES_DMAED may deadlock. This series addresses those issues, briefly described as follows: a. As far as we can see, this drain is unneeded, so conditionally remove. b. Just insert broadcast events to revalidate the topology. c. and e. When processing any events from the LLD, make libsas keep the host active until finished processing all work related to that original event. d. Defer phyup event processing in case described. > Right now hisi_sas driver has already supported runtime PM, and it works > well on base functions. 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.edu/)
在 2021/12/13 18:31, John Garry 写道: > On 17/11/2021 02:44, chenxiang wrote: >> 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> > > This is missing your Signed-off-by Ok, i will add it in next version. > > Thanks, > John > > . >
在 2021/12/13 18:34, John Garry 写道: > On 17/11/2021 02:45, chenxiang wrote: >> From: Xiang Chen<chenxiang66@hisilicon.com> >> >> Add some prink at the beginning and end of suspend/resume. >> > > Same as subject, it should be /s/prink/logs or prints/. I assume. ok, i will use logs instead > > >> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> >> Signed-off-by: John Garry<john.garry@huawei.com> > > > . >
在 2021/12/13 18:35, John Garry 写道: > On 17/11/2021 02:45, chenxiang wrote: >> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> >> Signed-off-by: John Garry<john.garry@huawei.com> > > And please change this to Acked-by for my tag > Ok, thanks, i will change it in next version. > Thanks, > John > > . >
在 2021/12/13 18:50, John Garry 写道: > Please consider this rewrite: ok, i will consider to rewrite it in next version > > On 17/11/2021 02:45, chenxiang wrote: > > scsi: hisi_sas: Use autosuspend for the host controller > >> 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. > > The controller may frequently enter and exit suspend for each IO which > we need to deal with. This is inefficient and may cause too much > suspend+resume activity for the controller. > > To avoid this, use a default 5s autosuspend for the controller to stop > frequently suspending and resuming. This value may still be still > modified via sysfs. > >> >> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> >> Signed-off-by: John Garry<john.garry@huawei.com> > > Acked-by please ok > > Thanks, > John > > . >
Hi John, 在 2021/12/14 19:52, John Garry 写道: > On 17/11/2021 02:44, chenxiang wrote: >> From: Xiang Chen<chenxiang66@hisilicon.com> >> > > Please consider this rewrite: > > Currently the HiSilicon SAS controller v3 hw driver supports runtime > PM. However a number of corner-case bugs have been reported for this > feature. These include: > a. If a directly-attached disk is removed when the host is suspended a > system hang may occur during resume. libsas drains events after > resuming the host. Draining the events causes a deadlock as we need to > ensure that the host is resumed for some libsas events processing, > however the resume process will not complete until all events are > processed. > b. If a disk is attached to an expander when the host is suspended > then this new disk will not be detected when active again. > c. The host controller may not resume from suspension when sending SMP > IOs. > d. If a phy comes up when resuming the host controller then we may get > a deadlock from processing of events DISCE_DISCOVER_DOMAIN and > PORTE_BYTES_DMAED. > e. Similar to d., the work of PORTE_BROADCAST_RCVD and > PORTE_BYTES_DMAED may deadlock. > > This series addresses those issues, briefly described as follows: > a. As far as we can see, this drain is unneeded, so conditionally remove. > b. Just insert broadcast events to revalidate the topology. > c. and e. When processing any events from the LLD, make libsas keep > the host active until finished processing all work related to that > original event. > d. Defer phyup event processing in case described. Thanks for your detailed comments. I will improve those commit message according to your suggestions. > > >> Right now hisi_sas driver has already supported runtime PM, and it works >> well on base functions. 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.edu/) >> > > > . >
在 2021/12/13 18:44, John Garry 写道: > On 17/11/2021 02:45, chenxiang wrote: >> 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. > > Please consider this rewrite: > > It is possible that the controller may become suspended between > processing a phyup interrupt and the event being processed by libsas. > As such, we can't ensure that the controller is active when processing > the phyup event - this may cause the phyup event to be lost or other > issues. > To avoid any possible issues, add pm_runtime_get_noresume() in the > phyup interrupt handler and pm_runtime_put_sync() in the work handler > exit to ensure that we stay always active. Since we only want > pm_runtime_get_noresume() called for v3 hw, signal this will a new > event, HISI_PHYE_PHY_UP_PM. Ok, i will rewrite it in next version. > > Thanks, > John > > . >
在 2021/12/13 19:20, John Garry 写道: > Please consider these: > > On 17/11/2021 02:45, chenxiang wrote: > > I'd have "scsi: libsas: Refactor sas_queue_deferred_work()" Ok, i will change it. > >> 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(). > > The second part of the function __sas_drain_work() queues the deferred > work. This functionality would be duplicated in other places, so > factor out into sas_queue_deferred_work(). I will rewrite them in next verison. > > Thanks, > John > >> >> 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); >> > > > . >
在 2021/12/13 19:12, John Garry 写道: > Please consider these points: > > About "scsi: libsas: Resume sas host before sending SMP IOs", just > have "Resume host while sending SMP IOs" > > On 17/11/2021 02:45, chenxiang wrote: >> From: Xiang Chen<chenxiang66@hisilicon.com> >> >> Need to resume sas host before sending SMP IOs to ensure that >> SMP IOs are sent sucessfully. > > successfully ok > > > > > When sending SMP IOs to the host we need to ensure that that host is > not suspended and may handle the commands. This is a better approach > than relying on the host to resume itself to handle such commands. So > use pm_runtime_get_sync() and pm_runtime_get_sync() calls for the host > when executing SMP tasks. Ok, i will rewrite it. > >> >> Signed-off-by: Xiang Chen<chenxiang66@hisilicon.com> >> Reviewed-by: John Garry<john.garry@huawei.com> > > > . >
From: Xiang Chen <chenxiang66@hisilicon.com> Right now hisi_sas driver has already supported runtime PM, and it works well on base functions. 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.edu/) Alan Stern (1): scsi/block PM: Always set request queue runtime active in blk_post_runtime_resume() John Garry (2): libsas: Don't always drain event workqueue for HA resume Revert "scsi: hisi_sas: Filter out new PHY up events during suspend" 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 | 49 +++++++++++++++- 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, 181 insertions(+), 48 deletions(-)