* Re: [PATCH bpf-next] selftests/bpf: make verifier loop tests arch independent
From: Stanislav Fomichev @ 2019-07-08 21:20 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Y Song, Stanislav Fomichev, netdev, bpf, David Miller,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <99593C98-5DEC-4B18-AE6D-271DD8A8A7F6@linux.ibm.com>
On 07/08, Ilya Leoshkevich wrote:
>
>
> > Am 08.07.2019 um 18:13 schrieb Stanislav Fomichev <sdf@fomichev.me>:
> >
> > On 07/03, Y Song wrote:
> >> On Wed, Jul 3, 2019 at 1:51 PM Stanislav Fomichev <sdf@google.com> wrote:
> >>>
> >>> Take the first x bytes of pt_regs for scalability tests, there is
> >>> no real reason we need x86 specific rax.
> >>>
> >>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >>> ---
> >>> tools/testing/selftests/bpf/progs/loop1.c | 3 ++-
> >>> tools/testing/selftests/bpf/progs/loop2.c | 3 ++-
> >>> tools/testing/selftests/bpf/progs/loop3.c | 3 ++-
> >>> 3 files changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
> >>> index dea395af9ea9..d530c61d2517 100644
> >>> --- a/tools/testing/selftests/bpf/progs/loop1.c
> >>> +++ b/tools/testing/selftests/bpf/progs/loop1.c
> >>> @@ -14,11 +14,12 @@ SEC("raw_tracepoint/kfree_skb")
> >>> int nested_loops(volatile struct pt_regs* ctx)
> >>> {
> >>> int i, j, sum = 0, m;
> >>> + volatile int *any_reg = (volatile int *)ctx;
> >>>
> >>> for (j = 0; j < 300; j++)
> >>> for (i = 0; i < j; i++) {
> >>> if (j & 1)
> >>> - m = ctx->rax;
> >>> + m = *any_reg;
> >>
> >> I agree. ctx->rax here is only to generate some operations, which
> >> cannot be optimized away by the compiler. dereferencing a volatile
> >> pointee may just serve that purpose.
> >>
> >> Comparing the byte code generated with ctx->rax and *any_reg, they are
> >> slightly different. Using *any_reg is slighly worse, but this should
> >> be still okay for the test.
> >>
> >>> else
> >>> m = j;
> >>> sum += i * m;
> >>> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
> >>> index 0637bd8e8bcf..91bb89d901e3 100644
> >>> --- a/tools/testing/selftests/bpf/progs/loop2.c
> >>> +++ b/tools/testing/selftests/bpf/progs/loop2.c
> >>> @@ -14,9 +14,10 @@ SEC("raw_tracepoint/consume_skb")
> >>> int while_true(volatile struct pt_regs* ctx)
> >>> {
> >>> int i = 0;
> >>> + volatile int *any_reg = (volatile int *)ctx;
> >>>
> >>> while (true) {
> >>> - if (ctx->rax & 1)
> >>> + if (*any_reg & 1)
> >>> i += 3;
> >>> else
> >>> i += 7;
> >>> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
> >>> index 30a0f6cba080..3a7f12d7186c 100644
> >>> --- a/tools/testing/selftests/bpf/progs/loop3.c
> >>> +++ b/tools/testing/selftests/bpf/progs/loop3.c
> >>> @@ -14,9 +14,10 @@ SEC("raw_tracepoint/consume_skb")
> >>> int while_true(volatile struct pt_regs* ctx)
> >>> {
> >>> __u64 i = 0, sum = 0;
> >>> + volatile __u64 *any_reg = (volatile __u64 *)ctx;
> >>> do {
> >>> i++;
> >>> - sum += ctx->rax;
> >>> + sum += *any_reg;
> >>> } while (i < 0x100000000ULL);
> >>> return sum;
> >>> }
> >>> --
> >>> 2.22.0.410.gd8fdbe21b5-goog
> >>
> >> Ilya Leoshkevich (iii@linux.ibm.com, cc'ed) has another patch set
> >> trying to solve this problem by introducing s360 arch register access
> >> macros. I guess for now that patch set is not needed any more?
> > Oh, I missed them. Do they fix the tests for other (non-s360) arches as
> > well? I was trying to fix the issue by not depending on any arch
> > specific stuff because the test really doesn't care :-)
>
> They are supposed to work for everything that defines PT_REGS_RC in
> bpf_helpers.h, but I have to admit I tested only x86_64 and s390.
>
> The main source of problems with my approach were mismatching definitions
> of struct pt_regs for userspace and kernel, and because of that there was
> some tweaking required for both arches. I will double check how it looks
> for others (arm, mips, ppc, sparc) tomorrow.
Thanks, I've tested your patches and they fix my issue as well. So you
can have my Tested-by if we'd go with your approach.
One thing I don't understand is: why do you add 'ifdef __KERNEL__' to
the bpf_helpers.h for x86 case? Who is using bpf_helpers.h with
__KERNEL__ defined? Is it perf?
> Best regards,
> Ilya
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] net: stmmac: Introducing support for Page Pool
From: David Miller @ 2019-07-08 21:15 UTC (permalink / raw)
To: Jose.Abreu
Cc: ilias.apalodimas, linux-kernel, netdev, linux-stm32,
linux-arm-kernel, Joao.Pinto, peppe.cavallaro, alexandre.torgue,
brouer, arnd
In-Reply-To: <BN8PR12MB32667BCA58B617432CACE677D3F60@BN8PR12MB3266.namprd12.prod.outlook.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 8 Jul 2019 16:08:07 +0000
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org> | Date: Fri, Jul
> 05, 2019 at 16:24:53
>
>> Well ideally we'd like to get the change in before the merge window ourselves,
>> since we dont want to remove->re-add the same function in stable kernels. If
>> that doesn't go in i am fine fixing it in the next merge window i guess, since
>> it offers substantial speedups
>
> I think the series is marked as "Changes Requested" in patchwork. What's
> the status of this ?
That means I expect a respin based upon feedback or similar. If Ilias and
you agreed to put this series in as-is, my apologies and just resend the
series with appropriate ACK and Review tags added.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v5 3/5] devlink: Introduce PCI PF port flavour and port attribute
From: Jakub Kicinski @ 2019-07-08 21:14 UTC (permalink / raw)
To: Parav Pandit; +Cc: netdev, jiri, saeedm
In-Reply-To: <20190708041549.56601-4-parav@mellanox.com>
On Sun, 7 Jul 2019 23:15:47 -0500, Parav Pandit wrote:
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 3e5f8204c36f..88b2cf207cb2 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -519,6 +519,11 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
> if (devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL &&
> devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
> devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA)
> return 0;
> + if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
Thanks for making the changes! I'm not sure how this would work, tho.
We return early if flavour is not phys/cpu/dsa, so how can flavour be
pci here?..
> + if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
> + attrs->pci_pf.pf))
> + return -EMSGSIZE;
> + }
> if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER,
> attrs->physical.port_number))
> return -EMSGSIZE;
^ permalink raw reply
* Re: [PATCH v3 1/7] dt-bindings: net: Add bindings for Realtek PHYs
From: Andrew Lunn @ 2019-07-08 21:13 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190708200136.GM250418@google.com>
On Mon, Jul 08, 2019 at 01:01:36PM -0700, Matthias Kaehlcke wrote:
> On Mon, Jul 08, 2019 at 09:46:15PM +0200, Andrew Lunn wrote:
> > On Mon, Jul 08, 2019 at 12:24:53PM -0700, Matthias Kaehlcke wrote:
> > > Add the 'realtek,eee-led-mode-disable' property to disable EEE
> > > LED mode on Realtek PHYs that support it.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > TODO: adapt PHY core to deal with optional compatible strings
> >
> > Yes. Does this even work at the moment? I would expect
> > of_mdiobus_child_is_phy() to return false, indicating the device is
> > not actually a PHY.
>
> Indeed, it currently doesn't work atm. I found that removing the check
> for dev->of_node in of_mdiobus_link_mdiodev() helps, but I imagine
> doing (only) this might have undesired side-effects.
O.K.
Please put RFC in patches like this which don't actually work and so
should not be merged. We don't want David accidentally merging them!
Andrew
^ permalink raw reply
* Re: [PATCH] tipc: ensure skb->lock is initialised
From: Chris Packham @ 2019-07-08 21:13 UTC (permalink / raw)
To: Eric Dumazet, jon.maloy@ericsson.com, ying.xue@windriver.com,
davem@davemloft.net
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
linux-kernel@vger.kernel.org
In-Reply-To: <361940337b0d4833a5634ddd1e1896a9@svr-chch-ex1.atlnz.lc>
On 9/07/19 8:43 AM, Chris Packham wrote:
> On 8/07/19 8:18 PM, Eric Dumazet wrote:
>>
>>
>> On 7/8/19 12:53 AM, Chris Packham wrote:
>>> tipc_named_node_up() creates a skb list. It passes the list to
>>> tipc_node_xmit() which has some code paths that can call
>>> skb_queue_purge() which relies on the list->lock being initialised.
>>> Ensure tipc_named_node_up() uses skb_queue_head_init() so that the lock
>>> is explicitly initialised.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>
>> I would rather change the faulty skb_queue_purge() to __skb_queue_purge()
>>
>
> Makes sense. I'll look at that for v2.
>
Actually maybe not. tipc_rcast_xmit(), tipc_node_xmit_skb(),
tipc_send_group_msg(), __tipc_sendmsg(), __tipc_sendstream(), and
tipc_sk_timeout() all use skb_queue_head_init(). So my original change
brings tipc_named_node_up() into line with them.
I think it should be safe for tipc_node_xmit() to use
__skb_queue_purge() since all the callers seem to have exclusive access
to the list of skbs. It still seems that the callers should all use
skb_queue_head_init() for consistency.
^ permalink raw reply
* Re: [PATCH 1/2] forcedeth: add recv cache make nic work steadily
From: Jakub Kicinski @ 2019-07-08 20:52 UTC (permalink / raw)
To: Zhu Yanjun; +Cc: netdev, davem
In-Reply-To: <1562307568-21549-2-git-send-email-yanjun.zhu@oracle.com>
On Fri, 5 Jul 2019 02:19:27 -0400, Zhu Yanjun wrote:
> A recv cache is added. The size of recv cache is 1000Mb / skb_length.
> When the system memory is not enough, this recv cache can make nic work
> steadily.
> When nic is up, this recv cache and work queue are created. When nic
> is down, this recv cache will be destroyed and delayed workqueue is
> canceled.
> When nic is polled or rx interrupt is triggerred, rx handler will
> get a skb from recv cache. Then the state of recv cache is checked.
> If recv cache is not in filling up state, a work is queued to fill
> up recv cache.
> When skb size is changed, the old recv cache is destroyed and new recv
> cache is created.
> When the system memory is not enough, the allocation of skb failed.
> recv cache will continue allocate skb until the recv cache is filled up.
> When the system memory is not enough, this can make nic work steadily.
> Becase of recv cache, the performance of nic is enhanced.
>
> CC: Joe Jin <joe.jin@oracle.com>
> CC: Junxiao Bi <junxiao.bi@oracle.com>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
Could you tell us a little more about the use case and the system
condition?
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index b327b29..a673005 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -674,6 +674,11 @@ struct nv_ethtool_stats {
> u64 tx_broadcast;
> };
>
> +/* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
> + * The length of recv cache is 125M / skb_length
> + */
> +#define RECV_CACHE_LIST_LENGTH (125 * 1024 * 1024 / np->rx_buf_sz)
> +
> #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
> #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
> #define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6)
> @@ -844,8 +849,18 @@ struct fe_priv {
> char name_rx[IFNAMSIZ + 3]; /* -rx */
> char name_tx[IFNAMSIZ + 3]; /* -tx */
> char name_other[IFNAMSIZ + 6]; /* -other */
> +
> + /* This is to schedule work */
> + struct delayed_work recv_cache_work;
> + /* This list is to store skb queue for recv */
> + struct sk_buff_head recv_list;
> + unsigned long nv_recv_list_state;
> };
>
> +/* This is recv list state to fill up recv cache */
> +enum recv_list_state {
> + RECV_LIST_ALLOCATE
> +};
> /*
> * Maximum number of loops until we assume that a bit in the irq mask
> * is stuck. Overridable with module param.
> @@ -1804,7 +1819,11 @@ static int nv_alloc_rx(struct net_device *dev)
> less_rx = np->last_rx.orig;
>
> while (np->put_rx.orig != less_rx) {
> - struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> + struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> + if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> + schedule_delayed_work(&np->recv_cache_work, 0);
Interesting, this seems to be coming up in multiple places recently..
Could you explain why you have your own RECV_LIST_ALLOCATE bit here?
Workqueue implementation itself uses an atomic bit to avoid scheduling
work mutliple times:
bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
struct delayed_work *dwork, unsigned long delay)
{
struct work_struct *work = &dwork->work;
bool ret = false;
unsigned long flags;
/* read the comment in __queue_work() */
local_irq_save(flags);
if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
__queue_delayed_work(cpu, wq, dwork, delay);
ret = true;
}
local_irq_restore(flags);
return ret;
}
EXPORT_SYMBOL(queue_delayed_work_on);
> if (likely(skb)) {
> np->put_rx_ctx->skb = skb;
> np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1845,7 +1864,11 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
> less_rx = np->last_rx.ex;
>
> while (np->put_rx.ex != less_rx) {
> - struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> + struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> + if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> + schedule_delayed_work(&np->recv_cache_work, 0);
It seems a little heavy to schedule this work on every packet, would it
make sense to add this in nv_napi_poll() instead?
> if (likely(skb)) {
> np->put_rx_ctx->skb = skb;
> np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1957,6 +1980,40 @@ static void nv_init_tx(struct net_device *dev)
> }
> }
>
> +static void nv_init_recv_cache(struct net_device *dev)
> +{
> + struct fe_priv *np = netdev_priv(dev);
> +
> + skb_queue_head_init(&np->recv_list);
> + while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> + struct sk_buff *skb = netdev_alloc_skb(dev,
> + np->rx_buf_sz + NV_RX_ALLOC_PAD);
> + /* skb is null. This indicates that memory is not
> + * enough.
> + */
> + if (unlikely(!skb)) {
> + ndelay(3);
> + continue;
Does this path ever hit? Seems like doing an ndelay() and retrying
allocation is not the best idea for non-preempt kernel.
Also perhaps you should consider using __netdev_alloc_skb() and passing
GFP_KERNEL, that way the system has a chance to go into memory reclaim
(I presume this function can sleep).
> + }
> +
> + skb_queue_tail(&np->recv_list, skb);
> + }
> +}
> +
> +static void nv_destroy_recv_cache(struct net_device *dev)
> +{
> + struct sk_buff *skb;
> + struct fe_priv *np = netdev_priv(dev);
> +
> + cancel_delayed_work_sync(&np->recv_cache_work);
> + WARN_ON(delayed_work_pending(&np->recv_cache_work));
> +
> + while ((skb = skb_dequeue(&np->recv_list)))
> + kfree_skb(skb);
skb_queue_purge()
> + WARN_ON(skb_queue_len(&np->recv_list));
> +}
> +
> static int nv_init_ring(struct net_device *dev)
> {
> struct fe_priv *np = netdev_priv(dev);
> @@ -3047,6 +3104,8 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
> nv_drain_rxtx(dev);
> /* reinit driver view of the rx queue */
> set_bufsize(dev);
> + nv_destroy_recv_cache(dev);
> + nv_init_recv_cache(dev);
> if (nv_init_ring(dev)) {
> if (!np->in_shutdown)
> mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> @@ -4074,6 +4133,32 @@ static void nv_free_irq(struct net_device *dev)
> }
> }
>
> +static void nv_recv_cache_worker(struct work_struct *work)
> +{
> + struct fe_priv *np = container_of(work, struct fe_priv,
> + recv_cache_work.work);
> +
> + set_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> + while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> + struct sk_buff *skb = netdev_alloc_skb(np->dev,
> + np->rx_buf_sz + NV_RX_ALLOC_PAD);
> + /* skb is null. This indicates that memory is not
> + * enough.
> + * When the system memory is not enough, the kernel
> + * will compact memory or drop caches. At that time,
> + * if memory allocation fails, it had better wait some
> + * time for memory.
> + */
> + if (unlikely(!skb)) {
> + ndelay(3);
> + continue;
Same comments as for the init function.
> + }
> +
> + skb_queue_tail(&np->recv_list, skb);
> + }
> + clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> +}
> +
> static void nv_do_nic_poll(struct timer_list *t)
> {
> struct fe_priv *np = from_timer(np, t, nic_poll);
> @@ -4129,6 +4214,8 @@ static void nv_do_nic_poll(struct timer_list *t)
> nv_drain_rxtx(dev);
> /* reinit driver view of the rx queue */
> set_bufsize(dev);
> + nv_destroy_recv_cache(dev);
> + nv_init_recv_cache(dev);
> if (nv_init_ring(dev)) {
> if (!np->in_shutdown)
> mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
^ permalink raw reply
* Re: kernel BUG at lib/lockref.c:LINE!
From: syzbot @ 2019-07-08 20:50 UTC (permalink / raw)
To: arvid.brodin, darrick.wong, davem, linux-fsdevel, linux-kernel,
linux-xfs, netdev, sfr, syzkaller-bugs, viro
In-Reply-To: <000000000000b519af058d3091d1@google.com>
syzbot has bisected this bug to:
commit 867c90eeea9d81ad1336881b61a4dcf692fc5d50
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Mon Jul 8 00:22:38 2019 +0000
Merge remote-tracking branch 'xfs/for-next'
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16c69bc3a00000
start commit: d58b5ab9 Add linux-next specific files for 20190708
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=15c69bc3a00000
console output: https://syzkaller.appspot.com/x/log.txt?x=11c69bc3a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=bf9882946ecc11d9
dashboard link: https://syzkaller.appspot.com/bug?extid=f70e9b00f8c7d4187bd0
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=173375c7a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1536f9bfa00000
Reported-by: syzbot+f70e9b00f8c7d4187bd0@syzkaller.appspotmail.com
Fixes: 867c90eeea9d ("Merge remote-tracking branch 'xfs/for-next'")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH] tipc: ensure skb->lock is initialised
From: Chris Packham @ 2019-07-08 20:43 UTC (permalink / raw)
To: Eric Dumazet, jon.maloy@ericsson.com, ying.xue@windriver.com,
davem@davemloft.net
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
linux-kernel@vger.kernel.org
In-Reply-To: <2298b9eb-100f-6130-60c4-0e5e2c7b84d1@gmail.com>
On 8/07/19 8:18 PM, Eric Dumazet wrote:
>
>
> On 7/8/19 12:53 AM, Chris Packham wrote:
>> tipc_named_node_up() creates a skb list. It passes the list to
>> tipc_node_xmit() which has some code paths that can call
>> skb_queue_purge() which relies on the list->lock being initialised.
>> Ensure tipc_named_node_up() uses skb_queue_head_init() so that the lock
>> is explicitly initialised.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>
> I would rather change the faulty skb_queue_purge() to __skb_queue_purge()
>
Makes sense. I'll look at that for v2.
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-08 20:43 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tycho Andersen, Serge E. Hallyn, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190708181237.5poheliito7zpvmc@madcap2.tricolour.ca>
On July 8, 2019 8:12:56 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-05-30 19:26, Paul Moore wrote:
>> On Thu, May 30, 2019 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
>>> On Thu, May 30, 2019 at 03:29:32PM -0400, Paul Moore wrote:
>>>>
>>>>
>>>> [REMINDER: It is an "*audit* container ID" and not a general
>>>> "container ID" ;) Smiley aside, I'm not kidding about that part.]
>>>
>>> This sort of seems like a distinction without a difference; presumably
>>> audit is going to want to differentiate between everything that people
>>> in userspace call a container. So you'll have to support all this
>>> insanity anyway, even if it's "not a container ID".
>>
>> That's not quite right. Audit doesn't care about what a container is,
>> or is not, it also doesn't care if the "audit container ID" actually
>> matches the ID used by the container engine in userspace and I think
>> that is a very important line to draw. Audit is simply given a value
>> which it calls the "audit container ID", it ensures that the value is
>> inherited appropriately (e.g. children inherit their parent's audit
>> container ID), and it uses the value in audit records to provide some
>> additional context for log analysis. The distinction isn't limited to
>> the value itself, but also to how it is used; it is an "audit
>> container ID" and not a "container ID" because this value is
>> exclusively for use by the audit subsystem. We are very intentionally
>> not adding a generic container ID to the kernel. If the kernel does
>> ever grow a general purpose container ID we will be one of the first
>> ones in line to make use of it, but we are not going to be the ones to
>> generically add containers to the kernel. Enough people already hate
>> audit ;)
>>
>>>> I'm not interested in supporting/merging something that isn't useful;
>>>> if this doesn't work for your use case then we need to figure out what
>>>> would work. It sounds like nested containers are much more common in
>>>> the lxc world, can you elaborate a bit more on this?
>>>>
>>>>
>>>> As far as the possible solutions you mention above, I'm not sure I
>>>> like the per-userns audit container IDs, I'd much rather just emit the
>>>> necessary tracking information via the audit record stream and let the
>>>> log analysis tools figure it out. However, the bigger question is how
>>>> to limit (re)setting the audit container ID when you are in a non-init
>>>> userns. For reasons already mentioned, using capable() is a non
>>>> starter for everything but the initial userns, and using ns_capable()
>>>> is equally poor as it essentially allows any userns the ability to
>>>> munge it's audit container ID (obviously not good). It appears we
>>>> need a different method for controlling access to the audit container
>>>> ID.
>>>
>>> One option would be to make it a string, and have it be append only.
>>> That should be safe with no checks.
>>>
>>> I know there was a long thread about what type to make this thing. I
>>> think you could accomplish the append-only-ness with a u64 if you had
>>> some rule about only allowing setting lower order bits than those that
>>> are already set. With 4 bits for simplicity:
>>>
>>> 1100 # initial container id
>>> 1100 -> 1011 # not allowed
>>> 1100 -> 1101 # allowed, but now 1101 is set in stone since there are
>>> # no lower order bits left
>>>
>>> There are probably fancier ways to do it if you actually understand
>>> math :)
>>
>> ;)
>>
>>> Since userns nesting is limited to 32 levels (right now, IIRC), and
>>> you have 64 bits, this might be reasonable. You could just teach
>>> container engines to use the first say N bits for themselves, with a 1
>>> bit for the barrier at the end.
>>
>> I like the creativity, but I worry that at some point these
>> limitations are going to be raised (limits have a funny way of doing
>> that over time) and we will be in trouble. I say "trouble" because I
>> want to be able to quickly do an audit container ID comparison and
>> we're going to pay a penalty for these larger values (we'll need this
>> when we add multiple auditd support and the requisite record routing).
>>
>> Thinking about this makes me also realize we probably need to think a
>> bit longer about audit container ID conflicts between orchestrators.
>> Right now we just take the value that is given to us by the
>> orchestrator, but if we want to allow multiple container orchestrators
>> to work without some form of cooperation in userspace (I think we have
>> to assume the orchestrators will not talk to each other) we likely
>> need to have some way to block reuse of an audit container ID. We
>> would either need to prevent the orchestrator from explicitly setting
>> an audit container ID to a currently in use value, or instead generate
>> the audit container ID in the kernel upon an event triggered by the
>> orchestrator (e.g. a write to a /proc file). I suspect we should
>> start looking at the idr code, I think we will need to make use of it.
>
> To address this, I'd suggest that it is enforced to only allow the
> setting of descendants and to maintain a master list of audit container
> identifiers (with a hash table if necessary later) that includes the
> container owner.
>
> This also allows the orchestrator/engine to inject processes into
> existing containers by checking that the audit container identifier is
> only used again by the same owner.
>
> I have working code for both.
Just a quick note that due to some holiday travel I'm not going to be able to adequately respond to your latest messages on this thread for at least another week, likely a bit more. I'm only checking mail to put out fires, and the audit container ID work tends to be something that starts them ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()
From: Anton Protopopov @ 2019-07-08 20:37 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, Networking, bpf, open list, Andrii Nakryiko
In-Reply-To: <CAEf4BzaGGVv2z8jB8MnT7=gnn4nG0cp7DGYxfnnnpohOT=ujCA@mail.gmail.com>
пн, 8 июл. 2019 г. в 13:54, Andrii Nakryiko <andrii.nakryiko@gmail.com>:
>
> On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 07/05/2019 10:44 PM, Anton Protopopov wrote:
> > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in
> > > an object by maps pinned to a directory provided in the path argument. Namely,
> > > each map M in the object will be replaced by a map pinned to path/M.name.
> > >
> > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > > ---
> > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++
> > > tools/lib/bpf/libbpf.h | 2 ++
> > > tools/lib/bpf/libbpf.map | 1 +
> > > 3 files changed, 37 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 4907997289e9..84c9e8f7bfd3 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> > > return 0;
> > > }
> > >
> > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path)
>
> As is, bpf_object__reuse_maps() can be easily implemented by user
> applications, as it's only using public libbpf APIs, so I'm not 100%
> sure we need to add method like that to libbpf.
The bpf_object__reuse_maps() can definitely be implemented by user
applications, however, to use it a user also needs to re-implement the
bpf_prog_load_xattr funciton, so it seemed to me that adding this
functionality to the library is a better way.
>
> > > +{
> > > + struct bpf_map *map;
> > > +
> > > + if (!obj)
> > > + return -ENOENT;
> > > +
> > > + if (!path)
> > > + return -EINVAL;
> > > +
> > > + bpf_object__for_each_map(map, obj) {
> > > + int len, err;
> > > + int pinned_map_fd;
> > > + char buf[PATH_MAX];
> >
> > We'd need to skip the case of bpf_map__is_internal(map) since they are always
> > recreated for the given object.
> >
> > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> > > + if (len < 0) {
> > > + return -EINVAL;
> > > + } else if (len >= PATH_MAX) {
> > > + return -ENAMETOOLONG;
> > > + }
> > > +
> > > + pinned_map_fd = bpf_obj_get(buf);
> > > + if (pinned_map_fd < 0)
> > > + return pinned_map_fd;
> >
> > Should we rather have a new map definition attribute that tells to reuse
> > the map if it's pinned in bpf fs, and if not, we create it and later on
> > pin it? This is what iproute2 is doing and which we're making use of heavily.
>
> I'd like something like that as well. This would play nicely with
> recently added BTF-defined maps as well.
>
> I think it should be not just pin/don't pin flag, but rather pinning
> strategy, to accommodate various typical strategies of handling maps
> that are already pinned. So something like this:
>
> 1. BPF_PIN_NOTHING - default, don't pin;
> 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail;
> 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be
> exact state of object's map;
> 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this
> is how Cilium is pinning PROG_ARRAY maps, if I understand correctly);
> 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values.
>
> This list is only for illustrative purposes, ideally people that have
> a lot of experience using pinning for real-world use cases would chime
> in on what strategies are useful and make sense.
My case was simply to reuse existing maps when reloading a program.
Does it make sense for you to add only the simplest cases of listed above?
Also, libbpf doesn't use standard naming conventions for pinning maps.
Does it make sense to provide a list of already open maps to the
bpf_prog_load_xattr function as an attribute? In this case a user
can execute his own policy on pinning, but still will have an option
to reuse, reset, and merge maps.
>
> > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps
> > too limiting for a generic API as new version of an object file may contain
> > new maps which are not yet present in bpf fs at that point.
> >
> > > + err = bpf_map__reuse_fd(map, pinned_map_fd);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
> > > {
> > > struct bpf_program *prog;
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index d639f47e3110..7fe465a1be76 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
> > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
> > > const char *path);
> > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj,
> > > + const char *path);
> > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
> > > const char *path);
> > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index 2c6d835620d2..66a30be6696c 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 {
> > > btf_dump__new;
> > > btf__parse_elf;
> > > bpf_object__load_xattr;
> > > + bpf_object__reuse_maps;
> > > libbpf_num_possible_cpus;
> > > } LIBBPF_0.0.3;
> > >
> >
^ permalink raw reply
* Re: [PATCH] ath10k: work around uninitialized vht_pfr variable
From: Brian Norris @ 2019-07-08 20:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kalle Valo, Miaoqing Pan, David S. Miller, Rakesh Pillai,
Balaji Pothunoori, Wen Gong, Pradeep kumar Chitrapu, Sriram R,
ath10k, linux-wireless, <netdev@vger.kernel.org>,
Linux Kernel, clang-built-linux
In-Reply-To: <20190708125050.3689133-1-arnd@arndb.de>
Hi Arnd,
On Mon, Jul 8, 2019 at 5:50 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> As clang points out, the vht_pfr is assigned to a struct member
> without being initialized in one case:
>
> drivers/net/wireless/ath/ath10k/mac.c:7528:7: error: variable 'vht_pfr' is used uninitialized whenever 'if' condition
> is false [-Werror,-Wsometimes-uninitialized]
> if (!ath10k_mac_can_set_bitrate_mask(ar, band, mask,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7551:20: note: uninitialized use occurs here
> arvif->vht_pfr = vht_pfr;
> ^~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7528:3: note: remove the 'if' if its condition is always true
> if (!ath10k_mac_can_set_bitrate_mask(ar, band, mask,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7483:12: note: initialize the variable 'vht_pfr' to silence this warning
> u8 vht_pfr;
>
> Add an explicit but probably incorrect initialization here.
> I suspect we want a better fix here, but chose this approach to
> illustrate the issue.
>
> Fixes: 8b97b055dc9d ("ath10k: fix failure to set multiple fixed rate")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/wireless/ath/ath10k/mac.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index e43a566eef77..0606416dc971 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -7541,6 +7541,8 @@ static int ath10k_mac_op_set_bitrate_mask(struct ieee80211_hw *hw,
> &vht_nss,
> true);
^^ Technically, this call to ath10k_mac_bitrate_mask_get_single_rate()
can fail to assign 'vht_pfr' as well. I can't immediately tell whether
it provably will never hit the -EINVAL case, but if we do, then you'd
have another uninitialized case.
I *believe* it shouldn't fail, since we already pre-checked the VHT
MCS lists for "exactly 1" rate. But it still seems like better code to
pre-initialize and/or add error-handling, so we don't rely on that
implicit proof.
I'm not quite sure yet what the "better" answer should be for
resolving this, but at a minimum, I think the above could be improved.
Brian
> update_bitrate_mask = false;
> + } else {
> + vht_pfr = 0;
> }
>
> mutex_lock(&ar->conf_mutex);
> --
> 2.20.0
>
^ permalink raw reply
* Re: [PATCH v8 net-next 0/5] net: ethernet: ti: cpsw: Add XDP support
From: Ivan Khoronzhuk @ 2019-07-08 20:32 UTC (permalink / raw)
To: David Miller
Cc: grygorii.strashko, hawk, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190707.183511.503486832061897586.davem@davemloft.net>
On Sun, Jul 07, 2019 at 06:35:11PM -0700, David Miller wrote:
>From: David Miller <davem@davemloft.net>
>Date: Sun, 07 Jul 2019 18:31:46 -0700 (PDT)
>
>> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> Date: Fri, 5 Jul 2019 18:04:57 +0300
>>
>>> This patchset adds XDP support for TI cpsw driver and base it on
>>> page_pool allocator. It was verified on af_xdp socket drop,
>>> af_xdp l2f, ebpf XDP_DROP, XDP_REDIRECT, XDP_PASS, XDP_TX.
>>>
>>> It was verified with following configs enabled:
>> ...
>>
>> I'm applying this to net-next, please deal with whatever follow-ups are
>> necessary.
>
>Nevermind, you really have to fix this:
>
>drivers/net/ethernet/ti/davinci_cpdma.c: In function ‘cpdma_chan_submit_si’:
>drivers/net/ethernet/ti/davinci_cpdma.c:1047:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> buffer = (u32)si->data;
> ^
>drivers/net/ethernet/ti/davinci_cpdma.c: In function ‘cpdma_chan_idle_submit_mapped’:
>drivers/net/ethernet/ti/davinci_cpdma.c:1114:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> si.data = (void *)(u32)data;
> ^
>drivers/net/ethernet/ti/davinci_cpdma.c: In function ‘cpdma_chan_submit_mapped’:
>drivers/net/ethernet/ti/davinci_cpdma.c:1164:12: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> si.data = (void *)(u32)data;
> ^
Actrually that's fixed in reply v9 patch.
But, nevermind, i will send v9 for whole series.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net-next v6 04/15] ethtool: introduce ethtool netlink interface
From: Michal Kubecek @ 2019-07-08 20:22 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190708192629.GD2282@nanopsycho.orion>
On Mon, Jul 08, 2019 at 09:26:29PM +0200, Jiri Pirko wrote:
> Mon, Jul 08, 2019 at 07:27:29PM CEST, mkubecek@suse.cz wrote:
> >
> >There are two reasons for this design. First is to reduce the number of
> >requests needed to get the information. This is not so much a problem of
> >ethtool itself; the only existing commands that would result in multiple
> >request messages would be "ethtool <dev>" and "ethtool -s <dev>". Maybe
> >also "ethtool -x/-X <dev>" but even if the indirection table and hash
> >key have different bits assigned now, they don't have to be split even
> >if we split other commands. It may be bigger problem for daemons wanting
> >to keep track of system configuration which would have to issue many
> >requests whenever a new device appears.
> >
> >Second reason is that with 8-bit genetlink command/message id, the space
> >is not as infinite as it might seem. I counted quickly, right now the
> >full series uses 14 ids for kernel messages, with split you propose it
> >would most likely grow to 44. For full implementation of all ethtool
> >functionality, we could get to ~60 ids. It's still only 1/4 of the
> >available space but it's not clear what the future development will look
> >like. We would certainly need to be careful not to start allocating new
> >commands for single parameters and try to be foreseeing about what can
> >be grouped together. But we will need to do that in any case.
> >
> >On kernel side, splitting existing messages would make some things a bit
> >easier. It would also reduce the number of scenarios where only part of
> >requested information is available or only part of a SET request fails.
>
> Okay, I got your point. So why don't we look at if from the other angle.
> Why don't we have only single get/set command that would be in general
> used to get/set ALL info from/to the kernel. Where we can have these
> bits (perhaps rather varlen bitfield) to for user to indicate which data
> is he interested in? This scales. The other commands would be
> just for action.
>
> Something like RTM_GETLINK/RTM_SETLINK. Makes sense?
It's certainly an option but at the first glance it seems as just moving
what I tried to avoid one level lower. It would work around the u8 issue
(but as Johannes pointed out, we can handle it with genetlink when/if
the time comes). We would almost certainly have to split the replies
into multiple messages to keep the packet size reasonable. I'll have to
think more about the consequences for both kernel and userspace.
My gut feeling is that out of the two extreme options (one universal
message type and message types corresponding to current infomask bits),
the latter is more appealing. After all, ethtool has been gathering
features that would need those ~60 message types for 20 years.
Michal
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: make verifier loop tests arch independent
From: Ilya Leoshkevich @ 2019-07-08 20:14 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Y Song, Stanislav Fomichev, netdev, bpf, David Miller,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190708161338.GC29524@mini-arch>
> Am 08.07.2019 um 18:13 schrieb Stanislav Fomichev <sdf@fomichev.me>:
>
> On 07/03, Y Song wrote:
>> On Wed, Jul 3, 2019 at 1:51 PM Stanislav Fomichev <sdf@google.com> wrote:
>>>
>>> Take the first x bytes of pt_regs for scalability tests, there is
>>> no real reason we need x86 specific rax.
>>>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>> tools/testing/selftests/bpf/progs/loop1.c | 3 ++-
>>> tools/testing/selftests/bpf/progs/loop2.c | 3 ++-
>>> tools/testing/selftests/bpf/progs/loop3.c | 3 ++-
>>> 3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
>>> index dea395af9ea9..d530c61d2517 100644
>>> --- a/tools/testing/selftests/bpf/progs/loop1.c
>>> +++ b/tools/testing/selftests/bpf/progs/loop1.c
>>> @@ -14,11 +14,12 @@ SEC("raw_tracepoint/kfree_skb")
>>> int nested_loops(volatile struct pt_regs* ctx)
>>> {
>>> int i, j, sum = 0, m;
>>> + volatile int *any_reg = (volatile int *)ctx;
>>>
>>> for (j = 0; j < 300; j++)
>>> for (i = 0; i < j; i++) {
>>> if (j & 1)
>>> - m = ctx->rax;
>>> + m = *any_reg;
>>
>> I agree. ctx->rax here is only to generate some operations, which
>> cannot be optimized away by the compiler. dereferencing a volatile
>> pointee may just serve that purpose.
>>
>> Comparing the byte code generated with ctx->rax and *any_reg, they are
>> slightly different. Using *any_reg is slighly worse, but this should
>> be still okay for the test.
>>
>>> else
>>> m = j;
>>> sum += i * m;
>>> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
>>> index 0637bd8e8bcf..91bb89d901e3 100644
>>> --- a/tools/testing/selftests/bpf/progs/loop2.c
>>> +++ b/tools/testing/selftests/bpf/progs/loop2.c
>>> @@ -14,9 +14,10 @@ SEC("raw_tracepoint/consume_skb")
>>> int while_true(volatile struct pt_regs* ctx)
>>> {
>>> int i = 0;
>>> + volatile int *any_reg = (volatile int *)ctx;
>>>
>>> while (true) {
>>> - if (ctx->rax & 1)
>>> + if (*any_reg & 1)
>>> i += 3;
>>> else
>>> i += 7;
>>> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
>>> index 30a0f6cba080..3a7f12d7186c 100644
>>> --- a/tools/testing/selftests/bpf/progs/loop3.c
>>> +++ b/tools/testing/selftests/bpf/progs/loop3.c
>>> @@ -14,9 +14,10 @@ SEC("raw_tracepoint/consume_skb")
>>> int while_true(volatile struct pt_regs* ctx)
>>> {
>>> __u64 i = 0, sum = 0;
>>> + volatile __u64 *any_reg = (volatile __u64 *)ctx;
>>> do {
>>> i++;
>>> - sum += ctx->rax;
>>> + sum += *any_reg;
>>> } while (i < 0x100000000ULL);
>>> return sum;
>>> }
>>> --
>>> 2.22.0.410.gd8fdbe21b5-goog
>>
>> Ilya Leoshkevich (iii@linux.ibm.com, cc'ed) has another patch set
>> trying to solve this problem by introducing s360 arch register access
>> macros. I guess for now that patch set is not needed any more?
> Oh, I missed them. Do they fix the tests for other (non-s360) arches as
> well? I was trying to fix the issue by not depending on any arch
> specific stuff because the test really doesn't care :-)
They are supposed to work for everything that defines PT_REGS_RC in
bpf_helpers.h, but I have to admit I tested only x86_64 and s390.
The main source of problems with my approach were mismatching definitions
of struct pt_regs for userspace and kernel, and because of that there was
some tweaking required for both arches. I will double check how it looks
for others (arm, mips, ppc, sparc) tomorrow.
Best regards,
Ilya
^ permalink raw reply
* Re: Kernel BUG: epoll_wait() (and epoll_pwait) stall for 206 ms per call on sockets with a small-ish snd/rcv buffer.
From: Neal Cardwell @ 2019-07-08 20:11 UTC (permalink / raw)
To: Carlo Wood; +Cc: David Miller, Netdev
In-Reply-To: <20190706201912.435a2198@hikaru>
On Sat, Jul 6, 2019 at 2:19 PM Carlo Wood <carlo@alinoe.com> wrote:
>
> While investigating this further, I read on
> http://www.masterraghu.com/subjects/np/introduction/unix_network_programming_v1.3/ch07lev1sec5.html
> under "SO_RCVBUF and SO_SNDBUF Socket Options":
>
> When setting the size of the TCP socket receive buffer, the
> ordering of the function calls is important. This is because of
> TCP's window scale option (Section 2.6), which is exchanged with
> the peer on the SYN segments when the connection is established.
> For a client, this means the SO_RCVBUF socket option must be set
> before calling connect. For a server, this means the socket option
> must be set for the listening socket before calling listen. Setting
> this option for the connected socket will have no effect whatsoever
> on the possible window scale option because accept does not return
> with the connected socket until TCP's three-way handshake is
> complete. That is why this option must be set for the listening
> socket. (The sizes of the socket buffers are always inherited from
> the listening socket by the newly created connected socket: pp.
> 462–463 of TCPv2.)
>
> As mentioned in a previous post, I had already discovered about
> needing to set the socket buffers before connect, but I didn't know
> about setting them before the call to listen() in order to get the
> buffer sizes inherited by the accepted sockets.
>
> After fixing this in my test program, all problems disappeared when
> keeping the send and receive buffers the same on both sides.
>
> However, when only setting the send and receive buffers on the client
> socket (not on the (accepted or) listen socket), epoll_wait() still
> stalls 43ms. When the SO_SNDBUF is smaller than 33182 bytes.
>
> Here is the latest version of my test program:
>
> https://github.com/CarloWood/ai-evio-testsuite/blob/master/src/epoll_bug.c
>
> I have to retract most of my "bug" report, it might even not really be
> a bug then... but nevertheless, what remains strange is the fact
> that setting the socket buffer sizes on the accepted sockets can lead
> to so much crippling effect, while the quoted website states:
>
> Setting this option for the connected socket will have no effect
> whatsoever on the possible window scale option because accept does
> not return with the connected socket until TCP's three-way
> handshake is complete.
>
> And when only setting the socket buffer sizes for the client socket
> (that I use to send back received data; so this is the sending
> side now) then why does epoll_wait() stall 43 ms per call when the
> receiving side is using the default (much larger) socket buffer sizes?
>
> That 43 ms is STILL crippling-- slowing down the transmission of the
> data to a trickling speed compared to what it should be.
Based on the magic numbers you cite, including the fact that this test
program seems to send traffic over a loopback device (presumably
MTU=65536), epoll_wait() stalling 43 ms (slightly longer than the
typical Linux delayed ACK timer), and the problem only happening if
SO_SNDBUF is smaller than 33182 bytes (half the MTU)... a guess would
be that when you artificially make the SO_SNDBUF that low, then you
are asking the kernel to only allow your sending sockets to buffer
less than a single MTU of data, which means that the packets the
sender sends are going to be less than the MTU, which means that the
receiver may tend to eventually (after the initial quick ACKs expire)
delay its ACKs in hopes of eventually receiving a full MSS of data
(see __tcp_ack_snd_check()). Since the SO_SNDBUF is so small in this
case, the sender then would not be able to write() or transmit
anything else until the receiver sends a delayed ACK for the data
~40ms later, allowing the sending socket to free the previously sent
data and trigger the sender's next EPOLLOUT and write().
You could try grabbing a packet capture of the traffic over your
loopback device during the test to see if it matches that theory:
tcpdump -i lo -s 100 -w /tmp/test.pcap
cheers,
neal
^ permalink raw reply
* Re: [PATCH v3 6/7] dt-bindings: net: realtek: Add property to configure LED mode
From: Matthias Kaehlcke @ 2019-07-08 20:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190708194834.GI9027@lunn.ch>
Hi Andrew,
On Mon, Jul 08, 2019 at 09:48:34PM +0200, Andrew Lunn wrote:
> On Mon, Jul 08, 2019 at 12:24:58PM -0700, Matthias Kaehlcke wrote:
> > The LED behavior of some Realtek PHYs is configurable. Add the
> > property 'realtek,led-modes' to specify the configuration of the
> > LEDs.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>
> Hi Matthias
>
> Humm. I thought you were going to drop this and the next patch?
It wasn't clear to me whether not introducing a generic interface is a
definitive NACK and tought I could at least post a version with the
review comments addressed. If there is no way forward without a
generic interface I'll drop the two patches.
^ permalink raw reply
* Re: [PATCH v3 net-next 19/19] ionic: Add basic devlink interface
From: Jiri Pirko @ 2019-07-08 20:03 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev
In-Reply-To: <af206309-514d-9619-1455-efc93af8431e@pensando.io>
Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote:
>On 7/8/19 12:34 PM, Jiri Pirko wrote:
>> Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote:
>> > Add a devlink interface for access to information that isn't
>> > normally available through ethtool or the iplink interface.
>> >
>> > Example:
>> > $ ./devlink -j -p dev info pci/0000:b6:00.0
>> > {
>> > "info": {
>> > "pci/0000:b6:00.0": {
>> > "driver": "ionic",
>> > "serial_number": "FLM18420073",
>> > "versions": {
>> > "fixed": {
>> > "fw_version": "0.11.0-50",
>> > "fw_status": "0x1",
>> > "fw_heartbeat": "0x716ce",
>> > "asic_type": "0x0",
>> > "asic_rev": "0x0"
>> > }
>> > }
>> > }
>> > }
>> > }
>> >
>> > Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> > ---
>> > drivers/net/ethernet/pensando/ionic/Makefile | 2 +-
>> > drivers/net/ethernet/pensando/ionic/ionic.h | 1 +
>> > .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++
>> > .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++
>> > .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++
>> > 5 files changed, 110 insertions(+), 1 deletion(-)
>> > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> > create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>> >
>> > diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile
>> > index 4f3cfbf36c23..ce187c7b33a8 100644
>> > --- a/drivers/net/ethernet/pensando/ionic/Makefile
>> > +++ b/drivers/net/ethernet/pensando/ionic/Makefile
>> > @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o
>> >
>> > ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \
>> > ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \
>> > - ionic_stats.o
>> > + ionic_stats.o ionic_devlink.o
>> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
>> > index cd08166f73a9..a0034bc5b4a1 100644
>> > --- a/drivers/net/ethernet/pensando/ionic/ionic.h
>> > +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
>> > @@ -44,6 +44,7 @@ struct ionic {
>> > DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX);
>> > struct work_struct nb_work;
>> > struct notifier_block nb;
>> > + struct devlink *dl;
>> > };
>> >
>> > struct ionic_admin_ctx {
>> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> > index 98c12b770c7f..a8c99254489f 100644
>> > --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> > @@ -10,6 +10,7 @@
>> > #include "ionic_bus.h"
>> > #include "ionic_lif.h"
>> > #include "ionic_debugfs.h"
>> > +#include "ionic_devlink.h"
>> >
>> > /* Supported devices */
>> > static const struct pci_device_id ionic_id_table[] = {
>> > @@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> > goto err_out_deinit_lifs;
>> > }
>> >
>> > + err = ionic_devlink_register(ionic);
>> > + if (err)
>> > + dev_err(dev, "Cannot register devlink (ignored): %d\n", err);
>> > +
>> > return 0;
>> >
>> > err_out_deinit_lifs:
>> > + ionic_devlink_unregister(ionic);
>> > ionic_lifs_deinit(ionic);
>> > err_out_free_lifs:
>> > ionic_lifs_free(ionic);
>> > @@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev)
>> > struct ionic *ionic = pci_get_drvdata(pdev);
>> >
>> > if (ionic) {
>> > + ionic_devlink_unregister(ionic);
>> > ionic_lifs_unregister(ionic);
>> > ionic_lifs_deinit(ionic);
>> > ionic_lifs_free(ionic);
>> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> > new file mode 100644
>> > index 000000000000..fbbfcdde292f
>> > --- /dev/null
>> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> > @@ -0,0 +1,89 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> > +
>> > +#include <linux/module.h>
>> > +#include <linux/netdevice.h>
>> > +
>> > +#include "ionic.h"
>> > +#include "ionic_bus.h"
>> > +#include "ionic_lif.h"
>> > +#include "ionic_devlink.h"
>> > +
>> > +struct ionic_devlink {
>> > + struct ionic *ionic;
>> > +};
>> > +
>> > +static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>> > + struct netlink_ext_ack *extack)
>> > +{
>> > + struct ionic *ionic = *(struct ionic **)devlink_priv(dl);
>> > + struct ionic_dev *idev = &ionic->idev;
>> > + char buf[16];
>> > + u32 val;
>> > +
>> > + devlink_info_driver_name_put(req, DRV_NAME);
>> > +
>> > + devlink_info_version_fixed_put(req, "fw_version",
>> > + idev->dev_info.fw_version);
>> > +
>> > + val = ioread8(&idev->dev_info_regs->fw_status);
>> > + snprintf(buf, sizeof(buf), "0x%x", val);
>> > + devlink_info_version_fixed_put(req, "fw_status", buf);
>> > +
>> > + val = ioread32(&idev->dev_info_regs->fw_heartbeat);
>> > + snprintf(buf, sizeof(buf), "0x%x", val);
>> > + devlink_info_version_fixed_put(req, "fw_heartbeat", buf);
>> > +
>> > + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type);
>> > + devlink_info_version_fixed_put(req, "asic_type", buf);
>> > +
>> > + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev);
>> > + devlink_info_version_fixed_put(req, "asic_rev", buf);
>> > +
>> > + devlink_info_serial_number_put(req, idev->dev_info.serial_num);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static const struct devlink_ops ionic_dl_ops = {
>> > + .info_get = ionic_dl_info_get,
>> > +};
>> > +
>> > +int ionic_devlink_register(struct ionic *ionic)
>> > +{
>> > + struct devlink *dl;
>> > + struct ionic **ip;
>> > + int err;
>> > +
>> > + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *));
>> Oups. Something is wrong with your flow. The devlink alloc is allocating
>> the structure that holds private data (per-device data) for you. This is
>> misuse :/
>>
>> You are missing one parent device struct apparently.
>>
>> Oh, I think I see something like it. The unused "struct ionic_devlink".
>
>If I'm not mistaken, the alloc is only allocating enough for a pointer, not
>the whole per device struct, and a few lines down from here the pointer to
>the new devlink struct is assigned to ionic->dl. This was based on what I
>found in the qed driver's qed_devlink_register(), and it all seems to work.
I'm not saying your code won't work. What I say is that you should have
a struct for device that would be allocated by devlink_alloc()
The ionic struct should be associated with devlink_port. That you are
missing too.
>
>That unused struct ionic_devlink does need to go away, it was superfluous
>after working out a better typecast off of devlink_priv().
>
>I'll remove the unused struct ionic_devlink, but I think the rest is okay.
>
>sln
>
>>
>>
>> > + if (!dl) {
>> > + dev_warn(ionic->dev, "devlink_alloc failed");
>> > + return -ENOMEM;
>> > + }
>> > +
>> > + ip = (struct ionic **)devlink_priv(dl);
>> > + *ip = ionic;
>> > + ionic->dl = dl;
>> > +
>> > + err = devlink_register(dl, ionic->dev);
>> > + if (err) {
>> > + dev_warn(ionic->dev, "devlink_register failed: %d\n", err);
>> > + goto err_dl_free;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +err_dl_free:
>> > + ionic->dl = NULL;
>> > + devlink_free(dl);
>> > + return err;
>> > +}
>> > +
>> > +void ionic_devlink_unregister(struct ionic *ionic)
>> > +{
>> > + if (!ionic->dl)
>> > + return;
>> > +
>> > + devlink_unregister(ionic->dl);
>> > + devlink_free(ionic->dl);
>> > +}
>> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>> > new file mode 100644
>> > index 000000000000..35528884e29f
>> > --- /dev/null
>> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>> > @@ -0,0 +1,12 @@
>> > +/* SPDX-License-Identifier: GPL-2.0 */
>> > +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> > +
>> > +#ifndef _IONIC_DEVLINK_H_
>> > +#define _IONIC_DEVLINK_H_
>> > +
>> > +#include <net/devlink.h>
>> > +
>> > +int ionic_devlink_register(struct ionic *ionic);
>> > +void ionic_devlink_unregister(struct ionic *ionic);
>> > +
>> > +#endif /* _IONIC_DEVLINK_H_ */
>> > --
>> > 2.17.1
>> >
>
^ permalink raw reply
* Re: [PATCH v3 1/7] dt-bindings: net: Add bindings for Realtek PHYs
From: Matthias Kaehlcke @ 2019-07-08 20:01 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190708194615.GH9027@lunn.ch>
On Mon, Jul 08, 2019 at 09:46:15PM +0200, Andrew Lunn wrote:
> On Mon, Jul 08, 2019 at 12:24:53PM -0700, Matthias Kaehlcke wrote:
> > Add the 'realtek,eee-led-mode-disable' property to disable EEE
> > LED mode on Realtek PHYs that support it.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > TODO: adapt PHY core to deal with optional compatible strings
>
> Yes. Does this even work at the moment? I would expect
> of_mdiobus_child_is_phy() to return false, indicating the device is
> not actually a PHY.
Indeed, it currently doesn't work atm. I found that removing the check
for dev->of_node in of_mdiobus_link_mdiodev() helps, but I imagine
doing (only) this might have undesired side-effects.
^ permalink raw reply
* Re: [PATCH v3 net-next 19/19] ionic: Add basic devlink interface
From: Shannon Nelson @ 2019-07-08 19:58 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
In-Reply-To: <20190708193454.GF2282@nanopsycho.orion>
On 7/8/19 12:34 PM, Jiri Pirko wrote:
> Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote:
>> Add a devlink interface for access to information that isn't
>> normally available through ethtool or the iplink interface.
>>
>> Example:
>> $ ./devlink -j -p dev info pci/0000:b6:00.0
>> {
>> "info": {
>> "pci/0000:b6:00.0": {
>> "driver": "ionic",
>> "serial_number": "FLM18420073",
>> "versions": {
>> "fixed": {
>> "fw_version": "0.11.0-50",
>> "fw_status": "0x1",
>> "fw_heartbeat": "0x716ce",
>> "asic_type": "0x0",
>> "asic_rev": "0x0"
>> }
>> }
>> }
>> }
>> }
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>> drivers/net/ethernet/pensando/ionic/Makefile | 2 +-
>> drivers/net/ethernet/pensando/ionic/ionic.h | 1 +
>> .../ethernet/pensando/ionic/ionic_bus_pci.c | 7 ++
>> .../ethernet/pensando/ionic/ionic_devlink.c | 89 +++++++++++++++++++
>> .../ethernet/pensando/ionic/ionic_devlink.h | 12 +++
>> 5 files changed, 110 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile
>> index 4f3cfbf36c23..ce187c7b33a8 100644
>> --- a/drivers/net/ethernet/pensando/ionic/Makefile
>> +++ b/drivers/net/ethernet/pensando/ionic/Makefile
>> @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o
>>
>> ionic-y := ionic_main.o ionic_bus_pci.o ionic_dev.o ionic_ethtool.o \
>> ionic_lif.o ionic_rx_filter.o ionic_txrx.o ionic_debugfs.o \
>> - ionic_stats.o
>> + ionic_stats.o ionic_devlink.o
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
>> index cd08166f73a9..a0034bc5b4a1 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
>> @@ -44,6 +44,7 @@ struct ionic {
>> DECLARE_BITMAP(intrs, INTR_CTRL_REGS_MAX);
>> struct work_struct nb_work;
>> struct notifier_block nb;
>> + struct devlink *dl;
>> };
>>
>> struct ionic_admin_ctx {
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> index 98c12b770c7f..a8c99254489f 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> @@ -10,6 +10,7 @@
>> #include "ionic_bus.h"
>> #include "ionic_lif.h"
>> #include "ionic_debugfs.h"
>> +#include "ionic_devlink.h"
>>
>> /* Supported devices */
>> static const struct pci_device_id ionic_id_table[] = {
>> @@ -212,9 +213,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> goto err_out_deinit_lifs;
>> }
>>
>> + err = ionic_devlink_register(ionic);
>> + if (err)
>> + dev_err(dev, "Cannot register devlink (ignored): %d\n", err);
>> +
>> return 0;
>>
>> err_out_deinit_lifs:
>> + ionic_devlink_unregister(ionic);
>> ionic_lifs_deinit(ionic);
>> err_out_free_lifs:
>> ionic_lifs_free(ionic);
>> @@ -247,6 +253,7 @@ static void ionic_remove(struct pci_dev *pdev)
>> struct ionic *ionic = pci_get_drvdata(pdev);
>>
>> if (ionic) {
>> + ionic_devlink_unregister(ionic);
>> ionic_lifs_unregister(ionic);
>> ionic_lifs_deinit(ionic);
>> ionic_lifs_free(ionic);
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> new file mode 100644
>> index 000000000000..fbbfcdde292f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> @@ -0,0 +1,89 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +
>> +#include "ionic.h"
>> +#include "ionic_bus.h"
>> +#include "ionic_lif.h"
>> +#include "ionic_devlink.h"
>> +
>> +struct ionic_devlink {
>> + struct ionic *ionic;
>> +};
>> +
>> +static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct ionic *ionic = *(struct ionic **)devlink_priv(dl);
>> + struct ionic_dev *idev = &ionic->idev;
>> + char buf[16];
>> + u32 val;
>> +
>> + devlink_info_driver_name_put(req, DRV_NAME);
>> +
>> + devlink_info_version_fixed_put(req, "fw_version",
>> + idev->dev_info.fw_version);
>> +
>> + val = ioread8(&idev->dev_info_regs->fw_status);
>> + snprintf(buf, sizeof(buf), "0x%x", val);
>> + devlink_info_version_fixed_put(req, "fw_status", buf);
>> +
>> + val = ioread32(&idev->dev_info_regs->fw_heartbeat);
>> + snprintf(buf, sizeof(buf), "0x%x", val);
>> + devlink_info_version_fixed_put(req, "fw_heartbeat", buf);
>> +
>> + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_type);
>> + devlink_info_version_fixed_put(req, "asic_type", buf);
>> +
>> + snprintf(buf, sizeof(buf), "0x%x", idev->dev_info.asic_rev);
>> + devlink_info_version_fixed_put(req, "asic_rev", buf);
>> +
>> + devlink_info_serial_number_put(req, idev->dev_info.serial_num);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct devlink_ops ionic_dl_ops = {
>> + .info_get = ionic_dl_info_get,
>> +};
>> +
>> +int ionic_devlink_register(struct ionic *ionic)
>> +{
>> + struct devlink *dl;
>> + struct ionic **ip;
>> + int err;
>> +
>> + dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *));
> Oups. Something is wrong with your flow. The devlink alloc is allocating
> the structure that holds private data (per-device data) for you. This is
> misuse :/
>
> You are missing one parent device struct apparently.
>
> Oh, I think I see something like it. The unused "struct ionic_devlink".
If I'm not mistaken, the alloc is only allocating enough for a pointer,
not the whole per device struct, and a few lines down from here the
pointer to the new devlink struct is assigned to ionic->dl. This was
based on what I found in the qed driver's qed_devlink_register(), and it
all seems to work.
That unused struct ionic_devlink does need to go away, it was
superfluous after working out a better typecast off of devlink_priv().
I'll remove the unused struct ionic_devlink, but I think the rest is okay.
sln
>
>
>> + if (!dl) {
>> + dev_warn(ionic->dev, "devlink_alloc failed");
>> + return -ENOMEM;
>> + }
>> +
>> + ip = (struct ionic **)devlink_priv(dl);
>> + *ip = ionic;
>> + ionic->dl = dl;
>> +
>> + err = devlink_register(dl, ionic->dev);
>> + if (err) {
>> + dev_warn(ionic->dev, "devlink_register failed: %d\n", err);
>> + goto err_dl_free;
>> + }
>> +
>> + return 0;
>> +
>> +err_dl_free:
>> + ionic->dl = NULL;
>> + devlink_free(dl);
>> + return err;
>> +}
>> +
>> +void ionic_devlink_unregister(struct ionic *ionic)
>> +{
>> + if (!ionic->dl)
>> + return;
>> +
>> + devlink_unregister(ionic->dl);
>> + devlink_free(ionic->dl);
>> +}
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>> new file mode 100644
>> index 000000000000..35528884e29f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#ifndef _IONIC_DEVLINK_H_
>> +#define _IONIC_DEVLINK_H_
>> +
>> +#include <net/devlink.h>
>> +
>> +int ionic_devlink_register(struct ionic *ionic);
>> +void ionic_devlink_unregister(struct ionic *ionic);
>> +
>> +#endif /* _IONIC_DEVLINK_H_ */
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Andrii Nakryiko @ 2019-07-08 19:55 UTC (permalink / raw)
To: Matt Hart
Cc: Andrii Nakryiko, Alexei Starovoitov, bpf, Daniel Borkmann,
Kernel Team, Networking, Stanislav Fomichev
In-Reply-To: <CAH+k93G=qGLfEKe+3dSZPKhmxrc8JiPqDppGa-yLSwaQYRJU=Q@mail.gmail.com>
On Mon, Jul 8, 2019 at 12:27 PM Matt Hart <matthew.hart@linaro.org> wrote:
>
> On Mon, 8 Jul 2019 at 18:58, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 8:11 AM Matt Hart <matthew.hart@linaro.org> wrote:
> > >
> > > Hi all,
> > >
> > > I bisected a perf build error on ARMv7 to this patch:
> > > libbpf.c: In function ‘perf_event_open_probe’:
> > > libbpf.c:4112:17: error: cast from pointer to integer of different
> > > size [-Werror=pointer-to-int-cast]
> > > attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> > > ^
> > >
> > > Is this a known issue?
> >
> > No, thanks for reporting!
> >
> > It should be
> >
> > attr.config1 = (uint64_t)(uintptr_t)(void *)name;
> >
> > to avoid warning on 32-bit architectures.
>
> Tested with manual change and can confirm perf now builds without errors.
Thanks for testing!
I'll add Tested-by: Matt Hart <matthew.hart@linaro.org> when posting a fix.
>
> >
> > I'll post a fix later today, but if you could verify this fixes
> > warning for you, I'd really appreciate that! Thanks!
^ permalink raw reply
* Re: [PATCH v3 6/7] dt-bindings: net: realtek: Add property to configure LED mode
From: Andrew Lunn @ 2019-07-08 19:48 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190708192459.187984-7-mka@chromium.org>
On Mon, Jul 08, 2019 at 12:24:58PM -0700, Matthias Kaehlcke wrote:
> The LED behavior of some Realtek PHYs is configurable. Add the
> property 'realtek,led-modes' to specify the configuration of the
> LEDs.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Hi Matthias
Humm. I thought you were going to drop this and the next patch?
Andrew
^ permalink raw reply
* Re: [PATCH v3 1/7] dt-bindings: net: Add bindings for Realtek PHYs
From: Andrew Lunn @ 2019-07-08 19:46 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190708192459.187984-2-mka@chromium.org>
On Mon, Jul 08, 2019 at 12:24:53PM -0700, Matthias Kaehlcke wrote:
> Add the 'realtek,eee-led-mode-disable' property to disable EEE
> LED mode on Realtek PHYs that support it.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> TODO: adapt PHY core to deal with optional compatible strings
Yes. Does this even work at the moment? I would expect
of_mdiobus_child_is_phy() to return false, indicating the device is
not actually a PHY.
Andrew
^ permalink raw reply
* Re: [PATCH] [net-next] macb: fix build warning for !CONFIG_OF
From: David Miller @ 2019-07-08 19:43 UTC (permalink / raw)
To: arnd
Cc: nicolas.ferre, palmer, paul.walmsley, yash.shah, harini.katakam,
claudiu.beznea, netdev, linux-kernel, linux-riscv
In-Reply-To: <20190708124840.3616530-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 8 Jul 2019 14:48:23 +0200
> When CONFIG_OF is disabled, we get a harmless warning about the
> newly added variable:
>
> drivers/net/ethernet/cadence/macb_main.c:48:39: error: 'mgmt' defined but not used [-Werror=unused-variable]
> static struct sifive_fu540_macb_mgmt *mgmt;
>
> Move the variable closer to its use inside of the #ifdef.
>
> Fixes: c218ad559020 ("macb: Add support for SiFive FU540-C000")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thanks Arnd.
^ permalink raw reply
* Re: [PATCH] [net-next] gve: fix unused variable/label warnings
From: David Miller @ 2019-07-08 19:41 UTC (permalink / raw)
To: arnd; +Cc: csully, sagis, jonolson, willemb, lrizzo, netdev, linux-kernel
In-Reply-To: <20190708124350.3470436-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 8 Jul 2019 14:43:39 +0200
> On unusual page sizes, we get harmless warnings:
>
> drivers/net/ethernet/google/gve/gve_rx.c:283:6: error: unused variable 'pagecount' [-Werror,-Wunused-variable]
> drivers/net/ethernet/google/gve/gve_rx.c:336:1: error: unused label 'have_skb' [-Werror,-Wunused-label]
>
> Change the preprocessor #if to regular if() to avoid this.
>
> Fixes: f5cedc84a30d ("gve: Add transmit and receive support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: stmmac: Re-work the queue selection for TSO packets
From: David Miller @ 2019-07-08 19:41 UTC (permalink / raw)
To: Jose.Abreu
Cc: netdev, Joao.Pinto, peppe.cavallaro, alexandre.torgue,
mcoquelin.stm32, linux-stm32, linux-arm-kernel, linux-kernel, ben
In-Reply-To: <36018491f47206728e04d67a9e6263635e64f721.1562588640.git.joabreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 8 Jul 2019 14:26:28 +0200
> Ben Hutchings says:
> "This is the wrong place to change the queue mapping.
> stmmac_xmit() is called with a specific TX queue locked,
> and accessing a different TX queue results in a data race
> for all of that queue's state.
>
> I think this commit should be reverted upstream and in all
> stable branches. Instead, the driver should implement the
> ndo_select_queue operation and override the queue mapping there."
>
> Fixes: c5acdbee22a1 ("net: stmmac: Send TSO packets always from Queue 0")
> Suggested-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Applied and queued up for -stable.
^ 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