Re: [PATCH v2 05/17] ufs: core: mcq: Introduce Multi Circular Queue

From: Eddie Huang <eddie.huang@mediatek.com>
To: Asutosh Das <quic_asutoshd@quicinc.com>
Cc: <quic_cang@quicinc.com>, <quic_nitirawa@quicinc.com>,
	<quic_rampraka@quicinc.com>, <quic_bhaskarv@quicinc.com>,
	<quic_richardp@quicinc.com>, <linux-scsi@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>, <quic_nguyenb@quicinc.com>,
	<quic_xiaosenh@quicinc.com>, <bvanassche@acm.org>,
	<avri.altman@wdc.com>, <mani@kernel.org>, <beanhuo@micron.com>,
	<stanley.chu@mediatek.com>, <liang-yen.wang@mediatek.com>
Subject: Re: [PATCH v2 05/17] ufs: core: mcq: Introduce Multi Circular Queue
Date: Wed, 19 Oct 2022 18:28:30 +0800	[thread overview]
Message-ID: <052136dffaf0271e48d9ad5d7ade25805111ab27.camel@mediatek.com> (raw)
In-Reply-To: <20221018160048.GF10252@asutoshd-linux1.qualcomm.com>

Hi Asutosh,

On Tue, 2022-10-18 at 09:00 -0700, Asutosh Das wrote:
> On Tue, Oct 18 2022 at 22:29 -0700, Eddie Huang wrote:
> [...]
> > > ---
> > >  drivers/ufs/core/Makefile      |   2 +-
> > >  drivers/ufs/core/ufs-mcq.c     | 113
> > > +++++++++++++++++++++++++++++++++++++++++
> 
> [...]
> > >  create mode 100644 drivers/ufs/core/ufs-mcq.c
> > > 
> > 
> > [...]
> > 
> > >  /**
> > >   * ufshcd_probe_hba - probe hba to detect device and initialize
> > > it
> > >   * @hba: per-adapter instance
> > > @@ -8224,6 +8233,9 @@ static int ufshcd_probe_hba(struct ufs_hba
> > > *hba, bool init_dev_params)
> > >  			goto out;
> > > 
> > >  		if (is_mcq_supported(hba)) {
> > > +			ret = ufshcd_config_mcq(hba);
> 
> [...]
> 
> > 
> > ufshcd_probe_hba() may be called multiple times (from
> > ufshcd_async_scan() and ufshcd_host_reset_and_restore()). It is not
> > a
> > good idea to allocate memory in ufshcd_config_mcq(). Although use
> > parameter init_dev_params to decide call ufshcd_config_mcq() or
> > not, it
> > may cause ufshcd_host_reset_and_restore() not to configure MCQ
> > (init
> > SQ/CQ ptr...) again.
> > 
> 
> I don't think the memory allocation can be moved prior to reading the
> device
> descriptor since the bQueueDepth is necessary.
> But I agree to your point that ufshcd_host_reset_and_restore()
> wouldn't
> reconfigure MCQ now. Thanks.
> 
> > Suggest to separate configure MCQ (set hardware register) and
> > allocate
> > memory to different function
> > 
> 
> How about I keep the memory allocation in ufshcd_probe_hba() within
> the
> init_dev_params check and separate out the initialization outside the
> check.
> That'd ensure that the configuration is done for each call to
> ufshcd_probe_hba(). I'm open to any other idea that you may have,
> plmk.
> > 

Sounds good to me. Please go ahead to make the modification

Thanks
Eddie Huang