* Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors
From: David Miller @ 2019-07-29 21:25 UTC (permalink / raw)
To: jakub.kicinski; +Cc: jonathan.lemon, kernel-team, netdev, willy
In-Reply-To: <20190729142211.43b5ccd8@cakuba.netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 29 Jul 2019 14:22:11 -0700
> There is no need to ponder the connotations of verbs. Please just
> look at other function names in skbuff.h, especially those which
> copy fields :)
>
> static inline void skb_copy_hash(struct sk_buff *to, const struct sk_buff *from)
> static inline void skb_copy_secmark(struct sk_buff *to, const struct sk_buff *from)
> static inline void skb_copy_queue_mapping(struct sk_buff *to, const struct sk_buff *from)
> static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *src)
I have to agree :-)
^ permalink raw reply
* Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors
From: Jakub Kicinski @ 2019-07-29 21:25 UTC (permalink / raw)
To: Jonathan Lemon; +Cc: davem, kernel-team, netdev, Matthew Wilcox
In-Reply-To: <20190729142211.43b5ccd8@cakuba.netronome.com>
On Mon, 29 Jul 2019 14:22:11 -0700, Jakub Kicinski wrote:
> > > I realize you're following the existing code, but should we perhaps
> > > use
> > > the latest kdoc syntax? '()' after function name, and args should have
> > > '@' prefix, '%' would be for constants.
> >
> > That would be a task for a different cleanup. Not that I disagree with
> > you, but there's also nothing worse than mixing styles in the same file.
>
> Funny you should say that given that (a) I'm commenting on the new code
> you're adding, and (b) you did do an unrelated spelling fix above ;)
Ah, sorry I misread your comment there.
Some code already uses '()' in this file, as for the '%' skb_frag_
functions are the only one which have this mistake, the rest of kdoc
is correct.
^ permalink raw reply
* Re: [PATCH net] net/mlx5e: Fix unnecessary flow_block_cb_is_busy call
From: Saeed Mahameed @ 2019-07-29 21:30 UTC (permalink / raw)
To: wenxu@ucloud.cn, netdev@vger.kernel.org
In-Reply-To: <1564239595-23786-1-git-send-email-wenxu@ucloud.cn>
On Sat, 2019-07-27 at 22:59 +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> When call flow_block_cb_is_busy. The indr_priv is guaranteed to
> NULL ptr. So there is no need to call flow_bock_cb_is_busy.
>
> Fixes: 0d4fd02e7199 ("net: flow_offload: add flow_block_cb_is_busy()
> and use it")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
Applied to net-next-mlx5 branch.
Thanks,
Saeed.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jon Hunter @ 2019-07-29 21:33 UTC (permalink / raw)
To: Jose Abreu, Robin Murphy, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Catalin Marinas,
Will Deacon
Cc: Joao Pinto, Alexandre Torgue, Maxime Ripard, Chen-Yu Tsai,
Maxime Coquelin, linux-tegra, Giuseppe Cavallaro,
David S . Miller
In-Reply-To: <MN2PR12MB3279ABF628C52883021123C5D3DD0@MN2PR12MB3279.namprd12.prod.outlook.com>
On 29/07/2019 15:08, Jose Abreu wrote:
...
>>> Hi Catalin and Will,
>>>
>>> Sorry to add you in such a long thread but we are seeing a DMA issue
>>> with stmmac driver in an ARM64 platform with IOMMU enabled.
>>>
>>> The issue seems to be solved when buffers allocation for DMA based
>>> transfers are *not* mapped with the DMA_ATTR_SKIP_CPU_SYNC flag *OR*
>>> when IOMMU is disabled.
>>>
>>> Notice that after transfer is done we do use
>>> dma_sync_single_for_{cpu,device} and then we reuse *the same* page for
>>> another transfer.
>>>
>>> Can you please comment on whether DMA_ATTR_SKIP_CPU_SYNC can not be used
>>> in ARM64 platforms with IOMMU ?
>>
>> In terms of what they do, there should be no difference on arm64 between:
>>
>> dma_map_page(..., dir);
>> ...
>> dma_unmap_page(..., dir);
>>
>> and:
>>
>> dma_map_page_attrs(..., dir, DMA_ATTR_SKIP_CPU_SYNC);
>> dma_sync_single_for_device(..., dir);
>> ...
>> dma_sync_single_for_cpu(..., dir);
>> dma_unmap_page_attrs(..., dir, DMA_ATTR_SKIP_CPU_SYNC);
>>
>> provided that the first sync covers the whole buffer and any subsequent
>> ones cover at least the parts of the buffer which may have changed. Plus
>> for coherent hardware it's entirely moot either way.
>
> Thanks for confirming. That's indeed what stmmac is doing when buffer is
> received by syncing the packet size to CPU.
>
>>
>> Given Jon's previous findings, I would lean towards the idea that
>> performing the extra (redundant) cache maintenance plus barrier in
>> dma_unmap is mostly just perturbing timing in the same way as the debug
>> print which also made things seem OK.
>
> Mikko said that Tegra186 is not coherent so we have to explicit flush
> pipeline but I don't understand why sync_single() is not doing it ...
>
> Jon, can you please remove *all* debug prints, hacks, etc ... and test
> this one in attach with plain -net tree ?
So far I have just been testing on the mainline kernel branch. The issue
still persists after applying this on mainline. I can test on the -net
tree, but I am not sure that will make a difference.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* [PATCH bpf-next] tools: bpftool: add support for reporting the effective cgroup progs
From: Jakub Kicinski @ 2019-07-29 21:35 UTC (permalink / raw)
To: alexei.starovoitov, daniel
Cc: netdev, bpf, oss-drivers, ctakshak, kernel-team, Jakub Kicinski,
Quentin Monnet
Takshak said in the original submission:
With different bpf attach_flags available to attach bpf programs specially
with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
bpf-programs available to any sub-cgroups really needs to be available for
easy debugging.
Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
bpf-programs to a cgroup but also the inherited ones from parent cgroup.
So a new option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here
to list all the effective bpf-programs available for execution at a specified
cgroup.
Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
# ./test_cgroup_attach
With old bpftool:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
ID AttachType AttachFlags Name
271 egress multi pkt_cntr_1
272 egress multi pkt_cntr_2
Attached new program pkt_cntr_4 in cg2 gives following:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
ID AttachType AttachFlags Name
273 egress override pkt_cntr_4
And with new "effective" option it shows all effective programs for cg2:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 effective
ID AttachType AttachFlags Name
273 egress override pkt_cntr_4
271 egress override pkt_cntr_1
272 egress override pkt_cntr_2
Compared to original submission use a local flag instead of global
option.
We need to clear query_flags on every command, in case batch mode
wants to use varying settings.
Signed-off-by: Takshak Chahande <ctakshak@fb.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
.../bpftool/Documentation/bpftool-cgroup.rst | 16 +++--
tools/bpf/bpftool/bash-completion/bpftool | 15 +++--
tools/bpf/bpftool/cgroup.c | 65 ++++++++++++-------
3 files changed, 63 insertions(+), 33 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 585f270c2d25..06a28b07787d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -20,8 +20,8 @@ SYNOPSIS
CGROUP COMMANDS
===============
-| **bpftool** **cgroup { show | list }** *CGROUP*
-| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
+| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
+| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
| **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
| **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
| **bpftool** **cgroup help**
@@ -35,13 +35,17 @@ CGROUP COMMANDS
DESCRIPTION
===========
- **bpftool cgroup { show | list }** *CGROUP*
+ **bpftool cgroup { show | list }** *CGROUP* [**effective**]
List all programs attached to the cgroup *CGROUP*.
Output will start with program ID followed by attach type,
attach flags and program name.
- **bpftool cgroup tree** [*CGROUP_ROOT*]
+ If **effective** is specified retrieve effective programs that
+ will execute for events within a cgroup. This includes
+ inherited along with attached ones.
+
+ **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
Iterate over all cgroups in *CGROUP_ROOT* and list all
attached programs. If *CGROUP_ROOT* is not specified,
bpftool uses cgroup v2 mountpoint.
@@ -50,6 +54,10 @@ DESCRIPTION
commands: it starts with absolute cgroup path, followed by
program ID, attach type, attach flags and program name.
+ If **effective** is specified retrieve effective programs that
+ will execute for events within a cgroup. This includes
+ inherited along with attached ones.
+
**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
Attach program *PROG* to the cgroup *CGROUP* with attach type
*ATTACH_TYPE* and optional *ATTACH_FLAGS*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 6b961a5ed100..df16c5415444 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -710,12 +710,15 @@ _bpftool()
;;
cgroup)
case $command in
- show|list)
- _filedir
- return 0
- ;;
- tree)
- _filedir
+ show|list|tree)
+ case $cword in
+ 3)
+ _filedir
+ ;;
+ 4)
+ COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
+ ;;
+ esac
return 0
;;
attach|detach)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index f3c05b08c68c..339c2c78b8e4 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -29,6 +29,8 @@
" recvmsg4 | recvmsg6 | sysctl |\n" \
" getsockopt | setsockopt }"
+static unsigned int query_flags;
+
static const char * const attach_type_strings[] = {
[BPF_CGROUP_INET_INGRESS] = "ingress",
[BPF_CGROUP_INET_EGRESS] = "egress",
@@ -107,7 +109,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
__u32 prog_cnt = 0;
int ret;
- ret = bpf_prog_query(cgroup_fd, type, 0, NULL, NULL, &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
+ NULL, &prog_cnt);
if (ret)
return -1;
@@ -125,8 +128,8 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
int ret;
prog_cnt = ARRAY_SIZE(prog_ids);
- ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
- &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, &attach_flags,
+ prog_ids, &prog_cnt);
if (ret)
return ret;
@@ -158,20 +161,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
static int do_show(int argc, char **argv)
{
enum bpf_attach_type type;
+ const char *path;
int cgroup_fd;
int ret = -1;
- if (argc < 1) {
- p_err("too few parameters for cgroup show");
- goto exit;
- } else if (argc > 1) {
- p_err("too many parameters for cgroup show");
- goto exit;
+ query_flags = 0;
+
+ if (!REQ_ARGS(1))
+ return -1;
+ path = GET_ARG();
+
+ while (argc) {
+ if (is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'effective', got: '%s'?",
+ *argv);
+ return -1;
+ }
}
- cgroup_fd = open(argv[0], O_RDONLY);
+ cgroup_fd = open(path, O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[0]);
+ p_err("can't open cgroup %s", path);
goto exit;
}
@@ -297,23 +310,29 @@ static int do_show_tree(int argc, char **argv)
char *cgroup_root;
int ret;
- switch (argc) {
- case 0:
+ query_flags = 0;
+
+ if (!argc) {
cgroup_root = find_cgroup_root();
if (!cgroup_root) {
p_err("cgroup v2 isn't mounted");
return -1;
}
- break;
- case 1:
- cgroup_root = argv[0];
- break;
- default:
- p_err("too many parameters for cgroup tree");
- return -1;
+ } else {
+ cgroup_root = GET_ARG();
+
+ while (argc) {
+ if (is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'effective', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
}
-
if (json_output)
jsonw_start_array(json_wtr);
else
@@ -459,8 +478,8 @@ static int do_help(int argc, char **argv)
}
fprintf(stderr,
- "Usage: %s %s { show | list } CGROUP\n"
- " %s %s tree [CGROUP_ROOT]\n"
+ "Usage: %s %s { show | list } CGROUP [**effective**]\n"
+ " %s %s tree [CGROUP_ROOT] [**effective**]\n"
" %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n"
" %s %s detach CGROUP ATTACH_TYPE PROG\n"
" %s %s help\n"
--
2.21.0
^ permalink raw reply related
* [net-next:master 57/59] drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size
From: kbuild test robot @ 2019-07-29 21:36 UTC (permalink / raw)
To: Matthew Wilcox (Oracle); +Cc: kbuild-all, netdev
[-- Attachment #1: Type: text/plain, Size: 8294 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next.git master
head: 1cb9dfca39eb406960f8f84864ddd6ba329ec321
commit: 171a9bae68c72f2d1260c3825203760856e6793b [57/59] staging/octeon: Allow test build on !MIPS
config: sh-allmodconfig (attached as .config)
compiler: sh4-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
git checkout 171a9bae68c72f2d1260c3825203760856e6793b
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/staging/octeon/octeon-ethernet.h:41:0,
from drivers/staging/octeon/ethernet.c:22:
drivers/staging/octeon/octeon-stubs.h: In function 'cvmx_phys_to_ptr':
>> drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
return (void *)(physical_address);
^
--
In file included from drivers/staging/octeon/octeon-ethernet.h:41:0,
from drivers/staging/octeon/ethernet-mem.c:12:
drivers/staging/octeon/octeon-stubs.h: In function 'cvmx_phys_to_ptr':
>> drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
return (void *)(physical_address);
^
In file included from include/linux/scatterlist.h:9:0,
from include/linux/dma-mapping.h:11,
from include/linux/skbuff.h:31,
from include/linux/if_ether.h:19,
from include/uapi/linux/ethtool.h:19,
from include/linux/ethtool.h:18,
from include/linux/netdevice.h:37,
from drivers/staging/octeon/ethernet-mem.c:9:
drivers/staging/octeon/ethernet-mem.c: In function 'cvm_oct_free_hw_memory':
arch/sh/include/asm/io.h:244:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define phys_to_virt(address) ((void *)(address))
^
>> drivers/staging/octeon/ethernet-mem.c:123:18: note: in expansion of macro 'phys_to_virt'
fpa = (char *)phys_to_virt(cvmx_ptr_to_phys(fpa));
^~~~~~~~~~~~
--
In file included from drivers/staging/octeon/octeon-ethernet.h:41:0,
from drivers/staging/octeon/ethernet-tx.c:25:
drivers/staging/octeon/octeon-stubs.h: In function 'cvmx_phys_to_ptr':
>> drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
return (void *)(physical_address);
^
drivers/staging/octeon/ethernet-tx.c: In function 'cvm_oct_xmit':
>> drivers/staging/octeon/ethernet-tx.c:264:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro 'XKPHYS_TO_PHYS'
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:268:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro 'XKPHYS_TO_PHYS'
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:276:20: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
XKPHYS_TO_PHYS((u64)skb_frag_address(fs));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro 'XKPHYS_TO_PHYS'
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:280:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)CVM_OCT_SKB_CB(skb));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro 'XKPHYS_TO_PHYS'
#define XKPHYS_TO_PHYS(p) (p)
^
--
drivers/net/phy/mdio-octeon.c: In function 'octeon_mdiobus_probe':
>> drivers/net/phy/mdio-octeon.c:48:3: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(u64)devm_ioremap(&pdev->dev, mdio_phys, regsize);
^
In file included from include/linux/io.h:13:0,
from include/linux/of_address.h:7,
from drivers/net/phy/mdio-octeon.c:7:
>> drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
arch/sh/include/asm/io.h:33:71: note: in definition of macro '__raw_writeq'
#define __raw_writeq(v,a) (__chk_io_ptr(a), *(volatile u64 __force *)(a) = (v))
^
>> arch/sh/include/asm/io.h:58:32: note: in expansion of macro 'writeq_relaxed'
#define writeq(v,a) ({ wmb(); writeq_relaxed((v),(a)); })
^~~~~~~~~~~~~~
>> drivers/net/phy/mdio-cavium.h:111:36: note: in expansion of macro 'writeq'
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^~~~~~
>> drivers/net/phy/mdio-octeon.c:56:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
>> drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
arch/sh/include/asm/io.h:33:71: note: in definition of macro '__raw_writeq'
#define __raw_writeq(v,a) (__chk_io_ptr(a), *(volatile u64 __force *)(a) = (v))
^
>> arch/sh/include/asm/io.h:58:32: note: in expansion of macro 'writeq_relaxed'
#define writeq(v,a) ({ wmb(); writeq_relaxed((v),(a)); })
^~~~~~~~~~~~~~
>> drivers/net/phy/mdio-cavium.h:111:36: note: in expansion of macro 'writeq'
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^~~~~~
drivers/net/phy/mdio-octeon.c:77:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
drivers/net/phy/mdio-octeon.c: In function 'octeon_mdiobus_remove':
>> drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
arch/sh/include/asm/io.h:33:71: note: in definition of macro '__raw_writeq'
#define __raw_writeq(v,a) (__chk_io_ptr(a), *(volatile u64 __force *)(a) = (v))
^
>> arch/sh/include/asm/io.h:58:32: note: in expansion of macro 'writeq_relaxed'
#define writeq(v,a) ({ wmb(); writeq_relaxed((v),(a)); })
^~~~~~~~~~~~~~
>> drivers/net/phy/mdio-cavium.h:111:36: note: in expansion of macro 'writeq'
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^~~~~~
drivers/net/phy/mdio-octeon.c:91:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
vim +1205 drivers/staging/octeon/octeon-stubs.h
---
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: 51905 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors
From: Jonathan Lemon @ 2019-07-29 21:37 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, kernel-team, netdev, Matthew Wilcox
In-Reply-To: <20190729142211.43b5ccd8@cakuba.netronome.com>
On 29 Jul 2019, at 14:22, Jakub Kicinski wrote:
> On Mon, 29 Jul 2019 14:02:21 -0700, Jonathan Lemon wrote:
>> On 29 Jul 2019, at 13:50, Jakub Kicinski wrote:
>>> On Mon, 29 Jul 2019 10:19:39 -0700, Jonathan Lemon wrote:
>>>> Add skb_frag_off(), skb_frag_off_add(), skb_frag_off_set(),
>>>> and skb_frag_off_set_from() accessors for page_offset.
>>>>
>>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index 718742b1c505..7d94a78067ee 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -331,7 +331,7 @@ static inline void skb_frag_size_set(skb_frag_t
>>>> *frag, unsigned int size)
>>>> }
>>>>
>>>> /**
>>>> - * skb_frag_size_add - Incrementes the size of a skb fragment by
>>>> %delta
>>>> + * skb_frag_size_add - Increments the size of a skb fragment by
>>>> %delta
>>>> * @frag: skb fragment
>>>> * @delta: value to add
>>>> */
>>>> @@ -2857,6 +2857,46 @@ static inline void
>>>> skb_propagate_pfmemalloc(struct page *page,
>>>> skb->pfmemalloc = true;
>>>> }
>>>>
>>>> +/**
>>>> + * skb_frag_off - Returns the offset of a skb fragment
>>>> + * @frag: the paged fragment
>>>> + */
>>>> +static inline unsigned int skb_frag_off(const skb_frag_t *frag)
>>>> +{
>>>> + return frag->page_offset;
>>>> +}
>>>> +
>>>> +/**
>>>> + * skb_frag_off_add - Increments the offset of a skb fragment by
>>>> %delta
>>>
>>> I realize you're following the existing code, but should we perhaps
>>> use
>>> the latest kdoc syntax? '()' after function name, and args should
>>> have
>>> '@' prefix, '%' would be for constants.
>>
>> That would be a task for a different cleanup. Not that I disagree
>> with
>> you, but there's also nothing worse than mixing styles in the same
>> file.
>
> Funny you should say that given that (a) I'm commenting on the new
> code
> you're adding, and (b) you did do an unrelated spelling fix above ;)
>
>>>> + * @frag: skb fragment
>>>> + * @delta: value to add
>>>> + */
>>>> +static inline void skb_frag_off_add(skb_frag_t *frag, int delta)
>>>> +{
>>>> + frag->page_offset += delta;
>>>> +}
>>>> +
>>>> +/**
>>>> + * skb_frag_off_set - Sets the offset of a skb fragment
>>>> + * @frag: skb fragment
>>>> + * @offset: offset of fragment
>>>> + */
>>>> +static inline void skb_frag_off_set(skb_frag_t *frag, unsigned int
>>>> offset)
>>>> +{
>>>> + frag->page_offset = offset;
>>>> +}
>>>> +
>>>> +/**
>>>> + * skb_frag_off_set_from - Sets the offset of a skb fragment from
>>>> another fragment
>>>> + * @fragto: skb fragment where offset is set
>>>> + * @fragfrom: skb fragment offset is copied from
>>>> + */
>>>> +static inline void skb_frag_off_set_from(skb_frag_t *fragto,
>>>> + const skb_frag_t *fragfrom)
>>>
>>> skb_frag_off_copy() ?
>>
>> That was my initial inclination, but due to the often overloaded
>> connotations of the word "copy", opted to use the same "set" verbiage
>> that existed in the other functions.
>
> There is no need to ponder the connotations of verbs. Please just
> look at other function names in skbuff.h, especially those which
> copy fields :)
>
> static inline void skb_copy_hash(struct sk_buff *to, const struct
> sk_buff *from)
> static inline void skb_copy_secmark(struct sk_buff *to, const struct
> sk_buff *from)
> static inline void skb_copy_queue_mapping(struct sk_buff *to, const
> struct sk_buff *from)
> static inline void skb_ext_copy(struct sk_buff *dst, const struct
> sk_buff *src)
Okay, I missed those, let me respin.
--
Jonathan
^ permalink raw reply
* Re: [PATCH V36 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-07-29 21:47 UTC (permalink / raw)
To: James Morris
Cc: LSM List, Linux Kernel Mailing List, Linux API, David Howells,
Alexei Starovoitov, Network Development, Chun-Yi Lee,
Daniel Borkmann
In-Reply-To: <20190718194415.108476-24-matthewgarrett@google.com>
On Thu, Jul 18, 2019 at 12:45 PM Matthew Garrett
<matthewgarrett@google.com> wrote:
> bpf_read() and bpf_read_str() could potentially be abused to (eg) allow
> private keys in kernel memory to be leaked. Disable them if the kernel
> has been locked down in confidentiality mode.
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: netdev@vger.kernel.org
> cc: Chun-Yi Lee <jlee@suse.com>
> cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
Any further feedback on this?
^ permalink raw reply
* [net-next:master 57/59] drivers/net/phy/mdio-cavium.h:111:36: error: implicit declaration of function 'writeq'; did you mean 'writel'?
From: kbuild test robot @ 2019-07-29 21:45 UTC (permalink / raw)
To: Matthew Wilcox (Oracle); +Cc: kbuild-all, netdev
[-- Attachment #1: Type: text/plain, Size: 4206 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next.git master
head: 1cb9dfca39eb406960f8f84864ddd6ba329ec321
commit: 171a9bae68c72f2d1260c3825203760856e6793b [57/59] staging/octeon: Allow test build on !MIPS
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-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
git checkout 171a9bae68c72f2d1260c3825203760856e6793b
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/phy/mdio-octeon.c: In function 'octeon_mdiobus_probe':
drivers/net/phy/mdio-octeon.c:48:3: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(u64)devm_ioremap(&pdev->dev, mdio_phys, regsize);
^
In file included from drivers/net/phy/mdio-octeon.c:14:0:
>> drivers/net/phy/mdio-cavium.h:111:36: error: implicit declaration of function 'writeq'; did you mean 'writel'? [-Werror=implicit-function-declaration]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
drivers/net/phy/mdio-octeon.c:56:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
drivers/net/phy/mdio-octeon.c:56:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
drivers/net/phy/mdio-octeon.c:77:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
drivers/net/phy/mdio-octeon.c: In function 'octeon_mdiobus_remove':
drivers/net/phy/mdio-cavium.h:111:48: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
#define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
^
drivers/net/phy/mdio-octeon.c:91:2: note: in expansion of macro 'oct_mdio_writeq'
oct_mdio_writeq(smi_en.u64, bus->register_base + SMI_EN);
^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +111 drivers/net/phy/mdio-cavium.h
1eefee901fca020 David Daney 2016-03-11 105
1eefee901fca020 David Daney 2016-03-11 106 static inline u64 oct_mdio_readq(u64 addr)
1eefee901fca020 David Daney 2016-03-11 107 {
1eefee901fca020 David Daney 2016-03-11 108 return cvmx_read_csr(addr);
1eefee901fca020 David Daney 2016-03-11 109 }
1eefee901fca020 David Daney 2016-03-11 110 #else
1eefee901fca020 David Daney 2016-03-11 @111 #define oct_mdio_writeq(val, addr) writeq(val, (void *)addr)
1eefee901fca020 David Daney 2016-03-11 112 #define oct_mdio_readq(addr) readq((void *)addr)
1eefee901fca020 David Daney 2016-03-11 113 #endif
1eefee901fca020 David Daney 2016-03-11 114
1eefee901fca020 David Daney 2016-03-11 115 int cavium_mdiobus_read(struct mii_bus *bus, int phy_id, int regnum);
1eefee901fca020 David Daney 2016-03-11 116 int cavium_mdiobus_write(struct mii_bus *bus, int phy_id, int regnum, u16 val);
:::::: The code at line 111 was first introduced by commit
:::::: 1eefee901fca0208b8a56f20cdc134e2b8638ae7 phy: mdio-octeon: Refactor into two files/modules
:::::: TO: David Daney <david.daney@cavium.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: 49823 bytes --]
^ permalink raw reply
* [PATCH bpf-next 0/2] bpf: allocate extra memory for setsockopt hook buffer
From: Stanislav Fomichev @ 2019-07-29 21:51 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Current setsockopt hook is limited to the size of the buffer that
user had supplied. Since we always allocate memory and copy the value
into kernel space, allocate just a little bit more in case BPF
program needs to override input data with a larger value.
The canonical example is TCP_CONGESTION socket option where
input buffer is a string and if user calls it with a short string,
BPF program has no way of extending it.
The tests are extended with TCP_CONGESTION use case.
Stanislav Fomichev (2):
bpf: always allocate at least 16 bytes for setsockopt hook
selftests/bpf: extend sockopt_sk selftest with TCP_CONGESTION use case
kernel/bpf/cgroup.c | 17 ++++++++++---
.../testing/selftests/bpf/progs/sockopt_sk.c | 22 ++++++++++++++++
tools/testing/selftests/bpf/test_sockopt_sk.c | 25 +++++++++++++++++++
3 files changed, 60 insertions(+), 4 deletions(-)
--
2.22.0.709.g102302147b-goog
^ permalink raw reply
* [PATCH bpf-next 1/2] bpf: always allocate at least 16 bytes for setsockopt hook
From: Stanislav Fomichev @ 2019-07-29 21:51 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190729215111.209219-1-sdf@google.com>
Since we always allocate memory, allocate just a little bit more
for the BPF program in case it need to override user input with
bigger value. The canonical example is TCP_CONGESTION where
input string might be too small to override (nv -> bbr or cubic).
16 bytes are chosen to match the size of TCP_CA_NAME_MAX and can
be extended in the future if needed.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/cgroup.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 0a00eaca6fae..6a6a154cfa7b 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -964,7 +964,6 @@ static int sockopt_alloc_buf(struct bpf_sockopt_kern *ctx, int max_optlen)
return -ENOMEM;
ctx->optval_end = ctx->optval + max_optlen;
- ctx->optlen = max_optlen;
return 0;
}
@@ -984,7 +983,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
.level = *level,
.optname = *optname,
};
- int ret;
+ int ret, max_optlen;
/* Opportunistic check to see whether we have any BPF program
* attached to the hook so we don't waste time allocating
@@ -994,10 +993,18 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
__cgroup_bpf_prog_array_is_empty(cgrp, BPF_CGROUP_SETSOCKOPT))
return 0;
- ret = sockopt_alloc_buf(&ctx, *optlen);
+ /* Allocate a bit more than the initial user buffer for
+ * BPF program. The canonical use case is overriding
+ * TCP_CONGESTION(nv) to TCP_CONGESTION(cubic).
+ */
+ max_optlen = max_t(int, 16, *optlen);
+
+ ret = sockopt_alloc_buf(&ctx, max_optlen);
if (ret)
return ret;
+ ctx.optlen = *optlen;
+
if (copy_from_user(ctx.optval, optval, *optlen) != 0) {
ret = -EFAULT;
goto out;
@@ -1016,7 +1023,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
if (ctx.optlen == -1) {
/* optlen set to -1, bypass kernel */
ret = 1;
- } else if (ctx.optlen > *optlen || ctx.optlen < -1) {
+ } else if (ctx.optlen > max_optlen || ctx.optlen < -1) {
/* optlen is out of bounds */
ret = -EFAULT;
} else {
@@ -1063,6 +1070,8 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
if (ret)
return ret;
+ ctx.optlen = max_optlen;
+
if (!retval) {
/* If kernel getsockopt finished successfully,
* copy whatever was returned to the user back
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* [PATCH bpf-next 2/2] selftests/bpf: extend sockopt_sk selftest with TCP_CONGESTION use case
From: Stanislav Fomichev @ 2019-07-29 21:51 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190729215111.209219-1-sdf@google.com>
Ignore SOL_TCP:TCP_CONGESTION in getsockopt and always override
SOL_TCP:TCP_CONGESTION with "cubic" in setsockopt hook.
Call setsockopt(SOL_TCP, TCP_CONGESTION) with short optval ("nv")
to make sure BPF program has enough buffer space to replace it
with "cubic".
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../testing/selftests/bpf/progs/sockopt_sk.c | 22 ++++++++++++++++
tools/testing/selftests/bpf/test_sockopt_sk.c | 25 +++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index 076122c898e9..9a3d1c79e6fe 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
#include <netinet/in.h>
+#include <netinet/tcp.h>
#include <linux/bpf.h>
#include "bpf_helpers.h"
@@ -42,6 +44,14 @@ int _getsockopt(struct bpf_sockopt *ctx)
return 1;
}
+ if (ctx->level == SOL_TCP && ctx->optname == TCP_CONGESTION) {
+ /* Not interested in SOL_TCP:TCP_CONGESTION;
+ * let next BPF program in the cgroup chain or kernel
+ * handle it.
+ */
+ return 1;
+ }
+
if (ctx->level != SOL_CUSTOM)
return 0; /* EPERM, deny everything except custom level */
@@ -91,6 +101,18 @@ int _setsockopt(struct bpf_sockopt *ctx)
return 1;
}
+ if (ctx->level == SOL_TCP && ctx->optname == TCP_CONGESTION) {
+ /* Always use cubic */
+
+ if (optval + 5 > optval_end)
+ return 0; /* EPERM, bounds check */
+
+ memcpy(optval, "cubic", 5);
+ ctx->optlen = 5;
+
+ return 1;
+ }
+
if (ctx->level != SOL_CUSTOM)
return 0; /* EPERM, deny everything except custom level */
diff --git a/tools/testing/selftests/bpf/test_sockopt_sk.c b/tools/testing/selftests/bpf/test_sockopt_sk.c
index 036b652e5ca9..e4f6055d92e9 100644
--- a/tools/testing/selftests/bpf/test_sockopt_sk.c
+++ b/tools/testing/selftests/bpf/test_sockopt_sk.c
@@ -6,6 +6,7 @@
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
+#include <netinet/tcp.h>
#include <linux/filter.h>
#include <bpf/bpf.h>
@@ -25,6 +26,7 @@ static int getsetsockopt(void)
union {
char u8[4];
__u32 u32;
+ char cc[16]; /* TCP_CA_NAME_MAX */
} buf = {};
socklen_t optlen;
@@ -115,6 +117,29 @@ static int getsetsockopt(void)
goto err;
}
+ /* TCP_CONGESTION can extend the string */
+
+ strcpy(buf.cc, "nv");
+ err = setsockopt(fd, SOL_TCP, TCP_CONGESTION, &buf, strlen("nv"));
+ if (err) {
+ log_err("Failed to call setsockopt(TCP_CONGESTION)");
+ goto err;
+ }
+
+
+ optlen = sizeof(buf.cc);
+ err = getsockopt(fd, SOL_TCP, TCP_CONGESTION, &buf, &optlen);
+ if (err) {
+ log_err("Failed to call getsockopt(TCP_CONGESTION)");
+ goto err;
+ }
+
+ if (strcmp(buf.cc, "cubic") != 0) {
+ log_err("Unexpected getsockopt(TCP_CONGESTION) %s != %s",
+ buf.cc, "cubic");
+ goto err;
+ }
+
close(fd);
return 0;
err:
--
2.22.0.709.g102302147b-goog
^ permalink raw reply related
* Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors
From: Jonathan Lemon @ 2019-07-29 21:53 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, kernel-team, netdev, Matthew Wilcox
In-Reply-To: <20190729142548.028d5a2b@cakuba.netronome.com>
On 29 Jul 2019, at 14:25, Jakub Kicinski wrote:
> On Mon, 29 Jul 2019 14:22:11 -0700, Jakub Kicinski wrote:
>>>> I realize you're following the existing code, but should we perhaps
>>>> use
>>>> the latest kdoc syntax? '()' after function name, and args should have
>>>> '@' prefix, '%' would be for constants.
>>>
>>> That would be a task for a different cleanup. Not that I disagree with
>>> you, but there's also nothing worse than mixing styles in the same file.
>>
>> Funny you should say that given that (a) I'm commenting on the new code
>> you're adding, and (b) you did do an unrelated spelling fix above ;)
>
> Ah, sorry I misread your comment there.
>
> Some code already uses '()' in this file, as for the '%' skb_frag_
> functions are the only one which have this mistake, the rest of kdoc
> is correct.
The kernel-doc.rst guide seems to indicate that function names should
have () at the end - but none of them do so within this file. (only when
talking about the function in the document).
The %CONST indicates name of a constant - I'm unclear whether this is
supposed to refer to a constant parameter. For example:
/**
* __skb_peek - peek at the head of a non-empty &sk_buff_head
* @list_: list to peek at
*
* Like skb_peek(), but the caller knows that the list is not empty.
*/
static inline struct sk_buff *__skb_peek(const struct sk_buff_head *list_)
{
return list_->next;
}
^ permalink raw reply
* Re: [PATCH net-next 00/16] bnxt_en: Add TPA (GRO_HW and LRO) on 57500 chips.
From: Michael Chan @ 2019-07-29 22:00 UTC (permalink / raw)
To: David Miller; +Cc: Netdev
In-Reply-To: <20190729.142455.1728471766679878919.davem@davemloft.net>
On Mon, Jul 29, 2019 at 2:24 PM David Miller <davem@davemloft.net> wrote:
>
> From: Michael Chan <michael.chan@broadcom.com>
> Date: Mon, 29 Jul 2019 06:10:17 -0400
>
> > This patchset adds TPA v2 support on the 57500 chips. TPA v2 is
> > different from the legacy TPA scheme on older chips and requires major
> > refactoring and restructuring of the existing TPA logic. The main
> > difference is that the new TPA v2 has on-the-fly aggregation buffer
> > completions before a TPA packet is completed. The larger aggregation
> > ID space also requires a new ID mapping logic to make it more
> > memory efficient.
>
> Series applied, but please explain something to me.
>
> I thought initially while reviewing this that patch #5 makes the series
> non-bisectable because this only includes the logic that appends new
> entries to the agg array but lacks the changes to reset the agg count
> at TPE end time (which occurs in patch #8).
>
> However I then realized that you haven't turned on the logic yet that
> can result in CMP_TYPE_RX_TPA_AGG_CMP entries in this context.
>
> Am I right?
Yes, correct. Everything is built up incrementally and the new GRO_HW
and LRO features on the new chip can only be enabled after patch #14.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors
From: Jakub Kicinski @ 2019-07-29 22:02 UTC (permalink / raw)
To: Jonathan Lemon; +Cc: davem, kernel-team, netdev, Matthew Wilcox
In-Reply-To: <94802E4A-536A-4249-BEA3-5D89E8073738@gmail.com>
On Mon, 29 Jul 2019 14:53:45 -0700, Jonathan Lemon wrote:
> On 29 Jul 2019, at 14:25, Jakub Kicinski wrote:
>
> > On Mon, 29 Jul 2019 14:22:11 -0700, Jakub Kicinski wrote:
> >>>> I realize you're following the existing code, but should we perhaps
> >>>> use
> >>>> the latest kdoc syntax? '()' after function name, and args should have
> >>>> '@' prefix, '%' would be for constants.
> >>>
> >>> That would be a task for a different cleanup. Not that I disagree with
> >>> you, but there's also nothing worse than mixing styles in the same file.
> >>
> >> Funny you should say that given that (a) I'm commenting on the new code
> >> you're adding, and (b) you did do an unrelated spelling fix above ;)
> >
> > Ah, sorry I misread your comment there.
> >
> > Some code already uses '()' in this file, as for the '%' skb_frag_
> > functions are the only one which have this mistake, the rest of kdoc
> > is correct.
>
> The kernel-doc.rst guide seems to indicate that function names should
> have () at the end - but none of them do so within this file. (only when
> talking about the function in the document).
/**
* skb_complete_tx_timestamp() - deliver cloned skb with tx timestamps
/**
* skb_tx_timestamp() - Driver hook for transmit timestamping
> The %CONST indicates name of a constant - I'm unclear whether this is
> supposed to refer to a constant parameter. For example:
>
> /**
> * __skb_peek - peek at the head of a non-empty &sk_buff_head
> * @list_: list to peek at
> *
> * Like skb_peek(), but the caller knows that the list is not empty.
> */
> static inline struct sk_buff *__skb_peek(const struct sk_buff_head *list_)
> {
> return list_->next;
> }
Hmm.. I'm not sure I follow, this example does not use %, but & which
is for types. Quoting from:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#highlights-and-cross-references
@parameter
Name of a function parameter. (No cross-referencing, just formatting.)
%CONST
Name of a constant. (No cross-referencing, just formatting.)
So in your case you should use @delta, rather than %delta.
^ permalink raw reply
* [PATCH] net: smc911x: Mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-07-29 22:10 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, Gustavo A. R. Silva, Kees Cook
Mark switch cases where we are expecting to fall through.
This patch fixes the following warning (Building: arm):
drivers/net/ethernet/smsc/smc911x.c: In function ‘smc911x_phy_detect’:
drivers/net/ethernet/smsc/smc911x.c:677:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
if (cfg & HW_CFG_EXT_PHY_DET_) {
^
drivers/net/ethernet/smsc/smc911x.c:715:3: note: here
default:
^~~~~~~
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/net/ethernet/smsc/smc911x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index bd14803545de..8d88e4083456 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -712,6 +712,7 @@ static void smc911x_phy_detect(struct net_device *dev)
/* Found an external PHY */
break;
}
+ /* Else, fall through */
default:
/* Internal media only */
SMC_GET_PHY_ID1(lp, 1, id1);
--
2.22.0
^ permalink raw reply related
* Re: [PATCH] net: smc911x: Mark expected switch fall-through
From: David Miller @ 2019-07-29 22:13 UTC (permalink / raw)
To: gustavo; +Cc: netdev, linux-kernel, keescook
In-Reply-To: <20190729221016.GA17610@embeddedor>
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Date: Mon, 29 Jul 2019 17:10:16 -0500
> Mark switch cases where we are expecting to fall through.
>
> This patch fixes the following warning (Building: arm):
>
> drivers/net/ethernet/smsc/smc911x.c: In function ‘smc911x_phy_detect’:
> drivers/net/ethernet/smsc/smc911x.c:677:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
> if (cfg & HW_CFG_EXT_PHY_DET_) {
> ^
> drivers/net/ethernet/smsc/smc911x.c:715:3: note: here
> default:
> ^~~~~~~
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Applied.
^ permalink raw reply
* [PATCH] tipc: compat: allow tipc commands without arguments
From: Taras Kondratiuk @ 2019-07-29 22:15 UTC (permalink / raw)
To: Jon Maloy, Ying Xue, David S. Miller
Cc: netdev, tipc-discussion, linux-kernel, xe-linux-external, stable
Commit 2753ca5d9009 ("tipc: fix uninit-value in tipc_nl_compat_doit")
broke older tipc tools that use compat interface (e.g. tipc-config from
tipcutils package):
% tipc-config -p
operation not supported
The commit started to reject TIPC netlink compat messages that do not
have attributes. It is too restrictive because some of such messages are
valid (they don't need any arguments):
% grep 'tx none' include/uapi/linux/tipc_config.h
#define TIPC_CMD_NOOP 0x0000 /* tx none, rx none */
#define TIPC_CMD_GET_MEDIA_NAMES 0x0002 /* tx none, rx media_name(s) */
#define TIPC_CMD_GET_BEARER_NAMES 0x0003 /* tx none, rx bearer_name(s) */
#define TIPC_CMD_SHOW_PORTS 0x0006 /* tx none, rx ultra_string */
#define TIPC_CMD_GET_REMOTE_MNG 0x4003 /* tx none, rx unsigned */
#define TIPC_CMD_GET_MAX_PORTS 0x4004 /* tx none, rx unsigned */
#define TIPC_CMD_GET_NETID 0x400B /* tx none, rx unsigned */
#define TIPC_CMD_NOT_NET_ADMIN 0xC001 /* tx none, rx none */
This patch relaxes the original fix and rejects messages without
arguments only if such arguments are expected by a command (reg_type is
non zero).
Fixes: 2753ca5d9009 ("tipc: fix uninit-value in tipc_nl_compat_doit")
Cc: stable@vger.kernel.org
Signed-off-by: Taras Kondratiuk <takondra@cisco.com>
---
The patch is based on v5.3-rc2.
net/tipc/netlink_compat.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index d86030ef1232..e135d4e11231 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -55,6 +55,7 @@ struct tipc_nl_compat_msg {
int rep_type;
int rep_size;
int req_type;
+ int req_size;
struct net *net;
struct sk_buff *rep;
struct tlv_desc *req;
@@ -257,7 +258,8 @@ static int tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
int err;
struct sk_buff *arg;
- if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
+ if (msg->req_type && (!msg->req_size ||
+ !TLV_CHECK_TYPE(msg->req, msg->req_type)))
return -EINVAL;
msg->rep = tipc_tlv_alloc(msg->rep_size);
@@ -354,7 +356,8 @@ static int tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
{
int err;
- if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
+ if (msg->req_type && (!msg->req_size ||
+ !TLV_CHECK_TYPE(msg->req, msg->req_type)))
return -EINVAL;
err = __tipc_nl_compat_doit(cmd, msg);
@@ -1278,8 +1281,8 @@ static int tipc_nl_compat_recv(struct sk_buff *skb, struct genl_info *info)
goto send;
}
- len = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
- if (!len || !TLV_OK(msg.req, len)) {
+ msg.req_size = nlmsg_attrlen(req_nlh, GENL_HDRLEN + TIPC_GENL_HDRLEN);
+ if (msg.req_size && !TLV_OK(msg.req, msg.req_size)) {
msg.rep = tipc_get_err_tlv(TIPC_CFG_NOT_SUPPORTED);
err = -EOPNOTSUPP;
goto send;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH net] net: ipv6: Fix a bug in ndisc_send_ns when netdev only has a global address
From: David Ahern @ 2019-07-29 22:28 UTC (permalink / raw)
To: David Miller, suyj.fnst; +Cc: kuznet, yoshfuji, netdev
In-Reply-To: <20190729.141752.457438545178811941.davem@davemloft.net>
On 7/29/19 3:17 PM, David Miller wrote:
> David, can you take a quick look at this?
will do. I'll get back to you by tomorrow.
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: avoid some redundant vtu load/purge operations
From: Vivien Didelot @ 2019-07-29 22:46 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Rasmus Villemoes,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190722233713.31396-1-rasmus.villemoes@prevas.dk>
Hi Rasmus,
On Mon, 22 Jul 2019 23:37:26 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> We have an ERPS (Ethernet Ring Protection Switching) setup involving
> mv88e6250 switches which we're in the process of switching to a BSP
> based on the mainline driver. Breaking any link in the ring works as
> expected, with the ring reconfiguring itself quickly and traffic
> continuing with almost no noticable drops. However, when plugging back
> the cable, we see 5+ second stalls.
>
> This has been tracked down to the userspace application in charge of
> the protocol missing a few CCM messages on the good link (the one that
> was not unplugged), causing it to broadcast a "signal fail". That
> message eventually reaches its link partner, which responds by
> blocking the port. Meanwhile, the first node has continued to block
> the port with the just plugged-in cable, breaking the network. And the
> reason for those missing CCM messages has in turn been tracked down to
> the VTU apparently being too busy servicing load/purge operations that
> the normal lookups are delayed.
>
> Initial state, the link between C and D is blocked in software.
>
> _____________________
> / \
> | |
> A ----- B ----- C *---- D
>
> Unplug the cable between C and D.
>
> _____________________
> / \
> | |
> A ----- B ----- C * * D
>
> Reestablish the link between C and D.
> _____________________
> / \
> | |
> A ----- B ----- C *---- D
>
> Somehow, enough VTU/ATU operations happen inside C that prevents
> the application from receving the CCM messages from B in a timely
> manner, so a Signal Fail message is sent by C. When B receives
> that, it responds by blocking its port.
>
> _____________________
> / \
> | |
> A ----- B *---* C *---- D
>
> Very shortly after this, the signal fail condition clears on the
> BC link (some CCM messages finally make it through), so C
> unblocks the port. However, a guard timer inside B prevents it
> from removing the blocking before 5 seconds have elapsed.
>
> It is not unlikely that our userspace ERPS implementation could be
> smarter and/or is simply buggy. However, this patch fixes the symptoms
> we see, and is a small optimization that should not break anything
> (knock wood). The idea is simply to avoid doing an VTU load of an
> entry identical to the one already present. To do that, we need to
> know whether mv88e6xxx_vtu_get() actually found an existing entry, or
> has just prepared a struct mv88e6xxx_vtu_entry for us to load. To that
> end, let vlan->valid be an output parameter. The other two callers of
> mv88e6xxx_vtu_get() are not affected by this patch since they pass
> new=false.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 6b17cd961d06..2e500428670c 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1423,7 +1423,6 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
>
> /* Initialize a fresh VLAN entry */
> memset(entry, 0, sizeof(*entry));
> - entry->valid = true;
> entry->vid = vid;
>
> /* Exclude all ports */
> @@ -1618,6 +1617,9 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
> if (err)
> return err;
>
> + if (vlan.valid && vlan.member[port] == member)
> + return 0;
> + vlan.valid = true;
> vlan.member[port] = member;
>
> err = mv88e6xxx_vtu_loadpurge(chip, &vlan);
I'd prefer not to use the mv88e6xxx_vtu_entry structure for output
parameters, this can be confusing. As you correctly mentioned, this
initialization is only done for _mv88e6xxx_port_vlan_add, so I'll
prepare a patch which gets rid of the boolean parameter and move that
logic up.
Thanks,
Vivien
^ permalink raw reply
* [PATCH net] selftests/tls: fix TLS tests with CONFIG_TLS=n
From: Jakub Kicinski @ 2019-07-29 23:08 UTC (permalink / raw)
To: davem; +Cc: netdev, oss-drivers, Jakub Kicinski, kernel test robot
Build bot reports some recent TLS tests are failing
with CONFIG_TLS=n. Correct the expected return code
and skip TLS installation if not supported.
Tested with CONFIG_TLS=n and CONFIG_TLS=m.
Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: cf32526c8842 ("selftests/tls: add a test for ULP but no keys")
Fixes: 65d41fb317c6 ("selftests/tls: add a bidirectional test")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
tools/testing/selftests/net/tls.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 630c5b884d43..d995e6503b1a 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -69,7 +69,7 @@ FIXTURE_SETUP(tls_basic)
ret = setsockopt(self->fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
if (ret != 0) {
- ASSERT_EQ(errno, ENOTSUPP);
+ ASSERT_EQ(errno, ENOENT);
self->notls = true;
printf("Failure setting TCP_ULP, testing without tls\n");
return;
@@ -696,21 +696,26 @@ TEST_F(tls, recv_lowat)
TEST_F(tls, bidir)
{
- struct tls12_crypto_info_aes_gcm_128 tls12;
char const *test_str = "test_read";
int send_len = 10;
char buf[10];
int ret;
- memset(&tls12, 0, sizeof(tls12));
- tls12.info.version = TLS_1_3_VERSION;
- tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
+ if (!self->notls) {
+ struct tls12_crypto_info_aes_gcm_128 tls12;
- ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12, sizeof(tls12));
- ASSERT_EQ(ret, 0);
+ memset(&tls12, 0, sizeof(tls12));
+ tls12.info.version = TLS_1_3_VERSION;
+ tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
- ret = setsockopt(self->cfd, SOL_TLS, TLS_TX, &tls12, sizeof(tls12));
- ASSERT_EQ(ret, 0);
+ ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12,
+ sizeof(tls12));
+ ASSERT_EQ(ret, 0);
+
+ ret = setsockopt(self->cfd, SOL_TLS, TLS_TX, &tls12,
+ sizeof(tls12));
+ ASSERT_EQ(ret, 0);
+ }
ASSERT_EQ(strlen(test_str) + 1, send_len);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH bpf-next 00/10] CO-RE offset relocations
From: Andrii Nakryiko @ 2019-07-29 23:09 UTC (permalink / raw)
To: Song Liu
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Yonghong Song, Kernel Team
In-Reply-To: <CAPhsuW5H2QQjuASV2iXTdA73E7AQnj73b77x4FmJomc-gJy-Cg@mail.gmail.com>
On Mon, Jul 29, 2019 at 1:37 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Jul 29, 2019 at 1:20 PM Song Liu <liu.song.a23@gmail.com> wrote:
> >
> > On Wed, Jul 24, 2019 at 1:34 PM Andrii Nakryiko <andriin@fb.com> wrote:
> > >
> > > This patch set implements central part of CO-RE (Compile Once - Run
> > > Everywhere, see [0] and [1] for slides and video): relocating field offsets.
> > > Most of the details are written down as comments to corresponding parts of the
> > > code.
> > >
> > > Patch #1 adds loading of .BTF.ext offset relocations section and macros to
> > > work with its contents.
> > > Patch #2 implements CO-RE relocations algorithm in libbpf.
> > > Patches #3-#10 adds selftests validating various parts of relocation handling,
> > > type compatibility, etc.
> > >
> > > For all tests to work, you'll need latest Clang/LLVM supporting
> > > __builtin_preserve_access_index intrinsic, used for recording offset
> > > relocations. Kernel on which selftests run should have BTF information built
> > > in (CONFIG_DEBUG_INFO_BTF=y).
> > >
> > > [0] http://vger.kernel.org/bpfconf2019.html#session-2
> > > [1] http://vger.kernel.org/lpc-bpf2018.html#session-2CO-RE relocations
> > >
> > > This patch set implements central part of CO-RE (Compile Once - Run
> > > Everywhere, see [0] and [1] for slides and video): relocating field offsets.
> > > Most of the details are written down as comments to corresponding parts of the
> > > code.
> > >
> > > Patch #1 adds loading of .BTF.ext offset relocations section and macros to
> > > work with its contents.
> > > Patch #2 implements CO-RE relocations algorithm in libbpf.
> > > Patches #3-#10 adds selftests validating various parts of relocation handling,
> > > type compatibility, etc.
> > >
> > > For all tests to work, you'll need latest Clang/LLVM supporting
> > > __builtin_preserve_access_index intrinsic, used for recording offset
> > > relocations. Kernel on which selftests run should have BTF information built
> > > in (CONFIG_DEBUG_INFO_BTF=y).
> > >
> > > [0] http://vger.kernel.org/bpfconf2019.html#session-2
> > > [1] http://vger.kernel.org/lpc-bpf2018.html#session-2
> > >
> > > Andrii Nakryiko (10):
> > > libbpf: add .BTF.ext offset relocation section loading
> > > libbpf: implement BPF CO-RE offset relocation algorithm
> > > selftests/bpf: add CO-RE relocs testing setup
> > > selftests/bpf: add CO-RE relocs struct flavors tests
> > > selftests/bpf: add CO-RE relocs nesting tests
> > > selftests/bpf: add CO-RE relocs array tests
> > > selftests/bpf: add CO-RE relocs enum/ptr/func_proto tests
> > > selftests/bpf: add CO-RE relocs modifiers/typedef tests
> > > selftest/bpf: add CO-RE relocs ptr-as-array tests
> > > selftests/bpf: add CO-RE relocs ints tests
> > >
> > > tools/lib/bpf/btf.c | 64 +-
> > > tools/lib/bpf/btf.h | 4 +
> > > tools/lib/bpf/libbpf.c | 866 +++++++++++++++++-
> > > tools/lib/bpf/libbpf.h | 1 +
> > > tools/lib/bpf/libbpf_internal.h | 91 ++
> > > .../selftests/bpf/prog_tests/core_reloc.c | 363 ++++++++
> > > .../bpf/progs/btf__core_reloc_arrays.c | 3 +
> > > .../btf__core_reloc_arrays___diff_arr_dim.c | 3 +
> > > ...btf__core_reloc_arrays___diff_arr_val_sz.c | 3 +
> > > .../btf__core_reloc_arrays___err_non_array.c | 3 +
> > > ...btf__core_reloc_arrays___err_too_shallow.c | 3 +
> > > .../btf__core_reloc_arrays___err_too_small.c | 3 +
> > > ..._core_reloc_arrays___err_wrong_val_type1.c | 3 +
> > > ..._core_reloc_arrays___err_wrong_val_type2.c | 3 +
> > > .../bpf/progs/btf__core_reloc_flavors.c | 3 +
> > > .../btf__core_reloc_flavors__err_wrong_name.c | 3 +
> > > .../bpf/progs/btf__core_reloc_ints.c | 3 +
> > > .../bpf/progs/btf__core_reloc_ints___bool.c | 3 +
> > > .../btf__core_reloc_ints___err_bitfield.c | 3 +
> > > .../btf__core_reloc_ints___err_wrong_sz_16.c | 3 +
> > > .../btf__core_reloc_ints___err_wrong_sz_32.c | 3 +
> > > .../btf__core_reloc_ints___err_wrong_sz_64.c | 3 +
> > > .../btf__core_reloc_ints___err_wrong_sz_8.c | 3 +
> > > .../btf__core_reloc_ints___reverse_sign.c | 3 +
> > > .../bpf/progs/btf__core_reloc_mods.c | 3 +
> > > .../progs/btf__core_reloc_mods___mod_swap.c | 3 +
> > > .../progs/btf__core_reloc_mods___typedefs.c | 3 +
> > > .../bpf/progs/btf__core_reloc_nesting.c | 3 +
> > > .../btf__core_reloc_nesting___anon_embed.c | 3 +
> > > ...f__core_reloc_nesting___dup_compat_types.c | 5 +
> > > ...core_reloc_nesting___err_array_container.c | 3 +
> > > ...tf__core_reloc_nesting___err_array_field.c | 3 +
> > > ...e_reloc_nesting___err_dup_incompat_types.c | 4 +
> > > ...re_reloc_nesting___err_missing_container.c | 3 +
> > > ...__core_reloc_nesting___err_missing_field.c | 3 +
> > > ..._reloc_nesting___err_nonstruct_container.c | 3 +
> > > ...e_reloc_nesting___err_partial_match_dups.c | 4 +
> > > .../btf__core_reloc_nesting___err_too_deep.c | 3 +
> > > .../btf__core_reloc_nesting___extra_nesting.c | 3 +
> > > ..._core_reloc_nesting___struct_union_mixup.c | 3 +
> > > .../bpf/progs/btf__core_reloc_primitives.c | 3 +
> > > ...f__core_reloc_primitives___diff_enum_def.c | 3 +
> > > ..._core_reloc_primitives___diff_func_proto.c | 3 +
> > > ...f__core_reloc_primitives___diff_ptr_type.c | 3 +
> > > ...tf__core_reloc_primitives___err_non_enum.c | 3 +
> > > ...btf__core_reloc_primitives___err_non_int.c | 3 +
> > > ...btf__core_reloc_primitives___err_non_ptr.c | 3 +
> > > .../bpf/progs/btf__core_reloc_ptr_as_arr.c | 3 +
> > > .../btf__core_reloc_ptr_as_arr___diff_sz.c | 3 +
> > > .../selftests/bpf/progs/core_reloc_types.h | 642 +++++++++++++
> > > .../bpf/progs/test_core_reloc_arrays.c | 58 ++
> > > .../bpf/progs/test_core_reloc_flavors.c | 65 ++
> > > .../bpf/progs/test_core_reloc_ints.c | 48 +
> > > .../bpf/progs/test_core_reloc_kernel.c | 39 +
> > > .../bpf/progs/test_core_reloc_mods.c | 68 ++
> > > .../bpf/progs/test_core_reloc_nesting.c | 48 +
> > > .../bpf/progs/test_core_reloc_primitives.c | 50 +
> > > .../bpf/progs/test_core_reloc_ptr_as_arr.c | 34 +
> > > 58 files changed, 2527 insertions(+), 47 deletions(-)
> > > create mode 100644 tools/testing/selftests/bpf/prog_tests/core_reloc.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___diff_arr_dim.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___diff_arr_val_sz.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_non_array.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_too_shallow.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_too_small.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_wrong_val_type1.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_arrays___err_wrong_val_type2.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_flavors.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_flavors__err_wrong_name.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___bool.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___err_bitfield.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___err_wrong_sz_16.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___err_wrong_sz_32.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___err_wrong_sz_64.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___err_wrong_sz_8.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ints___reverse_sign.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_mods.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_mods___mod_swap.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_mods___typedefs.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___anon_embed.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___dup_compat_types.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_array_container.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_array_field.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_dup_incompat_types.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_missing_container.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_missing_field.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_nonstruct_container.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_partial_match_dups.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___err_too_deep.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___extra_nesting.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_nesting___struct_union_mixup.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___diff_enum_def.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___diff_func_proto.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___diff_ptr_type.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___err_non_enum.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___err_non_int.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_primitives___err_non_ptr.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ptr_as_arr.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/btf__core_reloc_ptr_as_arr___diff_sz.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/core_reloc_types.h
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_arrays.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_flavors.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_ints.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_mods.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_nesting.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_primitives.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_core_reloc_ptr_as_arr.c
> >
> > We have created a lot of small files. Would it be cleaner if we can
> > somehow put these
> > data in one file (maybe different sections?).
>
> After reading more, I guess you have tried this and end up with current
> design: keep most struct defines in core_reloc_types.h.
Yeah, I have all the definition in one header file, but then I need
individual combinations as separate BTFs, so I essentially "pick"
desired types using function declarations. Creating those BTFs by hand
would be a nightmare to create and maintain.
>
> >
> > Alternatively, maybe create a folder for these files:
> > tools/testing/selftests/bpf/progs/core/
>
> I guess this would still make it cleaner.
There is nothing too special about core tests to split them. Also it
would require Makefile changes and would deviate test_progs
definitions from analogous test_maps, test_verifier, test_btf, etc, so
I'm not sure about that. I though about putting those btf__* files
under separate directory, but I'm on the fence there as well, as I'd
rather have related files to stay together...
>
> Thanks,
> Song
^ permalink raw reply
* [PATCH v2 net-next 1/1] tc-testing: Clarify the use of tdc's -d option
From: Lucas Bates @ 2019-07-29 23:18 UTC (permalink / raw)
To: davem
Cc: netdev, nicolas.dichtel, jhs, xiyou.wangcong, jiri, mleitner,
vladbu, dcaratti, kernel, Lucas Bates
The -d command line argument to tdc requires the name of a physical device
on the system where the tests will be run. If -d has not been used, tdc
will skip tests that require a physical device.
This patch is intended to better document what the -d option does and how
it is used.
Signed-off-by: Lucas Bates <lucasb@mojatatu.com>
---
tools/testing/selftests/tc-testing/README | 4 +++-
tools/testing/selftests/tc-testing/tdc.py | 12 ++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/tc-testing/README b/tools/testing/selftests/tc-testing/README
index 22e5da9..b0954c8 100644
--- a/tools/testing/selftests/tc-testing/README
+++ b/tools/testing/selftests/tc-testing/README
@@ -128,7 +128,9 @@ optional arguments:
-v, --verbose Show the commands that are being run
-N, --notap Suppress tap results for command under test
-d DEVICE, --device DEVICE
- Execute the test case in flower category
+ Execute test cases that use a physical device, where
+ DEVICE is its name. (If not defined, tests that require
+ a physical device will be skipped)
-P, --pause Pause execution just before post-suite stage
selection:
diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
index f04321a..e566c70 100755
--- a/tools/testing/selftests/tc-testing/tdc.py
+++ b/tools/testing/selftests/tc-testing/tdc.py
@@ -356,12 +356,14 @@ def test_runner(pm, args, filtered_tests):
time.sleep(2)
for tidx in testlist:
if "flower" in tidx["category"] and args.device == None:
+ errmsg = "Tests using the DEV2 variable must define the name of a "
+ errmsg += "physical NIC with the -d option when running tdc.\n"
+ errmsg += "Test has been skipped."
if args.verbose > 1:
- print('Not executing test {} {} because DEV2 not defined'.
- format(tidx['id'], tidx['name']))
+ print(errmsg)
res = TestResult(tidx['id'], tidx['name'])
res.set_result(ResultState.skip)
- res.set_errormsg('Not executed because DEV2 is not defined')
+ res.set_errormsg(errmsg)
tsr.add_resultdata(res)
continue
try:
@@ -499,7 +501,9 @@ def set_args(parser):
choices=['none', 'xunit', 'tap'],
help='Specify the format for test results. (Default: TAP)')
parser.add_argument('-d', '--device',
- help='Execute the test case in flower category')
+ help='Execute test cases that use a physical device, ' +
+ 'where DEVICE is its name. (If not defined, tests ' +
+ 'that require a physical device will be skipped)')
parser.add_argument(
'-P', '--pause', action='store_true',
help='Pause execution just before post-suite stage')
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 1/4] dt-bindings: net: Add aspeed,ast2600-mdio binding
From: Rob Herring @ 2019-07-29 23:37 UTC (permalink / raw)
To: Andrew Jeffery
Cc: netdev, David Miller, Mark Rutland, Joel Stanley, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org
In-Reply-To: <20190729043926.32679-2-andrew@aj.id.au>
On Sun, Jul 28, 2019 at 10:39 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> The AST2600 splits out the MDIO bus controller from the MAC into its own
> IP block and rearranges the register layout. Add a new binding to
> describe the new hardware.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../bindings/net/aspeed,ast2600-mdio.yaml | 61 +++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
> new file mode 100644
> index 000000000000..fa86f6438473
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/aspeed,ast2600-mdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2600 MDIO Controller
> +
> +maintainers:
> + - Andrew Jeffery <andrew@aj.id.au>
> +
> +description: |+
> + The ASPEED AST2600 MDIO controller is the third iteration of ASPEED's MDIO
> + bus register interface, this time also separating out the controller from the
> + MAC.
> +
Should have a:
allOf:
- $ref: "mdio.yaml#"
> +properties:
> + compatible:
> + const: aspeed,ast2600-mdio
> + reg:
> + maxItems: 1
> + description: The register range of the MDIO controller instance
> + "#address-cells":
> + const: 1
> + "#size-cells":
> + const: 0
Then you can drop these 2.
> +
> +patternProperties:
> + "^ethernet-phy@[a-f0-9]$":
> + properties:
> + reg:
> + description:
> + The MDIO bus index of the PHY
And this.
> + compatible:
> + enum:
> + - ethernet-phy-ieee802.3-c22
> + - ethernet-phy-ieee802.3-c45
This isn't specific to ASpeed either and is already covered by
ethernet-phy.yaml.
So that means none of the child node schema is needed here.
> + required:
> + - reg
> +
> +required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> +
> +examples:
> + - |
> + mdio0: mdio@1e650000 {
> + compatible = "aspeed,ast2600-mdio";
> + reg = <0x1e650000 0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + status = "okay";
Don't show status in examples.
> +
> + ethphy0: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> + };
> --
> 2.20.1
>
^ permalink raw reply
* Re: [RFC PATCH 1/2] gianfar: convert to phylink
From: Vladimir Oltean @ 2019-07-29 23:39 UTC (permalink / raw)
To: Arseny Solokha, Claudiu Manoil, Russell King
Cc: Ioana Ciornei, Andrew Lunn, netdev, Florian Fainelli
In-Reply-To: <20190723151702.14430-2-asolokha@kb.kras.ru>
Hi Arseny,
Nice project!
On Wed, 24 Jul 2019 at 03:38, Arseny Solokha <asolokha@kb.kras.ru> wrote:
>
> Convert gianfar to use the phylink API for better SFP modules support.
>
> The driver still uses phylib for serdes configuration over the TBI
> interface, as there seems to be no functionally equivalent API present
> in phylink (yet). phylib usage is basically confined in two functions.
>
> One needs to change their Device Tree accordingly to get working SFP
> support:
>
> enet1: ethernet@25000 {
> <...>
> device_type = "network";
> model = "eTSEC";
> compatible = "gianfar";
> tbi-handle = <&tbi0>;
> + phy-connection-type = "sgmii";
> + managed = "in-band-status";
> + sfp = <&sfp>;
> max-speed = <1000>;
>
> mdio@520 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "fsl,gianfar-tbi";
> reg = <0x520 0x20>;
>
> tbi0: tbi-phy@1f {
> reg = <0x1f>;
> device_type = "tbi-phy";
> };
> };
> +
> + sfp: sfp0 {
> + compatible = "sff,sfp";
> + i2c-bus = <&i2c1>;
> + mod-def0-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> + rate-select0-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> + tx-disable-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
> + tx-fault-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;
> + los-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
> + };
> };
>
> Signed-off-by: Arseny Solokha <asolokha@kb.kras.ru>
> ---
> drivers/net/ethernet/freescale/Kconfig | 2 +-
> drivers/net/ethernet/freescale/gianfar.c | 409 +++++++++---------
> drivers/net/ethernet/freescale/gianfar.h | 26 +-
> .../net/ethernet/freescale/gianfar_ethtool.c | 79 ++--
> 4 files changed, 251 insertions(+), 265 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
> index 6a7e8993119f..8b51d423b61d 100644
> --- a/drivers/net/ethernet/freescale/Kconfig
> +++ b/drivers/net/ethernet/freescale/Kconfig
> @@ -89,7 +89,7 @@ config GIANFAR
> tristate "Gianfar Ethernet"
> depends on HAS_DMA
> select FSL_PQ_MDIO
> - select PHYLIB
> + select PHYLINK
> select CRC32
> ---help---
> This driver supports the Gigabit TSEC on the MPC83xx, MPC85xx,
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 7ea19e173339..64c7b174e591 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -96,6 +96,7 @@
> #include <linux/mii.h>
> #include <linux/phy.h>
> #include <linux/phy_fixed.h>
> +#include <linux/phylink.h>
You can remove linux/phy.h.
You (PHYLINK, actually) broke .ndo_change_carrier, and therefore
commit 6211d46713c5 ("gianfar: Add change_carrier() for Fixed PHYs").
If we were honest, we should probably revert that commit entirely now.
Once you do that, you can remove linux/phy_fixed.h as well.
> #include <linux/of.h>
> #include <linux/of_net.h>
>
> @@ -117,8 +118,18 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu);
> static irqreturn_t gfar_error(int irq, void *dev_id);
> static irqreturn_t gfar_transmit(int irq, void *dev_id);
> static irqreturn_t gfar_interrupt(int irq, void *dev_id);
> -static void adjust_link(struct net_device *dev);
> -static noinline void gfar_update_link_state(struct gfar_private *priv);
> +static void gfar_phylink_validate(struct phylink_config *config,
> + unsigned long *supported,
> + struct phylink_link_state *state);
> +static int gfar_mac_link_state(struct phylink_config *config,
> + struct phylink_link_state *state);
> +static void gfar_mac_config(struct phylink_config *config, unsigned int mode,
> + const struct phylink_link_state *state);
> +static void gfar_mac_an_restart(struct phylink_config *config);
> +static void gfar_mac_link_down(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface);
> +static void gfar_mac_link_up(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface, struct phy_device *phy);
Please do not add to this forward declaration madness. A cleanup patch
in this area would be highly appreciated.
> static int init_phy(struct net_device *dev);
> static int gfar_probe(struct platform_device *ofdev);
> static int gfar_remove(struct platform_device *ofdev);
> @@ -504,6 +515,15 @@ static const struct net_device_ops gfar_netdev_ops = {
> #endif
> };
>
> +static const struct phylink_mac_ops gfar_phylink_ops = {
> + .validate = gfar_phylink_validate,
> + .mac_link_state = gfar_mac_link_state,
> + .mac_config = gfar_mac_config,
> + .mac_an_restart = gfar_mac_an_restart,
> + .mac_link_down = gfar_mac_link_down,
> + .mac_link_up = gfar_mac_link_up,
> +};
> +
> static void gfar_ints_disable(struct gfar_private *priv)
> {
> int i;
> @@ -733,6 +753,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> struct gfar_private *priv = NULL;
> struct device_node *np = ofdev->dev.of_node;
> struct device_node *child = NULL;
> + struct phylink *phylink;
> u32 stash_len = 0;
> u32 stash_idx = 0;
> unsigned int num_tx_qs, num_rx_qs;
> @@ -891,11 +912,21 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
>
> err = of_property_read_string(np, "phy-connection-type", &ctype);
>
> - /* We only care about rgmii-id. The rest are autodetected */
> - if (err == 0 && !strcmp(ctype, "rgmii-id"))
> - priv->interface = PHY_INTERFACE_MODE_RGMII_ID;
> - else
> + /* We only care about rgmii-id and sgmii - the former
> + * is indistinguishable from rgmii in hardware, and phylink needs
> + * the latter to be set appropriately for correct phy configuration.
> + * The rest are autodetected
> + */
> + if (err == 0) {
> + if (!strcmp(ctype, "rgmii-id"))
> + priv->interface = PHY_INTERFACE_MODE_RGMII_ID;
> + else if (!strcmp(ctype, "sgmii"))
> + priv->interface = PHY_INTERFACE_MODE_SGMII;
> + else
> + priv->interface = PHY_INTERFACE_MODE_MII;
> + } else {
> priv->interface = PHY_INTERFACE_MODE_MII;
> + }
>
No. Don't do this. Just do:
err = of_get_phy_mode(np);
if (err < 0)
goto err_grp_init;
priv->interface = err;
> if (of_find_property(np, "fsl,magic-packet", NULL))
> priv->device_flags |= FSL_GIANFAR_DEV_HAS_MAGIC_PACKET;
> @@ -903,19 +934,21 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> if (of_get_property(np, "fsl,wake-on-filer", NULL))
> priv->device_flags |= FSL_GIANFAR_DEV_HAS_WAKE_ON_FILER;
>
> - priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
> + priv->device_node = np;
> + priv->speed = SPEED_UNKNOWN;
>
> - /* In the case of a fixed PHY, the DT node associated
> - * to the PHY is the Ethernet MAC DT node.
> - */
> - if (!priv->phy_node && of_phy_is_fixed_link(np)) {
> - err = of_phy_register_fixed_link(np);
> - if (err)
> - goto err_grp_init;
> + priv->phylink_config.dev = &priv->ndev->dev;
> + priv->phylink_config.type = PHYLINK_NETDEV;
>
> - priv->phy_node = of_node_get(np);
> + phylink = phylink_create(&priv->phylink_config, of_fwnode_handle(np),
> + priv->interface, &gfar_phylink_ops);
You introduced a bug here.
of_phy_connect used to take the PHY interface type (for good or bad)
from gfar_get_interface() (which is reconstructing it from the MAC
registers).
You are now passing the PHY interface type to phylink_create from the
"phy-connection-type" DT property.
At the very least, you are breaking LS1021A which uses phy-mode
instead of phy-connection-type (hence my comment above to use the
generic OF helper).
Actually I think you just uncovered a latent bug, in that the DT
bindings for phy-mode didn't mean much at all to the driver - it would
rely on what the bootloader had set up.
Actually DT bindings for phy-connection-type were most likely simply
bolt on on top of gianfar when they figured they couldn't just
auto-detect the various species of required RGMII delays.
But gfar_get_interface is a piece of history that was introduced in
the same commit as the enum phy_interface_t itself: e8a2b6a42073
("[PATCH] PHY: Add support for configuring the PHY connection
interface"). Its time has come.
> + if (IS_ERR(phylink)) {
> + err = PTR_ERR(phylink);
> + goto err_grp_init;
> }
>
> + priv->phylink = phylink;
> +
> /* Find the TBI PHY. If it's not there, we don't support SGMII */
> priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
>
> @@ -994,7 +1027,7 @@ static int gfar_hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
>
> static int gfar_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> {
> - struct phy_device *phydev = dev->phydev;
> + struct gfar_private *priv = netdev_priv(dev);
>
> if (!netif_running(dev))
> return -EINVAL;
> @@ -1004,10 +1037,7 @@ static int gfar_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> if (cmd == SIOCGHWTSTAMP)
> return gfar_hwtstamp_get(dev, rq);
>
> - if (!phydev)
> - return -ENODEV;
> -
> - return phy_mii_ioctl(phydev, rq, cmd);
> + return phylink_mii_ioctl(priv->phylink, rq, cmd);
> }
>
> static u32 cluster_entry_per_class(struct gfar_private *priv, u32 rqfar,
> @@ -1307,7 +1337,6 @@ static void gfar_init_addr_hash_table(struct gfar_private *priv)
> */
> static int gfar_probe(struct platform_device *ofdev)
> {
> - struct device_node *np = ofdev->dev.of_node;
> struct net_device *dev = NULL;
> struct gfar_private *priv = NULL;
> int err = 0, i;
> @@ -1463,12 +1492,10 @@ static int gfar_probe(struct platform_device *ofdev)
> return 0;
>
> register_fail:
> - if (of_phy_is_fixed_link(np))
> - of_phy_deregister_fixed_link(np);
> unmap_group_regs(priv);
> gfar_free_rx_queues(priv);
> gfar_free_tx_queues(priv);
> - of_node_put(priv->phy_node);
> + phylink_destroy(priv->phylink);
> of_node_put(priv->tbi_node);
> free_gfar_dev(priv);
> return err;
> @@ -1477,19 +1504,15 @@ static int gfar_probe(struct platform_device *ofdev)
> static int gfar_remove(struct platform_device *ofdev)
> {
> struct gfar_private *priv = platform_get_drvdata(ofdev);
> - struct device_node *np = ofdev->dev.of_node;
>
> - of_node_put(priv->phy_node);
> of_node_put(priv->tbi_node);
>
> unregister_netdev(priv->ndev);
>
> - if (of_phy_is_fixed_link(np))
> - of_phy_deregister_fixed_link(np);
> -
> unmap_group_regs(priv);
> gfar_free_rx_queues(priv);
> gfar_free_tx_queues(priv);
> + phylink_destroy(priv->phylink);
> free_gfar_dev(priv);
>
> return 0;
> @@ -1643,9 +1666,11 @@ static int gfar_suspend(struct device *dev)
> gfar_start_wol_filer(priv);
>
> } else {
> - phy_stop(ndev->phydev);
> + phylink_stop(phy->phylink);
> }
>
> + priv->speed = SPEED_UNKNOWN;
> +
> return 0;
> }
>
> @@ -1672,7 +1697,7 @@ static int gfar_resume(struct device *dev)
> gfar_filer_restore_table(priv);
>
> } else {
> - phy_start(ndev->phydev);
> + phylink_start(priv->phylink);
> }
>
> gfar_start(priv);
> @@ -1702,12 +1727,7 @@ static int gfar_restore(struct device *dev)
>
> gfar_start(priv);
>
> - priv->oldlink = 0;
> - priv->oldspeed = 0;
> - priv->oldduplex = -1;
> -
> - if (ndev->phydev)
> - phy_start(ndev->phydev);
> + phylink_start(priv->phylink);
>
> netif_device_attach(ndev);
> enable_napi(priv);
> @@ -1781,46 +1801,26 @@ static phy_interface_t gfar_get_interface(struct net_device *dev)
> */
> static int init_phy(struct net_device *dev)
> {
> - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> struct gfar_private *priv = netdev_priv(dev);
> phy_interface_t interface;
> - struct phy_device *phydev;
> struct ethtool_eee edata;
> + int flags = 0;
> + int err;
>
> - linkmode_set_bit_array(phy_10_100_features_array,
> - ARRAY_SIZE(phy_10_100_features_array),
> - mask);
> - linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
> - linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, mask);
> - if (priv->device_flags & FSL_GIANFAR_DEV_HAS_GIGABIT)
> - linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, mask);
> -
> - priv->oldlink = 0;
> - priv->oldspeed = 0;
> - priv->oldduplex = -1;
> -
> - interface = gfar_get_interface(dev);
> -
> - phydev = of_phy_connect(dev, priv->phy_node, &adjust_link, 0,
> - interface);
> - if (!phydev) {
> - dev_err(&dev->dev, "could not attach to PHY\n");
> - return -ENODEV;
> + err = phylink_of_phy_connect(priv->phylink, priv->device_node, flags);
> + if (err) {
> + netdev_err(dev, "could not attach to PHY: %d\n", err);
> + return err;
While you're at it, can you say "connect" instead of "attach" here?
> }
>
> + priv->tbi_phy = NULL;
> + interface = gfar_get_interface(dev);
Be consistent and just go for priv->interface. Nobody's changing it anyway.
> if (interface == PHY_INTERFACE_MODE_SGMII)
> gfar_configure_serdes(dev);
>
My gut feeling says that this should be moved to gfar_mac_config.
> - /* Remove any features not supported by the controller */
> - linkmode_and(phydev->supported, phydev->supported, mask);
> - linkmode_copy(phydev->advertising, phydev->supported);
> -
> - /* Add support for flow control */
> - phy_support_asym_pause(phydev);
> -
> /* disable EEE autoneg, EEE not supported by eTSEC */
> memset(&edata, 0, sizeof(struct ethtool_eee));
> - phy_ethtool_set_eee(phydev, &edata);
> + phylink_ethtool_set_eee(priv->phylink, &edata);
>
> return 0;
> }
> @@ -1850,6 +1850,8 @@ static void gfar_configure_serdes(struct net_device *dev)
> return;
> }
>
> + priv->tbi_phy = tbiphy;
> +
> /* If the link is already up, we must already be ok, and don't need to
> * configure and reset the TBI<->SerDes link. Maybe U-Boot configured
> * everything for us? Resetting it takes the link down and requires
> @@ -1964,7 +1966,7 @@ void stop_gfar(struct net_device *dev)
> /* disable ints and gracefully shut down Rx/Tx DMA */
> gfar_halt(priv);
>
> - phy_stop(dev->phydev);
> + phylink_stop(priv->phylink);
>
> free_skb_resources(priv);
> }
> @@ -2219,12 +2221,7 @@ int startup_gfar(struct net_device *ndev)
> /* Start Rx/Tx DMA and enable the interrupts */
> gfar_start(priv);
>
> - /* force link state update after mac reset */
> - priv->oldlink = 0;
> - priv->oldspeed = 0;
> - priv->oldduplex = -1;
> -
> - phy_start(ndev->phydev);
> + phylink_start(priv->phylink);
>
> enable_napi(priv);
>
> @@ -2593,7 +2590,7 @@ static int gfar_close(struct net_device *dev)
> stop_gfar(dev);
>
> /* Disconnect from the PHY */
> - phy_disconnect(dev->phydev);
> + phylink_disconnect_phy(priv->phylink);
It is very odd to disconnect from the PHY on ndo_close and connect
back on ndo_open. I don't know of any other driver that does that.
Can't you change the behavior to simply start and stop phylink here?
>
> gfar_free_irq(priv);
>
> @@ -3387,23 +3384,6 @@ static irqreturn_t gfar_interrupt(int irq, void *grp_id)
> return IRQ_HANDLED;
> }
>
> -/* Called every time the controller might need to be made
> - * aware of new link state. The PHY code conveys this
> - * information through variables in the phydev structure, and this
> - * function converts those variables into the appropriate
> - * register values, and can bring down the device if needed.
> - */
> -static void adjust_link(struct net_device *dev)
> -{
> - struct gfar_private *priv = netdev_priv(dev);
> - struct phy_device *phydev = dev->phydev;
> -
> - if (unlikely(phydev->link != priv->oldlink ||
> - (phydev->link && (phydev->duplex != priv->oldduplex ||
> - phydev->speed != priv->oldspeed))))
> - gfar_update_link_state(priv);
> -}
Getting rid of the cruft from this function deserves its own patch.
> -
> /* Update the hash table based on the current list of multicast
> * addresses we subscribe to. Also, change the promiscuity of
> * the device based on the flags (this function is called
> @@ -3635,132 +3615,169 @@ static irqreturn_t gfar_error(int irq, void *grp_id)
> return IRQ_HANDLED;
> }
>
> -static u32 gfar_get_flowctrl_cfg(struct gfar_private *priv)
> +static void gfar_phylink_validate(struct phylink_config *config,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + struct gfar_private *priv = netdev_priv(to_net_dev(config->dev));
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_SGMII &&
> + state->interface != PHY_INTERFACE_MODE_1000BASEX &&
Russell is right, you shouldn't advertise 1000Base-X if there is no
way of making sure that it works.
His question was:
> How is the difference between SGMII and 1000BASE-X handled?
To be honest I don't have a complete answer to that question. The
literature recommends writing 0x01a0 to the MII_ADVERTISE (0x4)
register of the MAC PCS for 1000Base-X, and 0x4001 for SGMII.
The FMan driver which uses the TSEC MAC does exactly that:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/freescale/fman/fman_dtsec.c#n58
However I can't seem to be able to trace down the definition of bit 14
from 0x4001 - it's reserved in all the manuals I see. I have a hunch
that it selects the format of the Config_Reg base page between
1000Base-X and SGMII.
But the bad news is that gianfar is only configuring the TBI PHY for
the 1000Base-X Config_Reg, even for SGMII PHYs. I'm not quite sure,
but I think that the only reason that this doesn't break in-band AN is
that we don't use in-band AN.
> + !phy_interface_mode_is_rgmii(state->interface)) {
> + phylink_zero(supported);
> + return;
> + }
> +
> + phylink_set(mask, Autoneg);
> + phylink_set(mask, Pause);
> + phylink_set(mask, Asym_Pause);
> + phylink_set_port_modes(mask);
> +
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);
> +
> + if (priv->device_flags & FSL_GIANFAR_DEV_HAS_GIGABIT) {
> + phylink_set(mask, 1000baseX_Full);
> + phylink_set(mask, 1000baseT_Full);
> + }
> +
> + linkmode_and(supported, supported, mask);
> + linkmode_and(state->advertising, state->advertising, mask);
> +}
> +
> +static int gfar_mac_link_state(struct phylink_config *config,
> + struct phylink_link_state *state)
> +{
> + if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> + state->interface == PHY_INTERFACE_MODE_1000BASEX) {
What if you reduce the indentation level by 1 here, by just exiting if
the interface mode is not SGMII?
> + struct gfar_private *priv =
> + netdev_priv(to_net_dev(config->dev));
> + u16 tbi_cr;
> +
> + if (!priv->tbi_phy)
> + return -ENODEV;
> +
> + tbi_cr = phy_read(priv->tbi_phy, MII_TBI_CR);
> +
> + state->duplex = !!(tbi_cr & TBI_CR_FULL_DUPLEX);
Woah there. Aren't you supposed to first ensure state->an_complete is
ok, based on TBI_MII_Register_Set_SR[AN_Done]? There's also a
Link_Status bit in that register that you could retrieve.
> + if ((tbi_cr & TBI_CR_SPEED_1000_MASK) == TBI_CR_SPEED_1000_MASK)
> + state->speed = SPEED_1000;
> + }
See the Speed_Bit table from TBI_MII_Register_Set_ANLPBPA_SGMII for
the link partner (aka SGMII PHY) advertisement. You have to do a
logical-and between that and your own. Also please make sure you
really are in SGMII AN and not 1000 Base-X when interpreting registers
4 & 5 as one way or another.
> +
> + return 1;
> +}
> +
> +static u32 gfar_get_flowctrl_cfg(const struct phylink_link_state *state)
> {
> - struct net_device *ndev = priv->ndev;
> - struct phy_device *phydev = ndev->phydev;
> u32 val = 0;
>
> - if (!phydev->duplex)
> + if (!state->duplex)
> return val;
>
> - if (!priv->pause_aneg_en) {
> - if (priv->tx_pause_en)
> - val |= MACCFG1_TX_FLOW;
> - if (priv->rx_pause_en)
> - val |= MACCFG1_RX_FLOW;
> - } else {
> - u16 lcl_adv, rmt_adv;
> - u8 flowctrl;
> - /* get link partner capabilities */
> - rmt_adv = 0;
> - if (phydev->pause)
> - rmt_adv = LPA_PAUSE_CAP;
> - if (phydev->asym_pause)
> - rmt_adv |= LPA_PAUSE_ASYM;
> -
> - lcl_adv = linkmode_adv_to_lcl_adv_t(phydev->advertising);
> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> - if (flowctrl & FLOW_CTRL_TX)
> - val |= MACCFG1_TX_FLOW;
> - if (flowctrl & FLOW_CTRL_RX)
> - val |= MACCFG1_RX_FLOW;
> - }
> + if (state->pause & MLO_PAUSE_TX)
> + val |= MACCFG1_TX_FLOW;
> + if (state->pause & MLO_PAUSE_RX)
> + val |= MACCFG1_RX_FLOW;
>
> return val;
> }
>
> -static noinline void gfar_update_link_state(struct gfar_private *priv)
> +static void gfar_mac_config(struct phylink_config *config, unsigned int mode,
> + const struct phylink_link_state *state)
> {
> + struct gfar_private *priv = netdev_priv(to_net_dev(config->dev));
> struct gfar __iomem *regs = priv->gfargrp[0].regs;
> - struct net_device *ndev = priv->ndev;
> - struct phy_device *phydev = ndev->phydev;
> - struct gfar_priv_rx_q *rx_queue = NULL;
> - int i;
> + u32 maccfg1, new_maccfg1;
> + u32 maccfg2, new_maccfg2;
> + u32 ecntrl, new_ecntrl;
> + u32 tx_flow, new_tx_flow;
Don't introduce new_ variables. Is there any issue if you
unconditionally write to the MAC registers?
>
> if (unlikely(test_bit(GFAR_RESETTING, &priv->state)))
> return;
>
> - if (phydev->link) {
> - u32 tempval1 = gfar_read(®s->maccfg1);
> - u32 tempval = gfar_read(®s->maccfg2);
> - u32 ecntrl = gfar_read(®s->ecntrl);
> - u32 tx_flow_oldval = (tempval1 & MACCFG1_TX_FLOW);
> + if (unlikely(phylink_autoneg_inband(mode)))
> + return;
>
> - if (phydev->duplex != priv->oldduplex) {
> - if (!(phydev->duplex))
> - tempval &= ~(MACCFG2_FULL_DUPLEX);
> - else
> - tempval |= MACCFG2_FULL_DUPLEX;
> + maccfg1 = gfar_read(®s->maccfg1);
> + maccfg2 = gfar_read(®s->maccfg2);
> + ecntrl = gfar_read(®s->ecntrl);
>
> - priv->oldduplex = phydev->duplex;
> - }
> + new_maccfg2 = maccfg2 & ~(MACCFG2_FULL_DUPLEX | MACCFG2_IF);
> + new_ecntrl = ecntrl & ~ECNTRL_R100;
>
> - if (phydev->speed != priv->oldspeed) {
> - switch (phydev->speed) {
> - case 1000:
> - tempval =
> - ((tempval & ~(MACCFG2_IF)) | MACCFG2_GMII);
> + if (state->duplex)
> + new_maccfg2 |= MACCFG2_FULL_DUPLEX;
>
> - ecntrl &= ~(ECNTRL_R100);
> - break;
> - case 100:
> - case 10:
> - tempval =
> - ((tempval & ~(MACCFG2_IF)) | MACCFG2_MII);
> -
> - /* Reduced mode distinguishes
> - * between 10 and 100
> - */
> - if (phydev->speed == SPEED_100)
> - ecntrl |= ECNTRL_R100;
> - else
> - ecntrl &= ~(ECNTRL_R100);
> - break;
> - default:
> - netif_warn(priv, link, priv->ndev,
> - "Ack! Speed (%d) is not 10/100/1000!\n",
> - phydev->speed);
Please 1. remove "Ack!" 2. treat SPEED_UNKNOWN here by setting the MAC
into a low-power state (e.g. 10 Mbps - the power savings are real).
Don't print that Speed -1 is not 10/100/1000, we know that.
> - break;
> - }
> -
> - priv->oldspeed = phydev->speed;
> - }
> -
> - tempval1 &= ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
> - tempval1 |= gfar_get_flowctrl_cfg(priv);
> -
> - /* Turn last free buffer recording on */
> - if ((tempval1 & MACCFG1_TX_FLOW) && !tx_flow_oldval) {
> - for (i = 0; i < priv->num_rx_queues; i++) {
> - u32 bdp_dma;
> -
> - rx_queue = priv->rx_queue[i];
> - bdp_dma = gfar_rxbd_dma_lastfree(rx_queue);
> - gfar_write(rx_queue->rfbptr, bdp_dma);
> - }
> -
> - priv->tx_actual_en = 1;
> - }
> -
> - if (unlikely(!(tempval1 & MACCFG1_TX_FLOW) && tx_flow_oldval))
> - priv->tx_actual_en = 0;
> -
> - gfar_write(®s->maccfg1, tempval1);
> - gfar_write(®s->maccfg2, tempval);
> - gfar_write(®s->ecntrl, ecntrl);
> -
> - if (!priv->oldlink)
> - priv->oldlink = 1;
> -
> - } else if (priv->oldlink) {
> - priv->oldlink = 0;
> - priv->oldspeed = 0;
> - priv->oldduplex = -1;
> + switch (state->speed) {
> + case SPEED_1000:
> + new_maccfg2 |= MACCFG2_GMII;
> + break;
> + case SPEED_100:
> + new_maccfg2 |= MACCFG2_MII;
> + new_ecntrl = ecntrl | ECNTRL_R100;
> + break;
> + case SPEED_10:
> + new_maccfg2 |= MACCFG2_MII;
> + break;
> + default:
> + netif_warn(priv, link, priv->ndev,
> + "Ack! Speed (%d) is not 10/100/1000!\n",
> + state->speed);
> + return;
> }
>
> - if (netif_msg_link(priv))
> - phy_print_status(phydev);
> + priv->speed = state->speed;
> +
> + new_maccfg1 = maccfg1 & ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
> + new_maccfg1 |= gfar_get_flowctrl_cfg(state);
> +
> + /* Turn last free buffer recording on */
> + tx_flow = maccfg1 & MACCFG1_TX_FLOW;
> + new_tx_flow = new_maccfg1 & MACCFG1_TX_FLOW;
> + if (new_tx_flow && !tx_flow) {
> + int i;
> +
> + for (i = 0; i < priv->num_rx_queues; i++) {
> + struct gfar_priv_rx_q *rx_queue;
> + u32 bdp_dma;
> +
> + rx_queue = priv->rx_queue[i];
> + bdp_dma = gfar_rxbd_dma_lastfree(rx_queue);
> + gfar_write(rx_queue->rfbptr, bdp_dma);
> + }
> +
> + priv->tx_actual_en = 1;
> + } else if (unlikely(!new_tx_flow && tx_flow)) {
> + priv->tx_actual_en = 0;
> + }
> +
> + if (new_maccfg1 != maccfg1)
> + gfar_write(®s->maccfg1, new_maccfg1);
> + if (new_maccfg2 != maccfg2)
> + gfar_write(®s->maccfg2, new_maccfg2);
> + if (new_ecntrl != ecntrl)
> + gfar_write(®s->ecntrl, new_ecntrl);
> +}
> +
> +static void gfar_mac_an_restart(struct phylink_config *config)
> +{
> + /* Not supported */
> +}
What about running gfar_configure_serdes again?
> +
> +static void gfar_mac_link_down(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface)
> +{
> + /* Not supported */
> +}
> +
What about disabling RX_EN and TX_EN from MACCFG1?
> +static void gfar_mac_link_up(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface, struct phy_device *phy)
> +{
> + /* Not supported */
> }
>
What about enabling RX_EN and TX_EN from MACCFG1?
> static const struct of_device_id gfar_match[] =
> diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
> index f2af96349c7b..0b28b1f60f2d 100644
> --- a/drivers/net/ethernet/freescale/gianfar.h
> +++ b/drivers/net/ethernet/freescale/gianfar.h
> @@ -31,8 +31,7 @@
> #include <linux/skbuff.h>
> #include <linux/spinlock.h>
> #include <linux/mm.h>
> -#include <linux/mii.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
>
> #include <asm/io.h>
> #include <asm/irq.h>
> @@ -149,8 +148,13 @@ extern const char gfar_driver_version[];
> #define GFAR_SUPPORTED_GBIT SUPPORTED_1000baseT_Full
>
> /* TBI register addresses */
> +#define MII_TBI_CR 0x00
> #define MII_TBICON 0x11
>
> +/* TBI_CR register bit fields */
> +#define TBI_CR_FULL_DUPLEX 0x0100
> +#define TBI_CR_SPEED_1000_MASK 0x0040
> +
I think BIT() definitions are preferred, even if that means you have
to convert existing code first.
> /* TBICON register bit fields */
> #define TBICON_CLK_SELECT 0x0020
>
> @@ -1148,12 +1152,12 @@ struct gfar_private {
>
> /* PHY stuff */
> phy_interface_t interface;
> - struct device_node *phy_node;
> + struct device_node *device_node;
> struct device_node *tbi_node;
> - struct mii_bus *mii_bus;
> - int oldspeed;
> - int oldduplex;
> - int oldlink;
> + struct phylink *phylink;
> + struct phylink_config phylink_config;
> + struct phy_device *tbi_phy;
> + int speed;
>
> uint32_t msg_enable;
>
> @@ -1165,11 +1169,7 @@ struct gfar_private {
> bd_stash_en:1,
> rx_filer_enable:1,
> /* Enable priorty based Tx scheduling in Hw */
> - prio_sched_en:1,
> - /* Flow control flags */
> - pause_aneg_en:1,
> - tx_pause_en:1,
> - rx_pause_en:1;
> + prio_sched_en:1;
>
> /* The total tx and rx ring size for the enabled queues */
> unsigned int total_tx_ring_size;
> @@ -1333,8 +1333,6 @@ void reset_gfar(struct net_device *dev);
> void gfar_mac_reset(struct gfar_private *priv);
> void gfar_halt(struct gfar_private *priv);
> void gfar_start(struct gfar_private *priv);
> -void gfar_phy_test(struct mii_bus *bus, struct phy_device *phydev, int enable,
> - u32 regnum, u32 read);
> void gfar_configure_coalescing_all(struct gfar_private *priv);
> int gfar_set_features(struct net_device *dev, netdev_features_t features);
>
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 3433b46b90c1..146b30d07789 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -35,7 +35,7 @@
> #include <asm/types.h>
> #include <linux/ethtool.h>
> #include <linux/mii.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
> #include <linux/sort.h>
> #include <linux/if_vlan.h>
> #include <linux/of_platform.h>
> @@ -207,12 +207,10 @@ static void gfar_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> static unsigned int gfar_usecs2ticks(struct gfar_private *priv,
> unsigned int usecs)
> {
> - struct net_device *ndev = priv->ndev;
> - struct phy_device *phydev = ndev->phydev;
Are you sure this still works? You missed a ndev->phydev check from
gfar_gcoalesce, where this is called from. Technically you can still
check ndev->phydev, it's just that PHYLINK doesn't guarantee you'll
have one (e.g. fixed-link interfaces).
> unsigned int count;
>
> /* The timer is different, depending on the interface speed */
> - switch (phydev->speed) {
> + switch (priv->speed) {
What about retrieving the speed from phylink_ethtool_ksettings_get,
and using that here? This would save you from introducing a cached
priv->speed.
> case SPEED_1000:
> count = GFAR_GBIT_TIME;
> break;
> @@ -234,12 +232,10 @@ static unsigned int gfar_usecs2ticks(struct gfar_private *priv,
> static unsigned int gfar_ticks2usecs(struct gfar_private *priv,
> unsigned int ticks)
> {
> - struct net_device *ndev = priv->ndev;
> - struct phy_device *phydev = ndev->phydev;
> unsigned int count;
>
> /* The timer is different, depending on the interface speed */
> - switch (phydev->speed) {
> + switch (priv->speed) {
Same comments as above, but for gfar_scoalesce.
> case SPEED_1000:
> count = GFAR_GBIT_TIME;
> break;
> @@ -489,58 +485,15 @@ static void gfar_gpauseparam(struct net_device *dev,
> {
> struct gfar_private *priv = netdev_priv(dev);
>
> - epause->autoneg = !!priv->pause_aneg_en;
> - epause->rx_pause = !!priv->rx_pause_en;
> - epause->tx_pause = !!priv->tx_pause_en;
> + phylink_ethtool_get_pauseparam(priv->phylink, epause);
> }
>
> static int gfar_spauseparam(struct net_device *dev,
> struct ethtool_pauseparam *epause)
> {
> struct gfar_private *priv = netdev_priv(dev);
> - struct phy_device *phydev = dev->phydev;
> - struct gfar __iomem *regs = priv->gfargrp[0].regs;
>
> - if (!phydev)
> - return -ENODEV;
> -
> - if (!phy_validate_pause(phydev, epause))
> - return -EINVAL;
> -
> - priv->rx_pause_en = priv->tx_pause_en = 0;
> - phy_set_asym_pause(phydev, epause->rx_pause, epause->tx_pause);
> - if (epause->rx_pause) {
> - priv->rx_pause_en = 1;
> -
> - if (epause->tx_pause) {
> - priv->tx_pause_en = 1;
> - }
> - } else if (epause->tx_pause) {
> - priv->tx_pause_en = 1;
> - }
> -
> - if (epause->autoneg)
> - priv->pause_aneg_en = 1;
> - else
> - priv->pause_aneg_en = 0;
> -
> - if (!epause->autoneg) {
> - u32 tempval = gfar_read(®s->maccfg1);
> -
> - tempval &= ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
> -
> - priv->tx_actual_en = 0;
> - if (priv->tx_pause_en) {
> - priv->tx_actual_en = 1;
> - tempval |= MACCFG1_TX_FLOW;
> - }
> -
> - if (priv->rx_pause_en)
> - tempval |= MACCFG1_RX_FLOW;
> - gfar_write(®s->maccfg1, tempval);
> - }
> -
> - return 0;
> + return phylink_ethtool_set_pauseparam(priv->phylink, epause);
> }
>
> int gfar_set_features(struct net_device *dev, netdev_features_t features)
> @@ -1519,6 +1472,24 @@ static int gfar_get_ts_info(struct net_device *dev,
> return 0;
> }
>
> +/* Set link ksettings (phy address, speed) for ethtools */
ethtool, not ethtools. Also, I'm not quite sure what you mean by
setting the "phy address" with ethtool.
> +static int gfar_get_link_ksettings(struct net_device *dev,
> + struct ethtool_link_ksettings *cmd)
> +{
> + struct gfar_private *priv = netdev_priv(dev);
> +
> + return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> +}
> +
> +/* Get link ksettings for ethtools */
> +static int gfar_set_link_ksettings(struct net_device *dev,
> + const struct ethtool_link_ksettings *cmd)
> +{
> + struct gfar_private *priv = netdev_priv(dev);
> +
> + return phylink_ethtool_ksettings_set(priv->phylink, cmd);
> +}
> +
> const struct ethtool_ops gfar_ethtool_ops = {
> .get_drvinfo = gfar_gdrvinfo,
> .get_regs_len = gfar_reglen,
> @@ -1542,6 +1513,6 @@ const struct ethtool_ops gfar_ethtool_ops = {
> .set_rxnfc = gfar_set_nfc,
> .get_rxnfc = gfar_get_nfc,
> .get_ts_info = gfar_get_ts_info,
> - .get_link_ksettings = phy_ethtool_get_link_ksettings,
> - .set_link_ksettings = phy_ethtool_set_link_ksettings,
> + .get_link_ksettings = gfar_get_link_ksettings,
> + .set_link_ksettings = gfar_set_link_ksettings,
> };
> --
> 2.22.0
>
Thanks a lot for doing this!
-Vladimir
^ 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