[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2024-02-06 (igb, igc)

[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2024-02-06 (igb, igc)

[Tony Nguyen]

This series contains updates to igb and igc drivers.

Kunwu Chan adjusts firmware version string implementation to resolve
possible NULL pointer issue for igb.

Sasha removes workaround on igc.

The following are changes since commit 1ce2654d87e2fb91fea83b288bd9b2641045e42a:
  net: stmmac: xgmac: fix a typo of register name in DPP safety handling
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE

Kunwu Chan (1):
  igb: Fix string truncation warnings in igb_set_fw_version

Sasha Neftin (1):
  igc: Remove temporary workaround

 drivers/net/ethernet/intel/igb/igb.h      |  2 +-
 drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++-----------
 drivers/net/ethernet/intel/igc/igc_phy.c  |  6 +---
 3 files changed, 20 insertions(+), 23 deletions(-)

-- 
2.41.0

[PATCH net 1/2] igb: Fix string truncation warnings in igb_set_fw_version

[Tony Nguyen]

From: Kunwu Chan <chentao@kylinos.cn>

Commit 1978d3ead82c ("intel: fix string truncation warnings")
fixes '-Wformat-truncation=' warnings in igb_main.c by using kasprintf.

drivers/net/ethernet/intel/igb/igb_main.c:3092:53: warning：‘%d’ directive output may be truncated writing between 1 and 5 bytes into a region of size between 1 and 13 [-Wformat-truncation=]
 3092 |                                  "%d.%d, 0x%08x, %d.%d.%d",
      |                                                     ^~
drivers/net/ethernet/intel/igb/igb_main.c:3092:34: note：directive argument in the range [0, 65535]
 3092 |                                  "%d.%d, 0x%08x, %d.%d.%d",
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/intel/igb/igb_main.c:3092:34: note：directive argument in the range [0, 65535]
drivers/net/ethernet/intel/igb/igb_main.c:3090:25: note：‘snprintf’ output between 23 and 43 bytes into a destination of size 32

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure.

Fix this warning by using a larger space for adapter->fw_version,
and then fall back and continue to use snprintf.

Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
Cc: Kunwu Chan <kunwu.chan@hotmail.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h      |  2 +-
 drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++-----------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index a2b759531cb7..3c2dc7bdebb5 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -637,7 +637,7 @@ struct igb_adapter {
 		struct timespec64 period;
 	} perout[IGB_N_PEROUT];
 
-	char fw_version[32];
+	char fw_version[48];
 #ifdef CONFIG_IGB_HWMON
 	struct hwmon_buff *igb_hwmon_buff;
 	bool ets;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 4df8d4153aa5..cebb44f51d5f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3069,7 +3069,6 @@ void igb_set_fw_version(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	struct e1000_fw_version fw;
-	char *lbuf;
 
 	igb_get_fw_version(hw, &fw);
 
@@ -3077,34 +3076,36 @@ void igb_set_fw_version(struct igb_adapter *adapter)
 	case e1000_i210:
 	case e1000_i211:
 		if (!(igb_get_flash_presence_i210(hw))) {
-			lbuf = kasprintf(GFP_KERNEL, "%2d.%2d-%d",
-					 fw.invm_major, fw.invm_minor,
-					 fw.invm_img_type);
+			snprintf(adapter->fw_version,
+				 sizeof(adapter->fw_version),
+				 "%2d.%2d-%d",
+				 fw.invm_major, fw.invm_minor,
+				 fw.invm_img_type);
 			break;
 		}
 		fallthrough;
 	default:
 		/* if option rom is valid, display its version too */
 		if (fw.or_valid) {
-			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x, %d.%d.%d",
-					 fw.eep_major, fw.eep_minor,
-					 fw.etrack_id, fw.or_major, fw.or_build,
-					 fw.or_patch);
+			snprintf(adapter->fw_version,
+				 sizeof(adapter->fw_version),
+				 "%d.%d, 0x%08x, %d.%d.%d",
+				 fw.eep_major, fw.eep_minor, fw.etrack_id,
+				 fw.or_major, fw.or_build, fw.or_patch);
 		/* no option rom */
 		} else if (fw.etrack_id != 0X0000) {
-			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x",
-					 fw.eep_major, fw.eep_minor,
-					 fw.etrack_id);
+			snprintf(adapter->fw_version,
+				 sizeof(adapter->fw_version),
+				 "%d.%d, 0x%08x",
+				 fw.eep_major, fw.eep_minor, fw.etrack_id);
 		} else {
-			lbuf = kasprintf(GFP_KERNEL, "%d.%d.%d", fw.eep_major,
-					 fw.eep_minor, fw.eep_build);
+			snprintf(adapter->fw_version,
+				 sizeof(adapter->fw_version),
+				 "%d.%d.%d",
+				 fw.eep_major, fw.eep_minor, fw.eep_build);
 		}
 		break;
 	}
