linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] scsi: allow auto suspend override by low-level driver
@ 2019-09-11  9:41 Stanley Chu
  2019-09-11  9:41 ` [PATCH v1 1/3] scsi: core: " Stanley Chu
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Stanley Chu @ 2019-09-11  9:41 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng, Stanley Chu

Until now the scsi mid-layer forbids runtime suspend till userspace
enables it. This is mainly to quarantine some disks with broken
runtime power management or have high latencies executing suspend
resume callbacks. If the userspace doesn't enable the runtime suspend
the underlying hardware will be always on even when it is not doing
any useful work and thus wasting power.

Some low-level drivers for the controllers can efficiently use runtime
power management to reduce power consumption and improve battery life.

This patchset allows runtime suspend parameters override within the LLD itself
instead of waiting for userspace to control the power management, and
make UFS as the first user of this capability.

Stanley Chu (3):
  scsi: core: allow auto suspend override by low-level driver
  scsi: ufs: override auto suspend tunables for ufs
  scsi: ufs-mediatek: enable auto suspend capability

 drivers/scsi/scsi_sysfs.c       |  3 ++-
 drivers/scsi/sd.c               |  3 +++
 drivers/scsi/ufs/ufs-mediatek.c |  7 +++++++
 drivers/scsi/ufs/ufshcd.c       |  8 ++++++++
 drivers/scsi/ufs/ufshcd.h       | 10 ++++++++++
 include/scsi/scsi_device.h      |  2 +-
 6 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 1/3] scsi: core: allow auto suspend override by low-level driver
  2019-09-11  9:41 [PATCH v1] scsi: allow auto suspend override by low-level driver Stanley Chu
@ 2019-09-11  9:41 ` Stanley Chu
  2019-09-11 10:54   ` Avri Altman
  2019-09-11  9:41 ` [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs Stanley Chu
  2019-09-11  9:41 ` [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability Stanley Chu
  2 siblings, 1 reply; 10+ messages in thread
From: Stanley Chu @ 2019-09-11  9:41 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng, Stanley Chu

Rework from previous work by:
Sujit Reddy Thumma <sthumma@codeaurora.org>

Until now the scsi mid-layer forbids runtime suspend till userspace
enables it. This is mainly to quarantine some disks with broken
runtime power management or have high latencies executing suspend
resume callbacks. If the userspace doesn't enable the runtime suspend
the underlying hardware will be always on even when it is not doing
any useful work and thus wasting power.

Some low-level drivers for the controllers can efficiently use runtime
power management to reduce power consumption and improve battery life.
Allow runtime suspend parameters override within the LLD itself
instead of waiting for userspace to control the power management.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/scsi_sysfs.c  | 3 ++-
 drivers/scsi/sd.c          | 3 +++
 include/scsi/scsi_device.h | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 64c96c7828ee..66a8a5c74352 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1300,7 +1300,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	device_enable_async_suspend(&sdev->sdev_gendev);
 	scsi_autopm_get_target(starget);
 	pm_runtime_set_active(&sdev->sdev_gendev);
-	pm_runtime_forbid(&sdev->sdev_gendev);
+	if (sdev->rpm_autosuspend_delay <= 0)
+		pm_runtime_forbid(&sdev->sdev_gendev);
 	pm_runtime_enable(&sdev->sdev_gendev);
 	scsi_autopm_put_target(starget);
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 149d406aacc9..2218d57c4c0c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev)
 	}
 
 	blk_pm_runtime_init(sdp->request_queue, dev);
+	if (sdp->rpm_autosuspend_delay > 0)
+		pm_runtime_set_autosuspend_delay(dev,
+						 sdp->rpm_autosuspend_delay);
 	device_add_disk(dev, gd, NULL);
 	if (sdkp->capacity)
 		sd_dif_config_host(sdkp);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 202f4d6a4342..133b282fae5a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -199,7 +199,7 @@ struct scsi_device {
 	unsigned broken_fua:1;		/* Don't set FUA bit */
 	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
 	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit for WRITE SAME */
-
+	int rpm_autosuspend_delay;
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs
  2019-09-11  9:41 [PATCH v1] scsi: allow auto suspend override by low-level driver Stanley Chu
  2019-09-11  9:41 ` [PATCH v1 1/3] scsi: core: " Stanley Chu
@ 2019-09-11  9:41 ` Stanley Chu
  2019-09-11 10:56   ` Avri Altman
  2019-09-11  9:41 ` [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability Stanley Chu
  2 siblings, 1 reply; 10+ messages in thread
From: Stanley Chu @ 2019-09-11  9:41 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng, Stanley Chu

Rework from previous work by:
Sujit Reddy Thumma <sthumma@codeaurora.org>

Override auto suspend tunables for UFS device LUNs during
initialization so as to efficiently manage background operations
and the power consumption.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c |  8 ++++++++
 drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 30b752c61b97..d253a018a73b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -88,6 +88,9 @@
 /* Interrupt aggregation default timeout, unit: 40us */
 #define INT_AGGR_DEF_TO	0x02
 
+/* default delay of autosuspend: 2000 ms */
+#define RPM_AUTOSUSPEND_DELAY_MS 2000
+
 #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
 	({                                                              \
 		int _ret;                                               \
@@ -4612,9 +4615,14 @@ static int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth)
  */
 static int ufshcd_slave_configure(struct scsi_device *sdev)
 {
+	struct ufs_hba *hba = shost_priv(sdev->host);
 	struct request_queue *q = sdev->request_queue;
 
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
+
+	if (ufshcd_is_rpm_autosuspend_allowed(hba))
+		sdev->rpm_autosuspend_delay = RPM_AUTOSUSPEND_DELAY_MS;
+
 	return 0;
 }
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index a43c7135f33d..99ea416519af 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -714,6 +714,12 @@ struct ufs_hba {
 	 * the performance of ongoing read/write operations.
 	 */
 #define UFSHCD_CAP_KEEP_AUTO_BKOPS_ENABLED_EXCEPT_SUSPEND (1 << 5)
+	/*
+	 * This capability allows host controller driver to automatically
+	 * enable runtime power management by itself instead of waiting
+	 * for userspace to control the power management.
+	 */
+#define UFSHCD_CAP_RPM_AUTOSUSPEND (1 << 6)
 
 	struct devfreq *devfreq;
 	struct ufs_clk_scaling clk_scaling;
@@ -747,6 +753,10 @@ static inline bool ufshcd_can_autobkops_during_suspend(struct ufs_hba *hba)
 {
 	return hba->caps & UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
 }
+static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba *hba)
+{
+	return hba->caps & UFSHCD_CAP_RPM_AUTOSUSPEND;
+}
 
 static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba)
 {
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability
  2019-09-11  9:41 [PATCH v1] scsi: allow auto suspend override by low-level driver Stanley Chu
  2019-09-11  9:41 ` [PATCH v1 1/3] scsi: core: " Stanley Chu
  2019-09-11  9:41 ` [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs Stanley Chu
@ 2019-09-11  9:41 ` Stanley Chu
  2019-09-11 10:58   ` Avri Altman
  2 siblings, 1 reply; 10+ messages in thread
From: Stanley Chu @ 2019-09-11  9:41 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng, Stanley Chu

Enable auto suspend capability in MediaTek UFS driver.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 0f6ff33ce52e..b7b177c6194c 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -117,6 +117,11 @@ static int ufs_mtk_setup_clocks(struct ufs_hba *hba, bool on,
 	return ret;
 }
 
+static void ufs_mtk_set_caps(struct ufs_hba *hba)
+{
+	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
+}
+
 /**
  * ufs_mtk_init - find other essential mmio bases
  * @hba: host controller instance
@@ -147,6 +152,8 @@ static int ufs_mtk_init(struct ufs_hba *hba)
 	if (err)
 		goto out_variant_clear;
 
+	ufs_mtk_set_caps(hba);
+
 	/*
 	 * ufshcd_vops_init() is invoked after
 	 * ufshcd_setup_clock(true) in ufshcd_hba_init() thus
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 1/3] scsi: core: allow auto suspend override by low-level driver
  2019-09-11  9:41 ` [PATCH v1 1/3] scsi: core: " Stanley Chu
@ 2019-09-11 10:54   ` Avri Altman
  2019-09-12  2:24     ` Stanley Chu
  0 siblings, 1 reply; 10+ messages in thread
From: Avri Altman @ 2019-09-11 10:54 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi, martin.petersen, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

> 
> Rework from previous work by:
> Sujit Reddy Thumma <sthumma@codeaurora.org>
> 
> Until now the scsi mid-layer forbids runtime suspend till userspace enables it.
> This is mainly to quarantine some disks with broken runtime power
> management or have high latencies executing suspend resume callbacks. If
> the userspace doesn't enable the runtime suspend the underlying hardware
> will be always on even when it is not doing any useful work and thus wasting
> power.
> 
> Some low-level drivers for the controllers can efficiently use runtime power
> management to reduce power consumption and improve battery life.
> Allow runtime suspend parameters override within the LLD itself instead of
> waiting for userspace to control the power management.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Stanley hi,
Your series looks fine.
I added some comments/questions - feel free to ignore it.

Thanks,
Avri


> ---
>  drivers/scsi/scsi_sysfs.c  | 3 ++-
>  drivers/scsi/sd.c          | 3 +++
>  include/scsi/scsi_device.h | 2 +-
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index
> 64c96c7828ee..66a8a5c74352 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1300,7 +1300,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>         device_enable_async_suspend(&sdev->sdev_gendev);
>         scsi_autopm_get_target(starget);
>         pm_runtime_set_active(&sdev->sdev_gendev);
> -       pm_runtime_forbid(&sdev->sdev_gendev);
> +       if (sdev->rpm_autosuspend_delay <= 0)
> +               pm_runtime_forbid(&sdev->sdev_gendev);
>         pm_runtime_enable(&sdev->sdev_gendev);
>         scsi_autopm_put_target(starget);
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> 149d406aacc9..2218d57c4c0c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev)
>         }
> 
>         blk_pm_runtime_init(sdp->request_queue, dev);
> +       if (sdp->rpm_autosuspend_delay > 0)
> +               pm_runtime_set_autosuspend_delay(dev, 
> +
Redundant line ?
> + sdp->rpm_autosuspend_delay);
Don't you need to call now pm_runtime_use_autosuspend() ?

