Re: [PATCH v2] scsi: libsas: fix HA resume deadlock and hisi_sas disk-wake race

[yangxingui]

On 2026/7/15 16:23, John Garry wrote:
> On 02/07/2026 04:32, Xingui Yang wrote:
> > Commit fbefe22811c3140 ("scsi: libsas: Don't always drain event workqueue
> > for HA resume") introduced sas_resume_ha_no_sync() to avoid a 
> > deadlock: the
> > PHYE_RESUME_TIMEOUT handler, running on the HA event workqueue, calls
> > sas_deform_port() -> sas_destruct_devices(), which removes SCSI 
> > devices and
> > waits for the host to become runtime-active. But the host cannot resume
> > until sas_resume_ha() -> sas_drain_work() returns, and the drain is 
> > blocked
> > on that very handler.
> >
> > However skipping the drain reintroduces a race: hisi_sas returns from
> > resume before all PHY UP work and libsas discovery work finish. The
> > controller may then autosuspend while disks are still waking up. The 
> > disks
> > issue IO to a suspended controller, the IO fails, and the disks get
> > disabled.
> >
> > Fix the deadlock at its source by moving the PHYE_RESUME_TIMEOUT
> > notification to after sas_drain_work(). By then the host resume is 
> > about to
> > complete, so device removal through device_link no longer blocks on the
> > resume and the cycle is broken.
> >
> > With the deadlock gone, restore sas_resume_ha() (the draining variant) in
> > hisi_sas and remove sas_resume_ha_no_sync().
> >
> > Fixes: fbefe22811c3140 ("scsi: libsas: Don't always drain event 
> > workqueue for HA resume")
> > Signed-off-by: Xingui Yang <yangxingui@xxxxxxxxxx>
> > ---
> >   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  9 +-------
> >   drivers/scsi/libsas/sas_init.c         | 32 ++++++++++++++------------
> >   include/scsi/libsas.h                  |  1 -
> >   3 files changed, 18 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c 
> > b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > index 0687bdefcd63..c8673ae4e472 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > @@ -5263,14 +5263,7 @@ static int _resume_v3_hw(struct device *device)
> >       }
> >       phys_init_v3_hw(hisi_hba);
> > -    /*
> > -     * 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);
> > +    sas_resume_ha(sha);
> >       clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
> >       dev_warn(dev, "end of resuming controller\n");
> > diff --git a/drivers/scsi/libsas/sas_init.c 
> > b/drivers/scsi/libsas/sas_init.c
> > index 0bec236f0fb5..624850f1483d 100644
> > --- a/drivers/scsi/libsas/sas_init.c
> > +++ b/drivers/scsi/libsas/sas_init.c
> > @@ -410,7 +410,7 @@ static void sas_resume_insert_broadcast_ha(struct 
> > sas_ha_struct *ha)
> >       }
> >   }
> > -static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
> > +static void _sas_resume_ha(struct sas_ha_struct *ha)
> >   {
> >       const unsigned long tmo = msecs_to_jiffies(25000);
> >       int i;
> > @@ -426,6 +426,21 @@ static void _sas_resume_ha(struct sas_ha_struct 
> > *ha, bool drain)
> >           dev_info(ha->dev, "waiting up to 25 seconds for %d phy%s to 
> > resume\n",
> >                i, i > 1 ? "s" : "");
> >       wait_event_timeout(ha->eh_wait_q, phys_suspended(ha) == 0, tmo);
> > +
> > +    /* all phys are back up or timed out, turn on i/o so we can
>
> That is obviously not the standard formatting for comments. And 
> sentences begin with an upper case letter.
>
> > +     * flush out disks that did not return
>
> . to end a sentence.
>
> Check punctuation in future.

The comment was pre-existing (from 303694eeee5e) and carried over
without correction — apologies for that. I'll fix it in v3, along
with the new comment added by this patch.

> > +     */
> > +    scsi_unblock_requests(ha->shost);
> > +    sas_drain_work(ha);
> > +
> > +    /* Send PHYE_RESUME_TIMEOUT after sas_drain_work(). The handler
> > +     * calls sas_deform_port() -> sas_destruct_devices(), which removes
> > +     * SCSI devices and, for LLDDs using device_link() PM sync, waits
> > +     * for the host to be runtime-active. Sending it before the drain
> > +     * would deadlock: the drain waits for the handler, the handler
> > +     * waits for host resume, and host resume waits for the drain to
> > +     * finish.
> > +     */
> >       for (i = 0; i < ha->num_phys; i++) {
> >           struct asd_sas_phy *phy = ha->sas_phy[i];
> > @@ -436,12 +451,6 @@ static void _sas_resume_ha(struct sas_ha_struct 
> > *ha, bool drain)
> >           }
> >       }
> > -    /* all phys are back up or timed out, turn on i/o so we can
> > -     * flush out disks that did not return
> > -     */
> > -    scsi_unblock_requests(ha->shost);
> > -    if (drain)
> > -        sas_drain_work(ha);
> >       clear_bit(SAS_HA_RESUMING, &ha->state);
> >       sas_queue_deferred_work(ha);
> > @@ -453,17 +462,10 @@ static void _sas_resume_ha(struct sas_ha_struct 
> > *ha, bool drain)
> >   void sas_resume_ha(struct sas_ha_struct *ha)
>
> what is the purpose of this wrapper now?

The wrapper is indeed redundant now. I'll fold _sas_resume_ha()
directly into sas_resume_ha() and remove the static helper in v3.
Will send v3 shortly.

Thanks,
Xingui