Re: [PATCH 1/3] scsi: ufs: ufs-qcom: Restore TX Equalization settings on FOM failure

[Can Guo]

On 6/23/2026 2:33 PM, Manivannan Sadhasivam wrote:
> On Sat, Jun 20, 2026 at 01:03:20AM -0700, Can Guo wrote:
> > ufs_qcom_get_rx_fom() applies temporary device TX Equalization values
> > before forcing HS mode and running the EOM-based SW FOM scan.
> >
> > When one of these steps fails, the function can bypass the shared
> > cleanup path and leave temporary TX Equalization settings programmed.
> >
> > Route those failures through the cleanup label so the original TX EQ
> > settings are restored and link recovery runs before exit.
> >
> > This path also reuses ret for cleanup, so it may overwrite the original
> > error. Keep that on purpose: if cleanup succeeds, the caller can proceed
> > with the FOM result for the current iteration.
> >
> > Signed-off-by: Can Guo <can.guo@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/ufs/host/ufs-qcom.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index c084ccc72523..7d7c001435bf 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -2791,7 +2791,7 @@ static int ufs_qcom_get_rx_fom(struct ufs_hba *hba,
> >   	if (ret) {
> >   		dev_err(hba->dev, "%s: Failed to apply TX EQ settings for HS-G%u: %d\n",
> >   			__func__, gear, ret);
> > -		return ret;
> > +		goto link_recover_and_restore;
> IIUC, if ufshcd_apply_tx_eq_settings() fails, then it means the TX EQ settings
> were not applied. So do we really need to restore the original TQ EQ here?
Hi Mani,

Thanks for the review.

In many cases yes, but not always. `ufshcd_apply_tx_eq_settings()` 
writes multiple
DME attributes, so a failure may happen after partial programming. 
Routing all failure
cases through a single cleanup path makes us always attempt restore and 
link recovery,
instead of exiting early with possibly transient settings.
> >   	}
> >   
> >   	/* Force PMC to target HS Gear to use new TX Equalization settings. */
> > @@ -2799,16 +2799,15 @@ static int ufs_qcom_get_rx_fom(struct ufs_hba *hba,
> >   	if (ret) {
> >   		dev_err(hba->dev, "%s: Failed to change power mode to HS-G%u, Rate-%s: %d\n",
> >   			__func__, gear, ufs_hs_rate_to_str(rate), ret);
> > -		return ret;
> > +		goto link_recover_and_restore;
> >   	}
> >   
> >   	ret = ufs_qcom_host_sw_rx_fom(hba, pwr_mode->lane_rx, fom);
> > -	if (ret) {
> > +	if (ret)
> >   		dev_err(hba->dev, "Failed to get SW FOM of TX (PreShoot: %u, DeEmphasis: %u): %d\n",
> >   			d_iter->preshoot, d_iter->deemphasis, ret);
> > -		return ret;
> > -	}
> >   
> > +link_recover_and_restore:
> >   	/* Restore Device's TX Equalization settings. */
> >   	ret = ufshcd_apply_tx_eq_settings(hba, &hba->tx_eq_params[gear - 1], gear);
> >   	if (ret) {
> 'ret' will be overridden here and '0' might be returned to the caller.
Correct, this is intentional for the TX EQTR sweep. This helper runs 
per-iteration, and I want a
failed sample to be treated as FOM=0 so the sweep can continue to the 
next point, rather than
aborting the full procedure. We only stop/abort TX EQTR sweep when 
cleanup/recovery itself fails.

Thanks,
Can Guo.
> - Mani