On 10/10/22 02:29, Bean Huo wrote:
From: Bean Huo <beanhuo@xxxxxxxxxx>
Combine ufshcd_get_lu_power_on_wp_status() and ufshcd_set_queue_depth()
into one single ufshcd_lu_init(), so that we only need to read the LUN
descriptor once to replace the original twice.
The following part can probably be left out from the patch description
without reducing clarity: " to replace the original twice".
+/**
+ * ufshcd_lu_power_on_wp_init - Initialize LU's power on write protect state
+ * @hba: per-adapter instance
+ * @sdev: pointer to SCSI device
+ * @b_lu_write_protect: bLUWriteProtect value read from LU descriptor
+ */
+static inline void ufshcd_lu_power_on_wp_init(struct ufs_hba *hba, const struct scsi_device *sdev,
+ u8 b_lu_write_protect)
+{
+ if (hba->dev_info.f_power_on_wp_en && !hba->dev_info.is_lu_power_on_wp &&
+ b_lu_write_protect == UFS_LU_POWER_ON_WP)
+ hba->dev_info.is_lu_power_on_wp = true;
+}
The body of this function is only three lines long and this function is
only called once. Are you sure that you want a separate function instead
of inlining this function in its only caller?
+static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+ int ret;
+ int len;
+ u8 lun;
+ u8 lun_qdepth;
+ u8 *desc_buf;
Most kernel developers these days order local variable declarations from
longest to shortest line ("reverse Christmas tree").
+ lun_qdepth = hba->nutrs;
+ lun = ufshcd_scsi_to_upiu_lun(sdev->lun);
+ len = hba->desc_size[QUERY_DESC_IDN_UNIT];
+
+ desc_buf = kmalloc(len, GFP_KERNEL);
+ if (!desc_buf)
+ goto set_qdepth;
+
+ ret = ufshcd_read_unit_desc_param(hba, lun, 0, desc_buf, len);
+ if (ret == -EOPNOTSUPP)
+ /* If LU doesn't support unit descriptor, its queue depth is set to 1 */
+ lun_qdepth = 1;
+ else if (desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH])
+ lun_qdepth = min_t(int, desc_buf[UNIT_DESC_PARAM_LU_Q_DEPTH], hba->nutrs);
ufshcd_read_unit_desc_param() can return fewer bytes than requested. How
about modifying ufshcd_read_unit_desc_param() such that it returns the
number of bytes that has been copied and using that return value above
to check whether at least UNIT_DESC_PARAM_LU_Q_DEPTH bytes have been
initialized in desc_buf?
+ /*
+ * According to UFS device spec, The write protection mode is only supported by normal LU,
+ * not supported by WLUN.
+ */
+ if (!ret && lun < hba->dev_info.max_lu_supported)
+ ufshcd_lu_power_on_wp_init(hba, sdev, desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT]);
Please insert an if (ret < 0) check after the
ufshcd_read_unit_desc_param() call and jump to the kfree() statement if
ret < 0 instead of checking several times whether or not ret < 0.
Thanks,
Bart.
