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

[Shannon Nelson <snelson@pensando.io>]

On 6/20/19 2:24 PM, Andrew Lunn wrote:

Hi Andrew, thanks for your time and comments.  Responses below...

>> +++ 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.
Yes, any version numbering thing from the big distros is put into 
question, but I find this number useful to me for tracking what has been 
put into the upstream kernel.  This plus the full kernel version gives 
me a pretty good idea of what I'm looking at.

>> +
>> +// 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?

Yes, there is an instance of sharing planned.

>
>   +
>> +#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.
Sure
>
>> +
>> +	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().
It has been useful in testing, but it can go away.
>
> Also, i think the core will NULL out the drive data for you. But you
> should check.
I'll 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.
Thanks, I'll look at that.

Cheers,
sln
>
> 	Andrew