Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 13 Jan 2020 10:45:28 -0800
From:   asutoshd@codeaurora.org
To:     Bean Huo <huobean@gmail.com>
Cc:     alim.akhtar@samsung.com, avri.altman@wdc.com,
        pedrom.sousa@synopsys.com, jejb@linux.ibm.com,
        martin.petersen@oracle.com, stanley.chu@mediatek.com,
        beanhuo@micron.com, bvanassche@acm.org, tomas.winkler@intel.com,
        cang@codeaurora.org, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-scsi-owner@vger.kernel.org
Subject: Re: [PATCH 3/3] scsi: ufs: use UFS device indicated maximum LU number
In-Reply-To: <20200110183606.10102-4-huobean@gmail.com>
References: <20200110183606.10102-1-huobean@gmail.com>
 <20200110183606.10102-4-huobean@gmail.com>
Message-ID: <3c6080a44d2943f86d6991d48cd2dd28@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-01-10 10:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> According to Jedec standard UFS 3.0 and UFS 2.1 Spec, Maximum number of 
> logical
> units supported by the UFS device is indicated by parameter 
> bMaxNumberLU in
> Geometry Descriptor. This patch is to delete current hard code macro 
> definition
> of UFS_UPIU_MAX_GENERAL_LUN, and switch to use device indicated number 
> instead.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c |  2 +-
>  drivers/scsi/ufs/ufs.h       | 12 +++++++++---
>  drivers/scsi/ufs/ufshcd.c    |  4 ++--
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
> b/drivers/scsi/ufs/ufs-sysfs.c
> index 720be3f64be7..dbdf8b01abed 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -713,7 +713,7 @@ static ssize_t _pname##_show(struct device 
> *dev,			\
>  	struct scsi_device *sdev = to_scsi_device(dev);			\
>  	struct ufs_hba *hba = shost_priv(sdev->host);			\
>  	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\
> -	if (!ufs_is_valid_unit_desc_lun(lun))				\
> +	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))		\
>  		return -EINVAL;						\
>  	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
>  		lun, _duname##_DESC_PARAM##_puname, buf, _size);	\
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 5ca7ea4f223e..810eeca0de63 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -63,7 +63,6 @@
>  #define UFS_UPIU_MAX_UNIT_NUM_ID	0x7F
>  #define UFS_MAX_LUNS		(SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID)
>  #define UFS_UPIU_WLUN_ID	(1 << 7)
> -#define UFS_UPIU_MAX_GENERAL_LUN	8
> 
>  /* Well known logical unit id in LUN field of UPIU */
>  enum {
> @@ -548,12 +547,19 @@ struct ufs_dev_desc {
> 
>  /**
>   * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit 
> descriptor
> + * @dev_info: pointer of instance of struct ufs_dev_info
>   * @lun: LU number to check
>   * @return: true if the lun has a matching unit descriptor, false 
> otherwise
>   */
> -static inline bool ufs_is_valid_unit_desc_lun(u8 lun)
> +static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info 
> *dev_info,
> +		u8 lun)
>  {
> -	return lun == UFS_UPIU_RPMB_WLUN || (lun < UFS_UPIU_MAX_GENERAL_LUN);
> +	if (!dev_info || !dev_info->max_lu_supported) {
> +		pr_err("Max General LU supported by UFS isn't initilized\n");
> +		return false;
> +	}
> +
> +	return lun == UFS_UPIU_RPMB_WLUN || (lun < 
> dev_info->max_lu_supported);
>  }
> 
>  #endif /* End of Header */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a297fe55e36a..c6ea5d88222d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3286,7 +3286,7 @@ static inline int
> ufshcd_read_unit_desc_param(struct ufs_hba *hba,
>  	 * Unit descriptors are only available for general purpose LUs (LUN 
> id
>  	 * from 0 to 7) and RPMB Well known LU.
>  	 */
> -	if (!ufs_is_valid_unit_desc_lun(lun))
> +	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun))
>  		return -EOPNOTSUPP;
> 
>  	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
> @@ -4540,7 +4540,7 @@ static int ufshcd_get_lu_wp(struct ufs_hba *hba,
>  	 * protected so skip reading bLUWriteProtect parameter for
>  	 * it. For other W-LUs, UNIT DESCRIPTOR is not available.
>  	 */
> -	else if (lun >= UFS_UPIU_MAX_GENERAL_LUN)
> +	else if (lun >= hba->dev_info.max_lu_supported)
>  		ret = -ENOTSUPP;
>  	else
>  		ret = ufshcd_read_unit_desc_param(hba,

Looks good to me.
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>