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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bart,
Thank you so much for a detailed code review.

On 4/25/2023 5:04 PM, Bart Van Assche wrote:
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".
Yes I will change.


+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.
I think it is is actually the other way. & has lower precedence than !=.
Please correct me if I am wrong.


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

Please use time_is_before_jiffies() instead of time_after(jiffies, ...).
time_is_before_jiffies() seems to be defined as time_after(). Could you please explain the benefits to choose one over the other?


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

Please remove the variable 'err' and return the return value directly.
Yes I will change.

+
+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'?).
Yes I will change.


+    u32 i = hwq->id;

Same comment here.
Yes I will change.


+/**
+ * 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").
The UFS Host Controller specification uses upper case for the Submission Queue and Completion Queue, so I tried to follow the the spec language. I don't have a preference. I will make the change.


+    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?
This function can be called from multiple non-interrupt contexts. I needed to prevent concurrent accesses to the sq hw, so yes mutex would work better. I will change.


+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.
Yes, I will add this as a separate patch.


+/**
+ * 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?
Yes, I will make the correction to this function and rename it to ufshcd_mcq_nullify_sqe()


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.
Yes, I will correct this.


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?
The blank line was removed by mistake. I will put it back.

@@ -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?
I am moving the definition of UFSHCD_NUM_RESERVED to include/ufs/ufshci.h file so that I can access it from /core/ufs-mcq.c


Thanks,

Bart.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux