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);
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
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.
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
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.
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?
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Ivan Bornyakov -
Jakub Kicinski