| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4c3648fb-f41c-d097-cd38-f2efcee9ec71@quicinc.com>
Date: Wed, 3 May 2023 20:53:38 -0700
From: "Bao D. Nguyen" <quic_nguyenb@...cinc.com>
To: Bart Van Assche <bvanassche@....org>, <quic_asutoshd@...cinc.com>,
<quic_cang@...cinc.com>, <mani@...nel.org>,
<Powen.Kao@...iatek.com>, <stanley.chu@...iatek.com>,
<adrian.hunter@...el.com>, <beanhuo@...ron.com>,
<avri.altman@....com>, <martin.petersen@...cle.com>
CC: <linux-scsi@...r.kernel.org>,
Alim Akhtar <alim.akhtar@...sung.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
Arthur Simchaev <Arthur.Simchaev@....com>,
Eric Biggers <ebiggers@...gle.com>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] ufs: mcq: Add supporting functions for mcq abort
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.
Powered by blists - more mailing lists