All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
@ 2018-06-24  8:45 Sasha Neftin
  2018-06-24 12:33 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sasha Neftin @ 2018-06-24  8:45 UTC (permalink / raw)
  To: intel-wired-lan

Now that we have the ability to configure the basic settings on the device
we can start allocating and configuring a netdev for the interface.

Sasha Neftin (v2):
added description

Sasha Neftin (v3):
minor cosmetic changes

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
 drivers/net/ethernet/intel/igc/e1000_defines.h |  15 +
 drivers/net/ethernet/intel/igc/e1000_hw.h      |   1 +
 drivers/net/ethernet/intel/igc/igc.h           |  48 +++
 drivers/net/ethernet/intel/igc/igc_main.c      | 490 ++++++++++++++++++++++++-
 4 files changed, 551 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/e1000_defines.h b/drivers/net/ethernet/intel/igc/e1000_defines.h
index 6831a0864bb4..66b05898eb00 100644
--- a/drivers/net/ethernet/intel/igc/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igc/e1000_defines.h
@@ -4,6 +4,8 @@
 #ifndef _E1000_DEFINES_H_
 #define _E1000_DEFINES_H_
 
+#define E1000_CTRL_EXT_DRV_LOAD         0x10000000 /* Drv loaded bit for FW */
+
 #define E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES	0x00C00000
 /* Priority on PCI. 0=rx,1=fair */
 #define E1000_CTRL_PRIOR			0x00000004
@@ -28,6 +30,16 @@
 #define E1000_PCI_PMCSR			0x44
 #define E1000_PCI_PMCSR_D3		0x03
 
+/* Receive Address
+ * Number of high/low register pairs in the RAR. The RAR (Receive Address
+ * Registers) holds the directed and multicast addresses that we monitor.
+ * Technically, we have 16 spots.  However, we reserve one of these spots
+ * (RAR[15]) for our directed address used by controllers with
+ * manageability enabled, allowing us room for 15 multicast addresses.
+ */
+#define E1000_RAH_AV		0x80000000 /* Receive descriptor valid */
+#define E1000_RAH_POOL_1	0x00040000
+
 /* Error Codes */
 #define E1000_SUCCESS				0
 #define E1000_ERR_NVM				1
@@ -37,6 +49,9 @@
 #define E1000_ERR_MAC_INIT			5
 #define E1000_ERR_RESET				9
 
+/* PBA constants */
+#define E1000_PBA_34K			0x0022
+
 /* Device Status */
 #define E1000_STATUS_FD		0x00000001      /* Full duplex.0=half,1=full */
 #define E1000_STATUS_LU		0x00000002      /* Link up.0=no,1=link */
diff --git a/drivers/net/ethernet/intel/igc/e1000_hw.h b/drivers/net/ethernet/intel/igc/e1000_hw.h
index b8f82f4ba998..6abe722f0bc4 100644
--- a/drivers/net/ethernet/intel/igc/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igc/e1000_hw.h
@@ -87,6 +87,7 @@ struct e1000_mac_info {
 
 	bool autoneg;
 	bool autoneg_failed;
+	bool get_link_status;
 };
 
 struct e1000_bus_info {
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index bd732390bd4b..29df87285b9b 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -28,15 +28,63 @@
 extern char igc_driver_name[];
 extern char igc_driver_version[];
 
+/* Transmit and receive queues */
+#define IGC_MAX_RX_QUEUES                 4
+#define IGC_MAX_TX_QUEUES                 4
+
+#define MAX_Q_VECTORS                     10
+#define MAX_STD_JUMBO_FRAME_SIZE        9216
+
+enum e1000_state_t {
+	 __IGC_TESTING,
+	__IGC_RESETTING,
+	 __IGC_DOWN,
+	 __IGC_PTP_TX_IN_PROGRESS,
+};
+
+struct igc_q_vector {
+	struct igc_adapter *adapter;    /* backlink */
+
+	struct napi_struct napi;
+};
+
+struct igc_mac_addr {
+	u8 addr[ETH_ALEN];
+	u8 queue;
+	u8 state; /* bitmask */
+};
+
+#define IGC_MAC_STATE_DEFAULT   0x1
+#define IGC_MAC_STATE_MODIFIED  0x2
+#define IGC_MAC_STATE_IN_USE    0x4
+
 /* Board specific private data structure */
 struct igc_adapter {
+	struct net_device *netdev;
+
+	unsigned long state;
+	unsigned int flags;
+	unsigned int num_q_vectors;
+	u16 link_speed;
+	u16 link_duplex;
+
+	u8 port_num;
+
 	u8 __iomem *io_addr;
+	struct work_struct watchdog_task;
+
+	int msg_enable;
+	u32 max_frame_size;
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
 
 	/* structs defined in e1000_hw.h */
 	struct e1000_hw hw;
+
+	struct igc_q_vector *q_vector[MAX_Q_VECTORS];
+
+	struct igc_mac_addr *mac_table;
 };
 
 #endif /* _IGC_H_ */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ec3451dad91e..520f49be8e19 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -3,6 +3,8 @@
 
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/if_vlan.h>
+#include <linux/aer.h>
 
 #include "igc.h"
 #include "e1000_hw.h"
@@ -10,6 +12,7 @@
 #define DRV_VERSION	"0.0.1-k"
 #define DRV_SUMMARY	"Intel(R) 2.5G Ethernet Linux Driver"
 
+static int debug = -1;
 char igc_driver_name[] = "igc";
 char igc_driver_version[] = DRV_VERSION;
 static const char igc_driver_string[] = DRV_SUMMARY;
@@ -25,8 +28,371 @@ static const struct pci_device_id igc_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
 
-/* Forward declaration */
+/* forward declaration */
 static int igc_sw_init(struct igc_adapter *);
+static void igc_configure(struct igc_adapter *adapter);
+static void igc_power_down_link(struct igc_adapter *adapter);
+static void igc_set_default_mac_filter(struct igc_adapter *adapter);
+
+void igc_reset(struct igc_adapter *adapter)
+{
+	if (!netif_running(adapter->netdev))
+		igc_power_down_link(adapter);
+}
+
+/**
+ *  igc_power_up_link - Power up the phy/serdes link
+ *  @adapter: address of board private structure
+ **/
+void igc_power_up_link(struct igc_adapter *adapter)
+{
+}
+
+/**
+ *  igc_power_down_link - Power down the phy/serdes link
+ *  @adapter: address of board private structure
+ **/
+static void igc_power_down_link(struct igc_adapter *adapter)
+{
+}
+
+/**
+ *  igc_release_hw_control - release control of the h/w to f/w
+ *  @adapter: address of board private structure
+ *
+ *  igc_release_hw_control resets CTRL_EXT:DRV_LOAD bit.
+ *  For ASF and Pass Through versions of f/w this means that the
+ *  driver is no longer loaded.
+ **/
+static void igc_release_hw_control(struct igc_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 ctrl_ext;
+
+	/* Let firmware take over control of h/w */
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	wr32(E1000_CTRL_EXT,
+	     ctrl_ext & ~E1000_CTRL_EXT_DRV_LOAD);
+}
+
+/**
+ *  igc_get_hw_control - get control of the h/w from f/w
+ *  @adapter: address of board private structure
+ *
+ *  igc_get_hw_control sets CTRL_EXT:DRV_LOAD bit.
+ *  For ASF and Pass Through versions of f/w this means that
+ *  the driver is loaded.
+ **/
+static void igc_get_hw_control(struct igc_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 ctrl_ext;
+
+	/* Let firmware know the driver has taken over */
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	wr32(E1000_CTRL_EXT,
+	     ctrl_ext | E1000_CTRL_EXT_DRV_LOAD);
+}
+
+/**
+ *  igc_set_mac - Change the Ethernet Address of the NIC
+ *  @netdev: network interface device structure
+ *  @p: pointer to an address structure
+ *
+ *  Returns 0 on success, negative on failure
+ **/
+static int igc_set_mac(struct net_device *netdev, void *p)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	struct sockaddr *addr = p;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
+	memcpy(hw->mac.addr, addr->sa_data, netdev->addr_len);
+
+	/* set the correct pool for the new PF MAC address in entry 0 */
+	igc_set_default_mac_filter(adapter);
+
+	return 0;
+}
+
+static netdev_tx_t igc_xmit_frame(struct sk_buff *skb,
+				  struct net_device *netdev)
+{
+	dev_kfree_skb_any(skb);
+	return NETDEV_TX_OK;
+}
+
+/**
+ *  igc_ioctl - I/O control method
+ *  @netdev: network interface device structure
+ *  @ifreq: frequency
+ *  @cmd: command
+ **/
+static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+	switch (cmd) {
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+/**
+ *  igc_up - Open the interface and prepare it to handle traffic
+ *  @adapter: board private structure
+ **/
+int igc_up(struct igc_adapter *adapter)
+{
+	int i = 0;
+
+	/* hardware has been reset, we need to reload some things */
+	igc_configure(adapter);
+
+	clear_bit(__IGC_DOWN, &adapter->state);
+
+	for (i = 0; i < adapter->num_q_vectors; i++)
+		napi_enable(&adapter->q_vector[i]->napi);
+
+	return 0;
+}
+
+/**
+ *  igc_down - Close the interface
+ *  @adapter: board private structure
+ **/
+void igc_down(struct igc_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	int i = 0;
+
+	set_bit(__IGC_DOWN, &adapter->state);
+
+	/* set trans_start so we don't get spurious watchdogs during reset */
+	netif_trans_update(netdev);
+
+	netif_carrier_off(netdev);
+	netif_tx_stop_all_queues(netdev);
+
+	for (i = 0; i < adapter->num_q_vectors; i++)
+		napi_disable(&adapter->q_vector[i]->napi);
+
+	adapter->link_speed = 0;
+	adapter->link_duplex = 0;
+}
+
+/**
+ *  igc_change_mtu - Change the Maximum Transfer Unit
+ *  @netdev: network interface device structure
+ *  @new_mtu: new value for maximum frame size
+ *
+ *  Returns 0 on success, negative on failure
+ **/
+static int igc_change_mtu(struct net_device *netdev, int new_mtu)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct pci_dev *pdev = adapter->pdev;
+	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+
+	/* adjust max frame to be at least the size of a standard frame */
+	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
+		max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
+
+	while (test_and_set_bit(__IGC_RESETTING, &adapter->state))
+		usleep_range(1000, 2000);
+
+	/* igc_down has a dependency on max_frame_size */
+	adapter->max_frame_size = max_frame;
+
+	if (netif_running(netdev))
+		igc_down(adapter);
+
+	dev_info(&pdev->dev, "changing MTU from %d to %d\n",
+		 netdev->mtu, new_mtu);
+	netdev->mtu = new_mtu;
+
+	if (netif_running(netdev))
+		igc_up(adapter);
+	else
+		igc_reset(adapter);
+
+	clear_bit(__IGC_RESETTING, &adapter->state);
+
+	return 0;
+}
+
+/**
+ *  igc_update_stats - Update the board statistics counters
+ *  @adapter: board private structure
+ **/
+void igc_update_stats(struct igc_adapter *adapter)
+{
+}
+
+/**
+ *  igc_get_stats - Get System Network Statistics
+ *  @netdev: network interface device structure
+ *
+ *  Returns the address of the device statistics structure.
+ *  The statistics are updated here and also from the timer callback.
+ **/
+static struct net_device_stats *igc_get_stats(struct net_device *netdev)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+
+	if (!test_bit(__IGC_RESETTING, &adapter->state))
+		igc_update_stats(adapter);
+
+	/* only return the current stats */
+	return &netdev->stats;
+}
+
+/**
+ *  igc_configure - configure the hardware for RX and TX
+ *  @adapter: private board structure
+ **/
+static void igc_configure(struct igc_adapter *adapter)
+{
+	igc_get_hw_control(adapter);
+}
+
+/**
+ *  igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
+ *  @adapter: Pointer to adapter structure
+ *  @index: Index of the RAR entry which need to be synced with MAC table
+ **/
+static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 rar_low, rar_high;
+	u8 *addr = adapter->mac_table[index].addr;
+
+	/* HW expects these to be in network order when they are plugged
+	 * into the registers which are little endian.  In order to guarantee
+	 * that ordering we need to do an leXX_to_cpup here in order to be
+	 * ready for the byteswap that occurs with writel
+	 */
+	rar_low = le32_to_cpup((__le32 *)(addr));
+	rar_high = le16_to_cpup((__le16 *)(addr + 4));
+
+	/* Indicate to hardware the Address is Valid. */
+	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
+		if (is_valid_ether_addr(addr))
+			rar_high |= E1000_RAH_AV;
+
+		rar_high |= E1000_RAH_POOL_1 <<
+			adapter->mac_table[index].queue;
+	}
+
+	wr32(E1000_RAL(index), rar_low);
+	wrfl();
+	wr32(E1000_RAH(index), rar_high);
+	wrfl();
+}
+
+/* Set default MAC address for the PF in the first RAR entry */
+static void igc_set_default_mac_filter(struct igc_adapter *adapter)
+{
+	struct igc_mac_addr *mac_table = &adapter->mac_table[0];
+
+	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
+	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
+
+	igc_rar_set_index(adapter, 0);
+}
+
+/**
+ * igc_open - Called when a network interface is made active
+ * @netdev: network interface device structure
+ *
+ * Returns 0 on success, negative value on failure
+ *
+ * The open entry point is called when a network interface is made
+ * active by the system (IFF_UP).  At this point all resources needed
+ * for transmit and receive operations are allocated, the interrupt
+ * handler is registered with the OS, the watchdog timer is started,
+ * and the stack is notified that the interface is ready.
+ **/
+static int __igc_open(struct net_device *netdev, bool resuming)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	int i = 0;
+
+	/* disallow open during test */
+
+	if (test_bit(__IGC_TESTING, &adapter->state)) {
+		WARN_ON(resuming);
+		return -EBUSY;
+	}
+
+	netif_carrier_off(netdev);
+
+	igc_power_up_link(adapter);
+
+	igc_configure(adapter);
+
+	/* From here on the code is the same as igc_up() */
+	clear_bit(__IGC_DOWN, &adapter->state);
+
+	for (i = 0; i < adapter->num_q_vectors; i++)
+		napi_enable(&adapter->q_vector[i]->napi);
+
+	/* start the watchdog. */
+	hw->mac.get_link_status = 1;
+	schedule_work(&adapter->watchdog_task);
+
+	return E1000_SUCCESS;
+}
+
+int igc_open(struct net_device *netdev)
+{
+	return __igc_open(netdev, false);
+}
+
+/**
+ * igc_close - Disables a network interface
+ * @netdev: network interface device structure
+ *
+ * Returns 0, this is not allowed to fail
+ *
+ * The close entry point is called when an interface is de-activated
+ * by the OS.  The hardware is still under the driver's control, but
+ * needs to be disabled.  A global MAC reset is issued to stop the
+ * hardware, and all transmit and receive resources are freed.
+ **/
+static int __igc_close(struct net_device *netdev, bool suspending)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+
+	WARN_ON(test_bit(__IGC_RESETTING, &adapter->state));
+
+	igc_down(adapter);
+
+	igc_release_hw_control(adapter);
+
+	return 0;
+}
+
+int igc_close(struct net_device *netdev)
+{
+	if (netif_device_present(netdev) || netdev->dismantle)
+		return __igc_close(netdev, false);
+	return 0;
+}
+
+static const struct net_device_ops igc_netdev_ops = {
+	.ndo_open               = igc_open,
+	.ndo_stop               = igc_close,
+	.ndo_start_xmit         = igc_xmit_frame,
+	.ndo_set_mac_address    = igc_set_mac,
+	.ndo_change_mtu         = igc_change_mtu,
+	.ndo_get_stats          = igc_get_stats,
+	.ndo_do_ioctl           = igc_ioctl,
+
+};
 
 /* PCIe configuration access */
 void igc_read_pci_cfg(struct e1000_hw *hw, u32 reg, u16 *value)
