Re: [PATCH V3 2/3] scsi: ufs-qcom: Add support for dumping MCQ registers

[MANISH PANDEY]

On 3/24/2025 1:09 PM, Manivannan Sadhasivam wrote:
> On Wed, Mar 19, 2025 at 11:51:07AM +0530, MANISH PANDEY wrote:
> > On 3/18/2025 12:14 PM, Manivannan Sadhasivam wrote:
> > > On Thu, Mar 13, 2025 at 10:46:34AM +0530, Manish Pandey wrote:
> > > > This patch adds functionality to dump MCQ registers.
> > > > This will help in diagnosing issues related to MCQ
> > > > operations by providing detailed register dumps.
> > >
> > > Same comment as previous patch. Also, make use of 75 column width.
> > will Update in next patch set.>> Signed-off-by: Manish Pandey
> > <quic_mapa@xxxxxxxxxxx>
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - Addressed Bart's review comments by adding explanations for the
> > > >     in_task() and usleep_range() calls.
> > > > Changes in v2:
> > > > - Rebased patchsets.
> > > > - Link to v1: https://lore.kernel.org/linux-arm-msm/20241025055054.23170-1-quic_mapa@xxxxxxxxxxx/
> > > > ---
> > > >    drivers/ufs/host/ufs-qcom.c | 60 +++++++++++++++++++++++++++++++++++++
> > > >    drivers/ufs/host/ufs-qcom.h |  2 ++
> > > >    2 files changed, 62 insertions(+)
> > > >
> > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > index f5181773c0e5..fb9da04c0d35 100644
> > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > @@ -1566,6 +1566,54 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
> > > >    	return 0;
> > > >    }
> > > > +static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
> > > > +{
> > > > +	/* sleep intermittently to prevent CPU hog during data dumps. */
> > > > +	/* RES_MCQ_1 */
> > > > +	ufshcd_dump_regs(hba, 0x0, 256 * 4, "MCQ HCI 1da0000-1da03f0 ");
> > > > +	usleep_range(1000, 1100);
> > >
> > > If your motivation is just to not hog the CPU, use cond_resched().
> > >
> > > - Mani
> > The intention here is to introduce a specific delay between each dump.
>
> What is the reason for that?
>
> > Therefore, i would like to use usleep_range() instead of cond_resched().
> > Please let me know if i am getting it wrong..
>
> Without knowing the reason, I cannot judge. Your comment said that you do not
> want to hog the CPU during dump. But now you are saying that you wanted to have
> a delay. Both are contradictions.
>
> - Mani
Hi Mani, Could you please clarify what you meant by delay? Did you mean 
udelay? That's not the case here, as we are using usleep(), which is 
similar to cond_resched(). I believe both serve the same purpose in this 
case. Therefore, I chose usleep() to provide a fixed delay between dumps 
since we are dumping a large amount of data. Additionally, I wanted to 
avoid any extra scheduling latency associated with cond_resched().

How ever i am open to change it to cond_resched() if needed.
Please suggest.

Regards
Manish