netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Russell King - ARM Linux admin" <linux@armlinux.org.uk>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Marek Behún" <kabel@kernel.org>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
Date: Thu, 31 Dec 2020 16:40:30 +0100	[thread overview]
Message-ID: <20201231154030.tbofxtq4ravct4sg@pali> (raw)
In-Reply-To: <X+3ppQhbjCGFzs6P@lunn.ch>

On Thursday 31 December 2020 16:09:25 Andrew Lunn wrote:
> On Thu, Dec 31, 2020 at 01:14:10PM +0100, Pali Rohár wrote:
> > On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> > > On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > > > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > > > Hi Pali
> > > > > 
> > > > > I have to agree with Russell here. I would rather have no diagnostics
> > > > > than untrustable diagnostics.
> > > > 
> > > > Ok!
> > > > 
> > > > So should we completely skip hwmon_device_register_with_info() call
> > > > if (i2c_block_size < 2) ?
> > > 
> > > I don't think that alone is sufficient - there's also the matter of
> > > ethtool -m which will dump that information as well, and we don't want
> > > to offer it to userspace in an unreliable form.
> > 
> > Any idea/preference how to disable access to these registers?
> > 
> > > For reference, here is what SFF-8472 which defines the diagnostics, says
> > > about this:
> > > 
> > >   To guarantee coherency of the diagnostic monitoring data, the host is
> > >   required to retrieve any multi-byte fields from the diagnostic
> > >   monitoring data structure (IE: Rx Power MSB - byte 104 in A2h, Rx Power
> > >   LSB - byte 105 in A2h) by the use of a single two-byte read sequence
> > >   across the two-wire interface interface.
> > > 
> > >   The transceiver is required to ensure that any multi-byte fields which
> > >   are updated with diagnostic monitoring data (e.g. Rx Power MSB - byte
> > >   104 in A2h, Rx Power LSB - byte 105 in A2h) must have this update done
> > >   in a fashion which guarantees coherency and consistency of the data. In
> > >   other words, the update of a multi-byte field by the transceiver must
> > >   not occur such that a partially updated multi-byte field can be
> > >   transferred to the host. Also, the transceiver shall not update a
> > >   multi-byte field within the structure during the transfer of that
> > >   multi-byte field to the host, such that partially updated data would be
> > >   transferred to the host.
> > > 
> > > The first paragraph is extremely definitive in how these fields shall
> > > be read atomically - by a _single_ two-byte read sequence. From what
> > > you are telling us, these modules do not support that. Therefore, by
> > > definition, they do *not* support proper and reliable reporting of
> > > diagnostic data, and are non-conformant with the SFP MSAs.
> > > 
> > > So, they are basically broken, and the diagnostics can't be used to
> > > retrieve data that can be said to be useful.
> > 
> > I agree they are broken. We really should disable access to those 16bit
> > registers.
> > 
> > Anyway here is "datasheet" to some CarlitoxxPro SFP:
> > https://www.docdroid.net/hRsJ560/cpgos03-0490v2-datasheet-10-pdf
> > 
> > And on page 10 is written:
> > 
> >     The I2C system can support the mode of random address / single
> >     byteread which conform to SFF-8431.
> > 
> > Which seems to be wrong.
> 
> Searching around, i found:
> 
> http://read.pudn.com/downloads776/doc/3073304/RTL9601B-CG_Datasheet.pdf
> 
> It has two i2c busses, a master and a slave. The master bus can do
> multi-byte transfers. The slave bus description says nothing in words
> about multi-byte transfers, but the diagram shows only single byte
> transfers.

Yes. Only i2c slave is used for communication with external entity and
diagrams clearly shows that only single byte i2c transfers are
supported.

> So any SFP based around this device is broken.

Exactly. That is why I send this patch in a way that try to detect these
RTL chips and not particular vendors who create product on top of this
chip.

All SFP modules with this RTL9601B chip are broken and cannot be fixed.

Re-branders and OEM vendors like CarlitoxxPro or UBNT should stop saying
that they are compliant to SFP/SFF standards, because based on above
descriptions it is not truth.

> The silly thing is, it is reading/writing from a shadow SRAM. The CPU
> is not directly involved in an I2C transaction. So it could easily
> read multiple bytes from the SRAM and return them. But it would still
> need a synchronisation method to handle writes from the CPU to the
> SRAM, in order to make these word values safe.

But there is a still issue how to read these values from SRAM outside of
SFP module via i2c. And with current one-byte transfers of that i2c
slave on RTL9601B it is impossible via current SFP diagnostic API
specification.

  reply	other threads:[~2020-12-31 15:41 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2020-12-30 16:10   ` Russell King - ARM Linux admin
2020-12-30 16:56     ` Pali Rohár
2020-12-30 17:05       ` Russell King - ARM Linux admin
2020-12-30 17:13         ` Andrew Lunn
2020-12-30 17:43           ` Pali Rohár
2020-12-30 19:09             ` Russell King - ARM Linux admin
2020-12-30 19:49               ` Andrew Lunn
2020-12-31 12:14               ` Pali Rohár
2020-12-31 15:09                 ` Andrew Lunn
2020-12-31 15:40                   ` Pali Rohár [this message]
2020-12-31 15:30                 ` Andrew Lunn
2020-12-31 17:00                   ` Pali Rohár
2020-12-31 17:13                     ` Andrew Lunn
2021-01-02  1:49                       ` Pali Rohár
2021-01-03  2:25                         ` Thomas Schreiber
2021-01-03  2:41                           ` Pali Rohár
2021-01-06 14:55                             ` Pali Rohár
2021-01-06 15:21                               ` Russell King - ARM Linux admin
2021-01-06 15:23                                 ` Russell King - ARM Linux admin
2021-01-06 15:27                                   ` Russell King - ARM Linux admin
2021-01-06 15:35                                     ` Pali Rohár
2020-12-30 19:30             ` Andrew Lunn
2020-12-30 19:39               ` Russell King - ARM Linux admin
2020-12-30 17:31         ` Pali Rohár
2020-12-30 19:18           ` Russell King - ARM Linux admin
2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
2020-12-30 16:11   ` Russell King - ARM Linux admin
2020-12-30 17:06     ` Pali Rohár
2020-12-30 17:27       ` Marek Behún
2020-12-30 19:12         ` Russell King - ARM Linux admin
2020-12-31 13:52           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2020-12-30 16:13   ` Russell King - ARM Linux admin
2020-12-30 16:57     ` Pali Rohár
2020-12-30 17:06       ` Russell King - ARM Linux admin
2020-12-30 17:17         ` Andrew Lunn
2020-12-30 17:32           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-07  2:02     ` Andrew Lunn
2021-01-07  9:08       ` Pali Rohár
2021-01-07 17:19     ` Andrew Lunn
2021-01-07 17:40       ` Russell King - ARM Linux admin
2021-01-07 19:18         ` Pali Rohár
2021-01-07 19:17       ` Pali Rohár
2021-01-07 19:45       ` Russell King - ARM Linux admin
2021-01-07 20:21         ` Marek Behún
2021-01-08  0:49           ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2021-01-07 16:54     ` Andrew Lunn
2021-01-09 15:46       ` Russell King - ARM Linux admin
2021-01-09 15:54         ` Andrew Lunn
2021-01-09 16:27           ` Russell King - ARM Linux admin
2021-01-09 19:14         ` Pali Rohár
2021-01-09 23:19           ` Russell King - ARM Linux admin
2021-01-09 23:50             ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-07 16:51     ` Andrew Lunn
2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-11 15:28     ` Marek Behún
2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-11 15:32     ` Marek Behún
2021-01-12 13:33   ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-18  9:34   ` Pali Rohár
2021-01-25 14:09     ` Pali Rohár
2021-01-25 14:16       ` Russell King - ARM Linux admin
2021-01-25 14:23         ` Pali Rohár
2021-01-25 14:42           ` Russell King - ARM Linux admin
2021-01-25 14:47             ` Pali Rohár
2021-01-25 15:41               ` Andrew Lunn
2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-25 15:02   ` [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-28 21:50   ` [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201231154030.tbofxtq4ravct4sg@pali \
    --to=pali@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hkallweit1@gmail.com \
    --cc=kabel@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).