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

[sashiko-bot@kernel.org]

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.

> +	}
> +
> +	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