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 12:00 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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.
> 

Will address it in V4.

>> +     }
>> +
>> +     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.
> 
ufshcd_suspend_prepare and ufshcd_resume_prepare are not guarded by 
CONFIG_PM_SLEEP so this is a false positive.

>> +};
>> +
>> +static struct platform_driver ufs_s32n7_driver = {
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260916084411.941297-1-larisa.grigore@oss.nxp.com?part=7