* [PATCH net] bpf/xskmap: Return ERR_PTR for failure case instead of NULL.
From: Jonathan Lemon @ 2019-09-24 16:24 UTC (permalink / raw)
To: daniel, bjorn.topel, netdev; +Cc: kernel-team
When kzalloc() failed, NULL was returned to the caller, which
tested the pointer with IS_ERR(), which didn't match, so the
pointer was used later, resulting in a NULL dereference.
Return ERR_PTR(-ENOMEM) instead of NULL.
Reported-by: syzbot+491c1b7565ba9069ecae@syzkaller.appspotmail.com
Fixes: 0402acd683c6 ("xsk: remove AF_XDP socket from map when the socket is released")
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
kernel/bpf/xskmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index 942c662e2eed..82a1ffe15dfa 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -37,7 +37,7 @@ static struct xsk_map_node *xsk_map_node_alloc(struct xsk_map *map,
node = kzalloc(sizeof(*node), GFP_ATOMIC | __GFP_NOWARN);
if (!node)
- return NULL;
+ return ERR_PTR(-ENOMEM);
err = xsk_map_inc(map);
if (err) {
--
2.17.1
^ permalink raw reply related
* [PATCH net] bpf/xskmap: Return ERR_PTR for failure case instead of NULL.
From: Jonathan Lemon @ 2019-09-24 16:25 UTC (permalink / raw)
To: daniel, bjorn.topel, netdev; +Cc: kernel-team
When kzalloc() failed, NULL was returned to the caller, which
tested the pointer with IS_ERR(), which didn't match, so the
pointer was used later, resulting in a NULL dereference.
Return ERR_PTR(-ENOMEM) instead of NULL.
Reported-by: syzbot+491c1b7565ba9069ecae@syzkaller.appspotmail.com
Fixes: 0402acd683c6 ("xsk: remove AF_XDP socket from map when the socket is released")
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
kernel/bpf/xskmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index 942c662e2eed..82a1ffe15dfa 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -37,7 +37,7 @@ static struct xsk_map_node *xsk_map_node_alloc(struct xsk_map *map,
node = kzalloc(sizeof(*node), GFP_ATOMIC | __GFP_NOWARN);
if (!node)
- return NULL;
+ return ERR_PTR(-ENOMEM);
err = xsk_map_inc(map);
if (err) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 4.19-stable 0/7] mlx5 checksum fixes for 4.19
From: Greg KH @ 2019-09-24 16:28 UTC (permalink / raw)
To: David Miller; +Cc: saeedm, netdev, stable
In-Reply-To: <20190923.175106.799482393811705736.davem@davemloft.net>
On Mon, Sep 23, 2019 at 05:51:06PM +0200, David Miller wrote:
> From: Saeed Mahameed <saeedm@mellanox.com>
> Date: Mon, 23 Sep 2019 12:39:57 +0000
>
> > This series includes some upstream patches aimed to fix multiple checksum
> > issues with mlx5 driver in 4.19-stable kernels.
> >
> > Since the patches didn't apply cleanly to 4.19 back when they were
> > submitted for the first time around 5.1 kernel release to the netdev
> > mailing list, i couldn't mark them for -stable 4.19, so now as the issue
> > is being reported on 4.19 LTS kernels, I had to do the backporting and
> > this submission myself.
> >
> > This series required some dependency patches and some manual touches
> > to apply some of them.
> >
> > Please apply to 4.19-stable and let me know if there's any problem.
> > I tested and the patches apply cleanly and work on top of: v4.19.75
>
> FWIW, I'm fine with this.
Thanks for the review, will go queue these up now...
greg k-h
^ permalink raw reply
* Re: [RFC PATCH bpf-next v2 1/2] bpf: adding map batch processing support
From: Yonghong Song @ 2019-09-24 16:28 UTC (permalink / raw)
To: Brian Vazquez
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, Kernel Team,
Jakub Kicinski, Brian Vazquez, Stanislav Fomichev
In-Reply-To: <CABCgpaULua4BQAwr8F4+azGBV5A1JLyEtth_+M-NLLjowC8owA@mail.gmail.com>
On 9/23/19 4:20 PM, Brian Vazquez wrote:
> Hi Yonghong, thanks for working on this!
>
> I have some concerns about this implementation but overall I think
> this might work for our use case too!
Thanks for reviewing the patch. I will get back to this very soon.
>
> On Sun, Sep 8, 2019 at 1:11 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Brian Vazquez has proposed BPF_MAP_DUMP command to look up more than one
>> map entries per syscall.
>> https://lore.kernel.org/bpf/CABCgpaU3xxX6CMMxD+1knApivtc2jLBHysDXw-0E9bQEL0qC3A@mail.gmail.com/T/#t
>>
>> During discussion, we found more use cases can be supported in a similar
>> map operation batching framework. For example, batched map lookup and delete,
>> which can be really helpful for bcc.
>> https://github.com/iovisor/bcc/blob/master/tools/tcptop.py#L233-L243
>> https://github.com/iovisor/bcc/blob/master/tools/slabratetop.py#L129-L138
>>
>> Also, in bcc, we have API to delete all entries in a map.
>> https://github.com/iovisor/bcc/blob/master/src/cc/api/BPFTable.h#L257-L264
>>
>> For map update, batched operations also useful as sometimes applications need
>> to populate initial maps with more than one entry. For example, the below
>> example is from kernel/samples/bpf/xdp_redirect_cpu_user.c:
>> https://github.com/torvalds/linux/blob/master/samples/bpf/xdp_redirect_cpu_user.c#L543-L550
>>
>> This patch addresses all the above use cases. To make uapi stable, it also
>> covers other potential use cases. For bpf syscall subcommands are introduced:
>> BPF_MAP_LOOKUP_BATCH
>> BPF_MAP_LOOKUP_AND_DELETE_BATCH
>> BPF_MAP_UPDATE_BATCH
>> BPF_MAP_DELETE_BATCH
>>
>> The UAPI attribute structure looks like:
>>
>> struct { /* struct used by BPF_MAP_*_BATCH commands */
>> __u64 batch; /* input/output:
>> * input: start batch,
>> * 0 to start from beginning.
>> * output: next start batch,
>> * 0 to end batching.
>> */
>> __aligned_u64 keys;
>> __aligned_u64 values;
>> __u32 count; /* input/output:
>> * input: # of elements keys/values.
>> * output: # of filled elements.
>> */
>> __u32 map_fd;
>> __u64 elem_flags;
>> __u64 flags;
>> } batch;
>>
>> An opaque value 'batch' is used for user/kernel space communication
>> for where in the map to start the operation for lookup/lookup_and_delete/delete.
>> input 'batch' = 0: to start the operation from the beginning of the map.
>> output 'batch': if not 0, the next input for batch operation.
>>
>> For lookup/lookup_and_delete:
>> operation: lookup/lookup_and_delete starting from a particular 'batch'.
>> return:
>> 'batch' 'count' return code meaning
>> 0 0 0 Done. Nothing left
>> 0 0 -ENOSPC no space to handle batch 0
>> > 0 0 -ENOSPC no space to handle 'batch'
>> > 0 > 0 0 stopped right before 'batch'
>> Note that:
>> (1). Even if return code is 0 and return 'count' > 0, the return 'count' may
>> not be equal to input 'count'. This happens when there is no enough space
>> to handle a batch.
>> (2). If the return code is an error and not -EFAULT,
>> 'batch' indicates the batch has issues and 'count' indicates the number
>> of elements successfully processed.
>>
>> For delete:
>> operation: deletion starting from a particular 'batch'.
>> return: 0 means everything is deleted from 'batch'.
>> error code means something deletion not happening.
>>
>> For update:
>> operation: update 'count' number of elements in 'keys'/'values'.
>> return: 0 means successful updates for all elements.
>> error code, if not -EFAULT, 'count' is the number of successful updates.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 9 ++
>> include/uapi/linux/bpf.h | 22 +++
>> kernel/bpf/hashtab.c | 324 +++++++++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 68 ++++++++
>> 4 files changed, 423 insertions(+)
>>
[...]
>> +static int
>> +__htab_map_lookup_and_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr,
>> + bool do_delete, bool is_lru_map)
>> +{
>> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>> + u32 bucket_cnt, total, key_size, value_size, roundup_key_size;
>> + void *keys = NULL, *values = NULL, *value, *dst_key, *dst_val;
>> + u64 elem_map_flags, map_flags;
>> + struct hlist_nulls_head *head;
>> + void __user *ukeys, *uvalues;
>> + struct hlist_nulls_node *n;
>> + u32 batch, max_count;
>> + unsigned long flags;
>> + struct htab_elem *l;
>> + struct bucket *b;
>> + int ret = 0;
>> +
>> + max_count = attr->batch.count;
>> + if (!max_count)
>> + return 0;
>> +
>> + elem_map_flags = attr->batch.elem_flags;
>> + if ((elem_map_flags & ~BPF_F_LOCK) ||
>> + ((elem_map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map)))
>> + return -EINVAL;
>> +
>> + map_flags = attr->batch.flags;
>> + if (map_flags)
>> + return -EINVAL;
>> +
>> + batch = (u32)attr->batch.batch;
>> + if (batch >= htab->n_buckets)
>> + return -EINVAL;
>> +
>> + /* We cannot do copy_from_user or copy_to_user inside
>> + * the rcu_read_lock. Allocate enough space here.
>> + */
>> + key_size = htab->map.key_size;
>> + roundup_key_size = round_up(htab->map.key_size, 8);
>> + value_size = htab->map.value_size;
>> + keys = kvmalloc(key_size * max_count, GFP_USER | __GFP_NOWARN);
>> + values = kvmalloc(value_size * max_count, GFP_USER | __GFP_NOWARN);
>> + if (!keys || !values) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + dst_key = keys;
>> + dst_val = values;
>> + total = 0;
>> +
>> + preempt_disable();
>> + this_cpu_inc(bpf_prog_active);
>> + rcu_read_lock();
>> +
>> +again:
>> + b = &htab->buckets[batch];
>> + head = &b->head;
>> + raw_spin_lock_irqsave(&b->lock, flags);
>> +
>
> Would it be possible to avoid that lock when we're not deleting (just
> batching lookup)? To be honest I don't how much impact would have to
> grab that lock when concurrent additions are happening in a bpf
> program.
Yes, it is possible.
Another reason I took the lock to ensure that when I tested
that bucket_cnt is smaller or equal to the remaining buffer size,
it should remain that way during subsequent elem copying.
Without lock, if in parallel some updates added some
elements and there will be no enough space to copy.
But on the other hand, I think it is okay to just stop
when the buffer is full and miss a few elements.
>
>> + bucket_cnt = 0;
>> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
>> + bucket_cnt++;
>> +
>> + if (bucket_cnt > (max_count - total)) {
>> + if (total == 0)
>> + ret = -ENOSPC;
>> + goto after_loop;
>> + }
>> +
>> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
>> + memcpy(dst_key, l->key, key_size);
>> +
>> + value = l->key + roundup_key_size;
>> + if (elem_map_flags & BPF_F_LOCK)
>> + copy_map_value_locked(map, dst_val, value, true);
>> + else
>> + copy_map_value(map, dst_val, value);
>> + check_and_init_map_lock(map, dst_val);
>> +
>> + dst_key += key_size;
>> + dst_val += value_size;
>> + total++;
>> + }
>> +
>> + if (do_delete) {
>> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
>> + hlist_nulls_del_rcu(&l->hash_node);
>> + if (is_lru_map)
>> + bpf_lru_push_free(&htab->lru, &l->lru_node);
>> + else
>> + free_htab_elem(htab, l);
>> + }
>> + }
>> +
>> + batch++;
>> + if (batch >= htab->n_buckets) {
>> + batch = 0;
>> + goto after_loop;
>> + }
>> +
>> + raw_spin_unlock_irqrestore(&b->lock, flags);
>> + goto again;
>> +
>> +after_loop:
>> + raw_spin_unlock_irqrestore(&b->lock, flags);
>> +
>> + rcu_read_unlock();
>> + this_cpu_dec(bpf_prog_active);
>> + preempt_enable();
>> +
>> + /* copy data back to user */
>> + ukeys = u64_to_user_ptr(attr->batch.keys);
>> + uvalues = u64_to_user_ptr(attr->batch.values);
>> + if (put_user(batch, &uattr->batch.batch) ||
>> + copy_to_user(ukeys, keys, total * key_size) ||
>> + copy_to_user(uvalues, values, total * value_size) ||
>> + put_user(total, &uattr->batch.count))
>> + ret = -EFAULT;
>> +
>> +out:
>> + kvfree(keys);
>> + kvfree(values);
>> + return ret;
>> +}
>> +
>> +static int
>> +__htab_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
>> + union bpf_attr __user *uattr, bool is_lru_map)
>> +{
>> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>> + u32 count, max_count, key_size, roundup_key_size, value_size;
>> + u64 elem_map_flags, map_flags;
>> + void __user *ukey, *uvalue;
>> + void *key, *value;
>> + int ret = 0;
>> +
>> + max_count = attr->batch.count;
>> + if (!max_count)
>> + return 0;
>> +
>> + elem_map_flags = attr->batch.elem_flags;
>> + if ((elem_map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map))
>> + return -EINVAL;
>> +
>> + map_flags = attr->batch.flags;
>> + if (map_flags)
>> + return -EINVAL;
>> +
>> + key_size = htab->map.key_size;
>> + roundup_key_size = round_up(htab->map.key_size, 8);
>> + value_size = htab->map.value_size;
>> + key = kmalloc(key_size, GFP_USER | __GFP_NOWARN);
>> + value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
>> + if (!key || !value) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + ukey = u64_to_user_ptr(attr->batch.keys);
>> + uvalue = u64_to_user_ptr(attr->batch.values);
>> + for (count = 0; count < max_count; count++) {
>> + if (copy_from_user(key, ukey + count * key_size, key_size) ||
>> + copy_from_user(value, uvalue + count * value_size, value_size)) {
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>> + preempt_disable();
>> + __this_cpu_inc(bpf_prog_active);
>> + rcu_read_lock();
>> + if (is_lru_map)
>> + ret = htab_lru_map_update_elem(map, key, value, elem_map_flags);
>> + else
>> + ret = htab_map_update_elem(map, key, value, elem_map_flags);
>> + rcu_read_unlock();
>> + __this_cpu_dec(bpf_prog_active);
>> + preempt_enable();
>> +
>> + if (ret) {
>> + if (put_user(count, &uattr->batch.count))
>> + ret = -EFAULT;
>> + break;
>> + }
>> + }
>> +
>> +out:
>> + kfree(key);
>> + kfree(value);
>> + return ret;
>> +}
>> +
>> +static int
>> +__htab_map_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr,
>> + bool is_lru_map)
>> +{
>> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>> + u64 elem_map_flags, map_flags;
>> + struct hlist_nulls_head *head;
>> + struct hlist_nulls_node *n;
>> + u32 batch, max_count;
>> + unsigned long flags;
>> + struct htab_elem *l;
>> + struct bucket *b;
>> +
>> + elem_map_flags = attr->batch.elem_flags;
>> + map_flags = attr->batch.flags;
>> + if (elem_map_flags || map_flags)
>> + return -EINVAL;
>> +
>> + max_count = attr->batch.count;
>> + batch = (u32)attr->batch.batch;
>> + if (max_count || batch >= htab->n_buckets)
>> + return -EINVAL;
>> +
>> + preempt_disable();
>> + __this_cpu_inc(bpf_prog_active);
>> + rcu_read_lock();
>> +
>> +again:
>> + b = &htab->buckets[batch];
>> + head = &b->head;
>> + raw_spin_lock_irqsave(&b->lock, flags);
>> +
>> + hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
>> + hlist_nulls_del_rcu(&l->hash_node);
>> + if (is_lru_map)
>> + bpf_lru_push_free(&htab->lru, &l->lru_node);
>> + else
>> + free_htab_elem(htab, l);
>> + }
>> +
>> + batch++;
>> + if (batch >= htab->n_buckets)
>> + goto out;
>> +
>> + raw_spin_unlock_irqrestore(&b->lock, flags);
>> + goto again;
>> +
>> +out:
>> + raw_spin_unlock_irqrestore(&b->lock, flags);
>> + rcu_read_unlock();
>> + __this_cpu_dec(bpf_prog_active);
>> + preempt_enable();
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +htab_map_lookup_batch(struct bpf_map *map, const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, false,
>> + false);
>> +}
>> +
>> +static int
>> +htab_map_lookup_and_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, true,
>> + false);
>> +}
>> +
>> +static int
>> +htab_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_update_batch(map, attr, uattr, false);
>> +}
>> +
>> +static int
>> +htab_map_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_delete_batch(map, attr, uattr, false);
>> +}
>> +
>> +static int
>> +htab_lru_map_lookup_batch(struct bpf_map *map, const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, false,
>> + true);
>> +}
>> +
>> +static int
>> +htab_lru_map_lookup_and_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_lookup_and_delete_batch(map, attr, uattr, true,
>> + true);
>> +}
>> +
>> +static int
>> +htab_lru_map_update_batch(struct bpf_map *map, const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_update_batch(map, attr, uattr, true);
>> +}
>> +
>> +static int
>> +htab_lru_map_delete_batch(struct bpf_map *map,
>> + const union bpf_attr *attr,
>> + union bpf_attr __user *uattr)
>> +{
>> + return __htab_map_delete_batch(map, attr, uattr, true);
>> +}
>> +
>> const struct bpf_map_ops htab_map_ops = {
>> .map_alloc_check = htab_map_alloc_check,
>> .map_alloc = htab_map_alloc,
>> @@ -1242,6 +1558,10 @@ const struct bpf_map_ops htab_map_ops = {
>> .map_delete_elem = htab_map_delete_elem,
>> .map_gen_lookup = htab_map_gen_lookup,
>> .map_seq_show_elem = htab_map_seq_show_elem,
>> + .map_lookup_batch = htab_map_lookup_batch,
>> + .map_lookup_and_delete_batch = htab_map_lookup_and_delete_batch,
>> + .map_update_batch = htab_map_update_batch,
>> + .map_delete_batch = htab_map_delete_batch,
>> };
>>
>> const struct bpf_map_ops htab_lru_map_ops = {
>> @@ -1255,6 +1575,10 @@ const struct bpf_map_ops htab_lru_map_ops = {
>> .map_delete_elem = htab_lru_map_delete_elem,
>> .map_gen_lookup = htab_lru_map_gen_lookup,
>> .map_seq_show_elem = htab_map_seq_show_elem,
>> + .map_lookup_batch = htab_lru_map_lookup_batch,
>> + .map_lookup_and_delete_batch = htab_lru_map_lookup_and_delete_batch,
>> + .map_update_batch = htab_lru_map_update_batch,
>> + .map_delete_batch = htab_lru_map_delete_batch,
>> };
>>
>> /* Called from eBPF program */
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index ca60eafa6922..e83bdf7efbd8 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -2816,6 +2816,62 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
>> return err;
>> }
>>
>> +#define BPF_MAP_BATCH_LAST_FIELD batch.flags
>> +
>> +#define BPF_DO_BATCH(fn) \
>> + do { \
>> + if (!fn) { \
>> + err = -ENOTSUPP; \
>> + goto err_put; \
>> + } \
>> + err = fn(map, attr, uattr); \
>> + } while(0)
>> +
>> +static int bpf_map_do_batch(const union bpf_attr *attr,
>> + union bpf_attr __user *uattr,
>> + int cmd)
>> +{
>> + struct bpf_map *map;
>> + int err, ufd;
>> + struct fd f;
>> +
>> + if (CHECK_ATTR(BPF_MAP_BATCH))
>> + return -EINVAL;
>> +
>> + ufd = attr->batch.map_fd;
>> + f = fdget(ufd);
>> + map = __bpf_map_get(f);
>> + if (IS_ERR(map))
>> + return PTR_ERR(map);
>> +
>> + if ((cmd == BPF_MAP_LOOKUP_BATCH ||
>> + cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH) &&
>> + !(map_get_sys_perms(map, f) & FMODE_CAN_READ)) {
>> + err = -EPERM;
>> + goto err_put;
>> + }
>> +
>> + if (cmd != BPF_MAP_LOOKUP_BATCH &&
>> + !(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) {
>> + err = -EPERM;
>> + goto err_put;
>> + }
>> +
>> + if (cmd == BPF_MAP_LOOKUP_BATCH) {
>> + BPF_DO_BATCH(map->ops->map_lookup_batch);
>> + } else if (cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH) {
>> + BPF_DO_BATCH(map->ops->map_lookup_and_delete_batch);
>> + } else if (cmd == BPF_MAP_UPDATE_BATCH) {
>> + BPF_DO_BATCH(map->ops->map_update_batch);
>> + } else {
>> + BPF_DO_BATCH(map->ops->map_delete_batch);
>> + }
>> +
>> +err_put:
>> + fdput(f);
>> + return err;
>> +}
>> +
>> SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
>> {
>> union bpf_attr attr = {};
>> @@ -2913,6 +2969,18 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
>> case BPF_MAP_LOOKUP_AND_DELETE_ELEM:
>> err = map_lookup_and_delete_elem(&attr);
>> break;
>> + case BPF_MAP_LOOKUP_BATCH:
>> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_LOOKUP_BATCH);
>> + break;
>> + case BPF_MAP_LOOKUP_AND_DELETE_BATCH:
>> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_LOOKUP_AND_DELETE_BATCH);
>> + break;
>> + case BPF_MAP_UPDATE_BATCH:
>> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_UPDATE_BATCH);
>> + break;
>> + case BPF_MAP_DELETE_BATCH:
>> + err = bpf_map_do_batch(&attr, uattr, BPF_MAP_DELETE_BATCH);
>> + break;
>> default:
>> err = -EINVAL;
>> break;
>> --
>> 2.17.1
>>
>
> In general it'd be great if we could express the old functions
> (get_next_key, lookup, delete) in terms of this new ones so we avoid
> having twice the code.
They do share some initial checkings at least. Let me check how I can do
this.
>
> Also to be honest I don't see how batching updates would be useful,
> maybe just try to do batching lookup and lookup_and_delete?
We do have a use case for batching updates.
Internally we have a bpf based firewall. it uses map-in-map.
The inner map is a hash table with thousands of entries.
Periodically some user space alerts will trigger firewall
rule changes. In this case, the new map is prepared
and thousands of bpf update syscalls are called to
update the new map. Once the new map is ready,
a final update into outer map will enable using the
new map.
Replacing these thousands map update syscalls with just
one map batch update is still a very good optimization.
^ permalink raw reply
* Re: Linux 5.4 - bpf test build fails
From: Shuah Khan @ 2019-09-24 16:39 UTC (permalink / raw)
To: Cristian Marussi, Alexei Starovoitov, Daniel Borkmann
Cc: open list:KERNEL SELFTEST FRAMEWORK, bpf, Networking,
David S. Miller, Shuah Khan
In-Reply-To: <34a9bd63-a251-0b4f-73b6-06b9bbf9d3fa@linuxfoundation.org>
On 9/24/19 10:03 AM, Shuah Khan wrote:
> On 9/24/19 9:52 AM, Cristian Marussi wrote:
>> Hi Shuah
>>
>> On 24/09/2019 16:26, Shuah Khan wrote:
>>> Hi Alexei and Daniel,
>>>
>>> bpf test doesn't build on Linux 5.4 mainline. Do you know what's
>>> happening here.
>>>
>>>
>>> make -C tools/testing/selftests/bpf/
>>
>> side question, since I'm writing arm64/ tests.
>>
>> my "build-testcases" following the KSFT docs are:
>>
>> make kselftest
>> make TARGETS=arm64 kselftest
>> make -C tools/testing/selftests/
>> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
>> make TARGETS=arm64 -C tools/testing/selftests/
>> make TARGETS=arm64 -C tools/testing/selftests/
>> INSTALL_PATH=<install-path> install
>> ./kselftest_install.sh <install-path>
Cristian,
That being said, I definitely want to see this list limited to
a few options.
One problem is that if somebody wants to do just a build, there
is no option from the main makefile. I have sent support for that
a few months ago and the patch didn't got lost it appears. I am
working on resending those patches. The same is true for install.
I sent in a patch for that a while back and I am going to resend.
These will make it easier for users.
I would really want to get to supporting only these options:
These are supported now:
make kselftest
make TARGETS=arm64 kselftest (one or more targets)
Replace the following:
make -C tools/testing/selftests/ with
make kselftes_build option from main makefile
Replace this:
make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
with
make kselftest_install
That way we can support all the use-cases from the main Makefile
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH net] skge: fix checksum byte order
From: Florian Fainelli @ 2019-09-24 16:49 UTC (permalink / raw)
To: Stephen Hemminger, davem; +Cc: netdev
In-Reply-To: <20190920161826.15942-1-sthemmin@microsoft.com>
On 9/20/19 9:18 AM, Stephen Hemminger wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> Running old skge driver on PowerPC causes checksum errors
> because hardware reported 1's complement checksum is in little-endian
> byte order.
>
> Reported-by: Benoit <benoit.sansoni@gmail.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 383181ac7e59 ("[PATCH] skge: check length from PHY")
> ---
> drivers/net/ethernet/marvell/skge.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
> index 0a2ec387a482..095f6c71b4fa 100644
> --- a/drivers/net/ethernet/marvell/skge.c
> +++ b/drivers/net/ethernet/marvell/skge.c
> @@ -3108,7 +3108,7 @@ static struct sk_buff *skge_rx_get(struct net_device *dev,
> skb_put(skb, len);
>
> if (dev->features & NETIF_F_RXCSUM) {
> - skb->csum = csum;
> + skb->csum = le16_to_cpu(csum);
> skb->ip_summed = CHECKSUM_COMPLETE;
> }
>
>
--
Florian
^ permalink raw reply
* Re: [PATCH V2 net 1/1] net/tls(TLS_SW): Fix list_del double free caused by a race condition in tls_tx_records
From: Pooja Trivedi @ 2019-09-24 16:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, daniel, john.fastabend, davejwatson, aviadye,
borisp, Pooja Trivedi, Mallesham Jatharakonda
In-Reply-To: <20190923172811.1f620803@cakuba.netronome.com>
On Mon, Sep 23, 2019 at 8:28 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Sat, 21 Sep 2019 23:19:20 -0400, Pooja Trivedi wrote:
> > On Wed, Sep 18, 2019 at 5:45 PM Jakub Kicinski wrote:
> > > On Wed, 18 Sep 2019 17:37:44 -0400, Pooja Trivedi wrote:
> > > > Hi Jakub,
> > > >
> > > > I have explained one potential way for the race to happen in my
> > > > original message to the netdev mailing list here:
> > > > https://marc.info/?l=linux-netdev&m=156805120229554&w=2
> > > >
> > > > Here is the part out of there that's relevant to your question:
> > > >
> > > > -----------------------------------------
> > > >
> > > > One potential way for race condition to appear:
> > > >
> > > > When under tcp memory pressure, Thread 1 takes the following code path:
> > > > do_sendfile ---> ... ---> .... ---> tls_sw_sendpage --->
> > > > tls_sw_do_sendpage ---> tls_tx_records ---> tls_push_sg --->
> > > > do_tcp_sendpages ---> sk_stream_wait_memory ---> sk_wait_event
> > >
> > > Ugh, so do_tcp_sendpages() can also release the lock :/
> > >
> > > Since the problem occurs in tls_sw_do_sendpage() and
> > > tls_sw_do_sendmsg() as well, should we perhaps fix it at that level?
> >
> > That won't do because tls_tx_records also gets called when completion
> > callbacks schedule delayed work. That was the code path that caused
> > the crash for my test. Cavium's nitrox crypto offload driver calling
> > tls_encrypt_done, which calls schedule_delayed_work. Delayed work that
> > was scheduled would then be processed by tx_work_handler.
> > Notice in my previous reply,
> > "Thread 2 code path:
> > tx_work_handler ---> tls_tx_records"
> >
> > "Thread 2 code path:
> > tx_work_handler ---> tls_tx_records"
>
> Right, the work handler would obviously also have to obey the exclusion
> mechanism of choice.
>
> Having said that this really does feel like we are trying to lock code,
> not data here :(
Agree with you and exactly the thought process I went through. So what
are some other options?
1) A lock member inside of ctx to protect tx_list
We are load testing ktls offload with nitrox and the performance was
quite adversely affected by this. This approach can be explored more,
but the original design of using socket lock didn't follow this model
either.
2) Allow tagging of individual record inside of tx_list to indicate if
it has been 'processed'
This approach would likely protect the data without compromising
performance. It will allow Thread 2 to proceed with the TX portion of
tls_tx_records while Thread 1 sleeps waiting for memory. There will
need to be careful cleanup and backtracking after the thread wakes up
to ensure a consistent state of tx_list and record transmission.
The approach has several problems, however -- (a) It could cause
out-of-order record tx (b) If Thread 1 is waiting for memory, Thread 2
most likely will (c) Again, socket lock wasn't designed to follow this
model to begin with
Given that socket lock essentially was working as a code protector --
as an exclusion mechanism to allow only a single writer through
tls_tx_records at a time -- what other clean ways do we have to fix
the race without a significant refactor of the design and code?
^ permalink raw reply
* Re: [PATCH net v3 0/3] Fix Qdisc destroy issues caused by adding fine-grained locking to filter API
From: Cong Wang @ 2019-09-24 16:42 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <20190924155118.2488-1-vladbu@mellanox.com>
On Tue, Sep 24, 2019 at 8:51 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> TC filter API unlocking introduced several new fine-grained locks. The
> change caused sleeping-while-atomic BUGs in several Qdiscs that call cls
> APIs which need to obtain new mutex while holding sch tree spinlock. This
> series fixes affected Qdiscs by ensuring that cls API that became sleeping
> is only called outside of sch tree lock critical section.
>
> Vlad Buslov (3):
> net: sched: sch_htb: don't call qdisc_put() while holding tree lock
> net: sched: multiq: don't call qdisc_put() while holding tree lock
> net: sched: sch_sfb: don't call qdisc_put() while holding tree lock
>
For the whole series:
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Thanks.
^ permalink raw reply
* Re: [PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
From: Wei Wang @ 2019-09-24 17:02 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linux Kernel Network Developers, lkml, David S . Miller, stable
In-Reply-To: <20190924140128.19394-1-Jason@zx2c4.com>
On Tue, Sep 24, 2019 at 7:01 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case added here
> causes the reference underflow by causing packets to evaluate a suppress
> rule.
>
> Cc: stable@vger.kernel.org
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>
Good catch. Thanks for the fix.
> net/ipv6/fib6_rules.c | 3 ++-
> tools/testing/selftests/net/fib_tests.sh | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index d22b6c140f23..f9e8fe3ff0c5 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
> return false;
>
> suppress_route:
> - ip6_rt_put(rt);
> + if (!(arg->flags & FIB_LOOKUP_NOREF))
> + ip6_rt_put(rt);
> return true;
> }
>
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 4465fc2dae14..c2c5f2bf0f95 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -9,7 +9,7 @@ ret=0
> ksft_skip=4
>
> # all tests in this script. Can be overridden with -t option
> -TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
> +TESTS="unregister down carrier nexthop suppress ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
>
> VERBOSE=0
> PAUSE_ON_FAIL=no
> @@ -614,6 +614,20 @@ fib_nexthop_test()
> cleanup
> }
>
> +fib_suppress_test()
> +{
> + $IP link add dummy1 type dummy
> + $IP link set dummy1 up
> + $IP -6 route add default dev dummy1
> + $IP -6 rule add table main suppress_prefixlength 0
> + ping -f -c 1000 -W 1 1234::1 || true
> + $IP -6 rule del table main suppress_prefixlength 0
> + $IP link del dummy1
> +
> + # If we got here without crashing, we're good.
> + return 0
> +}
> +
> ################################################################################
> # Tests on route add and replace
>
> @@ -1591,6 +1605,7 @@ do
> fib_carrier_test|carrier) fib_carrier_test;;
> fib_rp_filter_test|rp_filter) fib_rp_filter_test;;
> fib_nexthop_test|nexthop) fib_nexthop_test;;
> + fib_suppress_test|suppress) fib_suppress_test;;
> ipv6_route_test|ipv6_rt) ipv6_route_test;;
> ipv4_route_test|ipv4_rt) ipv4_route_test;;
> ipv6_addr_metric) ipv6_addr_metric_test;;
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
From: Vijay Khemka @ 2019-09-24 17:26 UTC (permalink / raw)
To: David S. Miller, Florian Fainelli, YueHaibing, Andrew Lunn,
Kate Stewart, Mauro Carvalho Chehab, Luis Chamberlain,
Thomas Gleixner, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-aspeed@lists.ozlabs.org,
joel@jms.id.au
Cc: openbmc @ lists . ozlabs . org, Sai Dasari
In-Reply-To: <8C4C62C8-8835-4D33-B317-A75DD1DBB7A3@fb.com>
Florian/Joel,
Can you please look into below patch and let me know who can apply this.
Regards
-Vijay
On 9/17/19, 12:34 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
Please review below patch and provide your valuable feedback.
Regards
-Vijay
On 9/11/19, 1:05 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote:
HW checksum generation is not working for AST2500, specially with IPV6
over NCSI. All TCP packets with IPv6 get dropped. By disabling this
it works perfectly fine with IPV6. As it works for IPV4 so enabled
hw checksum back for IPV4.
Verified with IPV6 enabled and can do ssh.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
Changes since v1:
Enabled IPV4 hw checksum generation as it works for IPV4.
drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 030fed65393e..0255a28d2958 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device *pdev)
/* AST2400 doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->hw_features &= ~NETIF_F_HW_CSUM;
+
+ /* AST2500 doesn't have working HW checksum generation for IPV6
+ * but it works for IPV4, so disabling hw checksum and enabling
+ * it for only IPV4.
+ */
+ if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
+ netdev->hw_features &= ~NETIF_F_HW_CSUM;
+ netdev->hw_features |= NETIF_F_IP_CSUM;
+ }
+
if (np && of_get_property(np, "no-hw-checksum", NULL))
- netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+ netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM
+ | NETIF_F_IP_CSUM);
netdev->features |= netdev->hw_features;
/* register network device */
--
2.17.1
^ permalink raw reply
* Re: Linux 5.4 - bpf test build fails
From: Cristian Marussi @ 2019-09-24 17:29 UTC (permalink / raw)
To: Shuah Khan, Alexei Starovoitov, Daniel Borkmann
Cc: open list:KERNEL SELFTEST FRAMEWORK, bpf, Networking,
David S. Miller
In-Reply-To: <a603ee8e-b0af-6506-0667-77269b0951b2@linuxfoundation.org>
Hi Shuah
On 24/09/2019 17:39, Shuah Khan wrote:
> On 9/24/19 10:03 AM, Shuah Khan wrote:
>> On 9/24/19 9:52 AM, Cristian Marussi wrote:
>>> Hi Shuah
>>>
>>> On 24/09/2019 16:26, Shuah Khan wrote:
>>>> Hi Alexei and Daniel,
>>>>
>>>> bpf test doesn't build on Linux 5.4 mainline. Do you know what's
>>>> happening here.
>>>>
>>>>
>>>> make -C tools/testing/selftests/bpf/
>>>
>>> side question, since I'm writing arm64/ tests.
>>>
>>> my "build-testcases" following the KSFT docs are:
>>>
>>> make kselftest
>>> make TARGETS=arm64 kselftest
>>> make -C tools/testing/selftests/
>>> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
>>> make TARGETS=arm64 -C tools/testing/selftests/
>>> make TARGETS=arm64 -C tools/testing/selftests/
>>> INSTALL_PATH=<install-path> install
>>> ./kselftest_install.sh <install-path>
>
> Cristian,
>
> That being said, I definitely want to see this list limited to
> a few options.
>
> One problem is that if somebody wants to do just a build, there
> is no option from the main makefile. I have sent support for that
> a few months ago and the patch didn't got lost it appears. I am
> working on resending those patches. The same is true for install.
> I sent in a patch for that a while back and I am going to resend.
> These will make it easier for users.
>
> I would really want to get to supporting only these options:
>
> These are supported now:
>
> make kselftest
> make TARGETS=arm64 kselftest (one or more targets)
>
> Replace the following:
>
> make -C tools/testing/selftests/ with
>
> make kselftes_build option from main makefile
>
> Replace this:
> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
>
> with
> make kselftest_install
Yes these top level options would be absolutely useful to avoid multiplication
of build targets to support and test.
Moreover, currently, since there was a lot of test growing into arm64/
inside subdirs like arm64/signal, I support (still under review in fact) in the arm64/
toplevel makefile the possibility of building/installing by subdirs only, in order
to be able to limit what you want to build/install of a TARGET (resulting in quicker devel),
issuing something like:
make TARGETS=arm64 SUBTARGETS=signal -C tools/testing/selftests/
if possible, that would be useful if kept functional even in the
new schema. I mean being able to still issue:
make TARGETS=arm64 SUBTARGETS=signal kselftes_build
with the SUBTARGETS= or whatever ENV var handling delegated to the lower level
makefiles (so not handled by the toplevel, but just let go through)
Cheers
Cristian
>
> That way we can support all the use-cases from the main Makefile
>
> thanks,
> -- Shuah
>
>
^ permalink raw reply
* Re: [PATCH V11 0/4] BPF: New helper to obtain namespace data from current task
From: Daniel Borkmann @ 2019-09-24 18:01 UTC (permalink / raw)
To: Carlos Neira; +Cc: netdev, yhs, ebiederm, brouer, bpf
In-Reply-To: <20190924152005.4659-1-cneirabustos@gmail.com>
On Tue, Sep 24, 2019 at 12:20:01PM -0300, Carlos Neira wrote:
> Currently bpf_get_current_pid_tgid(), is used to do pid filtering in bcc's
> scripts but this helper returns the pid as seen by the root namespace which is
> fine when a bcc script is not executed inside a container.
> When the process of interest is inside a container, pid filtering will not work
> if bpf_get_current_pid_tgid() is used.
> This helper addresses this limitation returning the pid as it's seen by the current
> namespace where the script is executing.
>
> In the future different pid_ns files may belong to different devices, according to the
> discussion between Eric Biederman and Yonghong in 2017 Linux plumbers conference.
> To address that situation the helper requires inum and dev_t from /proc/self/ns/pid.
> This helper has the same use cases as bpf_get_current_pid_tgid() as it can be
> used to do pid filtering even inside a container.
>
> Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
>
> Carlos Neira (4):
> fs/nsfs.c: added ns_match
> bpf: added new helper bpf_get_ns_current_pid_tgid
> tools: Added bpf_get_ns_current_pid_tgid helper
> tools/testing/selftests/bpf: Add self-tests for new helper. self tests
> added for new helper
bpf-next is currently closed due to merge window. Please resubmit once back open, thanks.
^ permalink raw reply
* RE: Linux 5.4 - bpf test build fails
From: Tim.Bird @ 2019-09-24 18:07 UTC (permalink / raw)
To: cristian.marussi, skhan, alexei.starovoitov, daniel
Cc: linux-kselftest, bpf, netdev, davem
In-Reply-To: <c3dda8d0-1794-ffd1-4d76-690ac2be8b8f@arm.com>
> -----Original Message-----
> From: Cristian Marussi on Tuesday, September 24, 2019 7:30 AM
>
> Hi Shuah
>
> On 24/09/2019 17:39, Shuah Khan wrote:
> > On 9/24/19 10:03 AM, Shuah Khan wrote:
> >> On 9/24/19 9:52 AM, Cristian Marussi wrote:
> >>> Hi Shuah
> >>>
> >>> On 24/09/2019 16:26, Shuah Khan wrote:
> >>>> Hi Alexei and Daniel,
> >>>>
> >>>> bpf test doesn't build on Linux 5.4 mainline. Do you know what's
> >>>> happening here.
> >>>>
> >>>>
> >>>> make -C tools/testing/selftests/bpf/
> >>>
> >>> side question, since I'm writing arm64/ tests.
> >>>
> >>> my "build-testcases" following the KSFT docs are:
> >>>
> >>> make kselftest
> >>> make TARGETS=arm64 kselftest
> >>> make -C tools/testing/selftests/
> >>> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
> >>> make TARGETS=arm64 -C tools/testing/selftests/
> >>> make TARGETS=arm64 -C tools/testing/selftests/
> >>> INSTALL_PATH=<install-path> install
> >>> ./kselftest_install.sh <install-path>
> >
> > Cristian,
> >
> > That being said, I definitely want to see this list limited to
> > a few options.
> >
> > One problem is that if somebody wants to do just a build, there
> > is no option from the main makefile. I have sent support for that
> > a few months ago and the patch didn't got lost it appears. I am
> > working on resending those patches. The same is true for install.
> > I sent in a patch for that a while back and I am going to resend.
> > These will make it easier for users.
> >
> > I would really want to get to supporting only these options:
> >
> > These are supported now:
> >
> > make kselftest
> > make TARGETS=arm64 kselftest (one or more targets)
> >
> > Replace the following:
> >
> > make -C tools/testing/selftests/ with
> >
> > make kselftes_build option from main makefile
> >
> > Replace this:
> > make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
> >
> > with
> > make kselftest_install
>
> Yes these top level options would be absolutely useful to avoid multiplication
> of build targets to support and test.
>
> Moreover, currently, since there was a lot of test growing into arm64/
> inside subdirs like arm64/signal, I support (still under review in fact) in the
> arm64/
> toplevel makefile the possibility of building/installing by subdirs only, in order
> to be able to limit what you want to build/install of a TARGET (resulting in
> quicker devel),
> issuing something like:
>
> make TARGETS=arm64 SUBTARGETS=signal -C tools/testing/selftests/
>
> if possible, that would be useful if kept functional even in the
> new schema. I mean being able to still issue:
>
> make TARGETS=arm64 SUBTARGETS=signal kselftes_build
From a user perspective, instead of adding a new SUBTARGETS variable,
I would prefer something like the following:
make TARGET=arm64/signal kselftest_build
If you just add a single flat subsidiary namespace, then it doesn't support further
increasing the directory depth in the future.
-- Tim
> with the SUBTARGETS= or whatever ENV var handling delegated to the lower
> level
> makefiles (so not handled by the toplevel, but just let go through)
>
> Cheers
>
> Cristian
>
> >
> > That way we can support all the use-cases from the main Makefile
> >
> > thanks,
> > -- Shuah
> >
> >
^ permalink raw reply
* Re: [PATCH net v3] net/sched: cbs: Fix not adding cbs instance to list
From: Guedes, Andre @ 2019-09-24 18:12 UTC (permalink / raw)
To: Gomes, Vinicius
Cc: Linux Kernel Network Developers, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net
In-Reply-To: <20190924050458.14223-1-vinicius.gomes@intel.com>
Hi Vinicius,
> On Sep 23, 2019, at 10:04 PM, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>
> The problem happens because that when offloading is enabled, the cbs
> instance is not added to the list.
>
> Also, the current code doesn't handle correctly the case when offload
> is disabled without removing the qdisc: if the link speed changes the
> credit calculations will be wrong. When we create the cbs instance
> with offloading enabled, it's not added to the notification list, when
> later we disable offloading, it's not in the list, so link speed
> changes will not affect it.
>
> The solution for both issues is the same, add the cbs instance being
> created unconditionally to the global list, even if the link state
> notification isn't useful "right now".
I believe we could fix both issues described above and still don’t notify the qdisc about link state if we handled the list insertion/removal in cbs_change() instead.
Reading the cbs code more carefully, it seems it would be beneficial to refactor the offload handling. For example, we currently init the qdisc_watchdog even if it’s not useful when offload is enabled. Now, we’re going to notify the qdisc even if it’s not useful too.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH V11 0/4] BPF: New helper to obtain namespace data from current task
From: Carlos Antonio Neira Bustos @ 2019-09-24 18:14 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, yhs, ebiederm, brouer, bpf
In-Reply-To: <20190924180117.GA5889@pc-63.home>
On Tue, Sep 24, 2019 at 08:01:17PM +0200, Daniel Borkmann wrote:
> On Tue, Sep 24, 2019 at 12:20:01PM -0300, Carlos Neira wrote:
> > Currently bpf_get_current_pid_tgid(), is used to do pid filtering in bcc's
> > scripts but this helper returns the pid as seen by the root namespace which is
> > fine when a bcc script is not executed inside a container.
> > When the process of interest is inside a container, pid filtering will not work
> > if bpf_get_current_pid_tgid() is used.
> > This helper addresses this limitation returning the pid as it's seen by the current
> > namespace where the script is executing.
> >
> > In the future different pid_ns files may belong to different devices, according to the
> > discussion between Eric Biederman and Yonghong in 2017 Linux plumbers conference.
> > To address that situation the helper requires inum and dev_t from /proc/self/ns/pid.
> > This helper has the same use cases as bpf_get_current_pid_tgid() as it can be
> > used to do pid filtering even inside a container.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> >
> > Carlos Neira (4):
> > fs/nsfs.c: added ns_match
> > bpf: added new helper bpf_get_ns_current_pid_tgid
> > tools: Added bpf_get_ns_current_pid_tgid helper
> > tools/testing/selftests/bpf: Add self-tests for new helper. self tests
> > added for new helper
>
> bpf-next is currently closed due to merge window. Please resubmit once back open, thanks.
Thanks, Daniel, I'll do so.
Bests.
^ permalink raw reply
* Re: Linux 5.4 - bpf test build fails
From: Shuah Khan @ 2019-09-24 18:23 UTC (permalink / raw)
To: Tim.Bird, cristian.marussi, alexei.starovoitov, daniel
Cc: linux-kselftest, bpf, netdev, davem, skh >> Shuah Khan
In-Reply-To: <ECADFF3FD767C149AD96A924E7EA6EAF977BCBF5@USCULXMSG01.am.sony.com>
On 9/24/19 12:07 PM, Tim.Bird@sony.com wrote:
>
>
>> -----Original Message-----
>> From: Cristian Marussi on Tuesday, September 24, 2019 7:30 AM
>>
>> Hi Shuah
>>
>> On 24/09/2019 17:39, Shuah Khan wrote:
>>> On 9/24/19 10:03 AM, Shuah Khan wrote:
>>>> On 9/24/19 9:52 AM, Cristian Marussi wrote:
>>>>> Hi Shuah
>>>>>
>>>>> On 24/09/2019 16:26, Shuah Khan wrote:
>>>>>> Hi Alexei and Daniel,
>>>>>>
>>>>>> bpf test doesn't build on Linux 5.4 mainline. Do you know what's
>>>>>> happening here.
>>>>>>
>>>>>>
>>>>>> make -C tools/testing/selftests/bpf/
>>>>>
>>>>> side question, since I'm writing arm64/ tests.
>>>>>
>>>>> my "build-testcases" following the KSFT docs are:
>>>>>
>>>>> make kselftest
>>>>> make TARGETS=arm64 kselftest
>>>>> make -C tools/testing/selftests/
>>>>> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
>>>>> make TARGETS=arm64 -C tools/testing/selftests/
>>>>> make TARGETS=arm64 -C tools/testing/selftests/
>>>>> INSTALL_PATH=<install-path> install
>>>>> ./kselftest_install.sh <install-path>
>>>
>>> Cristian,
>>>
>>> That being said, I definitely want to see this list limited to
>>> a few options.
>>>
>>> One problem is that if somebody wants to do just a build, there
>>> is no option from the main makefile. I have sent support for that
>>> a few months ago and the patch didn't got lost it appears. I am
>>> working on resending those patches. The same is true for install.
>>> I sent in a patch for that a while back and I am going to resend.
>>> These will make it easier for users.
>>>
>>> I would really want to get to supporting only these options:
>>>
>>> These are supported now:
>>>
>>> make kselftest
>>> make TARGETS=arm64 kselftest (one or more targets)
>>>
>>> Replace the following:
>>>
>>> make -C tools/testing/selftests/ with
>>>
>>> make kselftes_build option from main makefile
>>>
>>> Replace this:
>>> make -C tools/testing/selftests/ INSTALL_PATH=<install-path> install
>>>
>>> with
>>> make kselftest_install
>>
>> Yes these top level options would be absolutely useful to avoid multiplication
>> of build targets to support and test.
>>
>> Moreover, currently, since there was a lot of test growing into arm64/
>> inside subdirs like arm64/signal, I support (still under review in fact) in the
>> arm64/
>> toplevel makefile the possibility of building/installing by subdirs only, in order
>> to be able to limit what you want to build/install of a TARGET (resulting in
>> quicker devel),
>> issuing something like:
>>
>> make TARGETS=arm64 SUBTARGETS=signal -C tools/testing/selftests/
>>
>> if possible, that would be useful if kept functional even in the
>> new schema. I mean being able to still issue:
>>
>> make TARGETS=arm64 SUBTARGETS=signal kselftes_build
>
> From a user perspective, instead of adding a new SUBTARGETS variable,
> I would prefer something like the following:
>
> make TARGET=arm64/signal kselftest_build
>
> If you just add a single flat subsidiary namespace, then it doesn't support further
> increasing the directory depth in the future.
>
TARGETS is make variable. Adding sub-targets might not be easy without
cluttering the selftests main Makefile. I will have to look into it.
thanks,
-- Shuah
^ permalink raw reply
* [PATCH 1/7] can: rx-offload: continue on error
From: Jeroen Hofstee @ 2019-09-24 18:45 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
While can_rx_offload_offload_one will call mailbox_read to discard
the mailbox in case of an error, can_rx_offload_irq_offload_timestamp
bails out in the error case. Since it is typically called from a while
loop, all message will eventually be discarded. So lets continue on
error instead to discard them directly.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/rx-offload.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/rx-offload.c b/drivers/net/can/rx-offload.c
index e6a668ee7730..39df41280e2d 100644
--- a/drivers/net/can/rx-offload.c
+++ b/drivers/net/can/rx-offload.c
@@ -158,7 +158,7 @@ int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload, u64 pen
skb = can_rx_offload_offload_one(offload, i);
if (!skb)
- break;
+ continue;
__skb_queue_add_sort(&skb_queue, skb, can_rx_offload_compare);
}
--
2.17.1
^ permalink raw reply related
* [PATCH 2/7] can: ti_hecc: release the mailbox a bit earlier
From: Jeroen Hofstee @ 2019-09-24 18:45 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
Release the mailbox after reading it, so it can be reused a bit earlier.
Since "can: rx-offload: continue on error" all pending message bits are
cleared directly, so remove clearing them in ti_hecc.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index f8b19eef5d26..461c28ab6d66 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -526,8 +526,9 @@ static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
u32 *timestamp, unsigned int mbxno)
{
struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
- u32 data;
+ u32 data, mbx_mask;
+ mbx_mask = BIT(mbxno);
data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
if (data & HECC_CANMID_IDE)
cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
@@ -547,6 +548,7 @@ static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
}
*timestamp = hecc_read_stamp(priv, mbxno);
+ hecc_write(priv, HECC_CANRMP, mbx_mask);
return 1;
}
@@ -695,7 +697,6 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
can_rx_offload_irq_offload_timestamp(&priv->offload,
rx_pending);
- hecc_write(priv, HECC_CANRMP, rx_pending);
}
}
--
2.17.1
^ permalink raw reply related
* [PATCH 4/7] can: ti_hecc: keep MIM and MD set
From: Jeroen Hofstee @ 2019-09-24 18:45 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
The HECC_CANMIM is set in the xmit path and cleared in the interrupt.
Since this is done with a read, modify, write action the register might
end up with some more MIM enabled then intended, since it is not
protected. That doesn't matter at all, since the tx interrupt disables
the mailbox with HECC_CANME (while holding a spinlock). So lets just
always keep MIM set.
While at it, since the mailbox direction never changes, don't set it
every time a message is send, ti_hecc_reset already sets them to tx.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index b82c011ddbec..35c82289f2a3 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -382,6 +382,9 @@ static void ti_hecc_start(struct net_device *ndev)
hecc_set_bit(priv, HECC_CANMIM, mbx_mask);
}
+ /* Enable tx interrupts */
+ hecc_set_bit(priv, HECC_CANMIM, BIT(HECC_MAX_TX_MBOX) - 1);
+
/* Prevent message over-write & Enable interrupts */
hecc_write(priv, HECC_CANOPC, HECC_SET_REG);
if (priv->use_hecc1int) {
@@ -511,8 +514,6 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
hecc_set_bit(priv, HECC_CANME, mbx_mask);
spin_unlock_irqrestore(&priv->mbx_lock, flags);
- hecc_clear_bit(priv, HECC_CANMD, mbx_mask);
- hecc_set_bit(priv, HECC_CANMIM, mbx_mask);
hecc_write(priv, HECC_CANTRS, mbx_mask);
return NETDEV_TX_OK;
@@ -675,7 +676,6 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
mbx_mask = BIT(mbxno);
if (!(mbx_mask & hecc_read(priv, HECC_CANTA)))
break;
- hecc_clear_bit(priv, HECC_CANMIM, mbx_mask);
hecc_write(priv, HECC_CANTA, mbx_mask);
spin_lock_irqsave(&priv->mbx_lock, flags);
hecc_clear_bit(priv, HECC_CANME, mbx_mask);
--
2.17.1
^ permalink raw reply related
* [PATCH 6/7] can: ti_hecc: properly report state changes
From: Jeroen Hofstee @ 2019-09-24 18:46 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
The HECC_CANES register handles the flags specially, it only updates
the flags after a one is written to them. Since the interrupt for
frame errors is not enabled an old error can hence been seen when a
state interrupt arrives. For example if the device is not connected
to the CAN-bus the error warning interrupt will have HECC_CANES
indicating there is no ack. The error passive interrupt thereafter
will have HECC_CANES flagging that there is a warning level. And if
thereafter there is a message successfully send HECC_CANES points to
an error passive event, while in reality it became error warning
again. In summary, the state is not always reported correctly.
So handle the state changes and frame errors separately. The state
changes are now based on the interrupt flags and handled directly
when they occur. The reporting of the frame errors is still done as
before, as a side effect of another interrupt.
note: the hecc_clear_bit will do a read, modify, write. So it will
not only clear the bit, but also reset all other bits being set as
a side affect, hence it is replaced with only clearing the flags.
note: The HECC_CANMC_CCR is no longer cleared in the state change
interrupt, it is completely unrelated.
And use net_ratelimit to make checkpatch happy.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 156 ++++++++++++++++++++------------------
1 file changed, 82 insertions(+), 74 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 4206ad5cb666..6098725bfdea 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -149,6 +149,8 @@ MODULE_VERSION(HECC_MODULE_VERSION);
#define HECC_BUS_ERROR (HECC_CANES_FE | HECC_CANES_BE |\
HECC_CANES_CRCE | HECC_CANES_SE |\
HECC_CANES_ACKE)
+#define HECC_CANES_FLAGS (HECC_BUS_ERROR | HECC_CANES_BO |\
+ HECC_CANES_EP | HECC_CANES_EW)
#define HECC_CANMCF_RTR BIT(4) /* Remote transmit request */
@@ -578,91 +580,63 @@ static int ti_hecc_error(struct net_device *ndev, int int_status,
struct sk_buff *skb;
u32 timestamp;
- /* propagate the error condition to the can stack */
- skb = alloc_can_err_skb(ndev, &cf);
- if (!skb) {
- if (printk_ratelimit())
- netdev_err(priv->ndev,
- "%s: alloc_can_err_skb() failed\n",
- __func__);
- return -ENOMEM;
- }
-
- if (int_status & HECC_CANGIF_WLIF) { /* warning level int */
- if ((int_status & HECC_CANGIF_BOIF) == 0) {
- priv->can.state = CAN_STATE_ERROR_WARNING;
- ++priv->can.can_stats.error_warning;
- cf->can_id |= CAN_ERR_CRTL;
- if (hecc_read(priv, HECC_CANTEC) > 96)
- cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
- if (hecc_read(priv, HECC_CANREC) > 96)
- cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
- }
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_EW);
- netdev_dbg(priv->ndev, "Error Warning interrupt\n");
- hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
- }
-
- if (int_status & HECC_CANGIF_EPIF) { /* error passive int */
- if ((int_status & HECC_CANGIF_BOIF) == 0) {
- priv->can.state = CAN_STATE_ERROR_PASSIVE;
- ++priv->can.can_stats.error_passive;
- cf->can_id |= CAN_ERR_CRTL;
- if (hecc_read(priv, HECC_CANTEC) > 127)
- cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
- if (hecc_read(priv, HECC_CANREC) > 127)
- cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
+ if (err_status & HECC_BUS_ERROR) {
+ /* propagate the error condition to the can stack */
+ skb = alloc_can_err_skb(ndev, &cf);
+ if (!skb) {
+ if (net_ratelimit())
+ netdev_err(priv->ndev,
+ "%s: alloc_can_err_skb() failed\n",
+ __func__);
+ return -ENOMEM;
}
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_EP);
- netdev_dbg(priv->ndev, "Error passive interrupt\n");
- hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
- }
-
- /* Need to check busoff condition in error status register too to
- * ensure warning interrupts don't hog the system
- */
- if ((int_status & HECC_CANGIF_BOIF) || (err_status & HECC_CANES_BO)) {
- priv->can.state = CAN_STATE_BUS_OFF;
- cf->can_id |= CAN_ERR_BUSOFF;
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_BO);
- hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
- /* Disable all interrupts in bus-off to avoid int hog */
- hecc_write(priv, HECC_CANGIM, 0);
- ++priv->can.can_stats.bus_off;
- can_bus_off(ndev);
- }
- if (err_status & HECC_BUS_ERROR) {
++priv->can.can_stats.bus_error;
cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
- if (err_status & HECC_CANES_FE) {
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_FE);
+ if (err_status & HECC_CANES_FE)
cf->data[2] |= CAN_ERR_PROT_FORM;
- }
- if (err_status & HECC_CANES_BE) {
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_BE);
+ if (err_status & HECC_CANES_BE)
cf->data[2] |= CAN_ERR_PROT_BIT;
- }
- if (err_status & HECC_CANES_SE) {
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_SE);
+ if (err_status & HECC_CANES_SE)
cf->data[2] |= CAN_ERR_PROT_STUFF;
- }
- if (err_status & HECC_CANES_CRCE) {
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_CRCE);
+ if (err_status & HECC_CANES_CRCE)
cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
- }
- if (err_status & HECC_CANES_ACKE) {
- hecc_set_bit(priv, HECC_CANES, HECC_CANES_ACKE);
+ if (err_status & HECC_CANES_ACKE)
cf->data[3] = CAN_ERR_PROT_LOC_ACK;
- }
+
+ timestamp = hecc_read(priv, HECC_CANLNT);
+ can_rx_offload_queue_sorted(&priv->offload, skb, timestamp);
}
- timestamp = hecc_read(priv, HECC_CANLNT);
- can_rx_offload_queue_sorted(&priv->offload, skb, timestamp);
+ hecc_write(priv, HECC_CANES, HECC_CANES_FLAGS);
return 0;
}
+static void change_state(struct ti_hecc_priv *priv, enum can_state rx_state,
+ enum can_state tx_state)
+{
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ u32 timestamp;
+
+ skb = alloc_can_err_skb(priv->ndev, &cf);
+ if (unlikely(!skb)) {
+ priv->can.state = max(tx_state, rx_state);
+ return;
+ }
+
+ can_change_state(priv->ndev, cf, tx_state, rx_state);
+
+ if (max(tx_state, rx_state) != CAN_STATE_BUS_OFF) {
+ cf->data[6] = hecc_read(priv, HECC_CANTEC);
+ cf->data[7] = hecc_read(priv, HECC_CANREC);
+ }
+
+ timestamp = hecc_read(priv, HECC_CANLNT);
+ can_rx_offload_queue_sorted(&priv->offload, skb, timestamp);
+}
+
static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
{
struct net_device *ndev = (struct net_device *)dev_id;
@@ -670,6 +644,7 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
struct net_device_stats *stats = &ndev->stats;
u32 mbxno, mbx_mask, int_status, err_status, stamp;
unsigned long flags, rx_pending;
+ u32 handled = 0;
int_status = hecc_read(priv,
priv->use_hecc1int ?
@@ -679,10 +654,43 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
return IRQ_NONE;
err_status = hecc_read(priv, HECC_CANES);
- if (err_status & (HECC_BUS_ERROR | HECC_CANES_BO |
- HECC_CANES_EP | HECC_CANES_EW))
+ if (unlikely(err_status & HECC_CANES_FLAGS))
ti_hecc_error(ndev, int_status, err_status);
+ if (unlikely(int_status & HECC_CANGIM_DEF_MASK)) {
+ enum can_state rx_state, tx_state;
+ u32 rec = hecc_read(priv, HECC_CANREC);
+ u32 tec = hecc_read(priv, HECC_CANTEC);
+
+ if (int_status & HECC_CANGIF_WLIF) {
+ handled |= HECC_CANGIF_WLIF;
+ rx_state = rec >= tec ? CAN_STATE_ERROR_WARNING : 0;
+ tx_state = rec <= tec ? CAN_STATE_ERROR_WARNING : 0;
+ netdev_dbg(priv->ndev, "Error Warning interrupt\n");
+ change_state(priv, rx_state, tx_state);
+ }
+
+ if (int_status & HECC_CANGIF_EPIF) {
+ handled |= HECC_CANGIF_EPIF;
+ rx_state = rec >= tec ? CAN_STATE_ERROR_PASSIVE : 0;
+ tx_state = rec <= tec ? CAN_STATE_ERROR_PASSIVE : 0;
+ netdev_dbg(priv->ndev, "Error passive interrupt\n");
+ change_state(priv, rx_state, tx_state);
+ }
+
+ if (int_status & HECC_CANGIF_BOIF) {
+ handled |= HECC_CANGIF_BOIF;
+ rx_state = CAN_STATE_BUS_OFF;
+ tx_state = CAN_STATE_BUS_OFF;
+ netdev_dbg(priv->ndev, "Bus off interrupt\n");
+
+ /* Disable all interrupts */
+ hecc_write(priv, HECC_CANGIM, 0);
+ can_bus_off(ndev);
+ change_state(priv, rx_state, tx_state);
+ }
+ }
+
if (int_status & HECC_CANGIF_GMIF) {
while (priv->tx_tail - priv->tx_head > 0) {
mbxno = get_tx_tail_mb(priv);
@@ -718,10 +726,10 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
/* clear all interrupt conditions - read back to avoid spurious ints */
if (priv->use_hecc1int) {
- hecc_write(priv, HECC_CANGIF1, HECC_SET_REG);
+ hecc_write(priv, HECC_CANGIF1, handled);
int_status = hecc_read(priv, HECC_CANGIF1);
} else {
- hecc_write(priv, HECC_CANGIF0, HECC_SET_REG);
+ hecc_write(priv, HECC_CANGIF0, handled);
int_status = hecc_read(priv, HECC_CANGIF0);
}
--
2.17.1
^ permalink raw reply related
* [PATCH 7/7] can: ti_hecc: add missing state changes
From: Jeroen Hofstee @ 2019-09-24 18:46 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
While the ti_hecc has interrupts to report when the error counters increase
to a certain level and which change state it doesn't handle the case that
the error counters go down again, so the reported state can actually be
wrong. Since there is no interrupt for that, do update state based on the
error counters, when the state is not error active and goes down again.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 6098725bfdea..c7c866da9c6a 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -689,6 +689,23 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
can_bus_off(ndev);
change_state(priv, rx_state, tx_state);
}
+ } else if (unlikely(priv->can.state != CAN_STATE_ERROR_ACTIVE)) {
+ enum can_state new_state, tx_state, rx_state;
+ u32 rec = hecc_read(priv, HECC_CANREC);
+ u32 tec = hecc_read(priv, HECC_CANTEC);
+
+ if (rec >= 128 || tec >= 128)
+ new_state = CAN_STATE_ERROR_PASSIVE;
+ else if (rec >= 96 || tec >= 96)
+ new_state = CAN_STATE_ERROR_WARNING;
+ else
+ new_state = CAN_STATE_ERROR_ACTIVE;
+
+ if (new_state < priv->can.state) {
+ rx_state = rec >= tec ? new_state : 0;
+ tx_state = rec <= tec ? new_state : 0;
+ change_state(priv, rx_state, tx_state);
+ }
}
if (int_status & HECC_CANGIF_GMIF) {
--
2.17.1
^ permalink raw reply related
* [PATCH 5/7] can: ti_hecc: add fifo underflow error reporting
From: Jeroen Hofstee @ 2019-09-24 18:46 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
When the rx fifo overflows the ti_hecc would silently drop them since
the overwrite protection is enabled for all mailboxes. So disable it
for the lowest priority mailbox and increment the rx_fifo_errors when
receive message lost is set. Drop the message itself in that case,
since it might be partially updated.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 35c82289f2a3..4206ad5cb666 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -82,7 +82,7 @@ MODULE_VERSION(HECC_MODULE_VERSION);
#define HECC_CANTA 0x10 /* Transmission acknowledge */
#define HECC_CANAA 0x14 /* Abort acknowledge */
#define HECC_CANRMP 0x18 /* Receive message pending */
-#define HECC_CANRML 0x1C /* Remote message lost */
+#define HECC_CANRML 0x1C /* Receive message lost */
#define HECC_CANRFP 0x20 /* Remote frame pending */
#define HECC_CANGAM 0x24 /* SECC only:Global acceptance mask */
#define HECC_CANMC 0x28 /* Master control */
@@ -385,8 +385,17 @@ static void ti_hecc_start(struct net_device *ndev)
/* Enable tx interrupts */
hecc_set_bit(priv, HECC_CANMIM, BIT(HECC_MAX_TX_MBOX) - 1);
- /* Prevent message over-write & Enable interrupts */
- hecc_write(priv, HECC_CANOPC, HECC_SET_REG);
+ /* Prevent message over-write to create a rx fifo, but not for the
+ * lowest priority mailbox, since that allows detecting overflows
+ * instead of the hardware silently dropping the messages. The lowest
+ * rx mailbox is one above the tx ones, hence its mbxno is the number
+ * of tx mailboxes.
+ */
+ mbxno = HECC_MAX_TX_MBOX;
+ mbx_mask = ~BIT(mbxno);
+ hecc_write(priv, HECC_CANOPC, mbx_mask);
+
+ /* Enable interrupts */
if (priv->use_hecc1int) {
hecc_write(priv, HECC_CANMIL, HECC_SET_REG);
hecc_write(priv, HECC_CANGIM, HECC_CANGIM_DEF_MASK |
@@ -531,6 +540,7 @@ static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
{
struct ti_hecc_priv *priv = rx_offload_to_priv(offload);
u32 data, mbx_mask;
+ int lost;
mbx_mask = BIT(mbxno);
data = hecc_read_mbx(priv, mbxno, HECC_CANMID);
@@ -552,9 +562,12 @@ static unsigned int ti_hecc_mailbox_read(struct can_rx_offload *offload,
}
*timestamp = hecc_read_stamp(priv, mbxno);
+ lost = hecc_read(priv, HECC_CANRML) & mbx_mask;
+ if (unlikely(lost))
+ priv->offload.dev->stats.rx_fifo_errors++;
hecc_write(priv, HECC_CANRMP, mbx_mask);
- return 1;
+ return !lost;
}
static int ti_hecc_error(struct net_device *ndev, int int_status,
--
2.17.1
^ permalink raw reply related
* [PATCH 3/7] can: ti_hecc: stop the CPK on down
From: Jeroen Hofstee @ 2019-09-24 18:45 UTC (permalink / raw)
To: linux-can@vger.kernel.org
Cc: Jeroen Hofstee, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, open list:NETWORKING DRIVERS, open list
In-Reply-To: <20190924184437.10607-1-jhofstee@victronenergy.com>
When the interface goes down, the CPK should no longer take an active
part in the CAN-bus communication, like sending acks and error frames.
So enable configuration mode in ti_hecc_stop, so the CPK is no longer
active.
When a transceiver switch is present the acks and errors don't make it
to the bus, but disabling the CPK then does prevent oddities, like
ti_hecc_reset failing, since the CPK can become bus-off and starts
counting the 11 bit recessive bits, which seems to block the reset. It
can also cause invalid interrupts and disrupt the CAN-bus, since
transmission can be stopped in the middle of a message, by disabling
the tranceiver while the CPK is sending.
Since the CPK is disabled after normal power on, it is typically only
seen when the interface is restarted.
Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
drivers/net/can/ti_hecc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 461c28ab6d66..b82c011ddbec 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -400,6 +400,9 @@ static void ti_hecc_stop(struct net_device *ndev)
{
struct ti_hecc_priv *priv = netdev_priv(ndev);
+ /* Disable the CPK; stop sending, erroring and acking */
+ hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
+
/* Disable interrupts and disable mailboxes */
hecc_write(priv, HECC_CANGIM, 0);
hecc_write(priv, HECC_CANMIM, 0);
--
2.17.1
^ permalink raw reply related
* Re: Linux 5.4 - bpf test build fails
From: Daniel Borkmann @ 2019-09-24 18:49 UTC (permalink / raw)
To: Shuah Khan
Cc: Yonghong Song, Alexei Starovoitov,
open list:KERNEL SELFTEST FRAMEWORK, bpf, Networking,
David S. Miller, Andrii Nakryiko
In-Reply-To: <05b7830c-1fa8-b613-0535-1f5f5a40a25a@linuxfoundation.org>
On Tue, Sep 24, 2019 at 09:48:35AM -0600, Shuah Khan wrote:
> On 9/24/19 9:43 AM, Yonghong Song wrote:
> > On 9/24/19 8:26 AM, Shuah Khan wrote:
> > > Hi Alexei and Daniel,
> > >
> > > bpf test doesn't build on Linux 5.4 mainline. Do you know what's
> > > happening here.
> > >
> > > make -C tools/testing/selftests/bpf/
> > >
> > > -c progs/test_core_reloc_ptr_as_arr.c -o - || echo "clang failed") | \
> > > llc -march=bpf -mcpu=generic -filetype=obj -o
> > > /mnt/data/lkml/linux_5.4/tools/testing/selftests/bpf/test_core_reloc_ptr_as_arr.o
> > >
> > > progs/test_core_reloc_ptr_as_arr.c:25:6: error: use of unknown builtin
> > > '__builtin_preserve_access_index' [-Wimplicit-function-declaration]
> > > if (BPF_CORE_READ(&out->a, &in[2].a))
> > > ^
> > > ./bpf_helpers.h:533:10: note: expanded from macro 'BPF_CORE_READ'
> > > __builtin_preserve_access_index(src))
> > > ^
> > > progs/test_core_reloc_ptr_as_arr.c:25:6: warning: incompatible integer to
> > > pointer conversion passing 'int' to parameter of type 'const void *'
> > > [-Wint-conversion]
> > > if (BPF_CORE_READ(&out->a, &in[2].a))
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./bpf_helpers.h:533:10: note: expanded from macro 'BPF_CORE_READ'
> > > __builtin_preserve_access_index(src))
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 1 warning and 1 error generated.
> > > llc: error: llc: <stdin>:1:1: error: expected top-level entity
> > > clang failed
> > >
> > > Also
> > >
> > > make TARGETS=bpf kselftest fails as well. Dependency between
> > > tools/lib/bpf and the test. How can we avoid this type of
> > > dependency or resolve it in a way it doesn't result in build
> > > failures?
> >
> > Thanks, Shuah.
> >
> > The clang __builtin_preserve_access_index() intrinsic is
> > introduced in LLVM9 (which just released last week) and
> > the builtin and other CO-RE features are only supported
> > in LLVM10 (current development branch) with more bug fixes
> > and added features.
> >
> > I think we should do a feature test for llvm version and only
> > enable these tests when llvm version >= 10.
>
> Yes. If new tests depend on a particular llvm revision, the failing
> the build is a regression. I would like to see older tests that don't
> have dependency build and run.
So far we haven't made it a requirement as majority of BPF contributors
that would run/add tests in here are also on bleeding edge LLVM anyway
and other CIs like 0-day bot have simply upgraded their LLVM version
from git whenever there was a failure similar to the one here so its
ensured that really /all/ test cases are running and nothing would be
skipped. There is worry to some degree that CIs just keep sticking to
an old compiler since tests "just" pass and regressions wouldn't be
caught on new releases for those that are skipped.
That said, for the C based tests, it should actually be straight forward
to categorize them based on built-in macros like ...
$ echo | clang -dM -E -
[...]
#define __clang_major__ 10
#define __clang_minor__ 0
[...]
... given there is now also bpf-gcc, the test matrix gets bigger anyway,
so it might be worth rethinking to run the suite multiple times with
different major llvm{,gcc} versions at some point to make sure their
generated BPF bytecode keeps passing the verifier, and yell loudly if
newer features had to be skipped due to lack of recent compiler version.
This would be a super set of /just/ skipping tests and improve coverage
at the same time.
Thanks,
Daniel
^ permalink raw reply
* Re: Linux 5.4 - bpf test build fails
From: Shuah Khan @ 2019-09-24 18:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Yonghong Song, Alexei Starovoitov,
open list:KERNEL SELFTEST FRAMEWORK, bpf, Networking,
David S. Miller, Andrii Nakryiko, Shuah Khan
In-Reply-To: <20190924184946.GB5889@pc-63.home>
On 9/24/19 12:49 PM, Daniel Borkmann wrote:
> On Tue, Sep 24, 2019 at 09:48:35AM -0600, Shuah Khan wrote:
>> On 9/24/19 9:43 AM, Yonghong Song wrote:
>>> On 9/24/19 8:26 AM, Shuah Khan wrote:
>>>> Hi Alexei and Daniel,
>>>>
>>>> bpf test doesn't build on Linux 5.4 mainline. Do you know what's
>>>> happening here.
>>>>
>>>> make -C tools/testing/selftests/bpf/
>>>>
>>>> -c progs/test_core_reloc_ptr_as_arr.c -o - || echo "clang failed") | \
>>>> llc -march=bpf -mcpu=generic -filetype=obj -o
>>>> /mnt/data/lkml/linux_5.4/tools/testing/selftests/bpf/test_core_reloc_ptr_as_arr.o
>>>>
>>>> progs/test_core_reloc_ptr_as_arr.c:25:6: error: use of unknown builtin
>>>> '__builtin_preserve_access_index' [-Wimplicit-function-declaration]
>>>> if (BPF_CORE_READ(&out->a, &in[2].a))
>>>> ^
>>>> ./bpf_helpers.h:533:10: note: expanded from macro 'BPF_CORE_READ'
>>>> __builtin_preserve_access_index(src))
>>>> ^
>>>> progs/test_core_reloc_ptr_as_arr.c:25:6: warning: incompatible integer to
>>>> pointer conversion passing 'int' to parameter of type 'const void *'
>>>> [-Wint-conversion]
>>>> if (BPF_CORE_READ(&out->a, &in[2].a))
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> ./bpf_helpers.h:533:10: note: expanded from macro 'BPF_CORE_READ'
>>>> __builtin_preserve_access_index(src))
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> 1 warning and 1 error generated.
>>>> llc: error: llc: <stdin>:1:1: error: expected top-level entity
>>>> clang failed
>>>>
>>>> Also
>>>>
>>>> make TARGETS=bpf kselftest fails as well. Dependency between
>>>> tools/lib/bpf and the test. How can we avoid this type of
>>>> dependency or resolve it in a way it doesn't result in build
>>>> failures?
>>>
>>> Thanks, Shuah.
>>>
>>> The clang __builtin_preserve_access_index() intrinsic is
>>> introduced in LLVM9 (which just released last week) and
>>> the builtin and other CO-RE features are only supported
>>> in LLVM10 (current development branch) with more bug fixes
>>> and added features.
>>>
>>> I think we should do a feature test for llvm version and only
>>> enable these tests when llvm version >= 10.
>>
>> Yes. If new tests depend on a particular llvm revision, the failing
>> the build is a regression. I would like to see older tests that don't
>> have dependency build and run.
>
> So far we haven't made it a requirement as majority of BPF contributors
> that would run/add tests in here are also on bleeding edge LLVM anyway
> and other CIs like 0-day bot have simply upgraded their LLVM version
> from git whenever there was a failure similar to the one here so its
> ensured that really /all/ test cases are running and nothing would be
> skipped. There is worry to some degree that CIs just keep sticking to
> an old compiler since tests "just" pass and regressions wouldn't be
> caught on new releases for those that are skipped. >
Sure. Bleeding edge is developer mode. We still have to be concerned
about users that might not upgrade quickly.
> That said, for the C based tests, it should actually be straight forward
> to categorize them based on built-in macros like ...
>
> $ echo | clang -dM -E -
> [...]
> #define __clang_major__ 10
> #define __clang_minor__ 0
> [...]
>
What would nice running the tests you can run and then say some tests
aren't going to run. Is this something you can support?
> ... given there is now also bpf-gcc, the test matrix gets bigger anyway,
> so it might be worth rethinking to run the suite multiple times with
> different major llvm{,gcc} versions at some point to make sure their
> generated BPF bytecode keeps passing the verifier, and yell loudly if
> newer features had to be skipped due to lack of recent compiler version.
> This would be a super set of /just/ skipping tests and improve coverage
> at the same time.
>
Probably. Reality is most users will just quit and add bpf to "hard to
run category" of tests.
thanks,
-- Shuah
^ 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