Re: [PATCH net-next 01/18] ionic: Add basic framework for IONIC Network device driver

[Andrew Lunn <andrew@lunn.ch>]

> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
> +
> +#ifndef _IONIC_H_
> +#define _IONIC_H_
> +
> +#define DRV_NAME		"ionic"
> +#define DRV_DESCRIPTION		"Pensando Ethernet NIC Driver"
> +#define DRV_VERSION		"0.11.0-k"

DRV_VERSION is pretty useless. What you really want to know is the
kernel git tree and commit. The big distributions might backport this
version of the driver back to the old kernel with a million
patches. At which point 0.11.0-k tells you nothing much.
> +
> +// TODO: register these with the official include/linux/pci_ids.h
> +#define PCI_VENDOR_ID_PENSANDO			0x1dd8

That file has a comment:

 *      Do not add new entries to this file unless the definitions
 *      are shared between multiple drivers.

Is it going to be shared?

 +
> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF	0x1002
> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF	0x1003
> +#define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT	0x1004
> +
> +#define IONIC_SUBDEV_ID_NAPLES_25	0x4000
> +#define IONIC_SUBDEV_ID_NAPLES_100_4	0x4001
> +#define IONIC_SUBDEV_ID_NAPLES_100_8	0x4002
> +
> +struct ionic {
> +	struct pci_dev *pdev;
> +	struct device *dev;
> +};
> +
> +#endif /* _IONIC_H_ */
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus.h b/drivers/net/ethernet/pensando/ionic/ionic_bus.h
> new file mode 100644
> index 000000000000..94ba0afc6f38
> --- /dev/null
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
> +
> +#ifndef _IONIC_BUS_H_
> +#define _IONIC_BUS_H_
> +
> +int ionic_bus_register_driver(void);
> +void ionic_bus_unregister_driver(void);
> +
> +#endif /* _IONIC_BUS_H_ */
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> new file mode 100644
> index 000000000000..ab6206c162d4
> --- /dev/null
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
> +
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/pci.h>
> +
> +#include "ionic.h"
> +#include "ionic_bus.h"
> +
> +/* Supported devices */
> +static const struct pci_device_id ionic_id_table[] = {
> +	{ PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF) },
> +	{ PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF) },
> +	{ PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT) },
> +	{ 0, }	/* end of table */
> +};
> +MODULE_DEVICE_TABLE(pci, ionic_id_table);
> +
> +static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ionic *ionic;
> +
> +	ionic = devm_kzalloc(dev, sizeof(*ionic), GFP_KERNEL);
> +	if (!ionic)
> +		return -ENOMEM;
> +
> +	ionic->pdev = pdev;
> +	pci_set_drvdata(pdev, ionic);
> +	ionic->dev = dev;
> +	dev_info(ionic->dev, "attached\n");

probed would be more accurate. But in general, please avoid all but
the minimum of such info messages.

> +
> +	return 0;
> +}
> +
> +static void ionic_remove(struct pci_dev *pdev)
> +{
> +	struct ionic *ionic = pci_get_drvdata(pdev);
> +
> +	pci_set_drvdata(pdev, NULL);
> +	dev_info(ionic->dev, "removed\n");

Not very useful dev_info().

Also, i think the core will NULL out the drive data for you. But you
should check.
> +}
> +
> +static struct pci_driver ionic_driver = {
> +	.name = DRV_NAME,
> +	.id_table = ionic_id_table,
> +	.probe = ionic_probe,
> +	.remove = ionic_remove,
> +};
> +
> +int ionic_bus_register_driver(void)
> +{
> +	return pci_register_driver(&ionic_driver);
> +}
> +
> +void ionic_bus_unregister_driver(void)
> +{
> +	pci_unregister_driver(&ionic_driver);
> +}

It looks like you can use module_pci_driver() and remove a lot of
boilerplate.

	Andrew