* Re: [PATCH] ufs: host: qcom: Enable MCQ support for sa8255p
2026-07-28 5:11 [PATCH] ufs: host: qcom: Enable MCQ support for sa8255p Shawn Guo
@ 2026-07-28 5:30 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-28 5:30 UTC (permalink / raw)
To: Shawn Guo; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Calling `platform_device_msi_free_irqs_all()` during device removal before `devres` frees the IRQ actions leads to a leak of `struct irqaction`.
- [High] Missing cleanup of MSI IRQs on the probe error path leads to a memory leak and a devres WARN_ON_ONCE.
- [High] MCQ ESI configuration is permanently lost across device resets and suspends because `ufs_qcom_config_esi` skips register programming if `host->esi_enabled` is already true.
- [High] The use of a static local variable `is_boot` in `ufs_qcom_fw_managed_device_reset` causes incorrect reset behavior if multiple UFS host instances exist or if the module is reloaded.
--
--- Patch [1]: [PATCH] ufs: host: qcom: Enable MCQ support for sa8255p ---
commit 7cb2953fc60985aba4ef6bffc3f9dc05ef977551
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>
ufs: host: qcom: Enable MCQ support for sa8255p
The sa8255p vops (fw-managed variant) was missing the MCQ hooks, causing
the driver to silently fall back to SDB mode even when MCQ resource is
available. Wire up the same MCQ ops used by the standard qcom vops,
so that MCQ mode works on sa8255p.
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 0500320545c43..d2acbbe647cb4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -2950,6 +2950,11 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_sa8255p_vops = {
> .resume = ufs_qcom_fw_managed_resume,
> .dbg_register_dump = ufs_qcom_dump_dbg_regs,
> .device_reset = ufs_qcom_fw_managed_device_reset,
[Severity: High]
This is a pre-existing issue, but does sharing a static variable across all
controller instances cause problems here?
In ufs_qcom_fw_managed_device_reset(), a static local variable is used to
track the first cold boot:
ufs_qcom_fw_managed_device_reset() {
static bool is_boot = true;
...
}
If there are multiple firmware-managed UFS controllers, or if the driver is
unbound and re-bound, won't subsequent initializations see is_boot as false
and trigger an unwanted device reset during their cold boot?
> + .mcq_config_resource = ufs_qcom_mcq_config_resource,
> + .get_hba_mac = ufs_qcom_get_hba_mac,
> + .op_runtime_config = ufs_qcom_op_runtime_config,
> + .get_outstanding_cqs = ufs_qcom_get_outstanding_cqs,
> + .config_esi = ufs_qcom_config_esi,
[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
ufs_qcom_config_esi() prevent ESI configuration from being restored after a
suspend or reset?
ufs_qcom_config_esi() has this check:
if (host->esi_enabled)
return 0;
When the device undergoes a reset or suspend, the controller's power is
toggled, clearing hardware registers like REG_UFS_CFG3. During resume, when
ufshcd_reset_and_restore() calls ufshcd_config_mcq(), which then calls
ufs_qcom_config_esi(), host->esi_enabled will still be true.
Will this skip reprogramming the hardware registers and leave ESI interrupts
broken?
[Severity: High]
This is also a pre-existing issue, but is there a memory leak if probe fails
after allocating MSIs?
ufs_qcom_config_esi() allocates MSI vectors via
platform_device_msi_init_and_alloc_irqs(). This API relies on devres to
release domain data but explicitly requires the caller to free the allocated
IRQs before the devres unwind phase.
If ufshcd_init() encounters an error later (for example, link startup fails)
and returns an error, the core and variant exit hooks don't call
platform_device_msi_free_irqs_all().
Will the devres teardown find the MSI xarray non-empty, triggering a
WARN_ON_ONCE(!xa_empty(&md->__macq)) and leaking resources?
[Severity: High]
Another pre-existing issue related to this: does driver unbind leak the
irqaction structures?
ufs_qcom_config_esi() uses devm_request_irq() to bind IRQ actions to devres.
During driver unbind, ufs_qcom_remove() manually calls
platform_device_msi_free_irqs_all(), which destroys the MSI descriptors and
IRQ mappings.
When devres unwinds after remove() returns, devm_irq_release() calls
free_irq(). Since the IRQ mapping was already destroyed, won't free_irq()
fail to find the descriptor and return early, leaking the irqaction?
> };
>
> /**
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260728051104.675088-1-shengchao.guo@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread