Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, Jul 08, 2024 at 10:10:43AM -0700, Bart Van Assche wrote:
> On 7/8/24 3:44 AM, Manivannan Sadhasivam wrote:
> > On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote:
> > > On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote:
> > > > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote:
> > > > > If an UFSHCI controller is reset, the controller is reset from MCQ mode
> > > > > to SDB mode and it is derived from the hba->mcq_enabled structure member
> > > > > that MCQ was enabled before the reset. In other words, moving all
> > > > > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would
> > > > > break the code that resets the UFSHCI controller.
> > > > 
> > > > Hmm, could you please point me to the code that does this? I tried looking for
> > > > it but couldn't spot.
> > > 
> > > * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS
> > >    driver core. This shows that the reset code does not reset
> > >    hba->mcq_enabled.
> > 
> > Right. So this flag is used to reconfigure the MCQ mode upon HCI reset.
> > Previously we never disabled MCQ mode as well. But that is being changed by this
> > patch.
> 
> Only in an error path.
> 
> > > * From the UFSHCI specification, offset 300h, global config register:
> > >    in the "reset" column I see 0h for the queue type (QT) bit. In other
> > >    words, if a UFS host controller is reset, if MCQ is enabled (QT=1),
> > >    it must be disabled (QT=0) until the application processor enables it
> > >    again.
> > > 
> > 
> > So this means, once the HCI is reset, the QT field will become '0' by default.
> > So if MCQ mode is supposed to be enabled, then driver has to explicitly enable
> > it.
> > 
> > But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this
> > patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to
> > understand why you cannot move the assignment to the enable/disable API itself.
> > 
> > If you do not set the flag after calling the ufshcd_mcq_disable() API, your
> > argument makes sense. But that's not the case. Perhaps I'm missing anything
> > obvious?
> 
> What do you want? That I move the "hba->mcq_enabled = false;" statement
> into ufshcd_mcq_disable()? That would be wrong because (a) it would
> introduce a confusing behavior difference between ufshcd_mcq_enable()
> (does not modify hba->mcq_enabled) and ufshcd_mcq_disable() (would
> modify hba->mcq_enabled if I implement your proposal) and (b) wouldn't
> reduce the code size because ufshcd_mcq_disable() only has one caller.
> 

No, I'm not asking to move the assignment inside just the ufshcd_mcq_disable()
API, but for both. You are saying that doing so will break UFSHCI reset, but I
fail to understand how.

After your series applied, 'hba->mcq_enabled' is set to true in 2 places of
ufshcd.c. And in those 2 places, ufshcd_mcq_enable() is accompanied. There is
only one place you haven't added the assignment which is during the start of
ufshcd_device_init()::

        /* Reconfigure MCQ upon reset */
        if (hba->mcq_enabled && !init_dev_params) {
                ufshcd_config_mcq(hba);
                ufshcd_mcq_enable(hba);
        }

And this makes sense because, if mcq_enabled was already set to true, then you
are not setting the flag again. But even if you have moved the 'hba->mcq_enabled
= true' assignment inside ufshcd_mcq_enable(), it wouldn't cause any issues.

Where does the assignment actually breaking the reset code? This part I don't
understand.

- Mani

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