Re: [PATCH v2 1/5] ufs: mcq: Add supporting functions for mcq abort

From: Bart Van Assche
Date: Tue Apr 25 2023 - 20:04:22 EST


On 4/17/23 14:05, Bao D. Nguyen wrote:
+/* Max mcq register polling time in milisecond unit */

A nit: please change "millisecond unit" into "milliseconds".

+static int ufshcd_mcq_poll_register(void __iomem *reg, u32 mask,
+ u32 val, unsigned long timeout_ms)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
+ int err = 0;
+
+ /* ignore bits that we don't intend to wait on */
+ val = val & mask;
+
+ while ((readl(reg) & mask) != val) {

& has a higher precedence than != so one pair of parentheses can be left out.

+ udelay(20);
+ if (time_after(jiffies, timeout)) {

Please use time_is_before_jiffies() instead of time_after(jiffies, ...).

+ err = -ETIMEDOUT;
+ break;
+ }
+ }
+
+ return err;
+}

Please remove the variable 'err' and return the return value directly.

+
+static int ufshcd_mcq_sq_stop(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
+{
+ void __iomem *reg;
+ u32 i = hwq->id;

Please use another variable name than 'i' for a hardware queue ID ('id'?).

+ u32 i = hwq->id;

Same comment here.

+/**
+ * ufshcd_mcq_sq_cleanup - Clean up Submission Queue resources

A nit: please use lower case text for "submission queue" and also in the comments below ("Clean up" -> "clean up").

+ spin_lock(&hwq->sq_lock);
+
+ /* stop the SQ fetching before working on it */
+ err = ufshcd_mcq_sq_stop(hba, hwq);
+ if (err)
+ goto unlock;

No spin locks around delay loops please. Is there anything that prevents to change sq_lock from a spin lock into a mutex?

+static u64 ufshcd_mcq_get_cmd_desc_addr(struct ufs_hba *hba,
+ int task_tag)
+{
+ struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
+ __le32 hi = lrbp->utr_descriptor_ptr->command_desc_base_addr_hi;
+ __le32 lo = lrbp->utr_descriptor_ptr->command_desc_base_addr_lo;
+
+ return le64_to_cpu((__le64)hi << 32 | lo);
+}

Please add a new patch at the head of this series that modifies struct utp_transfer_req_desc such that command_desc_base_addr_lo and command_desc_base_addr_hi are combined into a single __le64 variable.

+/**
+ * ufshcd_mcq_nullify_cmd - Nullify utrd. Host controller does not fetch
+ * transfer with Command Type = 0xF. post the Completion Queue with OCS=ABORTED.
+ * @hba - per adapter instance.
+ * @hwq - Hardware Queue of the nullified utrd.
+ */
+static void ufshcd_mcq_nullify_cmd(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
+{
+ struct utp_transfer_req_desc *utrd;
+ u32 dword_0;
+
+ utrd = (struct utp_transfer_req_desc *)(hwq->sqe_base_addr +
+ hwq->id * sizeof(struct utp_transfer_req_desc));

Please double check this function. It has "cmd" in the function name but none of the arguments passed to this function allows to uniquely identify a command. Is an argument perhaps missing from this function?

Additionally, hwq->sqe_base_addr points to an array of SQE entries. I do not understand why hwq->id * sizeof(struct utp_transfer_req_desc) is added to that base address. Please clarify.

+ utrd = (struct utp_transfer_req_desc *)(hwq->sqe_base_addr +
+ sq_head_slot * sizeof(struct utp_transfer_req_desc));

hwq->sqe_base_addr already has type struct utp_transfer_req_desc * so the " * sizeof(struct utp_transfer_req_desc)" part looks wrong to me.

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 35a3bd9..808387c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -56,7 +56,6 @@
#define NOP_OUT_RETRIES 10
/* Timeout after 50 msecs if NOP OUT hangs without response */
#define NOP_OUT_TIMEOUT 50 /* msecs */
-
/* Query request retries */
#define QUERY_REQ_RETRIES 3
/* Query request timeout */

Is the above change really necessary?

@@ -173,7 +172,6 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
enum {
UFSHCD_MAX_CHANNEL = 0,
UFSHCD_MAX_ID = 1,
- UFSHCD_NUM_RESERVED = 1,
UFSHCD_CMD_PER_LUN = 32 - UFSHCD_NUM_RESERVED,
UFSHCD_CAN_QUEUE = 32 - UFSHCD_NUM_RESERVED,
};

Same question here - is this change really necessary?

Thanks,

Bart.