* Re: [PATCH v3 1/1] r8169: Coalesce RTL8411b PHY power-down recovery programming instructions to reduce spinlock stalls
From: Mirsad Todorovac @ 2023-10-31 13:27 UTC (permalink / raw)
To: Andrew Lunn, Mirsad Todorovac
Cc: Heiner Kallweit, netdev, linux-kernel, nic_swsd, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Elver
In-Reply-To: <7fa02dd1-c894-4980-8439-4dc1e22d3634@lunn.ch>
On 10/31/2023 1:38 PM, Andrew Lunn wrote:
> On Tue, Oct 31, 2023 at 04:35:19AM +0100, Mirsad Todorovac wrote:
>> On 10/31/23 02:21, Andrew Lunn wrote:
>>>> I will not contradict, but the cummulative amount of memory barriers on each MMIO read/write
>>>> in each single one of the drivers could amount to some degrading of overall performance and
>>>> latency in a multicore system.
>>>
>>> For optimisations, we like to see benchmark results which show some
>>> improvements. Do you have any numbers?
>>
>> Hi, Andrew,
>>
>> Thank you for your interest in RTL NIC driver optimisations.
>>
>> My knowledge about the timing costs of synchronisation is mostly theoretical.
>
> The kernel tends to be very practical. Maybe try to turn the
> theoretical knowledge into practice. Write a benchmark test, or see if
> any of the existing RT Linux tests show there is a real problem here,
> and your changes fix it.
I stand corrected. Real benchmarks would indeed say more than the visual
inspection of the code.
As I see, as a maintainer of the PHYLIB you certainly have a greater
insight. What I've done is maybe too aggressive "loophole optimisation",
without much consideration of what Mr. Heiner Kallweit addressed as hot
paths and not so hot (less trodden) paths.
Best regards,
Mirsad Todorovac
^ permalink raw reply
* Re: [PATCH v2] selftests/net: synchronize udpgso_bench rx and tx
From: Lucas Karpinski @ 2023-10-31 13:26 UTC (permalink / raw)
To: Paolo Abeni
Cc: davem, edumazet, kuba, shuah, netdev, linux-kselftest,
linux-kernel
In-Reply-To: <e8a55d0518da5c1f9aba739359150cad58c03b2b.camel@redhat.com>
> Since you wrote the same function verbatim in 3 different files, I
> think it would be better place it in separate, new, net_helper.sh file
> and include such file from the various callers. Possibly additionally
> rename the function as wait_local_udp_port_listen.
>
Thanks, I'll move it over. I think it would be best though to leave udp
out of the name and to just pass the protocol as an argument. That way
any future tcp tests can also take advantage of it.
Lucas
^ permalink raw reply
* Re: [PATCH net-next v2 4/9] dt-bindings: net: add OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface
From: Parthiban.Veerasooran @ 2023-10-31 12:59 UTC (permalink / raw)
To: krzysztof.kozlowski
Cc: netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr,
davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
andrew
In-Reply-To: <478c0e7e-bdff-41af-a12f-f9930f1c665a@linaro.org>
Hi Krzysztof
As per the comments in the other email of this patch, this DT bindings
are going to be removed.
Best Regards,
Parthiban V
On 24/10/23 1:14 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 23/10/2023 17:46, Parthiban Veerasooran wrote:
>> Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
>> parameters. These are generic properties that can apply to any 10BASE-T1x
>> MAC-PHY which uses OPEN Alliance TC6 specification.
>
> Except that it was not tested at all few more issues.
>
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>> .../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 73 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/oa-tc6.yaml b/Documentation/devicetree/bindings/net/oa-tc6.yaml
>> new file mode 100644
>> index 000000000000..9f442fa6cace
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/oa-tc6.yaml
>
> Filename based on compatible.
>
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/oa-tc6.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: OPEN Alliance 10BASE-T1x MAC-PHY Specification Common Properties
>> +
>> +maintainers:
>> + - Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
>> +
>> +description:
>> + These are generic properties that can apply to any 10BASE-T1x MAC-PHY
>> + which uses OPEN Alliance TC6 specification.
>> +
>> + 10BASE-T1x MAC-PHY Serial Interface Specification can be found at:
>> + https://opensig.org/about/specifications/
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^oa-tc6(@.*)?"
>
> Drop
>
>> +
>> + "#address-cells":
>> + const: 1
>> +
>> + "#size-cells":
>> + const: 0
>
> Why?
>
>> +
>> + oa-cps:
>> + maxItems: 1
>> + description:
>> + Chunk Payload Size. Configures the data chunk payload size to 2^N,
>> + where N is the value of this bitfield. The minimum possible data
>> + chunk payload size is 8 bytes or N = 3. The default data chunk
>> + payload size is 64 bytes, or N = 6. The minimum supported data chunk
>> + payload size for this MAC-PHY device is indicated in the CPSMIN
>> + field of the CAPABILITY register. Valid values for this parameter
>> + are 8, 16, 32 and 64. All other values are reserved.
>> +
>> + oa-txcte:
>> + maxItems: 1
>> + description:
>> + Transmit Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the SPI host to the network.
>> +
>> + oa-rxcte:
>> + maxItems: 1
>> + description:
>> + Receive Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the network to the SPI host.
>> +
>> + oa-prote:
>> + maxItems: 1
>> + description:
>> + Control data read/write Protection Enable. When set, all control
>> + data written to and read from the MAC-PHY will be transferred with
>> + its complement for detection of bit errors.
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> + - |
>> + oa-tc6 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
> That's some total mess in indentation.
>
>> + oa-cps = <64>;
>> + oa-txcte;
>> + oa-rxcte;
>> + oa-prote;
>> + };
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH net-next v2 8/9] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY
From: Andrew Lunn @ 2023-10-31 12:53 UTC (permalink / raw)
To: Parthiban.Veerasooran
Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr
In-Reply-To: <296aa172-404e-414a-a56a-ca82b3e90397@microchip.com>
> Ah ok, then it is supposed to be like below, isn't it?
>
> static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
> {
> struct sockaddr *address = addr;
> int ret;
>
> if (netif_running(netdev))
> return -EBUSY;
>
> ret = lan865x_set_hw_macaddr(netdev);
> if (ret)
> return ret;
>
> eth_hw_addr_set(netdev, address->sa_data);
>
> return 0;
> }
Yes, that is better. In practice, its probably not an issue, setting
the MAC address will never fail, but it is good to get right, just in
case.
Andrew
^ permalink raw reply
* Re: [PATCH v6 1/5] Bluetooth: mgmt: add MGMT_OP_SET_QUALITY_REPORT for quality report
From: Jonas Dreßler @ 2023-10-31 12:50 UTC (permalink / raw)
To: josephsih
Cc: chromeos-bluetooth-upstreaming, davem, edumazet, johan.hedberg,
josephsih, kuba, linux-bluetooth, linux-kernel, luiz.dentz,
marcel, netdev, pabeni, pali
In-Reply-To: <20220526112135.2486883-1-josephsih@chromium.org>
Hi Joseph,
gentle ping about this one, are you still planning to propose a v7 here?
Regards,
Jonas
^ permalink raw reply
* Re: [PATCH net-next v2 5/9] net: ethernet: oa_tc6: implement internal PHY initialization
From: Andrew Lunn @ 2023-10-31 12:48 UTC (permalink / raw)
To: Parthiban.Veerasooran
Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr
In-Reply-To: <7873124e-d7b2-4983-9be3-f85865d46de2@microchip.com>
> >> + tc6->mdiobus = mdiobus_alloc();
> >> + if (!tc6->mdiobus) {
> >> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
> >
> > Does the standard define this ? BIT(1), not BIT(0)?
> Ok, I think here is a typo. Will correct it.
There is still the open question, does the standard define this? If
not, a vendor might decide to use some other address, not 0. So it
might be better to not set a mask and scan the whole bus.
phy_find_first() should then work, no matter what address it is using.
Andrew
^ permalink raw reply
* Re: [PATCH v3 1/1] r8169: Coalesce RTL8411b PHY power-down recovery programming instructions to reduce spinlock stalls
From: Andrew Lunn @ 2023-10-31 12:38 UTC (permalink / raw)
To: Mirsad Todorovac
Cc: Heiner Kallweit, netdev, linux-kernel, nic_swsd, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Elver
In-Reply-To: <11f59506-406a-463b-94c1-fe20246a102f@alu.unizg.hr>
On Tue, Oct 31, 2023 at 04:35:19AM +0100, Mirsad Todorovac wrote:
> On 10/31/23 02:21, Andrew Lunn wrote:
> > > I will not contradict, but the cummulative amount of memory barriers on each MMIO read/write
> > > in each single one of the drivers could amount to some degrading of overall performance and
> > > latency in a multicore system.
> >
> > For optimisations, we like to see benchmark results which show some
> > improvements. Do you have any numbers?
>
> Hi, Andrew,
>
> Thank you for your interest in RTL NIC driver optimisations.
>
> My knowledge about the timing costs of synchronisation is mostly theoretical.
The kernel tends to be very practical. Maybe try to turn the
theoretical knowledge into practice. Write a benchmark test, or see if
any of the existing RT Linux tests show there is a real problem here,
and your changes fix it.
Andrew
^ permalink raw reply
* [PATCH] net: ethernet: ti: am65-cpsw: rx_pause/tx_pause controls wrong direction
From: Ronald Wahl @ 2023-10-31 12:20 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: Grygorii Strashko, Dan Carpenter, Siddharth Vadapalli,
Jakub Kicinski, David S . Miller, Roger Quadros, Ronald Wahl
From: Ronald Wahl <ronald.wahl@raritan.com>
The rx_pause flag says that whether we support receiving Pause frames.
When a Pause frame is received TX is delayed for some time. This is TX
flow control. In the same manner tx_pause is actually RX flow control.
Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 24120605502f..ece9f8df98ae 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1588,10 +1588,10 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
/* rx_pause/tx_pause */
if (rx_pause)
- mac_control |= CPSW_SL_CTL_RX_FLOW_EN;
+ mac_control |= CPSW_SL_CTL_TX_FLOW_EN;
if (tx_pause)
- mac_control |= CPSW_SL_CTL_TX_FLOW_EN;
+ mac_control |= CPSW_SL_CTL_RX_FLOW_EN;
cpsw_sl_ctl_set(port->slave.mac_sl, mac_control);
--
2.41.0
^ permalink raw reply related
* Re: [syzbot] [perf?] general protection fault in inherit_task_group
From: Peter Zijlstra @ 2023-10-31 12:16 UTC (permalink / raw)
To: syzbot
Cc: acme, adrian.hunter, alexander.shishkin, bpf, irogers, jolsa,
linux-kernel, linux-perf-users, mark.rutland, mingo, namhyung,
netdev, syzkaller-bugs
In-Reply-To: <000000000000d9483d060901f460@google.com>
On Tue, Oct 31, 2023 at 05:04:27AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: c17cda15cc86 Merge tag 'net-6.6-rc8' of git://git.kernel.o..
> git tree: bpf
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=151ab177680000
> kernel config: https://syzkaller.appspot.com/x/.config?x=7d1f30869bb78ec6
> dashboard link: https://syzkaller.appspot.com/bug?extid=756fe9affda890e892ae
> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=103b572b680000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=143a82c3680000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/d47cb341912c/disk-c17cda15.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/f37f0cf41858/vmlinux-c17cda15.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/e256afecf3c3/bzImage-c17cda15.xz
>
> The issue was bisected to:
>
> commit 32671e3799ca2e4590773fd0e63aaa4229e50c06
> Author: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Oct 18 11:56:54 2023 +0000
>
> perf: Disallow mis-matched inherited group reads
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10fdf71d680000
> final oops: https://syzkaller.appspot.com/x/report.txt?x=12fdf71d680000
> console output: https://syzkaller.appspot.com/x/log.txt?x=14fdf71d680000
a71ef31485bb51b846e8db8b3a35e432cc15afb5 upstream
^ permalink raw reply
* Re: [PATCH net] hsr: Prevent use after free in prp_create_tagged_frame()
From: Paolo Abeni @ 2023-10-31 12:15 UTC (permalink / raw)
To: Dan Carpenter, Murali Karicheri
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski,
Sebastian Andrzej Siewior, YueHaibing, Ziyang Xuan, netdev,
kernel-janitors
In-Reply-To: <57af1f28-7f57-4a96-bcd3-b7a0f2340845@moroto.mountain>
On Fri, 2023-10-27 at 15:19 +0300, Dan Carpenter wrote:
> The prp_fill_rct() function can fail. In that situation, it frees the
> skb and returns NULL. Meanwhile on the success path, it returns the
> original skb. So it's straight forward to fix bug by using the returned
> value.
>
> Fixes: 451d8123f897 ("net: prp: add packet handling support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> net/hsr/hsr_forward.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index b71dab630a87..80cdc6f6b34c 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -342,9 +342,7 @@ struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
> skb = skb_copy_expand(frame->skb_std, 0,
> skb_tailroom(frame->skb_std) + HSR_HLEN,
> GFP_ATOMIC);
> - prp_fill_rct(skb, frame, port);
> -
> - return skb;
> + return prp_fill_rct(skb, frame, port);
> }
>
> static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,
Acked-by: Paolo Abeni <pabeni@redhat.com>
(note both trees are currently locked now due to the pending PR; this
tag is intended to speed-up the merge after the PR itself)
^ permalink raw reply
* Re: [PATCH iproute2] ss: add support for rcv_wnd and rehash
From: Neal Cardwell @ 2023-10-31 12:08 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Ahern, Stephen Hemminger, David S . Miller, Jakub Kicinski,
Paolo Abeni, netdev, eric.dumazet
In-Reply-To: <20231031111720.2871511-1-edumazet@google.com>
On Tue, Oct 31, 2023 at 7:17 AM Eric Dumazet <edumazet@google.com> wrote:
>
> tcpi_rcv_wnd and tcpi_rehash were added in linux-6.2.
>
> $ ss -ti
> ...
> cubic wscale:7,7 ... minrtt:0.01 snd_wnd:65536 rcv_wnd:458496
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
Thanks, Eric, for implementing this!
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* [syzbot] [perf?] general protection fault in inherit_task_group
From: syzbot @ 2023-10-31 12:04 UTC (permalink / raw)
To: acme, adrian.hunter, alexander.shishkin, bpf, irogers, jolsa,
linux-kernel, linux-perf-users, mark.rutland, mingo, namhyung,
netdev, peterz, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: c17cda15cc86 Merge tag 'net-6.6-rc8' of git://git.kernel.o..
git tree: bpf
console+strace: https://syzkaller.appspot.com/x/log.txt?x=151ab177680000
kernel config: https://syzkaller.appspot.com/x/.config?x=7d1f30869bb78ec6
dashboard link: https://syzkaller.appspot.com/bug?extid=756fe9affda890e892ae
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=103b572b680000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=143a82c3680000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/d47cb341912c/disk-c17cda15.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/f37f0cf41858/vmlinux-c17cda15.xz
kernel image: https://storage.googleapis.com/syzbot-assets/e256afecf3c3/bzImage-c17cda15.xz
The issue was bisected to:
commit 32671e3799ca2e4590773fd0e63aaa4229e50c06
Author: Peter Zijlstra <peterz@infradead.org>
Date: Wed Oct 18 11:56:54 2023 +0000
perf: Disallow mis-matched inherited group reads
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10fdf71d680000
final oops: https://syzkaller.appspot.com/x/report.txt?x=12fdf71d680000
console output: https://syzkaller.appspot.com/x/log.txt?x=14fdf71d680000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+756fe9affda890e892ae@syzkaller.appspotmail.com
Fixes: 32671e3799ca ("perf: Disallow mis-matched inherited group reads")
general protection fault, probably for non-canonical address 0xdffffc0000000011: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000088-0x000000000000008f]
CPU: 0 PID: 5052 Comm: syz-executor420 Not tainted 6.6.0-rc7-syzkaller-00089-gc17cda15cc86 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:inherit_group kernel/events/core.c:13375 [inline]
RIP: 0010:inherit_task_group.isra.0+0x248/0x5e0 kernel/events/core.c:13422
Code: 38 d0 7c 08 84 d2 0f 85 18 03 00 00 49 8d bf 8c 00 00 00 8b 9b 8c 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 eb
RSP: 0018:ffffc90003b1fa48 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000011 RSI: ffffffff81b0fbc9 RDI: 000000000000008c
RBP: ffff88801ce578a0 R08: 0000000000000005 R09: 0000000000000001
R10: 0000000000000001 R11: ffffffff8a60008b R12: 0000000000000000
R13: ffff88807532bb80 R14: ffffc90003b1fae0 R15: 0000000000000000
FS: 00007f07e8df36c0(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f07e8df3fe8 CR3: 0000000078a1f000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
perf_event_init_context kernel/events/core.c:13487 [inline]
perf_event_init_task+0x330/0x740 kernel/events/core.c:13538
copy_process+0x242c/0x73f0 kernel/fork.c:2475
kernel_clone+0xfd/0x920 kernel/fork.c:2909
__do_sys_clone+0xba/0x100 kernel/fork.c:3052
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f07e8e324d9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 18 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f07e8df3228 EFLAGS: 00000246 ORIG_RAX: 0000000000000038
RAX: ffffffffffffffda RBX: 00007f07e8ebc308 RCX: 00007f07e8e324d9
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 00007f07e8ebc300 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f07e8ebc30c
R13: 0000000000000016 R14: 00007ffca721dd10 R15: 00007ffca721ddf8
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:inherit_group kernel/events/core.c:13375 [inline]
RIP: 0010:inherit_task_group.isra.0+0x248/0x5e0 kernel/events/core.c:13422
Code: 38 d0 7c 08 84 d2 0f 85 18 03 00 00 49 8d bf 8c 00 00 00 8b 9b 8c 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 eb
RSP: 0018:ffffc90003b1fa48 EFLAGS: 00010207
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000011 RSI: ffffffff81b0fbc9 RDI: 000000000000008c
RBP: ffff88801ce578a0 R08: 0000000000000005 R09: 0000000000000001
R10: 0000000000000001 R11: ffffffff8a60008b R12: 0000000000000000
R13: ffff88807532bb80 R14: ffffc90003b1fae0 R15: 0000000000000000
FS: 00007f07e8df36c0(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f07e8e5ef20 CR3: 0000000078a1f000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
0: 38 d0 cmp %dl,%al
2: 7c 08 jl 0xc
4: 84 d2 test %dl,%dl
6: 0f 85 18 03 00 00 jne 0x324
c: 49 8d bf 8c 00 00 00 lea 0x8c(%r15),%rdi
13: 8b 9b 8c 00 00 00 mov 0x8c(%rbx),%ebx
19: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
20: fc ff df
23: 48 89 fa mov %rdi,%rdx
26: 48 c1 ea 03 shr $0x3,%rdx
* 2a: 0f b6 14 02 movzbl (%rdx,%rax,1),%edx <-- trapping instruction
2e: 48 89 f8 mov %rdi,%rax
31: 83 e0 07 and $0x7,%eax
34: 83 c0 03 add $0x3,%eax
37: 38 d0 cmp %dl,%al
39: 7c 08 jl 0x43
3b: 84 d2 test %dl,%dl
3d: 0f .byte 0xf
3e: 85 eb test %ebp,%ebx
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH net] tcp: fix fastopen code vs usec TS
From: Neal Cardwell @ 2023-10-31 12:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, kernel test robot, David Morley
In-Reply-To: <20231031061945.2801972-1-edumazet@google.com>
On Tue, Oct 31, 2023 at 2:19 AM Eric Dumazet <edumazet@google.com> wrote:
>
> After blamed commit, TFO client-ack-dropped-then-recovery-ms-timestamps
> packetdrill test failed.
>
> David Morley and Neal Cardwell started investigating and Neal pointed
> that we had :
>
> tcp_conn_request()
> tcp_try_fastopen()
> -> tcp_fastopen_create_child
> -> child = inet_csk(sk)->icsk_af_ops->syn_recv_sock()
> -> tcp_create_openreq_child()
> -> copy req_usec_ts from req:
> newtp->tcp_usec_ts = treq->req_usec_ts;
> // now the new TFO server socket always does usec TS, no matter
> // what the route options are...
> send_synack()
> -> tcp_make_synack()
> // disable tcp_rsk(req)->req_usec_ts if route option is not present:
> if (tcp_rsk(req)->req_usec_ts < 0)
> tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);
>
> tcp_conn_request() has the initial dst, we can initialize
> tcp_rsk(req)->req_usec_ts there instead of later in send_synack();
>
> This means tcp_rsk(req)->req_usec_ts can be a boolean.
>
> Many thanks to David an Neal for their help.
>
> Fixes: 614e8316aa4c ("tcp: add support for usec resolution in TCP TS values")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202310302216.f79d78bc-oliver.sang@intel.com
> Suggested-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Morley <morleyd@google.com>
> ---
Looks great to me. Thanks, Eric!
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH net-next v4 1/6] net: ethtool: allow symmetric-xor RSS hash for any flow type
From: Gal Pressman @ 2023-10-31 12:00 UTC (permalink / raw)
To: Ahmed Zaki, Jakub Kicinski, Alexander H Duyck
Cc: mkubecek, andrew, willemdebruijn.kernel, Wojciech Drewek, corbet,
netdev, linux-doc, jesse.brandeburg, edumazet, anthony.l.nguyen,
horms, vladimir.oltean, Jacob Keller, intel-wired-lan, pabeni,
davem
In-Reply-To: <a2a1164f-1492-43d1-9667-5917d0ececcb@intel.com>
On 29/10/2023 18:59, Ahmed Zaki wrote:
>
>
> On 2023-10-29 06:48, Gal Pressman wrote:
>> On 29/10/2023 14:42, Ahmed Zaki wrote:
>>>
>>>
>>> On 2023-10-29 06:25, Gal Pressman wrote:
>>>> On 21/10/2023 3:00, Ahmed Zaki wrote:
>>>>>
>>>>>
>>>>> On 2023-10-20 17:49, Jakub Kicinski wrote:
>>>>>> On Fri, 20 Oct 2023 17:14:11 -0600 Ahmed Zaki wrote:
>>>>>>> I replied to that here:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/afb4a06f-cfba-47ba-adb3-09bea7cb5f00@intel.com/
>>>>>>>
>>>>>>> I am kind of confused now so please bear with me. ethtool either
>>>>>>> sends
>>>>>>> "ethtool_rxfh" or "ethtool_rxnfc". AFAIK "ethtool_rxfh" is the
>>>>>>> interface
>>>>>>> for "ethtool -X" which is used to set the RSS algorithm. But we
>>>>>>> kind of
>>>>>>> agreed to go with "ethtool -U|-N" for symmetric-xor, and that uses
>>>>>>> "ethtool_rxnfc" (as implemented in this series).
>>>>>>
>>>>>> I have no strong preference. Sounds like Alex prefers to keep it
>>>>>> closer
>>>>>> to algo, which is "ethtool_rxfh".
>>>>>>
>>>>>>> Do you mean use "ethtool_rxfh" instead of "ethtool_rxnfc"? how would
>>>>>>> that work on the ethtool user interface?
>>>>>>
>>>>>> I don't know what you're asking of us. If you find the code to
>>>>>> confusing
>>>>>> maybe someone at Intel can help you :|
>>>>>
>>>>> The code is straightforward. I am confused by the requirements: don't
>>>>> add a new algorithm but use "ethtool_rxfh".
>>>>>
>>>>> I'll see if I can get more help, may be I am missing something.
>>>>>
>>>>
>>>> What was the decision here?
>>>> Is this going to be exposed through ethtool -N or -X?
>>>
>>> I am working on a new version that uses "ethtool_rxfh" to set the
>>> symmetric-xor. The user will set per-device via:
>>>
>>> ethtool -X eth0 hfunc toeplitz symmetric-xor
>>>
>>> then specify the per-flow type RSS fields as usual:
>>>
>>> ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n
>>>
>>> The downside is that all flow-types will have to be either symmetric or
>>> asymmetric.
>>
>> Why are we making the interface less flexible than it can be with -N?
>
> Alexander Duyck prefers to implement the "symmetric-xor" interface as an
> algorithm or extension (please refer to previous messages), but ethtool
> does not provide flowtype/RSS fields setting via "-X". The above was the
> best solution that we (at Intel) could think of.
OK, it's a weird we're deliberately limiting our interface, given
there's already hardware that supports controlling symmetric hashing per
flow type.
I saw you mentioned the way ice hardware implements symmetric-xor
somewhere, it definitely needs to be added somewhere in our
documentation to prevent confusion.
mlx5 hardware also does symmetric hashing with xor, but not exactly as
you described, we need the algorithm to be clear.
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: Use SMP threads for backlog NAPI (or optional).
From: Wander Lairson Costa @ 2023-10-31 11:36 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jakub Kicinski, juri.lelli, linux-kernel, netdev, David S. Miller,
Eric Dumazet, Paolo Abeni, Peter Zijlstra, Thomas Gleixner,
Come On Now
In-Reply-To: <20231031101424.I2hTisNY@linutronix.de>
On Tue, Oct 31, 2023 at 7:14 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2023-10-16 16:53:39 [+0200], To Jakub Kicinski wrote:
> > On 2023-10-16 07:17:56 [-0700], Jakub Kicinski wrote:
> > > On Mon, 16 Oct 2023 11:53:21 +0200 Sebastian Andrzej Siewior wrote:
> > > > > Do we have reason to believe nobody uses RPS?
> > > >
> > > > Not sure what you relate to. I would assume that RPS is used in general
> > > > on actual devices and not on loopback where backlog is used. But it is
> > > > just an assumption.
> > > > The performance drop, which I observed with RPS and stress-ng --udp, is
> > > > within the same range with threads and IPIs (based on memory). I can
> > > > re-run the test and provide actual numbers if you want.
> > >
> > > I was asking about RPS because with your current series RPS processing
> > > is forced into threads. IDK how well you can simulate the kind of
> > > workload which requires RPS. I've seen it used mostly on proxyies
> > > and gateways. For proxies Meta's experiments with threaded NAPI show
> > > regressions across the board. So "force-threading" RPS will most likely
> > > also cause regressions.
> >
> > Understood.
> >
> > Wandere/ Juri: Do you have any benchmark/ workload where you would see
> > whether RPS with IPI (now) vs RPS (this patch) shows any regression?
>
> So I poked offlist other RH people and I've been told that they hardly
> ever test RPS since the NICs these days have RSS in hardware.
Sorry, Juri is in PTO and I am just back from sick leave and still
catching up. I've been contacting some QE people, but so far it is
like you said, no stress test for RPS. If I have some news, I let you
know.
>
> Sebastian
>
^ permalink raw reply
* [net PATCH] octeontx2-pf: Free pending and dropped SQEs
From: Geetha sowjanya @ 2023-10-31 11:23 UTC (permalink / raw)
To: netdev, linux-kernel, bpf
Cc: kuba, davem, pabeni, edumazet, ast, daniel, hawk, sgoutham,
gakula, sbhatta, hkelam
On interface down, the pending SQEs in the NIX get dropped
or drained out during SMQ flush. But skb's pointed by these
SQEs never get free or updated to the stack as respective CQE
never get added.
This patch fixes the issue by freeing all valid skb's in SQ SG list.
Fixes: b1bc8457e9d0 ("octeontx2-pf: Cleanup all receive buffers in SG descriptor")
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
.../marvell/octeontx2/nic/otx2_common.c | 15 +++----
.../marvell/octeontx2/nic/otx2_common.h | 1 +
.../ethernet/marvell/octeontx2/nic/otx2_pf.c | 1 +
.../marvell/octeontx2/nic/otx2_txrx.c | 42 +++++++++++++++++++
4 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 1a42bfded872..7ca6941ea0b9 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -818,7 +818,6 @@ void otx2_sqb_flush(struct otx2_nic *pfvf)
int qidx, sqe_tail, sqe_head;
struct otx2_snd_queue *sq;
u64 incr, *ptr, val;
- int timeout = 1000;
ptr = (u64 *)otx2_get_regaddr(pfvf, NIX_LF_SQ_OP_STATUS);
for (qidx = 0; qidx < otx2_get_total_tx_queues(pfvf); qidx++) {
@@ -827,15 +826,11 @@ void otx2_sqb_flush(struct otx2_nic *pfvf)
continue;
incr = (u64)qidx << 32;
- while (timeout) {
- val = otx2_atomic64_add(incr, ptr);
- sqe_head = (val >> 20) & 0x3F;
- sqe_tail = (val >> 28) & 0x3F;
- if (sqe_head == sqe_tail)
- break;
- usleep_range(1, 3);
- timeout--;
- }
+ val = otx2_atomic64_add(incr, ptr);
+ sqe_head = (val >> 20) & 0x3F;
+ sqe_tail = (val >> 28) & 0x3F;
+ if (sqe_head != sqe_tail)
+ usleep_range(50, 60);
}
}
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
index c04a8ee53a82..e7c69b57147e 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
@@ -977,6 +977,7 @@ int otx2_txschq_config(struct otx2_nic *pfvf, int lvl, int prio, bool pfc_en);
int otx2_txsch_alloc(struct otx2_nic *pfvf);
void otx2_txschq_stop(struct otx2_nic *pfvf);
void otx2_txschq_free_one(struct otx2_nic *pfvf, u16 lvl, u16 schq);
+void otx2_free_pending_sqe(struct otx2_nic *pfvf);
void otx2_sqb_flush(struct otx2_nic *pfvf);
int otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
dma_addr_t *dma);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
index 6daf4d58c25d..8e82db6092af 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
@@ -1589,6 +1589,7 @@ static void otx2_free_hw_resources(struct otx2_nic *pf)
else
otx2_cleanup_tx_cqes(pf, cq);
}
+ otx2_free_pending_sqe(pf);
otx2_free_sq_res(pf);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 53b2a4ef5298..6ee15f3c25ed 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -1247,9 +1247,11 @@ void otx2_cleanup_rx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue *cq, int q
void otx2_cleanup_tx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue *cq)
{
+ int tx_pkts = 0, tx_bytes = 0;
struct sk_buff *skb = NULL;
struct otx2_snd_queue *sq;
struct nix_cqe_tx_s *cqe;
+ struct netdev_queue *txq;
int processed_cqe = 0;
struct sg_list *sg;
int qidx;
@@ -1270,12 +1272,20 @@ void otx2_cleanup_tx_cqes(struct otx2_nic *pfvf, struct otx2_cq_queue *cq)
sg = &sq->sg[cqe->comp.sqe_id];
skb = (struct sk_buff *)sg->skb;
if (skb) {
+ tx_bytes += skb->len;
+ tx_pkts++;
otx2_dma_unmap_skb_frags(pfvf, sg);
dev_kfree_skb_any(skb);
sg->skb = (u64)NULL;
}
}
+ if (likely(tx_pkts)) {
+ if (qidx >= pfvf->hw.tx_queues)
+ qidx -= pfvf->hw.xdp_queues;
+ txq = netdev_get_tx_queue(pfvf->netdev, qidx);
+ netdev_tx_completed_queue(txq, tx_pkts, tx_bytes);
+ }
/* Free CQEs to HW */
otx2_write64(pfvf, NIX_LF_CQ_OP_DOOR,
((u64)cq->cq_idx << 32) | processed_cqe);
@@ -1302,6 +1312,38 @@ int otx2_rxtx_enable(struct otx2_nic *pfvf, bool enable)
return err;
}
+void otx2_free_pending_sqe(struct otx2_nic *pfvf)
+{
+ int tx_pkts = 0, tx_bytes = 0;
+ struct sk_buff *skb = NULL;
+ struct otx2_snd_queue *sq;
+ struct netdev_queue *txq;
+ struct sg_list *sg;
+ int sq_idx, sqe;
+
+ for (sq_idx = 0; sq_idx < pfvf->hw.tx_queues; sq_idx++) {
+ sq = &pfvf->qset.sq[sq_idx];
+ for (sqe = 0; sqe < sq->sqe_cnt; sqe++) {
+ sg = &sq->sg[sqe];
+ skb = (struct sk_buff *)sg->skb;
+ if (skb) {
+ tx_bytes += skb->len;
+ tx_pkts++;
+ otx2_dma_unmap_skb_frags(pfvf, sg);
+ dev_kfree_skb_any(skb);
+ sg->skb = (u64)NULL;
+ }
+ }
+
+ if (!tx_pkts)
+ continue;
+ txq = netdev_get_tx_queue(pfvf->netdev, sq_idx);
+ netdev_tx_completed_queue(txq, tx_pkts, tx_bytes);
+ tx_pkts = 0;
+ tx_bytes = 0;
+ }
+}
+
static void otx2_xdp_sqe_add_sg(struct otx2_snd_queue *sq, u64 dma_addr,
int len, int *offset)
{
--
2.25.1
^ permalink raw reply related
* [RFC] Questionable RCU/BH usage in cgw_create_job().
From: Sebastian Andrzej Siewior @ 2023-10-31 11:23 UTC (permalink / raw)
To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can, netdev, Thomas Gleixner
Hi,
I stumbled over this piece in cgw_create_job():
| /* update modifications with disabled softirq & quit */
| local_bh_disable();
| memcpy(&gwj->mod, &mod, sizeof(mod));
| local_bh_enable();
| return 0;
unfortunately the comment did not provide much enlightenment for me.
Let's look. That memcpy() overwrites struct cf_mod within struct cgw_job
which is under RCU protection. memcpy() and RCU hardly works as a combo.
But why the local_bh_disable()?
Let's look further. The user of this data structure is can_can_gw_rcv().
There is something like:
| /* check for checksum updates */
| if (gwj->mod.csumfunc.crc8)
| (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
With optimisation enabled (as in -O2 or so) the compiler will fetch
mod.csumfunc.crc8, do the comparison and if non-NULL use the previously
fetched value and jump there. So one could argue that it is not really
affected by the memcpy() suddenly setting it to NULL. However, adding
any kind of a function in between, say
| /* check for checksum updates */
| if (gwj->mod.csumfunc.crc8) {
|+ trace_event_crc8_sth(cf)
| (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
| }
will force the compiler to reload mod.csumfunc.crc8. And here is the
possible NULL pointer if overwritten by update in cgw_create_job().
One reload that already happens is the one of mod.modfunc. First at the
top we have:
| if (gwj->mod.modfunc[0])
| nskb = skb_copy(skb, GFP_ATOMIC);
| else
| nskb = skb_clone(skb, GFP_ATOMIC);
Here mod.modfunc[0] is NULL and skb_clone() is invoked. Later if
mod.modfunc has been set to non-NULL value this piece
| while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
| (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
reloads mod.modfunc and may modify the skb assuming that skb_copy() has
been used earlier.
Looking at this makes me think that the local_bh_disable() has been
added to the memcpy() just to ensure that can_can_gw_rcv() won't run
because it is invoked in a BH disabled context. Clever little trick that
is. But this trick is limited to UP environments…
Am I missing something?
If not, my suggestion would be replacing the bh-off, memcpy part with:
| old_mod = rcu_replace_pointer(gwj->mod, new_mod, true);
| kfree_rcu_mightsleep(old_mod);
and doing the needed pointer replacement with for struct cgw_job::mod
and RCU annotation.
Sebastian
^ permalink raw reply
* [PATCH iproute2] ss: add support for rcv_wnd and rehash
From: Eric Dumazet @ 2023-10-31 11:17 UTC (permalink / raw)
To: David Ahern, Stephen Hemminger
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Neal Cardwell,
netdev, eric.dumazet, Eric Dumazet
tcpi_rcv_wnd and tcpi_rehash were added in linux-6.2.
$ ss -ti
...
cubic wscale:7,7 ... minrtt:0.01 snd_wnd:65536 rcv_wnd:458496
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
misc/ss.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/misc/ss.c b/misc/ss.c
index 2628c2e042f1cdb616ec8aa80d4f413b41dfd3f4..9438382b8e667529dc2cf4b020d8696a4175e992 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -865,6 +865,8 @@ struct tcpstat {
double min_rtt;
unsigned int rcv_ooopack;
unsigned int snd_wnd;
+ unsigned int rcv_wnd;
+ unsigned int rehash;
int rcv_space;
unsigned int rcv_ssthresh;
unsigned long long busy_time;
@@ -2711,6 +2713,10 @@ static void tcp_stats_print(struct tcpstat *s)
out(" rcv_ooopack:%u", s->rcv_ooopack);
if (s->snd_wnd)
out(" snd_wnd:%u", s->snd_wnd);
+ if (s->rcv_wnd)
+ out(" rcv_wnd:%u", s->rcv_wnd);
+ if (s->rehash)
+ out(" rehash:%u", s->rehash);
}
static void tcp_timer_print(struct tcpstat *s)
@@ -3147,6 +3153,8 @@ static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
s.bytes_retrans = info->tcpi_bytes_retrans;
s.rcv_ooopack = info->tcpi_rcv_ooopack;
s.snd_wnd = info->tcpi_snd_wnd;
+ s.rcv_wnd = info->tcpi_rcv_wnd;
+ s.rehash = info->tcpi_rehash;
tcp_stats_print(&s);
free(s.dctcp);
free(s.bbr_info);
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* Re: [PATCH net v1 2/2] octeontx2-pf: Fix holes in error code
From: Paolo Abeni @ 2023-10-31 11:06 UTC (permalink / raw)
To: Ratheesh Kannoth, netdev, linux-kernel
Cc: sgoutham, gakula, sbhatta, hkelam, davem, edumazet, kuba,
wojciech.drewek
In-Reply-To: <20231027021953.1819959-2-rkannoth@marvell.com>
On Fri, 2023-10-27 at 07:49 +0530, Ratheesh Kannoth wrote:
> Error code strings are not getting printed properly
> due to holes. Print error code as well.
>
> Fixes: 51afe9026d0c ("octeontx2-pf: NIX TX overwrites SQ_CTX_HW_S[SQ_INT]")
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
>
> ---
> ChangeLog:
>
> v0 -> v1: Splitted patch into two
> ---
> .../ethernet/marvell/octeontx2/nic/otx2_pf.c | 80 +++++++++++--------
> 1 file changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> index 6daf4d58c25d..125fe231702a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -1193,31 +1193,32 @@ static char *nix_mnqerr_e_str[NIX_MNQERR_MAX] = {
> };
>
> static char *nix_snd_status_e_str[NIX_SND_STATUS_MAX] = {
> - "NIX_SND_STATUS_GOOD",
> - "NIX_SND_STATUS_SQ_CTX_FAULT",
> - "NIX_SND_STATUS_SQ_CTX_POISON",
> - "NIX_SND_STATUS_SQB_FAULT",
> - "NIX_SND_STATUS_SQB_POISON",
> - "NIX_SND_STATUS_HDR_ERR",
> - "NIX_SND_STATUS_EXT_ERR",
> - "NIX_SND_STATUS_JUMP_FAULT",
> - "NIX_SND_STATUS_JUMP_POISON",
> - "NIX_SND_STATUS_CRC_ERR",
> - "NIX_SND_STATUS_IMM_ERR",
> - "NIX_SND_STATUS_SG_ERR",
> - "NIX_SND_STATUS_MEM_ERR",
> - "NIX_SND_STATUS_INVALID_SUBDC",
> - "NIX_SND_STATUS_SUBDC_ORDER_ERR",
> - "NIX_SND_STATUS_DATA_FAULT",
> - "NIX_SND_STATUS_DATA_POISON",
> - "NIX_SND_STATUS_NPC_DROP_ACTION",
> - "NIX_SND_STATUS_LOCK_VIOL",
> - "NIX_SND_STATUS_NPC_UCAST_CHAN_ERR",
> - "NIX_SND_STATUS_NPC_MCAST_CHAN_ERR",
> - "NIX_SND_STATUS_NPC_MCAST_ABORT",
> - "NIX_SND_STATUS_NPC_VTAG_PTR_ERR",
> - "NIX_SND_STATUS_NPC_VTAG_SIZE_ERR",
> - "NIX_SND_STATUS_SEND_STATS_ERR",
> + [NIX_SND_STATUS_GOOD] = "NIX_SND_STATUS_GOOD",
> + [NIX_SND_STATUS_SQ_CTX_FAULT] = "NIX_SND_STATUS_SQ_CTX_FAULT",
> + [NIX_SND_STATUS_SQ_CTX_POISON] = "NIX_SND_STATUS_SQ_CTX_POISON",
> + [NIX_SND_STATUS_SQB_FAULT] = "NIX_SND_STATUS_SQB_FAULT",
> + [NIX_SND_STATUS_SQB_POISON] = "NIX_SND_STATUS_SQB_POISON",
> + [NIX_SND_STATUS_HDR_ERR] = "NIX_SND_STATUS_HDR_ERR",
> + [NIX_SND_STATUS_EXT_ERR] = "NIX_SND_STATUS_EXT_ERR",
> + [NIX_SND_STATUS_JUMP_FAULT] = "NIX_SND_STATUS_JUMP_FAULT",
> + [NIX_SND_STATUS_JUMP_POISON] = "NIX_SND_STATUS_JUMP_POISON",
> + [NIX_SND_STATUS_CRC_ERR] = "NIX_SND_STATUS_CRC_ERR",
> + [NIX_SND_STATUS_IMM_ERR] = "NIX_SND_STATUS_IMM_ERR",
> + [NIX_SND_STATUS_SG_ERR] = "NIX_SND_STATUS_SG_ERR",
> + [NIX_SND_STATUS_MEM_ERR] = "NIX_SND_STATUS_MEM_ERR",
> + [NIX_SND_STATUS_INVALID_SUBDC] = "NIX_SND_STATUS_INVALID_SUBDC",
> + [NIX_SND_STATUS_SUBDC_ORDER_ERR] = "NIX_SND_STATUS_SUBDC_ORDER_ERR",
> + [NIX_SND_STATUS_DATA_FAULT] = "NIX_SND_STATUS_DATA_FAULT",
> + [NIX_SND_STATUS_DATA_POISON] = "NIX_SND_STATUS_DATA_POISON",
> + [NIX_SND_STATUS_NPC_DROP_ACTION] = "NIX_SND_STATUS_NPC_DROP_ACTION",
> + [NIX_SND_STATUS_LOCK_VIOL] = "NIX_SND_STATUS_LOCK_VIOL",
> + [NIX_SND_STATUS_NPC_UCAST_CHAN_ERR] = "NIX_SND_STAT_NPC_UCAST_CHAN_ERR",
> + [NIX_SND_STATUS_NPC_MCAST_CHAN_ERR] = "NIX_SND_STAT_NPC_MCAST_CHAN_ERR",
> + [NIX_SND_STATUS_NPC_MCAST_ABORT] = "NIX_SND_STATUS_NPC_MCAST_ABORT",
> + [NIX_SND_STATUS_NPC_VTAG_PTR_ERR] = "NIX_SND_STATUS_NPC_VTAG_PTR_ERR",
> + [NIX_SND_STATUS_NPC_VTAG_SIZE_ERR] = "NIX_SND_STATUS_NPC_VTAG_SIZE_ERR",
> + [NIX_SND_STATUS_SEND_MEM_FAULT] = "NIX_SND_STATUS_SEND_MEM_FAULT",
> + [NIX_SND_STATUS_SEND_STATS_ERR] = "NIX_SND_STATUS_SEND_STATS_ERR",
> };
>
> static irqreturn_t otx2_q_intr_handler(int irq, void *data)
> @@ -1238,14 +1239,16 @@ static irqreturn_t otx2_q_intr_handler(int irq, void *data)
> continue;
>
> if (val & BIT_ULL(42)) {
> - netdev_err(pf->netdev, "CQ%lld: error reading NIX_LF_CQ_OP_INT, NIX_LF_ERR_INT 0x%llx\n",
> + netdev_err(pf->netdev,
> + "CQ%lld: error reading NIX_LF_CQ_OP_INT, NIX_LF_ERR_INT 0x%llx\n",
> qidx, otx2_read64(pf, NIX_LF_ERR_INT));
> } else {
> if (val & BIT_ULL(NIX_CQERRINT_DOOR_ERR))
> netdev_err(pf->netdev, "CQ%lld: Doorbell error",
> qidx);
> if (val & BIT_ULL(NIX_CQERRINT_CQE_FAULT))
> - netdev_err(pf->netdev, "CQ%lld: Memory fault on CQE write to LLC/DRAM",
> + netdev_err(pf->netdev,
> + "CQ%lld: Memory fault on CQE write to LLC/DRAM",
> qidx);
> }
It's not a big deal (no need to repost just for this), but the above
chunk (and a couple below, too) is not related to the current fix, you
should have not included it here.
Cheers,
Paolo
>
> @@ -1272,7 +1275,8 @@ static irqreturn_t otx2_q_intr_handler(int irq, void *data)
> (val & NIX_SQINT_BITS));
>
> if (val & BIT_ULL(42)) {
> - netdev_err(pf->netdev, "SQ%lld: error reading NIX_LF_SQ_OP_INT, NIX_LF_ERR_INT 0x%llx\n",
> + netdev_err(pf->netdev,
> + "SQ%lld: error reading NIX_LF_SQ_OP_INT, NIX_LF_ERR_INT 0x%llx\n",
> qidx, otx2_read64(pf, NIX_LF_ERR_INT));
> goto done;
> }
> @@ -1282,8 +1286,11 @@ static irqreturn_t otx2_q_intr_handler(int irq, void *data)
> goto chk_mnq_err_dbg;
>
> sq_op_err_code = FIELD_GET(GENMASK(7, 0), sq_op_err_dbg);
> - netdev_err(pf->netdev, "SQ%lld: NIX_LF_SQ_OP_ERR_DBG(%llx) err=%s\n",
> - qidx, sq_op_err_dbg, nix_sqoperr_e_str[sq_op_err_code]);
> + netdev_err(pf->netdev,
> + "SQ%lld: NIX_LF_SQ_OP_ERR_DBG(0x%llx) err=%s(%#x)\n",
> + qidx, sq_op_err_dbg,
> + nix_sqoperr_e_str[sq_op_err_code],
> + sq_op_err_code);
>
> otx2_write64(pf, NIX_LF_SQ_OP_ERR_DBG, BIT_ULL(44));
>
> @@ -1300,16 +1307,21 @@ static irqreturn_t otx2_q_intr_handler(int irq, void *data)
> goto chk_snd_err_dbg;
>
> mnq_err_code = FIELD_GET(GENMASK(7, 0), mnq_err_dbg);
> - netdev_err(pf->netdev, "SQ%lld: NIX_LF_MNQ_ERR_DBG(%llx) err=%s\n",
> - qidx, mnq_err_dbg, nix_mnqerr_e_str[mnq_err_code]);
> + netdev_err(pf->netdev,
> + "SQ%lld: NIX_LF_MNQ_ERR_DBG(0x%llx) err=%s(%#x)\n",
> + qidx, mnq_err_dbg, nix_mnqerr_e_str[mnq_err_code],
> + mnq_err_code);
> otx2_write64(pf, NIX_LF_MNQ_ERR_DBG, BIT_ULL(44));
>
> chk_snd_err_dbg:
> snd_err_dbg = otx2_read64(pf, NIX_LF_SEND_ERR_DBG);
> if (snd_err_dbg & BIT(44)) {
> snd_err_code = FIELD_GET(GENMASK(7, 0), snd_err_dbg);
> - netdev_err(pf->netdev, "SQ%lld: NIX_LF_SND_ERR_DBG:0x%llx err=%s\n",
> - qidx, snd_err_dbg, nix_snd_status_e_str[snd_err_code]);
> + netdev_err(pf->netdev,
> + "SQ%lld: NIX_LF_SND_ERR_DBG:0x%llx err=%s(%#x)\n",
> + qidx, snd_err_dbg,
> + nix_snd_status_e_str[snd_err_code],
> + snd_err_code);
> otx2_write64(pf, NIX_LF_SEND_ERR_DBG, BIT_ULL(44));
> }
>
^ permalink raw reply
* Re: [PATCH net-next v2 8/9] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY
From: Parthiban.Veerasooran @ 2023-10-31 11:04 UTC (permalink / raw)
To: andrew
Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr
In-Reply-To: <eee6df3d-5e6b-4b4c-bfcc-55b31814fb82@lunn.ch>
Hi Andrew,
On 24/10/23 8:03 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
>> +{
>> + u32 regval;
>> + bool ret;
>> + struct lan865x_priv *priv = netdev_priv(netdev);
>> + const u8 *mac = netdev->dev_addr;
>> +
>> + ret = oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, ®val);
>> + if (ret)
>> + goto error_mac;
>> + if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {
>
> Would testing netif_running(netdev) be enough? That is a more common
> test you see. In fact, you already have that in
> lan865x_set_mac_address(). And in lan865x_probe() why would the
> hardware be enabled?
Ah yes, this check is not needed at all. Will correct it.
>
>
>> + if (netif_msg_drv(priv))
>> + netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
>> + return -EBUSY;
>> + }
>> + /* MAC address setting */
>> + regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0];
>> + ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAB1, regval);
>> + if (ret)
>> + goto error_mac;
>> +
>> + regval = (mac[5] << 8) | mac[4];
>> + ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
>> + if (ret)
>> + goto error_mac;
>> +
>> + return 0;
>> +
>> +error_mac:
>> + return -ENODEV;
>
> oa_tc6_write_register() should return an error code, so return it.
Yes, noted. Will do it.
>
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> + struct sockaddr *address = addr;
>> +
>> + if (netif_running(netdev))
>> + return -EBUSY;
>> +
>> + eth_hw_addr_set(netdev, address->sa_data);
>> +
>> + return lan865x_set_hw_macaddr(netdev);
>
> You should ideally only update the netdev MAC address, if you managed
> to change the value in the hardware.
Ah ok, then it is supposed to be like below, isn't it?
static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
{
struct sockaddr *address = addr;
int ret;
if (netif_running(netdev))
return -EBUSY;
ret = lan865x_set_hw_macaddr(netdev);
if (ret)
return ret;
eth_hw_addr_set(netdev, address->sa_data);
return 0;
}
>
>> +static void lan865x_set_multicast_list(struct net_device *netdev)
>> +{
>> + struct lan865x_priv *priv = netdev_priv(netdev);
>> + u32 regval = 0;
>> +
>> + if (netdev->flags & IFF_PROMISC) {
>> + /* Enabling promiscuous mode */
>> + regval |= MAC_PROMISCUOUS_MODE;
>> + regval &= (~MAC_MULTICAST_MODE);
>> + regval &= (~MAC_UNICAST_MODE);
>> + } else if (netdev->flags & IFF_ALLMULTI) {
>> + /* Enabling all multicast mode */
>> + regval &= (~MAC_PROMISCUOUS_MODE);
>> + regval |= MAC_MULTICAST_MODE;
>> + regval &= (~MAC_UNICAST_MODE);
>> + } else if (!netdev_mc_empty(netdev)) {
>> + /* Enabling specific multicast addresses */
>> + struct netdev_hw_addr *ha;
>> + u32 hash_lo = 0;
>> + u32 hash_hi = 0;
>> +
>> + netdev_for_each_mc_addr(ha, netdev) {
>> + u32 bit_num = lan865x_hash(ha->addr);
>> + u32 mask = 1 << (bit_num & 0x1f);
>> +
>> + if (bit_num & 0x20)
>> + hash_hi |= mask;
>> + else
>> + hash_lo |= mask;
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, hash_hi)) {
>> + if (netif_msg_timer(priv))
>> + netdev_err(netdev, "Failed to write reg_hashh");
>> + return;
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
>> + if (netif_msg_timer(priv))
>> + netdev_err(netdev, "Failed to write reg_hashl");
>> + return;
>> + }
>> + regval &= (~MAC_PROMISCUOUS_MODE);
>> + regval &= (~MAC_MULTICAST_MODE);
>> + regval |= MAC_UNICAST_MODE;
>> + } else {
>> + /* enabling local mac address only */
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, regval)) {
>> + if (netif_msg_timer(priv))
>> + netdev_err(netdev, "Failed to write reg_hashh");
>> + return;
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
>> + if (netif_msg_timer(priv))
>> + netdev_err(netdev, "Failed to write reg_hashl");
>> + return;
>> + }
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
>> + if (netif_msg_timer(priv))
>> + netdev_err(netdev, "Failed to enable promiscuous mode");
>> + }
>> +}
>
> Another of those big functions which could be lots of simple functions
> each doing one thing.
Sure, will split into multiple functions.
>
>> +/* Configures the number of bytes allocated to each buffer in the
>> + * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
>> + * is the default value. So it is enough to configure the queue buffer size only
>> + * for 32 bytes. Generally cps can't be changed during run time and also it is
>> + * configured in the device tree. The values for the Tx/Rx queue buffer size are
>> + * taken from the LAN865x datasheet.
>> + */
>
> Its unclear why this needs to be a callback. Why not just set it after
> oa_tc6_init() returns?
oa_tc6_init() function internally calls oa_tc6_configure() function to
configure different settings. At the end it writes SYNC bit to CONFIG0
register which enables the MAC-PHY to start the data transfer. So the
buffer configuration should be done prior to that. Since this is part of
the configuration this callback is implemented.
>
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> + struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> + oa_tc6_exit(priv->tc6);
>> + unregister_netdev(priv->netdev);
>
> Is that the correct order? Seems like you should unregister the netdev
> first.
Ah yes, will correct it.
>
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lan865x_acpi_ids[] = {
>> + { .id = "LAN865X",
>> + },
>> + {},
>
> Does this work? You don't have access to the device tree properties.
Going to remove all the OA specific configurations in the next revisions.
Best Regards,
Parthiban V
>
> Andrew
>
^ permalink raw reply
* Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset
From: Cristian Ciocaltea @ 2023-10-31 11:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Emil Renner Berthing, Samin Guo, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, Richard Cochran, Giuseppe Cavallaro
Cc: netdev, devicetree, linux-kernel, linux-riscv, linux-stm32,
linux-arm-kernel, kernel
In-Reply-To: <b67ee496-397b-42f1-8109-542878934385@linaro.org>
On 10/31/23 07:48, Krzysztof Kozlowski wrote:
> On 30/10/2023 20:07, Cristian Ciocaltea wrote:
>> On 10/30/23 09:26, Krzysztof Kozlowski wrote:
>>> On 29/10/2023 23:24, Cristian Ciocaltea wrote:
>>>> On 10/29/23 13:25, Krzysztof Kozlowski wrote:
>>>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>>>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires
>>>>>> just the 'ahb' reset name, but the binding allows selecting it only in
>>>>>> conjunction with 'stmmaceth'.
>>>>>>
>>>>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name.
>>>>>>
>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>> ---
>>>>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>>>> index 5c2769dc689a..a4d7172ea701 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>>>> @@ -146,7 +146,7 @@ properties:
>>>>>> reset-names:
>>>>>> minItems: 1
>>>>>> items:
>>>>>> - - const: stmmaceth
>>>>>> + - enum: [stmmaceth, ahb]
>>>>>
>>>>> Also, this makes sense only with patch #4, so this should be squashed there.
>>>>
>>>> I added this as a separate patch since it changes the generic schema
>>>> which is included by many other bindings. JH7100 just happens to be the
>>>> first use-case requiring this update. But I can squash the patch if
>>>> that's not a good enough reason to keep it separately.
>>>
>>> If there is no single user of this, why changing this? I would even
>>> argue that it is not correct from existing bindings point of view -
>>> nothing allows and uses ahb as the only reset. Even the commit msg
>>> mentions your hardware from patch 4.
>>
>> Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and,
>> as a matter of fact, something similar has been done recently while
>> adding support for JH7110.
>
> Every patch should stand on its own and at this point nothing uses it.
> We apply this patch #1 and we add dead code, unused case.
>
>>
>> In particular, commit [1] changed this binding before the JH7110
>> compatible was introduced in a subsequent patch. On a closer look that
>> commit made a statement which is not entirely correct:
>>
>> "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb)
>> reset signals"
>>
>> That's because stmmaceth is also optional in dwmac's driver, hence the
>> correct message would have been:
>>
>> "[...] may require one (stmmaceth OR ahb) [...]"
>
> Driver does not describe the hardware. The bindings do, so according to
> that description all supported hardware required MAC reset (stmmaceth).
> Otherwise please point me to any hardware which skips MAC reset and
> requires AHB reset instead (not future hardware, but current).
I might have found one (arch/arm/boot/dts/allwinner/sun6i-a31.dtsi):
gmac: ethernet@1c30000 {
compatible = "allwinner,sun7i-a20-gmac";
[...]
resets = <&ccu RST_AHB1_EMAC>;
reset-names = "stmmaceth";
[...]
};
It's possible the 'stmmaceth' naming has been used instead of 'ahb' just
to avoid updating the binding, but that's just an assumption looking at
RST_AHB1_EMAC.
I will proceed with the squash for v3.
Thanks for clarifying,
Cristian
^ permalink raw reply
* Re: [PATCH RFC net-next v2 1/2] netdevsim: implement DPLL for subsystem selftests
From: Jiri Pirko @ 2023-10-31 10:59 UTC (permalink / raw)
To: Michal Michalik
Cc: netdev, vadim.fedorenko, arkadiusz.kubalewski, jonathan.lemon,
pabeni, poros, milena.olech, mschmidt, linux-clk, bvanassche,
kuba, davem, edumazet
In-Reply-To: <20231030165326.24453-2-michal.michalik@intel.com>
Mon, Oct 30, 2023 at 05:53:25PM CET, michal.michalik@intel.com wrote:
>DPLL subsystem integration tests require a module which mimics the
>behavior of real driver which supports DPLL hardware. To fully test the
>subsystem the netdevsim is amended with DPLL implementation.
>
>Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>---
> drivers/net/Kconfig | 1 +
> drivers/net/netdevsim/Makefile | 2 +-
> drivers/net/netdevsim/dpll.c | 438 ++++++++++++++++++++++++++++++++++++++
> drivers/net/netdevsim/dpll.h | 81 +++++++
> drivers/net/netdevsim/netdev.c | 20 ++
> drivers/net/netdevsim/netdevsim.h | 4 +
> 6 files changed, 545 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/netdevsim/dpll.c
> create mode 100644 drivers/net/netdevsim/dpll.h
>
>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>index af0da4b..633ec89 100644
>--- a/drivers/net/Kconfig
>+++ b/drivers/net/Kconfig
>@@ -626,6 +626,7 @@ config NETDEVSIM
> depends on PSAMPLE || PSAMPLE=n
> depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
> select NET_DEVLINK
>+ select DPLL
> help
> This driver is a developer testing tool and software model that can
> be used to test various control path networking APIs, especially
>diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>index f8de93b..f338ffb 100644
>--- a/drivers/net/netdevsim/Makefile
>+++ b/drivers/net/netdevsim/Makefile
>@@ -3,7 +3,7 @@
> obj-$(CONFIG_NETDEVSIM) += netdevsim.o
>
> netdevsim-objs := \
>- netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
>+ netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
>
> ifeq ($(CONFIG_BPF_SYSCALL),y)
> netdevsim-objs += \
>diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
>new file mode 100644
>index 0000000..050f68e
>--- /dev/null
>+++ b/drivers/net/netdevsim/dpll.c
>@@ -0,0 +1,438 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2023, Intel Corporation.
>+ * Author: Michal Michalik <michal.michalik@intel.com>
>+ */
>+#include "dpll.h"
>+
>+static struct dpll_pin_properties *
>+create_pin_properties(const char *label, enum dpll_pin_type type,
Please make sure to follow the namespace prefix notation in your patch
everywhere, functions, structs, defines.
"nsim_"
>+ unsigned long caps, u32 freq_supp_num, u64 fmin, u64 fmax)
>+{
>+ struct dpll_pin_frequency *freq_supp;
>+ struct dpll_pin_properties *pp;
>+
>+ pp = kzalloc(sizeof(*pp), GFP_KERNEL);
>+ if (!pp)
>+ return ERR_PTR(-ENOMEM);
>+
>+ freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
>+ if (!freq_supp)
>+ goto err;
>+ *freq_supp =
>+ (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY_RANGE(fmin, fmax);
Drop the cast.
>+
>+ pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
>+ pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
>+ pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
>+ pp->freq_supported_num = freq_supp_num;
>+ pp->freq_supported = freq_supp;
>+ pp->capabilities = caps;
>+ pp->type = type;
>+
>+ return pp;
>+err:
>+ kfree(pp);
>+ return ERR_PTR(-ENOMEM);
>+}
>+
>+static struct dpll_pd *create_dpll_pd(int temperature, enum dpll_mode mode)
>+{
>+ struct dpll_pd *pd;
>+
>+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>+ if (!pd)
>+ return ERR_PTR(-ENOMEM);
>+
>+ pd->temperature = temperature;
>+ pd->mode = mode;
>+
>+ return pd;
>+}
>+
>+static struct pin_pd *create_pin_pd(u64 frequency, u32 prio,
>+ enum dpll_pin_direction direction)
>+{
>+ struct pin_pd *pd;
>+
>+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>+ if (!pd)
>+ return ERR_PTR(-ENOMEM);
>+
>+ pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
>+ pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
>+ pd->frequency = frequency;
>+ pd->direction = direction;
>+ pd->prio = prio;
>+
>+ return pd;
>+}
>+
>+static int
>+dds_ops_mode_get(const struct dpll_device *dpll, void *dpll_priv,
>+ enum dpll_mode *mode, struct netlink_ext_ack *extack)
>+{
>+ *mode = ((struct dpll_pd *)(dpll_priv))->mode;
Please have variable assigned by dpll_priv instead of this.
Also, fix in the rest of the code.
>+ return 0;
>+};
>+
>+static bool
>+dds_ops_mode_supported(const struct dpll_device *dpll, void *dpll_priv,
>+ const enum dpll_mode mode,
>+ struct netlink_ext_ack *extack)
>+{
>+ return true;
>+};
>+
>+static int
>+dds_ops_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
>+ enum dpll_lock_status *status,
>+ struct netlink_ext_ack *extack)
>+{
>+ if (((struct dpll_pd *)dpll_priv)->mode == DPLL_MODE_MANUAL)
>+ *status = DPLL_LOCK_STATUS_LOCKED;
>+ else
>+ *status = DPLL_LOCK_STATUS_UNLOCKED;
I don't understand the logic of this function. According to mode you
return if status is locked or not? For this, you should expose a debugfs
knob so the user can emulate changes of the HW.
>+ return 0;
>+};
>+
>+static int
>+dds_ops_temp_get(const struct dpll_device *dpll, void *dpll_priv, s32 *temp,
>+ struct netlink_ext_ack *extack)
>+{
>+ *temp = ((struct dpll_pd *)dpll_priv)->temperature;
>+ return 0;
>+};
>+
>+static int
>+pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ const u64 frequency, struct netlink_ext_ack *extack)
>+{
>+ ((struct pin_pd *)pin_priv)->frequency = frequency;
>+ return 0;
>+};
>+
>+static int
>+pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ u64 *frequency, struct netlink_ext_ack *extack)
>+{
>+ *frequency = ((struct pin_pd *)pin_priv)->frequency;
>+ return 0;
>+};
>+
>+static int
>+pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ const enum dpll_pin_direction direction,
>+ struct netlink_ext_ack *extack)
>+{
>+ ((struct pin_pd *)pin_priv)->direction = direction;
>+ return 0;
>+};
>+
>+static int
>+pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ enum dpll_pin_direction *direction,
>+ struct netlink_ext_ack *extack)
>+{
>+ *direction = ((struct pin_pd *)pin_priv)->direction;
>+ return 0;
>+};
>+
>+static int
>+pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_pin *parent_pin, void *parent_priv,
>+ enum dpll_pin_state *state,
>+ struct netlink_ext_ack *extack)
>+{
>+ *state = ((struct pin_pd *)pin_priv)->state_pin;
>+ return 0;
>+};
>+
>+static int
>+pin_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ enum dpll_pin_state *state,
>+ struct netlink_ext_ack *extack)
>+{
>+ *state = ((struct pin_pd *)pin_priv)->state_dpll;
>+ return 0;
>+};
>+
>+static int
>+pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_pin *parent_pin, void *parent_priv,
>+ const enum dpll_pin_state state,
>+ struct netlink_ext_ack *extack)
>+{
>+ ((struct pin_pd *)pin_priv)->state_pin = state;
>+ return 0;
>+};
>+
>+static int
>+pin_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ const enum dpll_pin_state state,
>+ struct netlink_ext_ack *extack)
>+{
>+ ((struct pin_pd *)pin_priv)->state_dpll = state;
>+ return 0;
>+};
>+
>+static int
>+pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ u32 *prio, struct netlink_ext_ack *extack)
>+{
>+ *prio = ((struct pin_pd *)pin_priv)->prio;
>+ return 0;
>+};
>+
>+static int
>+pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>+ const struct dpll_device *dpll, void *dpll_priv,
>+ const u32 prio, struct netlink_ext_ack *extack)
>+{
>+ ((struct pin_pd *)pin_priv)->prio = prio;
>+ return 0;
>+};
>+
>+static void
>+free_pin_properties(struct dpll_pin_properties *pp)
>+{
>+ if (pp) {
How exactly pp could be null?
>+ kfree(pp->board_label);
>+ kfree(pp->panel_label);
>+ kfree(pp->package_label);
>+ kfree(pp->freq_supported);
>+ kfree(pp);
>+ }
>+}
>+
>+static struct dpll_device_ops dds_ops = {
>+ .mode_get = dds_ops_mode_get,
>+ .mode_supported = dds_ops_mode_supported,
>+ .lock_status_get = dds_ops_lock_status_get,
>+ .temp_get = dds_ops_temp_get,
>+};
>+
>+static struct dpll_pin_ops pin_ops = {
>+ .frequency_set = pin_frequency_set,
>+ .frequency_get = pin_frequency_get,
>+ .direction_set = pin_direction_set,
>+ .direction_get = pin_direction_get,
>+ .state_on_pin_get = pin_state_on_pin_get,
>+ .state_on_dpll_get = pin_state_on_dpll_get,
>+ .state_on_pin_set = pin_state_on_pin_set,
>+ .state_on_dpll_set = pin_state_on_dpll_set,
>+ .prio_get = pin_prio_get,
>+ .prio_set = pin_prio_set,
>+};
>+
>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid)
>+{
>+ /* Create EEC DPLL */
>+ dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
"#define DPLLS_CLOCK_ID 234"
You guys always come up with some funky way of making up ids and names.
Why this can't be randomly generated u64?
>+ THIS_MODULE);
>+ if (IS_ERR(dpll->dpll_e))
>+ goto dpll_e;
>+ dpll->dpll_e_pd = create_dpll_pd(EEC_DPLL_TEMPERATURE,
>+ DPLL_MODE_AUTOMATIC);
>+ if (IS_ERR(dpll->dpll_e))
>+ goto dpll_e_pd;
>+ if (dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &dds_ops,
>+ (void *)dpll->dpll_e_pd))
Please avoid pointless casts. (void *) cast for arg of type (void *) are
especially pointless. Please make sure to fix this in the rest of the
code as well.
>+ goto e_reg;
>+
>+ /* Create PPS DPLL */
>+ dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>+ THIS_MODULE);
>+ if (IS_ERR(dpll->dpll_p))
>+ goto dpll_p;
>+ dpll->dpll_p_pd = create_dpll_pd(PPS_DPLL_TEMPERATURE,
>+ DPLL_MODE_MANUAL);
>+ if (IS_ERR(dpll->dpll_p_pd))
>+ goto dpll_p_pd;
>+ if (dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &dds_ops,
You always use "int err" to store the return value of function calls
returning 0/-EXXX like this one.
>+ (void *)dpll->dpll_p_pd))
>+ goto p_reg;
>+
>+ /* Create first pin (GNSS) */
>+ dpll->pp_gnss = create_pin_properties("GNSS", DPLL_PIN_TYPE_GNSS,
>+ PIN_GNSS_CAPABILITIES,
>+ 1, DPLL_PIN_FREQUENCY_1_HZ,
>+ DPLL_PIN_FREQUENCY_1_HZ);
>+ if (IS_ERR(dpll->pp_gnss))
>+ goto pp_gnss;
>+ dpll->p_gnss = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_GNSS,
>+ THIS_MODULE,
>+ dpll->pp_gnss);
>+ if (IS_ERR(dpll->p_gnss))
>+ goto p_gnss;
>+ dpll->p_gnss_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>+ PIN_GNSS_PRIORITY,
>+ DPLL_PIN_DIRECTION_INPUT);
>+ if (IS_ERR(dpll->p_gnss_pd))
>+ goto p_gnss_pd;
>+ if (dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+ (void *)dpll->p_gnss_pd))
>+ goto e_gnss_reg;
>+
>+ /* Create second pin (PPS) */
>+ dpll->pp_pps = create_pin_properties("PPS", DPLL_PIN_TYPE_EXT,
>+ PIN_PPS_CAPABILITIES,
>+ 1, DPLL_PIN_FREQUENCY_1_HZ,
>+ DPLL_PIN_FREQUENCY_1_HZ);
>+ if (IS_ERR(dpll->pp_pps))
>+ goto pp_pps;
>+ dpll->p_pps = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_PPS, THIS_MODULE,
>+ dpll->pp_pps);
>+ if (IS_ERR(dpll->p_pps))
>+ goto p_pps;
>+ dpll->p_pps_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>+ PIN_PPS_PRIORITY,
>+ DPLL_PIN_DIRECTION_INPUT);
>+ if (IS_ERR(dpll->p_pps_pd))
>+ goto p_pps_pd;
>+ if (dpll_pin_register(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+ (void *)dpll->p_pps_pd))
>+ goto e_pps_reg;
>+ if (dpll_pin_register(dpll->dpll_p, dpll->p_pps, &pin_ops,
>+ (void *)dpll->p_pps_pd))
>+ goto p_pps_reg;
>+
>+ return 0;
>+
>+p_pps_reg:
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+ (void *)dpll->p_pps_pd);
>+e_pps_reg:
>+ kfree(dpll->p_pps_pd);
>+p_pps_pd:
>+ dpll_pin_put(dpll->p_pps);
>+p_pps:
>+ free_pin_properties(dpll->pp_pps);
>+pp_pps:
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+ (void *)dpll->p_gnss_pd);
>+e_gnss_reg:
>+ kfree(dpll->p_gnss_pd);
>+p_gnss_pd:
>+ dpll_pin_put(dpll->p_gnss);
>+p_gnss:
>+ free_pin_properties(dpll->pp_gnss);
>+pp_gnss:
>+ dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>+p_reg:
>+ kfree(dpll->dpll_p_pd);
>+dpll_p_pd:
>+ dpll_device_put(dpll->dpll_p);
>+dpll_p:
>+ dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>+e_reg:
>+ kfree(dpll->dpll_e_pd);
>+dpll_e_pd:
>+ dpll_device_put(dpll->dpll_e);
>+dpll_e:
>+ return -1;
>+}
>+
>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll)
>+{
>+ /* Free GNSS pin */
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+ (void *)dpll->p_gnss_pd);
>+ dpll_pin_put(dpll->p_gnss);
>+ free_pin_properties(dpll->pp_gnss);
>+ kfree(dpll->p_gnss_pd);
>+
>+ /* Free PPS pin */
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+ (void *)dpll->p_pps_pd);
>+ dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &pin_ops,
>+ (void *)dpll->p_pps_pd);
>+ dpll_pin_put(dpll->p_pps);
>+ free_pin_properties(dpll->pp_pps);
>+ kfree(dpll->p_pps_pd);
>+
>+ /* Free DPLL EEC */
>+ dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>+ dpll_device_put(dpll->dpll_e);
>+ kfree(dpll->dpll_e_pd);
>+
>+ /* Free DPLL PPS */
>+ dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>+ dpll_device_put(dpll->dpll_p);
>+ kfree(dpll->dpll_p_pd);
>+}
>+
>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index)
>+{
>+ char *name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
>+
>+ /* Get EEC DPLL */
>+ dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
>+ THIS_MODULE);
>+ if (IS_ERR(dpll->dpll_e))
>+ goto dpll;
>+
>+ /* Get PPS DPLL */
>+ dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>+ THIS_MODULE);
>+ if (IS_ERR(dpll->dpll_p))
>+ goto dpll;
>+
>+ /* Create Recovered clock pin (RCLK) */
>+ dpll->pp_rclk = create_pin_properties(name,
>+ DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>+ PIN_RCLK_CAPABILITIES, 1, 1e6,
>+ 125e6);
>+ if (IS_ERR(dpll->pp_rclk))
>+ goto dpll;
>+ dpll->p_rclk = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_RCLK + index,
>+ THIS_MODULE, dpll->pp_rclk);
>+ if (IS_ERR(dpll->p_rclk))
>+ goto p_rclk;
>+ dpll->p_rclk_pd = create_pin_pd(DPLL_PIN_FREQUENCY_10_MHZ,
>+ PIN_RCLK_PRIORITY,
>+ DPLL_PIN_DIRECTION_INPUT);
>+ if (IS_ERR(dpll->p_rclk_pd))
>+ goto p_rclk_pd;
>+ if (dpll_pin_register(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+ (void *)dpll->p_rclk_pd))
>+ goto dpll_e_reg;
>+ if (dpll_pin_register(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>+ (void *)dpll->p_rclk_pd))
>+ goto dpll_p_reg;
>+
>+ return 0;
>+
>+dpll_p_reg:
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+ (void *)dpll->p_rclk_pd);
>+dpll_e_reg:
>+ kfree(dpll->p_rclk_pd);
>+p_rclk_pd:
>+ dpll_pin_put(dpll->p_rclk);
>+p_rclk:
>+ free_pin_properties(dpll->pp_rclk);
>+dpll:
>+ return -1;
>+}
>+
>+void nsim_rclk_free(struct nsim_dpll_info *dpll)
>+{
>+ /* Free RCLK pin */
>+ dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+ (void *)dpll->p_rclk_pd);
>+ dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>+ (void *)dpll->p_rclk_pd);
>+ dpll_pin_put(dpll->p_rclk);
>+ free_pin_properties(dpll->pp_rclk);
>+ kfree(dpll->p_rclk_pd);
>+ dpll_device_put(dpll->dpll_e);
>+ dpll_device_put(dpll->dpll_p);
>+}
>diff --git a/drivers/net/netdevsim/dpll.h b/drivers/net/netdevsim/dpll.h
>new file mode 100644
>index 0000000..17db7f7
>--- /dev/null
>+++ b/drivers/net/netdevsim/dpll.h
>@@ -0,0 +1,81 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ * Copyright (c) 2023, Intel Corporation.
>+ * Author: Michal Michalik <michal.michalik@intel.com>
>+ */
>+
>+#ifndef NSIM_DPLL_H
>+#define NSIM_DPLL_H
Why you need a separate header for this? Just put necessary parts in
netdevsim.h and leave the rest in the .c file.
>+
>+#include <linux/types.h>
>+#include <linux/netlink.h>
>+
>+#include <linux/dpll.h>
>+#include <uapi/linux/dpll.h>
Why exactly do you need to include uapi header directly?
>+
>+#define EEC_DPLL_DEV 0
>+#define EEC_DPLL_TEMPERATURE 20
>+#define PPS_DPLL_DEV 1
>+#define PPS_DPLL_TEMPERATURE 30
>+#define DPLLS_CLOCK_ID 234
>+
>+#define PIN_GNSS 0
>+#define PIN_GNSS_CAPABILITIES 2 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE */
>+#define PIN_GNSS_PRIORITY 5
>+
>+#define PIN_PPS 1
>+#define PIN_PPS_CAPABILITIES 7 /* DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE
>+ * || DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>+ * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
You are kidding, correct? :)
>+ */
>+#define PIN_PPS_PRIORITY 6
>+
>+#define PIN_RCLK 2
>+#define PIN_RCLK_CAPABILITIES 6 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>+ * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
>+ */
>+#define PIN_RCLK_PRIORITY 7
>+
>+#define EEC_PINS_NUMBER 3
>+#define PPS_PINS_NUMBER 2
>+
>+struct dpll_pd {
Have not clue what do you mean by "pd".
>+ enum dpll_mode mode;
>+ int temperature;
>+};
>+
>+struct pin_pd {
>+ u64 frequency;
>+ enum dpll_pin_direction direction;
>+ enum dpll_pin_state state_pin;
>+ enum dpll_pin_state state_dpll;
>+ u32 prio;
>+};
>+
>+struct nsim_dpll_info {
Drop "info".
>+ bool owner;
>+
>+ struct dpll_device *dpll_e;
>+ struct dpll_pd *dpll_e_pd;
>+ struct dpll_device *dpll_p;
>+ struct dpll_pd *dpll_p_pd;
>+
>+ struct dpll_pin_properties *pp_gnss;
Why pointer? Just embed the properties struct, no?
>+ struct dpll_pin *p_gnss;
>+ struct pin_pd *p_gnss_pd;
>+
>+ struct dpll_pin_properties *pp_pps;
>+ struct dpll_pin *p_pps;
>+ struct pin_pd *p_pps_pd;
>+
>+ struct dpll_pin_properties *pp_rclk;
>+ struct dpll_pin *p_rclk;
>+ struct pin_pd *p_rclk_pd;
>+};
>+
>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid);
>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll);
>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index);
>+void nsim_rclk_free(struct nsim_dpll_info *dpll);
>+
>+#endif /* NSIM_DPLL_H */
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index aecaf5f..78a936f 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -25,6 +25,7 @@
> #include <net/udp_tunnel.h>
>
> #include "netdevsim.h"
>+#include "dpll.h"
>
> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
>@@ -344,6 +345,20 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
> if (err)
> goto err_ipsec_teardown;
> rtnl_unlock();
>+
>+ if (ns->nsim_dev_port->port_index == 0) {
Does not make any sense to treat port 0 any different.
Please, move the init of dpll device to drivers/net/netdevsim/dev.c
probably somewhere in nsim_drv_probe().
>+ err = nsim_dpll_init_owner(&ns->dpll,
>+ ns->nsim_dev->nsim_bus_dev->dev.id);
>+ if (err)
>+ goto err_ipsec_teardown;
>+ }
>+
>+ err = nsim_rclk_init(&ns->dpll, ns->nsim_dev->nsim_bus_dev->dev.id,
>+ ns->nsim_dev_port->port_index);
This is not related to netdev directly. Please move the pin init into
drivers/net/netdevsim/dev.c, probably somewhere inside
__nsim_dev_port_add()
Also, you don't call netdev_dpll_pin_set() and netdev_dpll_pin_clear().
That is actually what you should call here in netdev initialization.
>+
>+ if (err)
>+ goto err_ipsec_teardown;
>+
> return 0;
>
> err_ipsec_teardown:
>@@ -419,6 +434,11 @@ void nsim_destroy(struct netdevsim *ns)
> if (nsim_dev_port_is_pf(ns->nsim_dev_port))
> nsim_udp_tunnels_info_destroy(dev);
> mock_phc_destroy(ns->phc);
>+
>+ nsim_rclk_free(&ns->dpll);
>+ if (ns->nsim_dev_port->port_index == 0)
>+ nsim_dpll_free_owner(&ns->dpll);
>+
> free_netdev(dev);
> }
>
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index 028c825..3d0138a 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -26,6 +26,8 @@
> #include <net/xdp.h>
> #include <net/macsec.h>
>
>+#include "dpll.h"
>+
> #define DRV_NAME "netdevsim"
>
> #define NSIM_XDP_MAX_MTU 4000
>@@ -125,6 +127,8 @@ struct netdevsim {
> } udp_ports;
>
> struct nsim_ethtool ethtool;
>+
>+ struct nsim_dpll_info dpll;
> };
>
> struct netdevsim *
>--
>2.9.5
>
^ permalink raw reply
* Re: [devel-ipsec] [PATCH v2 ipsec-next 2/2] xfrm: fix source address in icmp error generation from IPsec gateway
From: Michael Richardson @ 2023-10-31 7:59 UTC (permalink / raw)
To: Antony Antony
Cc: antony.antony, Herbert Xu, netdev, devel, Jakub Kicinski,
David S. Miller
In-Reply-To: <ZT4zUnhvbW2VZlRm@Antony2201.local>
[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]
Antony Antony <antony@phenome.org> wrote:
> On Fri, Oct 27, 2023 at 09:30:07AM -0400, Michael Richardson via Devel
> wrote:
>>
>> Antony Antony via Devel <devel@linux-ipsec.org> wrote: > When enabling
>> support for xfrm lookup using reverse ICMP payload, > We have
>> identified an issue where the source address of the IPv4 e.g >
>> "Destination Host Unreachable" message is incorrect. The IPv6 appear >
>> to do the right thing.
>>
>> One thing that operators of routers with a multitude of interfaces
>> want to do is send all ICMP messages from a specific IP address.
>> Often the public address, that has the sane reverse DNS name.
> While it makes sense for routers with multiple interfaces, receiving
> ICMP errors from private addresses can be confusing. However, wouldn't
> this also make it more challenging to adhere to BCP 32 and BCP 38?
> Routing with multiple interfaces is tricky on Linux, especially when it
> comes to compliance with these BCPs.
Yes, that's why sending from a public, topically significant source address
is really important. Yet, many links are numbered in 1918 because..
> I wonder if a netfilter rule would be a solution for you, something
> like:
> I would love see a simple option instead of a SNAT hack. May be a
> routing rule that will choose sourse address for ICMP error code.
yeah, I really don't want to do SNAT stuff.
I'd like to have a flag on each interface that says to use the "global" ICMP
source or use the heuristic we have now. And then we need a way to set that
source address. Most routing platforms put a /32 address (and /128) on lo
(or a dummy) as the device's reachable address, and then spread that through
OSPF.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]
^ permalink raw reply
* Re: [PATCH] net: stmmac: Wait a bit for the reset to take effect
From: Serge Semin @ 2023-10-31 10:32 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Alexandre Torgue, Jose Abreu, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel@vger.kernel.org
In-Reply-To: <AS8P193MB1285DECD77863E02EF45828BE4A1A@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM>
On Mon, Oct 30, 2023 at 07:01:11AM +0100, Bernd Edlinger wrote:
> otherwise the synopsys_id value may be read out wrong,
> because the GMAC_VERSION register might still be in reset
> state, for at least 1 us after the reset is de-asserted.
From what have you got that delay value?
-Serge(y)
>
> Add a wait for 10 us before continuing to be on the safe side.
>
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5801f4d50f95..e485f4db3605 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7398,6 +7398,9 @@ int stmmac_dvr_probe(struct device *device,
> dev_err(priv->device, "unable to bring out of ahb reset: %pe\n",
> ERR_PTR(ret));
>
> + /* Wait a bit for the reset to take effect */
> + udelay(10);
> +
> /* Init MAC and get the capabilities */
> ret = stmmac_hw_init(priv);
> if (ret)
> --
> 2.39.2
>
>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-next 2/6] i40e: Remove _t suffix from enum type names
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:29 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Richard Cochran,
Brandeburg, Jesse, open list, Nguyen, Anthony L,
moderated list:INTEL ETHERNET DRIVERS, Keller, Jacob E,
Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-2-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Richard Cochran <richardcochran@gmail.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 2/6] i40e: Remove _t suffix from enum type names
>
> Enum type names should not be suffixed by '_t'. Either to use
> 'typedef enum name name_t' to so plain 'name_t var' instead of
> 'enum name_t var'.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 4 ++--
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 6 +++---
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
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