Re: [PATCH v9 2/4] scsi: ufs: Introduce HPB feature
Hi, Bart
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 618b253e5ec8..df30622a2b67 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -588,6 +588,24 @@ struct ufs_hba_variant_params {
> > u16 hba_enable_delay_us;
> > u32 wb_flush_threshold;
> > };
> > +#ifdef CONFIG_SCSI_UFS_HPB
> > +/**
> > + * struct ufshpb_dev_info - UFSHPB device related info
> > + * @num_lu: the number of user logical unit to check whether all lu
> > finished
> > + * initialization
> > + * @rgn_size: device reported HPB region size
> > + * @srgn_size: device reported HPB sub-region size
> > + * @slave_conf_cnt: counter to check all lu finished initialization
> > + * @hpb_disabled: flag to check if HPB is disabled
> > + */
> > +struct ufshpb_dev_info {
> > + int num_lu;
> > + int rgn_size;
> > + int srgn_size;
> > + atomic_t slave_conf_cnt;
> > + bool hpb_disabled;
> > +};
> > +#endif
>
> Please insert a blank line above "#ifdef CONFIG_SCSI_UFS_HPB"
OK, I will insert a blank line.
> > +/* HPB enabled lu list */
> > +static LIST_HEAD(lh_hpb_lu);
>
> Is it necessary to maintain this list? Or in other words, is it possible to
> change all list_for_each_entry(hpb, _hpb_lu, list_hpb_lu) calls into
> shost_for_each_device() calls?
OK, I will remove lh_hpb_lu.
> > +/* SYSFS functions */
> > +#define ufshpb_sysfs_attr_show_func(__name)
> > \
> > +static ssize_t __name##_show(struct device *dev, \
> > + struct device_attribute *attr, char *buf) \
> > +{ \
> > + struct scsi_device *sdev = to_scsi_device(dev); \
> > + struct ufshpb_lu *hpb = sdev->hostdata; \
> > + if (!hpb) \
> > + return -ENOENT; \
> > + return snprintf(buf, PAGE_SIZE, "%d\n", \
> > + atomic_read(>stats.__name)); \
> > +} \
> > +static DEVICE_ATTR_RO(__name)
>
> Please leave a blank line after declarations (between the "hpb" declaration
> and "if (!hpb)").
OK, I will add a blank line.
> > +#ifndef CONFIG_SCSI_UFS_HPB
> > +[...]
> > +static struct attribute *hpb_dev_attrs[] = { NULL,};
> > +static struct attribute_group ufs_sysfs_hpb_stat_group = {.attrs =
> > hpb_dev_attrs,};
> > +#else
> > +[...]
> > +extern struct attribute_group ufs_sysfs_hpb_stat_group;
> > +#endif
>
> Defining static variables or arrays in header files is questionable because
> the definition of these variables will be duplicated in each source file that
> header file is included in. Although the general rule for kernel code is to
> confine #ifdefs to header files, for ufs_sysfs_hpb_stat_group I think that
> it is better to surround its use with #ifndef CONFIG_SCSI_UFS_HPB / #endif
> instead of defining a dummy structure as static variable in a header file if
> HPB support is disabled.
OK, I added #ifdef in ufshcd.c.
static const struct attribute_group *ufshcd_driver_groups[] = {
_sysfs_unit_descriptor_group,
_sysfs_lun_attributes_group,
#ifdef CONFIG_SCSI_UFS_HPB
_sysfs_hpb_stat_group,
#endif
NULL,
};
Thanks,
Daejun
Re: [PATCH v9 2/4] scsi: ufs: Introduce HPB feature
On 2020-08-28 00:18, Daejun Park wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 618b253e5ec8..df30622a2b67 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -588,6 +588,24 @@ struct ufs_hba_variant_params {
> u16 hba_enable_delay_us;
> u32 wb_flush_threshold;
> };
> +#ifdef CONFIG_SCSI_UFS_HPB
> +/**
> + * struct ufshpb_dev_info - UFSHPB device related info
> + * @num_lu: the number of user logical unit to check whether all lu finished
> + * initialization
> + * @rgn_size: device reported HPB region size
> + * @srgn_size: device reported HPB sub-region size
> + * @slave_conf_cnt: counter to check all lu finished initialization
> + * @hpb_disabled: flag to check if HPB is disabled
> + */
> +struct ufshpb_dev_info {
> + int num_lu;
> + int rgn_size;
> + int srgn_size;
> + atomic_t slave_conf_cnt;
> + bool hpb_disabled;
> +};
> +#endif
Please insert a blank line above "#ifdef CONFIG_SCSI_UFS_HPB"
> +/* HPB enabled lu list */
> +static LIST_HEAD(lh_hpb_lu);
Is it necessary to maintain this list? Or in other words, is it possible to
change all list_for_each_entry(hpb, _hpb_lu, list_hpb_lu) calls into
shost_for_each_device() calls?
> +/* SYSFS functions */
> +#define ufshpb_sysfs_attr_show_func(__name) \
> +static ssize_t __name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{\
> + struct scsi_device *sdev = to_scsi_device(dev); \
> + struct ufshpb_lu *hpb = sdev->hostdata; \
> + if (!hpb) \
> + return -ENOENT; \
> + return snprintf(buf, PAGE_SIZE, "%d\n", \
> + atomic_read(>stats.__name)); \
> +}\
> +static DEVICE_ATTR_RO(__name)
Please leave a blank line after declarations (between the "hpb" declaration
and "if (!hpb)").
> +#ifndef CONFIG_SCSI_UFS_HPB
> +[...]
> +static struct attribute *hpb_dev_attrs[] = { NULL,};
> +static struct attribute_group ufs_sysfs_hpb_stat_group = {.attrs =
> hpb_dev_attrs,};
> +#else
> +[...]
> +extern struct attribute_group ufs_sysfs_hpb_stat_group;
> +#endif
Defining static variables or arrays in header files is questionable because
the definition of these variables will be duplicated in each source file that
header file is included in. Although the general rule for kernel code is to
confine #ifdefs to header files, for ufs_sysfs_hpb_stat_group I think that
it is better to surround its use with #ifndef CONFIG_SCSI_UFS_HPB / #endif
instead of defining a dummy structure as static variable in a header file if
HPB support is disabled.
Otherwise this patch looks good to me.
Thanks,
Bart.
[PATCH v9 2/4] scsi: ufs: Introduce HPB feature
This is a patch for the HPB feature.
This patch adds HPB function calls to UFS core driver.
The mininum size of the memory pool used in the HPB is implemented
as a module parameter, so that it can be configurable by the user.
To gurantee a minimum memory pool size of 4MB: ufshpb_host_map_kbytes=4096
Acked-by: Avri Altman
Tested-by: Bean Huo
Signed-off-by: Daejun Park
---
drivers/scsi/ufs/Kconfig | 9 +
drivers/scsi/ufs/Makefile | 1 +
drivers/scsi/ufs/ufshcd.c | 58
drivers/scsi/ufs/ufshcd.h | 22 +-
drivers/scsi/ufs/ufshpb.c | 596 ++
drivers/scsi/ufs/ufshpb.h | 176 +++
6 files changed, 861 insertions(+), 1 deletion(-)
create mode 100644 drivers/scsi/ufs/ufshpb.c
create mode 100644 drivers/scsi/ufs/ufshpb.h
diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index f6394999b98c..773fd241ac84 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -182,3 +182,12 @@ config SCSI_UFS_CRYPTO
Enabling this makes it possible for the kernel to use the crypto
capabilities of the UFS device (if present) to perform crypto
operations on data being transferred to/from the device.
+
+config SCSI_UFS_HPB
+ bool "Support UFS Host Performance Booster"
+ depends on SCSI_UFSHCD
+ help
+ The UFS HPB feature improves random read performance. It caches
+ L2P (logical to physical) map of UFS to host DRAM. The driver uses HPB
+ read command by piggybacking physical page number for bypassing FTL
(flash
+ translation layer)'s L2P address translation.
\ No newline at end of file
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 4679af1b564e..663e17cee359 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
ufshcd-core-y += ufshcd.o ufs-sysfs.o
ufshcd-core-$(CONFIG_SCSI_UFS_BSG) += ufs_bsg.o
ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
+ufshcd-core-$(CONFIG_SCSI_UFS_HPB) += ufshpb.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 06e2439d523c..0d7af96588e2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -22,6 +22,7 @@
#include "ufs-sysfs.h"
#include "ufs_bsg.h"
#include "ufshcd-crypto.h"
+#include "ufshpb.h"
#include
#include
@@ -2546,6 +2547,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
struct scsi_cmnd *cmd)
ufshcd_comp_scsi_upiu(hba, lrbp);
+ ufshpb_prep(hba, lrbp);
+
err = ufshcd_map_sg(hba, lrbp);
if (err) {
lrbp->cmd = NULL;
@@ -4691,6 +4694,33 @@ static int ufshcd_change_queue_depth(struct scsi_device
*sdev, int depth)
return scsi_change_queue_depth(sdev, depth);
}
+static void ufshcd_hpb_destroy(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+ /* skip well-known LU */
+ if (sdev->lun >= UFS_UPIU_MAX_UNIT_NUM_ID)
+ return;
+
+ if (!ufshpb_is_allowed(hba))
+ return;
+
+ ufshpb_destroy_lu(hba, sdev);
+}
+
+static void ufshcd_hpb_configure(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+ /* skip well-known LU */
+ if (sdev->lun >= UFS_UPIU_MAX_UNIT_NUM_ID)
+ return;
+
+ if (!(hba->dev_info.b_ufs_feature_sup & UFS_DEV_HPB_SUPPORT))
+ return;
+
+ if (!ufshpb_is_allowed(hba))
+ return;
+
+ ufshpb_init_hpb_lu(hba, sdev);
+}
+
/**
* ufshcd_slave_configure - adjust SCSI device configurations
* @sdev: pointer to SCSI device
@@ -4700,6 +4730,8 @@ static int ufshcd_slave_configure(struct scsi_device
*sdev)
struct ufs_hba *hba = shost_priv(sdev->host);
struct request_queue *q = sdev->request_queue;
+ ufshcd_hpb_configure(hba, sdev);
+
blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
if (ufshcd_is_rpm_autosuspend_allowed(hba))
@@ -4719,6 +4751,9 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
struct ufs_hba *hba;
hba = shost_priv(sdev->host);
+
+ ufshcd_hpb_destroy(hba, sdev);
+
/* Drop the reference as it won't be needed anymore */
if (ufshcd_scsi_to_upiu_lun(sdev->lun) == UFS_UPIU_UFS_DEVICE_WLUN) {
unsigned long flags;
@@ -4828,6 +4863,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct
ufshcd_lrb *lrbp)
*/
pm_runtime_get_noresume(hba->dev);
}
+
+ if (scsi_status == SAM_STAT_GOOD)
+ ufshpb_rsp_upiu(hba, lrbp);
break;
case UPIU_TRANSACTION_REJECT_UPIU:
/* TODO: handle Reject UPIU
