Re: [PATCH v3 07/10] scsi: ufs: Add NXP S32N79 UFS host controller driver

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Sep 16, 2026 at 09:00:09AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] pm: positive return in ufs_s32n7_link_startup_notify() causes unclocked access
> - [Low] pm: missing pm_sleep_ptr() wrapper for .prepare and .complete callbacks
> --
>
> commit 774c9ff68bb8216bae4f82205ba8ce70b0364e59
> Author: Larisa Grigore <larisa.grigore@oss.nxp.com>
>
> scsi: ufs: Add NXP S32N79 UFS host controller driver
>
> This commit adds support for the UFS host controller on the NXP S32N79 SoC,
> built on the Synopsys DesignWare (DWC) UFS architecture. It implements
> S32N79-specific initialization sequences before link startup and applies
> post-link calibration for HS operation.
>
> > diff --git a/drivers/ufs/host/ufs-nxp-s32n7.c b/drivers/ufs/host/ufs-nxp-s32n7.c
> > new file mode 100644
> > index 0000000000000..4f9a001562024
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-nxp-s32n7.c
>
> [ ... ]
>
> > +static int ufs_s32n7_link_startup_notify(struct ufs_hba *hba,
> > +			     enum ufs_notify_change_status status)
> > +{
> > +	int err;
> > +
> > +	if (status == PRE_CHANGE) {
> > +		err = ufs_s32n7_link_startup_pre_change(hba);
> > +		if (err) {
> > +			dev_err(hba->dev, "MPHY setup failed (%d).\n", err);
> > +			return err;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	/* POST_CHANGE */
> > +	err = ufshcd_dwc_link_is_up(hba);
> > +	if (err) {
> > +		dev_err(hba->dev, "Link is not up.\n");
> > +		return err;
>
> [Severity: High]
> Could this propagate a positive return value on failure?
>
> When ufshcd_dwc_link_is_up() fails, it returns 1. If this positive value is
> returned here, it will propagate up through ufshcd_link_startup() and
> ufshcd_resume() to ufshcd_runtime_resume().
>
> The PM core's rpm_resume() will treat the positive return value as a failure,
> marking the device as RPM_SUSPENDED, but will propagate the positive 1
> back to callers of pm_runtime_get_sync().
>
> Since callers typically check for failure using a ret < 0 condition, they
> will assume the device successfully resumed and proceed to access unclocked
> or powered-off hardware registers. This can cause a system crash or bus hang.

look likes make sense, please check.

Frank

>
> > +	}
> > +
> > +	err = ufs_s32n7_link_startup_post_change(hba);
>
> [ ... ]
>
> > +static const struct dev_pm_ops ufs_s32n7_pm_ops = {
> > +	SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
> > +	RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
> > +	.prepare	= ufshcd_suspend_prepare,
> > +	.complete	= ufshcd_resume_complete,
>
> [Severity: Low]
> Should these sleep callbacks be wrapped with pm_sleep_ptr()?
>
> The power management subsystem guidelines require sleep callbacks like
> .prepare and .complete to be wrapped in pm_sleep_ptr() to prevent them
> from becoming dead code when CONFIG_PM_SLEEP is disabled.
>
> Assigning them unconditionally can cause minor memory bloat and potential
> compiler warnings due to unused symbols being retained.
>
> > +};
> > +
> > +static struct platform_driver ufs_s32n7_driver = {
>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260916084411.941297-1-larisa.grigore@oss.nxp.com?part=7