On 14-Mar-19 14:39, Jason Gunthorpe wrote:
> 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?
No particular reason, will change.
>
>> +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..
Didn't see this in for-next yet? The core API changes very frequently, these
changes could block the driver forever :).
>
>> +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
ACK.
>
>> + 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
Will remove.
>
>> + 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
Thanks, wasn't familiar with this.
