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

[Manivannan Sadhasivam]

On Tue, Mar 25, 2025 at 02:32:58PM +0530, MANISH PANDEY wrote:
> 
> 
> 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.

Even though usleep() allows the scheduler to reschedule other tasks, both are
not the same. usleep() puts the thread to sleep until the elapsed time and other
tasks may be scheduled in the meantime. But cond_resched() will allow the
scheduler to schedule other tasks *only* if necessary. So the scheduler may
decide to continue executing the current thread if it is well within its
timeslice.

> 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().
> 

This doesn't make sense to me. Why do you want to provide a fixed delay? Will
that delay affect the UFS controller by any means? If not (afaik), you should
use cond_resched() as that will allow for faster dumps and also not hog the CPU.

- Mani

-- 
மணிவண்ணன் சதாசிவம்