Messages in this thread | | | Date | Wed, 31 Jan 2024 16:14:06 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver framework and device initialization |
| |
On Sun, 28 Jan 2024 21:02:47 -0800 Shinas Rasheed wrote: > +static int octep_vf_stop(struct net_device *netdev) > +{ > + struct octep_vf_device *oct = netdev_priv(netdev); > + > + netdev_info(netdev, "Stopping the device ...\n"); > + > + /* Stop Tx from stack */ > + netif_tx_stop_all_queues(netdev);
netif_tx_disable() stops queues, IIRC. You seem to stop them twice.
> + netif_carrier_off(netdev); > + netif_tx_disable(netdev);
You haven't masked any IRQ or disabled NAPI. What prevents the queues from getting restarted right after this call?
> +static void octep_vf_tx_timeout(struct net_device *netdev, unsigned int txqueue) > +{ > + struct octep_vf_device *oct = netdev_priv(netdev); > + > + queue_work(octep_vf_wq, &oct->tx_timeout_task); > +}
I don't see you canceling this work. What if someone unregistered the device before it runs? You gotta netdev_hold() a reference.
> +err_register_dev: > +err_mbox_version: > + octep_vf_delete_mbox(octep_vf_dev); > +err_setup_mbox: > + octep_vf_device_cleanup(octep_vf_dev); > +err_octep_vf_config: > + free_netdev(netdev); > +err_alloc_netdev: > + pci_release_mem_regions(pdev); > +err_pci_regions: > +err_dma_mask: > + pci_disable_device(pdev); > + dev_err(&pdev->dev, "Device probe failed\n"); > + return err; > +}
Name the labels after what you're jumping to, please. It's so much easier to make sure the code is correct that way.
> +static int __init octep_vf_init_module(void) > +{ > + int ret; > + > + pr_info("%s: Loading %s ...\n", OCTEP_VF_DRV_NAME, OCTEP_VF_DRV_STRING); > + > + /* work queue for all deferred tasks */ > + octep_vf_wq = create_singlethread_workqueue(OCTEP_VF_DRV_NAME);
Is there a reason this wq has to be single threaded and different than system queue? All you schedule on it in this series is the reset task.
> + if (!octep_vf_wq) { > + pr_err("%s: Failed to create common workqueue\n", > + OCTEP_VF_DRV_NAME); > + return -ENOMEM; > + } > + > + ret = pci_register_driver(&octep_vf_driver); > + if (ret < 0) { > + pr_err("%s: Failed to register PCI driver; err=%d\n", > + OCTEP_VF_DRV_NAME, ret); > + return ret; > + } > + > + pr_info("%s: Loaded successfully !\n", OCTEP_VF_DRV_NAME);
One message when driver is loaded is probably fine, but two is really pushing it. Please don't spam the logs.
> + return ret; > +} -- pw-bot: cr
| |