Re: [net-next v10 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 9490 bytes --]

Hi Krzysztof,

> On 02/05/2025 09:44, Lukasz Majewski wrote:
> > +
> > +static int mtip_parse_of(struct switch_enet_private *fep,
> > +			 struct device_node *np)
> > +{
> > +	struct device_node *p;
> > +	unsigned int port_num;
> > +	int ret = 0;
> > +
> > +	p = of_find_node_by_name(np, "ethernet-ports");  
> 
> This should be looking for children, not any nodes. Otherwise you will
> take the ethernet ports from a next device as well.

Yes, you are right - I will replace of_find_node_by_name() with
of_get_child_by_name()

> 
> > +
> > +	for_each_available_child_of_node_scoped(p, port) {
> > +		if (of_property_read_u32(port, "reg", &port_num))
> > +			continue;
> > +
> > +		if (port_num > SWITCH_EPORT_NUMBER) {
> > +			dev_err(&fep->pdev->dev,
> > +				"%s: The switch supports up to %d
> > ports!\n",
> > +				__func__, SWITCH_EPORT_NUMBER);
> > +			goto of_get_err;
> > +		}
> > +
> > +		fep->n_ports = port_num;
> > +		ret = of_get_mac_address(port, &fep->mac[port_num
> > - 1][0]);
> > +		if (ret)
> > +			dev_dbg(&fep->pdev->dev,
> > +				"of_get_mac_address(%pOF) failed
> > (%d)!\n",
> > +				port, ret);
> > +
> > +		ret = of_property_read_string(port, "label",
> > +
> > &fep->ndev_name[port_num - 1]);
> > +		if (ret < 0) {
> > +			dev_err(&fep->pdev->dev,
> > +				"%s: Cannot get ethernet port name
> > (%d)!\n",
> > +				__func__, ret);
> > +			goto of_get_err;
> > +		}
> > +
> > +		ret = of_get_phy_mode(port,
> > &fep->phy_interface[port_num - 1]);
> > +		if (ret < 0) {
> > +			dev_err(&fep->pdev->dev,
> > +				"%s: Cannot get PHY mode (%d)!\n",
> > __func__,
> > +				ret);
> > +			goto of_get_err;
> > +		}
> > +
> > +		fep->phy_np[port_num - 1] = of_parse_phandle(port,
> > +
> > "phy-handle", 0);
> > +	}
> > +
> > + of_get_err:
> > +	of_node_put(p);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtip_sw_learning(void *arg)
> > +{
> > +	struct switch_enet_private *fep = arg;
> > +
> > +	while (!kthread_should_stop()) {
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +		/* check learning record valid */
> > +		mtip_atable_dynamicms_learn_migration(fep,
> > fep->curr_time,
> > +						      NULL, NULL);
> > +		schedule_timeout(HZ / 100);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtip_mii_unregister(struct switch_enet_private *fep)
> > +{
> > +	mdiobus_unregister(fep->mii_bus);
> > +	mdiobus_free(fep->mii_bus);
> > +}
> > +
> > +static const struct mtip_devinfo mtip_imx28_l2switch_info = {
> > +	.quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO |
> > +		  FEC_QUIRK_SWAP_FRAME,
> > +};
> > +
> > +static const struct of_device_id mtipl2_of_match[] = {
> > +	{ .compatible = "nxp,imx28-mtip-switch",
> > +	  .data = &mtip_imx28_l2switch_info},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtipl2_of_match);
> > +
> > +static int mtip_sw_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	const struct of_device_id *of_id;
> > +	struct switch_enet_private *fep;
> > +	struct mtip_devinfo *dev_info;
> > +	int ret;
> > +
> > +	fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL);
> > +	if (!fep)
> > +		return -ENOMEM;
> > +
> > +	of_id = of_match_node(mtipl2_of_match, pdev->dev.of_node);
> > +	if (of_id) {
> > +		dev_info = (struct mtip_devinfo *)of_id->data;  
> 
> Do not open-code of_device_get_match_data().

+1

> 
> > +		if (dev_info)
> > +			fep->quirks = dev_info->quirks;
> > +	}
> > +
> > +	fep->pdev = pdev;
> > +	platform_set_drvdata(pdev, fep);
> > +
> > +	fep->enet_addr = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(fep->enet_addr))
> > +		return PTR_ERR(fep->enet_addr);
> > +
> > +	fep->irq = platform_get_irq_byname(pdev, "enet_switch");
> > +	if (fep->irq < 0)
> > +		return fep->irq;
> > +
> > +	ret = mtip_parse_of(fep, np);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "%s: OF parse error (%d)!\n",
> > __func__,
> > +			ret);  
> 
> Syntax is:
> return dev_err_probe
> just like you have in other places

Ok.