>         device_add_disk(dev, gd, NULL);
>         if (sdkp->capacity)
>                 sd_dif_config_host(sdkp); diff --git a/include/scsi/scsi_device.h
> b/include/scsi/scsi_device.h index 202f4d6a4342..133b282fae5a 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -199,7 +199,7 @@ struct scsi_device {
>         unsigned broken_fua:1;          /* Don't set FUA bit */
>         unsigned lun_in_cdb:1;          /* Store LUN bits in CDB[1] */
>         unsigned unmap_limit_for_ws:1;  /* Use the UNMAP limit for WRITE
> SAME */
> -
> +       int rpm_autosuspend_delay;
Can suspend be negative?

>         atomic_t disk_events_disable_depth; /* disable depth for disk events */
> 
>         DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /*
> supported events */
> --
> 2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs
  2019-09-11  9:41 ` [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs Stanley Chu
@ 2019-09-11 10:56   ` Avri Altman
  2019-09-12  5:37     ` Stanley Chu
  0 siblings, 1 reply; 10+ messages in thread
From: Avri Altman @ 2019-09-11 10:56 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi, martin.petersen, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

> 
> Rework from previous work by:
> Sujit Reddy Thumma <sthumma@codeaurora.org>
> 
> Override auto suspend tunables for UFS device LUNs during initialization so
> as to efficiently manage background operations and the power consumption.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/scsi/ufs/ufshcd.c |  8 ++++++++  drivers/scsi/ufs/ufshcd.h | 10
> ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 30b752c61b97..d253a018a73b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -88,6 +88,9 @@
>  /* Interrupt aggregation default timeout, unit: 40us */
>  #define INT_AGGR_DEF_TO        0x02
> 
> +/* default delay of autosuspend: 2000 ms */ #define
Typo?

> +RPM_AUTOSUSPEND_DELAY_MS 2000
> +
>  #define ufshcd_toggle_vreg(_dev, _vreg, _on)                           \
>         ({                                                              \
>                 int _ret;                                               \
> @@ -4612,9 +4615,14 @@ static int ufshcd_change_queue_depth(struct
> scsi_device *sdev, int depth)
>   */
>  static int ufshcd_slave_configure(struct scsi_device *sdev)  {
> +       struct ufs_hba *hba = shost_priv(sdev->host);
>         struct request_queue *q = sdev->request_queue;
> 
>         blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
> +
> +       if (ufshcd_is_rpm_autosuspend_allowed(hba))
> +               sdev->rpm_autosuspend_delay =
> RPM_AUTOSUSPEND_DELAY_MS;
> +
>         return 0;
>  }
> 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> a43c7135f33d..99ea416519af 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -714,6 +714,12 @@ struct ufs_hba {
>          * the performance of ongoing read/write operations.
>          */
>  #define UFSHCD_CAP_KEEP_AUTO_BKOPS_ENABLED_EXCEPT_SUSPEND (1
> << 5)
> +       /*
> +        * This capability allows host controller driver to automatically
> +        * enable runtime power management by itself instead of waiting
> +        * for userspace to control the power management.
> +        */
> +#define UFSHCD_CAP_RPM_AUTOSUSPEND (1 << 6)
> 
>         struct devfreq *devfreq;
>         struct ufs_clk_scaling clk_scaling; @@ -747,6 +753,10 @@ static inline
> bool ufshcd_can_autobkops_during_suspend(struct ufs_hba *hba)  {
>         return hba->caps & UFSHCD_CAP_AUTO_BKOPS_SUSPEND;  }
> +static inline bool ufshcd_is_rpm_autosuspend_allowed(struct ufs_hba
> +*hba) {
> +       return hba->caps & UFSHCD_CAP_RPM_AUTOSUSPEND; }
> 
>  static inline bool ufshcd_is_intr_aggr_allowed(struct ufs_hba *hba)  {
> --
> 2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability
  2019-09-11  9:41 ` [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability Stanley Chu
@ 2019-09-11 10:58   ` Avri Altman
  2019-09-12  5:39     ` Stanley Chu
  0 siblings, 1 reply; 10+ messages in thread
From: Avri Altman @ 2019-09-11 10:58 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi, martin.petersen, alim.akhtar,
	pedrom.sousa, sthumma, jejb, bvanassche
  Cc: linux-mediatek, linux-arm-kernel, matthias.bgg, evgreen, beanhuo,
	marc.w.gonzalez, subhashj, vivek.gautam, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

> 
> Enable auto suspend capability in MediaTek UFS driver.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 0f6ff33ce52e..b7b177c6194c 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -117,6 +117,11 @@ static int ufs_mtk_setup_clocks(struct ufs_hba
> *hba, bool on,
>         return ret;
>  }
> 
> +static void ufs_mtk_set_caps(struct ufs_hba *hba) {
> +       hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND; }
Even a one-liner deserve new line for its closing brackets

> +
>  /**
>   * ufs_mtk_init - find other essential mmio bases
>   * @hba: host controller instance
> @@ -147,6 +152,8 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>         if (err)
>                 goto out_variant_clear;
> 
> +       ufs_mtk_set_caps(hba);
> +
>         /*
>          * ufshcd_vops_init() is invoked after
>          * ufshcd_setup_clock(true) in ufshcd_hba_init() thus
> --
> 2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 1/3] scsi: core: allow auto suspend override by low-level driver
  2019-09-11 10:54   ` Avri Altman
@ 2019-09-12  2:24     ` Stanley Chu
  0 siblings, 0 replies; 10+ messages in thread
From: Stanley Chu @ 2019-09-12  2:24 UTC (permalink / raw)
  To: Avri Altman
  Cc: linux-scsi, martin.petersen, alim.akhtar, pedrom.sousa, sthumma,
	jejb, bvanassche, linux-mediatek, linux-arm-kernel, matthias.bgg,
	evgreen, beanhuo, marc.w.gonzalez, subhashj, vivek.gautam,
	Kuohong Wang (王國鴻),
	Peter Wang (王信友),
	Chun-Hung Wu (巫駿宏),
	Andy Teng ( ^[$B{}G!9(^[(B)

Hi Avri,

> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > 149d406aacc9..2218d57c4c0c 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev)
> >         }
> > 
> >         blk_pm_runtime_init(sdp->request_queue, dev);
> > +       if (sdp->rpm_autosuspend_delay > 0)
> > +               pm_runtime_set_autosuspend_delay(dev, 
> > +
> Redundant line ?

checkpatch reported "WARNING:LONG_LINE:line over 80 characters" when I
made this as oneline : (

> > + sdp->rpm_autosuspend_delay);
> Don't you need to call now pm_runtime_use_autosuspend() ?

dev->power.user_autosuspend was set by blk_pm_runtime_init() above, thus
pm_runtime_use_autosuspend() is not necessary here.

> 
> >         device_add_disk(dev, gd, NULL);
> >         if (sdkp->capacity)
> >                 sd_dif_config_host(sdkp); diff --git a/include/scsi/scsi_device.h
> > b/include/scsi/scsi_device.h index 202f4d6a4342..133b282fae5a 100644
> > --- a/include/scsi/scsi_device.h
> > +++ b/include/scsi/scsi_device.h
> > @@ -199,7 +199,7 @@ struct scsi_device {
> >         unsigned broken_fua:1;          /* Don't set FUA bit */
> >         unsigned lun_in_cdb:1;          /* Store LUN bits in CDB[1] */
> >         unsigned unmap_limit_for_ws:1;  /* Use the UNMAP limit for WRITE
> > SAME */
> > -
> > +       int rpm_autosuspend_delay;
> Can suspend be negative?

Yes, however negative delay value will block rpm.

Here we just use the same type as parameter "delay" of
pm_runtime_set_autosuspend() even though we do not set it as negative
value in this version.

But thank you so much to remind me that
pm_runtime_set_autosuspend_delay() can accept "zero" delay so we shall
allow "zero" sdev->rpm_autosuspend_delay as well.

I will fix it in v2.

> 
> >         atomic_t disk_events_disable_depth; /* disable depth for disk events */
> > 
> >         DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /*
> > supported events */
> > --
> > 2.18.0
> 

Thanks,
Stanley



^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs
  2019-09-11 10:56   ` Avri Altman
@ 2019-09-12  5:37     ` Stanley Chu
  0 siblings, 0 replies; 10+ messages in thread
From: Stanley Chu @ 2019-09-12  5:37 UTC (permalink / raw)
  To: Avri Altman
  Cc: linux-scsi, martin.petersen, alim.akhtar, pedrom.sousa, sthumma,
	jejb, bvanassche, linux-mediatek, linux-arm-kernel, matthias.bgg,
	evgreen, beanhuo, marc.w.gonzalez, subhashj, vivek.gautam,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng

Hi Avri,

> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> > 30b752c61b97..d253a018a73b 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -88,6 +88,9 @@
> >  /* Interrupt aggregation default timeout, unit: 40us */
> >  #define INT_AGGR_DEF_TO        0x02
> > 
> > +/* default delay of autosuspend: 2000 ms */ #define
> Typo?
> 

This is wired because it looks fine in both my local patch and in
patchwork website: https://patchwork.kernel.org/patch/11140759/

Anyway I will try to fix and check it carefully in v2.

Thanks,
Stanley


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability
  2019-09-11 10:58   ` Avri Altman
@ 2019-09-12  5:39     ` Stanley Chu
  0 siblings, 0 replies; 10+ messages in thread
From: Stanley Chu @ 2019-09-12  5:39 UTC (permalink / raw)
  To: Avri Altman
  Cc: linux-scsi, martin.petersen, alim.akhtar, pedrom.sousa, sthumma,
	jejb, bvanassche, linux-mediatek, linux-arm-kernel, matthias.bgg,
	evgreen, beanhuo, marc.w.gonzalez, subhashj, vivek.gautam,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng

Hi Avri,

On Wed, 2019-09-11 at 10:58 +0000, Avri Altman wrote:
> > 
> > Enable auto suspend capability in MediaTek UFS driver.
> > 
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> 
> > ---
> >  drivers/scsi/ufs/ufs-mediatek.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> > index 0f6ff33ce52e..b7b177c6194c 100644
> > --- a/drivers/scsi/ufs/ufs-mediatek.c
> > +++ b/drivers/scsi/ufs/ufs-mediatek.c
> > @@ -117,6 +117,11 @@ static int ufs_mtk_setup_clocks(struct ufs_hba
> > *hba, bool on,
> >         return ret;
> >  }
> > 
> > +static void ufs_mtk_set_caps(struct ufs_hba *hba) {
> > +       hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND; }
> Even a one-liner deserve new line for its closing brackets

The wired format is just happening the same as
[PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs

It looks fine in patchwork website:
https://patchwork.kernel.org/patch/11140757/

I'll try to fix it in v2.

Thanks,
Stanley




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-09-12  5:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11  9:41 [PATCH v1] scsi: allow auto suspend override by low-level driver Stanley Chu
2019-09-11  9:41 ` [PATCH v1 1/3] scsi: core: " Stanley Chu
2019-09-11 10:54   ` Avri Altman
2019-09-12  2:24     ` Stanley Chu
2019-09-11  9:41 ` [PATCH v1 2/3] scsi: ufs: override auto suspend tunables for ufs Stanley Chu
2019-09-11 10:56   ` Avri Altman
2019-09-12  5:37     ` Stanley Chu
2019-09-11  9:41 ` [PATCH v1 3/3] scsi: ufs-mediatek: enable auto suspend capability Stanley Chu
2019-09-11 10:58   ` Avri Altman
2019-09-12  5:39     ` Stanley Chu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).