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

Lukasz Majewski
2025-05-04T10:49:54Z

[-- 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 --]