| Message ID | c0ac2033-3384-8605-2050-25af4386af5b@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: improve starting PHY | expand |
On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote: > The state machine is a no-op before phy_start() has been called. > Therefore let's enable it in phy_start() only. In phy_start() > let's call phy_start_machine() instead of phy_trigger_machine(). > phy_start_machine is an alias for phy_trigger_machine but it makes > clearer that we start the state machine here instead of just > triggering a run. Hi Heiner Documentation/networking/phy.txt has a section "Doing it all yourself" It would be good to review that, and make sure that documentation is still valid. I'm not sure any MAC driver actually does do it all itself. So it might be worth reviewing the whole document and making updates to remove parts of the text. Andrew
On 21.01.2019 17:35, Andrew Lunn wrote: > On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote: >> The state machine is a no-op before phy_start() has been called. >> Therefore let's enable it in phy_start() only. In phy_start() >> let's call phy_start_machine() instead of phy_trigger_machine(). >> phy_start_machine is an alias for phy_trigger_machine but it makes >> clearer that we start the state machine here instead of just >> triggering a run. > > Hi Heiner > > Documentation/networking/phy.txt has a section "Doing it all yourself" > It would be good to review that, and make sure that documentation is > still valid. I'm not sure any MAC driver actually does do it all > itself. So it might be worth reviewing the whole document and making > updates to remove parts of the text. > Right. I figured out that I have update phy.txt anyway because I recently removed phy_stop_interrupts which is referenced in the documentation. OK if we leave the patch series as is and I submit the documentation update as a separate patch? > Andrew > > Heiner
> Right. I figured out that I have update phy.txt anyway because I > recently removed phy_stop_interrupts which is referenced in the > documentation. OK if we leave the patch series as is and I submit > the documentation update as a separate patch? Hi Heiner Fixing the documentation separately is O.K. It might also be a good time to convert it to rst. Andrew
On 21.01.2019 19:42, Andrew Lunn wrote: >> Right. I figured out that I have update phy.txt anyway because I >> recently removed phy_stop_interrupts which is referenced in the >> documentation. OK if we leave the patch series as is and I submit >> the documentation update as a separate patch? > > Hi Heiner > > Fixing the documentation separately is O.K. It might also be a good > time to convert it to rst. > Thanks for the hint regarding rst. I converted the document and at least restview is (syntactically) happy with it. Are you aware of any checker for kernel rst documentation? The link to the 802.3 standard is dead. On the official IEEE website the standards are available for significant money only. Not sure whether any other link to this standard is legal with regard to copyright. > Andrew > Heiner
On Mon, Jan 21, 2019 at 10:52:48PM +0100, Heiner Kallweit wrote: > On 21.01.2019 19:42, Andrew Lunn wrote: > >> Right. I figured out that I have update phy.txt anyway because I > >> recently removed phy_stop_interrupts which is referenced in the > >> documentation. OK if we leave the patch series as is and I submit > >> the documentation update as a separate patch? > > > > Hi Heiner > > > > Fixing the documentation separately is O.K. It might also be a good > > time to convert it to rst. > > > Thanks for the hint regarding rst. I converted the document and at > least restview is (syntactically) happy with it. Are you aware of > any checker for kernel rst documentation? Hi Heiner Do you try make htmldocs? It might need linking into the documentation tree for that to work. > The link to the 802.3 standard is dead. On the official IEEE website > the standards are available for significant money only. Not sure > whether any other link to this standard is legal with regard to > copyright. I think it should still be available as part of IEEE GET Program. You probably cannot give a direct link to it, but maybe a link to the program could be given? Andrew
On 1/21/19 12:36 PM, Heiner Kallweit wrote: > On 21.01.2019 17:35, Andrew Lunn wrote: >> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote: >>> The state machine is a no-op before phy_start() has been called. >>> Therefore let's enable it in phy_start() only. In phy_start() >>> let's call phy_start_machine() instead of phy_trigger_machine(). >>> phy_start_machine is an alias for phy_trigger_machine but it makes >>> clearer that we start the state machine here instead of just >>> triggering a run. >> >> Hi Heiner >> >> Documentation/networking/phy.txt has a section "Doing it all yourself" >> It would be good to review that, and make sure that documentation is >> still valid. I'm not sure any MAC driver actually does do it all >> itself. So it might be worth reviewing the whole document and making >> updates to remove parts of the text. >> > Right. I figured out that I have update phy.txt anyway because I > recently removed phy_stop_interrupts which is referenced in the > documentation. OK if we leave the patch series as is and I submit > the documentation update as a separate patch? I think you need to be careful here and not break what is allowed in the "Doing it all yourself" section. The amd-xgbe driver makes use of this functionality and does not use phy_start()/phy_stop(). Specifically, it does: get_phy_device(); phy_device_register(); phy_attach_direct(); At which point it uses phy_start_aneg(), phy_read(), phy_write(), phy_read_status() and phy_aneg_done(). I'm not sure what other drivers out there that make use of this support within phylib. Btw, I did notice this revert that was applied that eliminated a warning that I started seeing in 5.0, so that is good: d9f903f6af3d ("net: phy: fix too strict check in phy_start_aneg") Thanks, Tom > >> Andrew >> >> > Heiner >
On 22.01.2019 15:46, Lendacky, Thomas wrote: > On 1/21/19 12:36 PM, Heiner Kallweit wrote: >> On 21.01.2019 17:35, Andrew Lunn wrote: >>> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote: >>>> The state machine is a no-op before phy_start() has been called. >>>> Therefore let's enable it in phy_start() only. In phy_start() >>>> let's call phy_start_machine() instead of phy_trigger_machine(). >>>> phy_start_machine is an alias for phy_trigger_machine but it makes >>>> clearer that we start the state machine here instead of just >>>> triggering a run. >>> >>> Hi Heiner >>> >>> Documentation/networking/phy.txt has a section "Doing it all yourself" >>> It would be good to review that, and make sure that documentation is >>> still valid. I'm not sure any MAC driver actually does do it all >>> itself. So it might be worth reviewing the whole document and making >>> updates to remove parts of the text. >>> >> Right. I figured out that I have update phy.txt anyway because I >> recently removed phy_stop_interrupts which is referenced in the >> documentation. OK if we leave the patch series as is and I submit >> the documentation update as a separate patch? > > I think you need to be careful here and not break what is allowed in the > "Doing it all yourself" section. The amd-xgbe driver makes use of this > functionality and does not use phy_start()/phy_stop(). Specifically, it > does: > get_phy_device(); > phy_device_register(); > phy_attach_direct(); > > At which point it uses phy_start_aneg(), phy_read(), phy_write(), > phy_read_status() and phy_aneg_done(). > Thanks for the hint, Tom. I *think* the changes should be safe. However, if AMD has a regression test suite I'd appreciate if you could test the changes upfront or once they reach net-next. > I'm not sure what other drivers out there that make use of this support > within phylib. > > Btw, I did notice this revert that was applied that eliminated a warning > that I started seeing in 5.0, so that is good: > d9f903f6af3d ("net: phy: fix too strict check in phy_start_aneg") > > Thanks, > Tom > >> >>> Andrew >>> >>> >> Heiner >>
Hi Heiner On 1/23/2019 12:39 AM, Heiner Kallweit wrote: > On 22.01.2019 15:46, Lendacky, Thomas wrote: >> On 1/21/19 12:36 PM, Heiner Kallweit wrote: >>> On 21.01.2019 17:35, Andrew Lunn wrote: >>>> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote: >>>>> The state machine is a no-op before phy_start() has been called. >>>>> Therefore let's enable it in phy_start() only. In phy_start() >>>>> let's call phy_start_machine() instead of phy_trigger_machine(). >>>>> phy_start_machine is an alias for phy_trigger_machine but it makes >>>>> clearer that we start the state machine here instead of just >>>>> triggering a run. >>>> Hi Heiner >>>> >>>> Documentation/networking/phy.txt has a section "Doing it all yourself" >>>> It would be good to review that, and make sure that documentation is >>>> still valid. I'm not sure any MAC driver actually does do it all >>>> itself. So it might be worth reviewing the whole document and making >>>> updates to remove parts of the text. >>>> >>> Right. I figured out that I have update phy.txt anyway because I >>> recently removed phy_stop_interrupts which is referenced in the >>> documentation. OK if we leave the patch series as is and I submit >>> the documentation update as a separate patch? >> I think you need to be careful here and not break what is allowed in the >> "Doing it all yourself" section. The amd-xgbe driver makes use of this >> functionality and does not use phy_start()/phy_stop(). Specifically, it >> does: >> get_phy_device(); >> phy_device_register(); >> phy_attach_direct(); >> >> At which point it uses phy_start_aneg(), phy_read(), phy_write(), >> phy_read_status() and phy_aneg_done(). >> > Thanks for the hint, Tom. I *think* the changes should be safe. > However, if AMD has a regression test suite I'd appreciate if you could > test the changes upfront or once they reach net-next. > Can you please cc: on your changes so you I can verify them before they get pushed upstream?
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 745a705a5..3df6aadc5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -883,7 +883,7 @@ void phy_start(struct phy_device *phydev) } mutex_unlock(&phydev->lock); - phy_trigger_machine(phydev); + phy_start_machine(phydev); } EXPORT_SYMBOL(phy_start); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index e0ceecbed..3e284d596 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -944,7 +944,6 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev, return rc; phy_prepare_link(phydev, handler); - phy_start_machine(phydev); if (phydev->irq > 0) phy_start_interrupts(phydev); diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index e7becc737..e9b8f1037 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -679,7 +679,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy) __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising); - phy_start_machine(phy); if (phy->irq > 0) phy_start_interrupts(phy);
The state machine is a no-op before phy_start() has been called. Therefore let's enable it in phy_start() only. In phy_start() let's call phy_start_machine() instead of phy_trigger_machine(). phy_start_machine is an alias for phy_trigger_machine but it makes clearer that we start the state machine here instead of just triggering a run. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 2 +- drivers/net/phy/phy_device.c | 1 - drivers/net/phy/phylink.c | 1 - 3 files changed, 1 insertion(+), 3 deletions(-)