ftgmac100: Mostly rewrite the driver
PATCH 1/1 ftgmac100: Mostly rewrite the driver
This is an almost complete rewrite of this driver.
The patch overall multiplies the performance of the driver on an AST2500 eval board with a gigabit link by 3 to 4.
I get arounnd 400Mbit/s with this vs. about 80Mbit/s with the current driver using iperf3. I've done some tests on NC-SI machines as well, I get close to peak (above 90Mbit/s).
It does that by, among other things, rewriting the receive and transmit code, to both simplify the fast path as much as possible, avoid flushing of the caches (the aspeed chips have really slow little ARM cores), implementing support for fragmented sends, fixing HW checksum generation (AST2500 only), etc...
In addition, I've added netpoll support, tx timeout recovery, better handling of link speed changes, multicast filter and promisc support, various ethtool config additions etc...
Finally, the code has been cleaned up and reorganized, and various corner cases fixed, such as recovery in some error situations.
--
While I tried initially to do an incremental series of patch this quickly became unrealistic as I rewrote more of the driver. This is better reviewed as a new replacement driver for the same chip instead. Since the Aspeed SoC is the only in-tree user and we are maintaining this for OpenBMC, the risk is limited, I didn't feel the need of submitting this as a separate driver and deprecate the old one.
Checkpatch note: There are some remaining checkpatch complaints that I chose not to address. The bulk is lines slightly over 80 characters which I feel fixing would impair the readability of the code. The ones about using BIT macros, I chose not to rewrite all the original driver bit definitions. We can do that as a separate mechanical patch later if needed. Finally the handful remaining ones are personal style choices that in my opinion make the code more pleasant to read.
--- .../devicetree/bindings/net/ftgmac100.txt | 37 + arch/arm/boot/dts/aspeed-g4.dtsi | 6 +- arch/arm/boot/dts/aspeed-g5.dtsi | 6 +- drivers/net/ethernet/faraday/ftgmac100.c | 2182 +++++++++++--------- drivers/net/ethernet/faraday/ftgmac100.h | 182 +- 5 files changed, 1450 insertions(+), 963 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/ftgmac100.txt
Documentation/devicetree/bindings/net/ftgmac100.txt new file +37 -0
@@ -0,0 +1,37 @@ +* Faraday Technology FTGMAC100 gigabit ethernet controller + +Required properties: +- compatible: "faraday,ftgmac100" + + Must also contain one of these if used as part of an Aspeed AST2400 + or 2500 family SoC as they have some subtle tweaks to the + implementation: + + - "aspeed,ast2400-mac" + - "aspeed,ast2500-mac" + +- reg: Address and length of the register set for the device +- interrupts: Should contain ethernet controller interrupt + +Optional properties: +- phy-mode: See ethernet.txt file in the same directory. If the property is + absent, "rgmii" is assumed. Supported values are "rgmii" and "rmii" +- use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes + rmii (100bT) but kept as a separate property in case NC-SI grows support + for a gigabit link. +- no-hw-checksum: Used to disable HW checksum support. Here for backward + compatibility as the driver now should have correct defaults based on + the SoC. +- pinctrl related properties + +Example: + + mac0: ethernet@1e660000 { + compatible = "aspeed,ast2500-mac", "faraday,ftgmac100"; + reg = <0x1e660000 0x180>; + interrupts = <2>; + status = "okay"; + use-ncsi; + }; + +
arch/arm/boot/dts/aspeed-g4.dtsi +2 -4
@@ -42,18 +42,16 @@ }; mac0: ethernet@1e660000 { - compatible = "faraday,ftgmac100"; + compatible = "aspeed,ast2400-mac", "faraday,ftgmac100"; reg = <0x1e660000 0x180>; interrupts = <2>; - no-hw-checksum; status = "disabled"; }; mac1: ethernet@1e680000 { - compatible = "faraday,ftgmac100"; + compatible = "aspeed,ast2400-mac", "faraday,ftgmac100"; reg = <0x1e680000 0x180>; interrupts = <3>; - no-hw-checksum; status = "disabled"; };
arch/arm/boot/dts/aspeed-g5.dtsi +2 -4
@@ -33,18 +33,16 @@ }; mac0: ethernet@1e660000 { - compatible = "faraday,ftgmac100"; + compatible = "aspeed,ast2500-mac", "faraday,ftgmac100"; reg = <0x1e660000 0x180>; interrupts = <2>; - no-hw-checksum; status = "disabled"; }; mac1: ethernet@1e680000 { - compatible = "faraday,ftgmac100"; + compatible = "aspeed,ast2500-mac", "faraday,ftgmac100"; reg = <0x1e680000 0x180>; interrupts = <3>; - no-hw-checksum; status = "disabled"; };
drivers/net/ethernet/faraday/ftgmac100.c +1232 -950
2,616 lines not on this page · raw message
drivers/net/ethernet/faraday/ftgmac100.h +177 -5
210 lines not on this page · raw message
David Miller
maintainerThis is unreviewable.
You must break this up into small, reviewable pieces.
If you didn't save the steps of your work in that way, that isn't our problem.
Quoted 6 lines · David Miller
This is unreviewable.
You must break this up into small, reviewable pieces.
If you didn't save the steps of your work in that way, that isn't our problem.
That's not realistic, it would probably not improve the readability much. I basically ended up rewriting the driver almost completely Dave. It's not even a matter of saving my work steps, each of them involved pulling appart an entire side of the old driver and re-doing it.
That's why I said it's better reviewed as a new driver. We have no other user of it in the tree anyway.
Do you prefer that I submit it as a new driver for that IP block instead and take out the old one later ?
Cheers,
Ben.
David Miller
maintainerFrom: Benjamin Herrenschmidt <[email protected]>
Date: Wed, 29 Mar 2017 16:03:07 +1100
Quoted 14 lines · David Miller
Quoted 6 lines
This is unreviewable.
You must break this up into small, reviewable pieces.
If you didn't save the steps of your work in that way, that isn't our problem.
That's not realistic, it would probably not improve the readability much. I basically ended up rewriting the driver almost completely Dave. It's not even a matter of saving my work steps, each of them involved pulling appart an entire side of the old driver and re-doing it.
Quoted 5 lines
That's why I said it's better reviewed as a new driver. We have no other user of it in the tree anyway.
Do you prefer that I submit it as a new driver for that IP block instead and take out the old one later ?
You've decided to do this work in a way that makes it nearly impossible to audit the individual changes for regressions and whatnot.
That puts a much larger burdon upon us, and introduces much greater potential risk.
Even a "complete driver rewrite" can and very often is done in a way which is evolutionary rather than revolutionary. You made a conscious decision to just work on this internally and in such a way that one big huge change is the result.
So, I'm sorry to say, your arguments about readability, realisticness, etc. I do not buy at all. You definitely could have done this work in a way which was more reviewable, and safer, but you choose not to.
Now, what is going to happen, is that we'll have no choice but to simply accept what you've done and try and review this monster.
Thanks a lot.
Quoted 9 lines · David Miller
Do you prefer that I submit it as a new driver for that IP block
Quoted 1 line
instead and take out the old one later ?
You've decided to do this work in a way that makes it nearly impossible to audit the individual changes for regressions and whatnot.
That puts a much larger burdon upon us, and introduces much greater potential risk.
Well, i started the "right way" ... it just got out of hand as I realized that I had to cut deeper than I expected I would.
I may not have been very clear but the regressions etc... aren't a huge issue at this point. The reason is that before we started putting the aspeed SoC support upstream, there was no in-tree user of that driver, it had been bitrotting for years. We are pretty much (OpenBMC project) the only user at this point.
Quoted 1 line
From the testing we've done, the "new" driver is already more solid
than the previous one ever was. So we're happy to take the chance here and submit any subsequent fix if we hit issues during testing.
Quoted 4 lines
Even a "complete driver rewrite" can and very often is done in a way which is evolutionary rather than revolutionary. You made a conscious decision to just work on this internally and in such a way that one big huge change is the result.
Not really. I started with incremental changes. Then it got out of control... I pretty much had to completely redo the tx and rx path, then link monitoring, then the chip config etc...
Quoted 7 lines
So, I'm sorry to say, your arguments about readability, realisticness,
etc. I do not buy at all. You definitely could have done this work in a way which was more reviewable, and safer, but you choose not to.Now, what is going to happen, is that we'll have no choice but to simply accept what you've done and try and review this monster.
I'm sorry for that, but that's why I suggest treating it as a whole new driver instead.
Viewed this way it becomes a rather simple ethernet driver.
If that's too hard, I can try to go back and re-construct the work as a series of patches on the old driver, pulling appart the tx path separately from the rx path, etc... but that will take quite a bit of time.
Otherwise, if that can help, I'll submit it as a separate file ftgmac100_new.c/.h in the email to make the review work easier, and later a patch to take out the old one.
Cheers,
Ben.
Quoted 46 lines · Benjamin Herrenschmidt
Quoted 9 lines · David Miller
Do you prefer that I submit it as a new driver for that IP block
Quoted 1 line
instead and take out the old one later ?
You've decided to do this work in a way that makes it nearly impossible to audit the individual changes for regressions and whatnot.
That puts a much larger burdon upon us, and introduces much greater potential risk.
Well, i started the "right way" ... it just got out of hand as I realized that I had to cut deeper than I expected I would.
I may not have been very clear but the regressions etc... aren't a huge issue at this point. The reason is that before we started putting the aspeed SoC support upstream, there was no in-tree user of that driver, it had been bitrotting for years. We are pretty much (OpenBMC project) the only user at this point.
From the testing we've done, the "new" driver is already more solid than the previous one ever was. So we're happy to take the chance here and submit any subsequent fix if we hit issues during testing.
Quoted 4 lines
Even a "complete driver rewrite" can and very often is done in a way which is evolutionary rather than revolutionary. You made a conscious decision to just work on this internally and in such a way that one big huge change is the result.
Not really. I started with incremental changes. Then it got out of control... I pretty much had to completely redo the tx and rx path, then link monitoring, then the chip config etc...
Quoted 7 lines
So, I'm sorry to say, your arguments about readability, realisticness,
etc. I do not buy at all. You definitely could have done this work in a way which was more reviewable, and safer, but you choose not to.Now, what is going to happen, is that we'll have no choice but to simply accept what you've done and try and review this monster.
I'm sorry for that, but that's why I suggest treating it as a whole new driver instead.
Viewed this way it becomes a rather simple ethernet driver.
Quoted 8 lines
If that's too hard, I can try to go back and re-construct the work as a series of patches on the old driver, pulling appart the tx path separately from the rx path, etc... but that will take quite a bit of time.
Otherwise, if that can help, I'll submit it as a separate file ftgmac100_new.c/.h in the email to make the review work easier, and later a patch to take out the old one.
This would not be any different from what you are actually doing here, except it would give the illusion of listening to the feedback you have been given but essentially it's the same thing ;)
Here is a bit of trivia: when I started working on the BCMGENET driver, it was not in a great shape, fortunately (or not) this was all downstream, it took about 200 small individual commits to make it into a shape where I would not be ashamed to submit it upstream, but in the process we could always ensure there would not be any crazy regressions introduced, and it was all quick and easy to review.
Based on your commit message, you should be able to easily split:
- netpoll support
- timeout/recovery
- additional ethtool operations (one patch for each presumably?)
- multicast support
- RX/TX path rewrites
etc.
That alone would be a lot more manageable for everyone on this mailing-list if presented as a collection of individual patches to review.
I hear your point that you are the only users of the driver and it's already in a bad shape, but take this as an opportunity to increase your commit count ;)
Signature · 1 line
Florian
Quoted 3 lines · Florian Fainelli
I hear your point that you are the only users of the driver and it's already in a bad shape, but take this as an opportunity to increase your commit count ;)
Haha, my commit count is fine thanks ;-)
I'll see what I can do. I need to respin anyway, I haven't tested the netpoll support and it doesn't build (ugh). So I'll try to break it down. Maybe not 200 patches but I'll see if I can make it more palatable.
Cheers,
Ben.
Quoted 6 lines · Benjamin Herrenschmidt
Quoted 4 lines · Florian Fainelli
I hear your point that you are the only users of the driver and it's
already in a bad shape, but take this as an opportunity to increase your commit count ;)
Quoted 6 lines
Haha, my commit count is fine thanks ;-)
I'll see what I can do. I need to respin anyway, I haven't tested the netpoll support and it doesn't build (ugh). So I'll try to break it down. Maybe not 200 patches but I'll see if I can make it more palatable.
FYI, Dave, Florian
I've started re-doing the work in the form of a series of patches.
I can't promise I'll manage to make them all really small but I'll do my best. So hold onto reviewing if you haven't started already.
The end result will, I hope, be identical.
Cheers,
Ben.
David Miller
maintainerFrom: Benjamin Herrenschmidt <[email protected]>
Date: Thu, 30 Mar 2017 11:46:18 +1100
Quoted 14 lines · Benjamin Herrenschmidt
Quoted 6 lines · Florian Fainelli
Quoted 4 lines
I hear your point that you are the only users of the driver and it's
already in a bad shape, but take this as an opportunity to increase your commit count ;)Quoted 6 lines
Haha, my commit count is fine thanks ;-)
I'll see what I can do. I need to respin anyway, I haven't tested the netpoll support and it doesn't build (ugh). So I'll try to break it down. Maybe not 200 patches but I'll see if I can make it more palatable.
Quoted 8 lines
FYI, Dave, Florian
I've started re-doing the work in the form of a series of patches.
I can't promise I'll manage to make them all really small but I'll do my best. So hold onto reviewing if you haven't started already.
The end result will, I hope, be identical.
Thanks for doing this.
Quoted 9 lines · David Miller
Quoted 7 lines
I've started re-doing the work in the form of a series of patches.
I can't promise I'll manage to make them all really small but I'll do my best. So hold onto reviewing if you haven't started already.
The end result will, I hope, be identical.Thanks for doing this.
Alllllright. 2 days and half a night later, we have a 55 patches series
...oops ;-)
I did a few things differently from the original patch but overall it's pretty similar. At least that exercise allowed me to find and fix a couple of bugs along the way. I hope I didn't introduce twice as many...
We're running some more testing tonight, if it's all solid I'll shoot it out tomorrow or sunday. Dave, it's ok to just spam the list with a 55 patches series like that ?
For the curious, if any, it's there along with an unrelated fix for testing:
https://github.com/ozbenh/linux-ast/commits/upstreaming-work
Cheers,
Ben.
Quoted 3 lines
We're running some more testing tonight, if it's all solid I'll shoot it out tomorrow or sunday. Dave, it's ok to just spam the list with a 55 patches series like that ?
Hi Ben
Is there a good reason to spam the list with 55 patches? The patches should be incremental, so getting them reviewed and applied in batches of 10 should not be a problem.
Andrew
Quoted 9 lines · Andrew Lunn
Quoted 3 lines
We're running some more testing tonight, if it's all solid I'll shoot it out tomorrow or sunday. Dave, it's ok to just spam the list with a 55 patches series like that ?
Hi Ben
Is there a good reason to spam the list with 55 patches? The patches should be incremental, so getting them reviewed and applied in batches of 10 should not be a problem.
They are incremental, but some of them are trivial and in the end it's the end result that matters but yes I could probably split some misc stuff, rx path, tx path, and more misc.
I found an issue with link down vs. pending tx packets last night so I need to fix that and test. I'll send things when that's done.
Cheers,
Ben.
Quoted 3 lines
They are incremental, but some of them are trivial and in the end it's the end result that matters but yes I could probably split some misc stuff, rx path, tx path, and more misc.
Hi Ben
Trivial patches are good. They are easy to review. You should be aiming for patches which are obviously correct, where ever possible. Refactoring existing code often has a lot of obviously correct patches, and a few complex patches which take some effort to review.
Andrew
David Miller
maintainerFrom: Benjamin Herrenschmidt <[email protected]>
Date: Fri, 31 Mar 2017 20:59:27 +1100
Quoted 3 lines
We're running some more testing tonight, if it's all solid I'll shoot it out tomorrow or sunday. Dave, it's ok to just spam the list with a 55 patches series like that ?
Please send about a dozen at a time, thank you. Group them logically as best as you can.
kbuild test robot
build boturl: https://github.com/0day-ci/linux/commits/Benjamin-Herrenschmidt/ftgmac100-Mostly-rewrite-the-driver/20170329-155424 config: arm-allmodconfig (attached as .config) compiler…
Hi Benjamin,
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Benjamin-Herrenschmidt/ftgmac100-Mostly-rewrite-the-driver/20170329-155424
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/net/ethernet/faraday/ftgmac100.c: In function 'ftgmac100_netpoll':
>> drivers/net/ethernet/faraday/ftgmac100.c:1277:14: error: 'dev' undeclared (first use in this function)
disable_irq(dev->irq);
^~~
drivers/net/ethernet/faraday/ftgmac100.c:1277:14: note: each undeclared identifier is reported only once for each function it appears in
drivers/net/ethernet/faraday/ftgmac100.c: At top level:
>> drivers/net/ethernet/faraday/ftgmac100.c:1295:25: error: 'ftgmac100_poll_controller' undeclared here (not in a function) .ndo_poll_controller = ftgmac100_poll_controller,
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/faraday/ftgmac100.c:1273:13: warning: 'ftgmac100_netpoll' defined but not used [-Wunused-function]
static void ftgmac100_netpoll(struct net_device *ndev)
^~~~~~~~~~~~~~~~~
vim +/dev +1277 drivers/net/ethernet/faraday/ftgmac100.c
1271
1272 #ifdef CONFIG_NET_POLL_CONTROLLER
1273 static void ftgmac100_netpoll(struct net_device *ndev)
1274 {
1275 unsigned long flags;
1276
> 1277 disable_irq(dev->irq);
1278 local_irq_save(flags);
1279 ftgmac100_interrupt(dev->irq, ndev);
1280 local_irq_restore(flags);
1281 enable_irq(dev->irq);
1282 }
1283 #endif
1284
1285 static const struct net_device_ops ftgmac100_netdev_ops = {
1286 .ndo_open = ftgmac100_open,
1287 .ndo_stop = ftgmac100_stop,
1288 .ndo_start_xmit = ftgmac100_hard_start_xmit,
1289 .ndo_set_mac_address = ftgmac100_set_mac_addr,
1290 .ndo_set_rx_mode = ftgmac100_set_rx_mode,
1291 .ndo_validate_addr = eth_validate_addr,
1292 .ndo_do_ioctl = ftgmac100_do_ioctl,
1293 .ndo_tx_timeout = ftgmac100_tx_timeout,
1294 #ifdef CONFIG_NET_POLL_CONTROLLER
> 1295 .ndo_poll_controller = ftgmac100_poll_controller, 1296 #endif
1297 };
1298
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation