* Re: [RFC v4 0/3] vhost: introduce mdev based hardware backend
From: Jason Wang @ 2019-09-18 5:51 UTC (permalink / raw)
To: Tiwei Bie
Cc: mst, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190917105801.GA24855@___>
On 2019/9/17 下午6:58, Tiwei Bie wrote:
> On Tue, Sep 17, 2019 at 11:32:03AM +0800, Jason Wang wrote:
>> On 2019/9/17 上午9:02, Tiwei Bie wrote:
>>> This RFC is to demonstrate below ideas,
>>>
>>> a) Build vhost-mdev on top of the same abstraction defined in
>>> the virtio-mdev series [1];
>>>
>>> b) Introduce /dev/vhost-mdev to do vhost ioctls and support
>>> setting mdev device as backend;
>>>
>>> Now the userspace API looks like this:
>>>
>>> - Userspace generates a compatible mdev device;
>>>
>>> - Userspace opens this mdev device with VFIO API (including
>>> doing IOMMU programming for this mdev device with VFIO's
>>> container/group based interface);
>>>
>>> - Userspace opens /dev/vhost-mdev and gets vhost fd;
>>>
>>> - Userspace uses vhost ioctls to setup vhost (userspace should
>>> do VHOST_MDEV_SET_BACKEND ioctl with VFIO group fd and device
>>> fd first before doing other vhost ioctls);
>>>
>>> Only compile test has been done for this series for now.
>>
>> Have a hard thought on the architecture:
> Thanks a lot! Do appreciate it!
>
>> 1) Create a vhost char device and pass vfio mdev device fd to it as a
>> backend and translate vhost-mdev ioctl to virtio mdev transport (e.g
>> read/write). DMA was done through the VFIO DMA mapping on the container that
>> is attached.
> Yeah, that's what we are doing in this series.
>
>> We have two more choices:
>>
>> 2) Use vfio-mdev but do not create vhost-mdev device, instead, just
>> implement vhost ioctl on vfio_device_ops, and translate them into
>> virtio-mdev transport or just pass ioctl to parent.
> Yeah. Instead of introducing /dev/vhost-mdev char device, do
> vhost ioctls on VFIO device fd directly. That's what we did
> in RFC v3.
>
>> 3) Don't use vfio-mdev, create a new vhost-mdev driver, during probe still
>> try to add dev to vfio group and talk to parent with device specific ops
> If my understanding is correct, this means we need to introduce
> a new VFIO device driver to replace the existing vfio-mdev driver
> in our case. Below is a quick draft just to show my understanding:
>
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
>
> #include "mdev_private.h"
>
> /* XXX: we need a proper way to include below vhost header. */
> #include "../../vhost/vhost.h"
>
> static int vfio_vhost_mdev_open(void *device_data)
> {
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
>
> /* ... */
> vhost_dev_init(...);
>
> return 0;
> }
>
> static void vfio_vhost_mdev_release(void *device_data)
> {
> /* ... */
> module_put(THIS_MODULE);
> }
>
> static long vfio_vhost_mdev_unlocked_ioctl(void *device_data,
> unsigned int cmd, unsigned long arg)
> {
> struct mdev_device *mdev = device_data;
> struct mdev_parent *parent = mdev->parent;
>
> /*
> * Use vhost ioctls.
> *
> * We will have a different parent_ops design.
> * And potentially, we can share the same parent_ops
> * with virtio_mdev.
> */
> switch (cmd) {
> case VHOST_GET_FEATURES:
> parent->ops->get_features(mdev, ...);
> break;
> /* ... */
> }
>
> return 0;
> }
>
> static ssize_t vfio_vhost_mdev_read(void *device_data, char __user *buf,
> size_t count, loff_t *ppos)
> {
> /* ... */
> return 0;
> }
>
> static ssize_t vfio_vhost_mdev_write(void *device_data, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> /* ... */
> return 0;
> }
>
> static int vfio_vhost_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> {
> /* ... */
> return 0;
> }
>
> static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> .name = "vfio-vhost-mdev",
> .open = vfio_vhost_mdev_open,
> .release = vfio_vhost_mdev_release,
> .ioctl = vfio_vhost_mdev_unlocked_ioctl,
> .read = vfio_vhost_mdev_read,
> .write = vfio_vhost_mdev_write,
> .mmap = vfio_vhost_mdev_mmap,
> };
>
> static int vfio_vhost_mdev_probe(struct device *dev)
> {
> struct mdev_device *mdev = to_mdev_device(dev);
>
> /* ... */
> return vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, mdev);
> }
>
> static void vfio_vhost_mdev_remove(struct device *dev)
> {
> /* ... */
> vfio_del_group_dev(dev);
> }
>
> static struct mdev_driver vfio_vhost_mdev_driver = {
> .name = "vfio_vhost_mdev",
> .probe = vfio_vhost_mdev_probe,
> .remove = vfio_vhost_mdev_remove,
> };
>
> static int __init vfio_vhost_mdev_init(void)
> {
> return mdev_register_driver(&vfio_vhost_mdev_driver, THIS_MODULE);
> }
> module_init(vfio_vhost_mdev_init)
>
> static void __exit vfio_vhost_mdev_exit(void)
> {
> mdev_unregister_driver(&vfio_vhost_mdev_driver);
> }
> module_exit(vfio_vhost_mdev_exit)
Yes, something like this basically.
>> So I have some questions:
>>
>> 1) Compared to method 2, what's the advantage of creating a new vhost char
>> device? I guess it's for keep the API compatibility?
> One benefit is that we can avoid doing vhost ioctls on
> VFIO device fd.
Yes, but any benefit from doing this?
>
>> 2) For method 2, is there any easy way for user/admin to distinguish e.g
>> ordinary vfio-mdev for vhost from ordinary vfio-mdev?
> I think device-api could be a choice.
Ok.
>
>> I saw you introduce
>> ops matching helper but it's not friendly to management.
> The ops matching helper is just to check whether a given
> vfio-device is based on a mdev device.
>
>> 3) A drawback of 1) and 2) is that it must follow vfio_device_ops that
>> assumes the parameter comes from userspace, it prevents support kernel
>> virtio drivers.
>>
>> 4) So comes the idea of method 3, since it register a new vhost-mdev driver,
>> we can use device specific ops instead of VFIO ones, then we can have a
>> common API between vDPA parent and vhost-mdev/virtio-mdev drivers.
> As the above draft shows, this requires introducing a new
> VFIO device driver. I think Alex's opinion matters here.
Yes, it is.
Thanks
> Thanks,
> Tiwei
>
>> What's your thoughts?
>>
>> Thanks
>>
>>
>>> RFCv3: https://patchwork.kernel.org/patch/11117785/
>>>
>>> [1] https://lkml.org/lkml/2019/9/10/135
>>>
>>> Tiwei Bie (3):
>>> vfio: support getting vfio device from device fd
>>> vfio: support checking vfio driver by device ops
>>> vhost: introduce mdev based hardware backend
>>>
>>> drivers/vfio/mdev/vfio_mdev.c | 3 +-
>>> drivers/vfio/vfio.c | 32 +++
>>> drivers/vhost/Kconfig | 9 +
>>> drivers/vhost/Makefile | 3 +
>>> drivers/vhost/mdev.c | 462 +++++++++++++++++++++++++++++++
>>> drivers/vhost/vhost.c | 39 ++-
>>> drivers/vhost/vhost.h | 6 +
>>> include/linux/vfio.h | 11 +
>>> include/uapi/linux/vhost.h | 10 +
>>> include/uapi/linux/vhost_types.h | 5 +
>>> 10 files changed, 573 insertions(+), 7 deletions(-)
>>> create mode 100644 drivers/vhost/mdev.c
>>>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 00/14] samples: bpf: improve/fix cross-compilation
From: Andrii Nakryiko @ 2019-09-18 5:33 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-1-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 4:02 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
Thanks for these changes, they look good overall. It would be great if
someone else could test and validate that cross-compilation works not
just in your environment and generated binaries successfully run on
target machines, though...
[...]
>
> Ivan Khoronzhuk (14):
> samples: bpf: makefile: fix HDR_PROBE "echo"
> samples: bpf: makefile: fix cookie_uid_helper_example obj build
> samples: bpf: makefile: use --target from cross-compile
> samples: bpf: use own EXTRA_CFLAGS for clang commands
> samples: bpf: makefile: use __LINUX_ARM_ARCH__ selector for arm
> samples: bpf: makefile: drop unnecessarily inclusion for bpf_load
> samples: bpf: add makefile.target for separate CC target build
> samples: bpf: makefile: base target programs rules on Makefile.target
> samples: bpf: makefile: use own flags but not host when cross compile
> samples: bpf: makefile: use target CC environment for HDR_PROBE
> libbpf: makefile: add C/CXX/LDFLAGS to libbpf.so and test_libpf
> targets
> samples: bpf: makefile: provide C/CXX/LD flags to libbpf
> samples: bpf: makefile: add sysroot support
> samples: bpf: README: add preparation steps and sysroot info
>
Prefixes like "samples: bpf: makefile: " are very verbose without
adding much value. We've been converging to essentially this set of
prefixes:
- "libbpf:" for libbpf changes
- "bpftool:" for bpftool changes
- "selftests/bpf:" for bpf selftests
- "samples/bpf:" for bpf samples
There is no need to prefix with "makefile: " either. Please update
your patch subjects in the next version. Thanks!
> samples/bpf/Makefile | 179 +++++++++++++++++++++---------------
> samples/bpf/Makefile.target | 75 +++++++++++++++
> samples/bpf/README.rst | 41 ++++++++-
> tools/lib/bpf/Makefile | 11 ++-
> 4 files changed, 225 insertions(+), 81 deletions(-)
> create mode 100644 samples/bpf/Makefile.target
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 13/14] samples: bpf: makefile: add sysroot support
From: Andrii Nakryiko @ 2019-09-18 5:23 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-14-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 4:00 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> Basically it only enables that was added by previous couple fixes.
> Sysroot contains correct libs installed and its headers ofc. Useful
Please, let's not use unnecessary abbreviations/slang. "Of course" is
not too long and is a proper English, let's stick to it.
> when working with NFC or virtual machine.
>
> Usage:
>
> clean (on demand)
> make ARCH=arm -C samples/bpf clean
> make ARCH=arm -C tools clean
> make ARCH=arm clean
>
> configure and install headers:
>
> make ARCH=arm defconfig
> make ARCH=arm headers_install
>
> build samples/bpf:
> make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- samples/bpf/ \
> SYSROOT="path/to/sysroot"
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> samples/bpf/Makefile | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 133123d4c7d7..57ddf055d6c3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -194,6 +194,11 @@ TPROGS_CFLAGS += -I$(srctree)/tools/lib/
> TPROGS_CFLAGS += -I$(srctree)/tools/include
> TPROGS_CFLAGS += -I$(srctree)/tools/perf
>
> +ifdef SYSROOT
> +TPROGS_CFLAGS += --sysroot=${SYSROOT}
> +TPROGS_LDFLAGS := -L${SYSROOT}/usr/lib
Please stay consistent: $() instead of ${}?
> +endif
> +
> EXTRA_CXXFLAGS := $(TPROGS_CFLAGS)
>
> # options not valid for C++
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 12/14] samples: bpf: makefile: provide C/CXX/LD flags to libbpf
From: Andrii Nakryiko @ 2019-09-18 5:20 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-13-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 3:58 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> In order to build libs using C/CXX/LD flags of target arch,
> provide them to libbpf make.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> samples/bpf/Makefile | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 18ec22e7b444..133123d4c7d7 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -182,8 +182,6 @@ ifdef CROSS_COMPILE
> TPROGS_CFLAGS += -Wall
> TPROGS_CFLAGS += -O2
> TPROGS_CFLAGS += -fomit-frame-pointer
> -TPROGS_CFLAGS += -Wmissing-prototypes
> -TPROGS_CFLAGS += -Wstrict-prototypes
> else
> TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
> TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> @@ -196,6 +194,14 @@ TPROGS_CFLAGS += -I$(srctree)/tools/lib/
> TPROGS_CFLAGS += -I$(srctree)/tools/include
> TPROGS_CFLAGS += -I$(srctree)/tools/perf
>
> +EXTRA_CXXFLAGS := $(TPROGS_CFLAGS)
> +
> +# options not valid for C++
> +ifdef CROSS_COMPILE
> +$(TPROGS_CFLAGS) += -Wmissing-prototypes
> +$(TPROGS_CFLAGS) += -Wstrict-prototypes
> +endif
> +
ugh, let's really get rid of dependency on C++ compiler, as suggested
for previous patch.
> TPROGCFLAGS_bpf_load.o += -Wno-unused-variable
>
> TPROGS_LDLIBS += $(LIBBPF) -lelf
> @@ -257,7 +263,9 @@ clean:
>
> $(LIBBPF): FORCE
> # Fix up variables inherited from Kbuild that tools/ build system won't like
> - $(MAKE) -C $(dir $@) RM='rm -rf' LDFLAGS= srctree=$(BPF_SAMPLES_PATH)/../../ O=
> + $(MAKE) -C $(dir $@) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
> + EXTRA_CXXFLAGS="$(EXTRA_CXXFLAGS)" LDFLAGS=$(TPROGS_LDFLAGS) \
> + srctree=$(BPF_SAMPLES_PATH)/../../ O=
>
> $(obj)/syscall_nrs.h: $(obj)/syscall_nrs.s FORCE
> $(call filechk,offsets,__SYSCALL_NRS_H__)
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 11/14] libbpf: makefile: add C/CXX/LDFLAGS to libbpf.so and test_libpf targets
From: Andrii Nakryiko @ 2019-09-18 5:19 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-12-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 4:00 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> In case of LDFLAGS and EXTRA_CC/CXX flags there is no way to pass them
> correctly to build command, for instance when --sysroot is used or
> external libraries are used, like -lelf, wich can be absent in
> toolchain. This can be used for samples/bpf cross-compiling allowing
> to get elf lib from sysroot.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> tools/lib/bpf/Makefile | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index c6f94cffe06e..bccfa556ef4e 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -94,6 +94,10 @@ else
> CFLAGS := -g -Wall
> endif
>
> +ifdef EXTRA_CXXFLAGS
> + CXXFLAGS := $(EXTRA_CXXFLAGS)
> +endif
> +
> ifeq ($(feature-libelf-mmap), 1)
> override CFLAGS += -DHAVE_LIBELF_MMAP_SUPPORT
> endif
> @@ -176,8 +180,9 @@ $(BPF_IN): force elfdep bpfdep
> $(OUTPUT)libbpf.so: $(OUTPUT)libbpf.so.$(LIBBPF_VERSION)
>
> $(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN)
> - $(QUIET_LINK)$(CC) --shared -Wl,-soname,libbpf.so.$(LIBBPF_MAJOR_VERSION) \
> - -Wl,--version-script=$(VERSION_SCRIPT) $^ -lelf -o $@
> + $(QUIET_LINK)$(CC) $(LDFLAGS) \
> + --shared -Wl,-soname,libbpf.so.$(LIBBPF_MAJOR_VERSION) \
> + -Wl,--version-script=$(VERSION_SCRIPT) $^ -lelf -o $@
> @ln -sf $(@F) $(OUTPUT)libbpf.so
> @ln -sf $(@F) $(OUTPUT)libbpf.so.$(LIBBPF_MAJOR_VERSION)
>
> @@ -185,7 +190,7 @@ $(OUTPUT)libbpf.a: $(BPF_IN)
> $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
>
> $(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
> - $(QUIET_LINK)$(CXX) $(INCLUDES) $^ -lelf -o $@
> + $(QUIET_LINK)$(CXX) $(CXXFLAGS) $(LDFLAGS) $(INCLUDES) $^ -lelf -o $@
Instead of doing ifdef EXTRA_CXXFLAGS bit above, you can just include
both $(CXXFLAGS) and $(EXTRA_CXXFLAGS), which will do the right thing
(and is actually recommended my make documentation way to do this).
But actually, there is no need to use C++ compiler here,
test_libbpf.cpp can just be plain C. Do you mind renaming it to .c and
using C compiler instead?
>
> $(OUTPUT)libbpf.pc:
> $(QUIET_GEN)sed -e "s|@PREFIX@|$(prefix)|" \
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH] drivers/net: release skb on failure
From: Navid Emamdoost @ 2019-09-18 4:45 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Manish Chopra,
GR-Linux-NIC-Dev, David S. Miller, netdev, linux-kernel
In ql_run_loopback_test, ql_lb_send does not release skb when fails. So
it must be released before returning.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
index a6886cc5654c..d539b71b2a5c 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
@@ -544,8 +544,10 @@ static int ql_run_loopback_test(struct ql_adapter *qdev)
skb_put(skb, size);
ql_create_lb_frame(skb, size);
rc = ql_lb_send(skb, qdev->ndev);
- if (rc != NETDEV_TX_OK)
+ if (rc != NETDEV_TX_OK) {
+ dev_kfree_skb_any(skb);
return -EPIPE;
+ }
atomic_inc(&qdev->lb_count);
}
/* Give queue time to settle before testing results. */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 bpf-next 10/14] samples: bpf: makefile: use target CC environment for HDR_PROBE
From: Andrii Nakryiko @ 2019-09-18 4:20 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-11-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> No need in hacking HOSTCC to be cross-compiler any more, so drop
> this trick and use target CC for HDR_PROBE.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
[...]
^ permalink raw reply
* Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Richard Cochran @ 2019-09-18 3:42 UTC (permalink / raw)
To: Jianyong Wu
Cc: netdev, pbonzini, sean.j.christopherson, maz, Mark.Rutland,
Will.Deacon, suzuki.poulose, linux-kernel, Steve.Capper, Kaly.Xin,
justin.he, nd, linux-arm-kernel
In-Reply-To: <20190917112430.45680-4-jianyong.wu@arm.com>
On Tue, Sep 17, 2019 at 07:24:27AM -0400, Jianyong Wu wrote:
> A number of PTP drivers (such as ptp-kvm) are assuming what the
> current clock source is, which could lead to interesting effects on
> systems where the clocksource can change depending on external events.
>
> For this purpose, add a new API that retrives both the current
> monotonic clock as well as its counter value.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
> include/linux/timekeeping.h | 3 +++
> kernel/time/timekeeping.c | 13 +++++++++++++
> 2 files changed, 16 insertions(+)
For core time keeping changes, you must CC lkml, tglx, and John Stultz.
Thanks,
Richard
^ permalink raw reply
* RE: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Jianyong Wu (Arm Technology China) @ 2019-09-18 2:50 UTC (permalink / raw)
To: Marc Zyngier, netdev@vger.kernel.org, pbonzini@redhat.com,
sean.j.christopherson@intel.com, richardcochran@gmail.com,
Mark Rutland, Will Deacon, Suzuki Poulose
Cc: linux-kernel@vger.kernel.org, Steve Capper,
Kaly Xin (Arm Technology China), Justin He (Arm Technology China),
nd, linux-arm-kernel@lists.infradead.org
In-Reply-To: <ad38f692-a7c4-34e0-8236-ebd2d237bd93@kernel.org>
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, September 18, 2019 1:10 AM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> Technology China) <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
>
> On 17/09/2019 12:24, Jianyong Wu wrote:
> > A number of PTP drivers (such as ptp-kvm) are assuming what the
> > current clock source is, which could lead to interesting effects on
> > systems where the clocksource can change depending on external events.
> >
> > For this purpose, add a new API that retrives both the current
> > monotonic clock as well as its counter value.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>
> There must be something wrong with the way you've taken this patch in your
> tree. My authorship is gone (not that I deeply care about it, but it is good
> practice to keep attributions), and the subject line has been rewritten.
>
> I'd appreciate it if you could fix this in a future revision of this series. For
> reference, the original patch is here[1].
>
Sorry for "steal" your patch, I'm not familiar with it and neglect this important change.
I just copy this patch from your email and add the subject myself.
I will fix all of them later in v3.
> > ---
> > include/linux/timekeeping.h | 3 +++
> > kernel/time/timekeeping.c | 13 +++++++++++++
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> > index a8ab0f143ac4..a5389adaa8bc 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -247,6 +247,9 @@ extern int get_device_system_crosststamp(
> > struct system_time_snapshot *history,
> > struct system_device_crosststamp *xtstamp);
> >
> > +/* Obtain current monotonic clock and its counter value */ extern
> > +void get_current_counterval(struct system_counterval_t *sc);
> > +
> > /*
> > * Simultaneously snapshot realtime and monotonic raw clocks
> > */
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 44b726bab4bd..07a0969625b1 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1098,6 +1098,19 @@ static bool cycle_between(u64 before, u64 test,
> u64 after)
> > return false;
> > }
> >
> > +/**
> > + * get_current_counterval - Snapshot the current clocksource and counter
> value
> > + * @sc: Pointer to a struct containing the current clocksource and its
> value
> > + */
> > +void get_current_counterval(struct system_counterval_t *sc) {
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > +
> > + sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > + sc->cycles = sc->cs->read(sc->cs);
> > +}
> > +EXPORT_SYMBOL_GPL(get_current_counterval);
>
> This export wasn't in my original patch. I guess you need it because your ptp
> driver builds as a module? It'd be good to mention it in the commit log.
>
Yeah, ptp_kvm will be a module, so export is necessary. I will mention this change
in commit log.
Thanks
Jianyong Wu
> > +
> > /**
> > * get_device_system_crosststamp - Synchronously capture
> system/device timestamp
> > * @get_time_fn: Callback to get simultaneous device time and
> >
>
> Thanks,
>
> M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-
> platforms.git/commit/?h=timer/counterval&id=a6e8abce025691b6a55e1c195
> 878d7f76bfeb9d1
> --
> Jazz is not dead, it just smells funny...
^ permalink raw reply
* Re: [PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases
From: zhong jiang @ 2019-09-18 2:43 UTC (permalink / raw)
To: zhong jiang
Cc: jakub.kicinski, davem, anna.schumaker, trond.myklebust, netdev,
linux-kernel
In-Reply-To: <1568774195-8677-1-git-send-email-zhongjiang@huawei.com>
On 2019/9/18 10:36, zhong jiang wrote:
> In general, Use kzfree() to replace memset() + kfree() is feasible and
> resonable. But It's btter to use memzero_explicit() to replace memset()
> in crypto cases.
s/btter/better/, will repost. sorry for that.
Thanks,
zhong jiang
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 113f608..7e4f32f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
> return 0;
>
> err_aead:
> - kzfree(xs->aead);
> + memzero_explicit(xs->aead, sizeof(*xs->aead));
> + kfree(xs->aead);
> err_xs:
> - kzfree(xs);
> + memzero_explicit(xs, sizeof(*xs));
> + kfree(xs);
> err_out:
> msgbuf[1] = err;
> return err;
> @@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
> ixgbe_ipsec_del_sa(xs);
>
> /* remove the xs that was made-up in the add request */
> - kzfree(xs);
> + memzero_explicit(xs, sizeof(*xs));
> + kfree(xs);
>
> return 0;
> }
^ permalink raw reply
* [RESENT PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases
From: zhong jiang @ 2019-09-18 2:38 UTC (permalink / raw)
To: jakub.kicinski, davem
Cc: anna.schumaker, trond.myklebust, netdev, linux-kernel, zhongjiang
In general, Use kzfree() to replace memset() + kfree() is feasible and
resonable. But It's better to use memzero_explicit() to replace memset()
in crypto cases.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 113f608..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
return 0;
err_aead:
- kzfree(xs->aead);
+ memzero_explicit(xs->aead, sizeof(*xs->aead));
+ kfree(xs->aead);
err_xs:
- kzfree(xs);
+ memzero_explicit(xs, sizeof(*xs));
+ kfree(xs);
err_out:
msgbuf[1] = err;
return err;
@@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
/* remove the xs that was made-up in the add request */
- kzfree(xs);
+ memzero_explicit(xs, sizeof(*xs));
+ kfree(xs);
return 0;
}
--
1.7.12.4
^ permalink raw reply related
* [PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases
From: zhong jiang @ 2019-09-18 2:36 UTC (permalink / raw)
To: jakub.kicinski, davem
Cc: anna.schumaker, trond.myklebust, netdev, linux-kernel, zhongjiang
In general, Use kzfree() to replace memset() + kfree() is feasible and
resonable. But It's btter to use memzero_explicit() to replace memset()
in crypto cases.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 113f608..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
return 0;
err_aead:
- kzfree(xs->aead);
+ memzero_explicit(xs->aead, sizeof(*xs->aead));
+ kfree(xs->aead);
err_xs:
- kzfree(xs);
+ memzero_explicit(xs, sizeof(*xs));
+ kfree(xs);
err_out:
msgbuf[1] = err;
return err;
@@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
/* remove the xs that was made-up in the add request */
- kzfree(xs);
+ memzero_explicit(xs, sizeof(*xs));
+ kfree(xs);
return 0;
}
--
1.7.12.4
^ permalink raw reply related
* Re: [RESENT PATCH v2] ixgbe: Use memzero_explicit directly in crypto cases
From: zhong jiang @ 2019-09-18 1:34 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, anna.schumaker, trond.myklebust, netdev, linux-kernel
In-Reply-To: <20190917111107.307295c6@cakuba.netronome.com>
On 2019/9/18 2:11, Jakub Kicinski wrote:
> On Tue, 17 Sep 2019 22:44:22 +0800, zhong jiang wrote:
>> It's better to use memzero_explicit() to replace memset() in crypto cases.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> Thank you for the follow up! Your previous patch to use kzfree()
> has been applied on its own merit, could you rebase this one on top
> of current net-next/master?
I will do that.
Thanks,
zhong jiang
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 31629fc..7e4f32f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -960,10 +960,10 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
>> return 0;
>>
>> err_aead:
>> - memset(xs->aead, 0, sizeof(*xs->aead));
>> + memzero_explicit(xs->aead, sizeof(*xs->aead));
>> kfree(xs->aead);
>> err_xs:
>> - memset(xs, 0, sizeof(*xs));
>> + memzero_explicit(xs, sizeof(*xs));
>> kfree(xs);
>> err_out:
>> msgbuf[1] = err;
>> @@ -1049,7 +1049,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf)
>> ixgbe_ipsec_del_sa(xs);
>>
>> /* remove the xs that was made-up in the add request */
>> - memset(xs, 0, sizeof(*xs));
>> + memzero_explicit(xs, sizeof(*xs));
>> kfree(xs);
>>
>> return 0;
>
> .
>
^ permalink raw reply
* Re: [PATCH] ethernet/intel: release the local packet buffer
From: kbuild test robot @ 2019-09-18 1:23 UTC (permalink / raw)
To: Navid Emamdoost
Cc: kbuild-all, emamd001, smccaman, kjlu, Navid Emamdoost,
Jeff Kirsher, David S. Miller, intel-wired-lan, netdev,
linux-kernel
In-Reply-To: <20190918000013.32083-1-navid.emamdoost@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4264 bytes --]
Hi Navid,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3 next-20190917]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Navid-Emamdoost/ethernet-intel-release-the-local-packet-buffer/20190918-080148
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sparc64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from include/linux/if_ether.h:19:0,
from include/uapi/linux/ethtool.h:19,
from include/linux/ethtool.h:18,
from include/linux/netdevice.h:37,
from drivers/net/ethernet/intel/e100.c:140:
drivers/net/ethernet/intel/e100.c: In function 'e100_loopback_test':
>> include/linux/skbuff.h:1149:26: warning: 'skb' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define dev_kfree_skb(a) consume_skb(a)
^~~~~~~~~~~
drivers/net/ethernet/intel/e100.c:2347:18: note: 'skb' was declared here
struct sk_buff *skb;
^~~
--
In file included from include/linux/if_ether.h:19:0,
from include/uapi/linux/ethtool.h:19,
from include/linux/ethtool.h:18,
from include/linux/netdevice.h:37,
from drivers/net//ethernet/intel/e100.c:140:
drivers/net//ethernet/intel/e100.c: In function 'e100_loopback_test':
>> include/linux/skbuff.h:1149:26: warning: 'skb' may be used uninitialized in this function [-Wmaybe-uninitialized]
#define dev_kfree_skb(a) consume_skb(a)
^~~~~~~~~~~
drivers/net//ethernet/intel/e100.c:2347:18: note: 'skb' was declared here
struct sk_buff *skb;
^~~
vim +/skb +1149 include/linux/skbuff.h
cd0a137acbb662 Florian Fainelli 2017-08-22 1133
cd0a137acbb662 Florian Fainelli 2017-08-22 1134 /**
cd0a137acbb662 Florian Fainelli 2017-08-22 1135 * skb_pad - zero pad the tail of an skb
cd0a137acbb662 Florian Fainelli 2017-08-22 1136 * @skb: buffer to pad
cd0a137acbb662 Florian Fainelli 2017-08-22 1137 * @pad: space to pad
cd0a137acbb662 Florian Fainelli 2017-08-22 1138 *
cd0a137acbb662 Florian Fainelli 2017-08-22 1139 * Ensure that a buffer is followed by a padding area that is zero
cd0a137acbb662 Florian Fainelli 2017-08-22 1140 * filled. Used by network drivers which may DMA or transfer data
cd0a137acbb662 Florian Fainelli 2017-08-22 1141 * beyond the buffer end onto the wire.
cd0a137acbb662 Florian Fainelli 2017-08-22 1142 *
cd0a137acbb662 Florian Fainelli 2017-08-22 1143 * May return error in out of memory cases. The skb is freed on error.
cd0a137acbb662 Florian Fainelli 2017-08-22 1144 */
cd0a137acbb662 Florian Fainelli 2017-08-22 1145 static inline int skb_pad(struct sk_buff *skb, int pad)
cd0a137acbb662 Florian Fainelli 2017-08-22 1146 {
cd0a137acbb662 Florian Fainelli 2017-08-22 1147 return __skb_pad(skb, pad, true);
cd0a137acbb662 Florian Fainelli 2017-08-22 1148 }
ead2ceb0ec9f85 Neil Horman 2009-03-11 @1149 #define dev_kfree_skb(a) consume_skb(a)
^1da177e4c3f41 Linus Torvalds 2005-04-16 1150
:::::: The code at line 1149 was first introduced by commit
:::::: ead2ceb0ec9f85cff19c43b5cdb2f8a054484431 Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
:::::: TO: Neil Horman <nhorman@tuxdriver.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58748 bytes --]
^ permalink raw reply
* [PATCH] ethernet/intel: release the local packet buffer
From: Navid Emamdoost @ 2019-09-18 0:00 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Jeff Kirsher,
David S. Miller, intel-wired-lan, netdev, linux-kernel
In e100_loopback_test the buffer allocated for the local packet needs to
be released.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ethernet/intel/e100.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index a65d5a9ba7db..4de7dca341fc 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2394,6 +2394,7 @@ static int e100_loopback_test(struct nic *nic, enum loopback loopback_mode)
e100_hw_reset(nic);
err_clean_rx:
e100_rx_clean_list(nic);
+ dev_kfree_skb(skb);
return err;
}
--
2.17.1
^ permalink raw reply related
* Re: [patch iproute2-next v2] devlink: add reload failed indication
From: David Ahern @ 2019-09-17 23:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
In-Reply-To: <20190917183629.GP2286@nanopsycho.orion>
On 9/17/19 12:36 PM, Jiri Pirko wrote:
> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Add indication about previous failed devlink reload.
>>>
>>> Example outputs:
>>>
>>> $ devlink dev
>>> netdevsim/netdevsim10: reload_failed true
>>
>> odd output to user. Why not just "reload failed"?
>
> Well it is common to have "name value". The extra space would seem
> confusing for the reader..
> Also it is common to have "_" instead of space for the output in cases
> like this.
>
I am not understanding your point.
"reload failed" is still a name/value pair. It is short and to the point
as to what it indicates. There is no need for the name in the uapi (ie.,
the name of the netlink attribute) to be dumped here.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 09/14] samples: bpf: makefile: use own flags but not host when cross compile
From: Andrii Nakryiko @ 2019-09-17 23:42 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-10-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> While compile natively, the hosts cflags and ldflags are equal to ones
> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> have own, used for target arch. While verification, for arm, arm64 and
> x86_64 the following flags were used alsways:
>
> -Wall
> -O2
> -fomit-frame-pointer
> -Wmissing-prototypes
> -Wstrict-prototypes
>
> So, add them as they were verified and used before adding
> Makefile.target, but anyway limit it only for cross compile options as
> for host can be some configurations when another options can be used,
> So, for host arch samples left all as is, it allows to avoid potential
> option mistmatches for existent environments.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> samples/bpf/Makefile | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 1579cc16a1c2..b5c87a8b8b51 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
> TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> endif
>
> +ifdef CROSS_COMPILE
> +TPROGS_CFLAGS += -Wall
> +TPROGS_CFLAGS += -O2
Specifying one arg per line seems like overkill, put them in one line?
> +TPROGS_CFLAGS += -fomit-frame-pointer
Why this one?
> +TPROGS_CFLAGS += -Wmissing-prototypes
> +TPROGS_CFLAGS += -Wstrict-prototypes
Are these in some way special that we want them in cross-compile mode only?
All of those flags seem useful regardless of cross-compilation or not,
shouldn't they be common? I'm a bit lost about the intent here...
> +else
> TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
> TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> +endif
> +
> TPROGS_CFLAGS += -I$(objtree)/usr/include
> TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
> TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] ionic: Remove unnecessary ternary operator in ionic_debugfs_add_ident
From: Shannon Nelson @ 2019-09-17 23:40 UTC (permalink / raw)
To: Nathan Chancellor, Pensando Drivers, David S. Miller
Cc: netdev, linux-kernel, clang-built-linux, Greg Kroah-Hartman
In-Reply-To: <20190917232616.125261-1-natechancellor@gmail.com>
On 9/17/19 4:26 PM, Nathan Chancellor wrote:
> clang warns:
>
> ../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c:60:37: warning:
> expression result unused [-Wunused-value]
> ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
> ^~~~~~~~~~~
> 1 warning generated.
>
> The return value of debugfs_create_file does not need to be checked [1]
> and the function returns void so get rid of the ternary operator, it is
> unnecessary.
>
> [1]: https://lore.kernel.org/linux-mm/20150815160730.GB25186@kroah.com/
>
> Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
> Link: https://github.com/ClangBuiltLinux/linux/issues/658
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Acked-by: Shannon Nelson <snelson@pensando.io>
> ---
> drivers/net/ethernet/pensando/ionic/ionic_debugfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> index 7afc4a365b75..bc03cecf80cc 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> @@ -57,7 +57,7 @@ DEFINE_SHOW_ATTRIBUTE(identity);
> void ionic_debugfs_add_ident(struct ionic *ionic)
> {
> debugfs_create_file("identity", 0400, ionic->dentry,
> - ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
> + ionic, &identity_fops);
> }
>
> void ionic_debugfs_add_sizes(struct ionic *ionic)
^ permalink raw reply
* Re: [PATCH v3 bpf-next 08/14] samples: bpf: makefile: base target programs rules on Makefile.target
From: Andrii Nakryiko @ 2019-09-17 23:28 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-9-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 3:58 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
Please don't prepend "samples: bpf: makefile:" to patches,
"samples/bpf: " is a typical we've used for BPF samples changes.
> The main reason for that - HOSTCC and CC have different aims.
> HOSTCC is used to build programs running on host, that can
> cross-comple target programs with CC. It was tested for arm and arm64
> cross compilation, based on linaro toolchain, but should work for
> others.
>
> So, in order to split cross compilation (CC) with host build (HOSTCC),
> lets base samples on Makefile.target. It allows to cross-compile
> samples/bpf programs with CC while auxialry tools running on host
> built with HOSTCC.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> samples/bpf/Makefile | 135 ++++++++++++++++++++++---------------------
> 1 file changed, 69 insertions(+), 66 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 9d923546e087..1579cc16a1c2 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -4,55 +4,53 @@ BPF_SAMPLES_PATH ?= $(abspath $(srctree)/$(src))
> TOOLS_PATH := $(BPF_SAMPLES_PATH)/../../tools
>
> # List of programs to build
> -hostprogs-y := test_lru_dist
> -hostprogs-y += sock_example
> -hostprogs-y += fds_example
> -hostprogs-y += sockex1
> -hostprogs-y += sockex2
> -hostprogs-y += sockex3
> -hostprogs-y += tracex1
> -hostprogs-y += tracex2
> -hostprogs-y += tracex3
> -hostprogs-y += tracex4
> -hostprogs-y += tracex5
> -hostprogs-y += tracex6
> -hostprogs-y += tracex7
> -hostprogs-y += test_probe_write_user
> -hostprogs-y += trace_output
> -hostprogs-y += lathist
> -hostprogs-y += offwaketime
> -hostprogs-y += spintest
> -hostprogs-y += map_perf_test
> -hostprogs-y += test_overhead
> -hostprogs-y += test_cgrp2_array_pin
> -hostprogs-y += test_cgrp2_attach
> -hostprogs-y += test_cgrp2_sock
> -hostprogs-y += test_cgrp2_sock2
> -hostprogs-y += xdp1
> -hostprogs-y += xdp2
> -hostprogs-y += xdp_router_ipv4
> -hostprogs-y += test_current_task_under_cgroup
> -hostprogs-y += trace_event
> -hostprogs-y += sampleip
> -hostprogs-y += tc_l2_redirect
> -hostprogs-y += lwt_len_hist
> -hostprogs-y += xdp_tx_iptunnel
> -hostprogs-y += test_map_in_map
> -hostprogs-y += per_socket_stats_example
> -hostprogs-y += xdp_redirect
> -hostprogs-y += xdp_redirect_map
> -hostprogs-y += xdp_redirect_cpu
> -hostprogs-y += xdp_monitor
> -hostprogs-y += xdp_rxq_info
> -hostprogs-y += syscall_tp
> -hostprogs-y += cpustat
> -hostprogs-y += xdp_adjust_tail
> -hostprogs-y += xdpsock
> -hostprogs-y += xdp_fwd
> -hostprogs-y += task_fd_query
> -hostprogs-y += xdp_sample_pkts
> -hostprogs-y += ibumad
> -hostprogs-y += hbm
> +tprogs-y := test_lru_dist
> +tprogs-y += sock_example
> +tprogs-y += fds_example
> +tprogs-y += sockex1
> +tprogs-y += sockex2
> +tprogs-y += sockex3
> +tprogs-y += tracex1
> +tprogs-y += tracex2
> +tprogs-y += tracex3
> +tprogs-y += tracex4
> +tprogs-y += tracex5
> +tprogs-y += tracex6
> +tprogs-y += tracex7
> +tprogs-y += test_probe_write_user
> +tprogs-y += trace_output
> +tprogs-y += lathist
> +tprogs-y += offwaketime
> +tprogs-y += spintest
> +tprogs-y += map_perf_test
> +tprogs-y += test_overhead
> +tprogs-y += test_cgrp2_array_pin
> +tprogs-y += test_cgrp2_attach
> +tprogs-y += test_cgrp2_sock
> +tprogs-y += test_cgrp2_sock2
> +tprogs-y += xdp1
> +tprogs-y += xdp2
> +tprogs-y += xdp_router_ipv4
> +tprogs-y += test_current_task_under_cgroup
> +tprogs-y += trace_event
> +tprogs-y += sampleip
> +tprogs-y += tc_l2_redirect
> +tprogs-y += lwt_len_hist
> +tprogs-y += xdp_tx_iptunnel
> +tprogs-y += test_map_in_map
> +tprogs-y += xdp_redirect_map
> +tprogs-y += xdp_redirect_cpu
> +tprogs-y += xdp_monitor
> +tprogs-y += xdp_rxq_info
> +tprogs-y += syscall_tp
> +tprogs-y += cpustat
> +tprogs-y += xdp_adjust_tail
> +tprogs-y += xdpsock
> +tprogs-y += xdp_fwd
> +tprogs-y += task_fd_query
> +tprogs-y += xdp_sample_pkts
> +tprogs-y += ibumad
> +tprogs-y += hbm
>
> # Libbpf dependencies
> LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
> @@ -111,7 +109,7 @@ ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS)
> hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
>
> # Tell kbuild to always build the programs
> -always := $(hostprogs-y)
> +always := $(tprogs-y)
> always += sockex1_kern.o
> always += sockex2_kern.o
> always += sockex3_kern.o
> @@ -170,21 +168,6 @@ always += ibumad_kern.o
> always += hbm_out_kern.o
> always += hbm_edt_kern.o
>
> -KBUILD_HOSTCFLAGS += -I$(objtree)/usr/include
> -KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/bpf/
> -KBUILD_HOSTCFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> -KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/ -I$(srctree)/tools/include
> -KBUILD_HOSTCFLAGS += -I$(srctree)/tools/perf
> -
> -HOSTCFLAGS_bpf_load.o += -Wno-unused-variable
> -
> -KBUILD_HOSTLDLIBS += $(LIBBPF) -lelf
> -HOSTLDLIBS_tracex4 += -lrt
> -HOSTLDLIBS_trace_output += -lrt
> -HOSTLDLIBS_map_perf_test += -lrt
> -HOSTLDLIBS_test_overhead += -lrt
> -HOSTLDLIBS_xdpsock += -pthread
> -
> ifeq ($(ARCH), arm)
> # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
> # headers when arm instruction set identification is requested.
> @@ -192,9 +175,27 @@ ARM_ARCH_SELECTOR = $(shell echo "$(KBUILD_CFLAGS) " | \
> sed 's/[[:blank:]]/\n/g' | sed '/^-D__LINUX_ARM_ARCH__/!d')
>
> CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
> -KBUILD_HOSTCFLAGS := $(ARM_ARCH_SELECTOR)
> +TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> endif
>
> +TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
Please group TPROGS_LDLIBS definition together with the one below,
there doesn't seem to be a reason to split them this way.
But also, it's kind of weird to use host libraries as cross-compiled
libraries as well. Is that intentional?
> +TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
Same here, is it right to use HOSTCFLAGS and HOST_EXTRACFLAGS as a
base for cross-compiled cflags?
> +TPROGS_CFLAGS += -I$(objtree)/usr/include
> +TPROGS_CFLAGS += -I$(srctree)/tools/lib/bpf/
> +TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
> +TPROGS_CFLAGS += -I$(srctree)/tools/lib/
> +TPROGS_CFLAGS += -I$(srctree)/tools/include
> +TPROGS_CFLAGS += -I$(srctree)/tools/perf
> +
> +TPROGCFLAGS_bpf_load.o += -Wno-unused-variable
> +
> +TPROGS_LDLIBS += $(LIBBPF) -lelf
> +TPROGLDLIBS_tracex4 += -lrt
> +TPROGLDLIBS_trace_output += -lrt
> +TPROGLDLIBS_map_perf_test += -lrt
> +TPROGLDLIBS_test_overhead += -lrt
> +TPROGLDLIBS_xdpsock += -pthread
> +
> # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
> # make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
> LLC ?= llc
> @@ -285,6 +286,8 @@ $(obj)/hbm_out_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
> $(obj)/hbm.o: $(src)/hbm.h
> $(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
>
> +-include $(BPF_SAMPLES_PATH)/Makefile.target
> +
> # asm/sysreg.h - inline assembly used by it is incompatible with llvm.
> # But, there is no easy way to fix it, so just exclude it since it is
> # useless for BPF samples.
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH] ionic: Remove unnecessary ternary operator in ionic_debugfs_add_ident
From: Nathan Chancellor @ 2019-09-17 23:26 UTC (permalink / raw)
To: Shannon Nelson, Pensando Drivers, David S. Miller
Cc: netdev, linux-kernel, clang-built-linux, Greg Kroah-Hartman,
Nathan Chancellor
clang warns:
../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c:60:37: warning:
expression result unused [-Wunused-value]
ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
^~~~~~~~~~~
1 warning generated.
The return value of debugfs_create_file does not need to be checked [1]
and the function returns void so get rid of the ternary operator, it is
unnecessary.
[1]: https://lore.kernel.org/linux-mm/20150815160730.GB25186@kroah.com/
Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
Link: https://github.com/ClangBuiltLinux/linux/issues/658
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/ethernet/pensando/ionic/ionic_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
index 7afc4a365b75..bc03cecf80cc 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
@@ -57,7 +57,7 @@ DEFINE_SHOW_ATTRIBUTE(identity);
void ionic_debugfs_add_ident(struct ionic *ionic)
{
debugfs_create_file("identity", 0400, ionic->dentry,
- ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
+ ionic, &identity_fops);
}
void ionic_debugfs_add_sizes(struct ionic *ionic)
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v3 bpf-next 07/14] samples: bpf: add makefile.target for separate CC target build
From: Andrii Nakryiko @ 2019-09-17 23:19 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190916105433.11404-8-ivan.khoronzhuk@linaro.org>
On Mon, Sep 16, 2019 at 3:58 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> The makefile.target is added only and will be used in
typo: Makefile
> sample/bpf/Makefile later in order to switch cross-compiling on CC
on -> to
> from HOSTCC environment.
>
> The HOSTCC is supposed to build binaries and tools running on the host
> afterwards, in order to simplify build or so, like "fixdep" or else.
> In case of cross compiling "fixdep" is executed on host when the rest
> samples should run on target arch. In order to build binaries for
> target arch with CC and tools running on host with HOSTCC, lets add
> Makefile.target for simplicity, having definition and routines similar
> to ones, used in script/Makefile.host. This allows later add
> cross-compilation to samples/bpf with minimum changes.
>
> The tprog stands for target programs built with CC.
Why tprog? Could we just use prog: hostprog vs prog.
>
> Makefile.target contains only stuff needed for samples/bpf, potentially
> can be reused later and now needed only for unblocking tricky
> samples/bpf cross compilation.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> samples/bpf/Makefile.target | 75 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 75 insertions(+)
> create mode 100644 samples/bpf/Makefile.target
>
> diff --git a/samples/bpf/Makefile.target b/samples/bpf/Makefile.target
> new file mode 100644
> index 000000000000..fb6de63f7d2f
> --- /dev/null
> +++ b/samples/bpf/Makefile.target
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Building binaries on the host system
> +# Binaries are not used during the compilation of the kernel, and intendent
typo: intended
> +# to be build for target board, target board can be host ofc. Added to build
What's ofc, is it "of course"?
> +# binaries to run not on host system.
> +#
> +# Sample syntax (see Documentation/kbuild/makefiles.rst for reference)
> +# tprogs-y := xsk_example
> +# Will compile xdpsock_example.c and create an executable named xsk_example
You mix references to xsk_example and xdpsock_example, which is very
confusing. I'm guessing you meant to use xdpsock_example consistently.
> +#
> +# tprogs-y := xdpsock
> +# xdpsock-objs := xdpsock_1.o xdpsock_2.o
> +# Will compile xdpsock_1.c and xdpsock_2.c, and then link the executable
> +# xdpsock, based on xdpsock_1.o and xdpsock_2.o
> +#
> +# Inherited from scripts/Makefile.host
"Inspired by" or "Derived from" would be probably more appropriate term :)
> +#
> +__tprogs := $(sort $(tprogs-y))
> +
> +# C code
> +# Executables compiled from a single .c file
> +tprog-csingle := $(foreach m,$(__tprogs), \
> + $(if $($(m)-objs),,$(m)))
> +
> +# C executables linked based on several .o files
> +tprog-cmulti := $(foreach m,$(__tprogs),\
> + $(if $($(m)-objs),$(m)))
> +
> +# Object (.o) files compiled from .c files
> +tprog-cobjs := $(sort $(foreach m,$(__tprogs),$($(m)-objs)))
> +
> +tprog-csingle := $(addprefix $(obj)/,$(tprog-csingle))
> +tprog-cmulti := $(addprefix $(obj)/,$(tprog-cmulti))
> +tprog-cobjs := $(addprefix $(obj)/,$(tprog-cobjs))
> +
> +#####
> +# Handle options to gcc. Support building with separate output directory
> +
> +_tprogc_flags = $(TPROGS_CFLAGS) \
> + $(TPROGCFLAGS_$(basetarget).o)
> +
> +# $(objtree)/$(obj) for including generated headers from checkin source files
> +ifeq ($(KBUILD_EXTMOD),)
> +ifdef building_out_of_srctree
> +_tprogc_flags += -I $(objtree)/$(obj)
> +endif
> +endif
> +
> +tprogc_flags = -Wp,-MD,$(depfile) $(_tprogc_flags)
> +
> +# Create executable from a single .c file
> +# tprog-csingle -> Executable
> +quiet_cmd_tprog-csingle = CC $@
> + cmd_tprog-csingle = $(CC) $(tprogc_flags) $(TPROGS_LDFLAGS) -o $@ $< \
> + $(TPROGS_LDLIBS) $(TPROGLDLIBS_$(@F))
> +$(tprog-csingle): $(obj)/%: $(src)/%.c FORCE
> + $(call if_changed_dep,tprog-csingle)
> +
> +# Link an executable based on list of .o files, all plain c
> +# tprog-cmulti -> executable
> +quiet_cmd_tprog-cmulti = LD $@
> + cmd_tprog-cmulti = $(CC) $(tprogc_flags) $(TPROGS_LDFLAGS) -o $@ \
> + $(addprefix $(obj)/,$($(@F)-objs)) \
> + $(TPROGS_LDLIBS) $(TPROGLDLIBS_$(@F))
> +$(tprog-cmulti): $(tprog-cobjs) FORCE
> + $(call if_changed,tprog-cmulti)
> +$(call multi_depend, $(tprog-cmulti), , -objs)
> +
> +# Create .o file from a single .c file
> +# tprog-cobjs -> .o
> +quiet_cmd_tprog-cobjs = CC $@
> + cmd_tprog-cobjs = $(CC) $(tprogc_flags) -c -o $@ $<
> +$(tprog-cobjs): $(obj)/%.o: $(src)/%.c FORCE
> + $(call if_changed_dep,tprog-cobjs)
> --
> 2.17.1
>
tprogs is quite confusing, but overall looks good to me.
^ permalink raw reply
* Re: [PATCH v2 0/2] net/ibmvnic: serialization fixes
From: Juliet Kim @ 2019-09-17 23:19 UTC (permalink / raw)
To: netdev; +Cc: tlfalcon, linuxppc-dev
In-Reply-To: <20190917145249.15334-1-julietk@linux.vnet.ibm.com>
I sent v3. Please disregard v2.
On 9/17/19 9:52 AM, Juliet Kim wrote:
> This series includes two fixes. The first improves reset code to allow
> linkwatch_event to proceed during reset. The second ensures that no more
> than one thread runs in reset at a time.
>
> v2:
> - Separate change param reset from do_reset()
> - Return IBMVNIC_OPEN_FAILED if __ibmvnic_open fails
> - Remove setting wait_for_reset to false from __ibmvnic_reset(), this
> is done in wait_for_reset()
> - Move the check for force_reset_recovery from patch 1 to patch 2
>
> Juliet Kim (2):
> net/ibmvnic: unlock rtnl_lock in reset so linkwatch_event can run
> net/ibmvnic: prevent more than one thread from running in reset
>
> drivers/net/ethernet/ibm/ibmvnic.c | 244 ++++++++++++++++++++++++++-----------
> drivers/net/ethernet/ibm/ibmvnic.h | 4 +
> 2 files changed, 180 insertions(+), 68 deletions(-)
>
^ permalink raw reply
* Re: Bug report (with fix) for DEC Tulip driver (de2104x.c)
From: John David Anglin @ 2019-09-17 22:51 UTC (permalink / raw)
To: Arlie Davis, Andrew Lunn; +Cc: netdev, linux-parisc
In-Reply-To: <CAK-9enOx8xt_+t6-rpCGEL0j-HJGm=sFXYq9-pgHQ26AwrGm5Q@mail.gmail.com>
On 2019-09-17 5:36 p.m., Arlie Davis wrote:
> Likewise, I'm at a loss for testing with real hardware. It's hard to
> find such things, now.
How does de2104x compare to ds2142/43? I have a c3750 with ds2142/43 tulip. Helge
or some others might have a machine with a de2104x.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply
* [PATCH] ieee802154: ca8210: prevent memory leak
From: Navid Emamdoost @ 2019-09-17 22:47 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Harry Morris,
Alexander Aring, Stefan Schmidt, David S. Miller, linux-wpan,
netdev, linux-kernel
In ca8210_probe the allocated pdata needs to be assigned to
spi_device->dev.platform_data before calling ca8210_get_platform_data.
Othrwise when ca8210_get_platform_data fails pdata cannot be released.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ieee802154/ca8210.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index b188fce3f641..229d70a897ca 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -3152,12 +3152,12 @@ static int ca8210_probe(struct spi_device *spi_device)
goto error;
}
+ priv->spi->dev.platform_data = pdata;
ret = ca8210_get_platform_data(priv->spi, pdata);
if (ret) {
dev_crit(&spi_device->dev, "ca8210_get_platform_data failed\n");
goto error;
}
- priv->spi->dev.platform_data = pdata;
ret = ca8210_dev_com_init(priv);
if (ret) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring
From: Florian Fainelli @ 2019-09-17 22:42 UTC (permalink / raw)
To: Iwan R Timmer, Andrew Lunn; +Cc: Vivien Didelot, David S. Miller, netdev
In-Reply-To: <20190917223232.GA32887@i5wan>
On 9/17/19 3:32 PM, Iwan R Timmer wrote:
> On Tue, Sep 17, 2019 at 10:55:05PM +0200, Andrew Lunn wrote:
>> On Tue, Sep 17, 2019 at 10:23:01PM +0200, Iwan R Timmer wrote:
>>> Add support for configuring port mirroring through the cls_matchall
>>> classifier. We do a full ingress and/or egress capture towards the
>>> capture port, configured with set_egress_port.
>>
>> Hi Iwan
>>
>> This looks good as far as it goes.
>>
>> Have you tried adding/deleting multiple port mirrors? Do we need to
>> limit how many are added. A quick look at the datasheet, you can
>> define one egress mirror port and one ingress mirror port. I think you
>> can have multiple ports mirroring ingress to that one ingress mirror
>> port. And you can have multiple port mirroring egress to the one
>> egress mirror port. We should add code to check this, and return
>> -EBUSY if the existing configuration prevents a new mirror being
>> configured.
>>
>> Thanks
>> Andrew
>
> Hi Andrew,
>
> I only own a simple 5 ports switch (88E6176) which has no problem of
> mirroring the other ports to a single port. Except for a bandwith
> shortage ofcourse. While I thought I checked adding and removing ports,
> I seemed to forgot to check removing ingress traffic as it will now
> disable mirroring egress traffic. Searching for how I can distinct
> ingress from egress mirroring in port_mirror_del, I saw there is a
> variable in the mirror struct called ingress. Which seems strange,
> because why is it a seperate argument to the port_mirror_add function?
>
> Origally I planned to be able to set the egress and ingress mirror
> seperatly. But in my laziness when I saw there already was a function
> to configure the destination port this functionality was lost.
>
> Because the other drivers which implemented the port_mirror_add (b53 and
> ksz9477) also lacks additional checks to prevent new mirror filters from
> breaking previous ones I assumed they were not necessary.
That does sound like a bug indeed, I just looked at b53_mirror_add()
again and clearing the MIRROR_MASK before setting up the new port
clearly sounds wrong, at least on ingress.
>
> At least I will soon sent a new version with at least the issue of
> removing mirror ingress traffic fixed and the ability to define a
> seperate ingress and egress port.
>
> Regards,
> Iwan
>
--
Florian
^ 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