@@ -73,6 +439,7 @@ s32 igc_write_pcie_cap_reg(struct e1000_hw *hw, u32 reg, u16 *value)
 
 u32 igc_rd32(struct e1000_hw *hw, u32 reg)
 {
+	struct igc_adapter *igc = container_of(hw, struct igc_adapter, hw);
 	u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
 	u32 value = 0;
 
@@ -82,8 +449,13 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
 	value = readl(&hw_addr[reg]);
 
 	/* reads should not return all F's */
-	if (!(~value) && (!reg || !(~readl(hw_addr))))
+	if (!(~value) && (!reg || !(~readl(hw_addr)))) {
+		struct net_device *netdev = igc->netdev;
+
 		hw->hw_addr = NULL;
+		netif_device_detach(netdev);
+		netdev_err(netdev, "PCIe link lost, device now detached\n");
+	}
 
 	return value;
 }
@@ -102,6 +474,7 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
 static int igc_probe(struct pci_dev *pdev,
 		     const struct pci_device_id *ent)
 {
+	struct net_device *netdev;
 	struct igc_adapter *adapter;
 	struct e1000_hw *hw;
 	int err, pci_using_dac;
@@ -136,8 +509,56 @@ static int igc_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_pci_reg;
 
+	pci_enable_pcie_error_reporting(pdev);
+
 	pci_set_master(pdev);
-	pci_save_state(pdev);
+
+	err = -ENOMEM;
+	netdev = alloc_etherdev_mq(sizeof(struct igc_adapter),
+				   IGC_MAX_TX_QUEUES);
+
+	if (!netdev)
+		goto err_alloc_etherdev;
+
+	SET_NETDEV_DEV(netdev, &pdev->dev);
+
+	pci_set_drvdata(pdev, netdev);
+	adapter = netdev_priv(netdev);
+	adapter->netdev = netdev;
+	adapter->pdev = pdev;
+	hw = &adapter->hw;
+	hw->back = adapter;
+	adapter->port_num = hw->bus.func;
+	adapter->msg_enable = GENMASK(debug - 1, 0);
+
+	err = pci_save_state(pdev);
+	if (err)
+		goto err_ioremap;
+
+	err = -EIO;
+	adapter->io_addr = ioremap(pci_resource_start(pdev, 0),
+				   pci_resource_len(pdev, 0));
+	if (!adapter->io_addr)
+		goto err_ioremap;
+
+	/* hw->hw_addr can be zeroed, so use adapter->io_addr for unmap */
+	hw->hw_addr = adapter->io_addr;
+
+	netdev->netdev_ops = &igc_netdev_ops;
+
+	netdev->watchdog_timeo = 5 * HZ;
+
+	strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
+
+	netdev->mem_start = pci_resource_start(pdev, 0);
+	netdev->mem_end = pci_resource_end(pdev, 0);
+
+	/* PCI config space info */
+	hw->vendor_id = pdev->vendor;
+	hw->device_id = pdev->device;
+	hw->revision_id = pdev->revision;
+	hw->subsystem_vendor_id = pdev->subsystem_vendor;
+	hw->subsystem_device_id = pdev->subsystem_device;
 
 	/* setup the private structure */
 	err = igc_sw_init(adapter);
@@ -146,9 +567,49 @@ static int igc_probe(struct pci_dev *pdev,
 
 	igc_get_bus_info_pcie(hw);
 
+	/* MTU range: 68 - 9216 */
+	netdev->min_mtu = ETH_MIN_MTU;
+	netdev->max_mtu = MAX_STD_JUMBO_FRAME_SIZE;
+
+	/* reset the hardware with the new settings */
+	igc_reset(adapter);
+
+	/* let the f/w know that the h/w is now under the control of the
+	 * driver.
+	 */
+	igc_get_hw_control(adapter);
+
+	strncpy(netdev->name, "eth%d", IFNAMSIZ);
+	err = register_netdev(netdev);
+	if (err)
+		goto err_register;
+
+	 /* carrier off reporting is important to ethtool even BEFORE open */
+	netif_carrier_off(netdev);
+
+	dev_info(&pdev->dev, "@SUMMARY@");
+	/* print bus type/speed/width info */
+	dev_info(&pdev->dev, "%s: (PCIe:%s:%s) ",
+		 netdev->name,
+		 ((hw->bus.speed == e1000_bus_speed_2500) ? "2.5GT/s" :
+		 (hw->bus.speed == e1000_bus_speed_5000) ? "5.0GT/s" :
+		 "unknown"),
+		 ((hw->bus.width == e1000_bus_width_pcie_x4) ? "Width x4" :
+		 (hw->bus.width == e1000_bus_width_pcie_x2) ? "Width x2" :
+		 (hw->bus.width == e1000_bus_width_pcie_x1) ? "Width x1" :
+		 "unknown"));
+	netdev_info(netdev, "MAC: %pM\n", netdev->dev_addr);
+
 	return 0;
 
+err_register:
+	igc_release_hw_control(adapter);
 err_sw_init:
+err_ioremap:
+	free_netdev(netdev);
+err_alloc_etherdev:
+	pci_release_selected_regions(pdev,
+				     pci_select_bars(pdev, IORESOURCE_MEM));
 err_pci_reg:
 err_dma:
 	pci_disable_device(pdev);
@@ -166,9 +627,22 @@ static int igc_probe(struct pci_dev *pdev,
  **/
 static void igc_remove(struct pci_dev *pdev)
 {
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct igc_adapter *adapter = netdev_priv(netdev);
+
+	set_bit(__IGC_DOWN, &adapter->state);
+	flush_scheduled_work();
+
+	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
+	 * would have already happened in close and is redundant.
+	 */
+	igc_release_hw_control(adapter);
+	unregister_netdev(netdev);
+
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 
+	free_netdev(netdev);
 	pci_disable_device(pdev);
 }
 
@@ -190,6 +664,7 @@ static struct pci_driver igc_driver = {
 static int igc_sw_init(struct igc_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
+	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
 
 	/* PCI config space info */
@@ -203,6 +678,12 @@ static int igc_sw_init(struct igc_adapter *adapter)
 
 	pci_read_config_word(pdev, PCI_COMMAND, &hw->bus.pci_cmd_word);
 
+	/* set default work limits */
+	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
+					VLAN_HLEN;
+
+	set_bit(__IGC_DOWN, &adapter->state);
+
 	return 0;
 }
 
@@ -244,4 +725,7 @@ MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
 MODULE_DESCRIPTION(DRV_SUMMARY);
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
+
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 /* igc_main.c */
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
  2018-06-24  8:45 [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Sasha Neftin
@ 2018-06-24 12:33 ` kbuild test robot
  2018-06-24 12:34 ` [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static kbuild test robot
  2018-06-27  0:32 ` [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Shannon Nelson
  2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2018-06-24 12:33 UTC (permalink / raw)
  To: intel-wired-lan

Hi Sasha,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jkirsher-next-queue/dev-queue]
[also build test WARNING on v4.18-rc1 next-20180622]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sasha-Neftin/igc-Add-skeletal-frame-for-Intel-R-2-5G-Ethernet-Controller-support/20180624-164739
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/intel/igc/igc_main.c:37:6: sparse: symbol 'igc_reset' was not declared. Should it be static?
>> drivers/net/ethernet/intel/igc/igc_main.c:47:6: sparse: symbol 'igc_power_up_link' was not declared. Should it be static?
   drivers/net/ethernet/intel/igc/igc_main.c:74:9: sparse: incorrect type in initializer (different address spaces) @@    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr @@    got deref] [usertype] <asn:2>*hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:74:9:    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:74:9:    got unsigned char [usertype] *__val
   drivers/net/ethernet/intel/igc/igc_main.c:93:9: sparse: incorrect type in initializer (different address spaces) @@    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr @@    got deref] [usertype] <asn:2>*hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:93:9:    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:93:9:    got unsigned char [usertype] *__val
>> drivers/net/ethernet/intel/igc/igc_main.c:147:5: sparse: symbol 'igc_up' was not declared. Should it be static?
>> drivers/net/ethernet/intel/igc/igc_main.c:166:6: sparse: symbol 'igc_down' was not declared. Should it be static?
>> drivers/net/ethernet/intel/igc/igc_main.c:230:6: sparse: symbol 'igc_update_stats' was not declared. Should it be static?
   drivers/net/ethernet/intel/igc/igc_main.c:289:9: sparse: incorrect type in initializer (different address spaces) @@    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr @@    got deref] [usertype] <asn:2>*hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:289:9:    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:289:9:    got unsigned char [usertype] *__val
   drivers/net/ethernet/intel/igc/igc_main.c:291:9: sparse: incorrect type in initializer (different address spaces) @@    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr @@    got deref] [usertype] <asn:2>*hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:291:9:    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:291:9:    got unsigned char [usertype] *__val
>> drivers/net/ethernet/intel/igc/igc_main.c:350:5: sparse: symbol 'igc_open' was not declared. Should it be static?
>> drivers/net/ethernet/intel/igc/igc_main.c:379:5: sparse: symbol 'igc_close' was not declared. Should it be static?
   drivers/net/ethernet/intel/igc/igc_main.c:443:31: sparse: incorrect type in initializer (different address spaces) @@    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr @@    got deref] [usertype] <asn:2>*hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:443:31:    expected unsigned char [noderef] [usertype] <asn:2>*hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:443:31:    got unsigned char [usertype] *__val
>> drivers/net/ethernet/intel/igc/igc_main.c:545:21: sparse: incorrect type in assignment (different address spaces) @@    expected unsigned char [usertype] *hw_addr @@    got unsigned char [nounsigned char [usertype] *hw_addr @@
   drivers/net/ethernet/intel/igc/igc_main.c:545:21:    expected unsigned char [usertype] *hw_addr
   drivers/net/ethernet/intel/igc/igc_main.c:545:21:    got unsigned char [noderef] [usertype] <asn:2>*io_addr

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static
  2018-06-24  8:45 [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Sasha Neftin
  2018-06-24 12:33 ` kbuild test robot
@ 2018-06-24 12:34 ` kbuild test robot
  2018-06-28  7:07   ` Neftin, Sasha
  2018-06-27  0:32 ` [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Shannon Nelson
  2 siblings, 1 reply; 8+ messages in thread
From: kbuild test robot @ 2018-06-24 12:34 UTC (permalink / raw)
  To: intel-wired-lan


Fixes: e1df6cb3a70b ("igc: Add netdev")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 igc_main.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 520f49b..32669855 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -34,7 +34,7 @@ static void igc_configure(struct igc_adapter *adapter);
 static void igc_power_down_link(struct igc_adapter *adapter);
 static void igc_set_default_mac_filter(struct igc_adapter *adapter);
 
-void igc_reset(struct igc_adapter *adapter)
+static void igc_reset(struct igc_adapter *adapter)
 {
 	if (!netif_running(adapter->netdev))
 		igc_power_down_link(adapter);
@@ -44,7 +44,7 @@ void igc_reset(struct igc_adapter *adapter)
  *  igc_power_up_link - Power up the phy/serdes link
  *  @adapter: address of board private structure
  **/
-void igc_power_up_link(struct igc_adapter *adapter)
+static void igc_power_up_link(struct igc_adapter *adapter)
 {
 }
 
@@ -144,7 +144,7 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
  *  igc_up - Open the interface and prepare it to handle traffic
  *  @adapter: board private structure
  **/
-int igc_up(struct igc_adapter *adapter)
+static int igc_up(struct igc_adapter *adapter)
 {
 	int i = 0;
 
@@ -163,7 +163,7 @@ int igc_up(struct igc_adapter *adapter)
  *  igc_down - Close the interface
  *  @adapter: board private structure
  **/
-void igc_down(struct igc_adapter *adapter)
+static void igc_down(struct igc_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 	int i = 0;
@@ -227,7 +227,7 @@ static int igc_change_mtu(struct net_device *netdev, int new_mtu)
  *  igc_update_stats - Update the board statistics counters
  *  @adapter: board private structure
  **/
-void igc_update_stats(struct igc_adapter *adapter)
+static void igc_update_stats(struct igc_adapter *adapter)
 {
 }
 
@@ -347,7 +347,7 @@ static int __igc_open(struct net_device *netdev, bool resuming)
 	return E1000_SUCCESS;
 }
 
-int igc_open(struct net_device *netdev)
+static int igc_open(struct net_device *netdev)
 {
 	return __igc_open(netdev, false);
 }
@@ -376,7 +376,7 @@ static int __igc_close(struct net_device *netdev, bool suspending)
 	return 0;
 }
 
-int igc_close(struct net_device *netdev)
+static int igc_close(struct net_device *netdev)
 {
 	if (netif_device_present(netdev) || netdev->dismantle)
 		return __igc_close(netdev, false);

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
  2018-06-24  8:45 [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Sasha Neftin
  2018-06-24 12:33 ` kbuild test robot
  2018-06-24 12:34 ` [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static kbuild test robot
@ 2018-06-27  0:32 ` Shannon Nelson
  2018-07-02 13:05   ` Neftin, Sasha
  2 siblings, 1 reply; 8+ messages in thread
From: Shannon Nelson @ 2018-06-27  0:32 UTC (permalink / raw)
  To: intel-wired-lan

On 6/24/2018 1:45 AM, Sasha Neftin wrote:
> Now that we have the ability to configure the basic settings on the device
> we can start allocating and configuring a netdev for the interface.
> 
> Sasha Neftin (v2):
> added description
> 
> Sasha Neftin (v3):
> minor cosmetic changes
> 
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/e1000_defines.h |  15 +
>   drivers/net/ethernet/intel/igc/e1000_hw.h      |   1 +
>   drivers/net/ethernet/intel/igc/igc.h           |  48 +++
>   drivers/net/ethernet/intel/igc/igc_main.c      | 490 ++++++++++++++++++++++++-
>   4 files changed, 551 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/e1000_defines.h b/drivers/net/ethernet/intel/igc/e1000_defines.h
> index 6831a0864bb4..66b05898eb00 100644
> --- a/drivers/net/ethernet/intel/igc/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igc/e1000_defines.h
> @@ -4,6 +4,8 @@
>   #ifndef _E1000_DEFINES_H_
>   #define _E1000_DEFINES_H_
>   
> +#define E1000_CTRL_EXT_DRV_LOAD         0x10000000 /* Drv loaded bit for FW */
> +
>   #define E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES	0x00C00000
>   /* Priority on PCI. 0=rx,1=fair */
>   #define E1000_CTRL_PRIOR			0x00000004
> @@ -28,6 +30,16 @@
>   #define E1000_PCI_PMCSR			0x44
>   #define E1000_PCI_PMCSR_D3		0x03
>   
> +/* Receive Address
> + * Number of high/low register pairs in the RAR. The RAR (Receive Address
> + * Registers) holds the directed and multicast addresses that we monitor.
> + * Technically, we have 16 spots.  However, we reserve one of these spots
> + * (RAR[15]) for our directed address used by controllers with
> + * manageability enabled, allowing us room for 15 multicast addresses.
> + */
> +#define E1000_RAH_AV		0x80000000 /* Receive descriptor valid */
> +#define E1000_RAH_POOL_1	0x00040000
> +
>   /* Error Codes */
>   #define E1000_SUCCESS				0
>   #define E1000_ERR_NVM				1
> @@ -37,6 +49,9 @@
>   #define E1000_ERR_MAC_INIT			5
>   #define E1000_ERR_RESET				9
>   
> +/* PBA constants */
> +#define E1000_PBA_34K			0x0022
> +
>   /* Device Status */
>   #define E1000_STATUS_FD		0x00000001      /* Full duplex.0=half,1=full */
>   #define E1000_STATUS_LU		0x00000002      /* Link up.0=no,1=link */
> diff --git a/drivers/net/ethernet/intel/igc/e1000_hw.h b/drivers/net/ethernet/intel/igc/e1000_hw.h
> index b8f82f4ba998..6abe722f0bc4 100644
> --- a/drivers/net/ethernet/intel/igc/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igc/e1000_hw.h
> @@ -87,6 +87,7 @@ struct e1000_mac_info {
>   
>   	bool autoneg;
>   	bool autoneg_failed;
> +	bool get_link_status;
>   };
>   
>   struct e1000_bus_info {
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index bd732390bd4b..29df87285b9b 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -28,15 +28,63 @@
>   extern char igc_driver_name[];
>   extern char igc_driver_version[];
>   
> +/* Transmit and receive queues */
> +#define IGC_MAX_RX_QUEUES                 4
> +#define IGC_MAX_TX_QUEUES                 4
> +
> +#define MAX_Q_VECTORS                     10
> +#define MAX_STD_JUMBO_FRAME_SIZE        9216
> +
> +enum e1000_state_t {
> +	 __IGC_TESTING,

This mixing of e1000 and IGC is just wrong...
Can you tell I don't like it?

> +	__IGC_RESETTING,

funky indent

> +	 __IGC_DOWN,
> +	 __IGC_PTP_TX_IN_PROGRESS,
> +};
> +
> +struct igc_q_vector {
> +	struct igc_adapter *adapter;    /* backlink */
> +
> +	struct napi_struct napi;
> +};
> +
> +struct igc_mac_addr {
> +	u8 addr[ETH_ALEN];
> +	u8 queue;
> +	u8 state; /* bitmask */
> +};
> +
> +#define IGC_MAC_STATE_DEFAULT   0x1
> +#define IGC_MAC_STATE_MODIFIED  0x2
> +#define IGC_MAC_STATE_IN_USE    0x4
> +
>   /* Board specific private data structure */
>   struct igc_adapter {
> +	struct net_device *netdev;
> +
> +	unsigned long state;
> +	unsigned int flags;

These state and flags fields should probably be specifically defined as 
u32 or u64.

Is there a reason that state is a long and flags an int?  Is there an 
expectation of different sizes?

> +	unsigned int num_q_vectors;
> +	u16 link_speed;
> +	u16 link_duplex;
> +
> +	u8 port_num;
> +
>   	u8 __iomem *io_addr;
> +	struct work_struct watchdog_task;
> +
> +	int msg_enable;
> +	u32 max_frame_size;
>   
>   	/* OS defined structs */
>   	struct pci_dev *pdev;
>   
>   	/* structs defined in e1000_hw.h */
>   	struct e1000_hw hw;
> +
> +	struct igc_q_vector *q_vector[MAX_Q_VECTORS];
> +
> +	struct igc_mac_addr *mac_table;
>   };
>   
>   #endif /* _IGC_H_ */
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index ec3451dad91e..520f49be8e19 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -3,6 +3,8 @@
>   
>   #include <linux/module.h>
>   #include <linux/types.h>
> +#include <linux/if_vlan.h>
> +#include <linux/aer.h>
>   
>   #include "igc.h"
>   #include "e1000_hw.h"
> @@ -10,6 +12,7 @@
>   #define DRV_VERSION	"0.0.1-k"
>   #define DRV_SUMMARY	"Intel(R) 2.5G Ethernet Linux Driver"
>   
> +static int debug = -1;
>   char igc_driver_name[] = "igc";
>   char igc_driver_version[] = DRV_VERSION;
>   static const char igc_driver_string[] = DRV_SUMMARY;
> @@ -25,8 +28,371 @@ static const struct pci_device_id igc_pci_tbl[] = {
>   
>   MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
>   
> -/* Forward declaration */
> +/* forward declaration */
>   static int igc_sw_init(struct igc_adapter *);
> +static void igc_configure(struct igc_adapter *adapter);
> +static void igc_power_down_link(struct igc_adapter *adapter);
> +static void igc_set_default_mac_filter(struct igc_adapter *adapter);

If you change the order of the functions in the file you shouldn't need 
the forward decl crutch.

> +
> +void igc_reset(struct igc_adapter *adapter)
> +{
> +	if (!netif_running(adapter->netdev))
> +		igc_power_down_link(adapter);
> +}
> +
> +/**
> + *  igc_power_up_link - Power up the phy/serdes link
> + *  @adapter: address of board private structure
> + **/
> +void igc_power_up_link(struct igc_adapter *adapter)
> +{
> +}
> +
> +/**
> + *  igc_power_down_link - Power down the phy/serdes link
> + *  @adapter: address of board private structure
> + **/
> +static void igc_power_down_link(struct igc_adapter *adapter)
> +{
> +}
> +
> +/**
> + *  igc_release_hw_control - release control of the h/w to f/w
> + *  @adapter: address of board private structure
> + *
> + *  igc_release_hw_control resets CTRL_EXT:DRV_LOAD bit.
> + *  For ASF and Pass Through versions of f/w this means that the
> + *  driver is no longer loaded.
> + **/
> +static void igc_release_hw_control(struct igc_adapter *adapter)
> +{
> +	struct e1000_hw *hw = &adapter->hw;
> +	u32 ctrl_ext;
> +
> +	/* Let firmware take over control of h/w */
> +	ctrl_ext = rd32(E1000_CTRL_EXT);
> +	wr32(E1000_CTRL_EXT,
> +	     ctrl_ext & ~E1000_CTRL_EXT_DRV_LOAD);
> +}
> +
> +/**
> + *  igc_get_hw_control - get control of the h/w from f/w
> + *  @adapter: address of board private structure
> + *
> + *  igc_get_hw_control sets CTRL_EXT:DRV_LOAD bit.
> + *  For ASF and Pass Through versions of f/w this means that
> + *  the driver is loaded.
> + **/
> +static void igc_get_hw_control(struct igc_adapter *adapter)
> +{
> +	struct e1000_hw *hw = &adapter->hw;
> +	u32 ctrl_ext;
> +
> +	/* Let firmware know the driver has taken over */
> +	ctrl_ext = rd32(E1000_CTRL_EXT);
> +	wr32(E1000_CTRL_EXT,
> +	     ctrl_ext | E1000_CTRL_EXT_DRV_LOAD);
> +}
> +
> +/**
> + *  igc_set_mac - Change the Ethernet Address of the NIC
> + *  @netdev: network interface device structure
> + *  @p: pointer to an address structure
> + *
> + *  Returns 0 on success, negative on failure
> + **/
> +static int igc_set_mac(struct net_device *netdev, void *p)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct e1000_hw *hw = &adapter->hw;
> +	struct sockaddr *addr = p;
> +
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;
> +
> +	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
> +	memcpy(hw->mac.addr, addr->sa_data, netdev->addr_len);
> +
> +	/* set the correct pool for the new PF MAC address in entry 0 */
> +	igc_set_default_mac_filter(adapter);
> +
> +	return 0;
> +}
> +
> +static netdev_tx_t igc_xmit_frame(struct sk_buff *skb,
> +				  struct net_device *netdev)
> +{
> +	dev_kfree_skb_any(skb);
> +	return NETDEV_TX_OK;
> +}
> +
> +/**
> + *  igc_ioctl - I/O control method
> + *  @netdev: network interface device structure
> + *  @ifreq: frequency
> + *  @cmd: command
> + **/
> +static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> +{
> +	switch (cmd) {
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +/**
> + *  igc_up - Open the interface and prepare it to handle traffic
> + *  @adapter: board private structure
> + **/
> +int igc_up(struct igc_adapter *adapter)
> +{
> +	int i = 0;
> +
> +	/* hardware has been reset, we need to reload some things */
> +	igc_configure(adapter);
> +
> +	clear_bit(__IGC_DOWN, &adapter->state);
> +
> +	for (i = 0; i < adapter->num_q_vectors; i++)
> +		napi_enable(&adapter->q_vector[i]->napi);
> +
> +	return 0;
> +}
> +
> +/**
> + *  igc_down - Close the interface
> + *  @adapter: board private structure
> + **/
> +void igc_down(struct igc_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	int i = 0;
> +
> +	set_bit(__IGC_DOWN, &adapter->state);
> +
> +	/* set trans_start so we don't get spurious watchdogs during reset */
> +	netif_trans_update(netdev);
> +
> +	netif_carrier_off(netdev);
> +	netif_tx_stop_all_queues(netdev);
> +
> +	for (i = 0; i < adapter->num_q_vectors; i++)
> +		napi_disable(&adapter->q_vector[i]->napi);
> +
> +	adapter->link_speed = 0;
> +	adapter->link_duplex = 0;
> +}
> +
> +/**
> + *  igc_change_mtu - Change the Maximum Transfer Unit
> + *  @netdev: network interface device structure
> + *  @new_mtu: new value for maximum frame size
> + *
> + *  Returns 0 on success, negative on failure
> + **/
> +static int igc_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct pci_dev *pdev = adapter->pdev;
> +	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +
> +	/* adjust max frame to be at least the size of a standard frame */
> +	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
> +		max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
> +
> +	while (test_and_set_bit(__IGC_RESETTING, &adapter->state))
> +		usleep_range(1000, 2000);
> +
> +	/* igc_down has a dependency on max_frame_size */
> +	adapter->max_frame_size = max_frame;
> +
> +	if (netif_running(netdev))
> +		igc_down(adapter);
> +
> +	dev_info(&pdev->dev, "changing MTU from %d to %d\n",
> +		 netdev->mtu, new_mtu);
> +	netdev->mtu = new_mtu;
> +
> +	if (netif_running(netdev))
> +		igc_up(adapter);
> +	else
> +		igc_reset(adapter);
> +
> +	clear_bit(__IGC_RESETTING, &adapter->state);
> +
> +	return 0;
> +}
> +
> +/**
> + *  igc_update_stats - Update the board statistics counters
> + *  @adapter: board private structure
> + **/
> +void igc_update_stats(struct igc_adapter *adapter)
> +{
> +}
> +
> +/**
> + *  igc_get_stats - Get System Network Statistics
> + *  @netdev: network interface device structure
> + *
> + *  Returns the address of the device statistics structure.
> + *  The statistics are updated here and also from the timer callback.
> + **/
> +static struct net_device_stats *igc_get_stats(struct net_device *netdev)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +
> +	if (!test_bit(__IGC_RESETTING, &adapter->state))
> +		igc_update_stats(adapter);
> +
> +	/* only return the current stats */
> +	return &netdev->stats;
> +}
> +
> +/**
> + *  igc_configure - configure the hardware for RX and TX
> + *  @adapter: private board structure
> + **/
> +static void igc_configure(struct igc_adapter *adapter)
> +{
> +	igc_get_hw_control(adapter);
> +}
> +
> +/**
> + *  igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
> + *  @adapter: Pointer to adapter structure
> + *  @index: Index of the RAR entry which need to be synced with MAC table
> + **/
> +static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
> +{
> +	struct e1000_hw *hw = &adapter->hw;
> +	u32 rar_low, rar_high;
> +	u8 *addr = adapter->mac_table[index].addr;
> +
> +	/* HW expects these to be in network order when they are plugged
> +	 * into the registers which are little endian.  In order to guarantee
> +	 * that ordering we need to do an leXX_to_cpup here in order to be
> +	 * ready for the byteswap that occurs with writel
> +	 */
> +	rar_low = le32_to_cpup((__le32 *)(addr));
> +	rar_high = le16_to_cpup((__le16 *)(addr + 4));
> +
> +	/* Indicate to hardware the Address is Valid. */
> +	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
> +		if (is_valid_ether_addr(addr))
> +			rar_high |= E1000_RAH_AV;
> +
> +		rar_high |= E1000_RAH_POOL_1 <<
> +			adapter->mac_table[index].queue;
> +	}
> +
> +	wr32(E1000_RAL(index), rar_low);
> +	wrfl();
> +	wr32(E1000_RAH(index), rar_high);
> +	wrfl();
> +}
> +
> +/* Set default MAC address for the PF in the first RAR entry */
> +static void igc_set_default_mac_filter(struct igc_adapter *adapter)
> +{
> +	struct igc_mac_addr *mac_table = &adapter->mac_table[0];
> +
> +	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
> +	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
> +
> +	igc_rar_set_index(adapter, 0);
> +}
> +
> +/**
> + * igc_open - Called when a network interface is made active
> + * @netdev: network interface device structure
> + *
> + * Returns 0 on success, negative value on failure
> + *
> + * The open entry point is called when a network interface is made
> + * active by the system (IFF_UP).  At this point all resources needed
> + * for transmit and receive operations are allocated, the interrupt
> + * handler is registered with the OS, the watchdog timer is started,
> + * and the stack is notified that the interface is ready.
> + **/
> +static int __igc_open(struct net_device *netdev, bool resuming)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct e1000_hw *hw = &adapter->hw;
> +	int i = 0;
> +
> +	/* disallow open during test */
> +
> +	if (test_bit(__IGC_TESTING, &adapter->state)) {
> +		WARN_ON(resuming);
> +		return -EBUSY;
> +	}
> +
> +	netif_carrier_off(netdev);
> +
> +	igc_power_up_link(adapter);
> +
> +	igc_configure(adapter);
> +
> +	/* From here on the code is the same as igc_up() */

Then why not use igc_up()?

> +	clear_bit(__IGC_DOWN, &adapter->state);
> +
> +	for (i = 0; i < adapter->num_q_vectors; i++)
> +		napi_enable(&adapter->q_vector[i]->napi);
> +
> +	/* start the watchdog. */
> +	hw->mac.get_link_status = 1;
> +	schedule_work(&adapter->watchdog_task);
> +
> +	return E1000_SUCCESS;
> +}
> +
> +int igc_open(struct net_device *netdev)
> +{
> +	return __igc_open(netdev, false);
> +}
> +
> +/**
> + * igc_close - Disables a network interface
> + * @netdev: network interface device structure
> + *
> + * Returns 0, this is not allowed to fail
> + *
> + * The close entry point is called when an interface is de-activated
> + * by the OS.  The hardware is still under the driver's control, but
> + * needs to be disabled.  A global MAC reset is issued to stop the
> + * hardware, and all transmit and receive resources are freed.
> + **/
> +static int __igc_close(struct net_device *netdev, bool suspending)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +
> +	WARN_ON(test_bit(__IGC_RESETTING, &adapter->state));
> +
> +	igc_down(adapter);
> +
> +	igc_release_hw_control(adapter);
> +
> +	return 0;
> +}
> +
> +int igc_close(struct net_device *netdev)
> +{
> +	if (netif_device_present(netdev) || netdev->dismantle)
> +		return __igc_close(netdev, false);
> +	return 0;
> +}
> +
> +static const struct net_device_ops igc_netdev_ops = {
> +	.ndo_open               = igc_open,
> +	.ndo_stop               = igc_close,
> +	.ndo_start_xmit         = igc_xmit_frame,
> +	.ndo_set_mac_address    = igc_set_mac,
> +	.ndo_change_mtu         = igc_change_mtu,
> +	.ndo_get_stats          = igc_get_stats,
> +	.ndo_do_ioctl           = igc_ioctl,
> +
> +};
>   
>   /* PCIe configuration access */
>   void igc_read_pci_cfg(struct e1000_hw *hw, u32 reg, u16 *value)
> @@ -73,6 +439,7 @@ s32 igc_write_pcie_cap_reg(struct e1000_hw *hw, u32 reg, u16 *value)
>   
>   u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>   {
> +	struct igc_adapter *igc = container_of(hw, struct igc_adapter, hw);
>   	u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
>   	u32 value = 0;
>   
> @@ -82,8 +449,13 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>   	value = readl(&hw_addr[reg]);
>   
>   	/* reads should not return all F's */
> -	if (!(~value) && (!reg || !(~readl(hw_addr))))
> +	if (!(~value) && (!reg || !(~readl(hw_addr)))) {
> +		struct net_device *netdev = igc->netdev;
> +
>   		hw->hw_addr = NULL;
> +		netif_device_detach(netdev);
> +		netdev_err(netdev, "PCIe link lost, device now detached\n");
> +	}
>   
>   	return value;
>   }
> @@ -102,6 +474,7 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>   static int igc_probe(struct pci_dev *pdev,
>   		     const struct pci_device_id *ent)
>   {
> +	struct net_device *netdev;

Reverse xmas tree

>   	struct igc_adapter *adapter;
>   	struct e1000_hw *hw;
>   	int err, pci_using_dac;
> @@ -136,8 +509,56 @@ static int igc_probe(struct pci_dev *pdev,
>   	if (err)
>   		goto err_pci_reg;
>   
> +	pci_enable_pcie_error_reporting(pdev);
> +
>   	pci_set_master(pdev);
> -	pci_save_state(pdev);
> +
> +	err = -ENOMEM;
> +	netdev = alloc_etherdev_mq(sizeof(struct igc_adapter),
> +				   IGC_MAX_TX_QUEUES);
> +
> +	if (!netdev)
> +		goto err_alloc_etherdev;
> +
> +	SET_NETDEV_DEV(netdev, &pdev->dev);
> +
> +	pci_set_drvdata(pdev, netdev);
> +	adapter = netdev_priv(netdev);
> +	adapter->netdev = netdev;
> +	adapter->pdev = pdev;
> +	hw = &adapter->hw;
> +	hw->back = adapter;
> +	adapter->port_num = hw->bus.func;
> +	adapter->msg_enable = GENMASK(debug - 1, 0);
> +
> +	err = pci_save_state(pdev);
> +	if (err)
> +		goto err_ioremap;
> +
> +	err = -EIO;
> +	adapter->io_addr = ioremap(pci_resource_start(pdev, 0),
> +				   pci_resource_len(pdev, 0));
> +	if (!adapter->io_addr)
> +		goto err_ioremap;
> +
> +	/* hw->hw_addr can be zeroed, so use adapter->io_addr for unmap */
> +	hw->hw_addr = adapter->io_addr;
> +
> +	netdev->netdev_ops = &igc_netdev_ops;
> +
> +	netdev->watchdog_timeo = 5 * HZ;
> +
> +	strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
> +
> +	netdev->mem_start = pci_resource_start(pdev, 0);
> +	netdev->mem_end = pci_resource_end(pdev, 0);
> +
> +	/* PCI config space info */
> +	hw->vendor_id = pdev->vendor;
> +	hw->device_id = pdev->device;
> +	hw->revision_id = pdev->revision;
> +	hw->subsystem_vendor_id = pdev->subsystem_vendor;
> +	hw->subsystem_device_id = pdev->subsystem_device;
>   
>   	/* setup the private structure */
>   	err = igc_sw_init(adapter);
> @@ -146,9 +567,49 @@ static int igc_probe(struct pci_dev *pdev,
>   
>   	igc_get_bus_info_pcie(hw);
>   
> +	/* MTU range: 68 - 9216 */
> +	netdev->min_mtu = ETH_MIN_MTU;
> +	netdev->max_mtu = MAX_STD_JUMBO_FRAME_SIZE;
> +
> +	/* reset the hardware with the new settings */
> +	igc_reset(adapter);
> +
> +	/* let the f/w know that the h/w is now under the control of the
> +	 * driver.
> +	 */
> +	igc_get_hw_control(adapter);
> +
> +	strncpy(netdev->name, "eth%d", IFNAMSIZ);

You're stomping on what you just set a few lines earlier.  Of course, 
some might argue that you shouldn't put anything here at all and let the 
system name the device.

> +	err = register_netdev(netdev);
> +	if (err)
> +		goto err_register;
> +
> +	 /* carrier off reporting is important to ethtool even BEFORE open */
> +	netif_carrier_off(netdev);
> +
> +	dev_info(&pdev->dev, "@SUMMARY@");
> +	/* print bus type/speed/width info */
> +	dev_info(&pdev->dev, "%s: (PCIe:%s:%s) ",
> +		 netdev->name,
> +		 ((hw->bus.speed == e1000_bus_speed_2500) ? "2.5GT/s" :
> +		 (hw->bus.speed == e1000_bus_speed_5000) ? "5.0GT/s" :
> +		 "unknown"),
> +		 ((hw->bus.width == e1000_bus_width_pcie_x4) ? "Width x4" :
> +		 (hw->bus.width == e1000_bus_width_pcie_x2) ? "Width x2" :
> +		 (hw->bus.width == e1000_bus_width_pcie_x1) ? "Width x1" :
> +		 "unknown"));

How about using the new pcie_print_link_status()?

> +	netdev_info(netdev, "MAC: %pM\n", netdev->dev_addr);
> +
>   	return 0;
>   
> +err_register:
> +	igc_release_hw_control(adapter);
>   err_sw_init:
> +err_ioremap:
> +	free_netdev(netdev);
> +err_alloc_etherdev:
> +	pci_release_selected_regions(pdev,
> +				     pci_select_bars(pdev, IORESOURCE_MEM));
>   err_pci_reg:
>   err_dma:
>   	pci_disable_device(pdev);
> @@ -166,9 +627,22 @@ static int igc_probe(struct pci_dev *pdev,
>    **/
>   static void igc_remove(struct pci_dev *pdev)
>   {
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +
> +	set_bit(__IGC_DOWN, &adapter->state);
> +	flush_scheduled_work();
> +
> +	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
> +	 * would have already happened in close and is redundant.
> +	 */
> +	igc_release_hw_control(adapter);
> +	unregister_netdev(netdev);
> +
>   	pci_release_selected_regions(pdev,
>   				     pci_select_bars(pdev, IORESOURCE_MEM));
>   
> +	free_netdev(netdev);
>   	pci_disable_device(pdev);
>   }
>   
> @@ -190,6 +664,7 @@ static struct pci_driver igc_driver = {
>   static int igc_sw_init(struct igc_adapter *adapter)
>   {
>   	struct e1000_hw *hw = &adapter->hw;
> +	struct net_device *netdev = adapter->netdev;
>   	struct pci_dev *pdev = adapter->pdev;
>   
>   	/* PCI config space info */
> @@ -203,6 +678,12 @@ static int igc_sw_init(struct igc_adapter *adapter)
>   
>   	pci_read_config_word(pdev, PCI_COMMAND, &hw->bus.pci_cmd_word);
>   
> +	/* set default work limits */
> +	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
> +					VLAN_HLEN;
> +
> +	set_bit(__IGC_DOWN, &adapter->state);
> +
>   	return 0;
>   }
>   
> @@ -244,4 +725,7 @@ MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
>   MODULE_DESCRIPTION(DRV_SUMMARY);
>   MODULE_LICENSE("GPL");
>   MODULE_VERSION(DRV_VERSION);
> +
> +module_param(debug, int, 0);
> +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>   /* igc_main.c */
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static
  2018-06-24 12:34 ` [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static kbuild test robot
@ 2018-06-28  7:07   ` Neftin, Sasha
  0 siblings, 0 replies; 8+ messages in thread
From: Neftin, Sasha @ 2018-06-28  7:07 UTC (permalink / raw)
  To: intel-wired-lan

On 6/24/2018 15:34, kbuild test robot wrote:
> 
> Fixes: e1df6cb3a70b ("igc: Add netdev")
> Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
> ---
>   igc_main.c |   14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 520f49b..32669855 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -34,7 +34,7 @@ static void igc_configure(struct igc_adapter *adapter);
>   static void igc_power_down_link(struct igc_adapter *adapter);
>   static void igc_set_default_mac_filter(struct igc_adapter *adapter);
>   
> -void igc_reset(struct igc_adapter *adapter)
> +static void igc_reset(struct igc_adapter *adapter)
>   {
>   	if (!netif_running(adapter->netdev))
>   		igc_power_down_link(adapter);
> @@ -44,7 +44,7 @@ void igc_reset(struct igc_adapter *adapter)
>    *  igc_power_up_link - Power up the phy/serdes link
>    *  @adapter: address of board private structure
>    **/
> -void igc_power_up_link(struct igc_adapter *adapter)
> +static void igc_power_up_link(struct igc_adapter *adapter)
>   {
>   }
>   
> @@ -144,7 +144,7 @@ static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>    *  igc_up - Open the interface and prepare it to handle traffic
>    *  @adapter: board private structure
>    **/
> -int igc_up(struct igc_adapter *adapter)
> +static int igc_up(struct igc_adapter *adapter)
>   {
>   	int i = 0;
>   
> @@ -163,7 +163,7 @@ int igc_up(struct igc_adapter *adapter)
>    *  igc_down - Close the interface
>    *  @adapter: board private structure
>    **/
> -void igc_down(struct igc_adapter *adapter)
> +static void igc_down(struct igc_adapter *adapter)
>   {
>   	struct net_device *netdev = adapter->netdev;
>   	int i = 0;
> @@ -227,7 +227,7 @@ static int igc_change_mtu(struct net_device *netdev, int new_mtu)
>    *  igc_update_stats - Update the board statistics counters
>    *  @adapter: board private structure
>    **/
> -void igc_update_stats(struct igc_adapter *adapter)
> +static void igc_update_stats(struct igc_adapter *adapter)
>   {
>   }
>   
> @@ -347,7 +347,7 @@ static int __igc_open(struct net_device *netdev, bool resuming)
>   	return E1000_SUCCESS;
>   }
>   
> -int igc_open(struct net_device *netdev)
> +static int igc_open(struct net_device *netdev)
>   {
>   	return __igc_open(netdev, false);
>   }
> @@ -376,7 +376,7 @@ static int __igc_close(struct net_device *netdev, bool suspending)
>   	return 0;
>   }
>   
> -int igc_close(struct net_device *netdev)
> +static int igc_close(struct net_device *netdev)
>   {
>   	if (netif_device_present(netdev) || netdev->dismantle)
>   		return __igc_close(netdev, false);
> 
good. will be applied to v4.
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
  2018-06-27  0:32 ` [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Shannon Nelson
@ 2018-07-02 13:05   ` Neftin, Sasha
  2018-07-02 16:40     ` Shannon Nelson
  0 siblings, 1 reply; 8+ messages in thread
From: Neftin, Sasha @ 2018-07-02 13:05 UTC (permalink / raw)
  To: intel-wired-lan

On 6/27/2018 03:32, Shannon Nelson wrote:
> On 6/24/2018 1:45 AM, Sasha Neftin wrote:
>> Now that we have the ability to configure the basic settings on the 
>> device
>> we can start allocating and configuring a netdev for the interface.
>>
>> Sasha Neftin (v2):
>> added description
>>
>> Sasha Neftin (v3):
>> minor cosmetic changes
>>
>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> ---
>> ? drivers/net/ethernet/intel/igc/e1000_defines.h |? 15 +
>> ? drivers/net/ethernet/intel/igc/e1000_hw.h????? |?? 1 +
>> ? drivers/net/ethernet/intel/igc/igc.h?????????? |? 48 +++
>> ? drivers/net/ethernet/intel/igc/igc_main.c????? | 490 
>> ++++++++++++++++++++++++-
>> ? 4 files changed, 551 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/e1000_defines.h 
>> b/drivers/net/ethernet/intel/igc/e1000_defines.h
>> index 6831a0864bb4..66b05898eb00 100644
>> --- a/drivers/net/ethernet/intel/igc/e1000_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/e1000_defines.h
>> @@ -4,6 +4,8 @@
>> ? #ifndef _E1000_DEFINES_H_
>> ? #define _E1000_DEFINES_H_
>> +#define E1000_CTRL_EXT_DRV_LOAD???????? 0x10000000 /* Drv loaded bit 
>> for FW */
>> +
>> ? #define E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES??? 0x00C00000
>> ? /* Priority on PCI. 0=rx,1=fair */
>> ? #define E1000_CTRL_PRIOR??????????? 0x00000004
>> @@ -28,6 +30,16 @@
>> ? #define E1000_PCI_PMCSR??????????? 0x44
>> ? #define E1000_PCI_PMCSR_D3??????? 0x03
>> +/* Receive Address
>> + * Number of high/low register pairs in the RAR. The RAR (Receive 
>> Address
>> + * Registers) holds the directed and multicast addresses that we 
>> monitor.
>> + * Technically, we have 16 spots.? However, we reserve one of these 
>> spots
>> + * (RAR[15]) for our directed address used by controllers with
>> + * manageability enabled, allowing us room for 15 multicast addresses.
>> + */
>> +#define E1000_RAH_AV??????? 0x80000000 /* Receive descriptor valid */
>> +#define E1000_RAH_POOL_1??? 0x00040000
>> +
>> ? /* Error Codes */
>> ? #define E1000_SUCCESS??????????????? 0
>> ? #define E1000_ERR_NVM??????????????? 1
>> @@ -37,6 +49,9 @@
>> ? #define E1000_ERR_MAC_INIT??????????? 5
>> ? #define E1000_ERR_RESET??????????????? 9
>> +/* PBA constants */
>> +#define E1000_PBA_34K??????????? 0x0022
>> +
>> ? /* Device Status */
>> ? #define E1000_STATUS_FD??????? 0x00000001????? /* Full 
>> duplex.0=half,1=full */
>> ? #define E1000_STATUS_LU??????? 0x00000002????? /* Link 
>> up.0=no,1=link */
>> diff --git a/drivers/net/ethernet/intel/igc/e1000_hw.h 
>> b/drivers/net/ethernet/intel/igc/e1000_hw.h
>> index b8f82f4ba998..6abe722f0bc4 100644
>> --- a/drivers/net/ethernet/intel/igc/e1000_hw.h
>> +++ b/drivers/net/ethernet/intel/igc/e1000_hw.h
>> @@ -87,6 +87,7 @@ struct e1000_mac_info {
>> ????? bool autoneg;
>> ????? bool autoneg_failed;
>> +??? bool get_link_status;
>> ? };
>> ? struct e1000_bus_info {
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h 
>> b/drivers/net/ethernet/intel/igc/igc.h
>> index bd732390bd4b..29df87285b9b 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -28,15 +28,63 @@
>> ? extern char igc_driver_name[];
>> ? extern char igc_driver_version[];
>> +/* Transmit and receive queues */
>> +#define IGC_MAX_RX_QUEUES???????????????? 4
>> +#define IGC_MAX_TX_QUEUES???????????????? 4
>> +
>> +#define MAX_Q_VECTORS???????????????????? 10
>> +#define MAX_STD_JUMBO_FRAME_SIZE??????? 9216
>> +
>> +enum e1000_state_t {
>> +???? __IGC_TESTING,
> 
> This mixing of e1000 and IGC is just wrong...
> Can you tell I don't like it?
> 
Yes, you are right, I agree. Fix will be applied to v4.
>> +??? __IGC_RESETTING,
> 
> funky inden >
Fix will be applied to v4.

>> +???? __IGC_DOWN,
>> +???? __IGC_PTP_TX_IN_PROGRESS,
>> +};
>> +
>> +struct igc_q_vector {
>> +??? struct igc_adapter *adapter;??? /* backlink */
>> +
>> +??? struct napi_struct napi;
>> +};
>> +
>> +struct igc_mac_addr {
>> +??? u8 addr[ETH_ALEN];
>> +??? u8 queue;
>> +??? u8 state; /* bitmask */
>> +};
>> +
>> +#define IGC_MAC_STATE_DEFAULT?? 0x1
>> +#define IGC_MAC_STATE_MODIFIED? 0x2
>> +#define IGC_MAC_STATE_IN_USE??? 0x4
>> +
>> ? /* Board specific private data structure */
>> ? struct igc_adapter {
>> +??? struct net_device *netdev;
>> +
>> +??? unsigned long state;
>> +??? unsigned int flags;
> 
> These state and flags fields should probably be specifically defined as 
> u32 or u64.
> 
Let me do not agree with you here. I've a two arguments:
1. general set_bit/clear_bit methods is used this type and do not allow 
pass another type. Actually we use set_bit/clear_bit a lot of.
2. i prefer stay consistently with another driver where is used same value.
> Is there a reason that state is a long and flags an int?? Is there an 
> expectation of different sizes?
> 
>> +??? unsigned int num_q_vectors;
>> +??? u16 link_speed;
>> +??? u16 link_duplex;
>> +
>> +??? u8 port_num;
>> +
>> ????? u8 __iomem *io_addr;
>> +??? struct work_struct watchdog_task;
>> +
>> +??? int msg_enable;
>> +??? u32 max_frame_size;
>> ????? /* OS defined structs */
>> ????? struct pci_dev *pdev;
>> ????? /* structs defined in e1000_hw.h */
>> ????? struct e1000_hw hw;
>> +
>> +??? struct igc_q_vector *q_vector[MAX_Q_VECTORS];
>> +
>> +??? struct igc_mac_addr *mac_table;
>> ? };
>> ? #endif /* _IGC_H_ */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c 
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index ec3451dad91e..520f49be8e19 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -3,6 +3,8 @@
>> ? #include <linux/module.h>
>> ? #include <linux/types.h>
>> +#include <linux/if_vlan.h>
>> +#include <linux/aer.h>
>> ? #include "igc.h"
>> ? #include "e1000_hw.h"
>> @@ -10,6 +12,7 @@
>> ? #define DRV_VERSION??? "0.0.1-k"
>> ? #define DRV_SUMMARY??? "Intel(R) 2.5G Ethernet Linux Driver"
>> +static int debug = -1;
>> ? char igc_driver_name[] = "igc";
>> ? char igc_driver_version[] = DRV_VERSION;
>> ? static const char igc_driver_string[] = DRV_SUMMARY;
>> @@ -25,8 +28,371 @@ static const struct pci_device_id igc_pci_tbl[] = {
>> ? MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
>> -/* Forward declaration */
>> +/* forward declaration */
>> ? static int igc_sw_init(struct igc_adapter *);
>> +static void igc_configure(struct igc_adapter *adapter);
>> +static void igc_power_down_link(struct igc_adapter *adapter);
>> +static void igc_set_default_mac_filter(struct igc_adapter *adapter);
> 
> If you change the order of the functions in the file you shouldn't need 
> the forward decl crutch.
> 
how for example? I am afraid from a mess.
>> +
>> +void igc_reset(struct igc_adapter *adapter)
>> +{
>> +??? if (!netif_running(adapter->netdev))
>> +??????? igc_power_down_link(adapter);
>> +}
>> +
>> +/**
>> + *? igc_power_up_link - Power up the phy/serdes link
>> + *? @adapter: address of board private structure
>> + **/
>> +void igc_power_up_link(struct igc_adapter *adapter)
>> +{
>> +}
>> +
>> +/**
>> + *? igc_power_down_link - Power down the phy/serdes link
>> + *? @adapter: address of board private structure
>> + **/
>> +static void igc_power_down_link(struct igc_adapter *adapter)
>> +{
>> +}
>> +
>> +/**
>> + *? igc_release_hw_control - release control of the h/w to f/w
>> + *? @adapter: address of board private structure
>> + *
>> + *? igc_release_hw_control resets CTRL_EXT:DRV_LOAD bit.
>> + *? For ASF and Pass Through versions of f/w this means that the
>> + *? driver is no longer loaded.
>> + **/
>> +static void igc_release_hw_control(struct igc_adapter *adapter)
>> +{
>> +??? struct e1000_hw *hw = &adapter->hw;
>> +??? u32 ctrl_ext;
>> +
>> +??? /* Let firmware take over control of h/w */
>> +??? ctrl_ext = rd32(E1000_CTRL_EXT);
>> +??? wr32(E1000_CTRL_EXT,
>> +???????? ctrl_ext & ~E1000_CTRL_EXT_DRV_LOAD);
>> +}
>> +
>> +/**
>> + *? igc_get_hw_control - get control of the h/w from f/w
>> + *? @adapter: address of board private structure
>> + *
>> + *? igc_get_hw_control sets CTRL_EXT:DRV_LOAD bit.
>> + *? For ASF and Pass Through versions of f/w this means that
>> + *? the driver is loaded.
>> + **/
>> +static void igc_get_hw_control(struct igc_adapter *adapter)
>> +{
>> +??? struct e1000_hw *hw = &adapter->hw;
>> +??? u32 ctrl_ext;
>> +
>> +??? /* Let firmware know the driver has taken over */
>> +??? ctrl_ext = rd32(E1000_CTRL_EXT);
>> +??? wr32(E1000_CTRL_EXT,
>> +???????? ctrl_ext | E1000_CTRL_EXT_DRV_LOAD);
>> +}
>> +
>> +/**
>> + *? igc_set_mac - Change the Ethernet Address of the NIC
>> + *? @netdev: network interface device structure
>> + *? @p: pointer to an address structure
>> + *
>> + *? Returns 0 on success, negative on failure
>> + **/
>> +static int igc_set_mac(struct net_device *netdev, void *p)
>> +{
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +??? struct e1000_hw *hw = &adapter->hw;
>> +??? struct sockaddr *addr = p;
>> +
>> +??? if (!is_valid_ether_addr(addr->sa_data))
>> +??????? return -EADDRNOTAVAIL;
>> +
>> +??? memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
>> +??? memcpy(hw->mac.addr, addr->sa_data, netdev->addr_len);
>> +
>> +??? /* set the correct pool for the new PF MAC address in entry 0 */
>> +??? igc_set_default_mac_filter(adapter);
>> +
>> +??? return 0;
>> +}
>> +
>> +static netdev_tx_t igc_xmit_frame(struct sk_buff *skb,
>> +????????????????? struct net_device *netdev)
>> +{
>> +??? dev_kfree_skb_any(skb);
>> +??? return NETDEV_TX_OK;
>> +}
>> +
>> +/**
>> + *? igc_ioctl - I/O control method
>> + *? @netdev: network interface device structure
>> + *? @ifreq: frequency
>> + *? @cmd: command
>> + **/
>> +static int igc_ioctl(struct net_device *netdev, struct ifreq *ifr, 
>> int cmd)
>> +{
>> +??? switch (cmd) {
>> +??? default:
>> +??????? return -EOPNOTSUPP;
>> +??? }
>> +}
>> +
>> +/**
>> + *? igc_up - Open the interface and prepare it to handle traffic
>> + *? @adapter: board private structure
>> + **/
>> +int igc_up(struct igc_adapter *adapter)
>> +{
>> +??? int i = 0;
>> +
>> +??? /* hardware has been reset, we need to reload some things */
>> +??? igc_configure(adapter);
>> +
>> +??? clear_bit(__IGC_DOWN, &adapter->state);
>> +
>> +??? for (i = 0; i < adapter->num_q_vectors; i++)
>> +??????? napi_enable(&adapter->q_vector[i]->napi);
>> +
>> +??? return 0;
>> +}
>> +
>> +/**
>> + *? igc_down - Close the interface
>> + *? @adapter: board private structure
>> + **/
>> +void igc_down(struct igc_adapter *adapter)
>> +{
>> +??? struct net_device *netdev = adapter->netdev;
>> +??? int i = 0;
>> +
>> +??? set_bit(__IGC_DOWN, &adapter->state);
>> +
>> +??? /* set trans_start so we don't get spurious watchdogs during 
>> reset */
>> +??? netif_trans_update(netdev);
>> +
>> +??? netif_carrier_off(netdev);
>> +??? netif_tx_stop_all_queues(netdev);
>> +
>> +??? for (i = 0; i < adapter->num_q_vectors; i++)
>> +??????? napi_disable(&adapter->q_vector[i]->napi);
>> +
>> +??? adapter->link_speed = 0;
>> +??? adapter->link_duplex = 0;
>> +}
>> +
>> +/**
>> + *? igc_change_mtu - Change the Maximum Transfer Unit
>> + *? @netdev: network interface device structure
>> + *? @new_mtu: new value for maximum frame size
>> + *
>> + *? Returns 0 on success, negative on failure
>> + **/
>> +static int igc_change_mtu(struct net_device *netdev, int new_mtu)
>> +{
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +??? struct pci_dev *pdev = adapter->pdev;
>> +??? int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
>> +
>> +??? /* adjust max frame to be at least the size of a standard frame */
>> +??? if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
>> +??????? max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
>> +
>> +??? while (test_and_set_bit(__IGC_RESETTING, &adapter->state))
>> +??????? usleep_range(1000, 2000);
>> +
>> +??? /* igc_down has a dependency on max_frame_size */
>> +??? adapter->max_frame_size = max_frame;
>> +
>> +??? if (netif_running(netdev))
>> +??????? igc_down(adapter);
>> +
>> +??? dev_info(&pdev->dev, "changing MTU from %d to %d\n",
>> +???????? netdev->mtu, new_mtu);
>> +??? netdev->mtu = new_mtu;
>> +
>> +??? if (netif_running(netdev))
>> +??????? igc_up(adapter);
>> +??? else
>> +??????? igc_reset(adapter);
>> +
>> +??? clear_bit(__IGC_RESETTING, &adapter->state);
>> +
>> +??? return 0;
>> +}
>> +
>> +/**
>> + *? igc_update_stats - Update the board statistics counters
>> + *? @adapter: board private structure
>> + **/
>> +void igc_update_stats(struct igc_adapter *adapter)
>> +{
>> +}
>> +
>> +/**
>> + *? igc_get_stats - Get System Network Statistics
>> + *? @netdev: network interface device structure
>> + *
>> + *? Returns the address of the device statistics structure.
>> + *? The statistics are updated here and also from the timer callback.
>> + **/
>> +static struct net_device_stats *igc_get_stats(struct net_device *netdev)
>> +{
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +
>> +??? if (!test_bit(__IGC_RESETTING, &adapter->state))
>> +??????? igc_update_stats(adapter);
>> +
>> +??? /* only return the current stats */
>> +??? return &netdev->stats;
>> +}
>> +
>> +/**
>> + *? igc_configure - configure the hardware for RX and TX
>> + *? @adapter: private board structure
>> + **/
>> +static void igc_configure(struct igc_adapter *adapter)
>> +{
>> +??? igc_get_hw_control(adapter);
>> +}
>> +
>> +/**
>> + *? igc_rar_set_index - Sync RAL[index] and RAH[index] registers with 
>> MAC table
>> + *? @adapter: Pointer to adapter structure
>> + *? @index: Index of the RAR entry which need to be synced with MAC 
>> table
>> + **/
>> +static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
>> +{
>> +??? struct e1000_hw *hw = &adapter->hw;
>> +??? u32 rar_low, rar_high;
>> +??? u8 *addr = adapter->mac_table[index].addr;
>> +
>> +??? /* HW expects these to be in network order when they are plugged
>> +???? * into the registers which are little endian.? In order to 
>> guarantee
>> +???? * that ordering we need to do an leXX_to_cpup here in order to be
>> +???? * ready for the byteswap that occurs with writel
>> +???? */
>> +??? rar_low = le32_to_cpup((__le32 *)(addr));
>> +??? rar_high = le16_to_cpup((__le16 *)(addr + 4));
>> +
>> +??? /* Indicate to hardware the Address is Valid. */
>> +??? if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
>> +??????? if (is_valid_ether_addr(addr))
>> +??????????? rar_high |= E1000_RAH_AV;
>> +
>> +??????? rar_high |= E1000_RAH_POOL_1 <<
>> +??????????? adapter->mac_table[index].queue;
>> +??? }
>> +
>> +??? wr32(E1000_RAL(index), rar_low);
>> +??? wrfl();
>> +??? wr32(E1000_RAH(index), rar_high);
>> +??? wrfl();
>> +}
>> +
>> +/* Set default MAC address for the PF in the first RAR entry */
>> +static void igc_set_default_mac_filter(struct igc_adapter *adapter)
>> +{
>> +??? struct igc_mac_addr *mac_table = &adapter->mac_table[0];
>> +
>> +??? ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
>> +??? mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
>> +
>> +??? igc_rar_set_index(adapter, 0);
>> +}
>> +
>> +/**
>> + * igc_open - Called when a network interface is made active
>> + * @netdev: network interface device structure
>> + *
>> + * Returns 0 on success, negative value on failure
>> + *
>> + * The open entry point is called when a network interface is made
>> + * active by the system (IFF_UP).? At this point all resources needed
>> + * for transmit and receive operations are allocated, the interrupt
>> + * handler is registered with the OS, the watchdog timer is started,
>> + * and the stack is notified that the interface is ready.
>> + **/
>> +static int __igc_open(struct net_device *netdev, bool resuming)
>> +{
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +??? struct e1000_hw *hw = &adapter->hw;
>> +??? int i = 0;
>> +
>> +??? /* disallow open during test */
>> +
>> +??? if (test_bit(__IGC_TESTING, &adapter->state)) {
>> +??????? WARN_ON(resuming);
>> +??????? return -EBUSY;
>> +??? }
>> +
>> +??? netif_carrier_off(netdev);
>> +
>> +??? igc_power_up_link(adapter);
>> +
>> +??? igc_configure(adapter);
>> +
>> +??? /* From here on the code is the same as igc_up() */
> 
> Then why not use igc_up()?
> 
Very good point. igc_up is sligthly different, so this comment is wrong. 
I will remove. Fix will be applied in v4.
>> +??? clear_bit(__IGC_DOWN, &adapter->state);
>> +
>> +??? for (i = 0; i < adapter->num_q_vectors; i++)
>> +??????? napi_enable(&adapter->q_vector[i]->napi);
>> +
>> +??? /* start the watchdog. */
>> +??? hw->mac.get_link_status = 1;
>> +??? schedule_work(&adapter->watchdog_task);
>> +
>> +??? return E1000_SUCCESS;
>> +}
>> +
>> +int igc_open(struct net_device *netdev)
>> +{
>> +??? return __igc_open(netdev, false);
>> +}
>> +
>> +/**
>> + * igc_close - Disables a network interface
>> + * @netdev: network interface device structure
>> + *
>> + * Returns 0, this is not allowed to fail
>> + *
>> + * The close entry point is called when an interface is de-activated
>> + * by the OS.? The hardware is still under the driver's control, but
>> + * needs to be disabled.? A global MAC reset is issued to stop the
>> + * hardware, and all transmit and receive resources are freed.
>> + **/
>> +static int __igc_close(struct net_device *netdev, bool suspending)
>> +{
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +
>> +??? WARN_ON(test_bit(__IGC_RESETTING, &adapter->state));
>> +
>> +??? igc_down(adapter);
>> +
>> +??? igc_release_hw_control(adapter);
>> +
>> +??? return 0;
>> +}
>> +
>> +int igc_close(struct net_device *netdev)
>> +{
>> +??? if (netif_device_present(netdev) || netdev->dismantle)
>> +??????? return __igc_close(netdev, false);
>> +??? return 0;
>> +}
>> +
>> +static const struct net_device_ops igc_netdev_ops = {
>> +??? .ndo_open?????????????? = igc_open,
>> +??? .ndo_stop?????????????? = igc_close,
>> +??? .ndo_start_xmit???????? = igc_xmit_frame,
>> +??? .ndo_set_mac_address??? = igc_set_mac,
>> +??? .ndo_change_mtu???????? = igc_change_mtu,
>> +??? .ndo_get_stats????????? = igc_get_stats,
>> +??? .ndo_do_ioctl?????????? = igc_ioctl,
>> +
>> +};
>> ? /* PCIe configuration access */
>> ? void igc_read_pci_cfg(struct e1000_hw *hw, u32 reg, u16 *value)
>> @@ -73,6 +439,7 @@ s32 igc_write_pcie_cap_reg(struct e1000_hw *hw, u32 
>> reg, u16 *value)
>> ? u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>> ? {
>> +??? struct igc_adapter *igc = container_of(hw, struct igc_adapter, hw);
>> ????? u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
>> ????? u32 value = 0;
>> @@ -82,8 +449,13 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>> ????? value = readl(&hw_addr[reg]);
>> ????? /* reads should not return all F's */
>> -??? if (!(~value) && (!reg || !(~readl(hw_addr))))
>> +??? if (!(~value) && (!reg || !(~readl(hw_addr)))) {
>> +??????? struct net_device *netdev = igc->netdev;
>> +
>> ????????? hw->hw_addr = NULL;
>> +??????? netif_device_detach(netdev);
>> +??????? netdev_err(netdev, "PCIe link lost, device now detached\n");
>> +??? }
>> ????? return value;
>> ? }
>> @@ -102,6 +474,7 @@ u32 igc_rd32(struct e1000_hw *hw, u32 reg)
>> ? static int igc_probe(struct pci_dev *pdev,
>> ?????????????? const struct pci_device_id *ent)
>> ? {
>> +??? struct net_device *netdev;
> 
> Reverse xmas tree
> 
Good. Fix will be applied in v4.
>> ????? struct igc_adapter *adapter;
>> ????? struct e1000_hw *hw;
>> ????? int err, pci_using_dac;
>> @@ -136,8 +509,56 @@ static int igc_probe(struct pci_dev *pdev,
>> ????? if (err)
>> ????????? goto err_pci_reg;
>> +??? pci_enable_pcie_error_reporting(pdev);
>> +
>> ????? pci_set_master(pdev);
>> -??? pci_save_state(pdev);
>> +
>> +??? err = -ENOMEM;
>> +??? netdev = alloc_etherdev_mq(sizeof(struct igc_adapter),
>> +?????????????????? IGC_MAX_TX_QUEUES);
>> +
>> +??? if (!netdev)
>> +??????? goto err_alloc_etherdev;
>> +
>> +??? SET_NETDEV_DEV(netdev, &pdev->dev);
>> +
>> +??? pci_set_drvdata(pdev, netdev);
>> +??? adapter = netdev_priv(netdev);
>> +??? adapter->netdev = netdev;
>> +??? adapter->pdev = pdev;
>> +??? hw = &adapter->hw;
>> +??? hw->back = adapter;
>> +??? adapter->port_num = hw->bus.func;
>> +??? adapter->msg_enable = GENMASK(debug - 1, 0);
>> +
>> +??? err = pci_save_state(pdev);
>> +??? if (err)
>> +??????? goto err_ioremap;
>> +
>> +??? err = -EIO;
>> +??? adapter->io_addr = ioremap(pci_resource_start(pdev, 0),
>> +?????????????????? pci_resource_len(pdev, 0));
>> +??? if (!adapter->io_addr)
>> +??????? goto err_ioremap;
>> +
>> +??? /* hw->hw_addr can be zeroed, so use adapter->io_addr for unmap */
>> +??? hw->hw_addr = adapter->io_addr;
>> +
>> +??? netdev->netdev_ops = &igc_netdev_ops;
>> +
>> +??? netdev->watchdog_timeo = 5 * HZ;
>> +
>> +??? strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
>> +
>> +??? netdev->mem_start = pci_resource_start(pdev, 0);
>> +??? netdev->mem_end = pci_resource_end(pdev, 0);
>> +
>> +??? /* PCI config space info */
>> +??? hw->vendor_id = pdev->vendor;
>> +??? hw->device_id = pdev->device;
>> +??? hw->revision_id = pdev->revision;
>> +??? hw->subsystem_vendor_id = pdev->subsystem_vendor;
>> +??? hw->subsystem_device_id = pdev->subsystem_device;
>> ????? /* setup the private structure */
>> ????? err = igc_sw_init(adapter);
>> @@ -146,9 +567,49 @@ static int igc_probe(struct pci_dev *pdev,
>> ????? igc_get_bus_info_pcie(hw);
>> +??? /* MTU range: 68 - 9216 */
>> +??? netdev->min_mtu = ETH_MIN_MTU;
>> +??? netdev->max_mtu = MAX_STD_JUMBO_FRAME_SIZE;
>> +
>> +??? /* reset the hardware with the new settings */
>> +??? igc_reset(adapter);
>> +
>> +??? /* let the f/w know that the h/w is now under the control of the
>> +???? * driver.
>> +???? */
>> +??? igc_get_hw_control(adapter);
>> +
>> +??? strncpy(netdev->name, "eth%d", IFNAMSIZ);
> 
> You're stomping on what you just set a few lines earlier.? Of course, 
> some might argue that you shouldn't put anything here at all and let the 
> system name the device.
> 
my apologies I do not understand you.
>> +??? err = register_netdev(netdev);
>> +??? if (err)
>> +??????? goto err_register;
>> +
>> +???? /* carrier off reporting is important to ethtool even BEFORE 
>> open */
>> +??? netif_carrier_off(netdev);
>> +
>> +??? dev_info(&pdev->dev, "@SUMMARY@");
>> +??? /* print bus type/speed/width info */
>> +??? dev_info(&pdev->dev, "%s: (PCIe:%s:%s) ",
>> +???????? netdev->name,
>> +???????? ((hw->bus.speed == e1000_bus_speed_2500) ? "2.5GT/s" :
>> +???????? (hw->bus.speed == e1000_bus_speed_5000) ? "5.0GT/s" :
>> +???????? "unknown"),
>> +???????? ((hw->bus.width == e1000_bus_width_pcie_x4) ? "Width x4" :
>> +???????? (hw->bus.width == e1000_bus_width_pcie_x2) ? "Width x2" :
>> +???????? (hw->bus.width == e1000_bus_width_pcie_x1) ? "Width x1" :
>> +???????? "unknown"));
> 
> How about using the new pcie_print_link_status()?
> 
Good idea. I will keep it in mind and replace in future. Currenly in my 
working kernel this method not exist yet.
>> +??? netdev_info(netdev, "MAC: %pM\n", netdev->dev_addr);
>> +
>> ????? return 0;
>> +err_register:
>> +??? igc_release_hw_control(adapter);
>> ? err_sw_init:
>> +err_ioremap:
>> +??? free_netdev(netdev);
>> +err_alloc_etherdev:
>> +??? pci_release_selected_regions(pdev,
>> +???????????????????? pci_select_bars(pdev, IORESOURCE_MEM));
>> ? err_pci_reg:
>> ? err_dma:
>> ????? pci_disable_device(pdev);
>> @@ -166,9 +627,22 @@ static int igc_probe(struct pci_dev *pdev,
>> ?? **/
>> ? static void igc_remove(struct pci_dev *pdev)
>> ? {
>> +??? struct net_device *netdev = pci_get_drvdata(pdev);
>> +??? struct igc_adapter *adapter = netdev_priv(netdev);
>> +
>> +??? set_bit(__IGC_DOWN, &adapter->state);
>> +??? flush_scheduled_work();
>> +
>> +??? /* Release control of h/w to f/w.? If f/w is AMT enabled, this
>> +???? * would have already happened in close and is redundant.
>> +???? */
>> +??? igc_release_hw_control(adapter);
>> +??? unregister_netdev(netdev);
>> +
>> ????? pci_release_selected_regions(pdev,
>> ?????????????????????? pci_select_bars(pdev, IORESOURCE_MEM));
>> +??? free_netdev(netdev);
>> ????? pci_disable_device(pdev);
>> ? }
>> @@ -190,6 +664,7 @@ static struct pci_driver igc_driver = {
>> ? static int igc_sw_init(struct igc_adapter *adapter)
>> ? {
>> ????? struct e1000_hw *hw = &adapter->hw;
>> +??? struct net_device *netdev = adapter->netdev;
>> ????? struct pci_dev *pdev = adapter->pdev;
>> ????? /* PCI config space info */
>> @@ -203,6 +678,12 @@ static int igc_sw_init(struct igc_adapter *adapter)
>> ????? pci_read_config_word(pdev, PCI_COMMAND, &hw->bus.pci_cmd_word);
>> +??? /* set default work limits */
>> +??? adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
>> +??????????????????? VLAN_HLEN;
>> +
>> +??? set_bit(__IGC_DOWN, &adapter->state);
>> +
>> ????? return 0;
>> ? }
>> @@ -244,4 +725,7 @@ MODULE_AUTHOR("Intel Corporation, 
>> <linux.nics@intel.com>");
>> ? MODULE_DESCRIPTION(DRV_SUMMARY);
>> ? MODULE_LICENSE("GPL");
>> ? MODULE_VERSION(DRV_VERSION);
>> +
>> +module_param(debug, int, 0);
>> +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>> ? /* igc_main.c */
>>
Hello Shannon,
Thanks you very much for a comments.
Sasha

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
  2018-07-02 13:05   ` Neftin, Sasha
@ 2018-07-02 16:40     ` Shannon Nelson
  2018-07-12  6:26       ` Neftin, Sasha
  0 siblings, 1 reply; 8+ messages in thread
From: Shannon Nelson @ 2018-07-02 16:40 UTC (permalink / raw)
  To: intel-wired-lan



On 7/2/2018 6:05 AM, Neftin, Sasha wrote:
> On 6/27/2018 03:32, Shannon Nelson wrote:
>> On 6/24/2018 1:45 AM, Sasha Neftin wrote:
>>> Now that we have the ability to configure the basic settings on the 
>>> device

[...]

>>> ? /* Board specific private data structure */
>>> ? struct igc_adapter {
>>> +??? struct net_device *netdev;
>>> +
>>> +??? unsigned long state;
>>> +??? unsigned int flags;
>>
>> These state and flags fields should probably be specifically defined 
>> as u32 or u64.
>>
> Let me do not agree with you here. I've a two arguments:
> 1. general set_bit/clear_bit methods is used this type and do not allow 
> pass another type. Actually we use set_bit/clear_bit a lot of.
> 2. i prefer stay consistently with another driver where is used same value.
>> Is there a reason that state is a long and flags an int?? Is there an 
>> expectation of different sizes?

The 'state' to be used in set/clear_bit makes sense, but it might be 
better to use DECLARE_BITMAP().

The 'flags' still might be specifically set to u32 as done in ixgbe and 
i40e.

[...]

>>> +
>>> +??? strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
>>> +

[...]

>>> +
>>> +??? strncpy(netdev->name, "eth%d", IFNAMSIZ);
>>
>> You're stomping on what you just set a few lines earlier.? Of course, 
>> some might argue that you shouldn't put anything here at all and let 
>> the system name the device.
>>
> my apologies I do not understand you.

You've already set the netdev->name once, here you're setting it again 
to something else.  In either case, the userland daemons will likely 
change whatever you put here.


>>> +??? err = register_netdev(netdev);
>>> +??? if (err)
>>> +??????? goto err_register;
>>> +
>>> +???? /* carrier off reporting is important to ethtool even BEFORE 
>>> open */
>>> +??? netif_carrier_off(netdev);
>>> +
>>> +??? dev_info(&pdev->dev, "@SUMMARY@");
>>> +??? /* print bus type/speed/width info */
>>> +??? dev_info(&pdev->dev, "%s: (PCIe:%s:%s) ",
>>> +???????? netdev->name,
>>> +???????? ((hw->bus.speed == e1000_bus_speed_2500) ? "2.5GT/s" :
>>> +???????? (hw->bus.speed == e1000_bus_speed_5000) ? "5.0GT/s" :
>>> +???????? "unknown"),
>>> +???????? ((hw->bus.width == e1000_bus_width_pcie_x4) ? "Width x4" :
>>> +???????? (hw->bus.width == e1000_bus_width_pcie_x2) ? "Width x2" :
>>> +???????? (hw->bus.width == e1000_bus_width_pcie_x1) ? "Width x1" :
>>> +???????? "unknown"));
>>
>> How about using the new pcie_print_link_status()?
>>
> Good idea. I will keep it in mind and replace in future. Currenly in my 
> working kernel this method not exist yet.

Then you're using an out-of-date kernel and you should update to what is 
currently upstream.

sln

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev
  2018-07-02 16:40     ` Shannon Nelson
@ 2018-07-12  6:26       ` Neftin, Sasha
  0 siblings, 0 replies; 8+ messages in thread
From: Neftin, Sasha @ 2018-07-12  6:26 UTC (permalink / raw)
  To: intel-wired-lan

On 7/2/2018 19:40, Shannon Nelson wrote:
> 
> 
> On 7/2/2018 6:05 AM, Neftin, Sasha wrote:
>> On 6/27/2018 03:32, Shannon Nelson wrote:
>>> On 6/24/2018 1:45 AM, Sasha Neftin wrote:
>>>> Now that we have the ability to configure the basic settings on the 
>>>> device
> 
> [...]
> 
>>>> ? /* Board specific private data structure */
>>>> ? struct igc_adapter {
>>>> +??? struct net_device *netdev;
>>>> +
>>>> +??? unsigned long state;
>>>> +??? unsigned int flags;
>>>
>>> These state and flags fields should probably be specifically defined 
>>> as u32 or u64.
>>>
>> Let me do not agree with you here. I've a two arguments:
>> 1. general set_bit/clear_bit methods is used this type and do not 
>> allow pass another type. Actually we use set_bit/clear_bit a lot of.
>> 2. i prefer stay consistently with another driver where is used same 
>> value.
>>> Is there a reason that state is a long and flags an int?? Is there an 
>>> expectation of different sizes?
> 
> The 'state' to be used in set/clear_bit makes sense, but it might be 
> better to use DECLARE_BITMAP().
> 
> The 'flags' still might be specifically set to u32 as done in ixgbe and 
> i40e.
> 
yes, I understand. This is might be good optimization and I will 
consider do it a bit later. I would like stable my driver at first.
> [...]
> 
>>>> +
>>>> +??? strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
>>>> +
> 
> [...]
> 
>>>> +
>>>> +??? strncpy(netdev->name, "eth%d", IFNAMSIZ);
>>>
>>> You're stomping on what you just set a few lines earlier.? Of course, 
>>> some might argue that you shouldn't put anything here at all and let 
>>> the system name the device.
>>>
>> my apologies I do not understand you.
> 
> You've already set the netdev->name once, here you're setting it again 
> to something else.? In either case, the userland daemons will likely 
> change whatever you put here.
> 
> 
yes. But without using this setting of name probe method stuck with 
initialization error 22. May be wee need look a more depth into.
>>>> +??? err = register_netdev(netdev);
>>>> +??? if (err)
>>>> +??????? goto err_register;
>>>> +
>>>> +???? /* carrier off reporting is important to ethtool even BEFORE 
>>>> open */
>>>> +??? netif_carrier_off(netdev);
>>>> +
>>>> +??? dev_info(&pdev->dev, "@SUMMARY@");
>>>> +??? /* print bus type/speed/width info */
>>>> +??? dev_info(&pdev->dev, "%s: (PCIe:%s:%s) ",
>>>> +???????? netdev->name,
>>>> +???????? ((hw->bus.speed == e1000_bus_speed_2500) ? "2.5GT/s" :
>>>> +???????? (hw->bus.speed == e1000_bus_speed_5000) ? "5.0GT/s" :
>>>> +???????? "unknown"),
>>>> +???????? ((hw->bus.width == e1000_bus_width_pcie_x4) ? "Width x4" :
>>>> +???????? (hw->bus.width == e1000_bus_width_pcie_x2) ? "Width x2" :
>>>> +???????? (hw->bus.width == e1000_bus_width_pcie_x1) ? "Width x1" :
>>>> +???????? "unknown"));
>>>
>>> How about using the new pcie_print_link_status()?
>>>
>> Good idea. I will keep it in mind and replace in future. Currenly in 
>> my working kernel this method not exist yet.
> 
> Then you're using an out-of-date kernel and you should update to what is 
> currently upstream.
> 
Good. I've liked this API and upgraded kernel to 4.18+. fix will be 
applied in v4.
> sln

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-07-12  6:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-24  8:45 [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Sasha Neftin
2018-06-24 12:33 ` kbuild test robot
2018-06-24 12:34 ` [Intel-wired-lan] [RFC PATCH] igc: igc_reset() can be static kbuild test robot
2018-06-28  7:07   ` Neftin, Sasha
2018-06-27  0:32 ` [Intel-wired-lan] [PATCH v3 03/11] igc: Add netdev Shannon Nelson
2018-07-02 13:05   ` Neftin, Sasha
2018-07-02 16:40     ` Shannon Nelson
2018-07-12  6:26       ` Neftin, Sasha

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.