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: <3112c599b68342ced021dab4521c96aca95e5bff.camel@gmail.com>
Subject: Re: [PATCH v3 2/3] scsi: ufs: core: Cleanup ufshcd_slave_alloc()
From:   Bean Huo <huobean@gmail.com>
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:   Sat, 22 Oct 2022 23:30:27 +0200
In-Reply-To: <9868763a-d360-db53-02b9-2d7ab9628d79@acm.org>
References: <20221018181627.326657-1-beanhuo@iokpp.de>
         <20221018181627.326657-3-beanhuo@iokpp.de>
         <9868763a-d360-db53-02b9-2d7ab9628d79@acm.org>
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 Tue, 2022-10-18 at 11:27 -0700, Bart Van Assche wrote:
> Should a test be added that verifies that UNIT_DESC_PARAM_LU_Q_DEPTH
> < len?
>=20
> Additionally, please use braces ({}) around multi-line if-statement
> bodies.
>=20
> > +=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 * According to UFS device s=
pecification, the write
> > protection mode is only supported by
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * normal LU, not supported =
by WLUN.
> > +=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=A0if (hba->dev_info.f_power_on=
_wp_en && lun < hba-
> > >dev_info.max_lu_supported &&
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !hba->dev=
_info.is_lu_power_on_wp &&
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 desc_buf[=
UNIT_DESC_PARAM_LU_WR_PROTECT] =3D=3D
> > UFS_LU_POWER_ON_WP)
> > +=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=A0hba->dev_info.is_lu_power_on_wp =3D true;
>=20
> Also here, should the following test be added:=20
> UNIT_DESC_PARAM_LU_WR_PROTECT < len?
>=20
> Otherwise this patch looks good to me.

Bart,=20

len is already the LU descriptor size. UNIT_DESC_PARAM_LU_Q_DEPTH (0x6)
and UNIT_DESC_PARAM_LU_WR_PROTECT(0x5) are very basic parameters in the
LU descriptor, no matter what version of UFS is, there are these two
parameters in the descriptor. So the return value of
ufshcd_read_unit_desc_param() will be -EOPNOTSUPP, -EINVAL, -ENOMEM or
0 (successful read of LU descriptor). the checkup of "< len" is not
neccessary.

Kind regards,
Bean