Re: [PATCH RFC v6 2/7] mfd: ls2kbmc: Sanity check for the connected pci port

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The global `mfd_cell` array `ls2k_bmc_cells` is dynamically mutated during probe with a stack-allocated variable pointer, violating MFD rules and causing a data race.
- [High] Missing cancellation of `bmc_reset_work` on device unbind or probe failure leads to Use-After-Free.
- [High] The `stop_machine()` callback performs highly illegal blocking operations, including a 10-second busy wait and dynamic memory mapping, leading to a system freeze.
--

--- Patch [2]: [PATCH RFC v6 2/7] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit 69486e003cb1f9b1540028b60b63bfa488d9ed41
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Sanity check for the connected pci port

This commit adds a sanity check to ensure the parent device of the BMC
is an LS7A port during initialization. This prevents the driver from
accidentally operating on non-LS7A ports when attempting recovery
procedures.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index fc9695eedd366..a9a30611dde9e 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

> @@ -488,6 +515,11 @@ static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (ret)
>  		return ret;
>  
> +	if (!ls2k_check_parent(dev)) {
> +		dev_err(&dev->dev, "Expected to be connected to LS7A PCI-E port\n");
> +		return -ENODEV;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this probe
function I noticed that it dynamically mutates a global mfd_cell array:

ls2k_bmc_probe() {
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
    ...
}

Could this cause a dangling pointer and data race? When the probe function
returns, the local stack variable pd goes out of scope, leaving a dangling
pointer in the global state. This also appears to violate the MFD subsystem
guidelines which require mfd_cell arrays to be static const.

[Severity: High]
This is also a pre-existing issue, but since the commit message mentions
the recovery procedure, I wanted to ask about the reset implementation.
The driver executes ls2k_bmc_recover_pci_data() inside stop_machine() via
ls2k_bmc_events_fn():

ls2k_bmc_events_fn() {
    ...
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
    ...
}

Inside ls2k_bmc_recover_pci_data(), the code calls mdelay() and pci_iomap()
which can sleep:

ls2k_bmc_recover_pci_data() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

Since stop_machine() executes its callback in an atomic context with local
IRQs disabled, wouldn't these blocking and sleeping operations cause a
deadlock or trigger the NMI watchdog?

>  	ddata = devm_kzalloc(&dev->dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2