LinuxLists.cc - [PATCH v3 0/1] scsi: ufs: Add hibernation callbacks

2023-01-20 11:45:58

Subject: [PATCH v3 0/1] scsi: ufs: Add hibernation callbacks

This patch adds hibernation callbacks in UFS driver.
Please take a look and let us know your thoughts.

v3:
-Address compilation issues

v2:
- Addressed Bart's comments
- Moved core and host related changes to single patch
- Note to Bart: Regrading the comment to pass "restore" as an
argument instead of adding a new member to ufs_hba structure, adding
new function argument in core file (ufshcd.c) is forcing us to make
changes to other vendor files to fix the compilation errors. Hence
we have retained our original change. Please let us know your inputs
on this.

Initial version:
- Adds hibernation callbacks - freeze, restore and thaw,
required for suspend to disk feature.

Anjana Hari (1):
scsi: ufs: Add hibernation callbacks

drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++++++++++++++++++
drivers/ufs/host/ufs-qcom.c | 6 +++-
include/ufs/ufshcd.h | 8 +++++
3 files changed, 75 insertions(+), 1 deletion(-)

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

2023-01-20 11:48:38

by Anjana Hari

[permalink] [raw]

Subject: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

Adds freeze, thaw and restore callbacks for hibernate and restore
functionality.

Signed-off-by: Anjana Hari <[email protected]>
---
drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++++++++++++++++++
drivers/ufs/host/ufs-qcom.c | 6 +++-
include/ufs/ufshcd.h | 8 +++++
3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fcd46251f7a8..d68222bb73ad 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9826,11 +9826,36 @@ static int ufshcd_resume(struct ufs_hba *hba)

/* enable the host irq as host controller would be active soon */
ufshcd_enable_irq(hba);
+
+ if (hba->restore) {
+ /* Configure UTRL and UTMRL base address registers */
+ ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
+ REG_UTP_TRANSFER_REQ_LIST_BASE_L);
+ ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
+ REG_UTP_TRANSFER_REQ_LIST_BASE_H);
+ ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
+ REG_UTP_TASK_REQ_LIST_BASE_L);
+ ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
+ REG_UTP_TASK_REQ_LIST_BASE_H);
+ /* Make sure that UTRL and UTMRL base address registers
+ * are updated with the latest queue addresses. Only after
+ * updating these addresses, we can queue the new commands.
+ */
+ mb();
+ }
+
+ /* Resuming from hibernate, assume that link was OFF */
+ if (hba->restore)
+ ufshcd_set_link_off(hba);
+
goto out;

disable_vreg:
ufshcd_vreg_set_lpm(hba);
out:
+ if (hba->restore)
+ hba->restore = false;
+
if (ret)
ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret);
return ret;
@@ -9989,6 +10014,43 @@ void ufshcd_remove(struct ufs_hba *hba)
}
EXPORT_SYMBOL_GPL(ufshcd_remove);

+int ufshcd_system_freeze(struct device *dev)
+{
+
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+ int ret = 0;
+
+ /*
+ * Run time resume the controller to make sure
+ * the PM work queue threads do not try to resume
+ * the child (scsi host), which leads to errors as
+ * the controller is not yet resumed.
+ */
+ pm_runtime_get_sync(hba->dev);
+ ret = ufshcd_system_suspend(dev);
+ pm_runtime_put_sync(hba->dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ufshcd_system_freeze);
+
+int ufshcd_system_restore(struct device *dev)
+{
+
+ struct ufs_hba *hba = dev_get_drvdata(dev);
+
+ hba->restore = true;
+ return ufshcd_system_resume(dev);
+
+}
+EXPORT_SYMBOL_GPL(ufshcd_system_restore);
+
+int ufshcd_system_thaw(struct device *dev)
+{
+ return ufshcd_system_resume(dev);
+}
+EXPORT_SYMBOL_GPL(ufshcd_system_thaw);
+
/**
* ufshcd_dealloc_host - deallocate Host Bus Adapter (HBA)
* @hba: pointer to Host Bus Adapter (HBA)
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 681da3ea7154..c92e041c5361 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1714,10 +1714,14 @@ MODULE_DEVICE_TABLE(acpi, ufs_qcom_acpi_match);
#endif

static const struct dev_pm_ops ufs_qcom_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
.prepare = ufshcd_suspend_prepare,
.complete = ufshcd_resume_complete,
+ .suspend = ufshcd_system_suspend,
+ .resume = ufshcd_system_resume,
+ .freeze = ufshcd_system_freeze,
+ .restore = ufshcd_system_restore,
+ .thaw = ufshcd_system_thaw,
};

static struct platform_driver ufs_qcom_pltform = {
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 1779238d8a56..6f50390ca262 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1071,6 +1071,9 @@ struct ufs_hba {
struct ufs_hw_queue *uhq;
struct ufs_hw_queue *dev_cmd_queue;
struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
+
+ /* Distinguish between resume and restore */
+ bool restore;
};

/**
@@ -1278,6 +1281,11 @@ extern int ufshcd_system_suspend(struct device *dev);
extern int ufshcd_system_resume(struct device *dev);
#endif
extern int ufshcd_shutdown(struct ufs_hba *hba);
+
+extern int ufshcd_system_freeze(struct device *dev);
+extern int ufshcd_system_thaw(struct device *dev);
+extern int ufshcd_system_restore(struct device *dev);
+
extern int ufshcd_dme_configure_adapt(struct ufs_hba *hba,
int agreed_gear,
int adapt_val);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

2023-01-20 21:54:53

by Bart Van Assche

[permalink] [raw]

Subject: Re: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

On 1/20/23 03:33, Anjana Hari wrote:
> + /* Make sure that UTRL and UTMRL base address registers
> + * are updated with the latest queue addresses. Only after
> + * updating these addresses, we can queue the new commands.
> + */

Please follow the kernel coding style and start comment blocks with "/*"
on a line by its own.

> + /* Resuming from hibernate, assume that link was OFF */
> + if (hba->restore)
> + ufshcd_set_link_off(hba);

Why two successive 'if (hba->restore)' statements? Can these two
if-statements be combined into a single if-statement?

> +int ufshcd_system_freeze(struct device *dev)
> +{
> +
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + /*
> + * Run time resume the controller to make sure
> + * the PM work queue threads do not try to resume
> + * the child (scsi host), which leads to errors as
> + * the controller is not yet resumed.
> + */
> + pm_runtime_get_sync(hba->dev);
> + ret = ufshcd_system_suspend(dev);
> + pm_runtime_put_sync(hba->dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_freeze);

Why does the above function use 'hba->dev' instead of 'dev'?

I do not understand the comment in the above function. My understanding
is that the power management core serializes hibernation and runtime
power management. How could runtime resume be in progress when
ufshcd_system_freeze() is called? Additionally, shouldn't
ufshcd_system_freeze() skip the UFS controller if it is already runtime
suspended instead of runtime resuming it? Some time ago I added the
following code in sd_suspend_system() (commit 9131bff6a9f1 ("scsi: core:
pm: Only runtime resume if necessary"):

if (pm_runtime_suspended(dev))
return 0;

Thanks,

Bart.

2023-01-20 21:55:35

by Bart Van Assche

[permalink] [raw]

Subject: Re: [PATCH v3 0/1] scsi: ufs: Add hibernation callbacks

On 1/20/23 03:33, Anjana Hari wrote:
> - Note to Bart: Regrading the comment to pass "restore" as an
> argument instead of adding a new member to ufs_hba structure, adding
> new function argument in core file (ufshcd.c) is forcing us to make
> changes to other vendor files to fix the compilation errors. Hence
> we have retained our original change. Please let us know your inputs
> on this.
Storing state information in a structure member that can be passed as a
function argument makes code harder to read and to maintain than
necessary. Please address my request before this patch goes upstream. I'm
concerned if someone would try to address my request after this patch went
upstream that there would be no motivation from your side to help with
testing the refactoring patch.

I think the patch below shows that it is easy to eliminate the new 'restore'
member variable. Please note that the patch below has not been tested in any
way.

---
drivers/ufs/core/ufshcd.c | 48 +++++++++++++++++++--------------------
include/ufs/ufshcd.h | 3 ---
2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 19608f3a38f9..b5cfbc1fccc6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9801,34 +9801,11 @@ static int ufshcd_resume(struct ufs_hba *hba)
/* enable the host irq as host controller would be active soon */
ufshcd_enable_irq(hba);

- if (hba->restore) {
- /* Configure UTRL and UTMRL base address registers */
- ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
- REG_UTP_TRANSFER_REQ_LIST_BASE_L);
- ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
- REG_UTP_TRANSFER_REQ_LIST_BASE_H);
- ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
- REG_UTP_TASK_REQ_LIST_BASE_L);
- ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
- REG_UTP_TASK_REQ_LIST_BASE_H);
- /* Make sure that UTRL and UTMRL base address registers
- * are updated with the latest queue addresses. Only after
- * updating these addresses, we can queue the new commands.
- */
- mb();
- }
-
- /* Resuming from hibernate, assume that link was OFF */
- if (hba->restore)
- ufshcd_set_link_off(hba);
-
goto out;

