| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Oct 2018 10:14:05 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc: davem@...emloft.net, Sasha Neftin <sasha.neftin@...el.com>,
netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com
Subject: Re: [net-next 01/11] igc: Add skeletal frame for Intel(R) 2.5G
Ethernet Controller support
On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote:
> From: Sasha Neftin <sasha.neftin@...el.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@...el.com>
> Tested-by: Aaron Brown <aaron.f.brown@...el.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.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@...el.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 :)
Powered by blists - more mailing lists