-
-	/* the truncate happens here if it doesn't fit */
-	strscpy(adapter->fw_version, lbuf, sizeof(adapter->fw_version));
-	kfree(lbuf);
 }
 
 /**
-- 
2.41.0

[PATCH net 2/2] igc: Remove temporary workaround

[Tony Nguyen]

From: Sasha Neftin <sasha.neftin@intel.com>

PHY_CONTROL register works as defined in the IEEE 802.3 specification
(IEEE 802.3-2008 22.2.4.1). Tide up the temporary workaround.

Fixes: 5586838fe9ce ("igc: Add code for PHY support")
Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_phy.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_phy.c b/drivers/net/ethernet/intel/igc/igc_phy.c
index 7cd8716d2ffa..861f37076861 100644
--- a/drivers/net/ethernet/intel/igc/igc_phy.c
+++ b/drivers/net/ethernet/intel/igc/igc_phy.c
@@ -130,11 +130,7 @@ void igc_power_down_phy_copper(struct igc_hw *hw)
 	/* The PHY will retain its settings across a power down/up cycle */
 	hw->phy.ops.read_reg(hw, PHY_CONTROL, &mii_reg);
 	mii_reg |= MII_CR_POWER_DOWN;
-
-	/* Temporary workaround - should be removed when PHY will implement
-	 * IEEE registers as properly
-	 */
-	/* hw->phy.ops.write_reg(hw, PHY_CONTROL, mii_reg);*/
+	hw->phy.ops.write_reg(hw, PHY_CONTROL, mii_reg);
 	usleep_range(1000, 2000);
 }
 
-- 
2.41.0

Re: [PATCH net 2/2] igc: Remove temporary workaround

[Jakub Kicinski]

On Tue,  6 Feb 2024 13:28:18 -0800 Tony Nguyen wrote:
> From: Sasha Neftin <sasha.neftin@intel.com>
> 
> PHY_CONTROL register works as defined in the IEEE 802.3 specification
> (IEEE 802.3-2008 22.2.4.1). Tide up the temporary workaround.

Any more info on this one?
What's the user impact?
What changed (e.g. which FW version fixed it)?
-- 
pw-bot: cr

Re: [PATCH net 2/2] igc: Remove temporary workaround

[Sasha Neftin]

On 09/02/2024 4:33, Jakub Kicinski wrote:
> On Tue,  6 Feb 2024 13:28:18 -0800 Tony Nguyen wrote:
>> From: Sasha Neftin <sasha.neftin@intel.com>
>>
>> PHY_CONTROL register works as defined in the IEEE 802.3 specification
>> (IEEE 802.3-2008 22.2.4.1). Tide up the temporary workaround.
> 
> Any more info on this one?
> What's the user impact?
> What changed (e.g. which FW version fixed it)?

User impact: PHY could be powered down when the link is down (ip link 
set down <device>)

Fix by PHY firmware and deployed via OEM updates (automatically, with 
OEM SW/FW updates). We checked the IEEE behavior and removed w/a.

The PHY vendor no longer works with Intel, but I can say this was fixed 
on a very early silicon step (years ago).

Re: [PATCH net 2/2] igc: Remove temporary workaround

[Jakub Kicinski]

On Sun, 11 Feb 2024 08:53:36 +0200 Sasha Neftin wrote:
> > Any more info on this one?
> > What's the user impact?
> > What changed (e.g. which FW version fixed it)?  
> 
> User impact: PHY could be powered down when the link is down (ip link 
> set down <device>)

to make it a tiny bit clearer:

s/could/can now/ ?
s/link is down/device is down/ ?
 
> Fix by PHY firmware and deployed via OEM updates (automatically, with 
> OEM SW/FW updates). We checked the IEEE behavior and removed w/a.
> 
> The PHY vendor no longer works with Intel, but I can say this was fixed 
> on a very early silicon step (years ago).

And the versions of the PHY components are not easily accessible so we
can't point to the version that was buggy or at least the oldest you
tested? If that's the case - it is what it is, please repost with the
improved commit msg.

Re: [PATCH net 2/2] igc: Remove temporary workaround

[Sasha Neftin]

On 12/02/2024 19:08, Jakub Kicinski wrote:
> On Sun, 11 Feb 2024 08:53:36 +0200 Sasha Neftin wrote:
>>> Any more info on this one?
>>> What's the user impact?
>>> What changed (e.g. which FW version fixed it)?
>>
>> User impact: PHY could be powered down when the link is down (ip link
>> set down <device>)
> 
> to make it a tiny bit clearer:
> 
> s/could/can now/ ?

yes

> s/link is down/device is down/ ?

I meant to the Ethernet link. The device is enumerated and PCIe 
configuration space is accessible. (ip link set down <device>)

>   
>> Fix by PHY firmware and deployed via OEM updates (automatically, with
>> OEM SW/FW updates). We checked the IEEE behavior and removed w/a.
>>
>> The PHY vendor no longer works with Intel, but I can say this was fixed
>> on a very early silicon step (years ago).
> 
> And the versions of the PHY components are not easily accessible so we
> can't point to the version that was buggy or at least the oldest you
> tested? If that's the case - it is what it is, please repost with the
> improved commit msg.

The version of the PHY FW is easily accessible (ethtool -i <device>). 
The problem is that I can not point to the specific buggy version. It 
was on the very early silicon step (it is what it is).
I will add the oldest version of the NVM we tested..