| Message ID | 20180504135643.23466-4-antoine.tenart@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/2018 06:56 AM, Antoine Tenart wrote: > In case no Tx disable pin is available the SFP modules will always be > emitting. This could be an issue when using modules using laser as their > light source as we would have no way to disable it when the fiber is > removed. This patch adds a warning when registering an SFP cage which do > not have its tx_disable pin wired or available. Is this something that was done in a possibly earlier revision of a given board design and which was finally fixed? Nothing wrong with the patch, but this seems like a pretty serious board design mistake, that needs to be addressed.
On Fri, May 04, 2018 at 10:07:53AM -0700, Florian Fainelli wrote: > On 05/04/2018 06:56 AM, Antoine Tenart wrote: > > In case no Tx disable pin is available the SFP modules will always be > > emitting. This could be an issue when using modules using laser as their > > light source as we would have no way to disable it when the fiber is > > removed. This patch adds a warning when registering an SFP cage which do > > not have its tx_disable pin wired or available. > > Is this something that was done in a possibly earlier revision of a > given board design and which was finally fixed? Nothing wrong with the > patch, but this seems like a pretty serious board design mistake, that > needs to be addressed. Hi Florian Zii Devel B is like this. Only the "Signal Detect" pin is wired to a GPIO. Andrew
On May 4, 2018 10:14:25 AM PDT, Andrew Lunn <andrew@lunn.ch> wrote: >On Fri, May 04, 2018 at 10:07:53AM -0700, Florian Fainelli wrote: >> On 05/04/2018 06:56 AM, Antoine Tenart wrote: >> > In case no Tx disable pin is available the SFP modules will always >be >> > emitting. This could be an issue when using modules using laser as >their >> > light source as we would have no way to disable it when the fiber >is >> > removed. This patch adds a warning when registering an SFP cage >which do >> > not have its tx_disable pin wired or available. >> >> Is this something that was done in a possibly earlier revision of a >> given board design and which was finally fixed? Nothing wrong with >the >> patch, but this seems like a pretty serious board design mistake, >that >> needs to be addressed. > >Hi Florian > >Zii Devel B is like this. Only the "Signal Detect" pin is wired to a >GPIO. Good point, indeed. BTW what do you think about exposing the SFF's EEPROM and diagnostics through the standard ethtool operations even if we have to keep the description of the SFF as a fixed link in Device Tree because of the unfortunate wiring?
On Sat, May 05, 2018 at 01:38:31PM -0700, Florian Fainelli wrote: > On May 4, 2018 10:14:25 AM PDT, Andrew Lunn <andrew@lunn.ch> wrote: > >On Fri, May 04, 2018 at 10:07:53AM -0700, Florian Fainelli wrote: > >> On 05/04/2018 06:56 AM, Antoine Tenart wrote: > >> > In case no Tx disable pin is available the SFP modules will always > >be > >> > emitting. This could be an issue when using modules using laser as > >their > >> > light source as we would have no way to disable it when the fiber > >is > >> > removed. This patch adds a warning when registering an SFP cage > >which do > >> > not have its tx_disable pin wired or available. > >> > >> Is this something that was done in a possibly earlier revision of a > >> given board design and which was finally fixed? Nothing wrong with > >the > >> patch, but this seems like a pretty serious board design mistake, > >that > >> needs to be addressed. > > > >Hi Florian > > > >Zii Devel B is like this. Only the "Signal Detect" pin is wired to a > >GPIO. > > Good point, indeed. BTW what do you think about exposing the SFF's > EEPROM and diagnostics through the standard ethtool operations even > if we have to keep the description of the SFF as a fixed link in > Device Tree because of the unfortunate wiring? I believe in Antoine case, all the control plane is broken. He cannot read the EEPROM, nor any of the modules pins via GPIOs. For Zii Devel B, the EEPROM is accessible, and so is the SD pin. What is missing is transmit disable. So i would expose it as an SFF module. Andrew
On Fri, May 04, 2018 at 03:56:33PM +0200, Antoine Tenart wrote: > In case no Tx disable pin is available the SFP modules will always be > emitting. This could be an issue when using modules using laser as their > light source as we would have no way to disable it when the fiber is > removed. This patch adds a warning when registering an SFP cage which do > not have its tx_disable pin wired or available. > > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> Looks fine, thanks. Acked-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/sfp.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 8e323a4b70da..d4f503b2e3e2 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -1093,6 +1093,15 @@ static int sfp_probe(struct platform_device *pdev) > if (!sfp->gpio[GPIO_MODDEF0] && !sfp->gpio[GPIO_LOS]) > sfp->sm_dev_state = SFP_DEV_UNKNOWN; > > + /* We could have an issue in cases no Tx disable pin is available or > + * wired as modules using a laser as their light source will continue to > + * be active when the fiber is removed. This could be a safety issue and > + * we should at least warn the user about that. > + */ > + if (!sfp->gpio[GPIO_TX_DISABLE]) > + dev_warn(sfp->dev, > + "No tx_disable pin: SFP modules will always be emitting.\n"); > + > return 0; > } > > -- > 2.17.0 >
On Sat, May 05, 2018 at 10:52:42PM +0200, Andrew Lunn wrote: > On Sat, May 05, 2018 at 01:38:31PM -0700, Florian Fainelli wrote: > > On May 4, 2018 10:14:25 AM PDT, Andrew Lunn <andrew@lunn.ch> wrote: > > >On Fri, May 04, 2018 at 10:07:53AM -0700, Florian Fainelli wrote: > > >> On 05/04/2018 06:56 AM, Antoine Tenart wrote: > > >> > In case no Tx disable pin is available the SFP modules will always > > >be > > >> > emitting. This could be an issue when using modules using laser as > > >their > > >> > light source as we would have no way to disable it when the fiber > > >is > > >> > removed. This patch adds a warning when registering an SFP cage > > >which do > > >> > not have its tx_disable pin wired or available. > > >> > > >> Is this something that was done in a possibly earlier revision of a > > >> given board design and which was finally fixed? Nothing wrong with > > >the > > >> patch, but this seems like a pretty serious board design mistake, > > >that > > >> needs to be addressed. > > > > > >Hi Florian > > > > > >Zii Devel B is like this. Only the "Signal Detect" pin is wired to a > > >GPIO. > > > > > Good point, indeed. BTW what do you think about exposing the SFF's > > EEPROM and diagnostics through the standard ethtool operations even > > if we have to keep the description of the SFF as a fixed link in > > Device Tree because of the unfortunate wiring? > > I believe in Antoine case, all the control plane is broken. He cannot > read the EEPROM, nor any of the modules pins via GPIOs. Correct. > For Zii Devel B, the EEPROM is accessible, and so is the SD pin. What > is missing is transmit disable. So i would expose it as an SFF module. Agreed.
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 8e323a4b70da..d4f503b2e3e2 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1093,6 +1093,15 @@ static int sfp_probe(struct platform_device *pdev) if (!sfp->gpio[GPIO_MODDEF0] && !sfp->gpio[GPIO_LOS]) sfp->sm_dev_state = SFP_DEV_UNKNOWN; + /* We could have an issue in cases no Tx disable pin is available or + * wired as modules using a laser as their light source will continue to + * be active when the fiber is removed. This could be a safety issue and + * we should at least warn the user about that. + */ + if (!sfp->gpio[GPIO_TX_DISABLE]) + dev_warn(sfp->dev, + "No tx_disable pin: SFP modules will always be emitting.\n"); + return 0; }
In case no Tx disable pin is available the SFP modules will always be emitting. This could be an issue when using modules using laser as their light source as we would have no way to disable it when the fiber is removed. This patch adds a warning when registering an SFP cage which do not have its tx_disable pin wired or available. Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> --- drivers/net/phy/sfp.c | 9 +++++++++ 1 file changed, 9 insertions(+)