| Message ID | ZxD6cVFajwBlC9eN@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: pcs: xpcs: yet more cleanups | expand |
Hi On Thu, Oct 17, 2024 at 12:52:17PM GMT, Russell King (Oracle) wrote: > Hi, > > I've found yet more potential for cleanups in the XPCS driver. > > The first patch switches to using generic register definitions. > > Next, there's an overly complex bit of code in xpcs_link_up_1000basex() > which can be simplified down to a simple if() statement. > > Then, rearrange xpcs_link_up_1000basex() to separate out the warnings > from the functional bit. > > Next, realising that the functional bit is just the helper function we > already have and are using in the SGMII version of this function, > switch over to that. > > We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii() > are basically functionally identical except for the warnings, so merge > the two functions. > > Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the > established pattern. > > Lastly, "return foo();" where foo is a void function and the function > being returned from is also void is a weird programming pattern. > Replace this with something more conventional. > > With these changes, we see yet another reduction in the amount of > code in this driver. If you wish this to be tested before merging in, I'll be able to do that tomorrow or on Wednesday. In anyway I'll get back with the results after testing the series out. -Serge(y) > > drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++----------------------- > drivers/net/pcs/pcs-xpcs.h | 12 ---- > 2 files changed, 65 insertions(+), 81 deletions(-) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! >
Hi netdev maintainers, I see patchwork has failed again. It claims this series does not have a cover letter, but it does, and lore has it: https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/ vs https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/ I guess the kernel.org infrastructure has failed in some way to deliver the cover message to patchwork. On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote: > Hi, > > I've found yet more potential for cleanups in the XPCS driver. > > The first patch switches to using generic register definitions. > > Next, there's an overly complex bit of code in xpcs_link_up_1000basex() > which can be simplified down to a simple if() statement. > > Then, rearrange xpcs_link_up_1000basex() to separate out the warnings > from the functional bit. > > Next, realising that the functional bit is just the helper function we > already have and are using in the SGMII version of this function, > switch over to that. > > We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii() > are basically functionally identical except for the warnings, so merge > the two functions. > > Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the > established pattern. > > Lastly, "return foo();" where foo is a void function and the function > being returned from is also void is a weird programming pattern. > Replace this with something more conventional. > > With these changes, we see yet another reduction in the amount of > code in this driver. > > drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++----------------------- > drivers/net/pcs/pcs-xpcs.h | 12 ---- > 2 files changed, 65 insertions(+), 81 deletions(-) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! >
On 10/22/24 10:59, Russell King (Oracle) wrote: > I see patchwork has failed again. It claims this series does not have a > cover letter, but it does, and lore has it: > > https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/ > > vs > > https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/ > > I guess the kernel.org infrastructure has failed in some way to deliver > the cover message to patchwork. Thanks for the head-up! I can't investigate the issue any deeper than you, lacking permissions on the relevant hosts, but I verified that the merged script fetch the cover letter correctly, so no need to repost just for that. Cheers, Paolo
On Tue, Oct 22, 2024 at 11:23:30AM +0200, Paolo Abeni wrote: > On 10/22/24 10:59, Russell King (Oracle) wrote: > > I see patchwork has failed again. It claims this series does not have a > > cover letter, but it does, and lore has it: > > > > https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/ > > > > vs > > > > https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/ > > > > I guess the kernel.org infrastructure has failed in some way to deliver > > the cover message to patchwork. > > Thanks for the head-up! > > I can't investigate the issue any deeper than you, lacking permissions > on the relevant hosts, but I verified that the merged script fetch the > cover letter correctly, so no need to repost just for that. Thanks for checking. Johannes explained that the checks are done by Jakub's nipa bot and uploaded to patchwork. https://github.com/linux-netdev/nipa/blob/main/tests/series/series_format/test.py Seems nipa (or Jakub?) missed out on the cover message.
On Mon, Oct 21, 2024 at 02:09:02PM GMT, Serge Semin wrote: > Hi > > On Thu, Oct 17, 2024 at 12:52:17PM GMT, Russell King (Oracle) wrote: > > Hi, > > > > I've found yet more potential for cleanups in the XPCS driver. > > > > The first patch switches to using generic register definitions. > > > > Next, there's an overly complex bit of code in xpcs_link_up_1000basex() > > which can be simplified down to a simple if() statement. > > > > Then, rearrange xpcs_link_up_1000basex() to separate out the warnings > > from the functional bit. > > > > Next, realising that the functional bit is just the helper function we > > already have and are using in the SGMII version of this function, > > switch over to that. > > > > We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii() > > are basically functionally identical except for the warnings, so merge > > the two functions. > > > > Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the > > established pattern. > > > > Lastly, "return foo();" where foo is a void function and the function > > being returned from is also void is a weird programming pattern. > > Replace this with something more conventional. > > > > With these changes, we see yet another reduction in the amount of > > code in this driver. > > If you wish this to be tested before merging in, I'll be able to do > that tomorrow or on Wednesday. In anyway I'll get back with the > results after testing the series out. As I promised, the series has been tested on the hardware with the next setup: DW XGMAC <-(XGMII)-> DW XPCS <-(10Gbase-R)-> Marvell 88x2222 <-(10gbase-R)-> SFP+ JT-DAC-SFP-05 SFP+ <-(10gbase-R)-> Marvell 88x2222 <-(10gbase-R)-> DW XPCS <-(XGMII)-> DW XGMAC No problem has been spotted for both STMMAC and DW XPCS drivers: Tested-by: Serge Semin <fancer.lancer@gmail.com> -Serge(y) > > -Serge(y) > > > > > drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++----------------------- > > drivers/net/pcs/pcs-xpcs.h | 12 ---- > > 2 files changed, 65 insertions(+), 81 deletions(-) > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! > >
On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote: > Hi, > > I've found yet more potential for cleanups in the XPCS driver. > > The first patch switches to using generic register definitions. > > Next, there's an overly complex bit of code in xpcs_link_up_1000basex() > which can be simplified down to a simple if() statement. > > Then, rearrange xpcs_link_up_1000basex() to separate out the warnings > from the functional bit. > > Next, realising that the functional bit is just the helper function we > already have and are using in the SGMII version of this function, > switch over to that. > > We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii() > are basically functionally identical except for the warnings, so merge > the two functions. > > Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the > established pattern. > > Lastly, "return foo();" where foo is a void function and the function > being returned from is also void is a weird programming pattern. > Replace this with something more conventional. > > With these changes, we see yet another reduction in the amount of > code in this driver. > > drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++----------------------- > drivers/net/pcs/pcs-xpcs.h | 12 ---- > 2 files changed, 65 insertions(+), 81 deletions(-) It's been almost a week, and this series has not been applied. First, Jakub's NIPA bot failed to spot the cover message that patchwork picked up - not my problem. Now, I find that patchwork says "changes requested". What changes? No one has replied asking for any changes to this series. Serge did reply saying he would test it, and he has now done so, and replied with his tested-by. So, marking this series as "changes requested" is entirely incorrect. If you think changes are required, please say what they are, or if no changes are required, please apply this series. Thanks.
On 10/23/24 13:17, Russell King (Oracle) wrote: > On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote: >> I've found yet more potential for cleanups in the XPCS driver. >> >> The first patch switches to using generic register definitions. >> >> Next, there's an overly complex bit of code in xpcs_link_up_1000basex() >> which can be simplified down to a simple if() statement. >> >> Then, rearrange xpcs_link_up_1000basex() to separate out the warnings >> from the functional bit. >> >> Next, realising that the functional bit is just the helper function we >> already have and are using in the SGMII version of this function, >> switch over to that. >> >> We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii() >> are basically functionally identical except for the warnings, so merge >> the two functions. >> >> Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the >> established pattern. >> >> Lastly, "return foo();" where foo is a void function and the function >> being returned from is also void is a weird programming pattern. >> Replace this with something more conventional. >> >> With these changes, we see yet another reduction in the amount of >> code in this driver. >> >> drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++----------------------- >> drivers/net/pcs/pcs-xpcs.h | 12 ---- >> 2 files changed, 65 insertions(+), 81 deletions(-) > > It's been almost a week, and this series has not been applied. Yep, we are lagging a little behind our PW queue due to limited capacity. > First, > Jakub's NIPA bot failed to spot the cover message that patchwork picked > up - not my problem. > > Now, I find that patchwork says "changes requested". What changes? No > one has replied asking for any changes to this series. Serge did > reply saying he would test it, and he has now done so, and replied > with his tested-by. I agree there is no reason for the 'changes requested' status, I guess such change happened due to some miscommunication between me and Andrew. Let me resurrect the series in PW. I'll go over that soon. Thanks, Paolo
Hello: This series was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Thu, 17 Oct 2024 12:52:17 +0100 you wrote: > Hi, > > I've found yet more potential for cleanups in the XPCS driver. > > The first patch switches to using generic register definitions. > > Next, there's an overly complex bit of code in xpcs_link_up_1000basex() > which can be simplified down to a simple if() statement. > > [...] Here is the summary with links: - [net-next,1/7] net: pcs: xpcs: use generic register definitions https://git.kernel.org/netdev/net-next/c/1d2709d6d390 - [net-next,2/7] net: pcs: xpcs: remove switch() in xpcs_link_up_1000basex() https://git.kernel.org/netdev/net-next/c/8d2aeab4ce78 - [net-next,3/7] net: pcs: xpcs: rearrange xpcs_link_up_1000basex() https://git.kernel.org/netdev/net-next/c/b61a465a7619 - [net-next,4/7] net: pcs: xpcs: replace open-coded mii_bmcr_encode_fixed() https://git.kernel.org/netdev/net-next/c/1c17f9d3fe17 - [net-next,5/7] net: pcs: xpcs: combine xpcs_link_up_{1000basex,sgmii}() https://git.kernel.org/netdev/net-next/c/4145921c3055 - [net-next,6/7] net: pcs: xpcs: rename xpcs_config_usxgmii() https://git.kernel.org/netdev/net-next/c/11afdf3b2ece - [net-next,7/7] net: pcs: xpcs: remove return statements in void function https://git.kernel.org/netdev/net-next/c/fd4056db7aee You are awesome, thank you!
On Wed, 23 Oct 2024 12:17:18 +0100 Russell King (Oracle) wrote: > It's been almost a week, and this series has not been applied. First, > Jakub's NIPA bot failed to spot the cover message that patchwork picked > up - not my problem. FWIW NIPA gets the patch <> series metadata from patchwork. The only possible explanation I have for cover letter being in patchwork but getting flagged as not present by NIPA is that it arrived late. I keep repeating ad nauseam that the patchwork checks have false positives so they are for maintainers to look at, not submitters. I think the real failure here is that someone marked this series as CR silently and incorrectly :(