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

[Larisa Ileana Grigore <larisa.grigore@oss.nxp.com>]

On 9/16/2026 7:36 PM, Frank Li wrote:
> 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
> 

Yes, I will create a new commit to address this issue. Thanks!

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