From: Jaegeuk Kim <[email protected]>
When giving a stress test which enables/disables clkgating, we hit device
timeout sometimes. This patch avoids subtle racy condition to address it.
Cc: Alim Akhtar <[email protected]>
Cc: Avri Altman <[email protected]>
Cc: Can Guo <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1d157ff58d817..d929c3d1e58cc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1791,19 +1791,19 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev,
return -EINVAL;
value = !!value;
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
if (value == hba->clk_gating.is_enabled)
goto out;
- if (value) {
- ufshcd_release(hba);
- } else {
- spin_lock_irqsave(hba->host->host_lock, flags);
+ if (value)
+ hba->clk_gating.active_reqs--;
+ else
hba->clk_gating.active_reqs++;
- spin_unlock_irqrestore(hba->host->host_lock, flags);
- }
hba->clk_gating.is_enabled = value;
out:
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
return count;
}
--
2.28.0.806.g8561365e88-goog
From: Jaegeuk Kim <[email protected]>
In order to conduct FFU or RPMB operations, UFS needs to clear UAC. This patch
clears it explicitly, so that we could get no failure given early execution.
Cc: Alim Akhtar <[email protected]>
Cc: Avri Altman <[email protected]>
Cc: Can Guo <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
drivers/scsi/ufs/ufshcd.h | 1 +
2 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d929c3d1e58cc..0bb07b50bd23e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6841,7 +6841,6 @@ static inline void ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
{
int ret = 0;
- struct scsi_device *sdev_rpmb;
struct scsi_device *sdev_boot;
hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
@@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
scsi_device_put(hba->sdev_ufs_device);
- sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
+ hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
- if (IS_ERR(sdev_rpmb)) {
- ret = PTR_ERR(sdev_rpmb);
+ if (IS_ERR(hba->sdev_rpmb)) {
+ ret = PTR_ERR(hba->sdev_rpmb);
goto remove_sdev_ufs_device;
}
- ufshcd_blk_pm_runtime_init(sdev_rpmb);
- scsi_device_put(sdev_rpmb);
+ ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
+ scsi_device_put(hba->sdev_rpmb);
sdev_boot = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
@@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
return ret;
}
+static int
+ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp);
+
+static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
+{
+ struct scsi_device *sdp;
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
+ sdp = hba->sdev_ufs_device;
+ else if (wlun == UFS_UPIU_RPMB_WLUN)
+ sdp = hba->sdev_rpmb;
+ else
+ BUG_ON(1);
+ if (sdp) {
+ ret = scsi_device_get(sdp);
+ if (!ret && !scsi_device_online(sdp)) {
+ ret = -ENODEV;
+ scsi_device_put(sdp);
+ }
+ } else {
+ ret = -ENODEV;
+ }
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ if (ret)
+ goto out_err;
+
+ ret = ufshcd_send_request_sense(hba, sdp);
+ scsi_device_put(sdp);
+out_err:
+ if (ret)
+ dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
+ __func__, wlun, ret);
+ return ret;
+}
+
+static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
+{
+ int ret = 0;
+
+ if (!hba->wlun_dev_clr_ua)
+ goto out;
+
+ ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
+ if (!ret)
+ ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
+ if (!ret)
+ hba->wlun_dev_clr_ua = false;
+out:
+ if (ret)
+ dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
+ __func__, ret);
+ return ret;
+}
+
/**
* ufshcd_probe_hba - probe hba to detect device and initialize
* @hba: per-adapter instance
@@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
pm_runtime_put_sync(hba->dev);
ufshcd_exit_clk_scaling(hba);
ufshcd_hba_exit(hba);
+ } else {
+ ufshcd_clear_ua_wluns(hba);
}
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 363589c0bd370..8344d8cb36786 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -662,6 +662,7 @@ struct ufs_hba {
* "UFS device" W-LU.
*/
struct scsi_device *sdev_ufs_device;
+ struct scsi_device *sdev_rpmb;
enum ufs_dev_pwr_mode curr_dev_pwr_mode;
enum uic_link_state uic_link_state;
--
2.28.0.806.g8561365e88-goog
From: Jaegeuk Kim <[email protected]>
Must have WQ_MEM_RECLAIM
``WQ_MEM_RECLAIM``
All wq which might be used in the memory reclaim paths **MUST**
have this flag set. The wq is guaranteed to have at least one
execution context regardless of memory pressure.
Cc: Alim Akhtar <[email protected]>
Cc: Avri Altman <[email protected]>
Cc: Can Guo <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0bb07b50bd23e..76e95963887be 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
hba->host->host_no);
hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
- WQ_MEM_RECLAIM);
+ WQ_MEM_RECLAIM | WQ_HIGHPRI);
hba->clk_gating.is_enabled = true;
--
2.28.0.806.g8561365e88-goog
From: Jaegeuk Kim <[email protected]>
This adds user-friendly tracepoints with group id.
Cc: Alim Akhtar <[email protected]>
Cc: Avri Altman <[email protected]>
Cc: Can Guo <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 6 ++++--
include/trace/events/ufs.h | 21 +++++++++++++++++----
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 76e95963887be..a2db8182663da 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
unsigned int tag, const char *str)
{
sector_t lba = -1;
- u8 opcode = 0;
+ u8 opcode = 0, group_id = 0;
u32 intr, doorbell;
struct ufshcd_lrb *lrbp = &hba->lrb[tag];
struct scsi_cmnd *cmd = lrbp->cmd;
@@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
lba = cmd->request->bio->bi_iter.bi_sector;
transfer_len = be32_to_cpu(
lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
+ if (opcode == WRITE_10)
+ group_id = lrbp->cmd->cmnd[6];
}
}
intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
trace_ufshcd_command(dev_name(hba->dev), str, tag,
- doorbell, transfer_len, intr, lba, opcode);
+ doorbell, transfer_len, intr, lba, opcode, group_id);
}
static void ufshcd_print_clk_freqs(struct ufs_hba *hba)
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 84841b3a7ffd5..50654f3526392 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -11,6 +11,15 @@
#include <linux/tracepoint.h>
+#define str_opcode(opcode) \
+ __print_symbolic(opcode, \
+ { WRITE_16, "WRITE_16" }, \
+ { WRITE_10, "WRITE_10" }, \
+ { READ_16, "READ_16" }, \
+ { READ_10, "READ_10" }, \
+ { SYNCHRONIZE_CACHE, "SYNC" }, \
+ { UNMAP, "UNMAP" })
+
#define UFS_LINK_STATES \
EM(UIC_LINK_OFF_STATE) \
EM(UIC_LINK_ACTIVE_STATE) \
@@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
TRACE_EVENT(ufshcd_command,
TP_PROTO(const char *dev_name, const char *str, unsigned int tag,
u32 doorbell, int transfer_len, u32 intr, u64 lba,
- u8 opcode),
+ u8 opcode, u8 group_id),
- TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba, opcode),
+ TP_ARGS(dev_name, str, tag, doorbell, transfer_len,
+ intr, lba, opcode, group_id),
TP_STRUCT__entry(
__string(dev_name, dev_name)
@@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command,
__field(u32, intr)
__field(u64, lba)
__field(u8, opcode)
+ __field(u8, group_id)
),
TP_fast_assign(
@@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command,
__entry->intr = intr;
__entry->lba = lba;
__entry->opcode = opcode;
+ __entry->group_id = group_id;
),
TP_printk(
- "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x",
+ "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x",
__get_str(str), __get_str(dev_name), __entry->tag,
__entry->doorbell, __entry->transfer_len,
- __entry->intr, __entry->lba, (u32)__entry->opcode
+ __entry->intr, __entry->lba, (u32)__entry->opcode,
+ str_opcode(__entry->opcode), (u32)__entry->group_id
)
);
--
2.28.0.806.g8561365e88-goog
On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <[email protected]>
>
> In order to conduct FFU or RPMB operations, UFS needs to clear UAC.
> This patch
> clears it explicitly, so that we could get no failure given early
> execution.
>
Usually it is the user's/utility's/tool's responsiblity to clear UA by
sending a
request sense cmd and retry previous cmd, now we are doing it for the
users in driver?
As per my understanding, driver only reports UA to SCSI layer and let
users decide
what to do with it - maybe users need to do something specifically regs
it, but
the change clears it even before the users get to know it.
Besides, this change clears UA for W-LUs, but the UFS driver still
reports UA to SCSI
layer for each SCSI device by calling scsi_report_bus_reset() in
ufshcd_reset_and_restore(). This will make SCSI layer treat
sdev->expecting_cc_ua
wrongly, because for W-LUs, their expecting_cc_ua should not be set as
you have
cleared their UAs.
Thanks,
Can Guo.
> Cc: Alim Akhtar <[email protected]>
> Cc: Avri Altman <[email protected]>
> Cc: Can Guo <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
> drivers/scsi/ufs/ufshcd.h | 1 +
> 2 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d929c3d1e58cc..0bb07b50bd23e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6841,7 +6841,6 @@ static inline void
> ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
> static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
> {
> int ret = 0;
> - struct scsi_device *sdev_rpmb;
> struct scsi_device *sdev_boot;
>
> hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
> @@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> *hba)
> ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
> scsi_device_put(hba->sdev_ufs_device);
>
> - sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> + hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> - if (IS_ERR(sdev_rpmb)) {
> - ret = PTR_ERR(sdev_rpmb);
> + if (IS_ERR(hba->sdev_rpmb)) {
> + ret = PTR_ERR(hba->sdev_rpmb);
> goto remove_sdev_ufs_device;
> }
> - ufshcd_blk_pm_runtime_init(sdev_rpmb);
> - scsi_device_put(sdev_rpmb);
> + ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> + scsi_device_put(hba->sdev_rpmb);
>
> sdev_boot = __scsi_add_device(hba->host, 0, 0,
> ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> @@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
> return ret;
> }
>
> +static int
> +ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device
> *sdp);
> +
> +static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
> +{
> + struct scsi_device *sdp;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
> + sdp = hba->sdev_ufs_device;
> + else if (wlun == UFS_UPIU_RPMB_WLUN)
> + sdp = hba->sdev_rpmb;
> + else
> + BUG_ON(1);
> + if (sdp) {
> + ret = scsi_device_get(sdp);
> + if (!ret && !scsi_device_online(sdp)) {
> + ret = -ENODEV;
> + scsi_device_put(sdp);
> + }
> + } else {
> + ret = -ENODEV;
> + }
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + if (ret)
> + goto out_err;
> +
> + ret = ufshcd_send_request_sense(hba, sdp);
> + scsi_device_put(sdp);
> +out_err:
> + if (ret)
> + dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
> + __func__, wlun, ret);
> + return ret;
> +}
> +
> +static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
> +{
> + int ret = 0;
> +
> + if (!hba->wlun_dev_clr_ua)
> + goto out;
> +
> + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
> + if (!ret)
> + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> + if (!ret)
> + hba->wlun_dev_clr_ua = false;
> +out:
> + if (ret)
> + dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
> + __func__, ret);
> + return ret;
> +}
> +
> /**
> * ufshcd_probe_hba - probe hba to detect device and initialize
> * @hba: per-adapter instance
> @@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
> pm_runtime_put_sync(hba->dev);
> ufshcd_exit_clk_scaling(hba);
> ufshcd_hba_exit(hba);
> + } else {
> + ufshcd_clear_ua_wluns(hba);
> }
> }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 363589c0bd370..8344d8cb36786 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -662,6 +662,7 @@ struct ufs_hba {
> * "UFS device" W-LU.
> */
> struct scsi_device *sdev_ufs_device;
> + struct scsi_device *sdev_rpmb;
>
> enum ufs_dev_pwr_mode curr_dev_pwr_mode;
> enum uic_link_state uic_link_state;
On 2020-10-20 18:51, Avri Altman wrote:
>>
>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>> > From: Jaegeuk Kim <[email protected]>
>> >
>> > This adds user-friendly tracepoints with group id.
> You have the entire cdb as part of the upiu trace,
> Can't you parse what you need from there?
>
> Thanks,
> Avri
Yes, but assume we have a large trace log file, having a
groud id allows us to filter the data by it easily, right?
Thanks,
Can Guo.
>
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <[email protected]>
> >
> > This adds user-friendly tracepoints with group id.
You have the entire cdb as part of the upiu trace,
Can't you parse what you need from there?
Thanks,
Avri
>
> On 2020-10-20 18:51, Avri Altman wrote:
> >>
> >> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> >> > From: Jaegeuk Kim <[email protected]>
> >> >
> >> > This adds user-friendly tracepoints with group id.
> > You have the entire cdb as part of the upiu trace,
> > Can't you parse what you need from there?
> >
> > Thanks,
> > Avri
>
> Yes, but assume we have a large trace log file, having a
> groud id allows us to filter the data by it easily, right?
>
Ahha, ok.
Thanks,
Avri
On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <[email protected]>
>
> This adds user-friendly tracepoints with group id.
>
> Cc: Alim Akhtar <[email protected]>
> Cc: Avri Altman <[email protected]>
> Cc: Can Guo <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
Reviewed-by: Can Guo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 6 ++++--
> include/trace/events/ufs.h | 21 +++++++++++++++++----
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 76e95963887be..a2db8182663da 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba
> *hba,
> unsigned int tag, const char *str)
> {
> sector_t lba = -1;
> - u8 opcode = 0;
> + u8 opcode = 0, group_id = 0;
> u32 intr, doorbell;
> struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> struct scsi_cmnd *cmd = lrbp->cmd;
> @@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct
> ufs_hba *hba,
> lba = cmd->request->bio->bi_iter.bi_sector;
> transfer_len = be32_to_cpu(
> lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
> + if (opcode == WRITE_10)
> + group_id = lrbp->cmd->cmnd[6];
> }
> }
>
> intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> trace_ufshcd_command(dev_name(hba->dev), str, tag,
> - doorbell, transfer_len, intr, lba, opcode);
> + doorbell, transfer_len, intr, lba, opcode, group_id);
> }
>
> static void ufshcd_print_clk_freqs(struct ufs_hba *hba)
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 84841b3a7ffd5..50654f3526392 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -11,6 +11,15 @@
>
> #include <linux/tracepoint.h>
>
> +#define str_opcode(opcode) \
> + __print_symbolic(opcode, \
> + { WRITE_16, "WRITE_16" }, \
> + { WRITE_10, "WRITE_10" }, \
> + { READ_16, "READ_16" }, \
> + { READ_10, "READ_10" }, \
> + { SYNCHRONIZE_CACHE, "SYNC" }, \
> + { UNMAP, "UNMAP" })
> +
> #define UFS_LINK_STATES \
> EM(UIC_LINK_OFF_STATE) \
> EM(UIC_LINK_ACTIVE_STATE) \
> @@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
> TRACE_EVENT(ufshcd_command,
> TP_PROTO(const char *dev_name, const char *str, unsigned int tag,
> u32 doorbell, int transfer_len, u32 intr, u64 lba,
> - u8 opcode),
> + u8 opcode, u8 group_id),
>
> - TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba,
> opcode),
> + TP_ARGS(dev_name, str, tag, doorbell, transfer_len,
> + intr, lba, opcode, group_id),
>
> TP_STRUCT__entry(
> __string(dev_name, dev_name)
> @@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command,
> __field(u32, intr)
> __field(u64, lba)
> __field(u8, opcode)
> + __field(u8, group_id)
> ),
>
> TP_fast_assign(
> @@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command,
> __entry->intr = intr;
> __entry->lba = lba;
> __entry->opcode = opcode;
> + __entry->group_id = group_id;
> ),
>
> TP_printk(
> - "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode:
> 0x%x",
> + "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode:
> 0x%x (%s), group_id: 0x%x",
> __get_str(str), __get_str(dev_name), __entry->tag,
> __entry->doorbell, __entry->transfer_len,
> - __entry->intr, __entry->lba, (u32)__entry->opcode
> + __entry->intr, __entry->lba, (u32)__entry->opcode,
> + str_opcode(__entry->opcode), (u32)__entry->group_id
> )
> );
On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <[email protected]>
>
> When giving a stress test which enables/disables clkgating, we hit
> device
> timeout sometimes. This patch avoids subtle racy condition to address
> it.
>
> Cc: Alim Akhtar <[email protected]>
> Cc: Avri Altman <[email protected]>
> Cc: Can Guo <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1d157ff58d817..d929c3d1e58cc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1791,19 +1791,19 @@ static ssize_t
> ufshcd_clkgate_enable_store(struct device *dev,
> return -EINVAL;
>
> value = !!value;
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> if (value == hba->clk_gating.is_enabled)
> goto out;
>
> - if (value) {
> - ufshcd_release(hba);
> - } else {
> - spin_lock_irqsave(hba->host->host_lock, flags);
> + if (value)
> + hba->clk_gating.active_reqs--;
> + else
> hba->clk_gating.active_reqs++;
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - }
>
> hba->clk_gating.is_enabled = value;
> out:
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> return count;
> }
I agree that we should protect the flag "is_enabled" with spin lock,
but I prefer the old logic of calling ufshcd_release() instead of
just doing hba->clk_gating.active_reqs--, you can use
__ufshcd_release(),
which is free of locking.
Thanks,
Can Guo.
On 2020-10-06 06:36, Jaegeuk Kim wrote:
> From: Jaegeuk Kim <[email protected]>
>
> Must have WQ_MEM_RECLAIM
> ``WQ_MEM_RECLAIM``
> All wq which might be used in the memory reclaim paths **MUST**
> have this flag set. The wq is guaranteed to have at least one
> execution context regardless of memory pressure.
>
The commit msg is not telling the same story as the change/title does.
Regards,
Can Guo.
> Cc: Alim Akhtar <[email protected]>
> Cc: Avri Altman <[email protected]>
> Cc: Can Guo <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0bb07b50bd23e..76e95963887be 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba
> *hba)
> snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
> hba->host->host_no);
> hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
> - WQ_MEM_RECLAIM);
> + WQ_MEM_RECLAIM | WQ_HIGHPRI);
>
> hba->clk_gating.is_enabled = true;
On 2020-10-20 19:02, Can Guo wrote:
> On 2020-10-20 18:51, Avri Altman wrote:
>>>
>>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>>> > From: Jaegeuk Kim <[email protected]>
>>> >
>>> > This adds user-friendly tracepoints with group id.
>> You have the entire cdb as part of the upiu trace,
>> Can't you parse what you need from there?
>>
>> Thanks,
>> Avri
>
> Yes, but assume we have a large trace log file, having a
> groud id allows us to filter the data by it easily, right?
>
> Thanks,
>
> Can Guo.
I just dobule checked WRITE(10)'s CDB, byte 6 has group
ID ONLY. So Avri is right, we don't even need to parse it,
we can easily filter a ftrace log file by byte 6 to get the
WRITE(10) cmds with specific group ID - we don't need this
change.
Thanks,
Can Guo.
On 2020-10-20 19:57, Can Guo wrote:
> On 2020-10-20 19:02, Can Guo wrote:
>> On 2020-10-20 18:51, Avri Altman wrote:
>>>>
>>>> On 2020-10-06 06:36, Jaegeuk Kim wrote:
>>>> > From: Jaegeuk Kim <[email protected]>
>>>> >
>>>> > This adds user-friendly tracepoints with group id.
>>> You have the entire cdb as part of the upiu trace,
>>> Can't you parse what you need from there?
>>>
>>> Thanks,
>>> Avri
>>
>> Yes, but assume we have a large trace log file, having a
>> groud id allows us to filter the data by it easily, right?
>>
>> Thanks,
>>
>> Can Guo.
>
> I just dobule checked WRITE(10)'s CDB, byte 6 has group
> ID ONLY. So Avri is right, we don't even need to parse it,
> we can easily filter a ftrace log file by byte 6 to get the
> WRITE(10) cmds with specific group ID - we don't need this
> change.
>
> Thanks,
>
> Can Guo.
Please ignore my previous mail, I misunderstood the change. :(
You have my reivewed-by tag for this change.
Regards,
Can Guo.
On 10/20, Can Guo wrote:
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <[email protected]>
> >
> > In order to conduct FFU or RPMB operations, UFS needs to clear UAC. This
> > patch
> > clears it explicitly, so that we could get no failure given early
> > execution.
> >
>
> Usually it is the user's/utility's/tool's responsiblity to clear UA by
> sending a
> request sense cmd and retry previous cmd, now we are doing it for the users
> in driver?
> As per my understanding, driver only reports UA to SCSI layer and let users
> decide
> what to do with it - maybe users need to do something specifically regs it,
> but
> the change clears it even before the users get to know it.
Well, instead of your expectation, many users have actually complained about why
they need to do this. Even, taking a look at some custom implementations given
by SoC branches, they simply clears it internally.
>
> Besides, this change clears UA for W-LUs, but the UFS driver still reports
> UA to SCSI
> layer for each SCSI device by calling scsi_report_bus_reset() in
> ufshcd_reset_and_restore(). This will make SCSI layer treat
> sdev->expecting_cc_ua
> wrongly, because for W-LUs, their expecting_cc_ua should not be set as you
> have
> cleared their UAs.
Is that fully restricted? What is the buggy scenario if sub-scsi clears it under
its control?
>
> Thanks,
>
> Can Guo.
>
> > Cc: Alim Akhtar <[email protected]>
> > Cc: Avri Altman <[email protected]>
> > Cc: Can Guo <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 70 +++++++++++++++++++++++++++++++++++----
> > drivers/scsi/ufs/ufshcd.h | 1 +
> > 2 files changed, 65 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index d929c3d1e58cc..0bb07b50bd23e 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6841,7 +6841,6 @@ static inline void
> > ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
> > static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
> > {
> > int ret = 0;
> > - struct scsi_device *sdev_rpmb;
> > struct scsi_device *sdev_boot;
> >
> > hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
> > @@ -6854,14 +6853,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> > *hba)
> > ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
> > scsi_device_put(hba->sdev_ufs_device);
> >
> > - sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> > + hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> > ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> > - if (IS_ERR(sdev_rpmb)) {
> > - ret = PTR_ERR(sdev_rpmb);
> > + if (IS_ERR(hba->sdev_rpmb)) {
> > + ret = PTR_ERR(hba->sdev_rpmb);
> > goto remove_sdev_ufs_device;
> > }
> > - ufshcd_blk_pm_runtime_init(sdev_rpmb);
> > - scsi_device_put(sdev_rpmb);
> > + ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> > + scsi_device_put(hba->sdev_rpmb);
> >
> > sdev_boot = __scsi_add_device(hba->host, 0, 0,
> > ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> > @@ -7385,6 +7384,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
> > return ret;
> > }
> >
> > +static int
> > +ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device
> > *sdp);
> > +
> > +static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
> > +{
> > + struct scsi_device *sdp;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
> > + sdp = hba->sdev_ufs_device;
> > + else if (wlun == UFS_UPIU_RPMB_WLUN)
> > + sdp = hba->sdev_rpmb;
> > + else
> > + BUG_ON(1);
> > + if (sdp) {
> > + ret = scsi_device_get(sdp);
> > + if (!ret && !scsi_device_online(sdp)) {
> > + ret = -ENODEV;
> > + scsi_device_put(sdp);
> > + }
> > + } else {
> > + ret = -ENODEV;
> > + }
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > + if (ret)
> > + goto out_err;
> > +
> > + ret = ufshcd_send_request_sense(hba, sdp);
> > + scsi_device_put(sdp);
> > +out_err:
> > + if (ret)
> > + dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
> > + __func__, wlun, ret);
> > + return ret;
> > +}
> > +
> > +static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
> > +{
> > + int ret = 0;
> > +
> > + if (!hba->wlun_dev_clr_ua)
> > + goto out;
> > +
> > + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
> > + if (!ret)
> > + ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> > + if (!ret)
> > + hba->wlun_dev_clr_ua = false;
> > +out:
> > + if (ret)
> > + dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
> > + __func__, ret);
> > + return ret;
> > +}
> > +
> > /**
> > * ufshcd_probe_hba - probe hba to detect device and initialize
> > * @hba: per-adapter instance
> > @@ -7500,6 +7556,8 @@ static void ufshcd_async_scan(void *data,
> > async_cookie_t cookie)
> > pm_runtime_put_sync(hba->dev);
> > ufshcd_exit_clk_scaling(hba);
> > ufshcd_hba_exit(hba);
> > + } else {
> > + ufshcd_clear_ua_wluns(hba);
> > }
> > }
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 363589c0bd370..8344d8cb36786 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -662,6 +662,7 @@ struct ufs_hba {
> > * "UFS device" W-LU.
> > */
> > struct scsi_device *sdev_ufs_device;
> > + struct scsi_device *sdev_rpmb;
> >
> > enum ufs_dev_pwr_mode curr_dev_pwr_mode;
> > enum uic_link_state uic_link_state;
On 10/20, Can Guo wrote:
> On 2020-10-06 06:36, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim <[email protected]>
> >
> > Must have WQ_MEM_RECLAIM
> > ``WQ_MEM_RECLAIM``
> > All wq which might be used in the memory reclaim paths **MUST**
> > have this flag set. The wq is guaranteed to have at least one
> > execution context regardless of memory pressure.
> >
>
> The commit msg is not telling the same story as the change/title does.
This message explains why we need to keep WQ_MEM_RECLAIM when adding WQ_HIGHPRI.
Thanks,
>
> Regards,
>
> Can Guo.
>
> > Cc: Alim Akhtar <[email protected]>
> > Cc: Avri Altman <[email protected]>
> > Cc: Can Guo <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 0bb07b50bd23e..76e95963887be 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -1849,7 +1849,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba
> > *hba)
> > snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
> > hba->host->host_no);
> > hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
> > - WQ_MEM_RECLAIM);
> > + WQ_MEM_RECLAIM | WQ_HIGHPRI);
> >
> > hba->clk_gating.is_enabled = true;