disable_vreg:
ufshcd_vreg_set_lpm(hba);
out:
- if (hba->restore)
- hba->restore = false;

if (ret)
ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret);
@@ -10012,10 +9989,31 @@ int ufshcd_system_restore(struct device *dev)
{

struct ufs_hba *hba = dev_get_drvdata(dev);
+ int ret;

- hba->restore = true;
- return ufshcd_system_resume(dev);
+ ret = ufshcd_system_resume(dev);
+ if (ret)
+ return ret;
+
+ /* Configure UTRL and UTMRL base address registers */
+ ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
+ REG_UTP_TRANSFER_REQ_LIST_BASE_L);
+ ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
+ REG_UTP_TRANSFER_REQ_LIST_BASE_H);
+ ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
+ REG_UTP_TASK_REQ_LIST_BASE_L);
+ ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
+ REG_UTP_TASK_REQ_LIST_BASE_H);
+ /* Make sure that UTRL and UTMRL base address registers
+ * are updated with the latest queue addresses. Only after
+ * updating these addresses, we can queue the new commands.
+ */
+ mb();

+ /* Resuming from hibernate, assume that link was OFF */
+ ufshcd_set_link_off(hba);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(ufshcd_system_restore);

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 6f50390ca262..1d6dd13e1651 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1071,9 +1071,6 @@ struct ufs_hba {
struct ufs_hw_queue *uhq;
struct ufs_hw_queue *dev_cmd_queue;
struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX];
-
- /* Distinguish between resume and restore */
- bool restore;
};

/**

2023-01-21 05:15:40

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

2023-01-21 05:20:15

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

Hi Anjana,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on next-20230120]
[cannot apply to jejb-scsi/for-next linus/master v6.2-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Anjana-Hari/scsi-ufs-Add-hibernation-callbacks/20230120-193447
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link: https://lore.kernel.org/r/20230120113321.30433-2-quic_ahari%40quicinc.com
patch subject: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks
config: riscv-randconfig-r042-20230119 (https://download.01.org/0day-ci/archive/20230121/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/239ad2244616006dd39bc9a5380108435d168a86
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anjana-Hari/scsi-ufs-Add-hibernation-callbacks/20230120-193447
git checkout 239ad2244616006dd39bc9a5380108435d168a86
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/ufs/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/ufs/core/ufshcd.c:10004:8: error: call to undeclared function 'ufshcd_system_suspend'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
ret = ufshcd_system_suspend(dev);
^
>> drivers/ufs/core/ufshcd.c:10017:9: error: call to undeclared function 'ufshcd_system_resume'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
return ufshcd_system_resume(dev);
^
drivers/ufs/core/ufshcd.c:10017:9: note: did you mean 'ufshcd_system_restore'?
drivers/ufs/core/ufshcd.c:10011:5: note: 'ufshcd_system_restore' declared here
int ufshcd_system_restore(struct device *dev)
^
drivers/ufs/core/ufshcd.c:10024:9: error: call to undeclared function 'ufshcd_system_resume'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
return ufshcd_system_resume(dev);
^
drivers/ufs/core/ufshcd.c:10048:44: warning: shift count >= width of type [-Wshift-count-overflow]
if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
drivers/ufs/core/ufshcd.c:10048:44: warning: shift count >= width of type [-Wshift-count-overflow]
if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
drivers/ufs/core/ufshcd.c:10048:44: warning: shift count >= width of type [-Wshift-count-overflow]
if (!dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(64)))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
~~~~~~~~~~~~~~~~~^~~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
3 warnings and 3 errors generated.

vim +/ufshcd_system_suspend +10004 drivers/ufs/core/ufshcd.c

9993
9994 struct ufs_hba *hba = dev_get_drvdata(dev);
9995 int ret = 0;
9996
9997 /*
9998 * Run time resume the controller to make sure
9999 * the PM work queue threads do not try to resume
10000 * the child (scsi host), which leads to errors as
10001 * the controller is not yet resumed.
10002 */
10003 pm_runtime_get_sync(hba->dev);
10004 ret = ufshcd_system_suspend(dev);
10005 pm_runtime_put_sync(hba->dev);
10006
10007 return ret;
10008 }
10009 EXPORT_SYMBOL_GPL(ufshcd_system_freeze);
10010
10011 int ufshcd_system_restore(struct device *dev)
10012 {
10013
10014 struct ufs_hba *hba = dev_get_drvdata(dev);
10015
10016 hba->restore = true;
10017 return ufshcd_system_resume(dev);
10018

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests