Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
> Since 719655a14971 ("net: phy: Replace phy driver features u32
> with link_mode bitmap"), phy_probe() calls
> ethtool_convert_link_mode_to_legacy_u32() with phydrv->features
> as argument. Since features are NULL, we will get NULL pointer
> dereference.
Hi Jose
Thanks for pointing that out. I will fix it ASAP.
Andrew
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On 23-10-2018 11:58, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote:
>> On 23-10-2018 11:20, Russell King - ARM Linux wrote:
>>> I have no idea what you're proposing there - your patches weren't copied
>>> to me.
>> They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that
>> without this remote end doesn't detect link is down ...
>>
>> If it's okay for Generic 10G driver I can submit only this and
>> manually reset PHY in stmmac driver so that I don't need to
>> implement custom PHY driver ...
>
>
>> BTW, I just found out currently Generic 10G Driver is broken
>> without patch 4/4 of this series [1]
>>
>> [1]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_987570_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zYEDSMZPTCHiPc_3B8buyu0kXnlIEYawnHWAxPrsoSU&s=MlF6I2cBSYkGxgEwNV-hXpXJIvXv_gRYXP-CazjkUSw&e=
> How is it broken - what are the symptoms?
>
> The generic 10G driver is bound not via the normal bus matching and
> phy_bus_match(), but via a manual bind in phy_attach_direct(). This
> calls the probe function, which is phy_probe(), which initialises
> the supported/advertising to the driver's features (which as you note
> are zero.)
Since 719655a14971 ("net: phy: Replace phy driver features u32
with link_mode bitmap"), phy_probe() calls
ethtool_convert_link_mode_to_legacy_u32() with phydrv->features
as argument. Since features are NULL, we will get NULL pointer
dereference.
I guess Generic 10G driver was forgotten in the conversion.
Thanks and Best Regards,
Jose Miguel Abreu
>
> However, phy_attach_direct() goes on to call phy_init_hw(), which
> calls the config_init() method. The config_init() method initialises
> the supported/advertising masks to 10GbaseT. This is (partly) what
> I refer to when I say that the generic 10G support is crippled - it
> only supports this single speed and media.
>
> So the supported/advertising masks should be forced to only 10GbaseT
> at the completion of phy_attach_direct().
>
> The "generic 10G" support doesn't do autonegotiation, configuration
> or link mode forcing. It only assumes 10GbaseT is supported, and
> only checks for the "link up" bits.
>
> It isn't like the non-10G generic PHY support due to history - it
> was added in 2014 by Andy Fleming (see 124059fd53af).
>
> BTW, your patch 1 is wrong as well (introducing phy_update_link()).
> You don't take account that a 10G phy may have alternative ways of
> reading the link (like 88x3310 does, because it has multiple
> instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_*
> functions are legacy functions for the crippled "generic" 10G
> support.
>
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
> If it's okay for Generic 10G driver I can submit only this and > manually reset PHY in stmmac driver so that I don't need to > implement custom PHY driver ... Hi Jose That is a bad idea. What happens when somebody uses a different PHY which uses a different reset sequence? Please keep with the abstraction. Anything touching the PHY needs to be in a PHY driver, or phylib. Andrew
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote: > On 23-10-2018 11:20, Russell King - ARM Linux wrote: > > I have no idea what you're proposing there - your patches weren't copied > > to me. > > They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that > without this remote end doesn't detect link is down ... > > If it's okay for Generic 10G driver I can submit only this and > manually reset PHY in stmmac driver so that I don't need to > implement custom PHY driver ... > BTW, I just found out currently Generic 10G Driver is broken > without patch 4/4 of this series [1] > > [1] https://patchwork.ozlabs.org/patch/987570/ How is it broken - what are the symptoms? The generic 10G driver is bound not via the normal bus matching and phy_bus_match(), but via a manual bind in phy_attach_direct(). This calls the probe function, which is phy_probe(), which initialises the supported/advertising to the driver's features (which as you note are zero.) However, phy_attach_direct() goes on to call phy_init_hw(), which calls the config_init() method. The config_init() method initialises the supported/advertising masks to 10GbaseT. This is (partly) what I refer to when I say that the generic 10G support is crippled - it only supports this single speed and media. So the supported/advertising masks should be forced to only 10GbaseT at the completion of phy_attach_direct(). The "generic 10G" support doesn't do autonegotiation, configuration or link mode forcing. It only assumes 10GbaseT is supported, and only checks for the "link up" bits. It isn't like the non-10G generic PHY support due to history - it was added in 2014 by Andy Fleming (see 124059fd53af). BTW, your patch 1 is wrong as well (introducing phy_update_link()). You don't take account that a 10G phy may have alternative ways of reading the link (like 88x3310 does, because it has multiple instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_* functions are legacy functions for the crippled "generic" 10G support. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On 23-10-2018 11:20, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote:
>> On 22-10-2018 18:13, Florian Fainelli wrote:
>>> On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> Hello,
>
> On 22-10-2018 13:28, Andrew Lunn wrote:
>>> EXPORT_SYMBOL_GPL(gen10g_resume);
>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>>> .phy_id = 0x,
>>> .phy_id_mask= 0x,
>>> .name = "Generic 10G PHY",
>>> - .soft_reset = gen10g_no_soft_reset,
>>> + .soft_reset = gen10g_soft_reset,
>>> .config_init= gen10g_config_init,
>>> .features = 0,
>>> .aneg_done = genphy_c45_aneg_done,
>> Hi Jose
>>
>> You need to be careful here. There is a reason this is called
>> gen10g_no_soft_reset, rather than having an empty
>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
>> gen10g_soft_reset is fine, but don't change this here, without first
>> understanding the history, and talking to Russell King.
> Hmm, the reset function only interacts with standard PCS
> registers, which should always be available ...
>
> >From my tests I need to do at least 1 reset during power-up so in
> ultimate case I can add a feature quirk or similar.
>
> Russell, can you please comment ?
Setting the reset bit on 88x3310 causes the entire device to become
completely inaccessible until hardware reset. Therefore, this bit
must _never_ be set for these devices. That said, we have a separate
driver for these PHYs, but that will only be used for them if it's
present in the kernel. If we accidentally fall back to the generic
driver, then we'll screw the 88x3310 until a full hardware reset.
We also have a bunch of net devices that make use of this crippled
"generic" 10G support - we don't know whether resetting the PHY
for those systems will cause a regression - maybe board firmware
already configured the PHY? I can't say either way on that, except
that we've had crippled 10G support in PHYLIB for a number of years
now _with_ users, and adding reset support drastically changes the
subsystem's behaviour for these users.
I would recommend not touching the generic 10G driver, but instead
implement your own driver for your PHY to avoid causing regressions.
>>> Agreed.
>> What about .suspend / .resume ?
> I have no idea what you're proposing there - your patches weren't copied
> to me.
>
They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that
without this remote end doesn't detect link is down ...
If it's okay for Generic 10G driver I can submit only this and
manually reset PHY in stmmac driver so that I don't need to
implement custom PHY driver ...
BTW, I just found out currently Generic 10G Driver is broken
without patch 4/4 of this series [1]
[1] https://patchwork.ozlabs.org/patch/987570/
Thanks and Best Regards,
Jose Miguel Abreu
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote:
> On 22-10-2018 18:13, Florian Fainelli wrote:
> > On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
> >> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> >>> Hello,
> >>>
> >>> On 22-10-2018 13:28, Andrew Lunn wrote:
> > EXPORT_SYMBOL_GPL(gen10g_resume);
> > @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> > .phy_id = 0x,
> > .phy_id_mask= 0x,
> > .name = "Generic 10G PHY",
> > - .soft_reset = gen10g_no_soft_reset,
> > + .soft_reset = gen10g_soft_reset,
> > .config_init= gen10g_config_init,
> > .features = 0,
> > .aneg_done = genphy_c45_aneg_done,
> Hi Jose
>
> You need to be careful here. There is a reason this is called
> gen10g_no_soft_reset, rather than having an empty
> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
> gen10g_soft_reset is fine, but don't change this here, without first
> understanding the history, and talking to Russell King.
> >>> Hmm, the reset function only interacts with standard PCS
> >>> registers, which should always be available ...
> >>>
> >>> >From my tests I need to do at least 1 reset during power-up so in
> >>> ultimate case I can add a feature quirk or similar.
> >>>
> >>> Russell, can you please comment ?
> >> Setting the reset bit on 88x3310 causes the entire device to become
> >> completely inaccessible until hardware reset. Therefore, this bit
> >> must _never_ be set for these devices. That said, we have a separate
> >> driver for these PHYs, but that will only be used for them if it's
> >> present in the kernel. If we accidentally fall back to the generic
> >> driver, then we'll screw the 88x3310 until a full hardware reset.
> >>
> >> We also have a bunch of net devices that make use of this crippled
> >> "generic" 10G support - we don't know whether resetting the PHY
> >> for those systems will cause a regression - maybe board firmware
> >> already configured the PHY? I can't say either way on that, except
> >> that we've had crippled 10G support in PHYLIB for a number of years
> >> now _with_ users, and adding reset support drastically changes the
> >> subsystem's behaviour for these users.
> >>
> >> I would recommend not touching the generic 10G driver, but instead
> >> implement your own driver for your PHY to avoid causing regressions.
> >>
> > Agreed.
>
> What about .suspend / .resume ?
I have no idea what you're proposing there - your patches weren't copied
to me.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On 22-10-2018 18:13, Florian Fainelli wrote:
> On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
>> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
>>> Hello,
>>>
>>> On 22-10-2018 13:28, Andrew Lunn wrote:
> EXPORT_SYMBOL_GPL(gen10g_resume);
> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> .phy_id = 0x,
> .phy_id_mask= 0x,
> .name = "Generic 10G PHY",
> - .soft_reset = gen10g_no_soft_reset,
> + .soft_reset = gen10g_soft_reset,
> .config_init= gen10g_config_init,
> .features = 0,
> .aneg_done = genphy_c45_aneg_done,
Hi Jose
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an empty
gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
gen10g_soft_reset is fine, but don't change this here, without first
understanding the history, and talking to Russell King.
>>> Hmm, the reset function only interacts with standard PCS
>>> registers, which should always be available ...
>>>
>>> >From my tests I need to do at least 1 reset during power-up so in
>>> ultimate case I can add a feature quirk or similar.
>>>
>>> Russell, can you please comment ?
>> Setting the reset bit on 88x3310 causes the entire device to become
>> completely inaccessible until hardware reset. Therefore, this bit
>> must _never_ be set for these devices. That said, we have a separate
>> driver for these PHYs, but that will only be used for them if it's
>> present in the kernel. If we accidentally fall back to the generic
>> driver, then we'll screw the 88x3310 until a full hardware reset.
>>
>> We also have a bunch of net devices that make use of this crippled
>> "generic" 10G support - we don't know whether resetting the PHY
>> for those systems will cause a regression - maybe board firmware
>> already configured the PHY? I can't say either way on that, except
>> that we've had crippled 10G support in PHYLIB for a number of years
>> now _with_ users, and adding reset support drastically changes the
>> subsystem's behaviour for these users.
>>
>> I would recommend not touching the generic 10G driver, but instead
>> implement your own driver for your PHY to avoid causing regressions.
>>
> Agreed.
What about .suspend / .resume ?
Thanks and Best Regards,
Jose Miguel Abreu
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
>> Hello,
>>
>> On 22-10-2018 13:28, Andrew Lunn wrote:
EXPORT_SYMBOL_GPL(gen10g_resume);
@@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
.phy_id = 0x,
.phy_id_mask= 0x,
.name = "Generic 10G PHY",
- .soft_reset = gen10g_no_soft_reset,
+ .soft_reset = gen10g_soft_reset,
.config_init= gen10g_config_init,
.features = 0,
.aneg_done = genphy_c45_aneg_done,
>>> Hi Jose
>>>
>>> You need to be careful here. There is a reason this is called
>>> gen10g_no_soft_reset, rather than having an empty
>>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
>>> gen10g_soft_reset is fine, but don't change this here, without first
>>> understanding the history, and talking to Russell King.
>>
>> Hmm, the reset function only interacts with standard PCS
>> registers, which should always be available ...
>>
>> >From my tests I need to do at least 1 reset during power-up so in
>> ultimate case I can add a feature quirk or similar.
>>
>> Russell, can you please comment ?
>
> Setting the reset bit on 88x3310 causes the entire device to become
> completely inaccessible until hardware reset. Therefore, this bit
> must _never_ be set for these devices. That said, we have a separate
> driver for these PHYs, but that will only be used for them if it's
> present in the kernel. If we accidentally fall back to the generic
> driver, then we'll screw the 88x3310 until a full hardware reset.
>
> We also have a bunch of net devices that make use of this crippled
> "generic" 10G support - we don't know whether resetting the PHY
> for those systems will cause a regression - maybe board firmware
> already configured the PHY? I can't say either way on that, except
> that we've had crippled 10G support in PHYLIB for a number of years
> now _with_ users, and adding reset support drastically changes the
> subsystem's behaviour for these users.
>
> I would recommend not touching the generic 10G driver, but instead
> implement your own driver for your PHY to avoid causing regressions.
>
Agreed.
--
Florian
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> Hello,
>
> On 22-10-2018 13:28, Andrew Lunn wrote:
> >> EXPORT_SYMBOL_GPL(gen10g_resume);
> >> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> >>.phy_id = 0x,
> >>.phy_id_mask= 0x,
> >>.name = "Generic 10G PHY",
> >> - .soft_reset = gen10g_no_soft_reset,
> >> + .soft_reset = gen10g_soft_reset,
> >>.config_init= gen10g_config_init,
> >>.features = 0,
> >>.aneg_done = genphy_c45_aneg_done,
> > Hi Jose
> >
> > You need to be careful here. There is a reason this is called
> > gen10g_no_soft_reset, rather than having an empty
> > gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
> > gen10g_soft_reset is fine, but don't change this here, without first
> > understanding the history, and talking to Russell King.
>
> Hmm, the reset function only interacts with standard PCS
> registers, which should always be available ...
>
> >From my tests I need to do at least 1 reset during power-up so in
> ultimate case I can add a feature quirk or similar.
>
> Russell, can you please comment ?
Setting the reset bit on 88x3310 causes the entire device to become
completely inaccessible until hardware reset. Therefore, this bit
must _never_ be set for these devices. That said, we have a separate
driver for these PHYs, but that will only be used for them if it's
present in the kernel. If we accidentally fall back to the generic
driver, then we'll screw the 88x3310 until a full hardware reset.
We also have a bunch of net devices that make use of this crippled
"generic" 10G support - we don't know whether resetting the PHY
for those systems will cause a regression - maybe board firmware
already configured the PHY? I can't say either way on that, except
that we've had crippled 10G support in PHYLIB for a number of years
now _with_ users, and adding reset support drastically changes the
subsystem's behaviour for these users.
I would recommend not touching the generic 10G driver, but instead
implement your own driver for your PHY to avoid causing regressions.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Hello,
On 22-10-2018 13:28, Andrew Lunn wrote:
>> EXPORT_SYMBOL_GPL(gen10g_resume);
>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>> .phy_id = 0x,
>> .phy_id_mask= 0x,
>> .name = "Generic 10G PHY",
>> -.soft_reset = gen10g_no_soft_reset,
>> +.soft_reset = gen10g_soft_reset,
>> .config_init= gen10g_config_init,
>> .features = 0,
>> .aneg_done = genphy_c45_aneg_done,
> Hi Jose
>
> You need to be careful here. There is a reason this is called
> gen10g_no_soft_reset, rather than having an empty
> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
> gen10g_soft_reset is fine, but don't change this here, without first
> understanding the history, and talking to Russell King.
Hmm, the reset function only interacts with standard PCS
registers, which should always be available ...
>From my tests I need to do at least 1 reset during power-up so in
ultimate case I can add a feature quirk or similar.
Russell, can you please comment ?
Thanks and Best Regards,
Jose Miguel Abreu
>
> Andrew
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
> EXPORT_SYMBOL_GPL(gen10g_resume);
> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> .phy_id = 0x,
> .phy_id_mask= 0x,
> .name = "Generic 10G PHY",
> - .soft_reset = gen10g_no_soft_reset,
> + .soft_reset = gen10g_soft_reset,
> .config_init= gen10g_config_init,
> .features = 0,
> .aneg_done = genphy_c45_aneg_done,
Hi Jose
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an empty
gen10g_soft_reset. Some PHYs break when you do a reset. So adding a
gen10g_soft_reset is fine, but don't change this here, without first
understanding the history, and talking to Russell King.
Andrew
[PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Implement the missing callbacks for Generic 10G PHY. Tested using XGMAC
with a C45 PHY working at 10G Link.
Signed-off-by: Jose Abreu
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: "David S. Miller"
Cc: Joao Pinto
---
drivers/net/phy/phy-c45.c | 56 ++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index c0135217b81f..7e62bd7795a3 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1,6 +1,7 @@
/*
* Clause 45 PHY support
*/
+#include
#include
#include
#include
@@ -294,6 +295,35 @@ int gen10g_read_status(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(gen10g_read_status);
+static int gen10g_poll_reset(struct phy_device *phydev)
+{
+ /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
+ unsigned int retries = 12;
+ int ret;
+
+ do {
+ msleep(50);
+ ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+ if (ret < 0)
+ return ret;
+ } while (ret & MDIO_CTRL1_RESET && --retries);
+ if (ret & MDIO_CTRL1_RESET)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static int gen10g_soft_reset(struct phy_device *phydev)
+{
+ int val;
+
+ val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, MDIO_CTRL1_RESET);
+ if (val < 0)
+ return val;
+
+ return gen10g_poll_reset(phydev);
+}
+
int gen10g_no_soft_reset(struct phy_device *phydev)
{
/* Do nothing for now */
@@ -313,12 +343,36 @@ EXPORT_SYMBOL_GPL(gen10g_config_init);
int gen10g_suspend(struct phy_device *phydev)
{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+ if (val < 0)
+ return val;
+
+ val |= MDIO_CTRL1_LPOWER;
+
+ val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val);
+ if (val < 0)
+ return val;
+
return 0;
}
EXPORT_SYMBOL_GPL(gen10g_suspend);
int gen10g_resume(struct phy_device *phydev)
{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+ if (val < 0)
+ return val;
+
+ val &= ~MDIO_CTRL1_LPOWER;
+
+ val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val);
+ if (val < 0)
+ return val;
+
return 0;
}
EXPORT_SYMBOL_GPL(gen10g_resume);
@@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
.phy_id = 0x,
.phy_id_mask= 0x,
.name = "Generic 10G PHY",
- .soft_reset = gen10g_no_soft_reset,
+ .soft_reset = gen10g_soft_reset,
.config_init= gen10g_config_init,
.features = 0,
.aneg_done = genphy_c45_aneg_done,
--
2.7.4
