Re: [PATCH v9 2/4] scsi: ufs: Introduce HPB feature

[Bart Van Assche <bvanassche@acm.org>]

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, &lh_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(&hpb->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.