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

[Bart Van Assche]

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.