diff mbox series

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

Message ID 20230405153900.747-1-i.bornyakov@metrotek.ru
State New
Headers
Series [net] net: sfp: initialize sfp->i2c_block_size at sfp allocation |

Commit Message

Ivan Bornyakov April 5, 2023, 3:39 p.m. UTC 
  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(-)

Comments

Andrew Lunn April 5, 2023, 7:35 p.m. UTC | #1 
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
Ivan Bornyakov April 5, 2023, 8:41 p.m. UTC | #2 
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.
Andrew Lunn April 5, 2023, 8:51 p.m. UTC | #3 
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
Ivan Bornyakov April 5, 2023, 9:13 p.m. UTC | #4 
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.
Jakub Kicinski April 7, 2023, 2:02 a.m. UTC | #5 
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?
diff mbox series

Patch

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);