[PATCH net-next 0/5] qed*: eeprom access et al.

[PATCH net-next 0/5] qed*: eeprom access et al.

[Yuval Mintz]

This patch series contains some ethtool-related enhancements,
where the significant addition is the ability to read/write from
the device's non-volatile memory via the ethtool socket
[required for the adapter's fw-upgrade utility].

Dave,

Please consider applying this to `net-next'.

Thanks,
Yuval

P.s. - up until this point I've tried to maintain distinct patches for
qed and qede. But where the infrastructure is either scarce or else
encompass most of the changes [as in with the eeprom changes] I see
little merit in splitting the backend/frontend into seperate patches.
If there are any objections here, I'll step back into splitting things.

Sudarsana Reddy Kalluru (2):
  qed: add support for link pause configuration.
  qed*: Add support for read/write of eeprom

Yuval Mintz (4):
  qed*: Align statistics names
  qede: Add support for ethtool private flags
  qed*: Conditions for changing link
 
 drivers/net/ethernet/qlogic/qed/qed.h           |  10 +
 drivers/net/ethernet/qlogic/qed/qed_l2.c        |  20 +-
 drivers/net/ethernet/qlogic/qed/qed_main.c      |  57 +++++
 drivers/net/ethernet/qlogic/qed/qed_mcp.c       | 274 ++++++++++++++++++++++++
 drivers/net/ethernet/qlogic/qed/qed_mcp.h       | 103 +++++++++
 drivers/net/ethernet/qlogic/qede/qede.h         |  20 +-
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c |  91 ++++++--
 drivers/net/ethernet/qlogic/qede/qede_main.c    |  29 ++-
 include/linux/qed/qed_if.h                      |  60 +++++-
 9 files changed, 610 insertions(+), 54 deletions(-)

-- 
1.9.3

[PATCH net-next 1/5] qed*: Align statistics names

[Yuval Mintz]

There's a difference in statsitics' names starting at qed and
propagating to qede, where egress counters indicate ranges while ingress
counters indiciate high-end.
Align all statistcs to follow the same conventions - name indicates range.

Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
 drivers/net/ethernet/qlogic/qed/qed_l2.c        | 20 ++++++++---------
 drivers/net/ethernet/qlogic/qede/qede.h         | 20 ++++++++---------
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 20 ++++++++---------
 drivers/net/ethernet/qlogic/qede/qede_main.c    | 29 ++++++++++++++++---------
 include/linux/qed/qed_if.h                      | 20 ++++++++---------
 5 files changed, 59 insertions(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_l2.c b/drivers/net/ethernet/qlogic/qed/qed_l2.c
index fb5f3b8..31e1d51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_l2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_l2.c
@@ -1415,16 +1415,16 @@ static void __qed_get_vport_port_stats(struct qed_hwfn *p_hwfn,
 			sizeof(port_stats));
 
 	p_stats->rx_64_byte_packets		+= port_stats.pmm.r64;
-	p_stats->rx_127_byte_packets		+= port_stats.pmm.r127;
-	p_stats->rx_255_byte_packets		+= port_stats.pmm.r255;
-	p_stats->rx_511_byte_packets		+= port_stats.pmm.r511;
-	p_stats->rx_1023_byte_packets		+= port_stats.pmm.r1023;
-	p_stats->rx_1518_byte_packets		+= port_stats.pmm.r1518;
-	p_stats->rx_1522_byte_packets		+= port_stats.pmm.r1522;
-	p_stats->rx_2047_byte_packets		+= port_stats.pmm.r2047;
-	p_stats->rx_4095_byte_packets		+= port_stats.pmm.r4095;
-	p_stats->rx_9216_byte_packets		+= port_stats.pmm.r9216;
-	p_stats->rx_16383_byte_packets		+= port_stats.pmm.r16383;
+	p_stats->rx_65_to_127_byte_packets	+= port_stats.pmm.r127;
+	p_stats->rx_128_to_255_byte_packets	+= port_stats.pmm.r255;
+	p_stats->rx_256_to_511_byte_packets	+= port_stats.pmm.r511;
+	p_stats->rx_512_to_1023_byte_packets	+= port_stats.pmm.r1023;
+	p_stats->rx_1024_to_1518_byte_packets	+= port_stats.pmm.r1518;
+	p_stats->rx_1519_to_1522_byte_packets	+= port_stats.pmm.r1522;
+	p_stats->rx_1519_to_2047_byte_packets	+= port_stats.pmm.r2047;
+	p_stats->rx_2048_to_4095_byte_packets	+= port_stats.pmm.r4095;
+	p_stats->rx_4096_to_9216_byte_packets	+= port_stats.pmm.r9216;
+	p_stats->rx_9217_to_16383_byte_packets	+= port_stats.pmm.r16383;
 	p_stats->rx_crc_errors			+= port_stats.pmm.rfcs;
 	p_stats->rx_mac_crtl_frames		+= port_stats.pmm.rxcf;
 	p_stats->rx_pause_frames		+= port_stats.pmm.rxpf;
diff --git a/drivers/net/ethernet/qlogic/qede/qede.h b/drivers/net/ethernet/qlogic/qede/qede.h
index 16df159..a687e7a 100644
--- a/drivers/net/ethernet/qlogic/qede/qede.h
+++ b/drivers/net/ethernet/qlogic/qede/qede.h
@@ -59,16 +59,16 @@ struct qede_stats {
 
 	/* port */
 	u64 rx_64_byte_packets;
-	u64 rx_127_byte_packets;
-	u64 rx_255_byte_packets;
-	u64 rx_511_byte_packets;
-	u64 rx_1023_byte_packets;
-	u64 rx_1518_byte_packets;
-	u64 rx_1522_byte_packets;
-	u64 rx_2047_byte_packets;
-	u64 rx_4095_byte_packets;
-	u64 rx_9216_byte_packets;
-	u64 rx_16383_byte_packets;
+	u64 rx_65_to_127_byte_packets;
+	u64 rx_128_to_255_byte_packets;
+	u64 rx_256_to_511_byte_packets;
+	u64 rx_512_to_1023_byte_packets;
+	u64 rx_1024_to_1518_byte_packets;
+	u64 rx_1519_to_1522_byte_packets;
+	u64 rx_1519_to_2047_byte_packets;
+	u64 rx_2048_to_4095_byte_packets;
+	u64 rx_4096_to_9216_byte_packets;
+	u64 rx_9217_to_16383_byte_packets;
 	u64 rx_crc_errors;
 	u64 rx_mac_crtl_frames;
 	u64 rx_pause_frames;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index f0982f1..f87e83b 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -59,16 +59,16 @@ static const struct {
 	QEDE_STAT(tx_bcast_pkts),
 
 	QEDE_PF_STAT(rx_64_byte_packets),
-	QEDE_PF_STAT(rx_127_byte_packets),
-	QEDE_PF_STAT(rx_255_byte_packets),
-	QEDE_PF_STAT(rx_511_byte_packets),
-	QEDE_PF_STAT(rx_1023_byte_packets),
-	QEDE_PF_STAT(rx_1518_byte_packets),
-	QEDE_PF_STAT(rx_1522_byte_packets),
-	QEDE_PF_STAT(rx_2047_byte_packets),
-	QEDE_PF_STAT(rx_4095_byte_packets),
-	QEDE_PF_STAT(rx_9216_byte_packets),
-	QEDE_PF_STAT(rx_16383_byte_packets),
+	QEDE_PF_STAT(rx_65_to_127_byte_packets),
+	QEDE_PF_STAT(rx_128_to_255_byte_packets),
+	QEDE_PF_STAT(rx_256_to_511_byte_packets),
+	QEDE_PF_STAT(rx_512_to_1023_byte_packets),
+	QEDE_PF_STAT(rx_1024_to_1518_byte_packets),
+	QEDE_PF_STAT(rx_1519_to_1522_byte_packets),
+	QEDE_PF_STAT(rx_1519_to_2047_byte_packets),
+	QEDE_PF_STAT(rx_2048_to_4095_byte_packets),
+	QEDE_PF_STAT(rx_4096_to_9216_byte_packets),
+	QEDE_PF_STAT(rx_9217_to_16383_byte_packets),
 	QEDE_PF_STAT(tx_64_byte_packets),
 	QEDE_PF_STAT(tx_65_to_127_byte_packets),
 	QEDE_PF_STAT(tx_128_to_255_byte_packets),
diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index e5dc35a..3cdfc7d 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -1575,16 +1575,25 @@ void qede_fill_by_demand_stats(struct qede_dev *edev)
 	edev->stats.coalesced_bytes = stats.tpa_coalesced_bytes;
 
 	edev->stats.rx_64_byte_packets = stats.rx_64_byte_packets;
-	edev->stats.rx_127_byte_packets = stats.rx_127_byte_packets;
-	edev->stats.rx_255_byte_packets = stats.rx_255_byte_packets;
-	edev->stats.rx_511_byte_packets = stats.rx_511_byte_packets;
-	edev->stats.rx_1023_byte_packets = stats.rx_1023_byte_packets;
-	edev->stats.rx_1518_byte_packets = stats.rx_1518_byte_packets;
-	edev->stats.rx_1522_byte_packets = stats.rx_1522_byte_packets;
-	edev->stats.rx_2047_byte_packets = stats.rx_2047_byte_packets;
-	edev->stats.rx_4095_byte_packets = stats.rx_4095_byte_packets;
-	edev->stats.rx_9216_byte_packets = stats.rx_9216_byte_packets;
-	edev->stats.rx_16383_byte_packets = stats.rx_16383_byte_packets;
+	edev->stats.rx_65_to_127_byte_packets = stats.rx_65_to_127_byte_packets;
+	edev->stats.rx_128_to_255_byte_packets =
+				stats.rx_128_to_255_byte_packets;
+	edev->stats.rx_256_to_511_byte_packets =
+				stats.rx_256_to_511_byte_packets;
+	edev->stats.rx_512_to_1023_byte_packets =
+				stats.rx_512_to_1023_byte_packets;
+	edev->stats.rx_1024_to_1518_byte_packets =
+				stats.rx_1024_to_1518_byte_packets;
+	edev->stats.rx_1519_to_1522_byte_packets =
+				stats.rx_1519_to_1522_byte_packets;
+	edev->stats.rx_1519_to_2047_byte_packets =
+				stats.rx_1519_to_2047_byte_packets;
+	edev->stats.rx_2048_to_4095_byte_packets =
+				stats.rx_2048_to_4095_byte_packets;
+	edev->stats.rx_4096_to_9216_byte_packets =
+				stats.rx_4096_to_9216_byte_packets;
+	edev->stats.rx_9217_to_16383_byte_packets =
+				stats.rx_9217_to_16383_byte_packets;
 	edev->stats.rx_crc_errors = stats.rx_crc_errors;
 	edev->stats.rx_mac_crtl_frames = stats.rx_mac_crtl_frames;
 	edev->stats.rx_pause_frames = stats.rx_pause_frames;
diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h
index 67e8c20..82a7fe0 100644
--- a/include/linux/qed/qed_if.h
+++ b/include/linux/qed/qed_if.h
@@ -384,16 +384,16 @@ struct qed_eth_stats {
 
 	/* port */
 	u64	rx_64_byte_packets;
-	u64	rx_127_byte_packets;
-	u64	rx_255_byte_packets;
-	u64	rx_511_byte_packets;
-	u64	rx_1023_byte_packets;
-	u64	rx_1518_byte_packets;
-	u64	rx_1522_byte_packets;
-	u64	rx_2047_byte_packets;
-	u64	rx_4095_byte_packets;
-	u64	rx_9216_byte_packets;
-	u64	rx_16383_byte_packets;
+	u64	rx_65_to_127_byte_packets;
+	u64	rx_128_to_255_byte_packets;
+	u64	rx_256_to_511_byte_packets;
+	u64	rx_512_to_1023_byte_packets;
+	u64	rx_1024_to_1518_byte_packets;
+	u64	rx_1519_to_1522_byte_packets;
+	u64	rx_1519_to_2047_byte_packets;
+	u64	rx_2048_to_4095_byte_packets;
+	u64	rx_4096_to_9216_byte_packets;
+	u64	rx_9217_to_16383_byte_packets;
 	u64	rx_crc_errors;
 	u64	rx_mac_crtl_frames;
 	u64	rx_pause_frames;
-- 
1.9.3

[PATCH net-next 2/5] qede: Add support for ethtool private flags

[Yuval Mintz]

Adds a getter for the interfaces private flags.
The only parameter currently supported is whether the interface is a
coupled function [required for supporting 100g].

Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 27 +++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index f87e83b..5ba6b2a 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -116,6 +116,15 @@ static const struct {
 
 #define QEDE_NUM_STATS	ARRAY_SIZE(qede_stats_arr)
 
+enum {
+	QEDE_PRI_FLAG_CMT,
+	QEDE_PRI_FLAG_LEN,
+};
+
+static const char qede_private_arr[QEDE_PRI_FLAG_LEN][ETH_GSTRING_LEN] = {
+	"Coupled-Function",
+};
+
 static void qede_get_strings_stats(struct qede_dev *edev, u8 *buf)
 {
 	int i, j, k;
@@ -139,6 +148,10 @@ static void qede_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 	case ETH_SS_STATS:
 		qede_get_strings_stats(edev, buf);
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		memcpy(buf, qede_private_arr,
+		       ETH_GSTRING_LEN * QEDE_PRI_FLAG_LEN);
+		break;
 	default:
 		DP_VERBOSE(edev, QED_MSG_DEBUG,
 			   "Unsupported stringset 0x%08x\n", stringset);
@@ -177,6 +190,8 @@ static int qede_get_sset_count(struct net_device *dev, int stringset)
 	switch (stringset) {
 	case ETH_SS_STATS:
 		return num_stats + QEDE_NUM_RQSTATS;
+	case ETH_SS_PRIV_FLAGS:
+		return QEDE_PRI_FLAG_LEN;
 
 	default:
 		DP_VERBOSE(edev, QED_MSG_DEBUG,
@@ -185,6 +200,17 @@ static int qede_get_sset_count(struct net_device *dev, int stringset)
 	}
 }
 
+static u32 qede_get_priv_flags(struct net_device *dev)
+{
+	struct qede_dev *edev = netdev_priv(dev);
+	u32 flags = 0;
+
+	flags |= (!!(edev->dev_info.common.num_hwfns > 1)) <<
+		 QEDE_PRI_FLAG_CMT;
+
+	return flags;
+}
+
 static int qede_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct qede_dev *edev = netdev_priv(dev);
@@ -814,6 +840,7 @@ static const struct ethtool_ops qede_ethtool_ops = {
 	.get_strings = qede_get_strings,
 	.set_phys_id = qede_set_phys_id,
 	.get_ethtool_stats = qede_get_ethtool_stats,
+	.get_priv_flags = qede_get_priv_flags,
 	.get_sset_count = qede_get_sset_count,
 	.get_rxnfc = qede_get_rxnfc,
 	.set_rxnfc = qede_set_rxnfc,
-- 
1.9.3

[PATCH net-next 3/5] qed*: Conditions for changing link

[Yuval Mintz]

There's some inconsistency in current logic determining whether the
link settings of a given interface can be changed; I.e., in all modes
other than the so-called `deault' mode the interfaces are forbidden from
changing the configuration - but even this rule is not applied to all
user APIs that may change the configuration.

Instead, let the core-module [qed] decide whether an interface can change
the configuration by supporting a new API function. We also revise the
current rule, allowing all interfaces to change their configurations while
laying the infrastructure for future modes where an interface would be
blocked from making such a configuration.

Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
 drivers/net/ethernet/qlogic/qed/qed_main.c      |  6 ++++++
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 14 ++++++++++----
 include/linux/qed/qed_if.h                      | 10 ++++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 1e9f321..d189871 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -915,6 +915,11 @@ static u32 qed_sb_release(struct qed_dev *cdev,
 	return rc;
 }
 
+static bool qed_can_link_change(struct qed_dev *cdev)
+{
+	return true;
+}
+
 static int qed_set_link(struct qed_dev *cdev,
 			struct qed_link_params *params)
 {
@@ -1177,6 +1182,7 @@ const struct qed_common_ops qed_common_ops_pass = {
 	.sb_release = &qed_sb_release,
 	.simd_handler_config = &qed_simd_handler_config,
 	.simd_handler_clean = &qed_simd_handler_clean,
+	.can_link_change = &qed_can_link_change,
 	.set_link = &qed_set_link,
 	.get_link = &qed_get_current_link,
 	.drain = &qed_drain,
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 5ba6b2a..0f11685 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -243,9 +243,9 @@ static int qede_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	struct qed_link_params params;
 	u32 speed;
 
-	if (!edev->dev_info.common.is_mf_default) {
+	if (!edev->ops || !edev->ops->common->can_link_change(edev->cdev)) {
 		DP_INFO(edev,
-			"Link parameters can not be changed in non-default mode\n");
+			"Link settings are not allowed to be changed\n");
 		return -EOPNOTSUPP;
 	}
 
@@ -354,6 +354,12 @@ static int qede_nway_reset(struct net_device *dev)
 	struct qed_link_output current_link;
 	struct qed_link_params link_params;
 
+	if (!edev->ops || !edev->ops->common->can_link_change(edev->cdev)) {
+		DP_INFO(edev,
+			"Link settings are not allowed to be changed\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (!netif_running(dev))
 		return 0;
 
@@ -454,9 +460,9 @@ static int qede_set_pauseparam(struct net_device *dev,
 	struct qed_link_params params;
 	struct qed_link_output current_link;
 
-	if (!edev->dev_info.common.is_mf_default) {
+	if (!edev->ops || !edev->ops->common->can_link_change(edev->cdev)) {
 		DP_INFO(edev,
-			"Pause parameters can not be updated in non-default mode\n");
+			"Pause settings are not allowed to be changed\n");
 		return -EOPNOTSUPP;
 	}
 
diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h
index 82a7fe0..e5de42b 100644
--- a/include/linux/qed/qed_if.h
+++ b/include/linux/qed/qed_if.h
@@ -211,6 +211,16 @@ struct qed_common_ops {
 
 	void		(*simd_handler_clean)(struct qed_dev *cdev,
 					      int index);
+
+/**
+ * @brief can_link_change - can the instance change the link or not
+ *
+ * @param cdev
+ *
+ * @return true if link-change is allowed, false otherwise.
+ */
+	bool (*can_link_change)(struct qed_dev *cdev);
+
 /**
  * @brief set_link - set links according to params
  *
-- 
1.9.3

[PATCH net-next 4/5] qed: add support for link pause configuration.

[Yuval Mintz]

From: Sudarsana Reddy Kalluru <sudarsana.kalluru@qlogic.com>

The APIs for making this sort of configuration [e.g., via ethtool] are
already present in qede, but the current configuration flow in qed doesn't
respect it.

Signed-off-by: Sudarsana Reddy Kalluru <sudarsana.kalluru@qlogic.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index d189871..1918b83 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -962,6 +962,20 @@ static int qed_set_link(struct qed_dev *cdev,
 	}
 	if (params->override_flags & QED_LINK_OVERRIDE_SPEED_FORCED_SPEED)
 		link_params->speed.forced_speed = params->forced_speed;
+	if (params->override_flags & QED_LINK_OVERRIDE_PAUSE_CONFIG) {
+		if (params->pause_config & QED_LINK_PAUSE_AUTONEG_ENABLE)
+			link_params->pause.autoneg = true;
+		else
+			link_params->pause.autoneg = false;
+		if (params->pause_config & QED_LINK_PAUSE_RX_ENABLE)
+			link_params->pause.forced_rx = true;
+		else
+			link_params->pause.forced_rx = false;
+		if (params->pause_config & QED_LINK_PAUSE_TX_ENABLE)
+			link_params->pause.forced_tx = true;
+		else
+			link_params->pause.forced_tx = false;
+	}
 
 	rc = qed_mcp_set_link(hwfn, ptt, params->link_up);
 
-- 
1.9.3

[PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[Yuval Mintz]

From: Sudarsana Reddy Kalluru <sudarsana.kalluru@qlogic.com>

Add the driver logic needed for supporting the ethtool APIs for reading
and writing from the interface's eeprom.

There's quite a bit of `magic' here [based on the ethtool `magic' field]
which is retained by the user application responsible for interacting
with the driver for the purpose of upgrading the nvram image.

Basically, the interface goes via MFW which is responsible for actually
programming the non-volatile memory. There are 2 supported modes of
operations, one which is `file'-based and one which is based on raw data.

Signed-off-by: Sudarsana Reddy Kalluru <sudarsana.kalluru@qlogic.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
 drivers/net/ethernet/qlogic/qed/qed.h           |  10 +
 drivers/net/ethernet/qlogic/qed/qed_main.c      |  37 ++++
 drivers/net/ethernet/qlogic/qed/qed_mcp.c       | 274 ++++++++++++++++++++++++
 drivers/net/ethernet/qlogic/qed/qed_mcp.h       | 103 +++++++++
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c |  30 +++
 include/linux/qed/qed_if.h                      |  30 +++
 6 files changed, 484 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index 33e2ed6..025248f 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -38,6 +38,16 @@ enum qed_coalescing_mode {
 	QED_COAL_MODE_ENABLE
 };
 
+enum qed_nvm_cmd {
+	QED_PUT_FILE_BEGIN	= DRV_MSG_CODE_NVM_PUT_FILE_BEGIN,
+	QED_PUT_FILE_DATA	= DRV_MSG_CODE_NVM_PUT_FILE_DATA,
+	QED_NVM_READ_NVRAM	= DRV_MSG_CODE_NVM_READ_NVRAM,
+	QED_NVM_WRITE_NVRAM	= DRV_MSG_CODE_NVM_WRITE_NVRAM,
+	QED_NVM_DEL_FILE	= DRV_MSG_CODE_NVM_DEL_FILE,
+	QED_NVM_SET_SECURE_MODE = DRV_MSG_CODE_SET_SECURE_MODE,
+	QED_GET_MCP_NVM_RESP	= 0xFFFFFF00
+};
+
 struct qed_eth_cb_ops;
 struct qed_dev_info;
 
diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 1918b83..78b5966 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1165,6 +1165,41 @@ static int qed_drain(struct qed_dev *cdev)
 	return 0;
 }
 
+int qed_nvm_get_cmd(struct qed_dev *cdev, u32 cmd, u32 addr,
+		    u8 *buf, u32 len)
+{
+	switch (cmd) {
+	case QED_NVM_READ_NVRAM:
+		return qed_mcp_nvm_read(cdev, addr, buf, len);
+	case QED_GET_MCP_NVM_RESP:
+		return qed_mcp_nvm_resp(cdev, buf);
+	default:
+		cdev->mcp_nvm_resp = FW_MSG_CODE_NVM_OPERATION_FAILED;
+		DP_NOTICE(cdev, "Unknown command %d\n", cmd);
+		return -EOPNOTSUPP;
+	}
+}
+
+int qed_nvm_set_cmd(struct qed_dev *cdev, u32 cmd, u32 addr,
+		    u8 *buf, u32 len)
+{
+	switch (cmd) {
+	case QED_NVM_DEL_FILE:
+		return qed_mcp_nvm_del_file(cdev, addr);
+	case QED_PUT_FILE_BEGIN:
+		return qed_mcp_nvm_put_file_begin(cdev, addr);
+	case QED_PUT_FILE_DATA:
+	case QED_NVM_WRITE_NVRAM:
+		return qed_mcp_nvm_write(cdev, cmd, addr, buf, len);
+	case QED_NVM_SET_SECURE_MODE:
+		return qed_mcp_nvm_set_secure_mode(cdev, addr);
+	default:
+		cdev->mcp_nvm_resp = FW_MSG_CODE_NVM_OPERATION_FAILED;
+		DP_NOTICE(cdev, "Unknown command %d\n", cmd);
+		return -EOPNOTSUPP;
+	}
+}
+
 static int qed_set_led(struct qed_dev *cdev, enum qed_led_mode mode)
 {
 	struct qed_hwfn *hwfn = QED_LEADING_HWFN(cdev);
@@ -1203,5 +1238,7 @@ const struct qed_common_ops qed_common_ops_pass = {
 	.update_msglvl = &qed_init_dp,
 	.chain_alloc = &qed_chain_alloc,
 	.chain_free = &qed_chain_free,
+	.nvm_get_cmd = &qed_nvm_get_cmd,
+	.nvm_set_cmd = &qed_nvm_set_cmd,
 	.set_led = &qed_set_led,
 };
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
index b89c9a8..1812ff9 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c
@@ -395,6 +395,60 @@ int qed_mcp_cmd(struct qed_hwfn *p_hwfn,
 	return 0;
 }
 
+static int qed_mcp_nvm_wr_cmd(struct qed_hwfn *p_hwfn,
+			      struct qed_ptt *p_ptt,
+			      u32 cmd, u32 param,
+			      u32 *o_mcp_resp, u32 *o_mcp_param,
+			      u32 i_txn_size, u32 *i_buf)
+{
+	struct qed_mcp_mb_params mb_params;
+	union drv_union_data union_data;
+	int rc;
+
+	memset(&mb_params, 0, sizeof(mb_params));
+	mb_params.cmd = cmd;
+	mb_params.param = param;
+	memcpy(&union_data.raw_data, i_buf, i_txn_size);
+	mb_params.p_data_src = &union_data;
+
+	rc = qed_mcp_cmd_and_union(p_hwfn, p_ptt, &mb_params);
+	if (rc)
+		return rc;
+
+	*o_mcp_resp = mb_params.mcp_resp;
+	*o_mcp_param = mb_params.mcp_param;
+
+	return 0;
+}
+
+static int qed_mcp_nvm_rd_cmd(struct qed_hwfn *p_hwfn,
+			      struct qed_ptt *p_ptt,
+			      u32 cmd, u32 param,
+			      u32 *o_mcp_resp, u32 *o_mcp_param,
+			      u32 *o_txn_size, u32 *o_buf)
+{
+	struct qed_mcp_mb_params mb_params;
+	union drv_union_data union_data;
+	int rc;
+
+	memset(&mb_params, 0, sizeof(mb_params));
+	mb_params.cmd = cmd;
+	mb_params.param = param;
+	mb_params.p_data_dst = &union_data;
+
+	rc = qed_mcp_cmd_and_union(p_hwfn, p_ptt, &mb_params);
+	if (rc)
+		return rc;
+
+	*o_mcp_resp = mb_params.mcp_resp;
+	*o_mcp_param = mb_params.mcp_param;
+
+	*o_txn_size = *o_mcp_param;
+	memcpy(o_buf, &union_data.raw_data, *o_txn_size);
+
+	return 0;
+}
+
 int qed_mcp_load_req(struct qed_hwfn *p_hwfn,
 		     struct qed_ptt *p_ptt,
 		     u32 *p_load_code)
@@ -908,6 +962,43 @@ int qed_mcp_drain(struct qed_hwfn *p_hwfn,
 	return rc;
 }
 
+static int qed_mcp_nvm_command(struct qed_hwfn *p_hwfn,
+			       struct qed_ptt *p_ptt,
+			       struct qed_mcp_nvm_params *params)
+{
+	int rc;
+
+	switch (params->type) {
+	case QED_MCP_NVM_RD:
+		rc = qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt, params->nvm_common.cmd,
+					params->nvm_common.offset,
+					&params->nvm_common.resp,
+					&params->nvm_common.param,
+					params->nvm_rd.buf_size,
+					params->nvm_rd.buf);
+		break;
+	case QED_MCP_CMD:
+		rc = qed_mcp_cmd(p_hwfn, p_ptt, params->nvm_common.cmd,
+				 params->nvm_common.offset,
+				 &params->nvm_common.resp,
+				 &params->nvm_common.param);
+		break;
+	case QED_MCP_NVM_WR:
+		rc = qed_mcp_nvm_wr_cmd(p_hwfn, p_ptt, params->nvm_common.cmd,
+					params->nvm_common.offset,
+					&params->nvm_common.resp,
+					&params->nvm_common.param,
+					params->nvm_wr.buf_size,
+					params->nvm_wr.buf);
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	return rc;
+}
+
 int qed_mcp_get_flash_size(struct qed_hwfn *p_hwfn,
 			   struct qed_ptt *p_ptt,
 			   u32 *p_flash_size)
@@ -979,3 +1070,186 @@ int qed_mcp_set_led(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 
 	return rc;
 }
+
+int qed_mcp_nvm_read(struct qed_dev *cdev,
+		     u32 addr,
+		     u8 *p_buf,
+		     u32 len)
+{
+	u32 bytes_left = len, offset = 0, bytes_to_copy, buf_size;
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+	int rc = 0;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	params.type = QED_MCP_NVM_RD;
+	params.nvm_rd.buf_size = &buf_size;
+	params.nvm_common.cmd = DRV_MSG_CODE_NVM_READ_NVRAM;
+
+	while (bytes_left > 0) {
+		bytes_to_copy = min_t(u32, bytes_left,
+				      MCP_DRV_NVM_BUF_LEN);
+		params.nvm_common.offset = (addr + offset) |
+					   (bytes_to_copy <<
+					    DRV_MB_PARAM_NVM_LEN_SHIFT);
+		params.nvm_rd.buf = (u32 *)(p_buf + offset);
+
+		rc = qed_mcp_nvm_command(p_hwfn, p_ptt, &params);
+		if (rc || (params.nvm_common.resp != FW_MSG_CODE_NVM_OK)) {
+			DP_NOTICE(cdev, "MCP command rc = %d\n",
+				  rc);
+			break;
+		}
+
+		offset += *params.nvm_rd.buf_size;
+		bytes_left -= *params.nvm_rd.buf_size;
+	}
+
+	cdev->mcp_nvm_resp = params.nvm_common.resp;
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return rc;
+}
+
+int qed_mcp_nvm_resp(struct qed_dev *cdev,
+		     u8 *p_buf)
+{
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	memcpy(p_buf, &cdev->mcp_nvm_resp, sizeof(cdev->mcp_nvm_resp));
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return 0;
+}
+
+int qed_mcp_nvm_del_file(struct qed_dev *cdev,
+			 u32 addr)
+{
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+	int rc;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	params.type = QED_MCP_CMD;
+	params.nvm_common.cmd = DRV_MSG_CODE_NVM_DEL_FILE;
+	params.nvm_common.offset = addr;
+
+	rc = qed_mcp_nvm_command(p_hwfn, p_ptt, &params);
+	cdev->mcp_nvm_resp = params.nvm_common.resp;
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return rc;
+}
+
+int qed_mcp_nvm_put_file_begin(struct qed_dev *cdev,
+			       u32 addr)
+{
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+	int rc;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	params.type = QED_MCP_CMD;
+	params.nvm_common.cmd = DRV_MSG_CODE_NVM_PUT_FILE_BEGIN;
+	params.nvm_common.offset = addr;
+
+	rc = qed_mcp_nvm_command(p_hwfn, p_ptt, &params);
+	cdev->mcp_nvm_resp = params.nvm_common.resp;
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return rc;
+}
+
+int qed_mcp_nvm_write(struct qed_dev *cdev,
+		      u32 cmd,
+		      u32 addr,
+		      u8 *p_buf,
+		      u32 len)
+{
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+	u32 buf_idx = 0, buf_size;
+	int rc = -EINVAL;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	params.type = QED_MCP_NVM_WR;
+	if (cmd == QED_PUT_FILE_DATA)
+		params.nvm_common.cmd = DRV_MSG_CODE_NVM_PUT_FILE_DATA;
+	else
+		params.nvm_common.cmd = DRV_MSG_CODE_NVM_WRITE_NVRAM;
+
+	while (buf_idx < len) {
+		buf_size = min_t(u32, (len - buf_idx),
+				 MCP_DRV_NVM_BUF_LEN);
+		params.nvm_common.offset = ((buf_size <<
+					     DRV_MB_PARAM_NVM_LEN_SHIFT) |
+					    addr) + buf_idx;
+		params.nvm_wr.buf_size	= buf_size;
+		params.nvm_wr.buf = (u32 *)&p_buf[buf_idx];
+
+		rc = qed_mcp_nvm_command(p_hwfn, p_ptt, &params);
+		if (rc ||
+		    ((params.nvm_common.resp != FW_MSG_CODE_NVM_OK) &&
+		     (params.nvm_common.resp !=
+		      FW_MSG_CODE_NVM_PUT_FILE_FINISH_OK)))
+			DP_NOTICE(cdev, "MCP command rc = %d\n", rc);
+
+		buf_idx += buf_size;
+	}
+
+	cdev->mcp_nvm_resp = params.nvm_common.resp;
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return rc;
+}
+
+int qed_mcp_nvm_set_secure_mode(struct qed_dev *cdev,
+				u32 addr)
+{
+	struct qed_hwfn *p_hwfn = QED_LEADING_HWFN(cdev);
+	struct qed_mcp_nvm_params params;
+	struct qed_ptt *p_ptt;
+	int rc;
+
+	p_ptt = qed_ptt_acquire(p_hwfn);
+	if (!p_ptt)
+		return -EBUSY;
+
+	memset(&params, 0, sizeof(struct qed_mcp_nvm_params));
+	params.type = QED_MCP_CMD;
+	params.nvm_common.cmd = DRV_MSG_CODE_SET_SECURE_MODE;
+	params.nvm_common.offset = addr;
+
+	rc =  qed_mcp_nvm_command(p_hwfn, p_ptt, &params);
+	cdev->mcp_nvm_resp = params.nvm_common.resp;
+	qed_ptt_release(p_hwfn, p_ptt);
+
+	return rc;
+}
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.h b/drivers/net/ethernet/qlogic/qed/qed_mcp.h
index 50917a2..ab0f0a5 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_mcp.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.h
@@ -92,6 +92,33 @@ struct qed_mcp_nvm_common {
 	u32	cmd;
 };
 
+struct qed_mcp_nvm_rd {
+	u32	*buf_size;
+	u32	*buf;
+};
+
+struct qed_mcp_nvm_wr {
+	u32	buf_size;
+	u32	*buf;
+};
+
+enum qed_mcp_num_parameters_type {
+	QED_MCP_CMD,
+	QED_MCP_NVM_RD,
+	QED_MCP_NVM_WR,
+};
+
+struct qed_mcp_nvm_params {
+	enum qed_mcp_num_parameters_type	type;
+
+	struct qed_mcp_nvm_common		nvm_common;
+
+	union {
+		struct qed_mcp_nvm_rd		nvm_rd;
+		struct qed_mcp_nvm_wr		nvm_wr;
+	};
+};
+
 struct qed_mcp_drv_version {
 	u32	version;
 	u8	name[MCP_DRV_VER_STR_SIZE - 4];
@@ -237,6 +264,82 @@ int qed_mcp_set_led(struct qed_hwfn *p_hwfn,
 		    struct qed_ptt *p_ptt,
 		    enum qed_led_mode mode);
 
+/**
+ * @brief Change security mode to allow writing faulty boards
+ *
+ *  @param cdev
+ *  @param addr - nvm offset
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_set_secure_mode(struct qed_dev *cdev,
+				u32 addr);
+
+/**
+ * @brief Write to nvm
+ *
+ *  @param cdev
+ *  @param addr - nvm offset
+ *  @param cmd - nvm command
+ *  @param p_buf - nvm write buffer
+ *  @param len - buffer len
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_write(struct qed_dev *cdev,
+		      u32 cmd,
+		      u32 addr,
+		      u8 *p_buf,
+		      u32 len);
+
+/**
+ * @brief Write a file image to nvram
+ *
+ *  @param cdev
+ *  @param addr - nvm offset
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_put_file_begin(struct qed_dev *cdev,
+			       u32 addr);
+
+/**
+ * @brief Delete a file image from nvram
+ *
+ *  @param cdev
+ *  @param addr - nvm offset
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_del_file(struct qed_dev *cdev,
+			 u32 addr);
+
+/**
+ * @brief Check latest mfw response in regard to nvm access
+ *
+ *  @param cdev
+ *  @param p_buf - nvm write buffer
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_resp(struct qed_dev *cdev,
+		     u8 *p_buf);
+
+/**
+ * @brief Read from nvm
+ *
+ *  @param cdev
+ *  @param addr - nvm offset
+ *  @param p_buf - nvm write buffer
+ *  @param len - buffer len
+ *
+ * @return int - 0 - operation was successful.
+ */
+int qed_mcp_nvm_read(struct qed_dev *cdev,
+		     u32 addr,
+		     u8	*p_buf,
+		     u32 len);
+
 /* Using hwfn number (and not pf_num) is required since in CMT mode,
  * same pf_num may be used by two different hwfn
  * TODO - this shouldn't really be in .h file, but until all fields
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 0f11685..50580d2 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -389,6 +389,33 @@ static u32 qede_get_link(struct net_device *dev)
 	return current_link.link_up;
 }
 
+static int qede_get_eeprom_len(struct net_device *ndev)
+{
+	struct qede_dev *edev = netdev_priv(ndev);
+
+	return edev->dev_info.common.flash_size;
+}
+
+static int qede_get_eeprom(struct net_device *dev,
+			    struct ethtool_eeprom *eeprom, u8 *eebuf)
+{
+	struct qede_dev *edev = netdev_priv(dev);
+
+	return edev->ops->common->nvm_get_cmd(edev->cdev, eeprom->magic,
+					      eeprom->offset, eebuf,
+					      eeprom->len);
+}
+
+static int qede_set_eeprom(struct net_device *dev,
+			    struct ethtool_eeprom *eeprom, u8 *eebuf)
+{
+	struct qede_dev *edev = netdev_priv(dev);
+
+	return edev->ops->common->nvm_set_cmd(edev->cdev, eeprom->magic,
+					      eeprom->offset, eebuf,
+					      eeprom->len);
+}
+
 static void qede_get_ringparam(struct net_device *dev,
 			       struct ethtool_ringparam *ering)
 {
@@ -839,6 +866,9 @@ static const struct ethtool_ops qede_ethtool_ops = {
 	.set_msglevel = qede_set_msglevel,
 	.nway_reset = qede_nway_reset,
 	.get_link = qede_get_link,
+	.get_eeprom_len = qede_get_eeprom_len,
+	.get_eeprom = qede_get_eeprom,
+	.set_eeprom = qede_set_eeprom,
 	.get_ringparam = qede_get_ringparam,
 	.set_ringparam = qede_set_ringparam,
 	.get_pauseparam = qede_get_pauseparam,
diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h
index e5de42b..497cce9 100644
--- a/include/linux/qed/qed_if.h
+++ b/include/linux/qed/qed_if.h
@@ -270,6 +270,36 @@ struct qed_common_ops {
 				      struct qed_chain *p_chain);
 
 /**
+ * @brief nvm_get_cmd - Invoke mcp nvm get command
+ *
+ * @param cdev
+ * @param cmd - qed_mcp command
+ * @param offset
+ * @param buf - buffer
+ * @param len - buffer length
+ *
+ * @return 0 on success, error otherwise.
+ */
+	int (*nvm_get_cmd)(struct qed_dev *cdev,
+			   u32 cmd, u32 offset,
+			   u8 *buf, u32 len);
+
+/**
+ * @brief nvm_put_cmd - Invoke mcp nvm get command
+ *
+ * @param cdev
+ * @param cmd - qed_mcp command
+ * @param offset
+ * @param buf - buffer
+ * @param len - buffer length
+ *
+ * @return 0 on success, error otherwise.
+ */
+	int (*nvm_set_cmd)(struct qed_dev *cdev,
+			   u32 cmd, u32 offset,
+			   u8 *buf, u32 len);
+
+/**
  * @brief set_led - Configure LED mode
  *
  * @param cdev
-- 
1.9.3

Re: [PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[David Miller]

From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Sun, 17 Apr 2016 22:26:35 +0300

> There's quite a bit of `magic' here [based on the ethtool `magic'
> field] which is retained by the user application responsible for
> interacting with the driver for the purpose of upgrading the nvram
> image.

This is not how the 'magic' value of the eeprom structure is specified
to be used, please use it the way it is supposed to be used rather
than inventing semantics which only apply to your device.

The entire point of defining a common interface for device driver
interface interaction with the user is exactly so that driver authors
don't do things like this.

RE: [PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[Yuval Mintz]

> > There's quite a bit of `magic' here [based on the ethtool `magic'
> > field] which is retained by the user application responsible for
> > interacting with the driver for the purpose of upgrading the nvram
> > image.
> 
> This is not how the 'magic' value of the eeprom structure is specified to be used,
> please use it the way it is supposed to be used rather than inventing semantics
> which only apply to your device.
> 
> The entire point of defining a common interface for device driver interface
> interaction with the user is exactly so that driver authors don't do things like this.

While I obviously get what you're writing, I actually think this is the
more revealing API option.

I.e., the bottom line is that write to this device's nvram is complex in
nature and requires interacting with the management firmware.
The other immediate option would have been to implement this
in driver as simple read/write toward the management firmware,
and let the management firmware analyze what it needs to do with
the data based on hidden meta-data inside the buffer written by the
application.

While this might be less 'extending' of the ethtool API, it's a security-
in-obscurity kind of API, where a reviewer can't possibly know how to
perform read/write from reading the driver code.

The third option I see is to completely ditch this, stating that although
We obviously CAN set the non-volatile memory we CAN'T do it with the
regular API, and to move into doing this via some proprietary API such
as debugfs.

Thanks,
Yuval 

Re: [PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[David Miller]

From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Mon, 18 Apr 2016 03:59:42 +0000

> The third option I see is to completely ditch this, stating that although
> We obviously CAN set the non-volatile memory we CAN'T do it with the
> regular API, and to move into doing this via some proprietary API such
> as debugfs.

Why go to debugfs rather than gracefully extending the ethtool stuff to
explicitly support what you need?

Anyone who says debugfs just isn't willing to design things correctly.

That's why I basically have started rejecting everything that tries
to use debugfs in the networking, it's the "I'm too damn lazy to
do this properly" option and I simply won't stand for crap like that
any more.

Re: [PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[Yuval Mintz]

>> The third option I see is to completely ditch this, stating that although
>> We obviously CAN set the non-volatile memory we CAN'T do it with the
>> regular API, and to move into doing this via some proprietary API such
>> as debugfs.

> Why go to debugfs rather than gracefully extending the ethtool stuff to
> explicitly support what you need?

Gracefully extend it to where?

The most I can learn from this is that some adapters are in
need of additional metadata for doing this sort of stuff.
The state-machine itself is propriety and I don't think anyone
would gain from trying to formalize anything else from it.
If by 'graceful extension' of ethtool API you mean adding an
additional field or two for metadata - sure, I can do that,
I just don't think of it as graceful. ;-)

And if this is actually the way we want to take this, I would
still [don't get upset ;-)] argue in favor of overloading the
'magic' field for this purpose, as bnx2x and bnxt are already
doing.
Basically if the 'magic' would also contain metadata that
could be verified as appropriate [I.e., by having valid ranges],
it would still fit as a safety measure for guaranteeing the
correctness of the write request.

Re: [PATCH net-next 2/5] qede: Add support for ethtool private flags

[Sergei Shtylyov]

Hello.

On 4/17/2016 10:26 PM, Yuval Mintz wrote:

> Adds a getter for the interfaces private flags.
> The only parameter currently supported is whether the interface is a
> coupled function [required for supporting 100g].
>
> Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
> ---
>   drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 27 +++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> index f87e83b..5ba6b2a 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
[...]
> @@ -185,6 +200,17 @@ static int qede_get_sset_count(struct net_device *dev, int stringset)
>   	}
>   }
>
> +static u32 qede_get_priv_flags(struct net_device *dev)
> +{
> +	struct qede_dev *edev = netdev_priv(dev);
> +	u32 flags = 0;
> +
> +	flags |= (!!(edev->dev_info.common.num_hwfns > 1)) <<
> +		 QEDE_PRI_FLAG_CMT;

    Why not just '='?

> +
> +	return flags;

    ... or direct return of the value above?

> +}
> +
>   static int qede_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>   {
>   	struct qede_dev *edev = netdev_priv(dev);
[...]

MBR, Sergei

Re: [PATCH net-next 2/5] qede: Add support for ethtool private flags

[Yuval Mintz]

>> +     flags |= (!!(edev->dev_info.common.num_hwfns > 1)) <<
>> +              QEDE_PRI_FLAG_CMT;

>  Why not just '='?

>> +
>> +     return flags;

>  ... or direct return of the value above?

Probably wanted to lay the basis for future flags.
But you're right in that there's no need for it.
I'll change it in V2.

Thanks,
Yuval

RE: [PATCH net-next 5/5] qed*: Add support for read/write of eeprom

[Yuval Mintz]

> >> The third option I see is to completely ditch this, stating that
> >> although We obviously CAN set the non-volatile memory we CAN'T do it
> >> with the regular API, and to move into doing this via some
> >> proprietary API such as debugfs.
> 
> > Why go to debugfs rather than gracefully extending the ethtool stuff
> > to explicitly support what you need?
> 
> Gracefully extend it to where?
> 
> The most I can learn from this is that some adapters are in need of additional
> metadata for doing this sort of stuff.
> The state-machine itself is propriety and I don't think anyone would gain from
> trying to formalize anything else from it.
> If by 'graceful extension' of ethtool API you mean adding an additional field or
> two for metadata - sure, I can do that, I just don't think of it as graceful. ;-)
> 
> And if this is actually the way we want to take this, I would still [don't get upset
> ;-)] argue in favor of overloading the 'magic' field for this purpose, as bnx2x and
> bnxt are already doing.
> Basically if the 'magic' would also contain metadata that could be verified as
> appropriate [I.e., by having valid ranges], it would still fit as a safety measure for
> guaranteeing the correctness of the write request.

Hi Dave,

I'll probably send V2 [relatively] shortly, and it looks like I'll drop this one out
as I don't yet understand the expected direction through which to solve this.

I'd appreciate any suggested pointers here.

Thanks,
Yuval