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 <[email protected]>
Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Cc: [email protected]
---
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
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 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 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 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?