| Message ID | 20230714094432.1834489-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
On Fri, 2023-07-14 at 15:14 +0530, MD Danish Anwar wrote: > +static int prueth_netdev_init(struct prueth *prueth, > + struct device_node *eth_node) > +{ > + int ret, num_tx_chn = PRUETH_MAX_TX_QUEUES; > + struct prueth_emac *emac; > + struct net_device *ndev; > + enum prueth_port port; > + enum prueth_mac mac; > + > + port = prueth_node_port(eth_node); > + if (port == PRUETH_PORT_INVALID) > + return -EINVAL; > + > + mac = prueth_node_mac(eth_node); > + if (mac == PRUETH_MAC_INVALID) > + return -EINVAL; > + > + ndev = alloc_etherdev_mq(sizeof(*emac), num_tx_chn); > + if (!ndev) > + return -ENOMEM; > + > + emac = netdev_priv(ndev); > + prueth->emac[mac] = emac; > + emac->prueth = prueth; > + emac->ndev = ndev; > + emac->port_id = port; > + emac->cmd_wq = create_singlethread_workqueue("icssg_cmd_wq"); > + if (!emac->cmd_wq) { > + ret = -ENOMEM; > + goto free_ndev; > + } > + INIT_WORK(&emac->rx_mode_work, emac_ndo_set_rx_mode_work); > + > + ret = pruss_request_mem_region(prueth->pruss, > + port == PRUETH_PORT_MII0 ? > + PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1, > + &emac->dram); > + if (ret) { > + dev_err(prueth->dev, "unable to get DRAM: %d\n", ret); > + ret = -ENOMEM; > + goto free_wq; > + } > + > + emac->tx_ch_num = 1; > + > + SET_NETDEV_DEV(ndev, prueth->dev); > + spin_lock_init(&emac->lock); > + mutex_init(&emac->cmd_lock); > + > + emac->phy_node = of_parse_phandle(eth_node, "phy-handle", 0); > + if (!emac->phy_node && !of_phy_is_fixed_link(eth_node)) { > + dev_err(prueth->dev, "couldn't find phy-handle\n"); > + ret = -ENODEV; > + goto free; > + } else if (of_phy_is_fixed_link(eth_node)) { > + ret = of_phy_register_fixed_link(eth_node); > + if (ret) { > + ret = dev_err_probe(prueth->dev, ret, > + "failed to register fixed-link phy\n"); > + goto free; > + } > + > + emac->phy_node = eth_node; > + } > + > + ret = of_get_phy_mode(eth_node, &emac->phy_if); > + if (ret) { > + dev_err(prueth->dev, "could not get phy-mode property\n"); > + goto free; > + } > + > + if (emac->phy_if != PHY_INTERFACE_MODE_MII && > + !phy_interface_mode_is_rgmii(emac->phy_if)) { > + dev_err(prueth->dev, "PHY mode unsupported %s\n", phy_modes(emac->phy_if)); > + ret = -EINVAL; > + goto free; > + } > + > + /* AM65 SR2.0 has TX Internal delay always enabled by hardware > + * and it is not possible to disable TX Internal delay. The below > + * switch case block describes how we handle different phy modes > + * based on hardware restriction. > + */ > + switch (emac->phy_if) { > + case PHY_INTERFACE_MODE_RGMII_ID: > + emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; > + break; > + case PHY_INTERFACE_MODE_RGMII_TXID: > + emac->phy_if = PHY_INTERFACE_MODE_RGMII; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + dev_err(prueth->dev, "RGMII mode without TX delay is not supported"); > + return -EINVAL; At this point ndev prueth->emac[mac] == emac, so the caller will try to clean it up via prueth_netdev_exit(), which in turn expects the device being fully initialized, while this is not. Notably the napi instance has not being registered yet. You should 'goto free;' above and possibly move the 'ndev prueth- >emac[mac] = emac' assignment at the end of this function. Thanks! Paolo
Hi Paolo, On 18/07/23 1:52 pm, Paolo Abeni wrote: > On Fri, 2023-07-14 at 15:14 +0530, MD Danish Anwar wrote: >> +static int prueth_netdev_init(struct prueth *prueth, >> + struct device_node *eth_node) >> +{ >> + int ret, num_tx_chn = PRUETH_MAX_TX_QUEUES; >> + struct prueth_emac *emac; >> + struct net_device *ndev; >> + enum prueth_port port; >> + enum prueth_mac mac; >> + >> + port = prueth_node_port(eth_node); >> + if (port == PRUETH_PORT_INVALID) >> + return -EINVAL; >> + >> + mac = prueth_node_mac(eth_node); >> + if (mac == PRUETH_MAC_INVALID) >> + return -EINVAL; >> + >> + ndev = alloc_etherdev_mq(sizeof(*emac), num_tx_chn); >> + if (!ndev) >> + return -ENOMEM; >> + >> + emac = netdev_priv(ndev); >> + prueth->emac[mac] = emac; >> + emac->prueth = prueth; >> + emac->ndev = ndev; >> + emac->port_id = port; >> + emac->cmd_wq = create_singlethread_workqueue("icssg_cmd_wq"); >> + if (!emac->cmd_wq) { >> + ret = -ENOMEM; >> + goto free_ndev; >> + } >> + INIT_WORK(&emac->rx_mode_work, emac_ndo_set_rx_mode_work); >> + >> + ret = pruss_request_mem_region(prueth->pruss, >> + port == PRUETH_PORT_MII0 ? >> + PRUSS_MEM_DRAM0 : PRUSS_MEM_DRAM1, >> + &emac->dram); >> + if (ret) { >> + dev_err(prueth->dev, "unable to get DRAM: %d\n", ret); >> + ret = -ENOMEM; >> + goto free_wq; >> + } >> + >> + emac->tx_ch_num = 1; >> + >> + SET_NETDEV_DEV(ndev, prueth->dev); >> + spin_lock_init(&emac->lock); >> + mutex_init(&emac->cmd_lock); >> + >> + emac->phy_node = of_parse_phandle(eth_node, "phy-handle", 0); >> + if (!emac->phy_node && !of_phy_is_fixed_link(eth_node)) { >> + dev_err(prueth->dev, "couldn't find phy-handle\n"); >> + ret = -ENODEV; >> + goto free; >> + } else if (of_phy_is_fixed_link(eth_node)) { >> + ret = of_phy_register_fixed_link(eth_node); >> + if (ret) { >> + ret = dev_err_probe(prueth->dev, ret, >> + "failed to register fixed-link phy\n"); >> + goto free; >> + } >> + >> + emac->phy_node = eth_node; >> + } >> + >> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >> + if (ret) { >> + dev_err(prueth->dev, "could not get phy-mode property\n"); >> + goto free; >> + } >> + >> + if (emac->phy_if != PHY_INTERFACE_MODE_MII && >> + !phy_interface_mode_is_rgmii(emac->phy_if)) { >> + dev_err(prueth->dev, "PHY mode unsupported %s\n", phy_modes(emac->phy_if)); >> + ret = -EINVAL; >> + goto free; >> + } >> + >> + /* AM65 SR2.0 has TX Internal delay always enabled by hardware >> + * and it is not possible to disable TX Internal delay. The below >> + * switch case block describes how we handle different phy modes >> + * based on hardware restriction. >> + */ >> + switch (emac->phy_if) { >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; >> + break; >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + emac->phy_if = PHY_INTERFACE_MODE_RGMII; >> + break; >> + case PHY_INTERFACE_MODE_RGMII: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + dev_err(prueth->dev, "RGMII mode without TX delay is not supported"); >> + return -EINVAL; > > At this point ndev prueth->emac[mac] == emac, so the caller will try to > clean it up via prueth_netdev_exit(), which in turn expects the device > being fully initialized, while this is not. Notably the napi instance > has not being registered yet. > > You should 'goto free;' above and possibly move the 'ndev prueth- >> emac[mac] = emac' assignment at the end of this function. > Sure Paolo. Instead of returning -EINVAL in this switch case, I will do the below change. ret = -EINVAL goto free; Also I will move the 'ndev prueth-> emac[mac] = emac' assignment at the end of this function. Please let me know if you have any more comments / suggestions. I will try to address them and send a next revision.