| Message ID | E1khJlv-0003Jq-ET@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround | expand |
Hi Russell,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Russell-King/net-sfp-VSOL-V2801F-CarlitoxxPro-CPGOS03-0490-v2-0-workaround/20201124-055921
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 8e1e33ffa696b2d779dd5cd422a80960b88e508c
config: arc-randconfig-r016-20201123 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/90849b26224de3b2e508f1c3fa31665f4fd72d0a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Russell-King/net-sfp-VSOL-V2801F-CarlitoxxPro-CPGOS03-0490-v2-0-workaround/20201124-055921
git checkout 90849b26224de3b2e508f1c3fa31665f4fd72d0a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/phy/sfp.c: In function 'sfp_i2c_read':
>> drivers/net/phy/sfp.c:339:9: warning: variable 'block_size' set but not used [-Wunused-but-set-variable]
339 | size_t block_size;
| ^~~~~~~~~~
vim +/block_size +339 drivers/net/phy/sfp.c
334
335 static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
336 size_t len)
337 {
338 struct i2c_msg msgs[2];
> 339 size_t block_size;
340 size_t this_len;
341 u8 bus_addr;
342 int ret;
343
344 if (a2) {
345 block_size = 16;
346 bus_addr = 0x51;
347 } else {
348 block_size = sfp->i2c_block_size;
349 bus_addr = 0x50;
350 }
351
352 msgs[0].addr = bus_addr;
353 msgs[0].flags = 0;
354 msgs[0].len = 1;
355 msgs[0].buf = &dev_addr;
356 msgs[1].addr = bus_addr;
357 msgs[1].flags = I2C_M_RD;
358 msgs[1].len = len;
359 msgs[1].buf = buf;
360
361 while (len) {
362 this_len = len;
363 if (this_len > sfp->i2c_block_size)
364 this_len = sfp->i2c_block_size;
365
366 msgs[1].len = this_len;
367
368 ret = i2c_transfer(sfp->i2c, msgs, ARRAY_SIZE(msgs));
369 if (ret < 0)
370 return ret;
371
372 if (ret != ARRAY_SIZE(msgs))
373 break;
374
375 msgs[1].buf += this_len;
376 dev_addr += this_len;
377 len -= this_len;
378 }
379
380 return msgs[1].buf - (u8 *)buf;
381 }
382
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, Nov 24, 2020 at 01:20:21AM +0100, Andrew Lunn wrote: > > @@ -335,10 +336,19 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, > > size_t len) > > { > > struct i2c_msg msgs[2]; > > - u8 bus_addr = a2 ? 0x51 : 0x50; > > + size_t block_size; > > size_t this_len; > > + u8 bus_addr; > > int ret; > > > > + if (a2) { > > + block_size = 16; > > + bus_addr = 0x51; > > Hi Russell, Thomas > > Does this man the diagnostic page can be read 16 bytes at a time, even > when the other page has to be 1 bytes at a time? That seems rather > odd. Or is the diagnostic page not implemented in these SFPs? SFF8472 requires that multibyte values are read using sequential reads. So we can't use single byte reads to read a multibyte value - it's just not atomic.
On Tue, Nov 24, 2020 at 08:18:56AM +0800, kernel test robot wrote: > All warnings (new ones prefixed by >>): > > drivers/net/phy/sfp.c: In function 'sfp_i2c_read': > >> drivers/net/phy/sfp.c:339:9: warning: variable 'block_size' set but not used [-Wunused-but-set-variable] > 339 | size_t block_size; > | ^~~~~~~~~~ I'm waiting for Thomas to re-test the fixed patch I sent, but Thomas seems to be of the opinion that there's no need to re-test, despite the fixed patch having the intended effect of changing the behaviour on the I2C bus. If nothing is forthcoming, I'm intending to drop the patch; we don't need to waste time supporting untested workarounds for what are essentially broken SFPs by vendors twisting the SFP MSA in the kernel. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 25, 2020 at 02:00:20PM +0000, Russell King - ARM Linux admin wrote: > On Tue, Nov 24, 2020 at 08:18:56AM +0800, kernel test robot wrote: > > All warnings (new ones prefixed by >>): > > > > drivers/net/phy/sfp.c: In function 'sfp_i2c_read': > > >> drivers/net/phy/sfp.c:339:9: warning: variable 'block_size' set but not used [-Wunused-but-set-variable] > > 339 | size_t block_size; > > | ^~~~~~~~~~ > > I'm waiting for Thomas to re-test the fixed patch I sent, but Thomas > seems to be of the opinion that there's no need to re-test, despite > the fixed patch having the intended effect of changing the behaviour > on the I2C bus. > > If nothing is forthcoming, I'm intending to drop the patch; we don't > need to waste time supporting untested workarounds for what are > essentially broken SFPs by vendors twisting the SFP MSA in the > kernel. I have had no further co-operation from Thomas so far. If I don't hear from someone who is able to test this module by this weekend, I will be dropping this patch from my repository. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index debf91412a72..1e347afa951e 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -219,6 +219,7 @@ struct sfp { struct sfp_bus *sfp_bus; struct phy_device *mod_phy; const struct sff_data *type; + size_t i2c_block_size; u32 max_power_mW; unsigned int (*get_state)(struct sfp *); @@ -335,10 +336,19 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, size_t len) { struct i2c_msg msgs[2]; - u8 bus_addr = a2 ? 0x51 : 0x50; + size_t block_size; size_t this_len; + u8 bus_addr; int ret; + if (a2) { + block_size = 16; + bus_addr = 0x51; + } else { + block_size = sfp->i2c_block_size; + bus_addr = 0x50; + } + msgs[0].addr = bus_addr; msgs[0].flags = 0; msgs[0].len = 1; @@ -350,8 +360,8 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, while (len) { this_len = len; - if (this_len > 16) - this_len = 16; + if (this_len > sfp->i2c_block_size) + this_len = sfp->i2c_block_size; msgs[1].len = this_len; @@ -1673,14 +1683,20 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report) u8 check; int ret; - ret = sfp_read(sfp, false, 0, &id, sizeof(id)); + /* Some modules (CarlitoxxPro CPGOS03-0490) do not support multibyte + * reads from the EEPROM, so start by reading the base identifying + * information one byte at a time. + */ + sfp->i2c_block_size = 1; + + ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base)); if (ret < 0) { if (report) dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret); return -EAGAIN; } - if (ret != sizeof(id)) { + if (ret != sizeof(id.base)) { dev_err(sfp->dev, "EEPROM short read: %d\n", ret); return -EAGAIN; } @@ -1719,6 +1735,25 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report) } } + /* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a + * single read. Switch back to reading 16 byte blocks unless we have + * a CarlitoxxPro module (rebranded VSOL V2801F). + */ + if (memcmp(id.base.vendor_name, "VSOL ", 16)) + sfp->i2c_block_size = 16; + + ret = sfp_read(sfp, false, SFP_CC_BASE + 1, &id.ext, sizeof(id.ext)); + if (ret < 0) { + if (report) + dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret); + return -EAGAIN; + } + + if (ret != sizeof(id.ext)) { + dev_err(sfp->dev, "EEPROM short read: %d\n", ret); + return -EAGAIN; + } + check = sfp_check(&id.ext, sizeof(id.ext) - 1); if (check != id.ext.cc_ext) { if (cotsworks) {