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.
--
Thanks and Regards,
Danish.
