Re: [PATCH 08/12] pm80xx : Cleanup command when a reset times out.

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Deepak Ukey <deepak.ukey@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>,
	Vasanthalakshmi.Tharmarajan@microchip.com,
	Viswas.G@microchip.com, Jack Wang <jinpu.wang@profitbricks.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	dpf@google.com, jsperbeck@google.com,
	Vikram Auradkar <auradkar@google.com>,
	ianyar@google.com
Subject: Re: [PATCH 08/12] pm80xx : Cleanup command when a reset times out.
Date: Wed, 6 Nov 2019 11:39:26 +0100	[thread overview]
Message-ID: <CAMGffEmMBTWHVJfdcvZNegDS+yPQFsGhx1L9FbBuN=-QdCRc7A@mail.gmail.com> (raw)
In-Reply-To: <20191031051241.6762-9-deepak.ukey@microchip.com>

On Thu, Oct 31, 2019 at 6:12 AM Deepak Ukey <deepak.ukey@microchip.com> wrote:
>
> From: peter chang <dpf@google.com>
>
> Added the fix so the if driver properly sent the abort it try to remove
> it from the firmware's list of outstanding commands regardless of the
> abort status. This means that the task gets free-ed 'now' rather than
> possibly getting free-ed later when the scsi layer thinks it's leaked
> but still valid.
>
> Signed-off-by: peter chang <dpf@google.com>
> Signed-off-by: Deepak Ukey <deepak.ukey@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 50 +++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 4491de8d40fc..b562916a3d3d 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -1204,8 +1204,8 @@ int pm8001_abort_task(struct sas_task *task)
>         pm8001_dev = dev->lldd_dev;
>         pm8001_ha = pm8001_find_ha_by_dev(dev);
>         phy_id = pm8001_dev->attached_phy;
> -       rc = pm8001_find_tag(task, &tag);
> -       if (rc == 0) {
> +       ret = pm8001_find_tag(task, &tag);
> +       if (ret == 0) {
>                 pm8001_printk("no tag for task:%p\n", task);
>                 return TMF_RESP_FUNC_FAILED;
>         }
> @@ -1243,26 +1243,50 @@ int pm8001_abort_task(struct sas_task *task)
>
>                         /* 2. Send Phy Control Hard Reset */
>                         reinit_completion(&completion);
> +                       phy->port_reset_status = PORT_RESET_TMO;
>                         phy->reset_success = false;
>                         phy->enable_completion = &completion;
>                         phy->reset_completion = &completion_reset;
>                         ret = PM8001_CHIP_DISP->phy_ctl_req(pm8001_ha, phy_id,
>                                 PHY_HARD_RESET);
> -                       if (ret)
> -                               goto out;
> -                       PM8001_MSG_DBG(pm8001_ha,
> -                               pm8001_printk("Waiting for local phy ctl\n"));
> -                       wait_for_completion(&completion);
> -                       if (!phy->reset_success)
> +                       if (ret) {
> +                               phy->enable_completion = NULL;
> +                               phy->reset_completion = NULL;
>                                 goto out;
> +                       }
>
> -                       /* 3. Wait for Port Reset complete / Port reset TMO */
> +                       /* In the case of the reset timeout/fail we still
> +                        * abort the command at the firmware. Yhe assumption
s/Yhe/The
> +                        * here is that the drive is off doing something so
> +                        * that it's not processing requests, and we want to
> +                        * avoid getting a completion for this and either
> +                        * leaking the task in libsas or losing the race and
> +                        * getting a double free.
> +                        */
>                         PM8001_MSG_DBG(pm8001_ha,
> +                               pm8001_printk("Waiting for local phy ctl\n"));
> +                       ret = wait_for_completion_timeout(&completion,
> +                                       PM8001_TASK_TIMEOUT * HZ);
> +                       if (!ret || !phy->reset_success) {
> +                               phy->enable_completion = NULL;
> +                               phy->reset_completion = NULL;
> +                       } else {
> +                               /* 3. Wait for Port Reset complete or
> +                                * Port reset TMO
> +                                */
> +                               PM8001_MSG_DBG(pm8001_ha,
>                                 pm8001_printk("Waiting for Port reset\n"));
> -                       wait_for_completion(&completion_reset);
> -                       if (phy->port_reset_status) {
> -                               pm8001_dev_gone_notify(dev);
> -                               goto out;
> +                               ret = wait_for_completion_timeout(
> +                                       &completion_reset,
> +                                       PM8001_TASK_TIMEOUT * HZ);
> +                               if (!ret)
> +                                       phy->reset_completion = NULL;
> +                               WARN_ON(phy->port_reset_status ==
> +                                               PORT_RESET_TMO);
> +                               if (phy->port_reset_status == PORT_RESET_TMO) {
> +                                       pm8001_dev_gone_notify(dev);
> +                                       goto out;
> +                               }
>                         }
>
>                         /*
> --
> 2.16.3
>
Patch looks fine, please fix the typo above.
Acked-by: Jack Wang <jinpu.wang@cloud.ionos.com>