| Message ID | 20171108034911.16382-3-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: sfp: Fixes and debugging improvements | expand |
On Tue, Nov 07, 2017 at 07:49:09PM -0800, Florian Fainelli wrote: > The extended ID options 16-bit value is big-endian (and actually annotated as > such), but we would be accessing it with our CPU endian, which would not > allow the correct detection of whether the LOS signal is inverted or not. > > Fixes: 73970055450e ("sfp: add SFP module support") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/phy/sfp.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 942288aa9cdb..dfb28b269687 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -355,7 +355,7 @@ static void sfp_sm_link_check_los(struct sfp *sfp) > * SFP_OPTIONS_LOS_NORMAL are set? For now, we assume > * the same as SFP_OPTIONS_LOS_NORMAL set. > */ > - if (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED) > + if (be16_to_cpu(sfp->id.ext.options) & SFP_OPTIONS_LOS_INVERTED) It would be more efficient to convert the constants to BE16 rather than an indeterminant number to CPU endian. The compiler can optimise the constant. Same for the other two hunks.
On 11/08/2017 12:56 AM, Russell King - ARM Linux wrote: > On Tue, Nov 07, 2017 at 07:49:09PM -0800, Florian Fainelli wrote: >> The extended ID options 16-bit value is big-endian (and actually annotated as >> such), but we would be accessing it with our CPU endian, which would not >> allow the correct detection of whether the LOS signal is inverted or not. >> >> Fixes: 73970055450e ("sfp: add SFP module support") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/net/phy/sfp.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c >> index 942288aa9cdb..dfb28b269687 100644 >> --- a/drivers/net/phy/sfp.c >> +++ b/drivers/net/phy/sfp.c >> @@ -355,7 +355,7 @@ static void sfp_sm_link_check_los(struct sfp *sfp) >> * SFP_OPTIONS_LOS_NORMAL are set? For now, we assume >> * the same as SFP_OPTIONS_LOS_NORMAL set. >> */ >> - if (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED) >> + if (be16_to_cpu(sfp->id.ext.options) & SFP_OPTIONS_LOS_INVERTED) > > It would be more efficient to convert the constants to BE16 rather > than an indeterminant number to CPU endian. The compiler can optimise > the constant. Same for the other two hunks. Sure, I can do that.
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 942288aa9cdb..dfb28b269687 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -355,7 +355,7 @@ static void sfp_sm_link_check_los(struct sfp *sfp) * SFP_OPTIONS_LOS_NORMAL are set? For now, we assume * the same as SFP_OPTIONS_LOS_NORMAL set. */ - if (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED) + if (be16_to_cpu(sfp->id.ext.options) & SFP_OPTIONS_LOS_INVERTED) los ^= SFP_F_LOS; if (los) @@ -583,7 +583,8 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event) if (event == SFP_E_TX_FAULT) sfp_sm_fault(sfp, true); else if (event == - (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED ? + (be16_to_cpu(sfp->id.ext.options) & + SFP_OPTIONS_LOS_INVERTED ? SFP_E_LOS_HIGH : SFP_E_LOS_LOW)) sfp_sm_link_up(sfp); break; @@ -593,7 +594,8 @@ static void sfp_sm_event(struct sfp *sfp, unsigned int event) sfp_sm_link_down(sfp); sfp_sm_fault(sfp, true); } else if (event == - (sfp->id.ext.options & SFP_OPTIONS_LOS_INVERTED ? + (be16_to_cpu(sfp->id.ext.options) & + SFP_OPTIONS_LOS_INVERTED ? SFP_E_LOS_LOW : SFP_E_LOS_HIGH)) { sfp_sm_link_down(sfp); sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
The extended ID options 16-bit value is big-endian (and actually annotated as such), but we would be accessing it with our CPU endian, which would not allow the correct detection of whether the LOS signal is inverted or not. Fixes: 73970055450e ("sfp: add SFP module support") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/sfp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)