diff mbox series

[net-next] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround

Message ID E1khJlv-0003Jq-ET@rmk-PC.armlinux.org.uk (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 0 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 84 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 1
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Russell King (Oracle) Nov. 23, 2020, 9:53 p.m. UTC 
Add a workaround for the VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0
GPON module which the manufacturer states needs single byte I2C reads
to the EEPROM.

Reported-by: Thomas Schreiber <tschreibe@gmail.com>
Tested-by: Thomas Schreiber <tschreibe@gmail.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 45 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

Comments

kernel test robot Nov. 24, 2020, 12:18 a.m. UTC | #1 
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
Andrew Lunn Nov. 24, 2020, 12:20 a.m. UTC | #2 
> @@ -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?

     Andrew
Russell King (Oracle) Nov. 24, 2020, 9:43 a.m. UTC | #3 
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.
Russell King (Oracle) Nov. 25, 2020, 2 p.m. UTC | #4 
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.
Russell King (Oracle) Dec. 2, 2020, 1:11 p.m. UTC | #5 
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.
diff mbox series

Patch

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