On Thu, Mar 14, 2019 at 01:45:21PM +0200, Gal Pressman wrote:
> +static int efa_probe_device(struct pci_dev *pdev)
> +{
> + struct efa_com_dev *edev;
> + struct efa_dev *dev;
> + int bars;
> + int err;
> +
> + err = pci_enable_device_mem(pdev);
> + if (err) {
> + efa_err(&pdev->dev, "pci_enable_device_mem() failed!\n");
> + return err;
> + }
> +
> + pci_set_master(pdev);
> +
> + dev = ib_alloc_device(efa_dev, ibdev);
> + if (!dev) {
> + efa_err(&pdev->dev, "Device alloc failed\n");
> + err = -ENOMEM;
> + goto err_disable_device;
> + }
> +
> + edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> + if (!edev) {
> + err = -ENOMEM;
> + goto err_ibdev_destroy;
> + }
>
Why two peices of memory with identical life times? Put efa_com_dev in efa_dev?
> +static void efa_remove_device(struct pci_dev *pdev)
> +{
> + struct efa_dev *dev = pci_get_drvdata(pdev);
> + struct efa_com_dev *edev;
> +
> + edev = dev->edev;
> + efa_com_admin_destroy(edev);
> + efa_free_mgmnt_irq(dev);
> + efa_disable_msix(dev);
> + efa_com_mmio_reg_read_destroy(edev);
> + devm_iounmap(&pdev->dev, edev->reg_bar);
> + efa_release_bars(dev, EFA_BASE_BAR_MASK);
> + kfree(edev);
> + ib_dealloc_device(&dev->ibdev);
> + pci_disable_device(pdev);
This should probably be using the new dealloc_driver flow, it is
somewhat easier to see how the lifetimes work..
> +static int efa_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + struct efa_dev *dev;
> + int err;
> +
> + err = efa_probe_device(pdev);
> + if (err)
> + return err;
> +
> + dev = pci_get_drvdata(pdev);
Should try to avoid using drvdata unless it is necessary
> + err = efa_ib_device_add(dev);
> + if (err)
> + goto err_remove_device;
> +
> + return 0;
> +
> +err_remove_device:
> + efa_remove_device(pdev);
> + return err;
> +}
> +
> +static void efa_remove(struct pci_dev *pdev)
> +{
> + struct efa_dev *dev = (struct efa_dev *)pci_get_drvdata(pdev);
This cast is redundant
> + efa_ib_device_remove(dev);
> + efa_remove_device(pdev);
> +}
> +
> +static struct pci_driver efa_pci_driver = {
> + .name = DRV_MODULE_NAME,
> + .id_table = efa_pci_tbl,
> + .probe = efa_probe,
> + .remove = efa_remove,
> +};
> +
> +static int __init efa_init(void)
> +{
> + int err;
> +
> + err = pci_register_driver(&efa_pci_driver);
> + if (err) {
> + pr_err("Couldn't register efa driver\n");
> + goto err_register;
> + }
> +
> + return 0;
> +
> +err_register:
> + return err;
> +}
> +
> +static void __exit efa_exit(void)
> +{
> + pci_unregister_driver(&efa_pci_driver);
> +}
> +
> +module_init(efa_init);
> +module_exit(efa_exit);
This is all just module_pci_driver
Jason
