Hi,
> From: lijiaming3 <lijiaming3@xxxxxxxxxx>
>
> Implement File Based Optimization initialization and add sysfs interface.
>
> Stoage devices have a long lifespan. Device performance over its lifespan is not
> constant and may deteriorate over time
>
> This feature describes a method to improve the performance regression.
> The host needs to provide File System information to storage device first. Based
> on that information device analyzes the file system data and provides the host
> the level of performance regression. The host then may instruct the device to
> execute optimization procedure to improve the regression level.
You might want to break this one long patch into a series of smaller patches.
One option is to do it per-functional flows: Init flow, analysis phase, and defrag phase.
In your cover letter you might want to elaborate what this feature is all about,
And that your design expect a user-space companion that analyze the files.
Which is a good place to share a reference to an open source code that does that.
> +void ufsfbo_init(struct ufs_hba *hba)
Maybe ufshbo_probe to note that you are probing the device for this feature?
> +{
> + struct ufsfbo_dev *fbo;
> + int ret = 0;
> +
> + fbo = &hba->fbo;
> + fbo->hba = hba;
> +
> + ret = ufsfbo_get_fbo_support_state(fbo);
You can save reading the device descriptor, if you would call ufsfbo_init from ufs_get_device_desc,
Like ufshcd_wb_probe() does.
> + if (ret) {
> + FBO_ERR_MSG("NOT Support FBO. ret(%d)", ret);
> + return;
> + }
> + ret = ufsfbo_get_fbo_desc_info(fbo);
> + if (ret) {
> + FBO_ERR_MSG("Failed getting fbo info. ret(%d)", ret);
> + return;
> + }
> + fbo->fbo_debug = false;
> + fbo->block_suspend = false;
> +
> + if (ufshcd_is_auto_hibern8_supported(fbo->hba))
> + fbo->is_auto_enabled = true;
All this power management control code is not needed IMO.
> +
> + ret = ufsfbo_create_sysfs(fbo);
You might want to consider splitting your sysfs according to:
- FBOWritebuffer & FBOReadbuffer - part of sdev_group
it will allow defragmenting files across all luns.
- the rest per hba
> +static ssize_t ufsfbo_sysfs_show_debug(struct ufsfbo_dev *fbo, char
> +*buf) {
> + FBO_INFO_MSG(fbo, "Debug:%d", fbo->fbo_debug);
> + return sysfs_emit(buf, "%d\n", fbo->fbo_debug); }
I don't think the debug info should be part of your series.
Maybe add it as the last patch if you must.
> +/**
> + * struct ufsfbo_dev - FBO related structure
Maybe ufsfbo_ctrl - as it's your main controlling object.
And split it further to ufsfbo_dev_info - ver, rec_lrs, max_lrs, etc.
And then all other members
> +int ufsfbo_get_fbo_prog_state(struct ufsfbo_dev *fbo, int *prog_state);
> +int ufsfbo_operation_control(struct ufsfbo_dev *fbo, int *val); int
> +ufsfbo_get_exe_level(struct ufsfbo_dev *fbo, int *frag_exe_level); int
> +ufsfbo_set_exe_level(struct ufsfbo_dev *fbo, int *frag_exe_level); int
> +ufsfbo_lba_list_write(struct ufsfbo_dev *fbo, const char *buf); int
> +ufsfbo_read_frag_level(struct ufsfbo_dev *fbo, char *buf); int
> +ufsfbo_get_fbo_desc_info(struct ufsfbo_dev *fbo); int
> +ufsfbo_get_fbo_support_state(struct ufsfbo_dev *fbo); int
> +ufsfbo_get_state(struct ufs_hba *hba); void ufsfbo_set_state(struct
> +ufs_hba *hba, int state); void ufsfbo_init(struct ufs_hba *hba); void
> +ufsfbo_reset(struct ufs_hba *hba); void ufsfbo_reset_host(struct
> +ufs_hba *hba); void ufsfbo_remove(struct ufs_hba *hba); int
> +ufsfbo_is_not_present(struct ufsfbo_dev *fbo); void
> +ufsfbo_block_enter_suspend(struct ufsfbo_dev *fbo); void
> +ufsfbo_allow_enter_suspend(struct ufsfbo_dev *fbo); void
> +ufsfbo_auto_hibern8_enable(struct ufsfbo_dev *fbo, unsigned int val);
> +#endif /* _UFSFBO_H_ */
You should expose functionality outside the module only if you must.
Otherwise, make it privet to the module.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 2f6468f22b48..9c377c514e17 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4956,6 +4956,14 @@ static void ufshcd_hpb_configure(struct ufs_hba
> *hba, struct scsi_device *sdev)
> ufshpb_init_hpb_lu(hba, sdev);
> }
>
> +static void ufshcd_fbo_configure(struct ufs_hba *hba, struct
> +scsi_device *sdev) { #ifdef CONFIG_SCSI_UFS_FBO
> + if (sdev->lun == 0)
> + hba->fbo.sdev_ufs_lu = sdev; #endif }
You won't be needing this if you follow my suggestion above.
> @@ -8005,6 +8014,9 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>
> /* Probe and add UFS logical units */
> ret = ufshcd_add_lus(hba);
> +#ifdef CONFIG_SCSI_UFS_FBO
> + ufsfbo_init(hba);
> +#endif
Instead of wrapping it withing other modules,
Use it in ufsfbo.h
> +#ifdef CONFIG_SCSI_UFS_FBO
> +#include "ufsfbo.h"
> +#endif
Include it in ufshcd.c without the ifdef
> #define UFSHCD "ufshcd"
> #define UFSHCD_DRIVER_VERSION "0.2"
>
> @@ -916,6 +918,9 @@ struct ufs_hba {
> struct dentry *debugfs_root;
> struct delayed_work debugfs_ee_work;
> u32 debugfs_ee_rate_limit_ms;
> +#endif
> +#ifdef CONFIG_SCSI_UFS_FBO
> + struct ufsfbo_dev fbo;
> #endif
If you would use a struct pointer here, you won't need the ifdef.
Thanks,
Avri
