Re: [PATCH V6 2/3] scsi: ufs-qcom: Add support to dump MCQ registers

[Bart Van Assche <bvanassche@acm.org>]

On 4/7/25 7:21 AM, Manish Pandey wrote:
> +int ufs_qcom_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
> +		     const char *prefix, enum ufshcd_res id)
> +{
> +	u32 *regs __free(kfree) = NULL;
> +	size_t pos;
> +
> +	if (offset % 4 != 0 || len % 4 != 0)
> +		return -EINVAL;
> +
> +	regs = kzalloc(len, GFP_ATOMIC);
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	for (pos = 0; pos < len; pos += 4)
> +		regs[pos / 4] = readl(hba->res[id].base + offset + pos);
> +
> +	print_hex_dump(KERN_ERR, prefix,
> +			len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,
> +				16, 4, regs, len, false);

The indentation of the print_hex_dump() arguments is not compliant with
the Linux kernel coding style.

> +	return 0;
> +}
> +
> +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
> +{
> +	/* voluntarily yield the CPU to prevent CPU hog during data dumps */
> +	/* RES_MCQ_1 */
> +	ufs_qcom_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0", RES_MCQ);
> +	cond_resched();
> +
> +	/* RES_MCQ_2 */
> +	ufs_qcom_dump_regs(hba, 0x400, 256 * 4, "MCQ HCI 1da0400-1da07f0", RES_MCQ);
> +	cond_resched();
> +
> +	/*RES_MCQ_VS */
> +	ufs_qcom_dump_regs(hba, 0x0, 5 * 4, "MCQ VS 1da4000-1da4010", RES_MCQ_VS);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_1 */
> +	ufs_qcom_dump_regs(hba, 0x0, 256 * 4, "MCQ SQD 1da5000-1da53f0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_2 */
> +	ufs_qcom_dump_regs(hba, 0x400, 256 * 4, "MCQ SQD 1da5400-1da57f0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_3 */
> +	ufs_qcom_dump_regs(hba, 0x800, 256 * 4, "MCQ SQD 1da5800-1da5bf0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_4 */
> +	ufs_qcom_dump_regs(hba, 0xc00, 256 * 4, "MCQ SQD 1da5c00-1da5ff0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_5 */
> +	ufs_qcom_dump_regs(hba, 0x1000, 256 * 4, "MCQ SQD 1da6000-1da63f0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_6 */
> +	ufs_qcom_dump_regs(hba, 0x1400, 256 * 4, "MCQ SQD 1da6400-1da67f0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_7 */
> +	ufs_qcom_dump_regs(hba, 0x1800, 256 * 4, "MCQ SQD 1da6800-1da6bf0", RES_MCQ_SQD);
> +	cond_resched();
> +
> +	/* RES_MCQ_SQD_8 */
> +	ufs_qcom_dump_regs(hba, 0x1c00, 256 * 4, "MCQ SQD 1da6c00-1da6ff0", RES_MCQ_SQD);
> +	cond_resched();
> +}

There is a lot of repetition in the ufs_qcom_dump_mcq_hci_regs()
function. Has it been considered to move the cond_resched() call into
ufs_qcom_dump_regs() such that it occurs only once in this patch?

For the ufs_qcom_dump_mcq_hci_regs() function, a table-based approach
may be appropriate since what that function does is to call
ufs_qcom_dump_regs() repeatedly but each time with different arguments.

Thanks,

Bart.