* Re: [PATCH] IB/IPoIB: Check the headroom size
From: Honggang LI @ 2017-04-25 10:57 UTC (permalink / raw)
To: Or Gerlitz
Cc: Erez Shitrit, Doug Ledford, Hefty, Sean, Hal Rosenstock,
Paolo Abeni, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Kernel, Linux Netdev List
In-Reply-To: <CAJ3xEMg4_2ph7QwPsUb-tX-K4d2ppkqz98sPzytsmBCK=29WHw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Apr 25, 2017 at 01:32:59PM +0300, Or Gerlitz wrote:
> On Tue, Apr 25, 2017 at 12:55 PM, Honggang LI <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > From: Honggang Li <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > Minimal hard_header_len set by bond_compute_features is ETH_HLEN, which
> > is smaller than IPOIB_HARD_LEN. ipoib_hard_header should check the
> > size of headroom to avoid skb_under_panic.
>
> sounds terrible, ipoib bonding is supported since ~2007, thanks for
> reporting on that.
>
> > [ 122.871493] ipoib_hard_header: skb->head= ffff8808179d9400, skb->data= ffff8808179d9420, skb_headroom= 0x20
> > [ 123.055400] bond0: Releasing backup interface mthca_ib1
> > [ 123.560529] bond_compute_features:1112 bond0 bond_dev->hard_header_len = 14
> > [ 123.568822] CPU: 0 PID: 12336 Comm: ifdown-ib Not tainted 4.9.0-debug #1
>
> did you generate this trace by calling dump_stack or this is existing
> kernel code.
I inserted dump_stack to print this stack for debug.
>
> > Fixes: fc791b633515 ('IB/ipoib: move back IB LL address into the hard header')
>
> this is more of WA to avoid some crash or failure but not fixing the
> actual problem
>
> Erez, can you comment?
>
> Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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-next 2/2] l2tp: define "l2tpeth" device type
From: James Chapman @ 2017-04-25 10:41 UTC (permalink / raw)
To: netdev; +Cc: Guillaume Nault
In-Reply-To: <f9c1eb350915a145978d7f5d485ad210c11ba5ca.1493035407.git.g.nault@alphalink.fr>
On 24/04/17 13:16, Guillaume Nault wrote:
> Export type of l2tpeth interfaces to userspace
> (/sys/class/net/<iface>/uevent).
>
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Acked-by: James Chapman <jchapman@katalix.com>
^ permalink raw reply
* Re: [PATCH net-next 1/2] l2tp: set name_assign_type for devices created by l2tp_eth.c
From: James Chapman @ 2017-04-25 10:40 UTC (permalink / raw)
To: netdev; +Cc: Guillaume Nault
In-Reply-To: <062e3c5c1c8b9d6ddea34de74e98c159f78a2f0b.1493035407.git.g.nault@alphalink.fr>
On 24/04/17 13:16, Guillaume Nault wrote:
> Export naming scheme used when creating l2tpeth interfaces
> (/sys/class/net/<iface>/name_assign_type). This let userspace know if
> the device's name has been generated automatically or defined manually.
>
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Acked-by: James Chapman <jchapman@katalix.com>
^ permalink raw reply
* Re: [PATCH] IB/IPoIB: Check the headroom size
From: Or Gerlitz @ 2017-04-25 10:32 UTC (permalink / raw)
To: Honggang LI, Erez Shitrit
Cc: Doug Ledford, Hefty, Sean, Hal Rosenstock, Paolo Abeni,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Kernel,
Linux Netdev List
In-Reply-To: <1493114155-12101-1-git-send-email-honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Apr 25, 2017 at 12:55 PM, Honggang LI <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> From: Honggang Li <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Minimal hard_header_len set by bond_compute_features is ETH_HLEN, which
> is smaller than IPOIB_HARD_LEN. ipoib_hard_header should check the
> size of headroom to avoid skb_under_panic.
sounds terrible, ipoib bonding is supported since ~2007, thanks for
reporting on that.
> [ 122.871493] ipoib_hard_header: skb->head= ffff8808179d9400, skb->data= ffff8808179d9420, skb_headroom= 0x20
> [ 123.055400] bond0: Releasing backup interface mthca_ib1
> [ 123.560529] bond_compute_features:1112 bond0 bond_dev->hard_header_len = 14
> [ 123.568822] CPU: 0 PID: 12336 Comm: ifdown-ib Not tainted 4.9.0-debug #1
did you generate this trace by calling dump_stack or this is existing
kernel code.
> Fixes: fc791b633515 ('IB/ipoib: move back IB LL address into the hard header')
this is more of WA to avoid some crash or failure but not fixing the
actual problem
Erez, can you comment?
Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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] IB/IPoIB: Check the headroom size
From: Yuval Shaia @ 2017-04-25 10:11 UTC (permalink / raw)
To: Honggang LI
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
pabeni-H+wXaHxf7aLQT0dZR+AlfA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493114155-12101-1-git-send-email-honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Apr 25, 2017 at 05:55:55PM +0800, Honggang LI wrote:
> From: Honggang Li <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Minimal hard_header_len set by bond_compute_features is ETH_HLEN, which
> is smaller than IPOIB_HARD_LEN. ipoib_hard_header should check the
> size of headroom to avoid skb_under_panic.
>
> [ 122.871493] ipoib_hard_header: skb->head= ffff8808179d9400, skb->data= ffff8808179d9420, skb_headroom= 0x20
> [ 123.055400] bond0: Releasing backup interface mthca_ib1
> [ 123.560529] bond_compute_features:1112 bond0 bond_dev->hard_header_len = 14
> [ 123.568822] CPU: 0 PID: 12336 Comm: ifdown-ib Not tainted 4.9.0-debug #1
> [ 123.576668] Hardware name: Dell Inc. PowerEdge R415/0GXH08, BIOS 2.0.2 10/22/2012
> [ 123.585284] ffffc90009027be8 ffffffff81362d6c ffff8808198b7000 0000000000010000
> [ 123.593845] ffffc90009027c50 ffffffffa06cf833 ffff8808198b7000 ffff8808198b78c0
> [ 123.602392] ffffc90009027c30 ffffffff815ed725 ffff8808158a9c00 00000000a67486bf
> [ 123.610926] Call Trace:
> [ 123.614454] [<ffffffff81362d6c>] dump_stack+0x63/0x87
> [ 123.620661] [<ffffffffa06cf833>] bond_compute_features.isra.42+0x243/0x260 [bonding]
> [ 123.629546] [<ffffffff815ed725>] ? call_netdevice_notifiers_info+0x35/0x60
> [ 123.637557] [<ffffffffa06d3a7b>] __bond_release_one+0x2db/0x530 [bonding]
> [ 123.645483] [<ffffffffa06d3ce0>] bond_release+0x10/0x20 [bonding]
> [ 123.652711] [<ffffffffa06de038>] bond_option_slaves_set+0xe8/0x130 [bonding]
> [ 123.660874] [<ffffffffa06df336>] __bond_opt_set+0xd6/0x320 [bonding]
> [ 123.668357] [<ffffffffa06df5d6>] bond_opt_tryset_rtnl+0x56/0xa0 [bonding]
> [ 123.676284] [<ffffffffa06dbba5>] bonding_sysfs_store_option+0x35/0x60 [bonding]
> [ 123.684748] [<ffffffff814b0bd8>] dev_attr_store+0x18/0x30
> [ 123.691311] [<ffffffff812b6c5a>] sysfs_kf_write+0x3a/0x50
> [ 123.697879] [<ffffffff812b678b>] kernfs_fop_write+0x10b/0x190
> [ 123.704801] [<ffffffff81231647>] __vfs_write+0x37/0x160
> [ 123.711213] [<ffffffff812f0235>] ? selinux_file_permission+0xe5/0x120
> [ 123.718856] [<ffffffff812e5a8b>] ? security_file_permission+0x3b/0xc0
> [ 123.726506] [<ffffffff81231d72>] vfs_write+0xb2/0x1b0
> [ 123.732776] [<ffffffff81003510>] ? syscall_trace_enter+0x1d0/0x2b0
> [ 123.740148] [<ffffffff812331c5>] SyS_write+0x55/0xc0
> [ 123.746288] [<ffffffff81003a47>] do_syscall_64+0x67/0x180
> [ 123.752846] [<ffffffff8170f7ab>] entry_SYSCALL64_slow_path+0x25/0x25
> [ 123.760421] bond0: last VLAN challenged slave mthca_ib1 left bond bond0 - VLAN blocking is removed
> [ 124.023489] dump_LL_RESERVED_SPACE, bond0, dev->hard_header_len = 0xe, dev->needed_headroom= 0x0, HH_DATA_MOD= 0x10
> [ 124.023490] dump_LL_RESERVED_SPACE, bond0, LL_RESERVED_SPACE(dev) = 0x10
> [ 124.023491] dump_LL_RESERVED_SPACE, bond0, dev->hard_header_len = 0xe, dev->needed_headroom= 0x0, HH_DATA_MOD= 0x10
> [ 124.023492] dump_LL_RESERVED_SPACE, bond0, LL_RESERVED_SPACE(dev) = 0x10
> [ 124.023494] arp_create:547 skb->head= ffff8808179dac00, skb->data= ffff8808179dac00, skb_headroom= 0x0, <NULL>
> [ 124.023495] arp_create:549 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
> [ 124.023496] arp_create:551 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
> [ 124.023497] arp_create:553 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
> [ 124.023498] arp_create:564 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, bond0
> [ 124.023500] ipoib_hard_header: skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10
> [ 124.023502] skbuff: skb_under_panic: text:ffffffffa040f6a9 len:80 put:20 head:ffff8808179dac00 data:ffff8808179dabf8 tail:0x48 end:0xc0 dev:bond0
> [ 124.023536] ------------[ cut here ]------------
> [ 124.023537] kernel BUG at net/core/skbuff.c:105!
> [ 124.023539] invalid opcode: 0000 [#1] SMP
> [ 124.023563] Modules linked in: bonding amd64_edac_mod edac_mce_amd edac_core kvm_amd kvm ib_mthca ipmi_ssif ipmi_devintf irqbypass ipmi_si dcdbas acpi_power_meter sp5100_tco ipmi_msghandler sg pcspkr i2c_piix4 k10temp shpchp acpi_cpufreq rpcrdma ib_isert iscsi_target_mod ib_iser libiscsi scsi_transport_iscsi ib_srpt target_core_mod ib_srp scsi_transport_srp ib_ipoib nfsd rdma_ucm auth_rpcgss ib_ucm nfs_acl ib_uverbs lockd grace ib_umad rdma_cm sunrpc ib_cm iw_cm ib_core ip_tables xfs libcrc32c sd_mod ata_generic pata_acpi mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm ahci drm libahci pata_atiixp serio_raw libata i2c_core bnx2 fjes dm_mirror dm_region_hash dm_log dm_mod
> [ 124.023567] CPU: 2 PID: 12265 Comm: ping Not tainted 4.9.0-debug #1
> [ 124.023567] Hardware name: Dell Inc. PowerEdge R415/0GXH08, BIOS 2.0.2 10/22/2012
> [ 124.023569] task: ffff880818214080 task.stack: ffffc900085e0000
> [ 124.023577] RIP: 0010:[<ffffffff817005c4>] [<ffffffff817005c4>] skb_panic+0x66/0x68
> [ 124.023578] RSP: 0018:ffffc900085e38e0 EFLAGS: 00010246
> [ 124.023578] RAX: 0000000000000085 RBX: ffff880816a72500 RCX: 0000000000000000
> [ 124.023579] RDX: 0000000000000000 RSI: 0000000000000296 RDI: 0000000000000296
> [ 124.023580] RBP: ffffc900085e3900 R08: 0000000000000085 R09: ffffffff82012ce5
> [ 124.023581] R10: 00000000000003ed R11: 0000000000000000 R12: ffff8808198b7368
> [ 124.023581] R13: 0000000000000608 R14: 000000000701de0a R15: ffff8808198b7000
> [ 124.023583] FS: 00002b3922409b00(0000) GS:ffff88083fc80000(0000) knlGS:0000000000000000
> [ 124.023584] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 124.023584] CR2: 00002ac965af0072 CR3: 0000000814472000 CR4: 00000000000006e0
> [ 124.023585] Stack:
> [ 124.023588] ffff8808179dabf8 0000000000000048 00000000000000c0 ffff8808198b7000
> [ 124.023590] ffffc900085e3910 ffffffff815dcb5d ffffc900085e3938 ffffffffa040f6a9
> [ 124.023592] ffff8808179dac10 ffff8808198b7368 000000000601de0a ffffc900085e3990
> [ 124.023592] Call Trace:
> [ 124.023598] [<ffffffff815dcb5d>] skb_push+0x3d/0x40
> [ 124.023607] [<ffffffffa040f6a9>] ipoib_hard_header+0x69/0x90 [ib_ipoib]
> [ 124.023611] [<ffffffff8166c7ee>] arp_create+0x2ae/0x3e0
> [ 124.023613] [<ffffffff8166cd28>] arp_send_dst.part.19+0x28/0x50
> [ 124.023615] [<ffffffff8166ce65>] arp_solicit+0x115/0x290
> [ 124.023618] [<ffffffff815e050c>] ? skb_clone+0x4c/0xa0
> [ 124.023619] [<ffffffff815dd92e>] ? __skb_clone+0x2e/0x140
> [ 124.023622] [<ffffffff815ff235>] neigh_probe+0x45/0x60
> [ 124.023624] [<ffffffff81600117>] __neigh_event_send+0xa7/0x230
> [ 124.023625] [<ffffffff8160081e>] neigh_resolve_output+0x12e/0x1c0
> [ 124.023628] [<ffffffff8163bc2b>] ip_finish_output2+0x14b/0x370
> [ 124.023630] [<ffffffff8163d2e6>] ip_finish_output+0x136/0x1e0
> [ 124.023632] [<ffffffff8163dd7e>] ip_output+0x6e/0xf0
> [ 124.023633] [<ffffffff8163d402>] ? __ip_local_out+0x72/0x120
> [ 124.023635] [<ffffffff8163d1b0>] ? ip_fragment.constprop.49+0x80/0x80
> [ 124.023636] [<ffffffff8163d4e5>] ip_local_out+0x35/0x40
> [ 124.023638] [<ffffffff8163e819>] ip_send_skb+0x19/0x40
> [ 124.023640] [<ffffffff8163e873>] ip_push_pending_frames+0x33/0x40
> [ 124.023641] [<ffffffff81665dfa>] raw_sendmsg+0x77a/0xb00
> [ 124.023644] [<ffffffff815e6131>] ? skb_recv_datagram+0x41/0x60
> [ 124.023645] [<ffffffff81665044>] ? raw_recvmsg+0x94/0x1d0
> [ 124.023650] [<ffffffff812e9280>] ? sock_has_perm+0x70/0x90
> [ 124.023653] [<ffffffff815d6502>] ? ___sys_recvmsg+0xf2/0x1f0
> [ 124.023655] [<ffffffff816753b7>] inet_sendmsg+0x67/0xa0
> [ 124.023657] [<ffffffff815d5aa8>] sock_sendmsg+0x38/0x50
> [ 124.023659] [<ffffffff815d5f62>] SYSC_sendto+0x102/0x190
> [ 124.023662] [<ffffffff8113ed6f>] ? __audit_syscall_entry+0xaf/0x100
> [ 124.023665] [<ffffffff81003510>] ? syscall_trace_enter+0x1d0/0x2b0
> [ 124.023667] [<ffffffff8113ef9b>] ? __audit_syscall_exit+0x1db/0x260
> [ 124.023669] [<ffffffff815d6b0e>] SyS_sendto+0xe/0x10
> [ 124.023670] [<ffffffff81003a47>] do_syscall_64+0x67/0x180
> [ 124.023673] [<ffffffff8170f7ab>] entry_SYSCALL64_slow_path+0x25/0x25
> [ 124.023688] Code: 00 00 48 89 44 24 10 8b 87 c8 00 00 00 48 89 44 24 08 48 8b 87 d8 00 00 00 48 c7 c7 50 83 ab 81 48 89 04 24 31 c0 e8 5f e6 a9 ff <0f> 0b 55 48 89 e5 0f 0b 55 48 89 e5 0f 0b 0f 1f 44 00 00 55 48
> [ 124.023690] RIP [<ffffffff817005c4>] skb_panic+0x66/0x68
> [ 124.023691] RSP <ffffc900085e38e0>
> [ 124.023696] ---[ end trace 95c238901cb322be ]---
> [ 124.026071] Kernel panic - not syncing: Fatal exception in interrupt
> [ 124.026368] Kernel Offset: disabled
> [ 124.644414] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
>
> Fixes: fc791b633515 ('IB/ipoib: move back IB LL address into the hard header')
> Reported-by: Norbert P <noe-PRwTpj6vllL463JZfw7VRw@public.gmane.org>
> Signed-off-by: Honggang Li <honli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index d1d3fb7..3668e1e 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -1161,6 +1161,9 @@ static int ipoib_hard_header(struct sk_buff *skb,
> {
> struct ipoib_header *header;
>
> + if (unlikely(skb_headroom(skb) < IPOIB_HARD_LEN))
> + return -EINVAL;
> +
> header = (struct ipoib_header *) skb_push(skb, sizeof *header);
>
> header->proto = htons(type);
> --
> 1.8.3.1
Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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] stmmac: Add support for SIMATIC IOT2000 platform
From: Jan Kiszka @ 2017-04-25 10:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev,
Linux Kernel Mailing List, Sascha Weisenberger
In-Reply-To: <42fd4a72-526c-cbd9-52db-9d8b495035ee@siemens.com>
On 2017-04-25 12:07, Jan Kiszka wrote:
> On 2017-04-25 11:46, Andy Shevchenko wrote:
>> On Tue, Apr 25, 2017 at 12:00 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-04-25 09:30, Andy Shevchenko wrote:
>>>> On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2017-04-24 23:27, Andy Shevchenko wrote:
>>>>>> On Mon, Apr 24, 2017 at 10:27 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>> The IOT2000 is industrial controller platform, derived from the Intel
>>>>>>> Galileo Gen2 board. The variant IOT2020 comes with one LAN port, the
>>>>>>> IOT2040 has two of them. They can be told apart based on the board asset
>>>>>>> tag in the DMI table.
>>>>
>>>>>>> + const char *asset_tag;
>>>>>>
>>>>>> I guess this is redundant. See below.
>>>>>>
>>>>>>> + {
>>>>>>> + .name = "SIMATIC IOT2000",
>>>>>>> + .asset_tag = "6ES7647-0AA00-0YA2",
>>>>>>> + .func = 6,
>>>>>>> + .phy_addr = 1,
>>>>>>> + },
>>>>>>
>>>>>> The below has same definition disregard on asset_tag.
>>>>>>
>>>>>
>>>>> There is a small difference in the asset tag, just not at the last digit
>>>>> where one may expect it, look:
>>>>>
>>>>> ...-0YA2 -> IOT2020
>>>>> ...-1YA2 -> IOT2040
>>>>
>>>> Yes. And how does it change my statement? You may use one record here
>>>> instead of two.
>>>
>>> How? Please be more verbose in your comments.
>>
>> {
>> .name = "SIMATIC IOT2000",
>> .func = 6,
>> .phy_addr = 1,
>> },
>> {
>> .name = "SIMATIC IOT2000",
>> .func = 7,
>> .phy_addr = 1,
>> },
>>
>> That's all what you need.
>
> Nope. Again: the asset tag is the way to tell both apart AND to ensure
> that we do not match on future devices.
To be more verbose: your version (which is our old one) would even
enable the second, not connected port on the IOT2020. Incorrectly. Plus
the risk to match different future devices.
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply
* Re: [PATCH] stmmac: Add support for SIMATIC IOT2000 platform
From: Jan Kiszka @ 2017-04-25 10:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev,
Linux Kernel Mailing List, Sascha Weisenberger
In-Reply-To: <CAHp75VfqC166E57nZRsx97EQCCGnqYTk+4nRX3H2-rsnCohPfQ@mail.gmail.com>
On 2017-04-25 11:46, Andy Shevchenko wrote:
> On Tue, Apr 25, 2017 at 12:00 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-04-25 09:30, Andy Shevchenko wrote:
>>> On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2017-04-24 23:27, Andy Shevchenko wrote:
>>>>> On Mon, Apr 24, 2017 at 10:27 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> The IOT2000 is industrial controller platform, derived from the Intel
>>>>>> Galileo Gen2 board. The variant IOT2020 comes with one LAN port, the
>>>>>> IOT2040 has two of them. They can be told apart based on the board asset
>>>>>> tag in the DMI table.
>>>
>>>>>> + const char *asset_tag;
>>>>>
>>>>> I guess this is redundant. See below.
>>>>>
>>>>>> + {
>>>>>> + .name = "SIMATIC IOT2000",
>>>>>> + .asset_tag = "6ES7647-0AA00-0YA2",
>>>>>> + .func = 6,
>>>>>> + .phy_addr = 1,
>>>>>> + },
>>>>>
>>>>> The below has same definition disregard on asset_tag.
>>>>>
>>>>
>>>> There is a small difference in the asset tag, just not at the last digit
>>>> where one may expect it, look:
>>>>
>>>> ...-0YA2 -> IOT2020
>>>> ...-1YA2 -> IOT2040
>>>
>>> Yes. And how does it change my statement? You may use one record here
>>> instead of two.
>>
>> How? Please be more verbose in your comments.
>
> {
> .name = "SIMATIC IOT2000",
> .func = 6,
> .phy_addr = 1,
> },
> {
> .name = "SIMATIC IOT2000",
> .func = 7,
> .phy_addr = 1,
> },
>
> That's all what you need.
Nope. Again: the asset tag is the way to tell both apart AND to ensure
that we do not match on future devices.
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply
* Re: [PATCH] net: hso: fix module unloading
From: Johan Hovold @ 2017-04-25 10:04 UTC (permalink / raw)
To: Andreas Kemnade
Cc: davem, joe, peter, linux-usb, netdev, linux-kernel,
Discussions about the Letux Kernel
In-Reply-To: <1493061519-15785-1-git-send-email-andreas@kemnade.info>
On Mon, Apr 24, 2017 at 09:18:39PM +0200, Andreas Kemnade wrote:
> keep tty driver until usb driver is unregistered
> rmmod hso
> produces traces like this without that:
Yeah, a blatant use-after-free.
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Johan Hovold <johan@kernel.org>
> ---
> drivers/net/usb/hso.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 2e66340..b75e23f 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -3293,9 +3293,9 @@ static void __exit hso_exit(void)
> pr_info("unloaded\n");
>
> tty_unregister_driver(tty_drv);
> - put_tty_driver(tty_drv);
> /* deregister the usb driver */
> usb_deregister(&hso_driver);
> + put_tty_driver(tty_drv);
> }
>
> /* Module definitions */
Thanks,
Johan
^ permalink raw reply
* [PATCH] IB/IPoIB: Check the headroom size
From: Honggang LI @ 2017-04-25 9:55 UTC (permalink / raw)
To: dledford, sean.hefty, hal.rosenstock, pabeni, linux-rdma
Cc: linux-kernel, netdev, Honggang Li
From: Honggang Li <honli@redhat.com>
Minimal hard_header_len set by bond_compute_features is ETH_HLEN, which
is smaller than IPOIB_HARD_LEN. ipoib_hard_header should check the
size of headroom to avoid skb_under_panic.
[ 122.871493] ipoib_hard_header: skb->head= ffff8808179d9400, skb->data= ffff8808179d9420, skb_headroom= 0x20
[ 123.055400] bond0: Releasing backup interface mthca_ib1
[ 123.560529] bond_compute_features:1112 bond0 bond_dev->hard_header_len = 14
[ 123.568822] CPU: 0 PID: 12336 Comm: ifdown-ib Not tainted 4.9.0-debug #1
[ 123.576668] Hardware name: Dell Inc. PowerEdge R415/0GXH08, BIOS 2.0.2 10/22/2012
[ 123.585284] ffffc90009027be8 ffffffff81362d6c ffff8808198b7000 0000000000010000
[ 123.593845] ffffc90009027c50 ffffffffa06cf833 ffff8808198b7000 ffff8808198b78c0
[ 123.602392] ffffc90009027c30 ffffffff815ed725 ffff8808158a9c00 00000000a67486bf
[ 123.610926] Call Trace:
[ 123.614454] [<ffffffff81362d6c>] dump_stack+0x63/0x87
[ 123.620661] [<ffffffffa06cf833>] bond_compute_features.isra.42+0x243/0x260 [bonding]
[ 123.629546] [<ffffffff815ed725>] ? call_netdevice_notifiers_info+0x35/0x60
[ 123.637557] [<ffffffffa06d3a7b>] __bond_release_one+0x2db/0x530 [bonding]
[ 123.645483] [<ffffffffa06d3ce0>] bond_release+0x10/0x20 [bonding]
[ 123.652711] [<ffffffffa06de038>] bond_option_slaves_set+0xe8/0x130 [bonding]
[ 123.660874] [<ffffffffa06df336>] __bond_opt_set+0xd6/0x320 [bonding]
[ 123.668357] [<ffffffffa06df5d6>] bond_opt_tryset_rtnl+0x56/0xa0 [bonding]
[ 123.676284] [<ffffffffa06dbba5>] bonding_sysfs_store_option+0x35/0x60 [bonding]
[ 123.684748] [<ffffffff814b0bd8>] dev_attr_store+0x18/0x30
[ 123.691311] [<ffffffff812b6c5a>] sysfs_kf_write+0x3a/0x50
[ 123.697879] [<ffffffff812b678b>] kernfs_fop_write+0x10b/0x190
[ 123.704801] [<ffffffff81231647>] __vfs_write+0x37/0x160
[ 123.711213] [<ffffffff812f0235>] ? selinux_file_permission+0xe5/0x120
[ 123.718856] [<ffffffff812e5a8b>] ? security_file_permission+0x3b/0xc0
[ 123.726506] [<ffffffff81231d72>] vfs_write+0xb2/0x1b0
[ 123.732776] [<ffffffff81003510>] ? syscall_trace_enter+0x1d0/0x2b0
[ 123.740148] [<ffffffff812331c5>] SyS_write+0x55/0xc0
[ 123.746288] [<ffffffff81003a47>] do_syscall_64+0x67/0x180
[ 123.752846] [<ffffffff8170f7ab>] entry_SYSCALL64_slow_path+0x25/0x25
[ 123.760421] bond0: last VLAN challenged slave mthca_ib1 left bond bond0 - VLAN blocking is removed
[ 124.023489] dump_LL_RESERVED_SPACE, bond0, dev->hard_header_len = 0xe, dev->needed_headroom= 0x0, HH_DATA_MOD= 0x10
[ 124.023490] dump_LL_RESERVED_SPACE, bond0, LL_RESERVED_SPACE(dev) = 0x10
[ 124.023491] dump_LL_RESERVED_SPACE, bond0, dev->hard_header_len = 0xe, dev->needed_headroom= 0x0, HH_DATA_MOD= 0x10
[ 124.023492] dump_LL_RESERVED_SPACE, bond0, LL_RESERVED_SPACE(dev) = 0x10
[ 124.023494] arp_create:547 skb->head= ffff8808179dac00, skb->data= ffff8808179dac00, skb_headroom= 0x0, <NULL>
[ 124.023495] arp_create:549 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
[ 124.023496] arp_create:551 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
[ 124.023497] arp_create:553 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, <NULL>
[ 124.023498] arp_create:564 skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10, bond0
[ 124.023500] ipoib_hard_header: skb->head= ffff8808179dac00, skb->data= ffff8808179dac10, skb_headroom= 0x10
[ 124.023502] skbuff: skb_under_panic: text:ffffffffa040f6a9 len:80 put:20 head:ffff8808179dac00 data:ffff8808179dabf8 tail:0x48 end:0xc0 dev:bond0
[ 124.023536] ------------[ cut here ]------------
[ 124.023537] kernel BUG at net/core/skbuff.c:105!
[ 124.023539] invalid opcode: 0000 [#1] SMP
[ 124.023563] Modules linked in: bonding amd64_edac_mod edac_mce_amd edac_core kvm_amd kvm ib_mthca ipmi_ssif ipmi_devintf irqbypass ipmi_si dcdbas acpi_power_meter sp5100_tco ipmi_msghandler sg pcspkr i2c_piix4 k10temp shpchp acpi_cpufreq rpcrdma ib_isert iscsi_target_mod ib_iser libiscsi scsi_transport_iscsi ib_srpt target_core_mod ib_srp scsi_transport_srp ib_ipoib nfsd rdma_ucm auth_rpcgss ib_ucm nfs_acl ib_uverbs lockd grace ib_umad rdma_cm sunrpc ib_cm iw_cm ib_core ip_tables xfs libcrc32c sd_mod ata_generic pata_acpi mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm ahci drm libahci pata_atiixp serio_raw libata i2c_core bnx2 fjes dm_mirror dm_region_hash dm_log dm_mod
[ 124.023567] CPU: 2 PID: 12265 Comm: ping Not tainted 4.9.0-debug #1
[ 124.023567] Hardware name: Dell Inc. PowerEdge R415/0GXH08, BIOS 2.0.2 10/22/2012
[ 124.023569] task: ffff880818214080 task.stack: ffffc900085e0000
[ 124.023577] RIP: 0010:[<ffffffff817005c4>] [<ffffffff817005c4>] skb_panic+0x66/0x68
[ 124.023578] RSP: 0018:ffffc900085e38e0 EFLAGS: 00010246
[ 124.023578] RAX: 0000000000000085 RBX: ffff880816a72500 RCX: 0000000000000000
[ 124.023579] RDX: 0000000000000000 RSI: 0000000000000296 RDI: 0000000000000296
[ 124.023580] RBP: ffffc900085e3900 R08: 0000000000000085 R09: ffffffff82012ce5
[ 124.023581] R10: 00000000000003ed R11: 0000000000000000 R12: ffff8808198b7368
[ 124.023581] R13: 0000000000000608 R14: 000000000701de0a R15: ffff8808198b7000
[ 124.023583] FS: 00002b3922409b00(0000) GS:ffff88083fc80000(0000) knlGS:0000000000000000
[ 124.023584] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 124.023584] CR2: 00002ac965af0072 CR3: 0000000814472000 CR4: 00000000000006e0
[ 124.023585] Stack:
[ 124.023588] ffff8808179dabf8 0000000000000048 00000000000000c0 ffff8808198b7000
[ 124.023590] ffffc900085e3910 ffffffff815dcb5d ffffc900085e3938 ffffffffa040f6a9
[ 124.023592] ffff8808179dac10 ffff8808198b7368 000000000601de0a ffffc900085e3990
[ 124.023592] Call Trace:
[ 124.023598] [<ffffffff815dcb5d>] skb_push+0x3d/0x40
[ 124.023607] [<ffffffffa040f6a9>] ipoib_hard_header+0x69/0x90 [ib_ipoib]
[ 124.023611] [<ffffffff8166c7ee>] arp_create+0x2ae/0x3e0
[ 124.023613] [<ffffffff8166cd28>] arp_send_dst.part.19+0x28/0x50
[ 124.023615] [<ffffffff8166ce65>] arp_solicit+0x115/0x290
[ 124.023618] [<ffffffff815e050c>] ? skb_clone+0x4c/0xa0
[ 124.023619] [<ffffffff815dd92e>] ? __skb_clone+0x2e/0x140
[ 124.023622] [<ffffffff815ff235>] neigh_probe+0x45/0x60
[ 124.023624] [<ffffffff81600117>] __neigh_event_send+0xa7/0x230
[ 124.023625] [<ffffffff8160081e>] neigh_resolve_output+0x12e/0x1c0
[ 124.023628] [<ffffffff8163bc2b>] ip_finish_output2+0x14b/0x370
[ 124.023630] [<ffffffff8163d2e6>] ip_finish_output+0x136/0x1e0
[ 124.023632] [<ffffffff8163dd7e>] ip_output+0x6e/0xf0
[ 124.023633] [<ffffffff8163d402>] ? __ip_local_out+0x72/0x120
[ 124.023635] [<ffffffff8163d1b0>] ? ip_fragment.constprop.49+0x80/0x80
[ 124.023636] [<ffffffff8163d4e5>] ip_local_out+0x35/0x40
[ 124.023638] [<ffffffff8163e819>] ip_send_skb+0x19/0x40
[ 124.023640] [<ffffffff8163e873>] ip_push_pending_frames+0x33/0x40
[ 124.023641] [<ffffffff81665dfa>] raw_sendmsg+0x77a/0xb00
[ 124.023644] [<ffffffff815e6131>] ? skb_recv_datagram+0x41/0x60
[ 124.023645] [<ffffffff81665044>] ? raw_recvmsg+0x94/0x1d0
[ 124.023650] [<ffffffff812e9280>] ? sock_has_perm+0x70/0x90
[ 124.023653] [<ffffffff815d6502>] ? ___sys_recvmsg+0xf2/0x1f0
[ 124.023655] [<ffffffff816753b7>] inet_sendmsg+0x67/0xa0
[ 124.023657] [<ffffffff815d5aa8>] sock_sendmsg+0x38/0x50
[ 124.023659] [<ffffffff815d5f62>] SYSC_sendto+0x102/0x190
[ 124.023662] [<ffffffff8113ed6f>] ? __audit_syscall_entry+0xaf/0x100
[ 124.023665] [<ffffffff81003510>] ? syscall_trace_enter+0x1d0/0x2b0
[ 124.023667] [<ffffffff8113ef9b>] ? __audit_syscall_exit+0x1db/0x260
[ 124.023669] [<ffffffff815d6b0e>] SyS_sendto+0xe/0x10
[ 124.023670] [<ffffffff81003a47>] do_syscall_64+0x67/0x180
[ 124.023673] [<ffffffff8170f7ab>] entry_SYSCALL64_slow_path+0x25/0x25
[ 124.023688] Code: 00 00 48 89 44 24 10 8b 87 c8 00 00 00 48 89 44 24 08 48 8b 87 d8 00 00 00 48 c7 c7 50 83 ab 81 48 89 04 24 31 c0 e8 5f e6 a9 ff <0f> 0b 55 48 89 e5 0f 0b 55 48 89 e5 0f 0b 0f 1f 44 00 00 55 48
[ 124.023690] RIP [<ffffffff817005c4>] skb_panic+0x66/0x68
[ 124.023691] RSP <ffffc900085e38e0>
[ 124.023696] ---[ end trace 95c238901cb322be ]---
[ 124.026071] Kernel panic - not syncing: Fatal exception in interrupt
[ 124.026368] Kernel Offset: disabled
[ 124.644414] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
Fixes: fc791b633515 ('IB/ipoib: move back IB LL address into the hard header')
Reported-by: Norbert P <noe@physik.uzh.ch>
Signed-off-by: Honggang Li <honli@redhat.com>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index d1d3fb7..3668e1e 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1161,6 +1161,9 @@ static int ipoib_hard_header(struct sk_buff *skb,
{
struct ipoib_header *header;
+ if (unlikely(skb_headroom(skb) < IPOIB_HARD_LEN))
+ return -EINVAL;
+
header = (struct ipoib_header *) skb_push(skb, sizeof *header);
header->proto = htons(type);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] stmmac: Add support for SIMATIC IOT2000 platform
From: Andy Shevchenko @ 2017-04-25 9:46 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev,
Linux Kernel Mailing List, Sascha Weisenberger
In-Reply-To: <d7216317-e749-1347-972f-28dfe57038c0@siemens.com>
On Tue, Apr 25, 2017 at 12:00 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-04-25 09:30, Andy Shevchenko wrote:
>> On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-04-24 23:27, Andy Shevchenko wrote:
>>>> On Mon, Apr 24, 2017 at 10:27 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> The IOT2000 is industrial controller platform, derived from the Intel
>>>>> Galileo Gen2 board. The variant IOT2020 comes with one LAN port, the
>>>>> IOT2040 has two of them. They can be told apart based on the board asset
>>>>> tag in the DMI table.
>>
>>>>> + const char *asset_tag;
>>>>
>>>> I guess this is redundant. See below.
>>>>
>>>>> + {
>>>>> + .name = "SIMATIC IOT2000",
>>>>> + .asset_tag = "6ES7647-0AA00-0YA2",
>>>>> + .func = 6,
>>>>> + .phy_addr = 1,
>>>>> + },
>>>>
>>>> The below has same definition disregard on asset_tag.
>>>>
>>>
>>> There is a small difference in the asset tag, just not at the last digit
>>> where one may expect it, look:
>>>
>>> ...-0YA2 -> IOT2020
>>> ...-1YA2 -> IOT2040
>>
>> Yes. And how does it change my statement? You may use one record here
>> instead of two.
>
> How? Please be more verbose in your comments.
{
.name = "SIMATIC IOT2000",
.func = 6,
.phy_addr = 1,
},
{
.name = "SIMATIC IOT2000",
.func = 7,
.phy_addr = 1,
},
That's all what you need.
>> Got it, though asset_tag here is redundant as well.
> It's not as it is the only differentiating criteria to tell the
> two-ports variant apart from the one-port (and to avoid confusing it
> with any potential future variant).
And why exactly is it needed?
Sorry, but I don't see any reason to blow the code for no benefit.
> We could leave out the name, but I
> kept it for documentation purposes.
Nobody prevents you to add a comment.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Sergei Shtylyov @ 2017-04-25 9:41 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list, Ray Jui,
Scott Branden, Jon Mason
In-Reply-To: <20170424215022.30382-3-eric@anholt.net>
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus has a single amac controller connected to the B53 switch with 2
> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
> the external ethernet jacks.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> arch/arm/boot/dts/bcm-cygnus.dtsi | 60 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 009f1346b817..318899df9972 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
[...]
> @@ -295,6 +345,16 @@
> status = "disabled";
> };
>
> + eth0: enet@18042000 {
Oh, and this one should be named "ethernet", according to the DT spec.
> + compatible = "brcm,amac";
> + reg = <0x18042000 0x1000>,
> + <0x18110000 0x1000>;
> + reg-names = "amac_base", "idm_base";
> + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> + max-speed = <1000>;
> + status = "disabled";
> + };
> +
> nand: nand@18046000 {
> compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
> reg = <0x18046000 0x600>, <0xf8105408 0x600>,
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH net-next] rhashtable: remove insecure_max_entries param
From: Florian Westphal @ 2017-04-25 9:41 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
no users in the tree, insecure_max_entries is always set to
ht->p.max_size * 2 in rhtashtable_init().
Replace only spot that uses it with a ht->p.max_size check.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/linux/rhashtable.h | 6 ++----
lib/rhashtable.c | 6 ------
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index ae87dcdf52d2..ae93b65d13d7 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -125,7 +125,6 @@ struct rhashtable;
* @key_len: Length of key
* @key_offset: Offset of key in struct to be hashed
* @head_offset: Offset of rhash_head in struct to be hashed
- * @insecure_max_entries: Maximum number of entries (may be exceeded)
* @max_size: Maximum size while expanding
* @min_size: Minimum size while shrinking
* @nulls_base: Base value to generate nulls marker
@@ -140,7 +139,6 @@ struct rhashtable_params {
size_t key_len;
size_t key_offset;
size_t head_offset;
- unsigned int insecure_max_entries;
unsigned int max_size;
unsigned int min_size;
u32 nulls_base;
@@ -329,8 +327,8 @@ static inline bool rht_grow_above_100(const struct rhashtable *ht,
static inline bool rht_grow_above_max(const struct rhashtable *ht,
const struct bucket_table *tbl)
{
- return ht->p.insecure_max_entries &&
- atomic_read(&ht->nelems) >= ht->p.insecure_max_entries;
+ return ht->p.max_size &&
+ (atomic_read(&ht->nelems) / 2u) >= ht->p.max_size;
}
/* The bucket lock is selected based on the hash and protects mutations
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index d22a5ef109fb..f3b82e0d417b 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -961,12 +961,6 @@ int rhashtable_init(struct rhashtable *ht,
if (params->max_size)
ht->p.max_size = rounddown_pow_of_two(params->max_size);
- if (params->insecure_max_entries)
- ht->p.insecure_max_entries =
- rounddown_pow_of_two(params->insecure_max_entries);
- else
- ht->p.insecure_max_entries = ht->p.max_size * 2;
-
ht->p.min_size = max(ht->p.min_size, HASH_MIN_SIZE);
if (params->nelem_hint)
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Sergei Shtylyov @ 2017-04-25 9:40 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list, Ray Jui,
Scott Branden, Jon Mason
In-Reply-To: <20170424215022.30382-3-eric@anholt.net>
Hello.
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus has a single amac controller connected to the B53 switch with 2
> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
> the external ethernet jacks.
My spell checker trips on "amac" and "ethernet" -- perhaps they need
capitalization?
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> arch/arm/boot/dts/bcm-cygnus.dtsi | 60 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
> index 009f1346b817..318899df9972 100644
> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
> @@ -142,6 +142,56 @@
> interrupts = <0>;
> };
>
> + mdio: mdio@18002000 {
> + compatible = "brcm,iproc-mdio";
> + reg = <0x18002000 0x8>;
> + #size-cells = <1>;
> + #address-cells = <0>;
> +
> + gphy0: eth-gphy@0 {
The node anmes must be generic, the DT spec has standardized
"ethernet-phy" name for this case.
> + reg = <0>;
> + max-speed = <1000>;
> + };
> +
> + gphy1: eth-gphy@1 {
> + reg = <1>;
> + max-speed = <1000>;
> + };
> + };
[...]
> @@ -295,6 +345,16 @@
> status = "disabled";
> };
>
> + eth0: enet@18042000 {
> + compatible = "brcm,amac";
> + reg = <0x18042000 0x1000>,
> + <0x18110000 0x1000>;
> + reg-names = "amac_base", "idm_base";
I don't think "_base" suffixes are necessary here.
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] net: dsa: b53: Add compatible strings for the Cygnus-family BCM11360.
From: Sergei Shtylyov @ 2017-04-25 9:35 UTC (permalink / raw)
To: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
netdev, Rob Herring, Mark Rutland, devicetree
Cc: Scott Branden, Jon Mason, Ray Jui, linux-kernel,
bcm-kernel-feedback-list, linux-arm-kernel
In-Reply-To: <20170424215022.30382-2-eric@anholt.net>
Hello!
On 4/25/2017 12:50 AM, Eric Anholt wrote:
> Cygnus is a small family of SoCs, of which we currently have
> devicetree for BCM11360 and BCM58300. The 11360's B53 is mostly the
> same as 58xx, just requiring a tiny bit of setup that was previously
> missing.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> Documentation/devicetree/bindings/net/dsa/b53.txt | 3 +++
> drivers/net/dsa/b53/b53_srab.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/b53.txt b/Documentation/devicetree/bindings/net/dsa/b53.txt
> index d6c6e41648d4..49c93d3c0839 100644
> --- a/Documentation/devicetree/bindings/net/dsa/b53.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/b53.txt
> @@ -29,6 +29,9 @@ Required properties:
> "brcm,bcm58625-srab"
> "brcm,bcm88312-srab" and the mandatory "brcm,nsp-srab string
>
> + For the BCM11360 SoC, must be:
> + "brcm,bcm11360-srab" and the mandatory "brcm,cygnus-srab string
Missing closing quote here and above?
[...]
MBR, Sergei
^ permalink raw reply
* Re: xdp_redirect ifindex vs port. Was: best API for returning/setting egress port?
From: Jesper Dangaard Brouer @ 2017-04-25 9:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Andy Gospodarek, Daniel Borkmann,
Alexei Starovoitov, netdev@vger.kernel.org,
xdp-newbies@vger.kernel.org, John Fastabend, brouer
In-Reply-To: <20170420171006.GA97067@ast-mbp.thefacebook.com>
On Thu, 20 Apr 2017 10:10:08 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Thu, Apr 20, 2017 at 08:10:51AM +0200, Jesper Dangaard Brouer wrote:
> > On Wed, 19 Apr 2017 19:56:13 -0700
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > > On Thu, Apr 20, 2017 at 12:51:31AM +0200, Daniel Borkmann wrote:
> > > >
> > > > Is there a concrete reason that all the proposed future cases like sockets
> > > > have to be handled within the very same XDP_REDIRECT return code? F.e. why
> > > > not XDP_TX_NIC that only assumes ifindex as proposed in the patch, and future
> > > > ones would get a different return code f.e. XDP_TX_SK only handling sockets
> > > > when we get there implementation-wise?
> > >
> > > yeah. let's keep redirect to sockets, tunnels, crypto and exotic things
> > > out of this discussion.
> > > XDP_REDIRECT should assume L2 raw packet is being redirected to another L2 netdev.
> > > If we make it too generic it will lose performance.
> > >
> > > For cls_bpf the ifindex concept is symmetric. The program can access it as
> > > skb->ifindex on receive and can redirect to another ifindex via bpf_redirect() helper.
> > > Since netdev is not locked, it's actually big plus, since container management
> > > control plane can simply delete netns+veth and it goes away. The program can
> > > have dangling ifindex (if control plane is buggy and didn't update the bpf side),
> > > but it's harmless. Packets that redirect to non-existing ifindex are dropped.
> > > This approach already understood and works well, so for XDP I suggest to use
> > > the same approach initially before starting to reinvent the wheel.
> > > struct xdp_md needs ifindex field and xdp_redirect() helper that redirects
> > > to L2 netdev only. That's it. Simple and easy.
> > > I think the main use cases in John's and Jesper's minds is something like
> > > xdpswitch where packets are coming from VMs and from physical eths and
> > > being redirected to either physical eth or to VM via upcoming vhost+xdp support.
> > > This xdp_md->ifindex + xdp_redirect(to_ifindex) will solve it just fine.
> > >
> > > Once we have vhost+xdp and all other bits implemented, we must come back
> > > to this discussion about having port mapping table. As I mentioned
> > > during netconf I think it's very useful, but I don't think we should
> > > gate vhost+xdp and xdp_redirect work on this discussion.
> > > As far as this port mapping table we would need 'port' field in xdp_md as well
> > > and xdp_redirect_port() done via helper or via extra 'out_port' field in xdp_md
> > > plus another XDP_REDIRECT_PORT action code.
> >
> > Guess it would be easier to talk about if we name it "ingress_port" and
> > "egress_port".
> >
> > > The actual port table (array) should be populated by user space with netdevs
> > > and these netdev will have their refcnt incremented. Then we'll have discussion
> > > what to do with netdev_unregister notifiers, whether they should be auto-removed
> > > from port table or bpf should have a chance to be notified and act on it.
> > > Such port mapping will allow us to optimize inevitable call
> > > dev_get_by_index_rcu(dev_net(skb->dev), ri->ifindex);
> > > away, since netdevs will be stored in the port table and direct deref
> > > port_map_array[xdp_md->out_port] will give us target netdev quickly.
> >
> > I agree with above paragraph, and is happy that you can see that this
> > will actually be faster than using ifindex'es.
> >
> > > It's nice optimization and there are other more powerful optimizations we
> > > can do with such port table (since we will know in advance which netdevs
> > > the program will be redirecting too), but I still think we should do
> > > ifindex based xdp_redirect first and only add this port table later.
> >
> > No, we cannot first do an ifindex based xdp_redirect. The point of the
> > port table is to sandbox which ports XDP can use.
>
> hmm. port table cannot sandbox the ports. The only thing it does
> from 'safety' point of view is moving the checks from run-time into
> static insertion time.
> So the checks that we would do on netdev after looking it up
> based on ifindex are the same checks we will do at insertion time
> into port table. The user space will insert/delete them live
> from that port table, so from program point of view it's exactly
> the same as ifindex. The ports can disappear and can be added
> while the program is running.
I agree, that from the eBPF programs point of view using an ifindex or a
port number is the same. And I do like this model, that this is just a
number seem from bpf. It provides a clean separation between the
kernel and ebpf program world.
> Note the very first bpf patchset years ago contained the port table
> abstraction. ovs has concept of vports as well. These two very
> different projects needed port table to provide a layer of
> indirection between ifindex==netdev and virtual port number.
> This is still the case and I'd like to see this port table to be
> implemented for both cls_bpf and xdp. In that sense xdp is not
> special.
Glad to hear you want to see this implemented, I will start coding on
this then. Good point with cls_bpf, I was planning to make this port
table strongly connected to XDP, guess I should also think of cls_bpf.
> > XDP is different than TC/cls_bpf, as it does "bypass", there is no
> > other layer that can stop or inspect these packets. The TC hooks
> > redirect into the network stack, which have all the usual facilities
> > available for filtering, inspection and debugging what is going on
> > (e.g. tcpdump works for TC redirect).
>
> not true. when bpf_redirect() drops the packet due to incorrect ifindex
> that packet disappears without a trace. No tcpdump and no counter.
> And this is fine. We can add tracepoint there for debugging,
> but it wasn't a problem for anyone who's using it today, so it's
> 'nice to have', but certainly not mandatory.
I'm not worried about the DROP case, I agree that is fine (as you also
say). The problem is unintentionally sending a packet to a wrong
ifindex. This is clearly an eBPF program error, BUT with XDP this
becomes a very hard to debug program error. With TC-redirect/cls_bpf
we can tcpdump the packets, with XDP there is no visibility into this
happening (the NSA is going to love this "feature"). Maybe we could add
yet-another tracepoint to allow debugging this. My proposal that we
simply remove the possibility for such program errors, by as you say
move the validation from run-time into static insertion-time, via a
port table.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH v3] brcmfmac: Make skb header writable before use
From: James Hughes @ 2017-04-25 9:15 UTC (permalink / raw)
To: Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
brcm80211-dev-list.pdl-dY08KVG/lbpWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA
Cc: James Hughes
The driver was making changes to the skb_header without
ensuring it was writable (i.e. uncloned).
This patch also removes some boiler plate header size
checking/adjustment code as that is also handled by the
skb_cow_header function used to make header writable.
Please apply to 4.12, important fix.
This patch depends on
brcmfmac: Ensure pointer correctly set if skb data location changes
Signed-off-by: James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
Acked-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
Changes in v2
Makes the _cow_ call at the entry point of the skb in to the
stack, means only needs to be done once, and error handling
is easier.
Split a separate minor bug fix off to a separate patch (which
this patch depends on)
Changes in v3
Minor change to the 'if' logic to reduce patch size as per
maintainers request.
Flagged as important fix for 4.12 in commit message
.../net/wireless/broadcom/brcm80211/brcmfmac/core.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 9b7c19a508ac..433f2c8408e9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -211,22 +211,13 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
goto done;
}
- /* Make sure there's enough room for any header */
- if (skb_headroom(skb) < drvr->hdrlen) {
- struct sk_buff *skb2;
^ permalink raw reply related
* Re: [RFC 1/4] netlink: make extended ACK setting NULL-friendly
From: Daniel Borkmann @ 2017-04-25 9:12 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
kubakici, oss-drivers
In-Reply-To: <20170425080644.122536-2-jakub.kicinski@netronome.com>
On 04/25/2017 10:06 AM, Jakub Kicinski wrote:
> As we propagate extended ack reporting throughout various paths in
> the kernel it may happen that the same function is called with the
> extended ack parameter passed as NULL. Make the NL_SET_ERR_MSG()
> macro simply print the message to the logs if that happens.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> include/linux/netlink.h | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 8d2a8924705c..b59cfbf2e2c7 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -86,10 +86,14 @@ struct netlink_ext_ack {
> * Currently string formatting is not supported (due
> * to the lack of an output buffer.)
> */
> -#define NL_SET_ERR_MSG(extack, msg) do { \
> - static const char _msg[] = (msg); \
> - \
> - (extack)->_msg = _msg; \
> +#define NL_SET_ERR_MSG(extack, msg) do { \
> + struct netlink_ext_ack *_extack = (extack); \
> + static const char _msg[] = (msg); \
> + \
> + if (_extack) \
> + _extack->_msg = _msg; \
> + else \
> + pr_info("%s\n", _msg); \
> } while (0)
>
> extern void netlink_kernel_release(struct sock *sk);
Probably makes sense to have a NL_MOD_SET_ERR_MSG(), which
then also prepends a KBUILD_MODNAME ": " string to the
message (similar to pr_*()), so that it's easier to identify
whether the error came from a specific driver or rather
common core code?
^ permalink raw reply
* Re: [PATCH net v3] bridge: ebtables: fix reception of frames DNAT-ed to bridge device/port
From: Pablo Neira Ayuso @ 2017-04-25 9:10 UTC (permalink / raw)
To: Linus Lüssing
Cc: netdev, bridge, linux-kernel, coreteam, netfilter-devel,
Jozsef Kadlecsik, David S . Miller
In-Reply-To: <20170419194733.19006-1-linus.luessing@c0d3.blue>
On Wed, Apr 19, 2017 at 09:47:33PM +0200, Linus Lüssing wrote:
> When trying to redirect bridged frames to the bridge device itself or
> a bridge port (brouting) via the dnat target then this currently fails:
>
> The ethernet destination of the frame is dnat'ed to the MAC address of
> the bridge device or port just fine. However, the IP code drops it in
> the beginning of ip_input.c/ip_rcv() as the dnat target left
> the skb->pkt_type as PACKET_OTHERHOST.
>
> Fixing this by resetting skb->pkt_type to an appropriate type after
> dnat'ing.
Applied, thanks.
One comment below.
> @@ -18,11 +19,32 @@ static unsigned int
> ebt_dnat_tg(struct sk_buff *skb, const struct xt_action_param *par)
> {
> const struct ebt_nat_info *info = par->targinfo;
> + struct net_device *dev;
>
> if (!skb_make_writable(skb, 0))
> return EBT_DROP;
>
> ether_addr_copy(eth_hdr(skb)->h_dest, info->mac);
> +
> + if (is_multicast_ether_addr(info->mac)) {
> + if (is_broadcast_ether_addr(info->mac))
> + skb->pkt_type = PACKET_BROADCAST;
> + else
> + skb->pkt_type = PACKET_MULTICAST;
> + } else {
> + rcu_read_lock();
I'm going to manually remove this explicit rcu_read_lock() here, no
need to resend. We're guaranteed to run from packet path with read
side lock from netfilter hooks. So we just save some cycles from
running this unnecessary nesting.
Let me know if I'm missing anything. Thanks!
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
From: Jeff Kirsher @ 2017-04-25 9:07 UTC (permalink / raw)
To: Brown, Aaron F, Benjamin Poirier, Neftin, Sasha, David S Miller,
stephen
Cc: netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
Kirsher@f1.synalogic.ca, Stefan Priebe
In-Reply-To: <309B89C4C689E141A5FF6A0C5FB2118B8C5EF4F7@ORSMSX101.amr.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 5660 bytes --]
On Tue, 2017-04-25 at 07:10 +0000, Brown, Aaron F wrote:
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.
> > org] On
> > Behalf Of Benjamin Poirier
> > Sent: Monday, April 24, 2017 12:10 PM
> > To: Neftin, Sasha <sasha.neftin@intel.com>
> > Cc: Kirsher@f1.synalogic.ca; Stefan Priebe <s.priebe@profihost.ag>;
> > netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> > Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return
> > uninitialized
> > stats
> >
> > Sasha, please use reply-all to keep everyone in cc (including
> > me...).
> >
> > On 2017/04/24 11:17, Neftin, Sasha wrote:
> > > On 4/23/2017 15:53, Neftin, Sasha wrote:
> > > > -----Original Message-----
> > > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osu
> > > > osl.org]
> >
> > On Behalf Of Benjamin Poirier
> > > > Sent: Saturday, April 22, 2017 00:20
> > > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > > > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
> > > > Stefan
> >
> > Priebe <s.priebe@profihost.ag>
> > > > Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return
> > > > uninitialized
> >
> > stats
> > > >
> > > > Some statistics passed to ethtool are garbage because
> >
> > e1000e_get_stats64() doesn't write them, for example:
> > tx_heartbeat_errors.
> > This leaks kernel memory to userspace and confuses users.
> > > >
> > > > Do like ixgbe and use dev_get_stats() which first zeroes out
> >
> > rtnl_link_stats64.
> > > >
> > > > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > > ---
> > > > drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> >
> > b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > index 7aff68a4a4df..f117b90cdc2f 100644
> > > > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > @@ -2063,7 +2063,7 @@ static void
> > > > e1000_get_ethtool_stats(struct
> >
> > net_device *netdev,
> > > > pm_runtime_get_sync(netdev->dev.parent);
> > > > - e1000e_get_stats64(netdev, &net_stats);
> > > > + dev_get_stats(netdev, &net_stats);
> > > > pm_runtime_put_sync(netdev->dev.parent);
> > > > --
> > > > 2.12.2
> > > >
> > > > _______________________________________________
> > > > Intel-wired-lan mailing list
> > > > Intel-wired-lan@lists.osuosl.org
> > > > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> > >
> > > Hello,
> > >
> > > We would like to not accept this patch. Suggested generic method
> > > '*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64'
> > > method
> >
> > which
> > > eventually calls e1000e_get_stats64 (netdev.c) - so there is same
> > > functionality. Also, see that 'e1000e_get_stats64' method in
> > > netdev.c (line
> >
> > No, it's not the same functionality because dev_get_stats() does a
> > memset on the rtnl_link_stats64 struct.
> >
> > > 5928) calls 'memset' with 0's before update statistics. Local
> > > sanity check
> >
> > I don't see any memset in e1000e_get_stats64(). What kernel version
> > are
> > you looking at?
>
> The call to memset was removed from the upstream kernel with:
> -------------------------------------------------------------------
> -----------------
> commit 5944701df90d9577658e2354cc27c4ceaeca30fe
> Author: stephen hemminger <stephen@networkplumber.org>
> Date: Fri Jan 6 19:12:53 2017 -0800
>
> net: remove useless memset's in drivers get_stats64
>
> In dev_get_stats() the statistic structure storage has already
> been
> zeroed. Therefore network drivers do not need to call memset()
> again.
> ...
> < changes to other drivers snipped out >
> ...
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/int
> index 723025b..79651eb 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device
> *netdev,
> {
> struct e1000_adapter *adapter = netdev_priv(netdev);
>
> - memset(stats, 0, sizeof(struct rtnl_link_stats64));
> spin_lock(&adapter->stats64_lock);
> e1000e_update_stats(adapter);
> /* Fill out the OS statistics structure */
> -------------------------------------------------------------------
> -----------------
>
> This also is where the bad counters start to show up for e1000e for
> my test systems. From this driver on I see (very) large values for
> tx_dropped, rx_over_errors and tx_fifo_errors on driver load (even
> before bringing the interface up. It seems the memset is not so
> useless for this driver after all. Would simply reverting the e1000e
> portion of this patch resolve the issue?
Looks like Aaron beat me to the punch on pointing out that we had this
very code in there before. It appears that Stephen's
assertion/assumption was incorrect about the stats structure being
zero'd out, which is why we are seeing the issue.
I have no issue reverting Stephen's earlier patch, or do we want to
pursue why the stats structure is not zero'd out and resolve that
instead. Either way, just want to make sure we are all on the same
page as to the right solution so that we do not end up repeating this
in the future.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers
From: Niklas Cassel @ 2017-04-25 9:06 UTC (permalink / raw)
To: Florian Fainelli, Giuseppe CAVALLARO, Andrew Lunn, Yegor Yefremov
Cc: netdev, linux-omap@vger.kernel.org, Grygorii Strashko,
N, Mugunthan V, Rami Rosen, Fabrice GASNIER, rmk+kernel
In-Reply-To: <68e1a232-2fde-da42-d49f-20ea1fec6dbf@gmail.com>
On 04/18/2017 06:40 PM, Florian Fainelli wrote:
> On 04/18/2017 06:23 AM, Niklas Cassel wrote:
>> On 01/04/2017 03:33 PM, Florian Fainelli wrote:
>>> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>>>> Peppe, any thoughts on this?
>>>>>
>>>>> I share what you say.
>>>>>
>>>>> In sum, the EEE management inside the stmmac is:
>>>>>
>>>>> - the driver looks at own HW cap register if EEE is supported
>>>>>
>>>>> (indeed the user could keep disable EEE if bugged on some HW
>>>>> + Alex, Fabrice: we had some patches for this to propose where we
>>>>> called the phy_ethtool_set_eee to disable feature at phy
>>>>> level
>>>>>
>>>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>>> partners are EEE capable.
>>>>>
>>>>> - If all matches the EEE is actually initialized.
>>>>>
>>>>> the logic above should be respected when use ethtool, hmm, I will
>>>>> check the stmmac_ethtool_op_set_eee asap.
>>>>>
>>>>> Hoping this is useful
>>>>
>>>> This is definitively useful, the only part that I am struggling to
>>>> understand in phy_init_eee() is this:
>>>>
>>>> eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>>> MDIO_MMD_AN);
>>>> if (eee_adv <= 0)
>>>> goto eee_exit_err;
>>>>
>>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>>>> the time we call phy_init_eee(), then we cannot complete the EEE
>>>> configuration at the PHY level, and presumably we should abort the EEE
>>>> configuration at the MAC level.
>>>>
>>>> While this condition makes sense if e.g: you are re-negotiating the link
>>>> with your partner for instance and if EEE was already advertised, the
>>>> very first time this function is called, it seems to be like we should
>>>> skip the check, because phy_init_eee() should actually tell us if, as a
>>>> result of a successful check, we should be setting EEE as something we
>>>> advertise?
>>>>
>>>> Do you remember what was the logic behind this check when you added it?
>>>
>>> Peppe, can you remember why phy_init_eee() was written in a way that you
>>> need to have already locally advertised EEE for the function to
>>> successfully return? Thank you!
>>>
>>
>> I'm curious about this as well.
>>
>> I can get EEE to work with stmmac, but to be able to turn EEE on,
>> I need to set eee advertise via ethtool first.
>> (Tested with 2 different PHYs from different vendors, with their
>> PHY specific driver enabled.)
>>
>> Is this the same for all PHYs or are there certain PHYs/PHY drivers
>> that actually advertise eee by default?
>
> It depends on whether the PHY driver takes care of the EEE advertisement
> part for your or not, most drivers probably don't do that.
>
>> (From reading this mail thread there seems to be a suggestion that
>> the broadcom PHY driver might advertise eee by default.)
>
> As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do
> advertise EEE by default in order to validate the first check done in
> phy_init_eee(), but that's the only reason really.
>
> Since we have not been able to get a straight answer from Peppe about
> why there is this initial check, I think the cleanest path moving
> forward is the following:
>
> - rename phy_init_eee() into something like: phy_can_do_eee() and remove
> the first check on whether EEE is already advertised because that's
> precisely what we are trying to determine with this function
>
> - Ethernet MAC drivers keep calling phy_can_do_eee() (formerly
> phy_init_eee()) during their adjust_link callback in order to
> re-negotiate EEE with their link partner, just like they should call
> phy_ethtool_set_eee() to really enable EEE the first time they want to
> enable EEE with the link partner
>
> - remove the part from phy_init_eee() that tries to stop the PHY TX
> clock and provide a set of helpers: phy_can_stop_tx_clk() and
> phy_set_stop_tx_clk() which will take care of that
>
> Does that look reasonable?
Sounds very reasonable to me.
However, if I look specifically at the stmmac driver,
stmmac_eee_init() is called from adjust_link callback.
If we replace phy_init_eee() with a phy_can_do_eee()
in stmmac_eee_init(), then the driver will enable
EEE in the IP, and setup timers etc.
If I understand you correctly, the code in the adjust_link
callback should call phy_can_do_eee() so that the PHY
re-negotiate EEE with the link partner.
You will still need to use ethtool to actually enable it in the
PHY (call the new phy_init_eee()).
(Which sounds good, since we probably do not want to suddenly
enable EEE by default in a lot of drivers.)
The issue that I see is that we probably do not want to
setup timers, etc. in the adjust_link callback before
EEE has actually been enabled, so it might not be as
easy as just replacing phy_init_eee() with phy_can_do_eee()
in some drivers.
^ permalink raw reply
* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
From: Daniel Borkmann @ 2017-04-25 9:05 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
kubakici, oss-drivers
In-Reply-To: <20170425080644.122536-1-jakub.kicinski@netronome.com>
On 04/25/2017 10:06 AM, Jakub Kicinski wrote:
> Hi!
>
> This series is an attempt to make XDP more user friendly by
> enabling exploiting the recently added netlink extended ACK
> reporting to carry messages to user space.
>
> I made iproute2 parse the extended messages and have it showing
> the errors like this:
>
> # ip link set dev p4p1 xdp obj ipip_prepend.o sec ".text"
> RTNETLINK answers: Invalid argument (MTU too large w/ XDP enabled)
>
> Where the message is coming directly from the driver. There could
> still be a bit of a leap for a complete novice from the message
> above to the right settings. I wonder if it would be worthwhile
But still 100x better than the current situation. ;) I really
like the series, thanks for working on this!
> adding #defines for the most common configuration conflicts?
> Sharing the messages verbatim between drivers could make them easier
> to google.
Makes sense, once more drivers adapt to this reporting, these
messages could be consolidated.
> Also - is anyone working on adding proper extack support to iproute2?
> The code I have right now is a bit of a hack...
>
> Jakub Kicinski (4):
> netlink: make extended ACK setting NULL-friendly
> xdp: propagate extended ack to XDP setup
> nfp: make use of extended ack message reporting
> virtio_net: make use of extended ack message reporting
>
> drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 ++-
> .../net/ethernet/netronome/nfp/nfp_net_common.c | 22 +++++++++++++---------
> .../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 4 ++--
> drivers/net/virtio_net.c | 11 +++++++----
> include/linux/netdevice.h | 10 ++++++++--
> include/linux/netlink.h | 12 ++++++++----
> net/core/dev.c | 5 ++++-
> net/core/rtnetlink.c | 13 ++++++++-----
> 8 files changed, 52 insertions(+), 28 deletions(-)
>
^ permalink raw reply
* Re: [PATCH] stmmac: Add support for SIMATIC IOT2000 platform
From: Jan Kiszka @ 2017-04-25 9:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev,
Linux Kernel Mailing List, Sascha Weisenberger
In-Reply-To: <CAHp75VcTzBL4D6nobfDAPQOfEEp-RzBp4BANKYDSaaEtGCyj9A@mail.gmail.com>
On 2017-04-25 09:30, Andy Shevchenko wrote:
> On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-04-24 23:27, Andy Shevchenko wrote:
>>> On Mon, Apr 24, 2017 at 10:27 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> The IOT2000 is industrial controller platform, derived from the Intel
>>>> Galileo Gen2 board. The variant IOT2020 comes with one LAN port, the
>>>> IOT2040 has two of them. They can be told apart based on the board asset
>>>> tag in the DMI table.
>
>>>> + const char *asset_tag;
>>>
>>> I guess this is redundant. See below.
>>>
>>>> + {
>>>> + .name = "SIMATIC IOT2000",
>>>> + .asset_tag = "6ES7647-0AA00-0YA2",
>>>> + .func = 6,
>>>> + .phy_addr = 1,
>>>> + },
>>>
>>> The below has same definition disregard on asset_tag.
>>>
>>
>> There is a small difference in the asset tag, just not at the last digit
>> where one may expect it, look:
>>
>> ...-0YA2 -> IOT2020
>> ...-1YA2 -> IOT2040
>
> Yes. And how does it change my statement? You may use one record here
> instead of two.
How? Please be more verbose in your comments.
>
>>
>>>> + {
>>>> + .name = "SIMATIC IOT2000",
>>>> + .asset_tag = "6ES7647-0AA00-1YA2",
>>>> + .func = 6,
>>>> + .phy_addr = 1,
>>>> + },
>
>>>> + {
>>>> + .name = "SIMATIC IOT2000",
>>>> + .asset_tag = "6ES7647-0AA00-1YA2",
>>>> + .func = 7,
>>>> + .phy_addr = 1,
>>>> + },
>>>
>>> How this supposed to work if phy_addr is the same?
>> That address space is MAC-local, and we have two different MACs here.
>
> Got it, though asset_tag here is redundant as well.
>
It's not as it is the only differentiating criteria to tell the
two-ports variant apart from the one-port (and to avoid confusing it
with any potential future variant). We could leave out the name, but I
kept it for documentation purposes.
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply
* Re: [PATCH 0/8] Fix and complete CAN namespace support
From: Marc Kleine-Budde @ 2017-04-25 8:49 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can, davem; +Cc: dev, netdev
In-Reply-To: <d852c2e7-f543-ecea-01f0-9f66d8741476@hartkopp.net>
[-- Attachment #1.1: Type: text/plain, Size: 742 bytes --]
On 04/25/2017 10:48 AM, Oliver Hartkopp wrote:
> On 04/25/2017 10:45 AM, Marc Kleine-Budde wrote:
>
>> FYI:
>> This series is included in the latest pull request:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git
>> tags/linux-can-next-for-4.12-20170425
>>
>
> Ok, thanks!
>
> Sorry for pushing a bit harder this time. But I wanted to make sure that
> we don't get incomplete stuff into the merge window.
np :D
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] can: ti_hecc: fix return value check in ti_hecc_probe()
From: Marc Kleine-Budde @ 2017-04-25 8:49 UTC (permalink / raw)
To: Wei Yongjun, Wolfgang Grandegger, Anton Glukhov, Yegor Yefremov
Cc: Wei Yongjun, linux-can, netdev
In-Reply-To: <20170425064405.3964-1-weiyj.lk@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]
On 04/25/2017 08:44 AM, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> In case of error, the function devm_ioremap_resource() returns ERR_PTR()
> and never returns NULL. The NULL test in the return value check should
> be replaced with IS_ERR().
>
> Fixes: dabf54dd1c63 ("can: ti_hecc: Convert TI HECC driver to DT only driver")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Added to linux-can-next.
Tnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 0/8] Fix and complete CAN namespace support
From: Oliver Hartkopp @ 2017-04-25 8:48 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can, davem; +Cc: dev, netdev
In-Reply-To: <9a88aee4-ba8c-00e7-2525-e300d905e593@pengutronix.de>
On 04/25/2017 10:45 AM, Marc Kleine-Budde wrote:
> FYI:
> This series is included in the latest pull request:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git
> tags/linux-can-next-for-4.12-20170425
>
Ok, thanks!
Sorry for pushing a bit harder this time. But I wanted to make sure that
we don't get incomplete stuff into the merge window.
Regards,
Oliver
^ 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