Received-SPF: pass (google.com: domain of linux-kernel+bounces-214370-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240607175743.3986625-1-tadamsjr@google.com> <20240607175743.3986625-2-tadamsjr@google.com>
In-Reply-To: <20240607175743.3986625-2-tadamsjr@google.com>
From: Jinpu Wang <jinpu.wang@ionos.com>
Date: Fri, 14 Jun 2024 07:32:51 +0200
Message-ID: <CAMGffEmgcKh1xGOrRNsBq_GrEbL0a1UdG3dgg6Mg0vrsKzW_Vg@mail.gmail.com>
Subject: Re: [PATCH 1/3] scsi: pm80xx: Set phy->enable_completion only when we
 wait for it
To: TJ Adams <tadamsjr@google.com>
Cc: Jack Wang <jinpu.wang@cloud.ionos.com>, 
	"James E . J . Bottomley" <James.Bottomley@hansenpartnership.com>, 
	"Martin K . Petersen" <martin.petersen@oracle.com>, linux-scsi@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Igor Pylypiv <ipylypiv@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 7:57=E2=80=AFPM TJ Adams <tadamsjr@google.com> wrote=
:
>
> From: Igor Pylypiv <ipylypiv@google.com>
>
> pm8001_phy_control() populates the enable_completion pointer with a
> stack address, sends a PHY_LINK_RESET / PHY_HARD_RESET, waits 300 ms,
> and returns. The problem arises when a phy control response comes late.
> After 300 ms the pm8001_phy_control() function returns and the passed
> enable_completion stack address is no longer valid. Late phy control
> response invokes complete() on a dangling enable_completion pointer
> which leads to a kernel crash.
>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: Terrence Adams <tadamsjr@google.com>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm800=
1_sas.c
> index a5a31dfa4512..ee2da8e49d4c 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -166,7 +166,6 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, e=
num phy_func func,
>         unsigned long flags;
>         pm8001_ha =3D sas_phy->ha->lldd_ha;
>         phy =3D &pm8001_ha->phy[phy_id];
> -       pm8001_ha->phy[phy_id].enable_completion =3D &completion;
>
>         if (PM8001_CHIP_DISP->fatal_errors(pm8001_ha)) {
>                 /*
> @@ -190,6 +189,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, e=
num phy_func func,
>                                 rates->maximum_linkrate;
>                 }
>                 if (pm8001_ha->phy[phy_id].phy_state =3D=3D  PHY_LINK_DIS=
ABLE) {
> +                       pm8001_ha->phy[phy_id].enable_completion =3D &com=
pletion;
>                         PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id=
);
>                         wait_for_completion(&completion);
>                 }
> @@ -198,6 +198,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, e=
num phy_func func,
>                 break;
>         case PHY_FUNC_HARD_RESET:
>                 if (pm8001_ha->phy[phy_id].phy_state =3D=3D PHY_LINK_DISA=
BLE) {
> +                       pm8001_ha->phy[phy_id].enable_completion =3D &com=
pletion;
>                         PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id=
);
>                         wait_for_completion(&completion);
>                 }
> @@ -206,6 +207,7 @@ int pm8001_phy_control(struct asd_sas_phy *sas_phy, e=
num phy_func func,
>                 break;
>         case PHY_FUNC_LINK_RESET:
>                 if (pm8001_ha->phy[phy_id].phy_state =3D=3D PHY_LINK_DISA=
BLE) {
> +                       pm8001_ha->phy[phy_id].enable_completion =3D &com=
pletion;
>                         PM8001_CHIP_DISP->phy_start_req(pm8001_ha, phy_id=
);
>                         wait_for_completion(&completion);
>                 }
> --
> 2.45.2.505.gda0bf45e8d-goog
>