Re: [net-next 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support
On 10/18/2018 20:14, Jakub Kicinski wrote:
On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote:
From: Sasha Neftin
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
Tested-by: Aaron Brown
Signed-off-by: Jeff Kirsher
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 ..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
+
+#include
+#include
+#include
+
+#include
+
+#include
+
+#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
+#include
+#include
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 ..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 ..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
+#include
+
+#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, ");
+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(>dev, DMA_BIT_MASK(64));
+ if (!err) {
+ err = dma_set_coherent_mask(>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(>dev, DMA_BIT_MASK(32));
+ if (err) {
+ err = dma_set_coherent_mask(>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
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
>
> 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
> Tested-by: Aaron Brown
> Signed-off-by: Jeff Kirsher
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 ..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
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +
> +#define IGC_ERR(args...) pr_err("igc: " args)
Looks like you're reimplementing pr_fmt()
> +#define PFX "igc: "
> +
> +#include
> +#include
> +#include
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 ..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_V0x15F3
> +
> +#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 ..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
> +#include
> +
> +#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, ");
> +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(>dev, DMA_BIT_MASK(64));
> + if (!err) {
> + err = dma_set_coherent_mask(>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(>dev, DMA_BIT_MASK(32));
> + if (err) {
> + err = dma_set_coherent_mask(>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
