| Message ID | 20181017222322.2171-2-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | 1GbE Intel Wired LAN Driver Updates 2018-10-17 | expand |
On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote: > From: Sasha Neftin <sasha.neftin@intel.com> > > This patch adds the beginning framework onto which I am going to add > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G > Ethernet Controller. > > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> bunch of minor nit picks > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > new file mode 100644 > index 000000000000..afe595cfcf63 > --- /dev/null > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2018 Intel Corporation */ > + > +#ifndef _IGC_H_ > +#define _IGC_H_ > + > +#include <linux/kobject.h> > + > +#include <linux/pci.h> > +#include <linux/netdevice.h> > +#include <linux/vmalloc.h> > + > +#include <linux/ethtool.h> > + > +#include <linux/sctp.h> > + > +#define IGC_ERR(args...) pr_err("igc: " args) Looks like you're reimplementing pr_fmt() > +#define PFX "igc: " > + > +#include <linux/timecounter.h> > +#include <linux/net_tstamp.h> > +#include <linux/ptp_clock_kernel.h> Splitting the includes looks fairly weird. Also, you're not using any of them here. > +/* main */ > +extern char igc_driver_name[]; > +extern char igc_driver_version[]; > + > +#endif /* _IGC_H_ */ > diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h > new file mode 100644 > index 000000000000..aa68b4516700 > --- /dev/null > +++ b/drivers/net/ethernet/intel/igc/igc_hw.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2018 Intel Corporation */ > + > +#ifndef _IGC_HW_H_ > +#define _IGC_HW_H_ > + > +#define IGC_DEV_ID_I225_LM 0x15F2 > +#define IGC_DEV_ID_I225_V 0x15F3 > + > +#endif /* _IGC_HW_H_ */ > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > new file mode 100644 > index 000000000000..753749ce5ae0 > --- /dev/null > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -0,0 +1,146 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2018 Intel Corporation */ > + > +#include <linux/module.h> > +#include <linux/types.h> > + > +#include "igc.h" > +#include "igc_hw.h" > + > +#define DRV_VERSION "0.0.1-k" You can just use the kernel version, it works pretty well in presence of backports. > +#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver" > + > +MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>"); > +MODULE_DESCRIPTION(DRV_SUMMARY); > +MODULE_LICENSE("GPL v2"); > +MODULE_VERSION(DRV_VERSION); > + > +char igc_driver_name[] = "igc"; > +char igc_driver_version[] = DRV_VERSION; > +static const char igc_driver_string[] = DRV_SUMMARY; > +static const char igc_copyright[] = > + "Copyright(c) 2018 Intel Corporation."; > + > +static const struct pci_device_id igc_pci_tbl[] = { > + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) }, > + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) }, > + /* required last entry */ > + {0, } > +}; > + > +MODULE_DEVICE_TABLE(pci, igc_pci_tbl); > + > +/** > + * igc_probe - Device Initialization Routine > + * @pdev: PCI device information struct > + * @ent: entry in igc_pci_tbl > + * > + * Returns 0 on success, negative on failure > + * > + * igc_probe initializes an adapter identified by a pci_dev structure. > + * The OS initialization, configuring the adapter private structure, > + * and a hardware reset occur. > + */ > +static int igc_probe(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + int err, pci_using_dac; > + > + err = pci_enable_device_mem(pdev); > + if (err) > + return err; > + > + pci_using_dac = 0; > + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)); > + if (!err) { > + err = dma_set_coherent_mask(&pdev->dev, > + DMA_BIT_MASK(64)); > + if (!err) > + pci_using_dac = 1; You never use this pci_using_dac. dma_set_mask_and_coherent() maybe? > + } else { > + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); > + if (err) { > + err = dma_set_coherent_mask(&pdev->dev, > + DMA_BIT_MASK(32)); > + if (err) { > + IGC_ERR("Wrong DMA configuration, aborting\n"); > + goto err_dma; > + } > + } > + } > + > + err = pci_request_selected_regions(pdev, > + pci_select_bars(pdev, > + IORESOURCE_MEM), > + igc_driver_name); > + if (err) > + goto err_pci_reg; > + > + pci_set_master(pdev); > + err = pci_save_state(pdev); And you never look at err? > + return 0; > + > +err_pci_reg: > +err_dma: The error label should be named after what it points to, not where it's coming from. > + pci_disable_device(pdev); > + return err; > +} > + > +/** > + * igc_remove - Device Removal Routine > + * @pdev: PCI device information struct > + * > + * igc_remove is called by the PCI subsystem to alert the driver > + * that it should release a PCI device. This could be caused by a > + * Hot-Plug event, or because the driver is going to be removed from > + * memory. > + */ > +static void igc_remove(struct pci_dev *pdev) > +{ > + pci_release_selected_regions(pdev, > + pci_select_bars(pdev, IORESOURCE_MEM)); > + > + pci_disable_device(pdev); > +} > + > +static struct pci_driver igc_driver = { > + .name = igc_driver_name, > + .id_table = igc_pci_tbl, > + .probe = igc_probe, > + .remove = igc_remove, > +}; > + > +/** > + * igc_init_module - Driver Registration Routine > + * > + * igc_init_module is the first routine called when the driver is > + * loaded. All it does is register with the PCI subsystem. > + */ > +static int __init igc_init_module(void) > +{ > + int ret; > + > + pr_info("%s - version %s\n", > + igc_driver_string, igc_driver_version); > + > + pr_info("%s\n", igc_copyright); > + > + ret = pci_register_driver(&igc_driver); > + return ret; Why the variable? > +} > + > +module_init(igc_init_module); > + > +/** > + * igc_exit_module - Driver Exit Cleanup Routine > + * > + * igc_exit_module is called just before the driver is removed > + * from memory. > + */ > +static void __exit igc_exit_module(void) > +{ > + pci_unregister_driver(&igc_driver); > +} > + > +module_exit(igc_exit_module); > +/* igc_main.c */ I'd argue most editors make it fairly clear which file one is editing, hence making this sort of comments entirely superfluous :)
On 10/18/2018 20:14, Jakub Kicinski wrote: > On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote: >> From: Sasha Neftin <sasha.neftin@intel.com> >> >> This patch adds the beginning framework onto which I am going to add >> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G >> Ethernet Controller. >> >> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > bunch of minor nit picks > >> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h >> new file mode 100644 >> index 000000000000..afe595cfcf63 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc.h >> @@ -0,0 +1,29 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#ifndef _IGC_H_ >> +#define _IGC_H_ >> + >> +#include <linux/kobject.h> >> + >> +#include <linux/pci.h> >> +#include <linux/netdevice.h> >> +#include <linux/vmalloc.h> >> + >> +#include <linux/ethtool.h> >> + >> +#include <linux/sctp.h> >> + >> +#define IGC_ERR(args...) pr_err("igc: " args) > > Looks like you're reimplementing pr_fmt() > yes, it is convenient for a debug. Same methodological approach I saw in other drivers. >> +#define PFX "igc: " >> + >> +#include <linux/timecounter.h> >> +#include <linux/net_tstamp.h> >> +#include <linux/ptp_clock_kernel.h> > > Splitting the includes looks fairly weird. Also, you're not using any > of them here. > Good catch. I'll remove splits and unused "include"'s. All "include"'s will be add per demand. I will send patch to fix that. >> +/* main */ >> +extern char igc_driver_name[]; >> +extern char igc_driver_version[]; >> + >> +#endif /* _IGC_H_ */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h >> new file mode 100644 >> index 000000000000..aa68b4516700 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#ifndef _IGC_HW_H_ >> +#define _IGC_HW_H_ >> + >> +#define IGC_DEV_ID_I225_LM 0x15F2 >> +#define IGC_DEV_ID_I225_V 0x15F3 >> + >> +#endif /* _IGC_HW_H_ */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> new file mode 100644 >> index 000000000000..753749ce5ae0 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -0,0 +1,146 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#include <linux/module.h> >> +#include <linux/types.h> >> + >> +#include "igc.h" >> +#include "igc_hw.h" >> + >> +#define DRV_VERSION "0.0.1-k" > > You can just use the kernel version, it works pretty well in presence > of backports. > Good idea, I think I will do it too. >> +#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver" >> + >> +MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>"); >> +MODULE_DESCRIPTION(DRV_SUMMARY); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_VERSION(DRV_VERSION); >> + >> +char igc_driver_name[] = "igc"; >> +char igc_driver_version[] = DRV_VERSION; >> +static const char igc_driver_string[] = DRV_SUMMARY; >> +static const char igc_copyright[] = >> + "Copyright(c) 2018 Intel Corporation."; >> + >> +static const struct pci_device_id igc_pci_tbl[] = { >> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) }, >> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) }, >> + /* required last entry */ >> + {0, } >> +}; >> + >> +MODULE_DEVICE_TABLE(pci, igc_pci_tbl); >> + >> +/** >> + * igc_probe - Device Initialization Routine >> + * @pdev: PCI device information struct >> + * @ent: entry in igc_pci_tbl >> + * >> + * Returns 0 on success, negative on failure >> + * >> + * igc_probe initializes an adapter identified by a pci_dev structure. >> + * The OS initialization, configuring the adapter private structure, >> + * and a hardware reset occur. >> + */ >> +static int igc_probe(struct pci_dev *pdev, >> + const struct pci_device_id *ent) >> +{ >> + int err, pci_using_dac; >> + >> + err = pci_enable_device_mem(pdev); >> + if (err) >> + return err; >> + >> + pci_using_dac = 0; >> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)); >> + if (!err) { >> + err = dma_set_coherent_mask(&pdev->dev, >> + DMA_BIT_MASK(64)); >> + if (!err) >> + pci_using_dac = 1; > > You never use this pci_using_dac. dma_set_mask_and_coherent() maybe? > Right. I saw already somebody sent out patch to fix that. >> + } else { >> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); >> + if (err) { >> + err = dma_set_coherent_mask(&pdev->dev, >> + DMA_BIT_MASK(32)); >> + if (err) { >> + IGC_ERR("Wrong DMA configuration, aborting\n"); >> + goto err_dma; >> + } >> + } >> + } >> + >> + err = pci_request_selected_regions(pdev, >> + pci_select_bars(pdev, >> + IORESOURCE_MEM), >> + igc_driver_name); >> + if (err) >> + goto err_pci_reg; >> + >> + pci_set_master(pdev); >> + err = pci_save_state(pdev); > > And you never look at err? > Same as above. >> + return 0; >> + >> +err_pci_reg: >> +err_dma: > > The error label should be named after what it points to, not where it's > coming from. > >> + pci_disable_device(pdev); >> + return err; >> +} >> + >> +/** >> + * igc_remove - Device Removal Routine >> + * @pdev: PCI device information struct >> + * >> + * igc_remove is called by the PCI subsystem to alert the driver >> + * that it should release a PCI device. This could be caused by a >> + * Hot-Plug event, or because the driver is going to be removed from >> + * memory. >> + */ >> +static void igc_remove(struct pci_dev *pdev) >> +{ >> + pci_release_selected_regions(pdev, >> + pci_select_bars(pdev, IORESOURCE_MEM)); >> + >> + pci_disable_device(pdev); >> +} >> + >> +static struct pci_driver igc_driver = { >> + .name = igc_driver_name, >> + .id_table = igc_pci_tbl, >> + .probe = igc_probe, >> + .remove = igc_remove, >> +}; >> + >> +/** >> + * igc_init_module - Driver Registration Routine >> + * >> + * igc_init_module is the first routine called when the driver is >> + * loaded. All it does is register with the PCI subsystem. >> + */ >> +static int __init igc_init_module(void) >> +{ >> + int ret; >> + >> + pr_info("%s - version %s\n", >> + igc_driver_string, igc_driver_version); >> + >> + pr_info("%s\n", igc_copyright); >> + >> + ret = pci_register_driver(&igc_driver); >> + return ret; > > Why the variable? > we can return 'pci_register_driver' here, but variable is more clearly. >> +} >> + >> +module_init(igc_init_module); >> + >> +/** >> + * igc_exit_module - Driver Exit Cleanup Routine >> + * >> + * igc_exit_module is called just before the driver is removed >> + * from memory. >> + */ >> +static void __exit igc_exit_module(void) >> +{ >> + pci_unregister_driver(&igc_driver); >> +} >> + >> +module_exit(igc_exit_module); >> +/* igc_main.c */ > > I'd argue most editors make it fairly clear which file one is > editing, hence making this sort of comments entirely superfluous :) > Thanks for your comments.
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig index b542aba6f0e8..76f926d4bf13 100644 --- a/drivers/net/ethernet/intel/Kconfig +++ b/drivers/net/ethernet/intel/Kconfig @@ -287,4 +287,20 @@ config FM10K To compile this driver as a module, choose M here. The module will be called fm10k. MSI-X interrupt support is required +config IGC + tristate "Intel(R) Ethernet Controller I225-LM/I225-V support" + default n + depends on PCI + ---help--- + This driver supports Intel(R) Ethernet Controller I225-LM/I225-V + family of adapters. + + For more information on how to identify your adapter, go + to the Adapter & Driver ID Guide that can be located at: + + <http://support.intel.com> + + To compile this driver as a module, choose M here. The module + will be called igc. + endif # NET_VENDOR_INTEL diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile index b91153df6ee8..3075290063f6 100644 --- a/drivers/net/ethernet/intel/Makefile +++ b/drivers/net/ethernet/intel/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_E100) += e100.o obj-$(CONFIG_E1000) += e1000/ obj-$(CONFIG_E1000E) += e1000e/ obj-$(CONFIG_IGB) += igb/ +obj-$(CONFIG_IGC) += igc/ obj-$(CONFIG_IGBVF) += igbvf/ obj-$(CONFIG_IXGBE) += ixgbe/ obj-$(CONFIG_IXGBEVF) += ixgbevf/ diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile new file mode 100644 index 000000000000..3d13b015d401 --- /dev/null +++ b/drivers/net/ethernet/intel/igc/Makefile @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Intel Corporation + +# +# Intel(R) I225-LM/I225-V 2.5G Ethernet Controller +# + +obj-$(CONFIG_IGC) += igc.o + +igc-objs := igc_main.o diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h new file mode 100644 index 000000000000..afe595cfcf63 --- /dev/null +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2018 Intel Corporation */ + +#ifndef _IGC_H_ +#define _IGC_H_ + +#include <linux/kobject.h> + +#include <linux/pci.h> +#include <linux/netdevice.h> +#include <linux/vmalloc.h> + +#include <linux/ethtool.h> + +#include <linux/sctp.h> + +#define IGC_ERR(args...) pr_err("igc: " args) + +#define PFX "igc: " + +#include <linux/timecounter.h> +#include <linux/net_tstamp.h> +#include <linux/ptp_clock_kernel.h> + +/* main */ +extern char igc_driver_name[]; +extern char igc_driver_version[]; + +#endif /* _IGC_H_ */ diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h new file mode 100644 index 000000000000..aa68b4516700 --- /dev/null +++ b/drivers/net/ethernet/intel/igc/igc_hw.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2018 Intel Corporation */ + +#ifndef _IGC_HW_H_ +#define _IGC_HW_H_ + +#define IGC_DEV_ID_I225_LM 0x15F2 +#define IGC_DEV_ID_I225_V 0x15F3 + +#endif /* _IGC_HW_H_ */ diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c new file mode 100644 index 000000000000..753749ce5ae0 --- /dev/null +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2018 Intel Corporation */ + +#include <linux/module.h> +#include <linux/types.h> + +#include "igc.h" +#include "igc_hw.h" + +#define DRV_VERSION "0.0.1-k" +#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver" + +MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>"); +MODULE_DESCRIPTION(DRV_SUMMARY); +MODULE_LICENSE("GPL v2"); +MODULE_VERSION(DRV_VERSION); + +char igc_driver_name[] = "igc"; +char igc_driver_version[] = DRV_VERSION; +static const char igc_driver_string[] = DRV_SUMMARY; +static const char igc_copyright[] = + "Copyright(c) 2018 Intel Corporation."; + +static const struct pci_device_id igc_pci_tbl[] = { + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) }, + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) }, + /* required last entry */ + {0, } +}; + +MODULE_DEVICE_TABLE(pci, igc_pci_tbl); + +/** + * igc_probe - Device Initialization Routine + * @pdev: PCI device information struct + * @ent: entry in igc_pci_tbl + * + * Returns 0 on success, negative on failure + * + * igc_probe initializes an adapter identified by a pci_dev structure. + * The OS initialization, configuring the adapter private structure, + * and a hardware reset occur. + */ +static int igc_probe(struct pci_dev *pdev, + const struct pci_device_id *ent) +{ + int err, pci_using_dac; + + err = pci_enable_device_mem(pdev); + if (err) + return err; + + pci_using_dac = 0; + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)); + if (!err) { + err = dma_set_coherent_mask(&pdev->dev, + DMA_BIT_MASK(64)); + if (!err) + pci_using_dac = 1; + } else { + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); + if (err) { + err = dma_set_coherent_mask(&pdev->dev, + DMA_BIT_MASK(32)); + if (err) { + IGC_ERR("Wrong DMA configuration, aborting\n"); + goto err_dma; + } + } + } + + err = pci_request_selected_regions(pdev, + pci_select_bars(pdev, + IORESOURCE_MEM), + igc_driver_name); + if (err) + goto err_pci_reg; + + pci_set_master(pdev); + err = pci_save_state(pdev); + return 0; + +err_pci_reg: +err_dma: + pci_disable_device(pdev); + return err; +} + +/** + * igc_remove - Device Removal Routine + * @pdev: PCI device information struct + * + * igc_remove is called by the PCI subsystem to alert the driver + * that it should release a PCI device. This could be caused by a + * Hot-Plug event, or because the driver is going to be removed from + * memory. + */ +static void igc_remove(struct pci_dev *pdev) +{ + pci_release_selected_regions(pdev, + pci_select_bars(pdev, IORESOURCE_MEM)); + + pci_disable_device(pdev); +} + +static struct pci_driver igc_driver = { + .name = igc_driver_name, + .id_table = igc_pci_tbl, + .probe = igc_probe, + .remove = igc_remove, +}; + +/** + * igc_init_module - Driver Registration Routine + * + * igc_init_module is the first routine called when the driver is + * loaded. All it does is register with the PCI subsystem. + */ +static int __init igc_init_module(void) +{ + int ret; + + pr_info("%s - version %s\n", + igc_driver_string, igc_driver_version); + + pr_info("%s\n", igc_copyright); + + ret = pci_register_driver(&igc_driver); + return ret; +} + +module_init(igc_init_module); + +/** + * igc_exit_module - Driver Exit Cleanup Routine + * + * igc_exit_module is called just before the driver is removed + * from memory. + */ +static void __exit igc_exit_module(void) +{ + pci_unregister_driver(&igc_driver); +} + +module_exit(igc_exit_module); +/* igc_main.c */