> 
> > +		return ret;
> > +	}
> > +
> > +	/* Create an Ethernet device instance.
> > +	 * The switch lookup address memory starts at 0x800FC000
> > +	 */
> > +	fep->hwp_enet = fep->enet_addr;
> > +	fep->hwp = fep->enet_addr + ENET_SWI_PHYS_ADDR_OFFSET;
> > +	fep->hwentry = (struct mtip_addr_table __iomem *)
> > +		(fep->hwp + MCF_ESW_LOOKUP_MEM_OFFSET);
> > +
> > +	ret = devm_regulator_get_enable_optional(&pdev->dev,
> > "phy");
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Unable to get and enable
> > 'phy'\n"); +
> > +	fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> > +	if (IS_ERR(fep->clk_ipg))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ipg),
> > +				     "Unable to acquire 'ipg'
> > clock\n"); +
> > +	fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> > +	if (IS_ERR(fep->clk_ahb))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ahb),
> > +				     "Unable to acquire 'ahb'
> > clock\n"); +
> > +	fep->clk_enet_out =
> > devm_clk_get_optional_enabled(&pdev->dev,
> > +
> > "enet_out");
> > +	if (IS_ERR(fep->clk_enet_out))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_enet_out),
> > +				     "Unable to acquire 'enet_out'
> > clock\n"); +
> > +	/* setup MII interface for external switch ports */
> > +	mtip_enet_init(fep, 1);
> > +	mtip_enet_init(fep, 2);
> > +
> > +	spin_lock_init(&fep->learn_lock);
> > +	spin_lock_init(&fep->hw_lock);
> > +	spin_lock_init(&fep->mii_lock);
> > +
> > +	ret = devm_request_irq(&pdev->dev, fep->irq,
> > mtip_interrupt, 0,
> > +			       dev_name(&pdev->dev), fep);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, fep->irq,
> > +				     "Could not alloc IRQ\n");
> > +
> > +	ret = mtip_register_notifiers(fep);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtip_ndev_init(fep, pdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Failed to create virtual
> > ndev (%d)\n",
> > +			__func__, ret);
> > +		goto ndev_init_err;
> > +	}
> > +
> > +	ret = mtip_switch_dma_init(fep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: ethernet switch init fail
> > (%d)!\n",
> > +			__func__, ret);
> > +		goto dma_init_err;
> > +	}
> > +
> > +	ret = mtip_mii_init(fep, pdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Cannot init phy bus
> > (%d)!\n", __func__,
> > +			ret);
> > +		goto mii_init_err;
> > +	}
> > +	/* setup timer for learning aging function */
> > +	timer_setup(&fep->timer_aging, mtip_aging_timer, 0);
> > +	mod_timer(&fep->timer_aging,
> > +		  jiffies +
> > msecs_to_jiffies(LEARNING_AGING_INTERVAL)); +
> > +	fep->task = kthread_run(mtip_sw_learning, fep,
> > "mtip_l2sw_learning");
> > +	if (IS_ERR(fep->task)) {
> > +		ret = PTR_ERR(fep->task);
> > +		dev_err(&pdev->dev, "%s: learning kthread_run
> > error (%d)!\n",
> > +			__func__, ret);  
> 
> ret = dev_err_probe

I will replace two above lines with:
ret = dev_err_probe(&pdev->dev, PTR_ERR(fep->task),
	"learning kthread_run error\n");


> 
> > +		goto task_learning_err;
> > +	}
> > +
> > +	return 0;
> > +
> > + task_learning_err:
> > +	timer_delete_sync(&fep->timer_aging);
> > +	mtip_mii_unregister(fep);
> > + mii_init_err:
> > + dma_init_err:
> > +	mtip_ndev_cleanup(fep);
> > + ndev_init_err:
> > +	mtip_unregister_notifiers(fep);
> > +
> > +	return ret;
> > +}
> > +
> > +static void mtip_sw_remove(struct platform_device *pdev)
> > +{
> > +	struct switch_enet_private *fep =
> > platform_get_drvdata(pdev); +
> > +	mtip_unregister_notifiers(fep);
> > +	mtip_ndev_cleanup(fep);
> > +
> > +	mtip_mii_remove(fep);
> > +
> > +	kthread_stop(fep->task);
> > +	timer_delete_sync(&fep->timer_aging);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	kfree(fep);  
> 
> Jakub already pointed out that tools would find this bug but also
> testing. This was not ever tested. If it was, you would see nice clear
> double free.
> 
> All last three versions had trivial issues which are pointed out by
> tooling, compilers, static analyzers. Before you post next version.
> please run standard kernel tools for static analysis, like coccinelle,
> smatch and sparse, and fix reported warnings. Also please check for
> warnings when building with W=1 with clang. Most of these commands
> (checks or W=1 build) can build specific targets, like some directory,
> to narrow the scope to only your code. The code here looks like it
> needs a fix. Feel free to get in touch if the warning is not clear.
> 

Ok.

> You do not nede the the top-level, one of the most busy maintainers to
> point out the issues which compilers find as well. Using reviewers
> instead of automated tools is the easiest way to get grumpy responses.
> 

Maybe I've striven to upstream the code too much recently as it takes
already 6 years (with 3 major rewrites of the code)...

Nonetheless, the code shall pass all required checks, so it is my
responsibility to fix it.

> 
> Best regards,
> Krzysztof




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]