From: Serge Semin <fancer.lancer@gmail.com> To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>, Alexei Starovoitov <ast@kernel.org>, bpf@vger.kernel.org, Daniel Borkmann <daniel@iogearbox.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Jose Abreu <joabreu@synopsys.com>, linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org, Paolo Abeni <pabeni@redhat.com> Subject: Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink Date: Tue, 14 May 2024 02:04:00 +0300 [thread overview] Message-ID: <y2iz5uhcj5xh3dtpg3rnxap4qgvmgavzkf6qd7c2vqysmll3yx@drhs7upgpojz> (raw) In-Reply-To: <ZkDuJAx7atDXjf5m@shell.armlinux.org.uk> Hi Russell On Sun, May 12, 2024 at 05:28:20PM +0100, Russell King (Oracle) wrote: > Hi, > > As I noted recently in a thread (and was ignored) Sorry about that. I didn't mean to ignore. Your message reached me right when I caught a cold, which made me AFK for the rest of the week.( > As I noted recently in a thread (and was ignored) stmmac sucks. Can't argue with that. There are much more aspects in what it sucks than just the netif's. One glimpse at the plat_stmmacenet_data structure causes million questions aka why, how come, what the hell... I'll start submitting my cleanup patch sets after my another networking work (DW XPCS wise) is finally done, re-submitted, reviewed and merged in. > (I > won't hide my distain for drivers that make my life as phylink > maintainer more difficult!) > > One of the contract conditions for using phylink is that the driver > will _not_ mess with the netif carrier. stmmac developers/maintainers > clearly didn't read that, because stmmac messes with the netif > carrier, which destroys phylink's guarantee that it'll make certain > calls in a particular order (e.g. it won't call mac_link_up() twice > in a row without an intervening mac_link_down().) This is clearly > stated in the phylink documentation. > > Thus, this patch set attempts to fix this. Why does it mess with the > netif carrier? It has its own independent PCS implementation that > completely bypasses phylink _while_ phylink is still being used. > This is not acceptable. Either the driver uses phylink, or it doesn't > use phylink. There is no half-way house about this. Therefore, this > driver needs to either be fixed, or needs to stop using phylink. Thanks for submitting the series, especially for making the RGMII in-band status well-implemented in the driver. When I was studying the STMMAC internals I was surprised that it wasn't actually utilized for something useful. Furthermore at some point afterwards even the RGSMIIIS IRQ turned to be disabled. So the RGMII-part of the code has been completely unused after that. But even before that the RGMII in-band status change IRQ was utilized just to print the link state into the system log. > > Since I was ignored when I brought this up, I've hacked together the > following patch set - and it is hacky at the moment. It's also broken > because of recentl changes involving dwmac-qcom-ethqos.c - but there > isn't sufficient information in the driver for me to fix this. The > driver appears to use SGMII at 2500Mbps, which simply does not exist. > What interface mode (and neg_mode) does phylink pass to pcs_config() > in each of the speeds that dwmac-qcom-ethqos.c is interested in. > Without this information, I can't do that conversion. So for the > purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means > it will fail to build.) > > The patch splitup is not ideal, but that's not what I'm interested in > here. What I want to hear is the results of testing - does this switch > of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver? > > Please don't review the patches, but you are welcome to send fixes to > them. Once we know that the overall implementation works, then I'll > look at how best to split the patches. In the mean time, the present > form is more convenient for making changes and fixing things. I'll give your series a try later on this week on my DW GMAC with the RGMII interface (alas I don't have an SGMII capable device, so can't help with the AN-part testing). Today I've made a brief glance on it and already noted a few places which may require a fix to make the change working for RGMII (at least the RGSMIIIS IRQ must be got back enabled). After making the patch set working for my device in what form would you prefer me to submit the fixes? As incremental patches in-reply to this thread? -Serge(y) > > There is still more improvement that's needed here. > > Thanks. > > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > drivers/net/ethernet/stmicro/stmmac/common.h | 12 ++- > .../net/ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++--------- > drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 108 ++++++++++++-------- > .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 6 -- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 27 ++--- > .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 111 +------------------- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 19 ++-- > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c | 57 +++++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h | 84 ++------------- > 10 files changed, 227 insertions(+), 312 deletions(-) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
WARNING: multiple messages have this Message-ID (diff)
From: Serge Semin <fancer.lancer@gmail.com> To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>, Alexei Starovoitov <ast@kernel.org>, bpf@vger.kernel.org, Daniel Borkmann <daniel@iogearbox.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Jose Abreu <joabreu@synopsys.com>, linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org, Paolo Abeni <pabeni@redhat.com> Subject: Re: [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink Date: Tue, 14 May 2024 02:04:00 +0300 [thread overview] Message-ID: <y2iz5uhcj5xh3dtpg3rnxap4qgvmgavzkf6qd7c2vqysmll3yx@drhs7upgpojz> (raw) In-Reply-To: <ZkDuJAx7atDXjf5m@shell.armlinux.org.uk> Hi Russell On Sun, May 12, 2024 at 05:28:20PM +0100, Russell King (Oracle) wrote: > Hi, > > As I noted recently in a thread (and was ignored) Sorry about that. I didn't mean to ignore. Your message reached me right when I caught a cold, which made me AFK for the rest of the week.( > As I noted recently in a thread (and was ignored) stmmac sucks. Can't argue with that. There are much more aspects in what it sucks than just the netif's. One glimpse at the plat_stmmacenet_data structure causes million questions aka why, how come, what the hell... I'll start submitting my cleanup patch sets after my another networking work (DW XPCS wise) is finally done, re-submitted, reviewed and merged in. > (I > won't hide my distain for drivers that make my life as phylink > maintainer more difficult!) > > One of the contract conditions for using phylink is that the driver > will _not_ mess with the netif carrier. stmmac developers/maintainers > clearly didn't read that, because stmmac messes with the netif > carrier, which destroys phylink's guarantee that it'll make certain > calls in a particular order (e.g. it won't call mac_link_up() twice > in a row without an intervening mac_link_down().) This is clearly > stated in the phylink documentation. > > Thus, this patch set attempts to fix this. Why does it mess with the > netif carrier? It has its own independent PCS implementation that > completely bypasses phylink _while_ phylink is still being used. > This is not acceptable. Either the driver uses phylink, or it doesn't > use phylink. There is no half-way house about this. Therefore, this > driver needs to either be fixed, or needs to stop using phylink. Thanks for submitting the series, especially for making the RGMII in-band status well-implemented in the driver. When I was studying the STMMAC internals I was surprised that it wasn't actually utilized for something useful. Furthermore at some point afterwards even the RGSMIIIS IRQ turned to be disabled. So the RGMII-part of the code has been completely unused after that. But even before that the RGMII in-band status change IRQ was utilized just to print the link state into the system log. > > Since I was ignored when I brought this up, I've hacked together the > following patch set - and it is hacky at the moment. It's also broken > because of recentl changes involving dwmac-qcom-ethqos.c - but there > isn't sufficient information in the driver for me to fix this. The > driver appears to use SGMII at 2500Mbps, which simply does not exist. > What interface mode (and neg_mode) does phylink pass to pcs_config() > in each of the speeds that dwmac-qcom-ethqos.c is interested in. > Without this information, I can't do that conversion. So for the > purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means > it will fail to build.) > > The patch splitup is not ideal, but that's not what I'm interested in > here. What I want to hear is the results of testing - does this switch > of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver? > > Please don't review the patches, but you are welcome to send fixes to > them. Once we know that the overall implementation works, then I'll > look at how best to split the patches. In the mean time, the present > form is more convenient for making changes and fixing things. I'll give your series a try later on this week on my DW GMAC with the RGMII interface (alas I don't have an SGMII capable device, so can't help with the AN-part testing). Today I've made a brief glance on it and already noted a few places which may require a fix to make the change working for RGMII (at least the RGSMIIIS IRQ must be got back enabled). After making the patch set working for my device in what form would you prefer me to submit the fixes? As incremental patches in-reply to this thread? -Serge(y) > > There is still more improvement that's needed here. > > Thanks. > > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > drivers/net/ethernet/stmicro/stmmac/common.h | 12 ++- > .../net/ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++--------- > drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 108 ++++++++++++-------- > .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c | 6 -- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 27 ++--- > .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 111 +------------------- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 19 ++-- > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c | 57 +++++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h | 84 ++------------- > 10 files changed, 227 insertions(+), 312 deletions(-) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-05-13 23:04 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-05-12 16:28 [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink Russell King (Oracle) 2024-05-12 16:28 ` Russell King (Oracle) 2024-05-12 16:28 ` [PATCH RFC net-next 1/6] net: stmmac: convert sgmii/rgmii " Russell King (Oracle) 2024-05-12 16:28 ` Russell King (Oracle) 2024-05-12 16:29 ` [PATCH RFC net-next 2/6] net: stmmac: remove pcs_ctrl_ane() method and associated code Russell King (Oracle) 2024-05-12 16:29 ` Russell King (Oracle) 2024-05-13 4:13 ` kernel test robot 2024-05-13 5:15 ` kernel test robot 2024-05-25 4:13 ` kernel test robot 2024-05-25 4:24 ` kernel test robot 2024-05-12 16:29 ` [PATCH RFC net-next 3/6] net: stmmac: remove pcs_rane() method Russell King (Oracle) 2024-05-12 16:29 ` Russell King (Oracle) 2024-05-12 16:29 ` [PATCH RFC net-next 4/6] net: stmmac: remove calls to stmmac_pcs_get_adv_lp() Russell King (Oracle) 2024-05-12 16:29 ` Russell King (Oracle) 2024-05-12 16:29 ` [PATCH RFC net-next 5/6] net: stmmac: remove pcs_get_adv_lp() method and associated code Russell King (Oracle) 2024-05-12 16:29 ` Russell King (Oracle) 2024-05-12 16:29 ` [PATCH RFC net-next 6/6] net: stmmac: remove old pcs interrupt functions Russell King (Oracle) 2024-05-12 16:29 ` Russell King (Oracle) 2024-05-13 23:04 ` Serge Semin [this message] 2024-05-13 23:04 ` [PATCH RFC 0/6] net: stmmac: convert stmmac "pcs" to phylink Serge Semin 2024-05-13 23:21 ` Russell King (Oracle) 2024-05-13 23:21 ` Russell King (Oracle) 2024-05-13 23:53 ` Serge Semin 2024-05-13 23:53 ` Serge Semin 2024-05-24 23:54 ` Serge Semin 2024-05-24 23:54 ` Serge Semin 2024-05-24 21:02 ` [PATCH RFC net-next 1/3] net: stmmac: Prevent RGSMIIIS IRQs flood Serge Semin 2024-05-24 21:02 ` Serge Semin 2024-05-24 21:02 ` [PATCH RFC net-next 2/3] net: stmmac: Activate Inband/PCS flag based on the selected iface Serge Semin 2024-05-24 21:02 ` Serge Semin 2024-05-26 16:49 ` Russell King (Oracle) 2024-05-26 16:49 ` Russell King (Oracle) 2024-05-26 18:00 ` Russell King (Oracle) 2024-05-26 18:00 ` Russell King (Oracle) 2024-05-28 13:19 ` Serge Semin 2024-05-28 13:19 ` Serge Semin 2024-05-28 14:13 ` Russell King (Oracle) 2024-05-28 14:13 ` Russell King (Oracle) 2024-05-28 16:21 ` Russell King (Oracle) 2024-05-28 16:21 ` Russell King (Oracle) 2024-05-26 21:57 ` Serge Semin 2024-05-26 21:57 ` Serge Semin 2024-05-28 10:21 ` Russell King (Oracle) 2024-05-28 10:21 ` Russell King (Oracle) 2024-05-28 12:22 ` Serge Semin 2024-05-28 12:22 ` Serge Semin 2024-05-24 21:02 ` [PATCH RFC net-next 3/3] net: stmmac: Drop TBI/RTBI PCS flags Serge Semin 2024-05-24 21:02 ` Serge Semin 2024-05-28 10:23 ` Russell King (Oracle) 2024-05-28 10:23 ` Russell King (Oracle) 2024-05-28 12:23 ` Serge Semin 2024-05-28 12:23 ` Serge Semin 2024-05-28 10:24 ` [PATCH RFC net-next 1/3] net: stmmac: Prevent RGSMIIIS IRQs flood Russell King (Oracle) 2024-05-28 10:24 ` Russell King (Oracle) 2024-05-28 12:20 ` Serge Semin 2024-05-28 12:20 ` Serge Semin
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=y2iz5uhcj5xh3dtpg3rnxap4qgvmgavzkf6qd7c2vqysmll3yx@drhs7upgpojz \ --to=fancer.lancer@gmail.com \ --cc=alexandre.torgue@foss.st.com \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=hawk@kernel.org \ --cc=joabreu@synopsys.com \ --cc=john.fastabend@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=linux@armlinux.org.uk \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.