[PATCH net v2 0/2] fix EEPROM read of absent SFP module

[PATCH net v2 0/2] fix EEPROM read of absent SFP module

[Ivan Bornyakov]

The patchset is to improve EEPROM read requests when SFP module is
absent.

ChangeLog:
v1:
https://lore.kernel.org/netdev/20230405153900.747-1-i.bornyakov@metrotek.ru/
v2:
  * reword commit message of "net: sfp: initialize sfp->i2c_block_size
    at sfp allocation"
  * add second patch to eliminate excessive I2C transfers in
    sfp_module_eeprom() and sfp_module_eeprom_by_page()

Ivan Bornyakov (2):
  net: sfp: initialize sfp->i2c_block_size at sfp allocation
  net: sfp: avoid EEPROM read of absent SFP module

 drivers/net/phy/sfp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH net v2 1/2] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Ivan Bornyakov]

sfp->i2c_block_size is initialized at SFP module insertion in
sfp_sm_mod_probe(). Because of that, if SFP module was never inserted
since boot, sfp_read() call will lead to zero-length I2C read attempt,
and not all I2C controllers are happy with zero-length reads.

One way to issue sfp_read() on empty SFP cage is to execute ethtool -m.
If SFP module was never plugged since boot, there will be a zero-length
I2C read attempt.

  # ethtool -m xge0
  i2c i2c-3: adapter quirk: no zero length (addr 0x0050, size 0, read)
  Cannot get Module EEPROM data: Operation not supported

If SFP module was plugged then removed at least once,
sfp->i2c_block_size will be initialized and ethtool -m will fail with
different exit code and without I2C error

  # ethtool -m xge0
  Cannot get Module EEPROM data: Remote I/O error

Fix this by initializing sfp->i2_block_size at struct sfp allocation
stage so no wild sfp_read() could issue zero-length I2C read.

Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Cc: stable@vger.kernel.org
---
 drivers/net/phy/sfp.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 40c9a64c5e30..5663a184644d 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -212,6 +212,12 @@ static const enum gpiod_flags gpio_flags[] = {
 #define SFP_PHY_ADDR		22
 #define SFP_PHY_ADDR_ROLLBALL	17
 
+/* SFP_EEPROM_BLOCK_SIZE is the size of data chunk to read the EEPROM
+ * at a time. Some SFP modules and also some Linux I2C drivers do not like
+ * reads longer than 16 bytes.
+ */
+#define SFP_EEPROM_BLOCK_SIZE	16
+
 struct sff_data {
 	unsigned int gpios;
 	bool (*module_supported)(const struct sfp_eeprom_id *id);
@@ -1928,11 +1934,7 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	/* Some SFP modules and also some Linux I2C drivers do not like reads
-	 * longer than 16 bytes, so read the EEPROM in chunks of 16 bytes at
-	 * a time.
-	 */
-	sfp->i2c_block_size = 16;
+	sfp->i2c_block_size = SFP_EEPROM_BLOCK_SIZE;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -2615,6 +2617,7 @@ static struct sfp *sfp_alloc(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	sfp->dev = dev;
+	sfp->i2c_block_size = SFP_EEPROM_BLOCK_SIZE;
 
 	mutex_init(&sfp->sm_mutex);
 	mutex_init(&sfp->st_mutex);
-- 
2.39.2

Re: [PATCH net v2 1/2] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Andrew Lunn]

On Thu, Apr 06, 2023 at 04:08:32PM +0300, Ivan Bornyakov wrote:
> sfp->i2c_block_size is initialized at SFP module insertion in
> sfp_sm_mod_probe(). Because of that, if SFP module was never inserted
> since boot, sfp_read() call will lead to zero-length I2C read attempt,
> and not all I2C controllers are happy with zero-length reads.
> 
> One way to issue sfp_read() on empty SFP cage is to execute ethtool -m.
> If SFP module was never plugged since boot, there will be a zero-length
> I2C read attempt.
> 
>   # ethtool -m xge0
>   i2c i2c-3: adapter quirk: no zero length (addr 0x0050, size 0, read)
>   Cannot get Module EEPROM data: Operation not supported
> 
> If SFP module was plugged then removed at least once,
> sfp->i2c_block_size will be initialized and ethtool -m will fail with
> different exit code and without I2C error
> 
>   # ethtool -m xge0
>   Cannot get Module EEPROM data: Remote I/O error
> 
> Fix this by initializing sfp->i2_block_size at struct sfp allocation
> stage so no wild sfp_read() could issue zero-length I2C read.
> 
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
> Cc: stable@vger.kernel.org

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net v2 2/2] net: sfp: avoid EEPROM read of absent SFP module

[Ivan Bornyakov]

If SFP module is not present, it is sensible to fail sfp_module_eeprom()
and sfp_module_eeprom_by_page() early to avoid excessive I2C transfers
which are garanteed to fail.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/net/phy/sfp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 5663a184644d..6f32c2ab415d 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2481,6 +2481,9 @@ static int sfp_module_eeprom(struct sfp *sfp, struct ethtool_eeprom *ee,
 	unsigned int first, last, len;
 	int ret;
 
+	if (!(sfp->state & SFP_F_PRESENT))
+		return -ENODEV;
+
 	if (ee->len == 0)
 		return -EINVAL;
 
@@ -2513,6 +2516,9 @@ static int sfp_module_eeprom_by_page(struct sfp *sfp,
 				     const struct ethtool_module_eeprom *page,
 				     struct netlink_ext_ack *extack)
 {
+	if (!(sfp->state & SFP_F_PRESENT))
+		return -ENODEV;
+
 	if (page->bank) {
 		NL_SET_ERR_MSG(extack, "Banks not supported");
 		return -EOPNOTSUPP;
-- 
2.39.2

Re: [PATCH net v2 2/2] net: sfp: avoid EEPROM read of absent SFP module

[Andrew Lunn]

On Thu, Apr 06, 2023 at 04:08:33PM +0300, Ivan Bornyakov wrote:
> If SFP module is not present, it is sensible to fail sfp_module_eeprom()
> and sfp_module_eeprom_by_page() early to avoid excessive I2C transfers
> which are garanteed to fail.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v2 0/2] fix EEPROM read of absent SFP module

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu,  6 Apr 2023 16:08:31 +0300 you wrote:
> The patchset is to improve EEPROM read requests when SFP module is
> absent.
> 
> ChangeLog:
> v1:
> https://lore.kernel.org/netdev/20230405153900.747-1-i.bornyakov@metrotek.ru/
> v2:
>   * reword commit message of "net: sfp: initialize sfp->i2c_block_size
>     at sfp allocation"
>   * add second patch to eliminate excessive I2C transfers in
>     sfp_module_eeprom() and sfp_module_eeprom_by_page()
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] net: sfp: initialize sfp->i2c_block_size at sfp allocation
    https://git.kernel.org/netdev/net/c/813c2dd78618
  - [net,v2,2/2] net: sfp: avoid EEPROM read of absent SFP module
    https://git.kernel.org/netdev/net/c/bef227c1537c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html