Re: [PATCH v2 2/3] misc: tc9564: introduce base PCI driver

[Alex Elder <elder@riscstar.com>]

On 9/14/26 10:20 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] misc: missing of_platform_depopulate() on probe error path
> - [Low] kconfig: unused REGMAP_MMIO selection in TC9564_PCI
> --
> 
> commit 526cbf40d582cb34a1f4613745e5ca7890f929d2
> Author: Alex Elder <elder@riscstar.com>
> 
> misc: tc9564: introduce base PCI driver
> 
> This commit introduces a base PCI driver for the Toshiba TC9564 SoC, which
> implements a PCIe switch and Ethernet AVB/TSN bridge. It performs basic PCI
> initialization and uses of_platform_default_populate() to scan and probe
> child devices defined in the devicetree.
> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 7364931dad3a1..91950d2928f07 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -568,6 +568,16 @@ config MCHP_LAN966X_PCI
>>   	    - lan966x-miim (MDIO_MSCC_MIIM)
>>   	    - lan966x-switch (LAN966X_SWITCH)
>>   
>> +config TC9564_PCI
>> +	tristate "Toshiba TC9564 PCI function support"
>> +	depends on PCI
>> +	select REGMAP_MMIO
> 
> [Severity: Low]
> Is the REGMAP_MMIO selection required for this driver?
> 
> Looking at drivers/misc/tc9564-pci.c, it does not seem to include
> linux/regmap.h or use any regmap APIs. Could this lead to unnecessary
> kernel bloat if the regmap infrastructure isn't selected by other
> configurations?

This was an oversight when making the conversion to the endpoint
bus.  This driver no longer uses regmap, so that dependency will
be removed when version 3 is posted.

> 
> [ ... ]
>> diff --git a/drivers/misc/tc9564-pci.c b/drivers/misc/tc9564-pci.c
>> new file mode 100644
>> index 0000000000000..d7ebbd90d1584
>> --- /dev/null
>> +++ b/drivers/misc/tc9564-pci.c
> [ ... ]
>> +static int
>> +tc9564_function_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> +{
> [ ... ]
>> +	/* Scan for pci-ep-bus nodes and probe their sub-devices */
>> +	ret = of_platform_default_populate(np, NULL, dev);
>> +	if (ret)
>> +		goto err_clear_master;
>> +
>> +	return 0;
>> +
>> +err_clear_master:
>> +	pci_clear_master(pdev);
>> +
>> +	return dev_err_probe(dev, ret, "failed to populate platform bus\n");
>> +}
> 
> [Severity: High]
> Does this error path need to call of_platform_depopulate() before returning?

It looks like it does.  I now recognize that of_platform_populate()
(called by of_platform_default_populate()) does not unwind and clean
up its mess in the event of an error, so doing that is the caller's
responsibility.

I will fix this in version 3.

					-Alex

> 
> When of_platform_default_populate() fails midway, it leaves any successfully
> created child platform devices registered and sets the OF_POPULATED_BUS flag.
> If we only call pci_clear_master() and return an error here, devres will
> subsequently disable the PCI device completely via pcim_disable_device().
> 
> Since the orphaned child devices remain bound to their drivers, any later
> attempt by those drivers to perform MMIO accesses on the disabled PCI
> device could result in PCIe Unsupported Requests (UR), which can trigger a
> fatal SError/MCE on many architectures.
>