* RE: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Tristram.Ha @ 2017-09-08 18:40 UTC (permalink / raw)
To: andrew, muvarov
Cc: pavel, nathan.leigh.conrad, vivien.didelot, f.fainelli, netdev,
linux-kernel, Woojung.Huh
In-Reply-To: <20170908141225.GE25219@lunn.ch>
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, September 08, 2017 7:12 AM
> To: Maxim Uvarov
> Cc: Tristram Ha - C24268; Pavel Machek; Nathan Conrad; Vivien Didelot; Florian
> Fainelli; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
>
> On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> > 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> > >> -- compatible: For external switch chips, compatible string must be exactly
> one
> > >> - of: "microchip,ksz9477"
> > >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> > >> + "microchip,ksz8795" for KSZ8795 chip,
> > >> + "microchip,ksz8794" for KSZ8794 chip,
> > >> + "microchip,ksz8765" for KSZ8765 chip,
> > >> + "microchip,ksz8895" for KSZ8895 chip,
> > >> + "microchip,ksz8864" for KSZ8864 chip,
> > >> + "microchip,ksz8873" for KSZ8873 chip,
> > >> + "microchip,ksz8863" for KSZ8863 chip,
> > >> + "microchip,ksz8463" for KSZ8463 chip
> > >
> >
> > all that chips have the same spi access to get chip id on probe(). I
> > prefer common microship,ksz-spi rather than somebody will always
> > maintain that list.
>
> The Marvell DSA driver is similar. The compatibility string tells you
> enough to go find the switch ID in the switch itself.
>
> I suppose this comes down to, is there going to be one SPI driver for
> all the devices, or lots of drivers? In general, DSA has one driver
> for lots of devices. The mv88e6xxx supports around 25 devices. The b53
> has around 17, etc.
>
> So i would suggest one driver supporting all the different devices.
>
There will be 5 drivers to support these devices:
ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
ksz8795.c - KSZ8795/KSZ8795/KSZ8765
ksz8895.c - KSZ8895/KSZ8864
ksz8863.c - KSZ8863/KSZ8873
ksz8463.c - KSZ8463
These chips have different SPI access mechanisms, MIB counter reading,
and register set. These can be combined into one single driver using
function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
only concern is the memory footprint. The customer may not want a
big driver to cover all the switches while only one is used.
Out of topic I have a question to ask the community regarding the DSA
port creation:
Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
But our company Marketing does not want to promote that fact but treat
KSZ8794 as a distinct product.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
Is this okay to hide this information inside the driver? This is manageable
for KSZ8794 but for KSZ8864 the first port is disabled:
lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
I am not sure whether DSA has or will have a way to display the port
mapping to regular users.
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Florian Fainelli @ 2017-09-08 18:45 UTC (permalink / raw)
To: Tristram.Ha, andrew, muvarov
Cc: pavel, nathan.leigh.conrad, vivien.didelot, netdev, linux-kernel,
Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E68@CHN-SV-EXMX02.mchp-main.com>
On 09/08/2017 11:40 AM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Andrew Lunn [mailto:andrew@lunn.ch]
>> Sent: Friday, September 08, 2017 7:12 AM
>> To: Maxim Uvarov
>> Cc: Tristram Ha - C24268; Pavel Machek; Nathan Conrad; Vivien Didelot; Florian
>> Fainelli; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
>>> 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
>>>>> -- compatible: For external switch chips, compatible string must be exactly
>> one
>>>>> - of: "microchip,ksz9477"
>>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>>> + "microchip,ksz8795" for KSZ8795 chip,
>>>>> + "microchip,ksz8794" for KSZ8794 chip,
>>>>> + "microchip,ksz8765" for KSZ8765 chip,
>>>>> + "microchip,ksz8895" for KSZ8895 chip,
>>>>> + "microchip,ksz8864" for KSZ8864 chip,
>>>>> + "microchip,ksz8873" for KSZ8873 chip,
>>>>> + "microchip,ksz8863" for KSZ8863 chip,
>>>>> + "microchip,ksz8463" for KSZ8463 chip
>>>>
>>>
>>> all that chips have the same spi access to get chip id on probe(). I
>>> prefer common microship,ksz-spi rather than somebody will always
>>> maintain that list.
>>
>> The Marvell DSA driver is similar. The compatibility string tells you
>> enough to go find the switch ID in the switch itself.
>>
>> I suppose this comes down to, is there going to be one SPI driver for
>> all the devices, or lots of drivers? In general, DSA has one driver
>> for lots of devices. The mv88e6xxx supports around 25 devices. The b53
>> has around 17, etc.
>>
>> So i would suggest one driver supporting all the different devices.
>>
>
> There will be 5 drivers to support these devices:
>
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864
> ksz8863.c - KSZ8863/KSZ8873
> ksz8463.c - KSZ8463
>
> These chips have different SPI access mechanisms, MIB counter reading,
> and register set. These can be combined into one single driver using
> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
> only concern is the memory footprint. The customer may not want a
> big driver to cover all the switches while only one is used.
>
> Out of topic I have a question to ask the community regarding the DSA
> port creation:
>
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver? This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
What dictates all of that is ultimately Device Tree because it defines
the port mapping, including where the CPU port is. Once your driver
knows which chip it is "talking to" you could always have the driver
issue warnings and indicate that the Device Tree is malformed if the
user-specified port mapping does not match what the HW supports.
>
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.
DSA does display the port mapping of user facing ports under the
standard sysfs attribute /sys/class/net/*/phys_port_name. CPU port
mapping is not displayed because there is no CPU-port network device
(intentionally).
--
Florian
^ permalink raw reply
* Re: WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
From: Eric Dumazet @ 2017-09-08 18:46 UTC (permalink / raw)
To: Cong Wang
Cc: Shankara Pailoor, LKML, syzkaller, David Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpWOgBNmH456cOZcz-JgOYD1-YwwE4yRuLzzd4+ZzH4ALg@mail.gmail.com>
On Fri, 2017-09-08 at 10:21 -0700, Cong Wang wrote:
> (Cc'ing netdev)
>
> On Fri, Sep 8, 2017 at 5:59 AM, Shankara Pailoor <sp3485@columbia.edu> wrote:
> > Hi,
> >
> > I found a warning while fuzzing with Syzkaller on linux 4.13-rc7 on
> > x86_64. The full stack trace is below:
> >
> > WARNING: CPU: 2 PID: 4277 at lib/refcount.c:186
> > refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> > Kernel panic - not syncing: panic_on_warn set ...
> >
> > CPU: 2 PID: 4277 Comm: syz-executor0 Not tainted 4.13.0-rc7 #3
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > Ubuntu-1.8.2-1ubuntu1 04/01/2014
> > Call Trace:
> > <IRQ>
> > __dump_stack lib/dump_stack.c:16 [inline]
> > dump_stack+0xf7/0x1aa lib/dump_stack.c:52
> > panic+0x1ae/0x3a7 kernel/panic.c:180
> > __warn+0x1c4/0x1d9 kernel/panic.c:541
> > report_bug+0x211/0x2d0 lib/bug.c:183
> > fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:190
> > do_trap_no_signal arch/x86/kernel/traps.c:224 [inline]
> > do_trap+0x260/0x390 arch/x86/kernel/traps.c:273
> > do_error_trap+0x118/0x340 arch/x86/kernel/traps.c:310
> > do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:323
> > invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:846
> > RIP: 0010:refcount_sub_and_test+0x167/0x1b0 lib/refcount.c:186
> > RSP: 0018:ffff88006e006b60 EFLAGS: 00010286
> > RAX: 0000000000000026 RBX: 0000000000000000 RCX: 0000000000000000
> > RDX: 0000000000000026 RSI: 1ffff1000dc00d2c RDI: ffffed000dc00d60
> > RBP: ffff88006e006bf0 R08: 0000000000000001 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000dc00d6d
> > R13: 00000000ffffffff R14: 0000000000000001 R15: ffff88006ce9d340
> > refcount_dec_and_test+0x1a/0x20 lib/refcount.c:211
> > reqsk_put+0x71/0x2b0 include/net/request_sock.h:123
> > tcp_v4_rcv+0x259e/0x2e20 net/ipv4/tcp_ipv4.c:1729
> > ip_local_deliver_finish+0x2e2/0xba0 net/ipv4/ip_input.c:216
> > NF_HOOK include/linux/netfilter.h:248 [inline]
> > ip_local_deliver+0x1ce/0x6d0 net/ipv4/ip_input.c:257
> > dst_input include/net/dst.h:477 [inline]
> > ip_rcv_finish+0x8db/0x19c0 net/ipv4/ip_input.c:397
> > NF_HOOK include/linux/netfilter.h:248 [inline]
> > ip_rcv+0xc3f/0x17d0 net/ipv4/ip_input.c:488
> > __netif_receive_skb_core+0x1fb7/0x31f0 net/core/dev.c:4298
> > __netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4336
> > process_backlog+0x1c5/0x6d0 net/core/dev.c:5102
> > napi_poll net/core/dev.c:5499 [inline]
> > net_rx_action+0x6d3/0x14a0 net/core/dev.c:5565
> > __do_softirq+0x2cb/0xb2d kernel/softirq.c:284
> > do_softirq_own_stack+0x1c/0x30 arch/x86/entry/entry_64.S:898
> > </IRQ>
> > do_softirq.part.16+0x63/0x80 kernel/softirq.c:328
> > do_softirq kernel/softirq.c:176 [inline]
> > __local_bh_enable_ip+0x84/0x90 kernel/softirq.c:181
> > local_bh_enable include/linux/bottom_half.h:31 [inline]
> > rcu_read_unlock_bh include/linux/rcupdate.h:705 [inline]
> > ip_finish_output2+0x8ad/0x1360 net/ipv4/ip_output.c:231
> > ip_finish_output+0x74e/0xb80 net/ipv4/ip_output.c:317
> > NF_HOOK_COND include/linux/netfilter.h:237 [inline]
> > ip_output+0x1cc/0x850 net/ipv4/ip_output.c:405
> > dst_output include/net/dst.h:471 [inline]
> > ip_local_out+0x95/0x160 net/ipv4/ip_output.c:124
> > ip_queue_xmit+0x8c6/0x1810 net/ipv4/ip_output.c:504
> > tcp_transmit_skb+0x1963/0x3320 net/ipv4/tcp_output.c:1123
> > tcp_send_ack.part.35+0x38c/0x620 net/ipv4/tcp_output.c:3575
> > tcp_send_ack+0x49/0x60 net/ipv4/tcp_output.c:3545
> > tcp_rcv_synsent_state_process net/ipv4/tcp_input.c:5795 [inline]
> > tcp_rcv_state_process+0x4876/0x4b60 net/ipv4/tcp_input.c:5930
> > tcp_v4_do_rcv+0x58a/0x820 net/ipv4/tcp_ipv4.c:1483
> > sk_backlog_rcv include/net/sock.h:907 [inline]
> > __release_sock+0x124/0x360 net/core/sock.c:2223
> > release_sock+0xa4/0x2a0 net/core/sock.c:2715
> > inet_wait_for_connect net/ipv4/af_inet.c:557 [inline]
> > __inet_stream_connect+0x671/0xf00 net/ipv4/af_inet.c:643
> > inet_stream_connect+0x58/0xa0 net/ipv4/af_inet.c:682
> > SYSC_connect+0x204/0x470 net/socket.c:1628
> > SyS_connect+0x24/0x30 net/socket.c:1609
> > entry_SYSCALL_64_fastpath+0x18/0xad
> > RIP: 0033:0x451e59
> > RSP: 002b:00007f474843fc08 EFLAGS: 00000216 ORIG_RAX: 000000000000002a
> > RAX: ffffffffffffffda RBX: 0000000000718000 RCX: 0000000000451e59
> > RDX: 0000000000000010 RSI: 0000000020002000 RDI: 0000000000000007
> > RBP: 0000000000000046 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000216 R12: 0000000000000000
> > R13: 00007ffc040a0f8f R14: 00007f47484409c0 R15: 0000000000000000
> >
> >
> >
> >
> > I found that the following program is able to reproduce the warning:
> >
> >
> > Pastebin: https://pastebin.com/B75BdYKz
> >
> > Here are my configs: https://pastebin.com/zRYCXbak
> >
> > Regards,
> > Shankara
> >
Sweet, thanks for the report, I will have a look.
It seems one reqsk_put(req); is missing, but that would lead to a memory
leak, not a double reqsk_put(req) :/
( Same fix is needed for IPv6 )
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a63486afa7a7e7b4dce88b65bc27cfa872a3ba2f..d9416b5162bc1bdd1acd34fcb4da21cb6b62d0ae 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1669,9 +1669,9 @@ int tcp_v4_rcv(struct sk_buff *skb)
*/
sock_hold(sk);
refcounted = true;
- if (tcp_filter(sk, skb))
- goto discard_and_relse;
- nsk = tcp_check_req(sk, skb, req, false);
+ nsk = NULL;
+ if (!tcp_filter(sk, skb))
+ nsk = tcp_check_req(sk, skb, req, false);
if (!nsk) {
reqsk_put(req);
goto discard_and_relse;
^ permalink raw reply related
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Florian Fainelli @ 2017-09-08 18:48 UTC (permalink / raw)
To: Tristram.Ha, andrew, muvarov, pavel, nathan.leigh.conrad,
vivien.didelot, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121A5B@CHN-SV-EXMX02.mchp-main.com>
On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Add other KSZ switches support so that patch check does not complain.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117 ++++++++++++----------
> 1 file changed, 62 insertions(+), 55 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> index 0ab8b39..34af0e0 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>
> Required properties:
>
> -- compatible: For external switch chips, compatible string must be exactly one
> - of: "microchip,ksz9477"
> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> + "microchip,ksz8795" for KSZ8795 chip,
> + "microchip,ksz8794" for KSZ8794 chip,
> + "microchip,ksz8765" for KSZ8765 chip,
> + "microchip,ksz8895" for KSZ8895 chip,
> + "microchip,ksz8864" for KSZ8864 chip,
> + "microchip,ksz8873" for KSZ8873 chip,
> + "microchip,ksz8863" for KSZ8863 chip,
> + "microchip,ksz8463" for KSZ8463 chip
It becomes pretty obvious there is a 1 to 1 mapping between the
compatible name and what you should be using it for so specifying
ksz8795 for KSZ8795 chip is really redundant.
You could just list all compatible strings that you support and change
the verbiage to be:
compatible: Should be one of:
"microchip,ksz9477"
...
"microchip,ksz8463"
>
> See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional required and optional properties.
> @@ -13,60 +20,60 @@ Examples:
>
> Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>
> - eth0: ethernet@10001000 {
> - fixed-link {
> - speed = <1000>;
> - full-duplex;
> - };
> - };
> + eth0: ethernet@10001000 {
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
This is a good clean up, but it would probably belong in a separate
patch that you would do before adding compatible strings for instance.
>
> - spi1: spi@f8008000 {
> - pinctrl-0 = <&pinctrl_spi_ksz>;
> - cs-gpios = <&pioC 25 0>;
> - id = <1>;
> - status = "okay";
> + spi1: spi@f8008000 {
> + cs-gpios = <&pioC 25 0>;
> + id = <1>;
> + status = "okay";
>
> - ksz9477: ksz9477@0 {
> - compatible = "microchip,ksz9477";
> - reg = <0>;
> + ksz9477: ksz9477@0 {
> + compatible = "microchip,ksz9477";
> + reg = <0>;
>
> - spi-max-frequency = <44000000>;
> - spi-cpha;
> - spi-cpol;
> + spi-max-frequency = <44000000>;
> + spi-cpha;
> + spi-cpol;
> +
> + status = "okay";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + label = "lan1";
> + };
> + port@1 {
> + reg = <1>;
> + label = "lan2";
> + };
> + port@2 {
> + reg = <2>;
> + label = "lan3";
> + };
> + port@3 {
> + reg = <3>;
> + label = "lan4";
> + };
> + port@4 {
> + reg = <4>;
> + label = "lan5";
> + };
> + port@5 {
> + reg = <5>;
> + label = "cpu";
> + ethernet = <ð0>;
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> + };
> + };
> + };
>
> - status = "okay";
> - ports {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - port@0 {
> - reg = <0>;
> - label = "lan1";
> - };
> - port@1 {
> - reg = <1>;
> - label = "lan2";
> - };
> - port@2 {
> - reg = <2>;
> - label = "lan3";
> - };
> - port@3 {
> - reg = <3>;
> - label = "lan4";
> - };
> - port@4 {
> - reg = <4>;
> - label = "lan5";
> - };
> - port@5 {
> - reg = <5>;
> - label = "cpu";
> - ethernet = <ð0>;
> - fixed-link {
> - speed = <1000>;
> - full-duplex;
> - };
> - };
> - };
> - };
> - };
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: phy: realtek: add RTL8201F phy-id and functions
From: Florian Fainelli @ 2017-09-08 18:51 UTC (permalink / raw)
To: Kunihiko Hayashi, netdev, David S. Miller, Andrew Lunn
Cc: Rob Herring, Mark Rutland, linux-arm-kernel, linux-kernel,
devicetree, Masahiro Yamada, Masami Hiramatsu, Jassi Brar,
Jongsung Kim
In-Reply-To: <1504875731-3680-4-git-send-email-hayashi.kunihiko@socionext.com>
On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Add RTL8201F phy-id and the related functions to the driver.
>
> The original patch is as follows:
> https://patchwork.kernel.org/patch/2538341/
>
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> drivers/net/phy/realtek.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 9cbe645..d9974ce 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -29,10 +29,23 @@
> #define RTL8211F_PAGE_SELECT 0x1f
> #define RTL8211F_TX_DELAY 0x100
>
> +#define RTL8201F_ISR 0x1e
> +#define RTL8201F_PAGE_SELECT 0x1f
We have a page select register define for the RTL8211F right above, so
surely we can make that a common definition?
> +#define RTL8201F_IER 0x13
> +
> MODULE_DESCRIPTION("Realtek PHY driver");
> MODULE_AUTHOR("Johnson Leung");
> MODULE_LICENSE("GPL");
>
> +static int rtl8201_ack_interrupt(struct phy_device *phydev)
> +{
> + int err;
> +
> + err = phy_read(phydev, RTL8201F_ISR);
> +
> + return (err < 0) ? err : 0;
> +}
> +
> static int rtl821x_ack_interrupt(struct phy_device *phydev)
> {
> int err;
> @@ -54,6 +67,25 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
> return (err < 0) ? err : 0;
> }
>
> +static int rtl8201_config_intr(struct phy_device *phydev)
> +{
> + int err;
> +
> + /* switch to page 7 */
> + phy_write(phydev, RTL8201F_PAGE_SELECT, 0x7);
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> + err = phy_write(phydev, RTL8201F_IER,
> + BIT(13) | BIT(12) | BIT(11));
Can you detail what bits 11, 12 and 13 do? Do they correspond to link,
duplex and pause changes by any chance?
> + else
> + err = phy_write(phydev, RTL8201F_IER, 0);
> +
> + /* restore to default page 0 */
> + phy_write(phydev, RTL8201F_PAGE_SELECT, 0x0);
> +
> + return err;
> +}
> +
Other than that, LGTM:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> static int rtl8211b_config_intr(struct phy_device *phydev)
> {
> int err;
> @@ -129,6 +161,18 @@ static struct phy_driver realtek_drvs[] = {
> .config_aneg = &genphy_config_aneg,
> .read_status = &genphy_read_status,
> }, {
> + .phy_id = 0x001cc816,
> + .name = "RTL8201F 10/100Mbps Ethernet",
> + .phy_id_mask = 0x001fffff,
> + .features = PHY_BASIC_FEATURES,
> + .flags = PHY_HAS_INTERRUPT,
> + .config_aneg = &genphy_config_aneg,
> + .read_status = &genphy_read_status,
> + .ack_interrupt = &rtl8201_ack_interrupt,
> + .config_intr = &rtl8201_config_intr,
> + .suspend = genphy_suspend,
> + .resume = genphy_resume,
> + }, {
> .phy_id = 0x001cc912,
> .name = "RTL8211B Gigabit Ethernet",
> .phy_id_mask = 0x001fffff,
> @@ -181,6 +225,7 @@ static struct phy_driver realtek_drvs[] = {
> module_phy_driver(realtek_drvs);
>
> static struct mdio_device_id __maybe_unused realtek_tbl[] = {
> + { 0x001cc816, 0x001fffff },
> { 0x001cc912, 0x001fffff },
> { 0x001cc914, 0x001fffff },
> { 0x001cc915, 0x001fffff },
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 1/3] dt-bindings: net: add DT bindings for Socionext UniPhier AVE
From: Florian Fainelli @ 2017-09-08 18:54 UTC (permalink / raw)
To: Kunihiko Hayashi, netdev, David S. Miller, Andrew Lunn
Cc: Rob Herring, Mark Rutland, linux-arm-kernel, linux-kernel,
devicetree, Masahiro Yamada, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-2-git-send-email-hayashi.kunihiko@socionext.com>
On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> DT bindings for the AVE ethernet controller found on Socionext's
> UniPhier platforms.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> .../bindings/net/socionext,uniphier-ave4.txt | 44 ++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
>
> diff --git a/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> new file mode 100644
> index 0000000..57ae96d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt
> @@ -0,0 +1,44 @@
> +* Socionext AVE ethernet controller
> +
> +This describes the devicetree bindings for AVE ethernet controller
> +implemented on Socionext UniPhier SoCs.
> +
> +Required properties:
> + - compatible: Should be "socionext,uniphier-ave4"
> + - reg: Address where registers are mapped and size of region.
> + - interrupts: IRQ common for mac and phy interrupts.
> + - phy-mode: See ethernet.txt in the same directory.
> + - #address-cells: Must be <1>.
> + - #size-cells: Must be <0>.
> + - Node, with 'reg' property, for each PHY on the MDIO bus.
> +
> +Optional properties:
> + - socionext,desc-bits: 32/64 descriptor size. Default 32.
32 bits, this has an unit. Why is this configurable through Device Tree
and if this is describing the HW, as it should, why is not that inferred
from the compatible string?
> + - local-mac-address: See ethernet.txt in the same directory.
> + - pinctrl-names: List of assigned state names, see pinctrl
> + binding documentation.
> + - pinctrl-0: List of phandles to configure the GPIO pin used
> + as interrupt line, see also generic and your platform
> + specific pinctrl binding documentation.
> + - socionext,internal-phy-interrupt: Boolean to denote if the
> + PHY interrupt is internally handled by the MAC.
Interesting, why do you need to declare this as opposed to also
determining this entirely from the compatible string of the Ethernet MAC?
> +
> +
> +Example:
> +
> + eth: ethernet@65000000 {
> + compatible = "socionext,uniphier-ave4";
> + reg = <0x65000000 0x8500>;
> + interrupts = <0 66 4>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ether_rgmii>;
> + phy-mode = "rgmii";
> + socionext,desc-bits = <64>;
> + local-mac-address = [00 00 00 00 00 00];
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
There is no phy-handle property, yet there clearly seems to be one
attached, and I think Andrew now also responded to that in the driver
patch, this needs fixing.
> + ethphy: ethphy@1 {
> + reg = <1>;
> + };
> + };
>
--
Florian
^ permalink raw reply
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: Eduardo Valentin @ 2017-09-08 18:55 UTC (permalink / raw)
To: David Miller
Cc: dwmw2, eduval, vallish, shuah, richardcochran, xiyou.wangcong,
netdev, linux-kernel, anchalag, dwmw
In-Reply-To: <20170908.102645.1086537961399780085.davem@davemloft.net>
Hello,
On Fri, Sep 08, 2017 at 10:26:45AM -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Fri, 08 Sep 2017 18:23:22 +0100
>
> > I don't know that anyone's ever tried saying "show me the chapter and
> > verse of the documentation"
>
> Do you know why I brought this up? Because the person I am replying
> to told me that the syscall documentation should have suggested this
> or that.
>
> That's why.
:-) My intention was for sure not to upset anybody.
Just to reiterate, the point of patch is simple, there was a change in behavior in the system call from one kernel version to the other. As I mentioned, I agree that the userspace could use other means to achieve the same, but still the system call behavior has changed.
>
> So let's concentrate on the other aspects of my reply, ok?
I agree. I would prefer to understand here what is the technical reason not to accept these patches other than "use other system calls".
--
All the best,
Eduardo Valentin
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Maxim Uvarov @ 2017-09-08 19:00 UTC (permalink / raw)
To: Florian Fainelli
Cc: Tristram.Ha, Andrew Lunn, Pavel Machek, Nathan Conrad,
Vivien Didelot, netdev, linux-kernel, Woojung.Huh
In-Reply-To: <8e4aa981-7f41-047d-2101-370118b4f2c0@gmail.com>
2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>
>> Add other KSZ switches support so that patch check does not complain.
>>
>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>> ---
>> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117 ++++++++++++----------
>> 1 file changed, 62 insertions(+), 55 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> index 0ab8b39..34af0e0 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>
>> Required properties:
>>
>> -- compatible: For external switch chips, compatible string must be exactly one
>> - of: "microchip,ksz9477"
>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>> + "microchip,ksz8795" for KSZ8795 chip,
>> + "microchip,ksz8794" for KSZ8794 chip,
>> + "microchip,ksz8765" for KSZ8765 chip,
>> + "microchip,ksz8895" for KSZ8895 chip,
>> + "microchip,ksz8864" for KSZ8864 chip,
>> + "microchip,ksz8873" for KSZ8873 chip,
>> + "microchip,ksz8863" for KSZ8863 chip,
>> + "microchip,ksz8463" for KSZ8463 chip
>
Tristram, does any of this devices support chaining?
Maxim.
> It becomes pretty obvious there is a 1 to 1 mapping between the
> compatible name and what you should be using it for so specifying
> ksz8795 for KSZ8795 chip is really redundant.
>
> You could just list all compatible strings that you support and change
> the verbiage to be:
>
> compatible: Should be one of:
> "microchip,ksz9477"
> ...
> "microchip,ksz8463"
>>
>> See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional required and optional properties.
>> @@ -13,60 +20,60 @@ Examples:
>>
>> Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>>
>> - eth0: ethernet@10001000 {
>> - fixed-link {
>> - speed = <1000>;
>> - full-duplex;
>> - };
>> - };
>> + eth0: ethernet@10001000 {
>> + fixed-link {
>> + speed = <1000>;
>> + full-duplex;
>> + };
>> + };
>
> This is a good clean up, but it would probably belong in a separate
> patch that you would do before adding compatible strings for instance.
>
>>
>> - spi1: spi@f8008000 {
>> - pinctrl-0 = <&pinctrl_spi_ksz>;
>> - cs-gpios = <&pioC 25 0>;
>> - id = <1>;
>> - status = "okay";
>> + spi1: spi@f8008000 {
>> + cs-gpios = <&pioC 25 0>;
>> + id = <1>;
>> + status = "okay";
>>
>> - ksz9477: ksz9477@0 {
>> - compatible = "microchip,ksz9477";
>> - reg = <0>;
>> + ksz9477: ksz9477@0 {
>> + compatible = "microchip,ksz9477";
>> + reg = <0>;
>>
>> - spi-max-frequency = <44000000>;
>> - spi-cpha;
>> - spi-cpol;
>> + spi-max-frequency = <44000000>;
>> + spi-cpha;
>> + spi-cpol;
>> +
>> + status = "okay";
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + port@0 {
>> + reg = <0>;
>> + label = "lan1";
>> + };
>> + port@1 {
>> + reg = <1>;
>> + label = "lan2";
>> + };
>> + port@2 {
>> + reg = <2>;
>> + label = "lan3";
>> + };
>> + port@3 {
>> + reg = <3>;
>> + label = "lan4";
>> + };
>> + port@4 {
>> + reg = <4>;
>> + label = "lan5";
>> + };
>> + port@5 {
>> + reg = <5>;
>> + label = "cpu";
>> + ethernet = <ð0>;
>> + fixed-link {
>> + speed = <1000>;
>> + full-duplex;
>> + };
>> + };
>> + };
>> + };
>> + };
>>
>> - status = "okay";
>> - ports {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> - port@0 {
>> - reg = <0>;
>> - label = "lan1";
>> - };
>> - port@1 {
>> - reg = <1>;
>> - label = "lan2";
>> - };
>> - port@2 {
>> - reg = <2>;
>> - label = "lan3";
>> - };
>> - port@3 {
>> - reg = <3>;
>> - label = "lan4";
>> - };
>> - port@4 {
>> - reg = <4>;
>> - label = "lan5";
>> - };
>> - port@5 {
>> - reg = <5>;
>> - label = "cpu";
>> - ethernet = <ð0>;
>> - fixed-link {
>> - speed = <1000>;
>> - full-duplex;
>> - };
>> - };
>> - };
>> - };
>> - };
>>
>
>
> --
> Florian
--
Best regards,
Maxim Uvarov
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Andrew Lunn @ 2017-09-08 19:01 UTC (permalink / raw)
To: Tristram.Ha
Cc: muvarov, pavel, nathan.leigh.conrad, vivien.didelot, f.fainelli,
netdev, linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E68@CHN-SV-EXMX02.mchp-main.com>
> > So i would suggest one driver supporting all the different devices.
>
> There will be 5 drivers to support these devices:
>
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864
> ksz8863.c - KSZ8863/KSZ8873
> ksz8463.c - KSZ8463
>
> These chips have different SPI access mechanisms, MIB counter reading,
> and register set. These can be combined into one single driver using
> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
> only concern is the memory footprint. The customer may not want a
> big driver to cover all the switches while only one is used.
If memory footprint is your problem, make it a compile time choice
which devices are supported within the one driver. In practice, you
will find most distributions just enable them all.
> Out of topic I have a question to ask the community regarding the DSA
> port creation:
>
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver? This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
>
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.
So the port number to name is determined in the device tree. It
depends on the board how the ports are named. I always suggest going
by the label on the box. clearfog is a good example of this, 0 - lan5,
1 - lan4, etc.
So for the KSZ8864, ensure reg = 0 causes an error.
Does the hardware force which port is used for the CPU? I've boards
with Marvell devices where the CPU is port 0, or port 6, or port
7. The hardware does not care. So we don't force anything.
As for KSZ8794 vs KSZ8795, is there different IDs in the silicon? It
is these IDs you are using, not the compatible string, to determine
how the drive the silicon. You can trust the ID in the
silicon. Anything else can be wrong.
Andrew
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Florian Fainelli @ 2017-09-08 19:05 UTC (permalink / raw)
To: Andrew Lunn, Tristram.Ha
Cc: muvarov, pavel, nathan.leigh.conrad, vivien.didelot, netdev,
linux-kernel, Woojung.Huh
In-Reply-To: <20170908190122.GM25219@lunn.ch>
On 09/08/2017 12:01 PM, Andrew Lunn wrote:
>>> So i would suggest one driver supporting all the different devices.
>>
>> There will be 5 drivers to support these devices:
>>
>> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
>> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
>> ksz8895.c - KSZ8895/KSZ8864
>> ksz8863.c - KSZ8863/KSZ8873
>> ksz8463.c - KSZ8463
>>
>> These chips have different SPI access mechanisms, MIB counter reading,
>> and register set. These can be combined into one single driver using
>> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463. My
>> only concern is the memory footprint. The customer may not want a
>> big driver to cover all the switches while only one is used.
>
> If memory footprint is your problem, make it a compile time choice
> which devices are supported within the one driver. In practice, you
> will find most distributions just enable them all.
>
>> Out of topic I have a question to ask the community regarding the DSA
>> port creation:
>>
>> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
>> and so on. The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
>> So
>> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
>> But our company Marketing does not want to promote that fact but treat
>> KSZ8794 as a distinct product.
>> So
>> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
>> Is this okay to hide this information inside the driver? This is manageable
>> for KSZ8794 but for KSZ8864 the first port is disabled:
>> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
>>
>> I am not sure whether DSA has or will have a way to display the port
>> mapping to regular users.
>
> So the port number to name is determined in the device tree. It
> depends on the board how the ports are named. I always suggest going
> by the label on the box. clearfog is a good example of this, 0 - lan5,
> 1 - lan4, etc.
>
> So for the KSZ8864, ensure reg = 0 causes an error.
>
> Does the hardware force which port is used for the CPU? I've boards
> with Marvell devices where the CPU is port 0, or port 6, or port
> 7. The hardware does not care. So we don't force anything.
>
> As for KSZ8794 vs KSZ8795, is there different IDs in the silicon? It
> is these IDs you are using, not the compatible string, to determine
> how the drive the silicon. You can trust the ID in the
> silicon. Anything else can be wrong.
You can't always trust the silicon to report the right revision, just
like marketing people in semiconductors tend to like to play tricks and
pretend that the same chip is the same, but it is not quite, and a new
tape-out was not possible, so it has the same ID, including the
revision. This is of course not what should be done, but it happens all
the time.
Describe in Device Tree with a proper compatible string, which may
end-up being treated the same way by the driver, and if it happens that
the silicon is not wrong, and can be differentiated, then you can always
resort to that to warn the user the wrong compatible was specified, or
that this is not going to work, or anything really.
--
Florian
^ permalink raw reply
* Re: [PATCH v2 0/2] enable hires timer to timeout datagram socket
From: Eric Dumazet @ 2017-09-08 19:11 UTC (permalink / raw)
To: Eduardo Valentin, Thomas Gleixner
Cc: David Miller, dwmw2, vallish, shuah, richardcochran,
xiyou.wangcong, netdev, linux-kernel, anchalag, dwmw
In-Reply-To: <20170908185521.GA12340@u40b0340c692b58f6553c.ant.amazon.com>
On Fri, 2017-09-08 at 11:55 -0700, Eduardo Valentin wrote:
> Hello,
>
> On Fri, Sep 08, 2017 at 10:26:45AM -0700, David Miller wrote:
> > From: David Woodhouse <dwmw2@infradead.org>
> > Date: Fri, 08 Sep 2017 18:23:22 +0100
> >
> > > I don't know that anyone's ever tried saying "show me the chapter
> and
> > > verse of the documentation"
> >
> > Do you know why I brought this up? Because the person I am replying
> > to told me that the syscall documentation should have suggested this
> > or that.
> >
> > That's why.
>
> :-) My intention was for sure not to upset anybody.
>
> Just to reiterate, the point of patch is simple, there was a change in
> behavior in the system call from one kernel version to the other. As I
> mentioned, I agree that the userspace could use other means to achieve
> the same, but still the system call behavior has changed.
>
> >
> > So let's concentrate on the other aspects of my reply, ok?
>
> I agree. I would prefer to understand here what is the technical
> reason not to accept these patches other than "use other system
> calls".
So if we need to replace all 'legacy' timers to high resolution timer,
because some application was _relying_ on jiffies being kind of precise,
maybe it is better to revert the change done on legacy timers.
Or continue the migration and make them use high res internally.
select() and poll() are the standard way to have precise timeouts,
it is silly we have to maintain a timeout handling in the datagram fast
path.
^ permalink raw reply
* Re: ipset losing entries on its own
From: Akshat Kakkar @ 2017-09-08 19:24 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev, netdev-owner
In-Reply-To: <CAA5aLPgM9=VU4Cysy-LtN-uXPqvG00X2v8MP9EM5RtPVB6tGvw@mail.gmail.com>
Any more information needed?
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Florian Fainelli @ 2017-09-08 19:31 UTC (permalink / raw)
To: Kunihiko Hayashi, netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
Andrew Lunn
Cc: Rob Herring, Mark Rutland,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Masahiro Yamada,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <1504875731-3680-3-git-send-email-hayashi.kunihiko-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:
> The UniPhier platform from Socionext provides the AVE ethernet
> controller that includes MAC and MDIO bus supporting RGMII/RMII
> modes. The controller is named AVE.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Signed-off-by: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/net/ethernet/Kconfig | 1 +
> drivers/net/ethernet/Makefile | 1 +
> drivers/net/ethernet/socionext/Kconfig | 22 +
> drivers/net/ethernet/socionext/Makefile | 4 +
> drivers/net/ethernet/socionext/sni_ave.c | 1618 ++++++++++++++++++++++++++++++
> 5 files changed, 1646 insertions(+)
> create mode 100644 drivers/net/ethernet/socionext/Kconfig
> create mode 100644 drivers/net/ethernet/socionext/Makefile
> create mode 100644 drivers/net/ethernet/socionext/sni_ave.c
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index c604213..d50519e 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -170,6 +170,7 @@ source "drivers/net/ethernet/sis/Kconfig"
> source "drivers/net/ethernet/sfc/Kconfig"
> source "drivers/net/ethernet/sgi/Kconfig"
> source "drivers/net/ethernet/smsc/Kconfig"
> +source "drivers/net/ethernet/socionext/Kconfig"
> source "drivers/net/ethernet/stmicro/Kconfig"
> source "drivers/net/ethernet/sun/Kconfig"
> source "drivers/net/ethernet/tehuti/Kconfig"
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index a0a03d4..9f55b36 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_SFC) += sfc/
> obj-$(CONFIG_SFC_FALCON) += sfc/falcon/
> obj-$(CONFIG_NET_VENDOR_SGI) += sgi/
> obj-$(CONFIG_NET_VENDOR_SMSC) += smsc/
> +obj-$(CONFIG_NET_VENDOR_SOCIONEXT) += socionext/
> obj-$(CONFIG_NET_VENDOR_STMICRO) += stmicro/
> obj-$(CONFIG_NET_VENDOR_SUN) += sun/
> obj-$(CONFIG_NET_VENDOR_TEHUTI) += tehuti/
> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
> new file mode 100644
> index 0000000..788f26f
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Kconfig
> @@ -0,0 +1,22 @@
> +config NET_VENDOR_SOCIONEXT
> + bool "Socionext ethernet drivers"
> + default y
> + ---help---
> + Option to select ethernet drivers for Socionext platforms.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about Agere devices. If you say Y, you will be asked
> + for your specific card in the following questions.
> +
> +if NET_VENDOR_SOCIONEXT
> +
> +config SNI_AVE
> + tristate "Socionext AVE ethernet support"
> + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> + select PHYLIB
> + ---help---
> + Driver for gigabit ethernet MACs, called AVE, in the
> + Socionext UniPhier family.
> +
> +endif #NET_VENDOR_SOCIONEXT
> diff --git a/drivers/net/ethernet/socionext/Makefile b/drivers/net/ethernet/socionext/Makefile
> new file mode 100644
> index 0000000..0356341
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for all ethernet ip drivers on Socionext platforms
> +#
> +obj-$(CONFIG_SNI_AVE) += sni_ave.o
> diff --git a/drivers/net/ethernet/socionext/sni_ave.c b/drivers/net/ethernet/socionext/sni_ave.c
> new file mode 100644
> index 0000000..c870777
> --- /dev/null
> +++ b/drivers/net/ethernet/socionext/sni_ave.c
> @@ -0,0 +1,1618 @@
> +/**
> + * sni_ave.c - Socionext UniPhier AVE ethernet driver
> + *
> + * Copyright 2014 Panasonic Corporation
> + * Copyright 2015-2017 Socionext Inc.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/etherdevice.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of_net.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy.h>
> +#include <linux/reset.h>
> +#include <linux/types.h>
> +
> +/* General Register Group */
> +#define AVE_IDR 0x0 /* ID */
> +#define AVE_VR 0x4 /* Version */
> +#define AVE_GRR 0x8 /* Global Reset */
> +#define AVE_CFGR 0xc /* Configuration */
> +
> +/* Interrupt Register Group */
> +#define AVE_GIMR 0x100 /* Global Interrupt Mask */
> +#define AVE_GISR 0x104 /* Global Interrupt Status */
> +
> +/* MAC Register Group */
> +#define AVE_TXCR 0x200 /* TX Setup */
> +#define AVE_RXCR 0x204 /* RX Setup */
> +#define AVE_RXMAC1R 0x208 /* MAC address (lower) */
> +#define AVE_RXMAC2R 0x20c /* MAC address (upper) */
> +#define AVE_MDIOCTR 0x214 /* MDIO Control */
> +#define AVE_MDIOAR 0x218 /* MDIO Address */
> +#define AVE_MDIOWDR 0x21c /* MDIO Data */
> +#define AVE_MDIOSR 0x220 /* MDIO Status */
> +#define AVE_MDIORDR 0x224 /* MDIO Rd Data */
> +
> +/* Descriptor Control Register Group */
> +#define AVE_DESCC 0x300 /* Descriptor Control */
> +#define AVE_TXDC 0x304 /* TX Descriptor Configuration */
> +#define AVE_RXDC0 0x308 /* RX Descriptor Ring0 Configuration */
> +#define AVE_IIRQC 0x34c /* Interval IRQ Control */
> +
> +/* Frame Counter Register Group */
> +#define AVE_BFCR 0x400 /* Bad Frame Counter */
> +#define AVE_RX0OVFFC 0x414 /* OVF Frame Counter (Ring0) */
> +#define AVE_SN5FC 0x438 /* Frame Counter No5 */
> +#define AVE_SN6FC 0x43c /* Frame Counter No6 */
> +#define AVE_SN7FC 0x440 /* Frame Counter No7 */
> +
> +/* Packet Filter Register Group */
> +#define AVE_PKTF_BASE 0x800 /* PF Base Address */
> +#define AVE_PFMBYTE_BASE 0xd00 /* PF Mask Byte Base Address */
> +#define AVE_PFMBIT_BASE 0xe00 /* PF Mask Bit Base Address */
> +#define AVE_PFSEL_BASE 0xf00 /* PF Selector Base Address */
> +#define AVE_PFEN 0xffc /* Packet Filter Enable */
> +#define AVE_PKTF(ent) (AVE_PKTF_BASE + (ent) * 0x40)
> +#define AVE_PFMBYTE(ent) (AVE_PFMBYTE_BASE + (ent) * 8)
> +#define AVE_PFMBIT(ent) (AVE_PFMBIT_BASE + (ent) * 4)
> +#define AVE_PFSEL(ent) (AVE_PFSEL_BASE + (ent) * 4)
> +
> +/* 64bit descriptor memory */
> +#define AVE_DESC_SIZE_64 12 /* Descriptor Size */
> +
> +#define AVE_TXDM_64 0x1000 /* Tx Descriptor Memory */
> +#define AVE_RXDM_64 0x1c00 /* Rx Descriptor Memory */
> +
> +#define AVE_TXDM_SIZE_64 0x0ba0 /* Tx Descriptor Memory Size 3KB */
> +#define AVE_RXDM_SIZE_64 0x6000 /* Rx Descriptor Memory Size 24KB */
> +
> +/* 32bit descriptor memory */
> +#define AVE_DESC_SIZE_32 8 /* Descriptor Size */
> +
> +#define AVE_TXDM_32 0x1000 /* Tx Descriptor Memory */
> +#define AVE_RXDM_32 0x1800 /* Rx Descriptor Memory */
> +
> +#define AVE_TXDM_SIZE_32 0x07c0 /* Tx Descriptor Memory Size 2KB */
> +#define AVE_RXDM_SIZE_32 0x4000 /* Rx Descriptor Memory Size 16KB */
> +
> +/* RMII Bridge Register Group */
> +#define AVE_RSTCTRL 0x8028 /* Reset control */
> +#define AVE_RSTCTRL_RMIIRST BIT(16)
> +#define AVE_LINKSEL 0x8034 /* Link speed setting */
> +#define AVE_LINKSEL_100M BIT(0)
> +
> +/* AVE_GRR */
> +#define AVE_GRR_RXFFR BIT(5) /* Reset RxFIFO */
> +#define AVE_GRR_PHYRST BIT(4) /* Reset external PHY */
> +#define AVE_GRR_GRST BIT(0) /* Reset all MAC */
> +
> +/* AVE_GISR (common with GIMR) */
> +#define AVE_GI_PHY BIT(24) /* PHY interrupt */
> +#define AVE_GI_TX BIT(16) /* Tx complete */
> +#define AVE_GI_RXERR BIT(8) /* Receive frame more than max size */
> +#define AVE_GI_RXOVF BIT(7) /* Overflow at the RxFIFO */
> +#define AVE_GI_RXDROP BIT(6) /* Drop packet */
> +#define AVE_GI_RXIINT BIT(5) /* Interval interrupt */
> +
> +/* AVE_TXCR */
> +#define AVE_TXCR_FLOCTR BIT(18) /* Flow control */
> +#define AVE_TXCR_TXSPD_1G BIT(17)
> +#define AVE_TXCR_TXSPD_100 BIT(16)
> +
> +/* AVE_RXCR */
> +#define AVE_RXCR_RXEN BIT(30) /* Rx enable */
> +#define AVE_RXCR_FDUPEN BIT(22) /* Interface mode */
> +#define AVE_RXCR_FLOCTR BIT(21) /* Flow control */
> +#define AVE_RXCR_AFEN BIT(19) /* MAC address filter */
> +#define AVE_RXCR_DRPEN BIT(18) /* Drop pause frame */
> +#define AVE_RXCR_MPSIZ_MASK GENMASK(10, 0)
> +
> +/* AVE_MDIOCTR */
> +#define AVE_MDIOCTR_RREQ BIT(3) /* Read request */
> +#define AVE_MDIOCTR_WREQ BIT(2) /* Write request */
> +
> +/* AVE_MDIOSR */
> +#define AVE_MDIOSR_STS BIT(0) /* access status */
> +
> +/* AVE_DESCC */
> +#define AVE_DESCC_TD BIT(0) /* Enable Tx descriptor */
> +#define AVE_DESCC_RDSTP BIT(4) /* Pause Rx descriptor */
> +#define AVE_DESCC_RD0 BIT(8) /* Enable Rx descriptor Ring0 */
> +
> +#define AVE_DESCC_CTRL_MASK GENMASK(15, 0)
> +
> +/* AVE_TXDC */
> +#define AVE_TXDC_SIZE GENMASK(27, 16) /* Size of Tx descriptor */
> +#define AVE_TXDC_ADDR GENMASK(11, 0) /* Start address */
> +#define AVE_TXDC_ADDR_START 0
> +
> +/* AVE_RXDC0 */
> +#define AVE_RXDC0_SIZE GENMASK(30, 16) /* Size of Rx descriptor */
> +#define AVE_RXDC0_ADDR GENMASK(14, 0) /* Start address */
> +#define AVE_RXDC0_ADDR_START 0
> +
> +/* AVE_IIRQC */
> +#define AVE_IIRQC_EN0 BIT(27) /* Enable interval interrupt Ring0 */
> +#define AVE_IIRQC_BSCK GENMASK(15, 0) /* Interval count unit */
> +
> +/* Command status for Descriptor */
> +#define AVE_STS_OWN BIT(31) /* Descriptor ownership */
> +#define AVE_STS_INTR BIT(29) /* Request for interrupt */
> +#define AVE_STS_OK BIT(27) /* Normal transmit */
> +/* TX */
> +#define AVE_STS_NOCSUM BIT(28) /* No use HW checksum */
> +#define AVE_STS_1ST BIT(26) /* Head of buffer chain */
> +#define AVE_STS_LAST BIT(25) /* Tail of buffer chain */
> +#define AVE_STS_OWC BIT(21) /* Out of window,Late Collision */
> +#define AVE_STS_EC BIT(20) /* Excess collision occurred */
> +#define AVE_STS_PKTLEN_TX GENMASK(15, 0)
> +/* RX */
> +#define AVE_STS_CSSV BIT(21) /* Checksum check performed */
> +#define AVE_STS_CSER BIT(20) /* Checksum error detected */
> +#define AVE_STS_PKTLEN_RX GENMASK(10, 0)
> +
> +/* AVE_CFGR */
> +#define AVE_CFGR_FLE BIT(31) /* Filter Function */
> +#define AVE_CFGR_CHE BIT(30) /* Checksum Function */
> +#define AVE_CFGR_MII BIT(27) /* Func mode (1:MII/RMII, 0:RGMII) */
> +#define AVE_CFGR_IPFCEN BIT(24) /* IP fragment sum Enable */
> +
> +#define AVE_MAX_ETHFRAME 1518
> +
> +/* Packet filter size */
> +#define AVE_PF_SIZE 17 /* Number of all packet filter */
> +#define AVE_PF_MULTICAST_SIZE 7 /* Number of multicast filter */
> +
> +/* Packet filter definition */
> +#define AVE_PFNUM_FILTER 0 /* No.0 */
> +#define AVE_PFNUM_UNICAST 1 /* No.1 */
> +#define AVE_PFNUM_BROADCAST 2 /* No.2 */
> +#define AVE_PFNUM_MULTICAST 11 /* No.11-17 */
> +
> +/* NETIF Message control */
> +#define AVE_DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | \
> + NETIF_MSG_PROBE | \
> + NETIF_MSG_LINK | \
> + NETIF_MSG_TIMER | \
> + NETIF_MSG_IFDOWN | \
> + NETIF_MSG_IFUP | \
> + NETIF_MSG_RX_ERR | \
> + NETIF_MSG_TX_ERR)
> +
> +/* Parameter for descriptor */
> +#define AVE_NR_TXDESC 32 /* Tx descriptor */
> +#define AVE_NR_RXDESC 64 /* Rx descriptor */
> +
> +/* Parameter for interrupt */
> +#define AVE_INTM_COUNT 20
> +#define AVE_FORCE_TXINTCNT 1
> +
> +#define IS_DESC_64BIT(p) ((p)->desc_size == AVE_DESC_SIZE_64)
> +
> +enum desc_id {
> + AVE_DESCID_TX = 0,
> + AVE_DESCID_RX,
> +};
> +
> +enum desc_state {
> + AVE_DESC_STOP = 0,
> + AVE_DESC_START,
> + AVE_DESC_RX_SUSPEND,
> + AVE_DESC_RX_PERMIT,
> +};
> +
> +struct ave_desc {
> + struct sk_buff *skbs;
> + dma_addr_t skbs_dma;
> + size_t skbs_dmalen;
> +};
> +
> +struct ave_desc_info {
> + u32 ndesc; /* number of descriptor */
> + u32 daddr; /* start address of descriptor */
> + u32 proc_idx; /* index of processing packet */
> + u32 done_idx; /* index of processed packet */
> + struct ave_desc *desc; /* skb info related descriptor */
> +};
> +
> +struct ave_private {
> + int phy_id;
> + unsigned int desc_size;
> + u32 msg_enable;
> + struct clk *clk;
> + phy_interface_t phy_mode;
> + struct phy_device *phydev;
> + struct mii_bus *mdio;
> + struct net_device_stats stats;
> + bool internal_phy_interrupt;
> +
> + /* NAPI support */
> + struct net_device *ndev;
> + struct napi_struct napi_rx;
> + struct napi_struct napi_tx;
> +
> + /* descriptor */
> + struct ave_desc_info rx;
> + struct ave_desc_info tx;
> +};
> +
> +static inline u32 ave_r32(struct net_device *ndev, u32 offset)
> +{
> + void __iomem *addr = (void __iomem *)ndev->base_addr;
> +
> + return readl_relaxed(addr + offset);
> +}
Don't do this, ndev->base_addr is convenient, but is now deprecated
(unlike ISA cards, you can't change this anymore) instead, just pass a
reference to an ave_private structure and store the base register
pointer there.
> +
> +static inline void ave_w32(struct net_device *ndev, u32 offset, u32 value)
> +{
> + void __iomem *addr = (void __iomem *)ndev->base_addr;
> +
> + writel_relaxed(value, addr + offset);
> +}
Same here.
> +
> +static inline u32 ave_rdesc(struct net_device *ndev, enum desc_id id,
> + int entry, int offset)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;
> +
> + addr += entry * priv->desc_size + offset;
> +
> + return ave_r32(ndev, addr);
> +}
> +
> +static inline void ave_wdesc(struct net_device *ndev, enum desc_id id,
> + int entry, int offset, u32 val)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;
> +
> + addr += entry * priv->desc_size + offset;
> +
> + ave_w32(ndev, addr, val);
> +}
> +
> +static void ave_wdesc_addr(struct net_device *ndev, enum desc_id id,
> + int entry, int offset, dma_addr_t paddr)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> +
> + ave_wdesc(ndev, id, entry, offset, (u32)((u64)paddr & 0xFFFFFFFFULL));
lower_32_bits()
> + if (IS_DESC_64BIT(priv))
> + ave_wdesc(ndev, id,
> + entry, offset + 4, (u32)((u64)paddr >> 32));
upper_32_bits()
> + else if ((u64)paddr > (u64)U32_MAX)
> + netdev_warn(ndev, "DMA address exceeds the address space\n");
> +}
> +
> +static void ave_get_fwversion(struct net_device *ndev, char *buf, int len)
> +{
> + u32 major, minor;
> +
> + major = (ave_r32(ndev, AVE_VR) & GENMASK(15, 8)) >> 8;
> + minor = (ave_r32(ndev, AVE_VR) & GENMASK(7, 0));
> + snprintf(buf, len, "v%u.%u", major, minor);
> +}
> +
> +static void ave_get_drvinfo(struct net_device *ndev,
> + struct ethtool_drvinfo *info)
> +{
> + struct device *dev = ndev->dev.parent;
> +
> + strlcpy(info->driver, dev->driver->name, sizeof(info->driver));
> + strlcpy(info->bus_info, dev_name(dev), sizeof(info->bus_info));
bus_info would likely be platform, since this is a memory mapped
peripheral, right?
> + ave_get_fwversion(ndev, info->fw_version, sizeof(info->fw_version));
> +}
> +
> +static int ave_nway_reset(struct net_device *ndev)
> +{
> + return genphy_restart_aneg(ndev->phydev);
> +}
You can just set your ethtool_ops::nway_reset to be
phy_ethtool_nway_reset() which does additional checks.
> +
> +static u32 ave_get_link(struct net_device *ndev)
> +{
> + int err;
> +
> + err = genphy_update_link(ndev->phydev);
> + if (err)
> + return ethtool_op_get_link(ndev);
No, calling genphy_update_link() is bogus see:
e69e46261063a25c3907bed16a2e9d18b115d1fd ("net: dsa: Do not clobber PHY
link outside of state machine")
> +
> + return ndev->phydev->link;
This can just be ethtool_op_get_link()
> +}
> +
> +static u32 ave_get_msglevel(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> +
> + return priv->msg_enable;
> +}
> +
> +static void ave_set_msglevel(struct net_device *ndev, u32 val)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> +
> + priv->msg_enable = val;
> +}
> +
> +static void ave_get_wol(struct net_device *ndev,
> + struct ethtool_wolinfo *wol)
> +{
> + wol->supported = 0;
> + wol->wolopts = 0;
> + phy_ethtool_get_wol(ndev->phydev, wol);
This can be called before you successfully connected to a PHY so you
need to check that ndev->phydev is not NULL at the very least.
> +}
> +
> +static int ave_set_wol(struct net_device *ndev,
> + struct ethtool_wolinfo *wol)
> +{
> + if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))
> + return -EOPNOTSUPP;
> +
> + return phy_ethtool_set_wol(ndev->phydev, wol);
Same here.
> +
> +static int ave_mdio_busywait(struct net_device *ndev)
> +{
> + int ret = 1, loop = 100;
> + u32 mdiosr;
> +
> + /* wait until completion */
> + while (1) {
Since you are already bound by the number of times you allow this look,
just make that a clear condition for the while() loop to exit.
> + mdiosr = ave_r32(ndev, AVE_MDIOSR);
> + if (!(mdiosr & AVE_MDIOSR_STS))
> + break;
> +
> + usleep_range(10, 20);
> + if (!loop--) {
> + netdev_err(ndev,
> + "failed to read from MDIO (status:0x%08x)\n",
> + mdiosr);
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int ave_mdiobus_read(struct mii_bus *bus, int phyid, int regnum)
> +{
> + struct net_device *ndev = bus->priv;
> + u32 mdioctl;
> +
> + /* write address */
> + ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);
> +
> + /* read request */
> + mdioctl = ave_r32(ndev, AVE_MDIOCTR);
> + ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_RREQ);
> +
> + if (!ave_mdio_busywait(ndev)) {
> + netdev_err(ndev, "phy-%d reg-%x read failed\n",
> + phyid, regnum);
> + return 0xffff;
> + }
> +
> + return ave_r32(ndev, AVE_MDIORDR) & GENMASK(15, 0);
> +}
> +
> +static int ave_mdiobus_write(struct mii_bus *bus,
> + int phyid, int regnum, u16 val)
> +{
> + struct net_device *ndev = bus->priv;
> + u32 mdioctl;
> +
> + /* write address */
> + ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);
> +
> + /* write data */
> + ave_w32(ndev, AVE_MDIOWDR, val);
> +
> + /* write request */
> + mdioctl = ave_r32(ndev, AVE_MDIOCTR);
> + ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);
> +
> + if (!ave_mdio_busywait(ndev)) {
> + netdev_err(ndev, "phy-%d reg-%x write failed\n",
> + phyid, regnum);
> + return -1;
> + }
Just propagate the return value of ave_mdio_busywait().
> +
> + return 0;
> +}
> +
> +static dma_addr_t ave_dma_map(struct net_device *ndev, struct ave_desc *desc,
> + void *ptr, size_t len,
> + enum dma_data_direction dir)
> +{
> + dma_addr_t paddr;
> +
> + paddr = dma_map_single(ndev->dev.parent, ptr, len, dir);
> + if (dma_mapping_error(ndev->dev.parent, paddr)) {
> + desc->skbs_dma = 0;
> + paddr = (dma_addr_t)virt_to_phys(ptr);
Why do you do that? If dma_map_single() failed, that's it, game over,
you need to discad the packet, not assume that it is in the kernel's
linear mapping!
> + } else {
> + desc->skbs_dma = paddr;
> + desc->skbs_dmalen = len;
> + }
> +
> + return paddr;
> +}
> +
> +static void ave_dma_unmap(struct net_device *ndev, struct ave_desc *desc,
> + enum dma_data_direction dir)
> +{
> + if (!desc->skbs_dma)
> + return;
> +
> + dma_unmap_single(ndev->dev.parent,
> + desc->skbs_dma, desc->skbs_dmalen, dir);
> + desc->skbs_dma = 0;
> +}
> +
> +/* Set Rx descriptor and memory */
> +static int ave_set_rxdesc(struct net_device *ndev, int entry)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + struct sk_buff *skb;
> + unsigned long align;
> + dma_addr_t paddr;
> + void *buffptr;
> + int ret = 0;
> +
> + skb = priv->rx.desc[entry].skbs;
> + if (!skb) {
> + skb = netdev_alloc_skb_ip_align(ndev,
> + AVE_MAX_ETHFRAME + NET_SKB_PAD);
> + if (!skb) {
> + netdev_err(ndev, "can't allocate skb for Rx\n");
> + return -ENOMEM;
> + }
> + }
> +
> + /* set disable to cmdsts */
> + ave_wdesc(ndev, AVE_DESCID_RX, entry, 0, AVE_STS_INTR | AVE_STS_OWN);
> +
> + /* align skb data for cache size */
> + align = (unsigned long)skb_tail_pointer(skb) & (NET_SKB_PAD - 1);
> + align = NET_SKB_PAD - align;
> + skb_reserve(skb, align);
> + buffptr = (void *)skb_tail_pointer(skb);
Are you positive you need this? Because by default, the networking stack
will align to the maximum between your L1 cache line size and 64 bytes,
which should be a pretty good alignment guarantee.
> +
> + paddr = ave_dma_map(ndev, &priv->rx.desc[entry], buffptr,
> + AVE_MAX_ETHFRAME + NET_IP_ALIGN, DMA_FROM_DEVICE);
This needs to be checked, as mentioned before, you can't just assume the
linear virtual map is going to be satisfactory.
> + priv->rx.desc[entry].skbs = skb;
> +
> + /* set buffer pointer */
> + ave_wdesc_addr(ndev, AVE_DESCID_RX, entry, 4, paddr);
OK, so 4 is an offset, can you add a define for that so it's clear it is
not an address size instead?
> +
> + /* set enable to cmdsts */
> + ave_wdesc(ndev, AVE_DESCID_RX,
> + entry, 0, AVE_STS_INTR | AVE_MAX_ETHFRAME);
> +
> + return ret;
> +}
> +
> +/* Switch state of descriptor */
> +static int ave_desc_switch(struct net_device *ndev, enum desc_state state)
> +{
> + int counter;
> + u32 val;
> +
> + switch (state) {
> + case AVE_DESC_START:
> + ave_w32(ndev, AVE_DESCC, AVE_DESCC_TD | AVE_DESCC_RD0);
> + break;
> +
> + case AVE_DESC_STOP:
> + ave_w32(ndev, AVE_DESCC, 0);
> + counter = 0;
> + while (1) {
> + usleep_range(100, 150);
> + if (!ave_r32(ndev, AVE_DESCC))
> + break;
> +
> + counter++;
> + if (counter > 100) {
> + netdev_err(ndev, "can't stop descriptor\n");
> + return -EBUSY;
> + }
> + }
Same here, pleas rewrite the loop so the exit condition is clear.
> + break;
> +
> + case AVE_DESC_RX_SUSPEND:
> + val = ave_r32(ndev, AVE_DESCC);
> + val |= AVE_DESCC_RDSTP;
> + val &= AVE_DESCC_CTRL_MASK;
Should not this be a &= ~AVE_DESCC_CTRL_MASK? OK maybe not.
> + ave_w32(ndev, AVE_DESCC, val);
> +
> + counter = 0;
> + while (1) {
> + usleep_range(100, 150);
> + val = ave_r32(ndev, AVE_DESCC);
> + if (val & (AVE_DESCC_RDSTP << 16))
> + break;
> +
> + counter++;
> + if (counter > 1000) {
> + netdev_err(ndev, "can't suspend descriptor\n");
> + return -EBUSY;
> + }
> + }
> + break;
Same here, please rewrite with the counter as an exit condition.
> +
> + case AVE_DESC_RX_PERMIT:
> + val = ave_r32(ndev, AVE_DESCC);
> + val &= ~AVE_DESCC_RDSTP;
> + val &= AVE_DESCC_CTRL_MASK;
> + ave_w32(ndev, AVE_DESCC, val);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ave_tx_completion(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 proc_idx, done_idx, ndesc, val;
> + int freebuf_flag = 0;
> +
> + proc_idx = priv->tx.proc_idx;
> + done_idx = priv->tx.done_idx;
> + ndesc = priv->tx.ndesc;
> +
> + /* free pre stored skb from done to proc */
> + while (proc_idx != done_idx) {
> + /* get cmdsts */
> + val = ave_rdesc(ndev, AVE_DESCID_TX, done_idx, 0);
> +
> + /* do nothing if owner is HW */
> + if (val & AVE_STS_OWN)
> + break;
> +
> + /* check Tx status and updates statistics */
> + if (val & AVE_STS_OK) {
> + priv->stats.tx_bytes += val & AVE_STS_PKTLEN_TX;
AVE_STS_PKTLEN_TX should be suffixed with _MASK to make it clear this is
a bitmask.
> +
> + /* success */
> + if (val & AVE_STS_LAST)
> + priv->stats.tx_packets++;
> + } else {
> + /* error */
> + if (val & AVE_STS_LAST) {
> + priv->stats.tx_errors++;
> + if (val & (AVE_STS_OWC | AVE_STS_EC))
> + priv->stats.collisions++;
> + }
> + }
> +
> + /* release skb */
> + if (priv->tx.desc[done_idx].skbs) {
> + ave_dma_unmap(ndev, &priv->tx.desc[done_idx],
> + DMA_TO_DEVICE);
> + dev_consume_skb_any(priv->tx.desc[done_idx].skbs);
Kudos for using dev_consume_skb_any()!
> + priv->tx.desc[done_idx].skbs = NULL;
> + freebuf_flag++;
> + }
> + done_idx = (done_idx + 1) % ndesc;
> + }
> +
> + priv->tx.done_idx = done_idx;
> +
> + /* wake queue for freeing buffer */
> + if (netif_queue_stopped(ndev))
> + if (freebuf_flag)
> + netif_wake_queue(ndev);
This can be simplified to:
if (netif_queue_stopped(ndev) && freebuf_flag)
netif_wake_queue(ndev)
because checking for netif_running() is implicit by actually getting
called here, this would not happen if the network interface was not
operational.
> +static irqreturn_t ave_interrupt(int irq, void *netdev)
> +{
> + struct net_device *ndev = (struct net_device *)netdev;
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 gimr_val, gisr_val;
> +
> + gimr_val = ave_irq_disable_all(ndev);
> +
> + /* get interrupt status */
> + gisr_val = ave_r32(ndev, AVE_GISR);
> +
> + /* PHY */
> + if (gisr_val & AVE_GI_PHY) {
> + ave_w32(ndev, AVE_GISR, AVE_GI_PHY);
> + if (priv->internal_phy_interrupt)
> + phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
This is not correct you should be telling the PHY the new link state. If
you cannot, because what happens here is really that the PHY interrupt
is routed to your MAC controller, then what I would suggest doing is the
following: create an interrupt controller domain which allows the PHY
driver to manage the interrupt line using normal request_irq().
> +static int ave_poll_tx(struct napi_struct *napi, int budget)
> +{
> + struct ave_private *priv;
> + struct net_device *ndev;
> + int num;
> +
> + priv = container_of(napi, struct ave_private, napi_tx);
> + ndev = priv->ndev;
> +
> + num = ave_tx_completion(ndev);
> + if (num < budget) {
TX reclamation should not be bound against the budget, always process
all packets that have been successfully submitted.
> + napi_complete(napi);
> +
> + /* enable Tx interrupt when NAPI finishes */
> + ave_irq_enable(ndev, AVE_GI_TX);
> + }
> +
> + return num;
> +}
> +
> +static void ave_adjust_link(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + struct phy_device *phydev = ndev->phydev;
> + u32 val, txcr, rxcr, rxcr_org;
> +
> + /* set RGMII speed */
> + val = ave_r32(ndev, AVE_TXCR);
> + val &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);
> +
> + if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&
> + phydev->speed == SPEED_1000)
> + val |= AVE_TXCR_TXSPD_1G;
Using phy_interface_is_rgmii() would be more robust here.
> + else if (phydev->speed == SPEED_100)
> + val |= AVE_TXCR_TXSPD_100;
> +
> + ave_w32(ndev, AVE_TXCR, val);
> +
> + /* set RMII speed (100M/10M only) */
> + if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
PHY_INTERFACE_MODE_MII, PHY_INTERFACE_MODE_REVMII,
PHY_INTERFACE_MODE_RMII would all qualify here presumably so you need
this check to be more restrictive to just the modes that you support.
> + val = ave_r32(ndev, AVE_LINKSEL);
> + if (phydev->speed == SPEED_10)
> + val &= ~AVE_LINKSEL_100M;
> + else
> + val |= AVE_LINKSEL_100M;
> + ave_w32(ndev, AVE_LINKSEL, val);
> + }
> +
> + /* check current RXCR/TXCR */
> + rxcr = ave_r32(ndev, AVE_RXCR);
> + txcr = ave_r32(ndev, AVE_TXCR);
> + rxcr_org = rxcr;
> +
> + if (phydev->duplex)
> + rxcr |= AVE_RXCR_FDUPEN;
> + else
> + rxcr &= ~AVE_RXCR_FDUPEN;
> +
> + if (phydev->pause) {
> + rxcr |= AVE_RXCR_FLOCTR;
> + txcr |= AVE_TXCR_FLOCTR;
You must also check for phydev->asym_pause and keep in mind that
phydev->pause and phydev->asym_pause reflect what the link partner
reflects, you need to implement .get_pauseparam and .set_pauseparam or
default to flow control ON (which is what you are doing here).
> + } else {
> + rxcr &= ~AVE_RXCR_FLOCTR;
> + txcr &= ~AVE_TXCR_FLOCTR;
> + }
> +
> + if (rxcr_org != rxcr) {
> + /* disable Rx mac */
> + ave_w32(ndev, AVE_RXCR, rxcr & ~AVE_RXCR_RXEN);
> + /* change and enable TX/Rx mac */
> + ave_w32(ndev, AVE_TXCR, txcr);
> + ave_w32(ndev, AVE_RXCR, rxcr);
> + }
> +
> + if (phydev->link)
> + netif_carrier_on(ndev);
> + else
> + netif_carrier_off(ndev);
This is not necessary, PHYLIB is specifically taking care of that.
> +
> + phy_print_status(phydev);
> +}
> +
> +static int ave_init(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + struct device *dev = ndev->dev.parent;
> + struct device_node *phy_node, *np = dev->of_node;
> + struct phy_device *phydev;
> + const void *mac_addr;
> + u32 supported;
> +
> + /* get mac address */
> + mac_addr = of_get_mac_address(np);
> + if (mac_addr)
> + ether_addr_copy(ndev->dev_addr, mac_addr);
> +
> + /* if the mac address is invalid, use random mac address */
> + if (!is_valid_ether_addr(ndev->dev_addr)) {
> + eth_hw_addr_random(ndev);
> + dev_warn(dev, "Using random MAC address: %pM\n",
> + ndev->dev_addr);
> + }
> +
> + /* attach PHY with MAC */
> + phy_node = of_get_next_available_child(np, NULL);
You should be using a "phy-handle" property to connect to a designated
PHY, not the next child DT node.
> + phydev = of_phy_connect(ndev, phy_node,
> + ave_adjust_link, 0, priv->phy_mode);
> + if (!phydev) {
> + dev_err(dev, "could not attach to PHY\n");
> + return -ENODEV;
> + }
> + of_node_put(phy_node);
> +
> + priv->phydev = phydev;
> + phydev->autoneg = AUTONEG_ENABLE;
> + phydev->speed = 0;
> + phydev->duplex = 0;
This is not necessary.
> +
> + dev_info(dev, "connected to %s phy with id 0x%x\n",
> + phydev->drv->name, phydev->phy_id);
> +
> + if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
> + supported = phydev->supported;
> + phydev->supported &= ~PHY_GBIT_FEATURES;
> + phydev->supported |= supported & PHY_BASIC_FEATURES;
One of these two statements is redundant here.
> + }
> +
> + /* PHY interrupt stop instruction is needed because the interrupt
> + * continues to assert.
> + */
> + phy_stop_interrupts(phydev);
Wait, what?
> +
> + /* When PHY driver can't handle its interrupt directly,
> + * interrupt request always fails and polling method is used
> + * alternatively. In this case, the libphy says
> + * "libphy: uniphier-mdio: Can't get IRQ -1 (PHY)".
> + */
> + phy_start_interrupts(phydev);
> +
> + phy_start_aneg(phydev);
No, no, phy_start() would take care of all of that correctly for you,
you don't have have to do it just there because ave_open() eventually
calls phy_start() for you, so just drop these two calls.
> +
> + return 0;
> +}
> +
> +static void ave_uninit(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> +
> + phy_stop_interrupts(priv->phydev);
You are missing a call to phy_stop() here, calling phy_stop_interrupts()
directly should not happen.
> + phy_disconnect(priv->phydev);
> +}
> +
> +static int ave_open(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + int entry;
> + u32 val;
> +
> + /* initialize Tx work */
> + priv->tx.proc_idx = 0;
> + priv->tx.done_idx = 0;
> + memset(priv->tx.desc, 0, sizeof(struct ave_desc) * priv->tx.ndesc);
> +
> + /* initialize Tx descriptor */
> + for (entry = 0; entry < priv->tx.ndesc; entry++) {
> + ave_wdesc(ndev, AVE_DESCID_TX, entry, 0, 0);
> + ave_wdesc_addr(ndev, AVE_DESCID_TX, entry, 4, 0);
> + }
> + ave_w32(ndev, AVE_TXDC, AVE_TXDC_ADDR_START
> + | (((priv->tx.ndesc * priv->desc_size) << 16) & AVE_TXDC_SIZE));
> +
> + /* initialize Rx work */
> + priv->rx.proc_idx = 0;
> + priv->rx.done_idx = 0;
> + memset(priv->rx.desc, 0, sizeof(struct ave_desc) * priv->rx.ndesc);
> +
> + /* initialize Rx descriptor and buffer */
> + for (entry = 0; entry < priv->rx.ndesc; entry++) {
> + if (ave_set_rxdesc(ndev, entry))
> + break;
> + }
> + ave_w32(ndev, AVE_RXDC0, AVE_RXDC0_ADDR_START
> + | (((priv->rx.ndesc * priv->desc_size) << 16) & AVE_RXDC0_SIZE));
> +
> + /* enable descriptor */
> + ave_desc_switch(ndev, AVE_DESC_START);
> +
> + /* initialize filter */
> + ave_pfsel_init(ndev);
> +
> + /* set macaddr */
> + val = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);
> + ave_w32(ndev, AVE_RXMAC1R, val);
> + val = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);
> + ave_w32(ndev, AVE_RXMAC2R, val);
> +
> + /* set Rx configuration */
> + /* full duplex, enable pause drop, enalbe flow control */
> + val = AVE_RXCR_RXEN | AVE_RXCR_FDUPEN | AVE_RXCR_DRPEN |
> + AVE_RXCR_FLOCTR | (AVE_MAX_ETHFRAME & AVE_RXCR_MPSIZ_MASK);
> + ave_w32(ndev, AVE_RXCR, val);
> +
> + /* set Tx configuration */
> + /* enable flow control, disable loopback */
> + ave_w32(ndev, AVE_TXCR, AVE_TXCR_FLOCTR);
> +
> + /* enable timer, clear EN,INTM, and mask interval unit(BSCK) */
> + val = ave_r32(ndev, AVE_IIRQC) & AVE_IIRQC_BSCK;
> + val |= AVE_IIRQC_EN0 | (AVE_INTM_COUNT << 16);
> + ave_w32(ndev, AVE_IIRQC, val);
> +
> + /* set interrupt mask */
> + val = AVE_GI_RXIINT | AVE_GI_RXOVF | AVE_GI_TX;
> + val |= (priv->internal_phy_interrupt) ? AVE_GI_PHY : 0;
> + ave_irq_restore(ndev, val);
> +
> + napi_enable(&priv->napi_rx);
> + napi_enable(&priv->napi_tx);
> +
> + phy_start(ndev->phydev);
> + netif_start_queue(ndev);
> +
> + return 0;
> +}
> +
> +static int ave_stop(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + int entry;
> +
> + /* disable all interrupt */
> + ave_irq_disable_all(ndev);
> + disable_irq(ndev->irq);
> +
> + netif_tx_disable(ndev);
> + phy_stop(ndev->phydev);
> + napi_disable(&priv->napi_tx);
> + napi_disable(&priv->napi_rx);
> +
> + /* free Tx buffer */
> + for (entry = 0; entry < priv->tx.ndesc; entry++) {
> + if (!priv->tx.desc[entry].skbs)
> + continue;
> +
> + ave_dma_unmap(ndev, &priv->tx.desc[entry], DMA_TO_DEVICE);
> + dev_kfree_skb_any(priv->tx.desc[entry].skbs);
> + priv->tx.desc[entry].skbs = NULL;
> + }
> + priv->tx.proc_idx = 0;
> + priv->tx.done_idx = 0;
> +
> + /* free Rx buffer */
> + for (entry = 0; entry < priv->rx.ndesc; entry++) {
> + if (!priv->rx.desc[entry].skbs)
> + continue;
> +
> + ave_dma_unmap(ndev, &priv->rx.desc[entry], DMA_FROM_DEVICE);
> + dev_kfree_skb_any(priv->rx.desc[entry].skbs);
> + priv->rx.desc[entry].skbs = NULL;
> + }
> + priv->rx.proc_idx = 0;
> + priv->rx.done_idx = 0;
> +
> + return 0;
> +}
> +
> +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 proc_idx, done_idx, ndesc, cmdsts;
> + int freepkt;
> + unsigned char *buffptr = NULL; /* buffptr for descriptor */
> + unsigned int len;
> + dma_addr_t paddr;
> +
> + proc_idx = priv->tx.proc_idx;
> + done_idx = priv->tx.done_idx;
> + ndesc = priv->tx.ndesc;
> + freepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;
> +
> + /* not enough entry, then we stop queue */
> + if (unlikely(freepkt < 2)) {
> + netif_stop_queue(ndev);
> + if (unlikely(freepkt < 1))
> + return NETDEV_TX_BUSY;
> + }
> +
> + priv->tx.desc[proc_idx].skbs = skb;
> +
> + /* add padding for short packet */
> + if (skb_padto(skb, ETH_ZLEN)) {
> + dev_kfree_skb_any(skb);
> + return NETDEV_TX_OK;
> + }
> +
> + buffptr = skb->data - NET_IP_ALIGN;
> + len = max_t(unsigned int, ETH_ZLEN, skb->len);
> +
> + paddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,
> + len + NET_IP_ALIGN, DMA_TO_DEVICE);
> + paddr += NET_IP_ALIGN;
> +
> + /* set buffer address to descriptor */
> + ave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);
> +
> + /* set flag and length to send */
> + cmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST
> + | (len & AVE_STS_PKTLEN_TX);
> +
> + /* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */
> + if (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))
> + cmdsts |= AVE_STS_INTR;
> +
> + /* disable checksum calculation when skb doesn't calurate checksum */
> + if (skb->ip_summed == CHECKSUM_NONE ||
> + skb->ip_summed == CHECKSUM_UNNECESSARY)
> + cmdsts |= AVE_STS_NOCSUM;
> +
> + /* set cmdsts */
> + ave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);
> +
> + priv->tx.proc_idx = (proc_idx + 1) % ndesc;
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int ave_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
> +{
> + return phy_mii_ioctl(ndev->phydev, ifr, cmd);
> +}
> +
> +static void ave_set_rx_mode(struct net_device *ndev)
> +{
> + int count, mc_cnt = netdev_mc_count(ndev);
> + struct netdev_hw_addr *hw_adr;
> + u32 val;
> + u8 v4multi_macadr[6] = { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00 };
> + u8 v6multi_macadr[6] = { 0x33, 0x00, 0x00, 0x00, 0x00, 0x00 };
> +
> + /* MAC addr filter enable for promiscious mode */
> + val = ave_r32(ndev, AVE_RXCR);
> + if (ndev->flags & IFF_PROMISC || !mc_cnt)
> + val &= ~AVE_RXCR_AFEN;
> + else
> + val |= AVE_RXCR_AFEN;
> + ave_w32(ndev, AVE_RXCR, val);
> +
> + /* set all multicast address */
> + if ((ndev->flags & IFF_ALLMULTI) || (mc_cnt > AVE_PF_MULTICAST_SIZE)) {
> + ave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST,
> + v4multi_macadr, 1);
> + ave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + 1,
> + v6multi_macadr, 1);
> + } else {
> + /* stop all multicast filter */
> + for (count = 0; count < AVE_PF_MULTICAST_SIZE; count++)
> + ave_pfsel_stop(ndev, AVE_PFNUM_MULTICAST + count);
> +
> + /* set multicast addresses */
> + count = 0;
> + netdev_for_each_mc_addr(hw_adr, ndev) {
> + if (count == mc_cnt)
> + break;
> + ave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + count,
> + hw_adr->addr, 6);
> + count++;
> + }
> + }
> +}
> +
> +static struct net_device_stats *ave_stats(struct net_device *ndev)
> +{
> + struct ave_private *priv = netdev_priv(ndev);
> + u32 drop_num = 0;
> +
> + priv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);
> +
> + drop_num += ave_r32(ndev, AVE_RX0OVFFC);
> + drop_num += ave_r32(ndev, AVE_SN5FC);
> + drop_num += ave_r32(ndev, AVE_SN6FC);
> + drop_num += ave_r32(ndev, AVE_SN7FC);
> + priv->stats.rx_dropped = drop_num;
> +
> + return &priv->stats;
> +}
> +
> +static int ave_set_mac_address(struct net_device *ndev, void *p)
> +{
> + int ret = eth_mac_addr(ndev, p);
> + u32 val;
> +
> + if (ret)
> + return ret;
> +
> + /* set macaddr */
> + val = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);
> + ave_w32(ndev, AVE_RXMAC1R, val);
> + val = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);
> + ave_w32(ndev, AVE_RXMAC2R, val);
> +
> + /* pfsel unicast entry */
> + ave_pfsel_macaddr_set(ndev, AVE_PFNUM_UNICAST, ndev->dev_addr, 6);
> +
> + return 0;
> +}
> +
> +static const struct net_device_ops ave_netdev_ops = {
> + .ndo_init = ave_init,
> + .ndo_uninit = ave_uninit,
> + .ndo_open = ave_open,
> + .ndo_stop = ave_stop,
> + .ndo_start_xmit = ave_start_xmit,
> + .ndo_do_ioctl = ave_ioctl,
> + .ndo_set_rx_mode = ave_set_rx_mode,
> + .ndo_get_stats = ave_stats,
> + .ndo_set_mac_address = ave_set_mac_address,
> +};
> +
> +static int ave_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + u32 desc_bits, ave_id;
> + struct reset_control *rst;
> + struct ave_private *priv;
> + phy_interface_t phy_mode;
> + struct net_device *ndev;
> + struct resource *res;
> + void __iomem *base;
> + int irq, ret = 0;
> + char buf[ETHTOOL_FWVERS_LEN];
> +
> + /* get phy mode */
> + phy_mode = of_get_phy_mode(np);
> + if (phy_mode < 0) {
> + dev_err(dev, "phy-mode not found\n");
> + return -EINVAL;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "IRQ not found\n");
> + return irq;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + /* alloc netdevice */
> + ndev = alloc_etherdev(sizeof(struct ave_private));
> + if (!ndev) {
> + dev_err(dev, "can't allocate ethernet device\n");
> + return -ENOMEM;
> + }
> +
> + ndev->base_addr = (unsigned long)base;
This is deprecated as mentioned earlier, just store this in
priv->base_adr or something.
> + ndev->irq = irq;
Same here.
> + ndev->netdev_ops = &ave_netdev_ops;
> + ndev->ethtool_ops = &ave_ethtool_ops;
> + SET_NETDEV_DEV(ndev, dev);
> +
> + /* support hardware checksum */
> + ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
> + ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
> +
> + ndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN);
> +
> + priv = netdev_priv(ndev);
> + priv->ndev = ndev;
> + priv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE);
> + priv->phy_mode = phy_mode;
> +
> + /* get bits of descriptor from devicetree */
> + of_property_read_u32(np, "socionext,desc-bits", &desc_bits);
> + priv->desc_size = AVE_DESC_SIZE_32;
> + if (desc_bits == 64)
> + priv->desc_size = AVE_DESC_SIZE_64;
> + else if (desc_bits != 32)
> + dev_warn(dev, "Defaulting to 32bit descriptor\n");
This should really be determined by the compatible string.
> +
> + /* use internal phy interrupt */
> + priv->internal_phy_interrupt =
> + of_property_read_bool(np, "socionext,internal-phy-interrupt");
Same here.
> +
> + /* setting private data for descriptor */
> + priv->tx.daddr = IS_DESC_64BIT(priv) ? AVE_TXDM_64 : AVE_TXDM_32;
> + priv->tx.ndesc = AVE_NR_TXDESC;
> + priv->tx.desc = devm_kzalloc(dev,
> + sizeof(struct ave_desc) * priv->tx.ndesc,
> + GFP_KERNEL);
> + if (!priv->tx.desc)
> + return -ENOMEM;
> +
> + priv->rx.daddr = IS_DESC_64BIT(priv) ? AVE_RXDM_64 : AVE_RXDM_32;
> + priv->rx.ndesc = AVE_NR_RXDESC;
> + priv->rx.desc = devm_kzalloc(dev,
> + sizeof(struct ave_desc) * priv->rx.ndesc,
> + GFP_KERNEL);
> + if (!priv->rx.desc)
> + return -ENOMEM;
If your network device driver is probed, but never used after that, that
is the network device is not open, you just this memory sitting for
nothing, you should consider moving that to ndo_open() time.
> +
> + /* enable clock */
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk))
> + priv->clk = NULL;
> + clk_prepare_enable(priv->clk);
Same here with the clock, the block is clocked, so it can consume some
amount of power, just do the necessary HW initialization with the clock
enabled, then defer until ndo_open() before turning it back on.
> +
> + /* reset block */
> + rst = devm_reset_control_get(dev, NULL);
> + if (!IS_ERR_OR_NULL(rst)) {
> + reset_control_deassert(rst);
> + reset_control_put(rst);
> + }
Ah so that's interesting, you need it clocked first, then reset, I guess
that works :)
> +
> + /* reset mac and phy */
> + ave_global_reset(ndev);
> +
> + /* request interrupt */
> + ret = devm_request_irq(dev, irq, ave_interrupt,
> + IRQF_SHARED, ndev->name, ndev);
> + if (ret)
> + goto err_req_irq;
Same here, this is usually moved to ndo_open() for network drivers, but
then remember not to use devm_, just normal request_irq() because it
need to be balanced in ndo_close().
This looks like a pretty good first submission, and there does not
appear to be any obvious functional problems!
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname
From: Cong Wang @ 2017-09-08 19:35 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, David Miller, Florian Westphal
In-Reply-To: <cfb5c05d158b278beb2f1441c67a068a8bb27d44.1504670009.git.lucien.xin@gmail.com>
On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
> Now there is no lock protecting nlk ngroups/groups' accessing in
> netlink bind and getname. It's safe from nlk groups' setting in
> netlink_release, but not from netlink_realloc_groups called by
> netlink_setsockopt.
>
> netlink_lock_table is needed in both netlink bind and getname when
> accessing nlk groups.
This looks very odd.
netlink_lock_table() should be protecting nl_table, why
it also protects nlk->groups?? For me it looks like you
need lock_sock() instead.
^ permalink raw reply
* [PATCH net] tcp: fix a request socket leak
From: Eric Dumazet @ 2017-09-08 19:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
While the cited commit fixed a possible deadlock, it added a leak
of the request socket, since reqsk_put() must be called if the BPF
filter decided the ACK packet must be dropped.
Fixes: d624d276d1dd ("tcp: fix possible deadlock in TCP stack vs BPF filter")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
David, this is a stable candidate (linux-4.13 has this bug)
net/ipv4/tcp_ipv4.c | 6 +++---
net/ipv6/tcp_ipv6.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a63486afa7a7e7b4dce88b65bc27cfa872a3ba2f..d9416b5162bc1bdd1acd34fcb4da21cb6b62d0ae 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1669,9 +1669,9 @@ int tcp_v4_rcv(struct sk_buff *skb)
*/
sock_hold(sk);
refcounted = true;
- if (tcp_filter(sk, skb))
- goto discard_and_relse;
- nsk = tcp_check_req(sk, skb, req, false);
+ nsk = NULL;
+ if (!tcp_filter(sk, skb))
+ nsk = tcp_check_req(sk, skb, req, false);
if (!nsk) {
reqsk_put(req);
goto discard_and_relse;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 38f76d8b231e3e5923ad72d05b8d320b6aeb850f..64d94afa427f81fd7a505b1cfd1c0be66c273810 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1460,9 +1460,9 @@ static int tcp_v6_rcv(struct sk_buff *skb)
}
sock_hold(sk);
refcounted = true;
- if (tcp_filter(sk, skb))
- goto discard_and_relse;
- nsk = tcp_check_req(sk, skb, req, false);
+ nsk = NULL;
+ if (!tcp_filter(sk, skb))
+ nsk = tcp_check_req(sk, skb, req, false);
if (!nsk) {
reqsk_put(req);
goto discard_and_relse;
^ permalink raw reply related
* Re: [PATCH net] net: qualcomm: rmnet: Fix a double free
From: Subash Abhinov Kasiviswanathan @ 2017-09-08 19:44 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S. Miller, netdev, kernel-janitors
In-Reply-To: <20170908102356.tzysh6qaiesd2umz@mwanda>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 557c9bf1a469..0335fce54201 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -95,10 +95,8 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff
> *skb)
> skb_pull(skb, packet_len);
>
> /* Some hardware can send us empty frames. Catch them */
> - if (ntohs(maph->pkt_len) == 0) {
> - kfree_skb(skb);
> + if (ntohs(maph->pkt_len) == 0)
> return NULL;
> - }
>
> return skbn;
> }
Thanks for the patch. This is fixing the double free, but is leaking the
new skb skbn
created. Perhaps we should add the check earlier -
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 557c9bf..86b8c75 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -84,6 +84,10 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff
*skb)
if (((int)skb->len - (int)packet_len) < 0)
return NULL;
+ /* Some hardware can send us empty frames. Catch them */
+ if (ntohs(maph->pkt_len) == 0)
+ return NULL;
+
skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING,
GFP_ATOMIC);
if (!skbn)
return NULL;
@@ -94,11 +98,5 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff
*skb)
memcpy(skbn->data, skb->data, packet_len);
skb_pull(skb, packet_len);
- /* Some hardware can send us empty frames. Catch them */
- if (ntohs(maph->pkt_len) == 0) {
- kfree_skb(skb);
- return NULL;
- }
-
return skbn;
}
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply related
* RE: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Tristram.Ha @ 2017-09-08 19:48 UTC (permalink / raw)
To: muvarov
Cc: andrew, pavel, nathan.leigh.conrad, vivien.didelot, netdev,
linux-kernel, Woojung.Huh, f.fainelli
In-Reply-To: <CAJGZr0Kp5ZKmFJT+KX9jxMVyZT9=VO0_dpZqE+y1TNaYvURcoQ@mail.gmail.com>
> -----Original Message-----
> From: Maxim Uvarov [mailto:muvarov@gmail.com]
> Sent: Friday, September 08, 2017 12:00 PM
> To: Florian Fainelli
> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
>
> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> > On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> >> From: Tristram Ha <Tristram.Ha@microchip.com>
> >>
> >> Add other KSZ switches support so that patch check does not complain.
> >>
> >> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> >> ---
> >> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
> >> ++++++++++++----------
> >> 1 file changed, 62 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> index 0ab8b39..34af0e0 100644
> >> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
> >>
> >> Required properties:
> >>
> >> -- compatible: For external switch chips, compatible string must be
> >> exactly one
> >> - of: "microchip,ksz9477"
> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >> + "microchip,ksz8795" for KSZ8795 chip,
> >> + "microchip,ksz8794" for KSZ8794 chip,
> >> + "microchip,ksz8765" for KSZ8765 chip,
> >> + "microchip,ksz8895" for KSZ8895 chip,
> >> + "microchip,ksz8864" for KSZ8864 chip,
> >> + "microchip,ksz8873" for KSZ8873 chip,
> >> + "microchip,ksz8863" for KSZ8863 chip,
> >> + "microchip,ksz8463" for KSZ8463 chip
> >
>
>
> Tristram, does any of this devices support chaining?
>
> Maxim.
They do not if you mean daisy-chaining the switches together.
There is always a problem that once tail tagging mode is enabled
sending a frame through the MAC without going through the DSA
layer will cause the frame to be dropped.
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Subash Abhinov Kasiviswanathan @ 2017-09-08 19:50 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Eric Dumazet, Lorenzo Colitti
In-Reply-To: <CAM_iQpXKfUkbyuuZEZXyPrVVCoPaxnCmCARz=Jf0VtU1m9NsZg@mail.gmail.com>
On 2017-09-08 10:10, Cong Wang wrote:
> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org> wrote:
>> We are seeing a possible use after free in ip6_dst_destroy.
>>
>> It appears as if memory of the __DST_METRICS_PTR(old) was freed in
>> some path
>> and allocated
>> to ion driver. ion driver has also freed it. Finally the memory is
>> freed by
>> the
>> fib gc and crashes since it is already deallocated.
>
> Does the attach (compile-only) patch help anything?
>
> From my _quick_ glance, it seems we miss the refcnt'ing
> right in __dst_destroy_metrics_generic().
>
> Thanks!
Hi Cong
Thanks for patch. I'll try this out.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Florian Fainelli @ 2017-09-08 19:54 UTC (permalink / raw)
To: Tristram.Ha, muvarov
Cc: andrew, pavel, nathan.leigh.conrad, vivien.didelot, netdev,
linux-kernel, Woojung.Huh
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121EF7@CHN-SV-EXMX02.mchp-main.com>
On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Maxim Uvarov [mailto:muvarov@gmail.com]
>> Sent: Friday, September 08, 2017 12:00 PM
>> To: Florian Fainelli
>> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
>> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
>>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>>>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>>>
>>>> Add other KSZ switches support so that patch check does not complain.
>>>>
>>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>>>> ---
>>>> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
>>>> ++++++++++++----------
>>>> 1 file changed, 62 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> index 0ab8b39..34af0e0 100644
>>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>>>
>>>> Required properties:
>>>>
>>>> -- compatible: For external switch chips, compatible string must be
>>>> exactly one
>>>> - of: "microchip,ksz9477"
>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>> + "microchip,ksz8795" for KSZ8795 chip,
>>>> + "microchip,ksz8794" for KSZ8794 chip,
>>>> + "microchip,ksz8765" for KSZ8765 chip,
>>>> + "microchip,ksz8895" for KSZ8895 chip,
>>>> + "microchip,ksz8864" for KSZ8864 chip,
>>>> + "microchip,ksz8873" for KSZ8873 chip,
>>>> + "microchip,ksz8863" for KSZ8863 chip,
>>>> + "microchip,ksz8463" for KSZ8463 chip
>>>
>>
>>
>> Tristram, does any of this devices support chaining?
>>
>> Maxim.
>
> They do not if you mean daisy-chaining the switches together.
Marvell tags allow you to specify both a port and switch index
destination after setting up an appropriate routing table, I am assuming
this is not supported.
What happens though if I connect two KSZ switches ones to another say:
eth0
-> KSZ8463
-> KSZ8463
Will the first switch terminates KSZ tag if it sees two tags
encapsulated in another, something like this:
| MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |
>
> There is always a problem that once tail tagging mode is enabled
> sending a frame through the MAC without going through the DSA
> layer will cause the frame to be dropped.
Yes, once the master network device is used for DSA, it is still usable
directly by e.g: applications, but it won't do anything if the switch is
configured such that it drops ingressing frames not having the proper
tag. We documented that here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/dsa/dsa.txt#n275
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check
From: Florian Fainelli @ 2017-09-08 19:56 UTC (permalink / raw)
To: Woojung.Huh, linus.walleij
Cc: andrew, festevam, sergei.shtylyov, davem, netdev, fabio.estevam
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40B1B47F@CHN-SV-EXMX02.mchp-main.com>
On 09/07/2017 02:51 PM, Woojung.Huh@microchip.com wrote:
>>>>> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
>>>>> gpiod_get() and friends, this is very likely to be a bug, and what
>>>>> the driver wants to do is:
>>>>>
>>>>> depends on GPIOLIB
>>>>>
>>>>> or
>>>>>
>>>>> select GPIOLIB
>>>>>
>>>>> in Kconfig. The whole optional thing is mainly a leftover from when it
>>>>> was possible to have a local implementation of the GPIOLIB API in
>>>>> some custom header file, noone sane should be doing that anymore,
>>>>> and if they do, they can very well face the warnings.
>>>>>
>>>>> If someone is facing a lot of WARN_ON() messages to this, it is a clear
>>>>> indication that they need to fix their Kconfig and in that case it is proper.
>>>> Linus & Andrew,
>>>>
>>>> I knew that it is already in David's pulling request.
>>>> Configuring GPIOLIB is the right solution even if platform doesn't use it?
>>>
>>> I guess?
>>>
>>> "Platform doesn't use it" what does that mean?
>>>
>>> Does it mean it does not call the
>>> APIs of the GPIOLIB, does it mean it doesn't have a GPIO driver
>>> at probe (but may have one by having it probed from a module)
>>> or does it mean the platform can never have it?
>>
>> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
>> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
>> calls into GPIOLIB and attempts to configure a given GPIO if available.
> Yes. I'm facing issue on PC which won't need GPIOLIB as default.
> Warning message goes away when GPIOLIB is enabled, and fortunately,
> Ubuntu default config has it.
> So, it may not be seen by many users when with full/default configuration.
Woojung, I suppose you are also getting a warning from
gpiod_set_value_cansleep() done in mdiobus_unregister() right? With
CONFIG_GPIOLIB=n devm_gpiod_get_optional() returns NULL, which we don't
check as an error, on purpose however we still call
gpiod_set_value_cansleep() on a NULL GPIO descriptor, so the following
should do:
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index b6f9fa670168..67dbb7c26840 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -424,7 +424,8 @@ void mdiobus_unregister(struct mii_bus *bus)
}
/* Put PHYs in RESET to save power */
- gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+ if (!IS_ERR_OR_NULL(bus->reset_gpiod))
+ gpiod_set_value_cansleep(bus->reset_gpiod, 1);
device_del(&bus->dev);
}
>
>> This thread is actually what prompted me to write this email:
>>
>> https://lkml.org/lkml/2017/9/2/3
> Thanks for the link.
>
>
--
Florian
^ permalink raw reply related
* RE: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check
From: Woojung.Huh @ 2017-09-08 20:03 UTC (permalink / raw)
To: f.fainelli, linus.walleij
Cc: andrew, festevam, sergei.shtylyov, davem, netdev, fabio.estevam,
Bryan.Whitehead
In-Reply-To: <c960202c-02e3-3f54-7d0b-b5ece46c6a9e@gmail.com>
> >> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
> >> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
> >> calls into GPIOLIB and attempts to configure a given GPIO if available.
> > Yes. I'm facing issue on PC which won't need GPIOLIB as default.
> > Warning message goes away when GPIOLIB is enabled, and fortunately,
> > Ubuntu default config has it.
> > So, it may not be seen by many users when with full/default configuration.
>
> Woojung, I suppose you are also getting a warning from
> gpiod_set_value_cansleep() done in mdiobus_unregister() right? With
> CONFIG_GPIOLIB=n devm_gpiod_get_optional() returns NULL, which we
> don't
> check as an error, on purpose however we still call
> gpiod_set_value_cansleep() on a NULL GPIO descriptor, so the following
> should do:
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index b6f9fa670168..67dbb7c26840 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -424,7 +424,8 @@ void mdiobus_unregister(struct mii_bus *bus)
> }
>
> /* Put PHYs in RESET to save power */
> - gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> + if (!IS_ERR_OR_NULL(bus->reset_gpiod))
> + gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>
> device_del(&bus->dev);
> }
Hi Florian,
Thanks for the patch. I'm avoiding warning with CONFIG_GPIOLIB=y for now.
I'm curious that there is a final conclusion to resolve this issue,
either with CONFIG_GPIOLIB=y or something else.
- Woojung
^ permalink raw reply
* RE: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Tristram.Ha @ 2017-09-08 20:07 UTC (permalink / raw)
To: f.fainelli
Cc: andrew, pavel, nathan.leigh.conrad, vivien.didelot, netdev,
linux-kernel, Woojung.Huh, muvarov
In-Reply-To: <508d7ef0-e351-5369-b477-b027bae3dda4@gmail.com>
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Friday, September 08, 2017 12:54 PM
> To: Tristram Ha - C24268; muvarov@gmail.com
> Cc: andrew@lunn.ch; pavel@ucw.cz; nathan.leigh.conrad@gmail.com;
> vivien.didelot@savoirfairelinux.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
>
> On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
> >> -----Original Message-----
> >> From: Maxim Uvarov [mailto:muvarov@gmail.com]
> >> Sent: Friday, September 08, 2017 12:00 PM
> >> To: Florian Fainelli
> >> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
> >> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> >> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that
> new
> >> drivers can be added
> >>
> >> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> >>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> >>>> From: Tristram Ha <Tristram.Ha@microchip.com>
> >>>>
> >>>> Add other KSZ switches support so that patch check does not complain.
> >>>>
> >>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> >>>> ---
> >>>> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
> >>>> ++++++++++++----------
> >>>> 1 file changed, 62 insertions(+), 55 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> index 0ab8b39..34af0e0 100644
> >>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
> >>>>
> >>>> Required properties:
> >>>>
> >>>> -- compatible: For external switch chips, compatible string must be
> >>>> exactly one
> >>>> - of: "microchip,ksz9477"
> >>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >>>> + "microchip,ksz8795" for KSZ8795 chip,
> >>>> + "microchip,ksz8794" for KSZ8794 chip,
> >>>> + "microchip,ksz8765" for KSZ8765 chip,
> >>>> + "microchip,ksz8895" for KSZ8895 chip,
> >>>> + "microchip,ksz8864" for KSZ8864 chip,
> >>>> + "microchip,ksz8873" for KSZ8873 chip,
> >>>> + "microchip,ksz8863" for KSZ8863 chip,
> >>>> + "microchip,ksz8463" for KSZ8463 chip
> >>>
> >>
> >>
> >> Tristram, does any of this devices support chaining?
> >>
> >> Maxim.
> >
> > They do not if you mean daisy-chaining the switches together.
>
> Marvell tags allow you to specify both a port and switch index
> destination after setting up an appropriate routing table, I am assuming
> this is not supported.
>
> What happens though if I connect two KSZ switches ones to another say:
>
> eth0
> -> KSZ8463
> -> KSZ8463
>
> Will the first switch terminates KSZ tag if it sees two tags
> encapsulated in another, something like this:
>
> | MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |
>
In theory it is doable by adding more tags and remember which port
is connected to the cpu port of another switch, but there is no switch
forwarding and everything is handled by software.
> >
> > There is always a problem that once tail tagging mode is enabled
> > sending a frame through the MAC without going through the DSA
> > layer will cause the frame to be dropped.
>
> Yes, once the master network device is used for DSA, it is still usable
> directly by e.g: applications, but it won't do anything if the switch is
> configured such that it drops ingressing frames not having the proper
> tag. We documented that here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/networking/dsa/dsa.txt#n275
> --
As the DSA was developed for the Marvell switches I assumed they can
forward frame without a tag.
^ permalink raw reply
* Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
From: Florian Fainelli @ 2017-09-08 20:58 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, pavel, nathan.leigh.conrad, vivien.didelot, netdev,
linux-kernel, Woojung.Huh, muvarov
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121F35@CHN-SV-EXMX02.mchp-main.com>
On 09/08/2017 01:07 PM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>> Sent: Friday, September 08, 2017 12:54 PM
>> To: Tristram Ha - C24268; muvarov@gmail.com
>> Cc: andrew@lunn.ch; pavel@ucw.cz; nathan.leigh.conrad@gmail.com;
>> vivien.didelot@savoirfairelinux.com; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
>>>> -----Original Message-----
>>>> From: Maxim Uvarov [mailto:muvarov@gmail.com]
>>>> Sent: Friday, September 08, 2017 12:00 PM
>>>> To: Florian Fainelli
>>>> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
>>>> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>>>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that
>> new
>>>> drivers can be added
>>>>
>>>> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
>>>>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>>>>>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>>>>>
>>>>>> Add other KSZ switches support so that patch check does not complain.
>>>>>>
>>>>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>>>>>> ---
>>>>>> Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
>>>>>> ++++++++++++----------
>>>>>> 1 file changed, 62 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> index 0ab8b39..34af0e0 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>>>>>
>>>>>> Required properties:
>>>>>>
>>>>>> -- compatible: For external switch chips, compatible string must be
>>>>>> exactly one
>>>>>> - of: "microchip,ksz9477"
>>>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>>>> + "microchip,ksz8795" for KSZ8795 chip,
>>>>>> + "microchip,ksz8794" for KSZ8794 chip,
>>>>>> + "microchip,ksz8765" for KSZ8765 chip,
>>>>>> + "microchip,ksz8895" for KSZ8895 chip,
>>>>>> + "microchip,ksz8864" for KSZ8864 chip,
>>>>>> + "microchip,ksz8873" for KSZ8873 chip,
>>>>>> + "microchip,ksz8863" for KSZ8863 chip,
>>>>>> + "microchip,ksz8463" for KSZ8463 chip
>>>>>
>>>>
>>>>
>>>> Tristram, does any of this devices support chaining?
>>>>
>>>> Maxim.
>>>
>>> They do not if you mean daisy-chaining the switches together.
>>
>> Marvell tags allow you to specify both a port and switch index
>> destination after setting up an appropriate routing table, I am assuming
>> this is not supported.
>>
>> What happens though if I connect two KSZ switches ones to another say:
>>
>> eth0
>> -> KSZ8463
>> -> KSZ8463
>>
>> Will the first switch terminates KSZ tag if it sees two tags
>> encapsulated in another, something like this:
>>
>> | MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |
>>
>
> In theory it is doable by adding more tags and remember which port
> is connected to the cpu port of another switch, but there is no switch
> forwarding and everything is handled by software.
Fair enough.
>
>>>
>>> There is always a problem that once tail tagging mode is enabled
>>> sending a frame through the MAC without going through the DSA
>>> layer will cause the frame to be dropped.
>>
>> Yes, once the master network device is used for DSA, it is still usable
>> directly by e.g: applications, but it won't do anything if the switch is
>> configured such that it drops ingressing frames not having the proper
>> tag. We documented that here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
>> ntation/networking/dsa/dsa.txt#n275
>> --
>
> As the DSA was developed for the Marvell switches I assumed they can
> forward frame without a tag.
Once you use tags with your switch, eth0/master netdev/conduit interface
no longer has a purpose as a normal interface because we create per-port
network devices. That means if you want to send packets towards Port 0
you use the proper network interface. If you create a bridge, you will
use brX as the network device to send/receive packets from, in all
cases, the packets originate from the CPU and the frame ingresses the
switch with the proper information within the tag to target the vector
of ports.
If you have tags enabled and you use eth0 to send packets towards the
switch, there are not that many options, either you have the proper tag,
and the switch will forward to the right port which can be the CPU port
itself if you want, but why do that?
When tags are not enabled (e.g: b53) that is slightly different,
eth0/master/conduit remains usable as a normal interface would, but
unless you start adding VLAN tags, you cannot quite differentiate where
the traffic from LAN to CPU is coming from, so this has limited usefulness.
Hope this clears things.
--
Florian
^ permalink raw reply
* [net PATCH 0/3] Fixes for XDP/BPF
From: John Fastabend @ 2017-09-08 21:00 UTC (permalink / raw)
To: davem; +Cc: netdev, john.fastabend, daniel, ast
The following fixes, UAPI updates, and small improvement,
i. XDP needs to be called inside RCU with preempt disabled.
ii. Not strictly a bug fix but we have an attach command in the
sockmap UAPI already to avoid having a single kernel released with
only the attach and not the detach I'm pushing this into net branch.
Its early in the RC cycle so I think this is OK (not ideal but better
than supporting a UAPI with a missing detach forever).
iii. Final patch replace cpu_relax with cond_resched in devmap.
---
John Fastabend (3):
net: rcu lock and preempt disable missing around generic xdp
bpf: add support for sockmap detach programs
bpf: devmap, use cond_resched instead of cpu_relax
include/linux/bpf.h | 8 ++---
kernel/bpf/devmap.c | 2 +
kernel/bpf/sockmap.c | 2 +
kernel/bpf/syscall.c | 27 ++++++++++------
net/core/dev.c | 25 ++++++++++-----
tools/testing/selftests/bpf/test_maps.c | 51 ++++++++++++++++++++++++++++++-
6 files changed, 89 insertions(+), 26 deletions(-)
^ permalink raw reply
* [net PATCH 1/3] net: rcu lock and preempt disable missing around generic xdp
From: John Fastabend @ 2017-09-08 21:00 UTC (permalink / raw)
To: davem; +Cc: netdev, john.fastabend, daniel, ast
In-Reply-To: <150490397545.11590.1409723973253492363.stgit@john-XPS-13-9360>
do_xdp_generic must be called inside rcu critical section with preempt
disabled to ensure BPF programs are valid and per-cpu variables used
for redirect operations are consistent. This patch ensures this is true
and fixes the splat below.
The netif_receive_skb_internal() code path is now broken into two rcu
critical sections. I decided it was better to limit the preempt_enable/disable
block to just the xdp static key portion and the fallout is more
rcu_read_lock/unlock calls. Seems like the best option to me.
[ 607.596901] =============================
[ 607.596906] WARNING: suspicious RCU usage
[ 607.596912] 4.13.0-rc4+ #570 Not tainted
[ 607.596917] -----------------------------
[ 607.596923] net/core/dev.c:3948 suspicious rcu_dereference_check() usage!
[ 607.596927]
[ 607.596927] other info that might help us debug this:
[ 607.596927]
[ 607.596933]
[ 607.596933] rcu_scheduler_active = 2, debug_locks = 1
[ 607.596938] 2 locks held by pool/14624:
[ 607.596943] #0: (rcu_read_lock_bh){......}, at: [<ffffffff95445ffd>] ip_finish_output2+0x14d/0x890
[ 607.596973] #1: (rcu_read_lock_bh){......}, at: [<ffffffff953c8e3a>] __dev_queue_xmit+0x14a/0xfd0
[ 607.597000]
[ 607.597000] stack backtrace:
[ 607.597006] CPU: 5 PID: 14624 Comm: pool Not tainted 4.13.0-rc4+ #570
[ 607.597011] Hardware name: Dell Inc. Precision Tower 5810/0HHV7N, BIOS A17 03/01/2017
[ 607.597016] Call Trace:
[ 607.597027] dump_stack+0x67/0x92
[ 607.597040] lockdep_rcu_suspicious+0xdd/0x110
[ 607.597054] do_xdp_generic+0x313/0xa50
[ 607.597068] ? time_hardirqs_on+0x5b/0x150
[ 607.597076] ? mark_held_locks+0x6b/0xc0
[ 607.597088] ? netdev_pick_tx+0x150/0x150
[ 607.597117] netif_rx_internal+0x205/0x3f0
[ 607.597127] ? do_xdp_generic+0xa50/0xa50
[ 607.597144] ? lock_downgrade+0x2b0/0x2b0
[ 607.597158] ? __lock_is_held+0x93/0x100
[ 607.597187] netif_rx+0x119/0x190
[ 607.597202] loopback_xmit+0xfd/0x1b0
[ 607.597214] dev_hard_start_xmit+0x127/0x4e0
Fixes: d445516966dc ("net: xdp: support xdp generic on virtual devices")
Fixes: b5cdae3291f7 ("net: Generic XDP")
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
net/core/dev.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 6f845e4..fb766d9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3981,8 +3981,13 @@ static int netif_rx_internal(struct sk_buff *skb)
trace_netif_rx(skb);
if (static_key_false(&generic_xdp_needed)) {
- int ret = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
- skb);
+ int ret;
+
+ preempt_disable();
+ rcu_read_lock();
+ ret = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog), skb);
+ rcu_read_unlock();
+ preempt_enable();
/* Consider XDP consuming the packet a success from
* the netdev point of view we do not want to count
@@ -4500,18 +4505,20 @@ static int netif_receive_skb_internal(struct sk_buff *skb)
if (skb_defer_rx_timestamp(skb))
return NET_RX_SUCCESS;
- rcu_read_lock();
-
if (static_key_false(&generic_xdp_needed)) {
- int ret = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
- skb);
+ int ret;
- if (ret != XDP_PASS) {
- rcu_read_unlock();
+ preempt_disable();
+ rcu_read_lock();
+ ret = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog), skb);
+ rcu_read_unlock();
+ preempt_enable();
+
+ if (ret != XDP_PASS)
return NET_RX_DROP;
- }
}
+ rcu_read_lock();
#ifdef CONFIG_RPS
if (static_key_false(&rps_needed)) {
struct rps_dev_flow voidflow, *rflow = &voidflow;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox