Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d06a264c4544a23ccccd016c6797d889db526b64.camel@iokpp.de>
Subject: Re: [PATCH v2 1/2] scsi: ufs: core: Remove unnecessary if statement
From:   Bean Huo <beanhuo@iokpp.de>
To:     Bart Van Assche <bvanassche@acm.org>, alim.akhtar@samsung.com,
        avri.altman@wdc.com, asutoshd@codeaurora.org, jejb@linux.ibm.com,
        martin.petersen@oracle.com, stanley.chu@mediatek.com,
        beanhuo@micron.com, tomas.winkler@intel.com, cang@codeaurora.org,
        daejun7.park@samsung.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Fri, 14 Oct 2022 21:20:44 +0200
In-Reply-To: <a1cd6719-a743-fc96-e0e7-364a52b62952@acm.org>
References: <20221010092937.520013-1-beanhuo@iokpp.de>
         <20221010092937.520013-2-beanhuo@iokpp.de>
         <a1cd6719-a743-fc96-e0e7-364a52b62952@acm.org>
Organization: IOKPP
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-10-14 at 11:37 -0700, Bart Van Assche wrote:
> > @@ -300,9 +300,6 @@ static inline bool
> > ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0pr_err("Max General LU supported by UFS isn't
> > initialized\n");
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return false;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* WB is available only for =
the logical unit from 0 to 7 */
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (param_offset =3D=3D UNIT=
_DESC_PARAM_WB_BUF_ALLOC_UNITS)
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return lun < UFS_UPIU_MAX_WB_LUN_ID;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return lun =3D=3D UFS_U=
PIU_RPMB_WLUN || (lun < dev_info-
> > >max_lu_supported);
> > =C2=A0=C2=A0 }
>=20
> Hi Bean,
>=20
> I think the above patch reintroduces the stack overflow issue fixed
> by
> commit a2fca52ee640 ("scsi: ufs: WB is only available on LUN #0 to
> #7").
>=20
> How about reverting commit a2fca52ee640 and fixing the stack overflow
> issue in another way than by modifying ufs_is_valid_unit_desc_lun()?
>=20
> Thanks,
>=20
> Bart

Hi Bart,=20

I knew that fix, it was because the user tried to poll
dLUNumWriteBoosterBufferAllocUnits from RPMB LU, as you know, RPMB
doesn't support WB, but the root cause is that we don't separate normal
logical unit descriptors and RPMB unit descriptor when we create sysfs
group,


in ufshcd_driver_template {

...
.sdev_groups =3D ufshcd_driver_groups,

}


ufshcd_driver_groups {
...
&ufs_sysfs_unit_descriptor_group,
...
}


so all the logical units will have the unified unit descriptor sysfs
node. This is wrong. =20

Another problem is that Boot and device LU don't have unit descriptors,
but we still create a unit descriptor sysfs node group for boot and
device LU.

I am working on the Advanced RPMB, and trying to seperate normal unit
descriptor and RPMB unit descriptor, will let you know if it is
possible. or if you know the solution, please let me know, thanks.


Kind regards,
Bean