[PATCH net] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[PATCH net] 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 not inserted
since boot, ethtool -m leads to 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 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 ethtool -m with SFP module removed will fail the same way, i.e.
-EREMOTEIO, in both cases and without errors from I2C adapter.

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] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Andrew Lunn]

On Wed, Apr 05, 2023 at 06:39:00PM +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 not inserted
> since boot, ethtool -m leads to 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

Do i understand you correct in that this is when the SFP cage has
always been empty? The I2C transaction is going to fail whatever the
length is.

> If SFP module was plugged then removed at least once,
> sfp->i2c_block_size will be initialized and ethtool -m will fail with
> different error
> 
>   # ethtool -m xge0
>   Cannot get Module EEPROM data: Remote I/O error

So again, the SFP cage is empty?

I wonder if a better fix is to use

sfp->state & SFP_F_PRESENT

in sfp_module_eeprom() and sfp_module_eeprom_by_page() and don't even
do the I2C read if there is no module in the cage?

   Andrew

Re: [PATCH net] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Ivan Bornyakov]

On Wed, Apr 05, 2023 at 09:35:31PM +0200, Andrew Lunn wrote:
> On Wed, Apr 05, 2023 at 06:39:00PM +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 not inserted
> > since boot, ethtool -m leads to 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
> 
> Do i understand you correct in that this is when the SFP cage has
> always been empty? The I2C transaction is going to fail whatever the
> length is.
> 

Yes, SFP cage is empty, I2C transaction will fail anyways, but not all
I2C controllers are happy about zero-length reads.

> > If SFP module was plugged then removed at least once,
> > sfp->i2c_block_size will be initialized and ethtool -m will fail with
> > different error
> > 
> >   # ethtool -m xge0
> >   Cannot get Module EEPROM data: Remote I/O error
> 
> So again, the SFP cage is empty?
> 
> I wonder if a better fix is to use
> 
> sfp->state & SFP_F_PRESENT
> 
> in sfp_module_eeprom() and sfp_module_eeprom_by_page() and don't even
> do the I2C read if there is no module in the cage?
> 

This is also worthy addition to sfp.c, but IMHO sfp->i2c_block_size
initialization still need to be fixed since

  $ grep -c "sfp_read(" drivers/net/phy/sfp.c
  31

and I can't vouch all of them are possible only after SFP module
insertion. Also for future proof reason.

Re: [PATCH net] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Andrew Lunn]

On Wed, Apr 05, 2023 at 11:41:16PM +0300, Ivan Bornyakov wrote:
> On Wed, Apr 05, 2023 at 09:35:31PM +0200, Andrew Lunn wrote:
> > On Wed, Apr 05, 2023 at 06:39:00PM +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 not inserted
> > > since boot, ethtool -m leads to 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
> > 
> > Do i understand you correct in that this is when the SFP cage has
> > always been empty? The I2C transaction is going to fail whatever the
> > length is.
> > 
> 
> Yes, SFP cage is empty, I2C transaction will fail anyways, but not all
> I2C controllers are happy about zero-length reads.
> 
> > > If SFP module was plugged then removed at least once,
> > > sfp->i2c_block_size will be initialized and ethtool -m will fail with
> > > different error
> > > 
> > >   # ethtool -m xge0
> > >   Cannot get Module EEPROM data: Remote I/O error
> > 
> > So again, the SFP cage is empty?
> > 
> > I wonder if a better fix is to use
> > 
> > sfp->state & SFP_F_PRESENT
> > 
> > in sfp_module_eeprom() and sfp_module_eeprom_by_page() and don't even
> > do the I2C read if there is no module in the cage?
> > 
> 
> This is also worthy addition to sfp.c, but IMHO sfp->i2c_block_size
> initialization still need to be fixed since
> 
>   $ grep -c "sfp_read(" drivers/net/phy/sfp.c
>   31
> 
> and I can't vouch all of them are possible only after SFP module
> insertion. Also for future proof reason.

I think everything else should be safe. A lot of those reads are for
the HWMON code. And the HWMON code only registers when the module is
inserted.

How about two patches, what you have here, plus checking sfp->state &
SFP_F_PRESENT in the ethtool functions?

	Andrew

Re: [PATCH net] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Ivan Bornyakov]

On Wed, Apr 05, 2023 at 10:51:38PM +0200, Andrew Lunn wrote:
> On Wed, Apr 05, 2023 at 11:41:16PM +0300, Ivan Bornyakov wrote:
> > On Wed, Apr 05, 2023 at 09:35:31PM +0200, Andrew Lunn wrote:
> > > On Wed, Apr 05, 2023 at 06:39:00PM +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 not inserted
> > > > since boot, ethtool -m leads to 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
> > > 
> > > Do i understand you correct in that this is when the SFP cage has
> > > always been empty? The I2C transaction is going to fail whatever the
> > > length is.
> > > 
> > 
> > Yes, SFP cage is empty, I2C transaction will fail anyways, but not all
> > I2C controllers are happy about zero-length reads.
> > 
> > > > If SFP module was plugged then removed at least once,
> > > > sfp->i2c_block_size will be initialized and ethtool -m will fail with
> > > > different error
> > > > 
> > > >   # ethtool -m xge0
> > > >   Cannot get Module EEPROM data: Remote I/O error
> > > 
> > > So again, the SFP cage is empty?
> > > 
> > > I wonder if a better fix is to use
> > > 
> > > sfp->state & SFP_F_PRESENT
> > > 
> > > in sfp_module_eeprom() and sfp_module_eeprom_by_page() and don't even
> > > do the I2C read if there is no module in the cage?
> > > 
> > 
> > This is also worthy addition to sfp.c, but IMHO sfp->i2c_block_size
> > initialization still need to be fixed since
> > 
> >   $ grep -c "sfp_read(" drivers/net/phy/sfp.c
> >   31
> > 
> > and I can't vouch all of them are possible only after SFP module
> > insertion. Also for future proof reason.
> 
> I think everything else should be safe. A lot of those reads are for
> the HWMON code. And the HWMON code only registers when the module is
> inserted.
> 
> How about two patches, what you have here, plus checking sfp->state &
> SFP_F_PRESENT in the ethtool functions?
> 
> 	Andrew

Sure, will do in the near future.

Re: [PATCH net] net: sfp: initialize sfp->i2c_block_size at sfp allocation

[Jakub Kicinski]

On Wed,  5 Apr 2023 18:39:00 +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 not inserted
> since boot, ethtool -m leads to 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 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 ethtool -m with SFP module removed will fail the same way, i.e.
> -EREMOTEIO, in both cases and without errors from I2C adapter.

Hi Russell - yes / no / come back when both patches are ready?