* [PATCH v3 net-next 7/8] bpf: Add BPF_OBJ_GET_INFO_BY_FD
From: Martin KaFai Lau @ 2017-06-05 19:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <cover.1496686606.git.kafai@fb.com>
A single BPF_OBJ_GET_INFO_BY_FD cmd is used to obtain the info
for both bpf_prog and bpf_map. The kernel can figure out the
fd is associated with a bpf_prog or bpf_map.
The suggested struct bpf_prog_info and struct bpf_map_info are
not meant to be a complete list and it is not the goal of this patch.
New fields can be added in the future patch.
The focus of this patch is to create the interface,
BPF_OBJ_GET_INFO_BY_FD cmd for exposing the bpf_prog's and
bpf_map's info.
The obj's info, which will be extended (and get bigger) over time, is
separated from the bpf_attr to avoid bloating the bpf_attr.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Alexei Starovoitov <ast@fb.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/linux/filter.h | 2 -
include/uapi/linux/bpf.h | 28 ++++++++
kernel/bpf/syscall.c | 163 ++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 174 insertions(+), 19 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 1e2dddf21f3b..1fa26dc562ce 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -69,8 +69,6 @@ struct bpf_prog_aux;
/* BPF program can access up to 512 bytes of stack space. */
#define MAX_BPF_STACK 512
-#define BPF_TAG_SIZE 8
-
/* Helper macros for filter block array initializers. */
/* ALU ops on registers, bpf_add|sub|...: dst_reg += src_reg */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dd23f47ff00c..9b2c10b45733 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -86,6 +86,7 @@ enum bpf_cmd {
BPF_MAP_GET_NEXT_ID,
BPF_PROG_GET_FD_BY_ID,
BPF_MAP_GET_FD_BY_ID,
+ BPF_OBJ_GET_INFO_BY_FD,
};
enum bpf_map_type {
@@ -222,6 +223,12 @@ union bpf_attr {
};
__u32 next_id;
};
+
+ struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
+ __u32 bpf_fd;
+ __u32 info_len;
+ __aligned_u64 info;
+ } info;
} __attribute__((aligned(8)));
/* BPF helper function descriptions:
@@ -686,4 +693,25 @@ struct xdp_md {
__u32 data_end;
};
+#define BPF_TAG_SIZE 8
+
+struct bpf_prog_info {
+ __u32 type;
+ __u32 id;
+ __u8 tag[BPF_TAG_SIZE];
+ __u32 jited_prog_len;
+ __u32 xlated_prog_len;
+ __aligned_u64 jited_prog_insns;
+ __aligned_u64 xlated_prog_insns;
+} __attribute__((aligned(8)));
+
+struct bpf_map_info {
+ __u32 type;
+ __u32 id;
+ __u32 key_size;
+ __u32 value_size;
+ __u32 max_entries;
+ __u32 map_flags;
+} __attribute__((aligned(8)));
+
#endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 1802bb9c47d9..8942c820d620 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1239,6 +1239,145 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
+static int check_uarg_tail_zero(void __user *uaddr,
+ size_t expected_size,
+ size_t actual_size)
+{
+ unsigned char __user *addr;
+ unsigned char __user *end;
+ unsigned char val;
+ int err;
+
+ if (actual_size <= expected_size)
+ return 0;
+
+ addr = uaddr + expected_size;
+ end = uaddr + actual_size;
+
+ for (; addr < end; addr++) {
+ err = get_user(val, addr);
+ if (err)
+ return err;
+ if (val)
+ return -E2BIG;
+ }
+
+ return 0;
+}
+
+static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
+ const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
+ struct bpf_prog_info info = {};
+ u32 info_len = attr->info.info_len;
+ char __user *uinsns;
+ u32 ulen;
+ int err;
+
+ err = check_uarg_tail_zero(uinfo, sizeof(info), info_len);
+ if (err)
+ return err;
+ info_len = min_t(u32, sizeof(info), info_len);
+
+ if (copy_from_user(&info, uinfo, info_len))
+ return err;
+
+ info.type = prog->type;
+ info.id = prog->aux->id;
+
+ memcpy(info.tag, prog->tag, sizeof(prog->tag));
+
+ if (!capable(CAP_SYS_ADMIN)) {
+ info.jited_prog_len = 0;
+ info.xlated_prog_len = 0;
+ goto done;
+ }
+
+ ulen = info.jited_prog_len;
+ info.jited_prog_len = prog->jited_len;
+ if (info.jited_prog_len && ulen) {
+ uinsns = u64_to_user_ptr(info.jited_prog_insns);
+ ulen = min_t(u32, info.jited_prog_len, ulen);
+ if (copy_to_user(uinsns, prog->bpf_func, ulen))
+ return -EFAULT;
+ }
+
+ ulen = info.xlated_prog_len;
+ info.xlated_prog_len = bpf_prog_size(prog->len);
+ if (info.xlated_prog_len && ulen) {
+ uinsns = u64_to_user_ptr(info.xlated_prog_insns);
+ ulen = min_t(u32, info.xlated_prog_len, ulen);
+ if (copy_to_user(uinsns, prog->insnsi, ulen))
+ return -EFAULT;
+ }
+
+done:
+ if (copy_to_user(uinfo, &info, info_len) ||
+ put_user(info_len, &uattr->info.info_len))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int bpf_map_get_info_by_fd(struct bpf_map *map,
+ const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ struct bpf_map_info __user *uinfo = u64_to_user_ptr(attr->info.info);
+ struct bpf_map_info info = {};
+ u32 info_len = attr->info.info_len;
+ int err;
+
+ err = check_uarg_tail_zero(uinfo, sizeof(info), info_len);
+ if (err)
+ return err;
+ info_len = min_t(u32, sizeof(info), info_len);
+
+ info.type = map->map_type;
+ info.id = map->id;
+ info.key_size = map->key_size;
+ info.value_size = map->value_size;
+ info.max_entries = map->max_entries;
+ info.map_flags = map->map_flags;
+
+ if (copy_to_user(uinfo, &info, info_len) ||
+ put_user(info_len, &uattr->info.info_len))
+ return -EFAULT;
+
+ return 0;
+}
+
+#define BPF_OBJ_GET_INFO_BY_FD_LAST_FIELD info.info
+
+static int bpf_obj_get_info_by_fd(const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ int ufd = attr->info.bpf_fd;
+ struct fd f;
+ int err;
+
+ if (CHECK_ATTR(BPF_OBJ_GET_INFO_BY_FD))
+ return -EINVAL;
+
+ f = fdget(ufd);
+ if (!f.file)
+ return -EBADFD;
+
+ if (f.file->f_op == &bpf_prog_fops)
+ err = bpf_prog_get_info_by_fd(f.file->private_data, attr,
+ uattr);
+ else if (f.file->f_op == &bpf_map_fops)
+ err = bpf_map_get_info_by_fd(f.file->private_data, attr,
+ uattr);
+ else
+ err = -EINVAL;
+
+ fdput(f);
+ return err;
+}
+
SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
{
union bpf_attr attr = {};
@@ -1258,23 +1397,10 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
* user-space does not rely on any kernel feature
* extensions we dont know about yet.
*/
- if (size > sizeof(attr)) {
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
-
- addr = (void __user *)uattr + sizeof(attr);
- end = (void __user *)uattr + size;
-
- for (; addr < end; addr++) {
- err = get_user(val, addr);
- if (err)
- return err;
- if (val)
- return -E2BIG;
- }
- size = sizeof(attr);
- }
+ err = check_uarg_tail_zero(uattr, sizeof(attr), size);
+ if (err)
+ return err;
+ size = min_t(u32, size, sizeof(attr));
/* copy attributes from user space, may be less than sizeof(bpf_attr) */
if (copy_from_user(&attr, uattr, size) != 0)
@@ -1330,6 +1456,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
case BPF_MAP_GET_FD_BY_ID:
err = bpf_map_get_fd_by_id(&attr);
break;
+ case BPF_OBJ_GET_INFO_BY_FD:
+ err = bpf_obj_get_info_by_fd(&attr, uattr);
+ break;
default:
err = -EINVAL;
break;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 net-next 2/8] bpf: Introduce bpf_map ID
From: Martin KaFai Lau @ 2017-06-05 19:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <cover.1496686606.git.kafai@fb.com>
This patch generates an unique ID for each created bpf_map.
The approach is similar to the earlier patch for bpf_prog ID.
It is worth to note that the bpf_map's ID and bpf_prog's ID
are in two independent ID spaces and both have the same valid range:
[1, INT_MAX).
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Alexei Starovoitov <ast@fb.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 34 +++++++++++++++++++++++++++++++++-
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c5946d19f2ca..c32bace66d3d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -46,6 +46,7 @@ struct bpf_map {
u32 max_entries;
u32 map_flags;
u32 pages;
+ u32 id;
struct user_struct *user;
const struct bpf_map_ops *ops;
struct work_struct work;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2a1b32b470f1..4c3075b5d840 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -27,6 +27,8 @@
DEFINE_PER_CPU(int, bpf_prog_active);
static DEFINE_IDR(prog_idr);
static DEFINE_SPINLOCK(prog_idr_lock);
+static DEFINE_IDR(map_idr);
+static DEFINE_SPINLOCK(map_idr_lock);
int sysctl_unprivileged_bpf_disabled __read_mostly;
@@ -117,6 +119,29 @@ static void bpf_map_uncharge_memlock(struct bpf_map *map)
free_uid(user);
}
+static int bpf_map_alloc_id(struct bpf_map *map)
+{
+ int id;
+
+ spin_lock_bh(&map_idr_lock);
+ id = idr_alloc_cyclic(&map_idr, map, 1, INT_MAX, GFP_ATOMIC);
+ if (id > 0)
+ map->id = id;
+ spin_unlock_bh(&map_idr_lock);
+
+ if (WARN_ON_ONCE(!id))
+ return -ENOSPC;
+
+ return id > 0 ? 0 : id;
+}
+
+static void bpf_map_free_id(struct bpf_map *map)
+{
+ spin_lock_bh(&map_idr_lock);
+ idr_remove(&map_idr, map->id);
+ spin_unlock_bh(&map_idr_lock);
+}
+
/* called from workqueue */
static void bpf_map_free_deferred(struct work_struct *work)
{
@@ -141,6 +166,7 @@ static void bpf_map_put_uref(struct bpf_map *map)
void bpf_map_put(struct bpf_map *map)
{
if (atomic_dec_and_test(&map->refcnt)) {
+ bpf_map_free_id(map);
INIT_WORK(&map->work, bpf_map_free_deferred);
schedule_work(&map->work);
}
@@ -239,14 +265,20 @@ static int map_create(union bpf_attr *attr)
if (err)
goto free_map_nouncharge;
+ err = bpf_map_alloc_id(map);
+ if (err)
+ goto free_map;
+
err = bpf_map_new_fd(map);
if (err < 0)
/* failed to allocate fd */
- goto free_map;
+ goto free_id;
trace_bpf_map_create(map, err);
return err;
+free_id:
+ bpf_map_free_id(map);
free_map:
bpf_map_uncharge_memlock(map);
free_map_nouncharge:
--
2.9.3
^ permalink raw reply related
* [PATCH v3 net-next 4/8] bpf: Add BPF_PROG_GET_FD_BY_ID
From: Martin KaFai Lau @ 2017-06-05 19:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <cover.1496686606.git.kafai@fb.com>
Add BPF_PROG_GET_FD_BY_ID command to allow user to get a fd
from a bpf_prog's ID.
bpf_prog_inc_not_zero() is added and is called with prog_idr_lock
held.
__bpf_prog_put() is also added which has the 'bool do_idr_lock'
param to decide if the prog_idr_lock should be acquired when
freeing the prog->id.
In the error path of bpf_prog_inc_not_zero(), it may have to
call __bpf_prog_put(map, false) which does not need
to take the prog_idr_lock when freeing the prog->id.
It is currently limited to CAP_SYS_ADMIN which we can
consider to lift it in followup patches.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Alexei Starovoitov <ast@fb.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/uapi/linux/bpf.h | 8 +++--
kernel/bpf/syscall.c | 91 ++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 87 insertions(+), 12 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 629747a3f273..d70cfed19d5e 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -84,6 +84,7 @@ enum bpf_cmd {
BPF_PROG_TEST_RUN,
BPF_PROG_GET_NEXT_ID,
BPF_MAP_GET_NEXT_ID,
+ BPF_PROG_GET_FD_BY_ID,
};
enum bpf_map_type {
@@ -212,8 +213,11 @@ union bpf_attr {
__u32 duration;
} test;
- struct { /* anonymous struct used by BPF_*_GET_NEXT_ID */
- __u32 start_id;
+ struct { /* anonymous struct used by BPF_*_GET_*_ID */
+ union {
+ __u32 start_id;
+ __u32 prog_id;
+ };
__u32 next_id;
};
} __attribute__((aligned(8)));
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2405feedb8c1..dc6253bb8ebb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -703,15 +703,23 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
return id > 0 ? 0 : id;
}
-static void bpf_prog_free_id(struct bpf_prog *prog)
+static void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock)
{
/* cBPF to eBPF migrations are currently not in the idr store. */
if (!prog->aux->id)
return;
- spin_lock_bh(&prog_idr_lock);
+ if (do_idr_lock)
+ spin_lock_bh(&prog_idr_lock);
+ else
+ __acquire(&prog_idr_lock);
+
idr_remove(&prog_idr, prog->aux->id);
- spin_unlock_bh(&prog_idr_lock);
+
+ if (do_idr_lock)
+ spin_unlock_bh(&prog_idr_lock);
+ else
+ __release(&prog_idr_lock);
}
static void __bpf_prog_put_rcu(struct rcu_head *rcu)
@@ -723,16 +731,21 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
bpf_prog_free(aux->prog);
}
-void bpf_prog_put(struct bpf_prog *prog)
+static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
{
if (atomic_dec_and_test(&prog->aux->refcnt)) {
trace_bpf_prog_put_rcu(prog);
/* bpf_prog_free_id() must be called first */
- bpf_prog_free_id(prog);
+ bpf_prog_free_id(prog, do_idr_lock);
bpf_prog_kallsyms_del(prog);
call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
}
}
+
+void bpf_prog_put(struct bpf_prog *prog)
+{
+ __bpf_prog_put(prog, true);
+}
EXPORT_SYMBOL_GPL(bpf_prog_put);
static int bpf_prog_release(struct inode *inode, struct file *filp)
@@ -814,6 +827,24 @@ struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog)
}
EXPORT_SYMBOL_GPL(bpf_prog_inc);
+/* prog_idr_lock should have been held */
+static struct bpf_prog *bpf_prog_inc_not_zero(struct bpf_prog *prog)
+{
+ int refold;
+
+ refold = __atomic_add_unless(&prog->aux->refcnt, 1, 0);
+
+ if (refold >= BPF_MAX_REFCNT) {
+ __bpf_prog_put(prog, false);
+ return ERR_PTR(-EBUSY);
+ }
+
+ if (!refold)
+ return ERR_PTR(-ENOENT);
+
+ return prog;
+}
+
static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)
{
struct fd f = fdget(ufd);
@@ -928,16 +959,21 @@ static int bpf_prog_load(union bpf_attr *attr)
goto free_used_maps;
err = bpf_prog_new_fd(prog);
- if (err < 0)
- /* failed to allocate fd */
- goto free_id;
+ if (err < 0) {
+ /* failed to allocate fd.
+ * bpf_prog_put() is needed because the above
+ * bpf_prog_alloc_id() has published the prog
+ * to the userspace and the userspace may
+ * have refcnt-ed it through BPF_PROG_GET_FD_BY_ID.
+ */
+ bpf_prog_put(prog);
+ return err;
+ }
bpf_prog_kallsyms_add(prog);
trace_bpf_prog_load(prog, err);
return err;
-free_id:
- bpf_prog_free_id(prog);
free_used_maps:
free_used_maps(prog->aux);
free_prog:
@@ -1099,6 +1135,38 @@ static int bpf_obj_get_next_id(const union bpf_attr *attr,
return err;
}
+#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD prog_id
+
+static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
+{
+ struct bpf_prog *prog;
+ u32 id = attr->prog_id;
+ int fd;
+
+ if (CHECK_ATTR(BPF_PROG_GET_FD_BY_ID))
+ return -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ spin_lock_bh(&prog_idr_lock);
+ prog = idr_find(&prog_idr, id);
+ if (prog)
+ prog = bpf_prog_inc_not_zero(prog);
+ else
+ prog = ERR_PTR(-ENOENT);
+ spin_unlock_bh(&prog_idr_lock);
+
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
+
+ fd = bpf_prog_new_fd(prog);
+ if (fd < 0)
+ bpf_prog_put(prog);
+
+ return fd;
+}
+
SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
{
union bpf_attr attr = {};
@@ -1184,6 +1252,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
err = bpf_obj_get_next_id(&attr, uattr,
&map_idr, &map_idr_lock);
break;
+ case BPF_PROG_GET_FD_BY_ID:
+ err = bpf_prog_get_fd_by_id(&attr);
+ break;
default:
err = -EINVAL;
break;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 net-next 5/8] bpf: Add BPF_MAP_GET_FD_BY_ID
From: Martin KaFai Lau @ 2017-06-05 19:15 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <cover.1496686606.git.kafai@fb.com>
Add BPF_MAP_GET_FD_BY_ID command to allow user to get a fd
from a bpf_map's ID.
bpf_map_inc_not_zero() is added and is called with map_idr_lock
held.
__bpf_map_put() is also added which has the 'bool do_idr_lock'
param to decide if the map_idr_lock should be acquired when
freeing the map->id.
In the error path of bpf_map_inc_not_zero(), it may have to
call __bpf_map_put(map, false) which does not need
to take the map_idr_lock when freeing the map->id.
It is currently limited to CAP_SYS_ADMIN which we can
consider to lift it in followup patches.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Acked-by: Alexei Starovoitov <ast@fb.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/uapi/linux/bpf.h | 2 +
kernel/bpf/syscall.c | 95 +++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 87 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d70cfed19d5e..dd23f47ff00c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -85,6 +85,7 @@ enum bpf_cmd {
BPF_PROG_GET_NEXT_ID,
BPF_MAP_GET_NEXT_ID,
BPF_PROG_GET_FD_BY_ID,
+ BPF_MAP_GET_FD_BY_ID,
};
enum bpf_map_type {
@@ -217,6 +218,7 @@ union bpf_attr {
union {
__u32 start_id;
__u32 prog_id;
+ __u32 map_id;
};
__u32 next_id;
};
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index dc6253bb8ebb..1802bb9c47d9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -135,11 +135,19 @@ static int bpf_map_alloc_id(struct bpf_map *map)
return id > 0 ? 0 : id;
}
-static void bpf_map_free_id(struct bpf_map *map)
+static void bpf_map_free_id(struct bpf_map *map, bool do_idr_lock)
{
- spin_lock_bh(&map_idr_lock);
+ if (do_idr_lock)
+ spin_lock_bh(&map_idr_lock);
+ else
+ __acquire(&map_idr_lock);
+
idr_remove(&map_idr, map->id);
- spin_unlock_bh(&map_idr_lock);
+
+ if (do_idr_lock)
+ spin_unlock_bh(&map_idr_lock);
+ else
+ __release(&map_idr_lock);
}
/* called from workqueue */
@@ -163,16 +171,21 @@ static void bpf_map_put_uref(struct bpf_map *map)
/* decrement map refcnt and schedule it for freeing via workqueue
* (unrelying map implementation ops->map_free() might sleep)
*/
-void bpf_map_put(struct bpf_map *map)
+static void __bpf_map_put(struct bpf_map *map, bool do_idr_lock)
{
if (atomic_dec_and_test(&map->refcnt)) {
/* bpf_map_free_id() must be called first */
- bpf_map_free_id(map);
+ bpf_map_free_id(map, do_idr_lock);
INIT_WORK(&map->work, bpf_map_free_deferred);
schedule_work(&map->work);
}
}
+void bpf_map_put(struct bpf_map *map)
+{
+ __bpf_map_put(map, true);
+}
+
void bpf_map_put_with_uref(struct bpf_map *map)
{
bpf_map_put_uref(map);
@@ -271,15 +284,20 @@ static int map_create(union bpf_attr *attr)
goto free_map;
err = bpf_map_new_fd(map);
- if (err < 0)
- /* failed to allocate fd */
- goto free_id;
+ if (err < 0) {
+ /* failed to allocate fd.
+ * bpf_map_put() is needed because the above
+ * bpf_map_alloc_id() has published the map
+ * to the userspace and the userspace may
+ * have refcnt-ed it through BPF_MAP_GET_FD_BY_ID.
+ */
+ bpf_map_put(map);
+ return err;
+ }
trace_bpf_map_create(map, err);
return err;
-free_id:
- bpf_map_free_id(map);
free_map:
bpf_map_uncharge_memlock(map);
free_map_nouncharge:
@@ -331,6 +349,28 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd)
return map;
}
+/* map_idr_lock should have been held */
+static struct bpf_map *bpf_map_inc_not_zero(struct bpf_map *map,
+ bool uref)
+{
+ int refold;
+
+ refold = __atomic_add_unless(&map->refcnt, 1, 0);
+
+ if (refold >= BPF_MAX_REFCNT) {
+ __bpf_map_put(map, false);
+ return ERR_PTR(-EBUSY);
+ }
+
+ if (!refold)
+ return ERR_PTR(-ENOENT);
+
+ if (uref)
+ atomic_inc(&map->usercnt);
+
+ return map;
+}
+
int __weak bpf_stackmap_copy(struct bpf_map *map, void *key, void *value)
{
return -ENOTSUPP;
@@ -1167,6 +1207,38 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD map_id
+
+static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
+{
+ struct bpf_map *map;
+ u32 id = attr->map_id;
+ int fd;
+
+ if (CHECK_ATTR(BPF_MAP_GET_FD_BY_ID))
+ return -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ spin_lock_bh(&map_idr_lock);
+ map = idr_find(&map_idr, id);
+ if (map)
+ map = bpf_map_inc_not_zero(map, true);
+ else
+ map = ERR_PTR(-ENOENT);
+ spin_unlock_bh(&map_idr_lock);
+
+ if (IS_ERR(map))
+ return PTR_ERR(map);
+
+ fd = bpf_map_new_fd(map);
+ if (fd < 0)
+ bpf_map_put(map);
+
+ return fd;
+}
+
SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
{
union bpf_attr attr = {};
@@ -1255,6 +1327,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
case BPF_PROG_GET_FD_BY_ID:
err = bpf_prog_get_fd_by_id(&attr);
break;
+ case BPF_MAP_GET_FD_BY_ID:
+ err = bpf_map_get_fd_by_id(&attr);
+ break;
default:
err = -EINVAL;
break;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v4 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code
From: Andrew Lunn @ 2017-06-05 18:55 UTC (permalink / raw)
To: Paul Burton
Cc: netdev, David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser
In-Reply-To: <20170605173136.10795-3-paul.burton@imgtec.com>
On Mon, Jun 05, 2017 at 10:31:31AM -0700, Paul Burton wrote:
> The MIPS Boston development board uses the Intel EG20T Platform
> Controller Hub, including its gigabit ethernet controller, and requires
> that its RTL8211E PHY be reset much like the Minnow platform. Pull the
> PHY reset GPIO handling out of Minnow-specific code such that it can be
> shared by later patches.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v4 4/7] net: pch_gbe: Add device tree support
From: Andrew Lunn @ 2017-06-05 18:54 UTC (permalink / raw)
To: Paul Burton
Cc: netdev, David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser
In-Reply-To: <20170605173136.10795-5-paul.burton@imgtec.com>
> +static struct pch_gbe_privdata *
> +pch_gbe_get_priv(struct pci_dev *pdev, const struct pci_device_id *pci_id)
> +{
> + struct pch_gbe_privdata *pdata;
> + struct gpio_desc *gpio;
> +
> + if (!IS_ENABLED(CONFIG_OF))
> + return (struct pch_gbe_privdata *)pci_id->driver_data;
It is possible to enable CONFIG_OF on all architectures, including x86
used by Minnow. If somebody was to do this, i think Minnow breaks. What
i think you really want is:
if pci_id->driver_data;
return (struct pch_gbe_privdata *)pci_id->driver_data;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + gpio = devm_gpiod_get(&pdev->dev, "phy-reset", GPIOD_ASIS);
> + if (!IS_ERR(gpio))
> + pdata->phy_reset_gpio = gpio;
> + else if (PTR_ERR(gpio) != -ENOENT)
> + return ERR_CAST(gpio);
> +
> + return pdata;
> +}
There should not be a need to protect for !CONFIG_OF, and
devm_gpiod_get() knows how to look in ACPI tables, if an intel or
ARM64 platform it using that to list its GPIOs.
Andrew
^ permalink raw reply
* Re: More BPF verifier questions
From: Josef Bacik @ 2017-06-05 18:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Edward Cree, Alexei Starovoitov, Daniel Borkmann, netdev,
David Miller, iovisor-dev, Josef Bacik
In-Reply-To: <f75a8721-5cb0-f0bd-6224-b11bab1f036e@fb.com>
On Mon, Jun 05, 2017 at 11:11:05AM -0700, Alexei Starovoitov wrote:
> On 6/2/17 7:42 AM, Edward Cree wrote:
> >Also, I feel I haven't fully understood the semantics of {min,max}_value and
> > signed vs. unsigned comparisons. It seems that currently reg_set_min_max
> > [_inv] assumes that any given register-value will either only be used as
> > signed, or only be used as unsigned — which while potentially reasonable
> > for compiler-generated bytecode, could easily be untrue of a hand-crafted
> > BPF program.
> >For instance, take BPF_JGT(reg, val). This currently sets
> > false_reg->min_value to zero, but if val >= (1<<63), the false branch could
> > be taken for a value that's negative (when interpreted as signed).
>
> I think the way Josef intended it to behave is min/max_value are
> absolute values that 64-bits can hold.
> In that sense unsigned (JGT) comparison and the false branch are
> implying that min_value = 0.
> but if we don't treat min/max consistently as sign-free numbers
> than indeed it can cause issues.
> Do you have an asm test case that demonstrates that?
>
Well the min_value is a s64, but yeah anything negative is supposed to be
rejected, so it essentially acts as the range of unsigned absolute values it can
hold. I tried to hand craft a way to exploit this but I don't think it's
possible. In the normal BPF_JGT path with your case we'd end up with
false_reg->min_value = 0;
false_reg->max_value = 1<<63 = BPF_REGISTER_MAX_RANGE
true_reg->min_value = BPF_REGISTER_MIN_RANGE
>From here we want to exploit the fact that false_reg->min_value is not
necessarily correct, but in order to do that we need to get false_reg->max_value
below the actual size limit for the data we're reaching into, which means we
want to _only_ change false_reg->max_value. Thankfully there doesn't appear to
be a way to do that, everything changes either only min_value or both min_value
and max_value. I think we're safe here, unless I've missed something. Thanks,
Josef
^ permalink raw reply
* Re: [PATCH v4 3/7] dt-bindings: net: Document Intel pch_gbe binding
From: Sergei Shtylyov @ 2017-06-05 18:45 UTC (permalink / raw)
To: Paul Burton, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: David S . Miller, linux-mips-6z/3iImG2C8G8FEW9MqTrA, Eric Dumazet,
Jarod Wilson, Tobias Klauser, Mark Rutland, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170605173136.10795-4-paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Hello!
On 06/05/2017 08:31 PM, Paul Burton wrote:
> Introduce documentation for a device tree binding for the Intel Platform
> Controller Hub (PCH) GigaBit Ethernet (GBE) device. Although this is a
> PCIe device & thus largely auto-detectable, this binding will be used to
> provide the driver with the PHY reset GPIO.
>
> Signed-off-by: Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Cc: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>
> ---
>
> Changes in v4: None
>
> Changes in v3:
> - New patch.
>
> Changes in v2: None
>
> Documentation/devicetree/bindings/net/pch_gbe.txt | 25 +++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/pch_gbe.txt
>
> diff --git a/Documentation/devicetree/bindings/net/pch_gbe.txt b/Documentation/devicetree/bindings/net/pch_gbe.txt
> new file mode 100644
> index 000000000000..5de479c26b04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pch_gbe.txt
> @@ -0,0 +1,25 @@
> +Intel Platform Controller Hub (PCH) GigaBit Ethernet (GBE)
> +
> +Required properties:
> +- compatible: Should be the PCI vendor & device ID, eg. "pci8086,8802".
> +- reg: Should be a PCI device number as specified by the PCI bus
> + binding to IEEE Std 1275-1994.
> +- phy-reset-gpios: Should be a GPIO list containing a single GPIO that
> + resets the attached PHY when active.
> +
> +Example:
> +
> + eg20t_mac@2,0,1 {
> + compatible = "pci8086,8802";
> + reg = <0x00020100 0 0 0 0>;
> + phy-reset-gpios = <&eg20t_gpio 6
> + GPIO_ACTIVE_LOW>;
> + };
> +
> + eg20t_gpio: eg20t_gpio@2,0,2 {
Name it "gpio@2,0,2" please -- the node names need to be generic and
"gpio" is explicitly listed in the DT 1.0 spec...
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code
From: Andrew Lunn @ 2017-06-05 18:43 UTC (permalink / raw)
To: Paul Burton
Cc: netdev, Tobias Klauser, David S . Miller, Jarod Wilson,
linux-mips, Eric Dumazet
In-Reply-To: <4378321.AjzBvRIWiG@np-p-burton>
On Mon, Jun 05, 2017 at 10:21:50AM -0700, Paul Burton wrote:
> Hi Andrew,
>
> On Saturday, 3 June 2017 10:52:00 PDT Andrew Lunn wrote:
> > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > > b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c index
> > > d38198718005..cb9b904786e4 100644
> > > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > > @@ -360,6 +360,16 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw
> > > *hw, u8 * addr, u32 index)>
> > > pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
> > >
> > > }
> > >
> > > +static void pch_gbe_phy_set_reset(struct pch_gbe_hw *hw, int value)
> > > +{
> > > + struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
> > > +
> > > + if (!adapter->pdata || !adapter->pdata->phy_reset_gpio)
> > > + return;
> > > +
> > > + gpiod_set_value(adapter->pdata->phy_reset_gpio, value);
> >
> > Hi Paul
> >
> > Since you are using the gpiod_ API, the core will take notice of the
> > active low/active high flag when performing this set.
>
> Correct, and as desired.
>
> > > ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> > >
> > > "minnow_phy_reset");
> > >
> > > - if (ret) {
> > > + if (!ret)
> > > + pdata->phy_reset_gpio = gpio_to_desc(gpio);
> >
> > Here however, you are using the gpio_ API, which ignores the active
> > high/low flag in device tree. And in your binding patch, you give the
> > example:
> >
> > + phy-reset-gpios = <&eg20t_gpio 6
> > + GPIO_ACTIVE_LOW>;
> >
> > This active low is totally ignored.
>
> First of all, this path is for the existing Minnow platform, which doesn't use
> the device tree. That is, this code is the non-DT path so looking at what
> happens to flags in the device tree here makes no sense.
Thanks for the explanation. Now it makes sense.
Andrew
^ permalink raw reply
* Re: [PATCH 6/6] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Andrew Lunn @ 2017-06-05 18:21 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1dHq6O-0005XL-2m@rmk-PC.armlinux.org.uk>
On Mon, Jun 05, 2017 at 12:23:16PM +0100, Russell King wrote:
> Add phylib support for the Marvell Alaska X 10 Gigabit PHY (MV88X3310).
> This phy is able to operate at 10G, 1G, 100M and 10M speeds, and only
> supports Clause 45 accesses.
>
> The PHY appears (based on the vendor IDs) to be two different vendors
> IP, with each devad containing several instances.
>
> This PHY driver has only been tested with the RJ45 copper port, fiber
> port and a Marvell Armada 8040-based ethernet interface.
>
> It should be noted that to use the full range of speeds, MAC drivers
> need to also reconfigure the link mode as per phydev->interface, since
> the PHY automatically changes its interface mode depending on the
> negotiated speed.
Hi Russell
We are still missing documentation for the additions to adjust_link()
callback, but lets get this merged, and documentation can follow in a
separate patch.
Andrew
^ permalink raw reply
* Re: [PATCH 6/6] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Florian Fainelli @ 2017-06-05 18:21 UTC (permalink / raw)
To: Russell King, Andrew Lunn; +Cc: netdev
In-Reply-To: <E1dHq6O-0005XL-2m@rmk-PC.armlinux.org.uk>
On 06/05/2017 04:23 AM, Russell King wrote:
> Add phylib support for the Marvell Alaska X 10 Gigabit PHY (MV88X3310).
> This phy is able to operate at 10G, 1G, 100M and 10M speeds, and only
> supports Clause 45 accesses.
>
> The PHY appears (based on the vendor IDs) to be two different vendors
> IP, with each devad containing several instances.
>
> This PHY driver has only been tested with the RJ45 copper port, fiber
> port and a Marvell Armada 8040-based ethernet interface.
>
> It should be noted that to use the full range of speeds, MAC drivers
> need to also reconfigure the link mode as per phydev->interface, since
> the PHY automatically changes its interface mode depending on the
> negotiated speed.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 6/6] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Andrew Lunn @ 2017-06-05 18:20 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1dHq6O-0005XL-2m@rmk-PC.armlinux.org.uk>
On Mon, Jun 05, 2017 at 12:23:16PM +0100, Russell King wrote:
> Add phylib support for the Marvell Alaska X 10 Gigabit PHY (MV88X3310).
> This phy is able to operate at 10G, 1G, 100M and 10M speeds, and only
> supports Clause 45 accesses.
>
> The PHY appears (based on the vendor IDs) to be two different vendors
> IP, with each devad containing several instances.
>
> This PHY driver has only been tested with the RJ45 copper port, fiber
> port and a Marvell Armada 8040-based ethernet interface.
>
> It should be noted that to use the full range of speeds, MAC drivers
> need to also reconfigure the link mode as per phydev->interface, since
> the PHY automatically changes its interface mode depending on the
> negotiated speed.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: More BPF verifier questions
From: Alexei Starovoitov @ 2017-06-05 18:11 UTC (permalink / raw)
To: Edward Cree, Alexei Starovoitov, Daniel Borkmann
Cc: netdev, David Miller, iovisor-dev, Josef Bacik
In-Reply-To: <24a519bc-d403-f429-5d72-2a1d31edfbe7@solarflare.com>
On 6/2/17 7:42 AM, Edward Cree wrote:
> Also, I feel I haven't fully understood the semantics of {min,max}_value and
> signed vs. unsigned comparisons. It seems that currently reg_set_min_max
> [_inv] assumes that any given register-value will either only be used as
> signed, or only be used as unsigned — which while potentially reasonable
> for compiler-generated bytecode, could easily be untrue of a hand-crafted
> BPF program.
> For instance, take BPF_JGT(reg, val). This currently sets
> false_reg->min_value to zero, but if val >= (1<<63), the false branch could
> be taken for a value that's negative (when interpreted as signed).
I think the way Josef intended it to behave is min/max_value are
absolute values that 64-bits can hold.
In that sense unsigned (JGT) comparison and the false branch are
implying that min_value = 0.
but if we don't treat min/max consistently as sign-free numbers
than indeed it can cause issues.
Do you have an asm test case that demonstrates that?
> I tried to rewrite it to always base min_value on the signed and max_value
> on the unsigned interpretation of the value (which, by looking at the sign
> bit of the immediate, it can sometimes learn about the signed value from an
> unsigned compare or vice versa), but this fails to validate e.g. test
> "helper access to variable memory: stack, JMP (signed), correct bounds",
> which first checks r2 s<= 64, then checks r2 s> 0. If the checks were done
> in the reverse order, we'd know when checking r2 s<= 64 that r2 is
> positive, and that thus r2 u<= 64... but since we don't know that yet, when
> we check r2 s<= 64 we learn nothing about r2's unsigned max_value.
> So, my current theory is that to do this right, we need to track four bounds
> - s64 signed_min_value
> - s64 signed_max_value
> - u64 unsigned_min_value
> - u64 unsigned_max_value
that would be unfortunate.
We already don't track negative values. Hence
BPF_REGISTER_MIN_RANGE = -1
so pretty much anything negative is rejected.
I don't think it worth complicating things for them.
^ permalink raw reply
* [PATCH][netdev-next] mdio: mux: fix an incorrect less than zero error check using a u32
From: Colin King @ 2017-06-05 18:08 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The u32 variable v is being checked to see if an error return is
less than zero and this check has no effect because it is unsigned.
Fix this by making v and int (this also matches the type of
cb->bus_number which is assigned to the value in v).
Detected by CoverityScan, CID#1440454 ("Unsigned compared against zero")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/phy/mdio-mux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
index 47ded3904050..00755b6a42cf 100644
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -133,7 +133,7 @@ int mdio_mux_init(struct device *dev,
ret_val = -ENODEV;
for_each_available_child_of_node(dev->of_node, child_bus_node) {
- u32 v;
+ int v;
v = of_mdio_parse_addr(dev, child_bus_node);
if (v < 0) {
--
2.11.0
^ permalink raw reply related
* re: phy: cpcap-usb: Add CPCAP PMIC USB support
From: Colin Ian King @ 2017-06-05 17:54 UTC (permalink / raw)
To: Tony Lindgren
Cc: Andrew Lunn, Florian Fainelli, netdev,
linux-kernel@vger.kernel.org
Hi Tony,
While running static analysis on linux-next, CoverityScan picked up a
NULL pointer deference on ddata->pins when calling pinctrl_lookup_state:
466 ddata->pins = devm_pinctrl_get(ddata->dev);
1. Condition IS_ERR(ddata->pins), taking true branch.
467 if (IS_ERR(ddata->pins)) {
468 dev_info(ddata->dev, "default pins not configured:
%ld\n",
469 PTR_ERR(ddata->pins));
2. assign_zero: Assigning: ddata->pins = NULL.
470 ddata->pins = NULL;
471 }
472
CID 1440453 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)3.
var_deref_model: Passing null pointer ddata->pins to
pinctrl_lookup_state, which dereferences it. [show details]
473 ddata->pins_ulpi = pinctrl_lookup_state(ddata->pins, "ulpi");
I suspect the IS_ERROR() check should return with some error return
rather than continuing.
Colin.
^ permalink raw reply
* Re: [PATCH v1] net: phy: Delete unused function phy_ethtool_gset
From: Florian Fainelli @ 2017-06-05 17:41 UTC (permalink / raw)
To: Yuval Shaia, andrew, corbet, netdev, linux-doc
In-Reply-To: <20170605071840.16492-1-yuval.shaia@oracle.com>
On 06/05/2017 12:18 AM, Yuval Shaia wrote:
> It's unused, so remove it.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* [PATCH v4 7/7] net: pch_gbe: Allow build on MIPS platforms
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
Allow the pch_gbe driver to be built on MIPS platforms, in preparation
for its use on the MIPS Boston board.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
index 5f7a35212796..4d3809ae75e1 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
@@ -4,7 +4,7 @@
config PCH_GBE
tristate "OKI SEMICONDUCTOR IOH(ML7223/ML7831) GbE"
- depends on PCI && (X86_32 || COMPILE_TEST)
+ depends on PCI && (X86_32 || MIPS || COMPILE_TEST)
select MII
select PTP_1588_CLOCK_PCH
select NET_PTP_CLASSIFY
--
2.13.0
^ permalink raw reply related
* [PATCH v4 6/7] net: pch_gbe: Allow longer for resets
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
Resets of the EG20T MAC on the MIPS Boston development board take longer
than the 1000 loops that pch_gbe_wait_clr_bit was performing. Rather
than simply increasing the number of loops, switch to using
readl_poll_timeout_atomic() from linux/iopoll.h in order to provide some
independence from the speed of the CPU.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4: None
Changes in v3:
- Switch to using readl_poll_timeout_atomic().
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index c8554d3adf1c..c109646803a4 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -24,6 +24,7 @@
#include <linux/ptp_classify.h>
#include <linux/gpio.h>
#include <linux/gpio/consumer.h>
+#include <linux/iopoll.h>
#include <linux/of_gpio.h>
#define DRV_VERSION "1.01"
@@ -318,13 +319,11 @@ s32 pch_gbe_mac_read_mac_addr(struct pch_gbe_hw *hw)
*/
static void pch_gbe_wait_clr_bit(void *reg, u32 bit)
{
+ int err;
u32 tmp;
- /* wait busy */
- tmp = 1000;
- while ((ioread32(reg) & bit) && --tmp)
- cpu_relax();
- if (!tmp)
+ err = readl_poll_timeout_atomic(reg, tmp, !(tmp & bit), 10, 500);
+ if (err)
pr_err("Error: busy bit is not cleared\n");
}
--
2.13.0
^ permalink raw reply related
* [PATCH v4 5/7] net: pch_gbe: Always reset PHY along with MAC
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
On the MIPS Boston development board, the EG20T MAC does not report
receiving the RX clock from the (RGMII) RTL8211E PHY unless the PHY is
reset at the same time as the MAC. Since the pch_gbe driver resets the
MAC a number of times - twice during probe, and when taking down the
network interface - we need to reset the PHY at all the same times. Do
that from pch_gbe_mac_reset_hw which is used to reset the MAC in all
cases.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index b9d8504eb09c..c8554d3adf1c 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -380,10 +380,13 @@ static void pch_gbe_mac_reset_hw(struct pch_gbe_hw *hw)
{
/* Read the MAC address. and store to the private data */
pch_gbe_mac_read_mac_addr(hw);
+ pch_gbe_phy_set_reset(hw, 1);
iowrite32(PCH_GBE_ALL_RST, &hw->reg->RESET);
#ifdef PCH_GBE_MAC_IFOP_RGMII
iowrite32(PCH_GBE_MODE_GMII_ETHER, &hw->reg->MODE);
#endif
+ pch_gbe_phy_set_reset(hw, 0);
+ usleep_range(1250, 1500);
pch_gbe_wait_clr_bit(&hw->reg->RESET, PCH_GBE_ALL_RST);
/* Setup the receive addresses */
pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
--
2.13.0
^ permalink raw reply related
* [PATCH v4 4/7] net: pch_gbe: Add device tree support
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
Introduce support for retrieving the PHY reset GPIO from device tree,
which will be used on the MIPS Boston development board. This requires
support for probe deferral in order to work correctly, since the order
of device probe is not guaranteed & typically the EG20T GPIO controller
device will be probed after the ethernet MAC.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4:
- Use ERR_CAST(), thanks kbuild test robot/Fengguang!
Changes in v3: None
Changes in v2:
- Tidy up handling of parsing private data, drop err_out.
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 31 +++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index cb9b904786e4..b9d8504eb09c 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -23,6 +23,8 @@
#include <linux/net_tstamp.h>
#include <linux/ptp_classify.h>
#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
#define DRV_VERSION "1.01"
const char pch_driver_version[] = DRV_VERSION;
@@ -2565,13 +2567,40 @@ static void pch_gbe_remove(struct pci_dev *pdev)
free_netdev(netdev);
}
+static struct pch_gbe_privdata *
+pch_gbe_get_priv(struct pci_dev *pdev, const struct pci_device_id *pci_id)
+{
+ struct pch_gbe_privdata *pdata;
+ struct gpio_desc *gpio;
+
+ if (!IS_ENABLED(CONFIG_OF))
+ return (struct pch_gbe_privdata *)pci_id->driver_data;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+
+ gpio = devm_gpiod_get(&pdev->dev, "phy-reset", GPIOD_ASIS);
+ if (!IS_ERR(gpio))
+ pdata->phy_reset_gpio = gpio;
+ else if (PTR_ERR(gpio) != -ENOENT)
+ return ERR_CAST(gpio);
+
+ return pdata;
+}
+
static int pch_gbe_probe(struct pci_dev *pdev,
const struct pci_device_id *pci_id)
{
struct net_device *netdev;
struct pch_gbe_adapter *adapter;
+ struct pch_gbe_privdata *pdata;
int ret;
+ pdata = pch_gbe_get_priv(pdev, pci_id);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+
ret = pcim_enable_device(pdev);
if (ret)
return ret;
@@ -2609,7 +2638,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
adapter->pdev = pdev;
adapter->hw.back = adapter;
adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
- adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
+ adapter->pdata = pdata;
if (adapter->pdata && adapter->pdata->platform_init)
adapter->pdata->platform_init(pdev, adapter->pdata);
--
2.13.0
^ permalink raw reply related
* [PATCH v4 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
The MIPS Boston development board uses the Intel EG20T Platform
Controller Hub, including its gigabit ethernet controller, and requires
that its RTL8211E PHY be reset much like the Minnow platform. Pull the
PHY reset GPIO handling out of Minnow-specific code such that it can be
shared by later patches.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4: None
Changes in v3:
- Use adapter->pdata as arg to platform_init, to fix bisectability.
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 4 ++-
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 33 +++++++++++++++-------
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 8d710a3b4db0..de1dd08050f4 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -580,15 +580,17 @@ struct pch_gbe_hw_stats {
/**
* struct pch_gbe_privdata - PCI Device ID driver data
+ * @phy_reset_gpio: PHY reset GPIO descriptor.
* @phy_tx_clk_delay: Bool, configure the PHY TX delay in software
* @phy_disable_hibernate: Bool, disable PHY hibernation
* @platform_init: Platform initialization callback, called from
* probe, prior to PHY initialization.
*/
struct pch_gbe_privdata {
+ struct gpio_desc *phy_reset_gpio;
bool phy_tx_clk_delay;
bool phy_disable_hibernate;
- int (*platform_init)(struct pci_dev *pdev);
+ int (*platform_init)(struct pci_dev *, struct pch_gbe_privdata *);
};
/**
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index d38198718005..cb9b904786e4 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -360,6 +360,16 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index)
pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
}
+static void pch_gbe_phy_set_reset(struct pch_gbe_hw *hw, int value)
+{
+ struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
+
+ if (!adapter->pdata || !adapter->pdata->phy_reset_gpio)
+ return;
+
+ gpiod_set_value(adapter->pdata->phy_reset_gpio, value);
+}
+
/**
* pch_gbe_mac_reset_hw - Reset hardware
* @hw: Pointer to the HW structure
@@ -2601,7 +2611,14 @@ static int pch_gbe_probe(struct pci_dev *pdev,
adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
if (adapter->pdata && adapter->pdata->platform_init)
- adapter->pdata->platform_init(pdev);
+ adapter->pdata->platform_init(pdev, adapter->pdata);
+
+ if (adapter->pdata && adapter->pdata->phy_reset_gpio) {
+ pch_gbe_phy_set_reset(&adapter->hw, 1);
+ usleep_range(1250, 1500);
+ pch_gbe_phy_set_reset(&adapter->hw, 0);
+ usleep_range(1250, 1500);
+ }
adapter->ptp_pdev = pci_get_bus_and_slot(adapter->pdev->bus->number,
PCI_DEVFN(12, 4));
@@ -2694,7 +2711,8 @@ static int pch_gbe_probe(struct pci_dev *pdev,
/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
* ensure it is awake for probe and init. Request the line and reset the PHY.
*/
-static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
+static int pch_gbe_minnow_platform_init(struct pci_dev *pdev,
+ struct pch_gbe_privdata *pdata)
{
unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_LOW |
GPIOF_EXPORT | GPIOF_ACTIVE_LOW;
@@ -2703,16 +2721,11 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
"minnow_phy_reset");
- if (ret) {
+ if (!ret)
+ pdata->phy_reset_gpio = gpio_to_desc(gpio);
+ else
dev_err(&pdev->dev,
"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
- return ret;
- }
-
- gpio_set_value(gpio, 1);
- usleep_range(1250, 1500);
- gpio_set_value(gpio, 0);
- usleep_range(1250, 1500);
return ret;
}
--
2.13.0
^ permalink raw reply related
* [PATCH v4 1/7] net: pch_gbe: Mark Minnow PHY reset GPIO active low
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170605173136.10795-1-paul.burton@imgtec.com>
The Minnow PHY reset GPIO is set to 0 to enter reset & 1 to leave reset
- that is, it is an active low GPIO. In order to allow for the code to
be made more generic by further patches, indicate to the GPIO subsystem
that the GPIO is active low & invert the values it is set to such that
they reflect logically whether the device is being reset or not.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: linux-mips@linux-mips.org
Cc: netdev@vger.kernel.org
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 5ae9681a2da7..d38198718005 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -2696,7 +2696,8 @@ static int pch_gbe_probe(struct pci_dev *pdev,
*/
static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
{
- unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+ unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_LOW |
+ GPIOF_EXPORT | GPIOF_ACTIVE_LOW;
unsigned gpio = MINNOW_PHY_RESET_GPIO;
int ret;
@@ -2708,10 +2709,10 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
return ret;
}
- gpio_set_value(gpio, 0);
- usleep_range(1250, 1500);
gpio_set_value(gpio, 1);
usleep_range(1250, 1500);
+ gpio_set_value(gpio, 0);
+ usleep_range(1250, 1500);
return ret;
}
--
2.13.0
^ permalink raw reply related
* [PATCH v4 0/7] net: pch_gbe: Fixes & MIPS support
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, linux-mips, Eric Dumazet, Jarod Wilson,
Tobias Klauser, Paul Burton
In-Reply-To: <20170602234042.22782-1-paul.burton@imgtec.com>
The Intel EG20T Platform Controller Hub is used on the MIPS Boston
development board to provide various peripherals including ethernet.
This series fixes some issues with the pch_gbe driver discovered whilst
in use on the Boston board, and implements support for device tree which
we use to provide the PHY reset GPIO.
Applies atop v4.12-rc4.
Paul Burton (7):
net: pch_gbe: Mark Minnow PHY reset GPIO active low
net: pch_gbe: Pull PHY GPIO handling out of Minnow code
dt-bindings: net: Document Intel pch_gbe binding
net: pch_gbe: Add device tree support
net: pch_gbe: Always reset PHY along with MAC
net: pch_gbe: Allow longer for resets
net: pch_gbe: Allow build on MIPS platforms
Documentation/devicetree/bindings/net/pch_gbe.txt | 25 +++++++
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 2 +-
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 4 +-
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 79 +++++++++++++++++-----
4 files changed, 91 insertions(+), 19 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/pch_gbe.txt
--
2.13.0
^ permalink raw reply
* [PATCH v4 3/7] dt-bindings: net: Document Intel pch_gbe binding
From: Paul Burton @ 2017-06-05 17:31 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: David S . Miller, linux-mips-6z/3iImG2C8G8FEW9MqTrA, Eric Dumazet,
Jarod Wilson, Tobias Klauser, Paul Burton, Mark Rutland,
Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170605173136.10795-1-paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Introduce documentation for a device tree binding for the Intel Platform
Controller Hub (PCH) GigaBit Ethernet (GBE) device. Although this is a
PCIe device & thus largely auto-detectable, this binding will be used to
provide the driver with the PHY reset GPIO.
Signed-off-by: Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Cc: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
Changes in v4: None
Changes in v3:
- New patch.
Changes in v2: None
Documentation/devicetree/bindings/net/pch_gbe.txt | 25 +++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/pch_gbe.txt
diff --git a/Documentation/devicetree/bindings/net/pch_gbe.txt b/Documentation/devicetree/bindings/net/pch_gbe.txt
new file mode 100644
index 000000000000..5de479c26b04
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pch_gbe.txt
@@ -0,0 +1,25 @@
+Intel Platform Controller Hub (PCH) GigaBit Ethernet (GBE)
+
+Required properties:
+- compatible: Should be the PCI vendor & device ID, eg. "pci8086,8802".
+- reg: Should be a PCI device number as specified by the PCI bus
+ binding to IEEE Std 1275-1994.
+- phy-reset-gpios: Should be a GPIO list containing a single GPIO that
+ resets the attached PHY when active.
+
+Example:
+
+ eg20t_mac@2,0,1 {
+ compatible = "pci8086,8802";
+ reg = <0x00020100 0 0 0 0>;
+ phy-reset-gpios = <&eg20t_gpio 6
+ GPIO_ACTIVE_LOW>;
+ };
+
+ eg20t_gpio: eg20t_gpio@2,0,2 {
+ compatible = "pci8086,8803";
+ reg = <0x00020200 0 0 0 0>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
--
2.13.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v3 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code
From: Paul Burton @ 2017-06-05 17:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, Tobias Klauser, David S . Miller, Jarod Wilson,
linux-mips, Eric Dumazet
In-Reply-To: <20170603175200.GC17099@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]
Hi Andrew,
On Saturday, 3 June 2017 10:52:00 PDT Andrew Lunn wrote:
> > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c index
> > d38198718005..cb9b904786e4 100644
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > @@ -360,6 +360,16 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw
> > *hw, u8 * addr, u32 index)>
> > pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
> >
> > }
> >
> > +static void pch_gbe_phy_set_reset(struct pch_gbe_hw *hw, int value)
> > +{
> > + struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
> > +
> > + if (!adapter->pdata || !adapter->pdata->phy_reset_gpio)
> > + return;
> > +
> > + gpiod_set_value(adapter->pdata->phy_reset_gpio, value);
>
> Hi Paul
>
> Since you are using the gpiod_ API, the core will take notice of the
> active low/active high flag when performing this set.
Correct, and as desired.
> > ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> >
> > "minnow_phy_reset");
> >
> > - if (ret) {
> > + if (!ret)
> > + pdata->phy_reset_gpio = gpio_to_desc(gpio);
>
> Here however, you are using the gpio_ API, which ignores the active
> high/low flag in device tree. And in your binding patch, you give the
> example:
>
> + phy-reset-gpios = <&eg20t_gpio 6
> + GPIO_ACTIVE_LOW>;
>
> This active low is totally ignored.
First of all, this path is for the existing Minnow platform, which doesn't use
the device tree. That is, this code is the non-DT path so looking at what
happens to flags in the device tree here makes no sense.
If you want to examine what happens in the DT case then please look at
pch_gbe_get_priv() which uses devm_gpiod_get() which should honor the flags
provided by the DT.
> I personally would say this is all messed up, and going to result in
> problems for somebody with a board which actually needs an
> GPIO_ACTIVE_HIGH.
It's a path which only applies to the Minnow board, which is always active
low. Before patch 1 of this series that was done without the GPIOF_ACTIVE_LOW
flag by setting GPIO values to reflect the physical GPIO line low/high rather
than the logical active/not-active. After patch 1 this path began using
GPIOF_ACTIVE_LOW such that the rest of the code can use logical active/not-
active values which work with either active low or active high GPIOs. In this
Minnow-specific path GPIOF_ACTIVE_LOW is hardcoded, but again only applies to
the Minnow board which doesn't take the GPIO value from device tree.
> Please use the gpiod_ API through out and respect the flags in the
> device tree binding.
The gpiod_ API, quite rightly, retrieves GPIOs associated with a device - for
example via the device tree. The Minnow board, which is what the driver
already supports in-tree, does not do this but instead hardcodes a GPIO number
(MINNOW_PHY_RESET_GPIO). I don't own, use or care about the Minnow platform so
that is not something that I can change. In the path that my patch does add,
the path which is used with DT, I already do use the gpiod_ API & respect
flags from the DT.
Thanks,
Paul
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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