Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips

[Russell King - ARM Linux admin]

On Wed, Dec 30, 2020 at 04:47:52PM +0100, Pali Rohár wrote:
> Workaround for GPON SFP modules based on VSOL V2801F brand was added in
> commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
> v2.0 workaround"). But it works only for ids explicitly added to the list.
> As there are more rebraded VSOL V2801F modules and OEM vendors are putting
> into vendor name random strings we cannot build workaround based on ids.
> 
> Moreover issue which that commit tried to workaround is generic not only to
> VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
> and RTL9601C chips.
> 
> They can be found for example in following GPON modules:
> * V-SOL V2801F
> * C-Data FD511GX-RM0
> * OPTON GP801R
> * BAUDCOM BD-1234-SFM
> * CPGOS03-0490 v2.0
> * Ubiquiti U-Fiber Instant
> * EXOT EGS1
> 
> Those Realtek chips have broken EEPROM emulator which for N-byte read
> operation returns just one byte of EEPROM data followed by N-1 zeros.
> 
> So introduce a new function sfp_id_needs_byte_io() which detects SFP
> modules with these Realtek chips which have broken EEPROM emulator based on
> N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
> this issue.
> 
> This patch fixes reading EEPROM content from SFP modules based on Realtek
> RTL8672 and RTL9601C chips.
> 
> Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
> Co-developed-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> ---
>  drivers/net/phy/sfp.c | 78 ++++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 91d74c1a920a..490e78a72dd6 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>  			size_t len)
>  {
>  	struct i2c_msg msgs[2];
> -	size_t block_size;
> +	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t block_size = sfp->i2c_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;
> -	}
> -

NAK. You are undoing something that is definitely needed. The
diagnostics must be read with sequential reads to be able to properly
read the 16-bit values.

The rest of the patch is fine; it's a shame the entire thing has
been spoilt by this extra addition that was not in the patch we had
been discussing off-list.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!