* b43 / b43 legacy dma mask cleanups
From: Christoph Hellwig @ 2019-06-25 10:29 UTC (permalink / raw)
To: Larry Finger, Kalle Valo; +Cc: b43-dev, linux-wireless, netdev, linux-kernel
Hi all,
below are a few cleanups for the DMA mask handling. I came up with these
untested patches after looking through the code to debug the 32-bit pmac
30-bit dma issue involving b43legacy.
^ permalink raw reply
* Re: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface
From: Russell King - ARM Linux admin @ 2019-06-25 10:34 UTC (permalink / raw)
To: Parshuram Raju Thombare
Cc: andrew@lunn.ch, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <SN2PR07MB24800C63DCBC143B3A802A6EC1E30@SN2PR07MB2480.namprd07.prod.outlook.com>
On Tue, Jun 25, 2019 at 09:38:37AM +0000, Parshuram Raju Thombare wrote:
>
> >> >In which case, gem_phylink_validate() must clear the support mask when
> >> >SGMII mode is requested to indicate that the interface mode is not
> >> >supported.
> >> >The same goes for _all_ other PHY link modes that the hardware does not
> >> >actually support, such as PHY_INTERFACE_MODE_10GKR...
> >> If interface is not supported by hardware probe returns with error, so we don't
> >> net interface is not registered at all.
> >That does not negate my comment above.
> Sorry if I misunderstood your question, but hardware supports interfaces and based
> on that link modes are supported. So if interface is not supported by hardware,
> net device is not registered and there will be no phylink_validate call.
> If hardware support particular interface, link modes supported by interface
> are added to (not cleared from) supported mask, provided configs is not trying to limit
> data rate using GIGABIT_ENABLE* macro.
Why do you want to use phylink?
If you are only interested in supporting 10G PHYs, you don't need
phylink for that.
If you are interested in supporting SFPs as well, then using phylink
makes sense, but you need to implement your phylink conversion properly,
and that means supporting dynamic switching of the PHY interface mode,
and allowing phylink to determine whether a PHY interface mode is
supported or not.
However, with what you've indicated through our discussion, your MAC
does not support BASE-X modes, nor does it support 10GBASE-R, both of
which are required for direct connection of SFP or SFP+ modules.
The only phy link mode that you support which SFPs can make use of is
SGMII, and that will only be useful for copper SFPs configured for
SGMII mode. It basically means you can not use any fiber SFPs.
The only other way SFPs can be supported is via an intermediary PHY to
convert the MAC interface to BASE-X, SGMII or 10GBASE-R, and we don't
yet have support for that in mainline.
So, given that you seem unwilling to take on board my comments, and
your hardware does not appear support SFPs, I'm wondering what the
point of this conversion actually is.
As a result of our reviews, I've been improving the documentation for
phylink, so there has been some positives coming out of this - which
will hopefully help others to avoid the same mistakes.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH] let proc net directory inodes reflect to active net namespace
From: Hallsmark, Per @ 2019-06-25 10:36 UTC (permalink / raw)
To: Alexey Dobriyan, David S. Miller
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Hallsmark, Per
[-- Attachment #1: Type: text/plain, Size: 6364 bytes --]
Hi,
Linux kernel recently got a bugfix 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)"),
but unfortunately it only solves the issue for procfs net file inodes so they get correct
content after a process change namespace.
Checking on a v5.2-rc6 kernel :
sh-4.4# sh netns_procfs_test.sh
[ 16.451640] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
eth0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
sit0: 0 0 0 0 0 0 0 0 0 0
if_other: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
This kernel is fixed for file inode bug but suffers dir inode bug
sh-4.4#
As can be seen above, after the namespace change we see new content in procfs net/dev
but the listing of procfs net/dev_snmp6 still shows entries from previous namespace.
We need to apply similar bugfix for directory creation in procfs net as the mentioned
commit do for files.
Checking on a v5.2-rc6 kernel with bugfixes :
sh-4.4# sh netns_procfs_test.sh
[ 745.993882] ip (108) used greatest stack depth: 12264 bytes left
Before net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
lo: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
eth0: 0 0 0 0 0 0 0 0 0 0
if_default: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
eth0
sit0
if_default
After net namespace change :
==== /proc/net/dev ====
Inter-| Receive | Transmit
face |bytes packets errs drop fifo frame compressed multicast|bytes packd
if_other: 0 0 0 0 0 0 0 0 0 0
sit0: 0 0 0 0 0 0 0 0 0 0
lo: 0 0 0 0 0 0 0 0 0 0
==== files in /proc/net/dev_snmp6 ====
.
..
lo
sit0
if_other
This kernel is fixed for both file and dir inode bug
sh-4.4#
Here we see that the directory procfs net/dev_snmp6 is updated according to the namespace
change.
The fix is two commits, first updates proc_net_mkdir() entries similar to mentioned patch
and second one is changing net/ipv6/proc.c to use proc_net_mkdir() instead.
Speaking about proc_net_mkdir()...
[phallsma@arn-phallsma-l3 linux]$ git grep proc_mkdir | grep proc_net
drivers/isdn/divert/divert_procfs.c: isdn_proc_entry = proc_mkdir("isdn", init_net.proc_net);
drivers/isdn/hysdn/hysdn_procconf.c: hysdn_proc_entry = proc_mkdir(PROC_SUBDIR_NAME, init_net.proc_net);
drivers/net/bonding/bond_procfs.c: bn->proc_dir = proc_mkdir(DRV_NAME, bn->net->proc_net);
drivers/net/wireless/intel/ipw2x00/libipw_module.c: libipw_proc = proc_mkdir(DRV_PROCNAME, init_net.proc_net);
drivers/net/wireless/intersil/hostap/hostap_main.c: hostap_proc = proc_mkdir("hostap", init_net.proc_net);
drivers/staging/rtl8192u/ieee80211/ieee80211_module.c: ieee80211_proc = proc_mkdir(DRV_NAME, init_net.proc_net);
drivers/staging/rtl8192u/r8192U_core.c: rtl8192_proc = proc_mkdir(RTL819XU_MODULE_NAME, init_net.proc_net);
net/appletalk/atalk_proc.c: if (!proc_mkdir("atalk", init_net.proc_net))
net/core/pktgen.c: pn->proc_dir = proc_mkdir(PG_PROC_DIR, pn->net->proc_net);
net/ipv4/netfilter/ipt_CLUSTERIP.c: cn->procdir = proc_mkdir("ipt_CLUSTERIP", net->proc_net);
net/ipv6/proc.c: net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
net/llc/llc_proc.c: llc_proc_dir = proc_mkdir("llc", init_net.proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ipt_hashlimit = proc_mkdir("ipt_hashlimit", net->proc_net);
net/netfilter/xt_hashlimit.c: hashlimit_net->ip6t_hashlimit = proc_mkdir("ip6t_hashlimit", net->proc_net);
net/netfilter/xt_recent.c: recent_net->xt_recent = proc_mkdir("xt_recent", net->proc_net);
net/sunrpc/cache.c: cd->procfs = proc_mkdir(cd->name, sn->proc_net_rpc);
net/sunrpc/stats.c: sn->proc_net_rpc = proc_mkdir("rpc", net->proc_net);
net/x25/x25_proc.c: if (!proc_mkdir("x25", init_net.proc_net))
[phallsma@arn-phallsma-l3 linux]$
IMHO all code should use proc_net_mkdir() instead of proc_mkdir() for procfs net entries,
or am I missing something here? If not possible to changeover to proc_net_mkdir() there
is a need for duplicating my first commit at those places. I'm fixing the one for dev_snmp6()
which is what I've tested as well.
Also wonder if it all is optimal. Wouldn't it be better to re-enable dcache for these (files as well as directories)
and in addition have kernel drop dcache in case of a namespace change?
Attaching patches and app/script for verifying.
I'm not on the mailing lists so please keep me on CC in case of responding.
Best regards,
Per
--
Per Hallsmark per.hallsmark@windriver.com
Senior Member Technical Staff Wind River AB
Mobile: +46733249340 Office: +46859461127
Torshamnsgatan 27 164 40 Kista
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch --]
[-- Type: text/x-patch; name="0001-Make-directory-inodes-in-proc-net-adhere-to-net-name.patch", Size: 1895 bytes --]
From 66769af1f931124da1aa04b34350b3623a9003e3 Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Wed, 19 Jun 2019 15:46:39 +0200
Subject: [PATCH 1/2] Make directory inodes in /proc/net adhere to net
namespace
This patch fixes /proc/net directory inodes in similar way as
commit 1fde6f21d90f ("proc: fix /proc/net/* after setns(2)")
fixes file inodes.
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
fs/proc/proc_net.c | 12 ++++++++++++
include/linux/proc_fs.h | 7 ++-----
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 76ae278df1c4..1c4231a60c9e 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -55,6 +55,18 @@ static void pde_force_lookup(struct proc_dir_entry *pde)
pde->proc_dops = &proc_net_dentry_ops;
}
+struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
+ struct proc_dir_entry *parent)
+{
+ struct proc_dir_entry *pde;
+
+ pde = proc_mkdir_data(name, 0, parent, net);
+ pde->proc_dops = &proc_net_dentry_ops;
+
+ return pde;
+}
+EXPORT_SYMBOL(proc_net_mkdir);
+
static int seq_open_net(struct inode *inode, struct file *file)
{
unsigned int state_size = PDE(inode)->state_size;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 52a283ba0465..608b8a10e338 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -124,11 +124,8 @@ static inline struct pid *tgid_pidfd_to_pid(const struct file *file)
struct net;
-static inline struct proc_dir_entry *proc_net_mkdir(
- struct net *net, const char *name, struct proc_dir_entry *parent)
-{
- return proc_mkdir_data(name, 0, parent, net);
-}
+extern struct proc_dir_entry *proc_net_mkdir(
+ struct net *net, const char *name, struct proc_dir_entry *parent);
struct ns_common;
int open_related_ns(struct ns_common *ns,
--
2.20.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-net-Directories-created-in-proc-net-should-be-done-v.patch --]
[-- Type: text/x-patch; name="0002-net-Directories-created-in-proc-net-should-be-done-v.patch", Size: 976 bytes --]
From 6925a5408ffce37dc71871c2ee05db96e60cae0d Mon Sep 17 00:00:00 2001
From: Per Hallsmark <per.hallsmark@windriver.com>
Date: Thu, 20 Jun 2019 10:21:31 +0200
Subject: [PATCH 2/2] net: Directories created in /proc/net should be done via
proc_net_mkdir()
Directories created in /proc/net should be done via proc_net_mkdir()
Signed-off-by: Per Hallsmark <per.hallsmark@windriver.com>
---
net/ipv6/proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 4a8da679866e..3728c57e93dc 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -282,7 +282,8 @@ static int __net_init ipv6_proc_init_net(struct net *net)
snmp6_seq_show, NULL))
goto proc_snmp6_fail;
- net->mib.proc_net_devsnmp6 = proc_mkdir("dev_snmp6", net->proc_net);
+ net->mib.proc_net_devsnmp6 = proc_net_mkdir(net,
+ "dev_snmp6", net->proc_net);
if (!net->mib.proc_net_devsnmp6)
goto proc_dev_snmp6_fail;
return 0;
--
2.20.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: netns_procfs_test.c --]
[-- Type: text/x-csrc; name="netns_procfs_test.c", Size: 3920 bytes --]
/*
Verifier of /proc/net and net namespace consistency
Author : Per Hallsmark <per.hallsmark@windriver.com>
First setup a net namespace and add a veth so we get something to test with
ip netns add netns_other
ip link add if_default type veth peer name if_other
ip link set if_other netns netns_other
Run test app without first reading some info from /proc/net in default namespace
./netns_procfs_test
Run test app again, this time reading some info from /proc/net in default namespace
before changing net namespace
./netns_procfs_test cached
On a bugfree kernel, the output after "After net namespace change" should be same as
in first test run, meaning we should see if_other in the /proc/net/dev dump and we
should see if_other in the directory.
The issue is not visible if it is different processes doing the readings since then
the inode's aren't cached.
*/
#define _GNU_SOURCE
#include <sched.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include <fcntl.h>
#include <errno.h>
#include <limits.h>
#include <sys/types.h>
#include <sys/mount.h>
#include <errno.h>
#include <dirent.h>
#define NETNS_RUN_DIR "/var/run/netns"
static int get_nsfd(const char *ns)
{
char path[strlen(NETNS_RUN_DIR) + strlen(ns) + 2];
int fd;
snprintf(path, sizeof(path), "%s/%s", NETNS_RUN_DIR, ns);
fd = open(path, O_RDONLY, 0);
return fd;
}
static int dump_file(const char *fn, const int checkforbug)
{
int fd;
char buf[512];
ssize_t len;
char *wholebuf = NULL;
ssize_t wholelen = 0;
int status = 0;
printf("==== %s ====\n", fn);
fd = open(fn, O_RDONLY);
if (fd == -1) {
printf("Couldn't open %s (%s)\n", fn, strerror(errno));
return -1;
}
do {
len = read(fd, buf, sizeof(buf)-1);
buf[len] = 0;
wholebuf = realloc(wholebuf, wholelen + len + 1);
sprintf(&wholebuf[wholelen], "%s", buf);
wholelen += len;
} while (len > 0);
printf("%s\n", wholebuf);
if (checkforbug && strstr(wholebuf, "if_other")) {
status = 1;
}
free(wholebuf);
close(fd);
return status;
}
static int dump_dir(const char *dn, const int checkforbug)
{
DIR *dir;
struct dirent *dir_entry;
int status = 0;
printf("==== files in %s ====\n", dn);
dir = opendir(dn);
if (!dir) {
printf("Couldn't open %s (%s)\n", dn, strerror(errno));
return -1;
}
while ((dir_entry = readdir(dir)) != NULL) {
printf(" %s", dir_entry->d_name);
if (checkforbug && !strcmp(dir_entry->d_name, "if_other")) {
status = 2;
}
printf("\n");
}
closedir(dir);
return status;
}
static int switch_ns(const char *ns)
{
int nsfd = get_nsfd(ns);
if (setns(nsfd, CLONE_NEWNET) < 0) {
fprintf(stderr, "Unable to switch to namespace(fd=%d): %s.\n",
nsfd, strerror(errno));
exit(-1);
}
return 0;
}
int main (int argc, char **argv)
{
int bugcheck = 0;
int checkforbug = 1;
if (argc == 2 && !strcmp(argv[1], "cached")) {
// access /proc/net a bit so we dcache file and
// directory inodes
printf("Before net namespace change :\n");
dump_file("/proc/net/dev", 0);
dump_dir("/proc/net/dev_snmp6", 0);
} else {
// we cannot check for bug if not run with cached arg
checkforbug = 0;
}
// change namespace
switch_ns("netns_other");
// access /proc/net again, if all works we should see
// new info because of net namespace change
printf("\n\nAfter net namespace change :\n");
bugcheck += dump_file("/proc/net/dev", 1);
bugcheck += dump_dir("/proc/net/dev_snmp6", 1);
if (!checkforbug)
return 0;
switch (bugcheck) {
case 1 :
printf("This kernel is fixed for file inode bug but suffers dir inode bug\n");
break;
case 3 :
printf("This kernel is fixed for both file and dir inode bug\n");
break;
default :
printf("This kernel suffers both file and dir inode bug\n");
}
return bugcheck != 3;
}
[-- Attachment #5: netns_procfs_test.sh --]
[-- Type: application/x-shellscript, Size: 144 bytes --]
^ permalink raw reply related
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Ivan Khoronzhuk @ 2019-06-25 10:50 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce
In-Reply-To: <156086314789.27760.6549333469314693352.stgit@firesoul>
Hi Jesper,
Could you please clarify one question.
On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
>This patch is needed before we can allow drivers to use page_pool for
>DMA-mappings. Today with page_pool and XDP return API, it is possible to
>remove the page_pool object (from rhashtable), while there are still
>in-flight packet-pages. This is safely handled via RCU and failed lookups in
>__xdp_return() fallback to call put_page(), when page_pool object is gone.
>In-case page is still DMA mapped, this will result in page note getting
>correctly DMA unmapped.
>
>To solve this, the page_pool is extended with tracking in-flight pages. And
>XDP disconnect system queries page_pool and waits, via workqueue, for all
>in-flight pages to be returned.
>
>To avoid killing performance when tracking in-flight pages, the implement
>use two (unsigned) counters, that in placed on different cache-lines, and
>can be used to deduct in-flight packets. This is done by mapping the
>unsigned "sequence" counters onto signed Two's complement arithmetic
>operations. This is e.g. used by kernel's time_after macros, described in
>kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
>
>The trick is these two incrementing counters only need to be read and
>compared, when checking if it's safe to free the page_pool structure. Which
>will only happen when driver have disconnected RX/alloc side. Thus, on a
>non-fast-path.
>
>It is chosen that page_pool tracking is also enabled for the non-DMA
>use-case, as this can be used for statistics later.
>
>After this patch, using page_pool requires more strict resource "release",
>e.g. via page_pool_release_page() that was introduced in this patchset, and
>previous patches implement/fix this more strict requirement.
>
>Drivers no-longer call page_pool_destroy(). Drivers already call
>xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
>attempt to disconnect the mem id, and if attempt fails schedule the
>disconnect for later via delayed workqueue.
>
>Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>---
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
> include/net/page_pool.h | 41 ++++++++++---
> net/core/page_pool.c | 62 +++++++++++++++-----
> net/core/xdp.c | 65 +++++++++++++++++++--
> 4 files changed, 136 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>index 2f647be292b6..6c9d4d7defbc 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> }
>
> xdp_rxq_info_unreg(&rq->xdp_rxq);
>- if (rq->page_pool)
>- page_pool_destroy(rq->page_pool);
>-
> mlx5_wq_destroy(&rq->wq_ctrl);
> }
>
>diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>index 754d980700df..f09b3f1994e6 100644
>--- a/include/net/page_pool.h
>+++ b/include/net/page_pool.h
>@@ -16,14 +16,16 @@
> * page_pool_alloc_pages() call. Drivers should likely use
> * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
> *
>- * If page_pool handles DMA mapping (use page->private), then API user
>- * is responsible for invoking page_pool_put_page() once. In-case of
>- * elevated refcnt, the DMA state is released, assuming other users of
>- * the page will eventually call put_page().
>+ * API keeps track of in-flight pages, in-order to let API user know
>+ * when it is safe to dealloactor page_pool object. Thus, API users
>+ * must make sure to call page_pool_release_page() when a page is
>+ * "leaving" the page_pool. Or call page_pool_put_page() where
>+ * appropiate. For maintaining correct accounting.
> *
>- * If no DMA mapping is done, then it can act as shim-layer that
>- * fall-through to alloc_page. As no state is kept on the page, the
>- * regular put_page() call is sufficient.
>+ * API user must only call page_pool_put_page() once on a page, as it
>+ * will either recycle the page, or in case of elevated refcnt, it
>+ * will release the DMA mapping and in-flight state accounting. We
>+ * hope to lift this requirement in the future.
> */
> #ifndef _NET_PAGE_POOL_H
> #define _NET_PAGE_POOL_H
>@@ -66,9 +68,10 @@ struct page_pool_params {
> };
>
> struct page_pool {
>- struct rcu_head rcu;
> struct page_pool_params p;
>
>+ u32 pages_state_hold_cnt;
>+
> /*
> * Data structure for allocation side
> *
>@@ -96,6 +99,8 @@ struct page_pool {
> * TODO: Implement bulk return pages into this structure.
> */
> struct ptr_ring ring;
>+
>+ atomic_t pages_state_release_cnt;
> };
>
> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>
> struct page_pool *page_pool_create(const struct page_pool_params *params);
>
>-void page_pool_destroy(struct page_pool *pool);
>-
> void __page_pool_free(struct page_pool *pool);
> static inline void page_pool_free(struct page_pool *pool)
> {
>@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> __page_pool_put_page(pool, page, true);
> }
>
>+/* API user MUST have disconnected alloc-side (not allowed to call
>+ * page_pool_alloc_pages()) before calling this. The free-side can
>+ * still run concurrently, to handle in-flight packet-pages.
>+ *
>+ * A request to shutdown can fail (with false) if there are still
>+ * in-flight packet-pages.
>+ */
>+bool __page_pool_request_shutdown(struct page_pool *pool);
>+static inline bool page_pool_request_shutdown(struct page_pool *pool)
>+{
>+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
>+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>+ */
>+#ifdef CONFIG_PAGE_POOL
>+ return __page_pool_request_shutdown(pool);
>+#endif
>+}
The free side can ran in softirq, that means fast cache recycle is accessed.
And it increments not atomic pool->alloc.count.
For instance While redirect, for remote interface, while .ndo_xdp_xmit the
xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
In the same time, simultaneously, the work queue can try one more time to clear
cash, calling __page_pool_request_shutdown()....
Question, what prevents pool->alloc.count to be corrupted by race,
causing to wrong array num and as result wrong page to be unmapped/put ....or
even page leak. alloc.count usage is not protected,
__page_pool_request_shutdown() is called not from same rx NAPI, even not from
NAPI.
Here, while alloc cache empty procedure in __page_pool_request_shutdown():
while (pool->alloc.count) {
page = pool->alloc.cache[--pool->alloc.count];
__page_pool_return_page(pool, page);
}
For me seems all works fine, but I can't find what have I missed?
...
Same question about how xdp frame should be returned for drivers running
tx napi exclusively, it can be still softirq but another CPU? What API
should be used to return xdp frame.
[...]
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH v4 4/5] net: macb: add support for high speed interface
From: Russell King - ARM Linux admin @ 2019-06-25 10:51 UTC (permalink / raw)
To: Parshuram Raju Thombare
Cc: Andrew Lunn, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <SN2PR07MB2480DBE64F2550C0135335EEC1E30@SN2PR07MB2480.namprd07.prod.outlook.com>
On Tue, Jun 25, 2019 at 08:26:29AM +0000, Parshuram Raju Thombare wrote:
> Hi Andrew,
>
> >What i'm saying is that the USXGMII rate is fixed. So why do you need a device
> >tree property for the SERDES rate?
> This is based on Cisco USXGMII specification, it specify USXGMII 5G and USXGMII 10G.
> Sorry I can't share that document here.
The closed nature of the USXGMII spec makes it very hard for us to know
whether your implementation is correct or not.
I have some documentation which suggests that USVGMII is a USXGMII link
running at "5GR" rate as opposed to USXGMII running at "10GR" rate.
So, I think 5G mode should be left out until it becomes clear that (a)
we should specify it as USXGMII with a 5G rate, or as USVGMII.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Sergei Shtylyov @ 2019-06-25 10:51 UTC (permalink / raw)
To: Jose Abreu, linux-kernel, netdev
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <895a67c1-3b83-d7be-b64e-61cff86d057d@cogentembedded.com>
On 25.06.2019 13:29, Sergei Shtylyov wrote:
>> Some DT bindings do not have the PHY handle. Let's fallback to manually
>> discovery in case phylink_of_phy_connect() fails.
>>
>> Reported-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>> Fixes: 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic")
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>> Hello Katsuhiro,
>>
>> Can you please test this patch ?
>> ---
>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index a48751989fa6..f4593d2d9d20 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -950,9 +950,12 @@ static int stmmac_init_phy(struct net_device *dev)
>> node = priv->plat->phylink_node;
>> - if (node) {
>> + if (node)
>> ret = phylink_of_phy_connect(priv->phylink, node, 0);
>> - } else {
>> +
>> + /* Some DT bindings do not set-up the PHY handle. Let's try to
>> + * manually parse it */
>
> The multi-line comments inb the networking code should be formatted like
> below:
>
> /*
> * bla
Oops, that was the general comment format, networking code starts without
the leading empty line:\
/* bla
> * bla
> */
[...]
MBR, Sergei
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Jamal Hadi Salim @ 2019-06-25 10:55 UTC (permalink / raw)
To: Eric Dumazet, Joe Stringer, Florian Westphal
Cc: netdev, john fastabend, Daniel Borkmann, Lorenz Bauer,
Jakub Sitnicki, Paolo Abeni
In-Reply-To: <d2692f14-6ac7-1335-3359-d397fbe1676f@gmail.com>
On 2019-06-24 12:49 p.m., Eric Dumazet wrote:
>
>
> Well, I would simply remove the skb_orphan() call completely.
My experience: You still need to call skb_orphan() from the lower level
(and set the skb destructor etc).
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-25 11:02 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-L2dgypSCTDwdEXa7EUYyWTcD_hLwW_kuUkk0tA_iggDw@mail.gmail.com>
On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > + po->wait_on_complete = 1;
> > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > >
> > > This resets timeout on every loop. should only set above the loop once.
> > >
> > I explained exactly why I did that in the change log. Its because I reuse the
> > timeout variable to get the return value of the wait_for_complete call.
> > Otherwise I need to add additional data to the stack, which I don't want to do.
> > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > path, so I see no reason not to reuse the variable.
>
> The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> each time. Whereas wait_for_common and its variants return the
> number of jiffies left in case the loop needs to sleep again later.
>
> Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> common pattern across the stack.
>
But those patterns are unique to those situations. For instance, in
tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
wait for the entire message send operation to complete, which consists of
potentially several blocking operations (waiting for the tcp connection to be
established, waiting for socket memory, etc). In that situation we want to wait
for all of those operations to complete to send a single message, and fail if
they exceed the timeout in aggregate. The semantics are different with
AF_PACKET. In this use case, the message is in effect empty, and just used to
pass some control information. tpacket_snd, sends as many frames from the
memory mapped buffer as possible, and on each iteration we want to wait for the
specified timeout for those frames to complete sending. I think resetting the
timeout on each wait instance is the right way to go here.
> > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > err = net_xmit_errno(err);
> > > > if (err && __packet_get_status(po, ph) ==
> > > > TP_STATUS_AVAILABLE) {
> > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > + */
> > > > + po->wait_on_complete = 0;
> > >
> > > If setting where sleeping, no need for resetting if a failure happens
> > > between those blocks.
> > >
> > > > + init_completion(&po->skb_completion);
> > >
> > > no need to reinit between each use?
> > >
> > I explained exactly why I did this in the comment above. We have to set
> > wait_for_complete prior to calling transmit, so as to ensure that we call
> > wait_for_completion before we exit the loop. However, in this error case, we
> > exit the loop prior to calling wait_for_complete, so we need to reset the
> > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > case where, on the next entrace to this loop we will have a completion variable
> > with completion->done > 0, meaning the next wait will be a fall through case,
> > which we don't want.
>
> By moving back to the point where schedule() is called, hopefully this
> complexity automatically goes away. Same as my comment to the line
> immediately above.
>
Its going to change what the complexity is, actually. I was looking at this
last night, and I realized that your assertion that we could remove
packet_next_frame came at a cost. This is because we need to determine if we
have to wait before we call po->xmit, but we need to actually do the wait after
po->xmit (to ensure that wait_on_complete is set properly when the desructor is
called). By moving the wait to the top of the loop and getting rid of
packet_next_frame we now have a race condition in which we might call
tpacket_destruct_skb with wait_on_complete set to false, causing us to wait for
the maximum timeout erroneously. So I'm going to have to find a new way to
signal the need to call complete, which I think will introduce a different level
of complexity.
> > > > @@ -2740,6 +2772,20 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > }
> > > > packet_increment_head(&po->tx_ring);
> > > > len_sum += tp_len;
> > > > +
> > > > + if (po->wait_on_complete) {
> > > > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> > > > + po->wait_on_complete = 0;
> > >
> > > I was going to argue for clearing in tpacket_destruct_skb. But then we
> > > would have to separate clear on timeout instead of signal, too.
> > >
> > > po->wait_on_complete = 1;
> > > timeo = wait_for_completion...
> > > po->wait_on_complete = 0;
> > >
> > Also, we would have a race condition, since the destructor may be called from
> > softirq context (the first cause of the bug I'm fixing here), and so if the
> > packet is freed prior to us checking wait_for_complete in tpacket_snd, we will
> > be in the above situation again, exiting the loop with a completion variable in
> > an improper state.
>
> Good point.
>
> The common pattern is to clear in tpacket_destruct_skb. Then
> we do need to handle the case where the wait is interrupted or
> times out and reset it in those cases.
>
As noted above, if we restore the original control flow, the wait_on_complete
flag is not useable, as its state needs to be determined prior to actually
sending the frame to the driver via po->xmit.
I'm going to try some logic in which both tpacket_snd and tpacket_destruct_skb
key off of packet_read_pending. I think this will require a re-initalization of
the completion queue on each entry to tpacket_snd, but perhaps thats more
pallatable.
> > > This is basically replacing a busy-wait with schedule() with sleeping
> > > using wait_for_completion_interruptible_timeout. My main question is
> > > does this really need to move control flow around and add
> > > packet_next_frame? If not, especially for net, the shortest, simplest
> > > change is preferable.
> > >
> > Its not replacing a busy wait at all, its replacing a non-blocking schedule with
> > a blocking schedule (via completion queues). As for control flow, Im not sure I
> > why you are bound to the existing control flow, and given that we already have
> > packet_previous_frame, I didn't see anything egregious about adding
> > packet_next_frame as well, but since you've seen a way to eliminate it, I'm ok
> > with it.
>
> The benefit of keeping to the existing control flow is that that is a
> smaller change, so easier to verify.
>
I don't think it will really, because its going to introduce different
complexities, but we'll see for certain when I finish getting it rewritten
again.
> I understand the benefit of moving the wait outside the loop. Before
> this report, I was not even aware of that behavior on !MSG_DONTWAIT,
> because it is so co-located.
>
> But moving it elsewhere in the loop does not have the same benefit,
> imho. Either way, I think we better leave any such code improvements
> to net-next and focus on the minimal , least risky, patch for net.
>
Ok.
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Jamal Hadi Salim @ 2019-06-25 11:06 UTC (permalink / raw)
To: Joe Stringer
Cc: Eric Dumazet, Florian Westphal, netdev, john fastabend,
Daniel Borkmann, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni
In-Reply-To: <CAOftzPj_+6hfrb-FwU+E2P83RLLp6dtv0nJizSG1Fw7+vCgYwA@mail.gmail.com>
On 2019-06-24 11:26 p.m., Joe Stringer wrote:
[..]
>
> I haven't got as far as UDP yet, but I didn't see any need for a
> dependency on netfilter.
I'd be curious to see what you did. My experience, even for TCP is
the socket(transparent/tproxy) lookup code (to set skb->sk either
listening or established) is entangled in
CONFIG_NETFILTER_SOMETHING_OR_OTHER. You have to rip it out of
there (in the tproxy tc action into that code). Only then can you
compile out netfilter.
I didnt bother to rip out code for udp case.
i.e if you needed udp to work with the tc action,
youd have to turn on NF. But that was because we had
no need for udp transparent proxying.
IOW:
There is really no reason, afaik, for tproxy code to only be
accessed if netfilter is compiled in. Not sure i made sense.
cheers,
jamal
^ permalink raw reply
* memory leak in genl_register_family
From: syzbot @ 2019-06-25 11:07 UTC (permalink / raw)
To: davem, dsahern, johannes.berg, linux-kernel, marcel, mkubecek,
netdev, syzkaller-bugs, yuehaibing
Hello,
syzbot found the following crash on:
HEAD commit: 4b972a01 Linux 5.2-rc6
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1305b385a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=1db8bd6825f9661c
dashboard link: https://syzkaller.appspot.com/bug?extid=fc577f12f25f2ac3b211
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15bd1385a00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+fc577f12f25f2ac3b211@syzkaller.appspotmail.com
BUG: memory leak
unreferenced object 0xffff88812a7f0c80 (size 64):
comm "swapper/0", pid 1, jiffies 4294937561 (age 881.930s)
hex dump (first 32 bytes):
2f 64 65 76 69 63 65 73 2f 76 69 72 74 75 61 6c /devices/virtual
2f 6e 65 74 2f 6c 6f 2f 71 75 65 75 65 73 2f 74 /net/lo/queues/t
backtrace:
[<00000000d629f5fa>] kmemleak_alloc_recursive
include/linux/kmemleak.h:43 [inline]
[<00000000d629f5fa>] slab_post_alloc_hook mm/slab.h:439 [inline]
[<00000000d629f5fa>] slab_alloc mm/slab.c:3326 [inline]
[<00000000d629f5fa>] __do_kmalloc mm/slab.c:3658 [inline]
[<00000000d629f5fa>] __kmalloc+0x161/0x2c0 mm/slab.c:3669
[<000000003291d450>] kmalloc_array include/linux/slab.h:670 [inline]
[<000000003291d450>] genl_register_family net/netlink/genetlink.c:355
[inline]
[<000000003291d450>] genl_register_family+0x5e1/0x7e0
net/netlink/genetlink.c:322
[<00000000f8e2dd0d>] netlbl_unlabel_genl_init+0x15/0x17
net/netlabel/netlabel_unlabeled.c:1387
[<00000000189b4a0c>] netlbl_netlink_init+0x39/0x46
net/netlabel/netlabel_user.c:65
[<0000000083adc8f0>] netlbl_init+0x65/0xa8
net/netlabel/netlabel_kapi.c:1502
[<000000003a053024>] do_one_initcall+0x5c/0x2ca init/main.c:915
[<00000000d70991fc>] do_initcall_level init/main.c:983 [inline]
[<00000000d70991fc>] do_initcalls init/main.c:991 [inline]
[<00000000d70991fc>] do_basic_setup init/main.c:1009 [inline]
[<00000000d70991fc>] kernel_init_freeable+0x1af/0x26c init/main.c:1169
[<00000000232b80c4>] kernel_init+0x10/0x155 init/main.c:1087
[<000000006b3bb174>] ret_from_fork+0x1f/0x30
arch/x86/entry/entry_64.S:352
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Baruch Siach @ 2019-06-25 11:04 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf,
Dmitry V . Levin, Arnd Bergmann, linux-arch, Baruch Siach,
Jiri Olsa, Geert Uytterhoeven, Linus Torvalds
Merge commit 1c8c5a9d38f60 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
applications") by taking the gpl_compatible 1-bit field definition from
commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
bpf_prog_info") as is. That breaks architectures with 16-bit alignment
like m68k. Embed gpl_compatible into an anonymous union with 32-bit pad
member to restore alignment of following fields.
Thanks to Dmitry V. Levin his analysis of this bug history.
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2:
Use anonymous union with pad to make it less likely to break again in
the future.
---
include/uapi/linux/bpf.h | 5 ++++-
tools/include/uapi/linux/bpf.h | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a8b823c30b43..766eae02d7ae 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3142,7 +3142,10 @@ struct bpf_prog_info {
__aligned_u64 map_ids;
char name[BPF_OBJ_NAME_LEN];
__u32 ifindex;
- __u32 gpl_compatible:1;
+ union {
+ __u32 gpl_compatible:1;
+ __u32 pad;
+ };
__u64 netns_dev;
__u64 netns_ino;
__u32 nr_jited_ksyms;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a8b823c30b43..766eae02d7ae 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3142,7 +3142,10 @@ struct bpf_prog_info {
__aligned_u64 map_ids;
char name[BPF_OBJ_NAME_LEN];
__u32 ifindex;
- __u32 gpl_compatible:1;
+ union {
+ __u32 gpl_compatible:1;
+ __u32 pad;
+ };
__u64 netns_dev;
__u64 netns_ino;
__u32 nr_jited_ksyms;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next 3/3] net: stmmac: Convert to phylink and remove phylib logic
From: Jon Hunter @ 2019-06-25 11:10 UTC (permalink / raw)
To: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Russell King, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, linux-tegra
In-Reply-To: <78EB27739596EE489E55E81C33FEC33A0B9D7024@DE02WEMBXB.internal.synopsys.com>
On 25/06/2019 08:37, Jose Abreu wrote:
> From: Jon Hunter <jonathanh@nvidia.com>
>
>> Any further feedback? I am still seeing this issue on today's -next.
>
> Apologies but I was in FTO.
>
> Is there any possibility you can just disable the ethX configuration in
> the rootfs mount and manually configure it after rootfs is done ?
>
> I just want to make sure in which conditions this is happening (if in
> ifdown or ifup).
I have been looking at this a bit closer and I can see the problem. What
happens is that ...
1. stmmac_mac_link_up() is called and priv->eee_active is set to false
2. stmmac_eee_init() is called but because priv->eee_active is false,
timer_setup() for eee_ctrl_timer is never called.
3. stmmac_eee_init() returns true and so then priv->eee_enabled is set
to true.
4. When stmmac_tx_clean() is called because priv->eee_enabled is set to
true, mod_timer() is called for the eee_ctrl_timer, but because
timer_setup() was never called, we hit the BUG defined at
kernel/time/timer.c:952, because no function is defined for the
timer.
The following fixes it for me ...
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -399,10 +399,13 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
mutex_lock(&priv->lock);
/* Check if it needs to be deactivated */
- if (!priv->eee_active && priv->eee_enabled) {
- netdev_dbg(priv->dev, "disable EEE\n");
- del_timer_sync(&priv->eee_ctrl_timer);
- stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
+ if (!priv->eee_active) {
+ if (priv->eee_enabled) {
+ netdev_dbg(priv->dev, "disable EEE\n");
+ del_timer_sync(&priv->eee_ctrl_timer);
+ stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
+ }
+ mutex_unlock(&priv->lock);
return false;
}
It also looks like you have a potention deadlock in the current code
because in the case of if (!priv->eee_active && priv->eee_enabled)
you don't unlock the mutex. The above fixes this as well. I can send a
formal patch if this looks correct.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH net] sctp: change to hold sk after auth shkey is created successfully
From: Neil Horman @ 2019-06-25 11:18 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner,
syzkaller-bugs
In-Reply-To: <14de0d292dc2fe01ecadaba00feb925b337b558f.1561393305.git.lucien.xin@gmail.com>
On Tue, Jun 25, 2019 at 12:21:45AM +0800, Xin Long wrote:
> Now in sctp_endpoint_init(), it holds the sk then creates auth
> shkey. But when the creation fails, it doesn't release the sk,
> which causes a sk defcnf leak,
>
> Here to fix it by only holding the sk when auth shkey is created
> successfully.
>
> Fixes: a29a5bd4f5c3 ("[SCTP]: Implement SCTP-AUTH initializations.")
> Reported-by: syzbot+afabda3890cc2f765041@syzkaller.appspotmail.com
> Reported-by: syzbot+276ca1c77a19977c0130@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/endpointola.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index e358437..69cebb2 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -118,10 +118,6 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
> /* Initialize the bind addr area */
> sctp_bind_addr_init(&ep->base.bind_addr, 0);
>
> - /* Remember who we are attached to. */
> - ep->base.sk = sk;
> - sock_hold(ep->base.sk);
> -
> /* Create the lists of associations. */
> INIT_LIST_HEAD(&ep->asocs);
>
> @@ -154,6 +150,10 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
> ep->prsctp_enable = net->sctp.prsctp_enable;
> ep->reconf_enable = net->sctp.reconf_enable;
>
> + /* Remember who we are attached to. */
> + ep->base.sk = sk;
> + sock_hold(ep->base.sk);
> +
> return ep;
>
> nomem_shkey:
> --
> 2.1.0
>
>
Acked-by: Neil Horman <nhorman@redhat.com>
^ permalink raw reply
* Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred
From: Jamal Hadi Salim @ 2019-06-25 11:18 UTC (permalink / raw)
To: John Hurley, netdev; +Cc: davem, fw, simon.horman, jakub.kicinski, oss-drivers
In-Reply-To: <1561414416-29732-1-git-send-email-john.hurley@netronome.com>
On 2019-06-24 6:13 p.m., John Hurley wrote:
> These patches aim to prevent act_mirred causing stack overflow events from
> recursively calling packet xmit or receive functions. Such events can
> occur with poor TC configuration that causes packets to travel in loops
> within the system.
>
> Florian Westphal advises that a recursion crash and packets looping are
> separate issues and should be treated as such. David Miller futher points
> out that pcpu counters cannot track the precise skb context required to
> detect loops. Hence these patches are not aimed at detecting packet loops,
> rather, preventing stack flows arising from such loops.
Sigh. So we are still trying to save 2 bits?
John, you said ovs has introduced a similar loop handling code;
Is their approach similar to this? Bigger question: Is this approach
"good enough"?
Not to put more work for you, but one suggestion is to use skb metadata
approach which is performance unfriendly (could be argued to more
correct).
Also: Please consider submitting a test case or two for tdc.
cheers,
jamal
^ permalink raw reply
* RE: [PATCH v5 2/5] net: macb: add support for sgmii MAC-PHY interface
From: Parshuram Raju Thombare @ 2019-06-25 11:22 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: andrew@lunn.ch, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <20190625103408.5rh2slqobruavyju@shell.armlinux.org.uk>
>If you are interested in supporting SFPs as well, then using phylink
>makes sense, but you need to implement your phylink conversion properly,
>and that means supporting dynamic switching of the PHY interface mode,
>and allowing phylink to determine whether a PHY interface mode is
>supported or not.
Yes, we want to support SFP+.
10G is tested in fixed mode on SFP+.
Based on your suggestions, dynamic switching of PHY interface is handled.
And check in probe is for hardware support for user selected interface,
which can be moved to validate callback but then supported mask will be empty.
>However, with what you've indicated through our discussion, your MAC
>does not support BASE-X modes, nor does it support 10GBASE-R, both of
>which are required for direct connection of SFP or SFP+ modules.
1000Base-X and 10GBASE-R supported by separate PCS.
Regards,
Parshuram Thombare
^ permalink raw reply
* RE: [PATCH v4 4/5] net: macb: add support for high speed interface
From: Parshuram Raju Thombare @ 2019-06-25 11:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, nicolas.ferre@microchip.com, davem@davemloft.net,
f.fainelli@gmail.com, netdev@vger.kernel.org,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Anil Joy Varughese, Piotr Sroka
In-Reply-To: <20190625105101.xvcwgt3jh5pk7p2x@shell.armlinux.org.uk>
>The closed nature of the USXGMII spec makes it very hard for us to know
>whether your implementation is correct or not.
>
>I have some documentation which suggests that USVGMII is a USXGMII link
>running at "5GR" rate as opposed to USXGMII running at "10GR" rate.
>
>So, I think 5G mode should be left out until it becomes clear that (a)
>we should specify it as USXGMII with a 5G rate, or as USVGMII.
>
Ok, I will remove 5G rate for USXGMII.
Regards,
Parshuram Thombare
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred
From: Jamal Hadi Salim @ 2019-06-25 11:24 UTC (permalink / raw)
To: John Hurley, Eyal Birger
Cc: Linux Netdev List, David Miller, Florian Westphal, Simon Horman,
Jakub Kicinski, oss-drivers, shmulik
In-Reply-To: <CAK+XE=mOjtp16tdz83RZ-x_jEp3nPRY3smxbG=OfCmGi9_DnXg@mail.gmail.com>
On 2019-06-25 5:06 a.m., John Hurley wrote:
> On Tue, Jun 25, 2019 at 9:30 AM Eyal Birger <eyal.birger@gmail.com> wrote:
> I'm not sure on the history of why a value of 4 was selected here but
> it seems to fall into line with my findings.
Back then we could only loop in one direction (as opposed to two right
now) - so seeing something twice would have been suspect enough,
so 4 seems to be a good number. I still think 4 is a good number.
> Is there a hard requirement for >4 recursive calls here?
I think this is where testcases help (which then get permanently
added in tdc repository). Eyal - if you have a test scenario where
this could be demonstrated it would help.
cheers,
jamal
^ permalink raw reply
* RE: [PATCH net-next 3/3] net: stmmac: Convert to phylink and remove phylib logic
From: Jose Abreu @ 2019-06-25 11:25 UTC (permalink / raw)
To: Jon Hunter, Jose Abreu, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Russell King, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, linux-tegra
In-Reply-To: <113f37a2-c37f-cdb5-5194-4361d949258a@nvidia.com>
From: Jon Hunter <jonathanh@nvidia.com>
> I have been looking at this a bit closer and I can see the problem. What
> happens is that ...
>
> 1. stmmac_mac_link_up() is called and priv->eee_active is set to false
> 2. stmmac_eee_init() is called but because priv->eee_active is false,
> timer_setup() for eee_ctrl_timer is never called.
> 3. stmmac_eee_init() returns true and so then priv->eee_enabled is set
> to true.
> 4. When stmmac_tx_clean() is called because priv->eee_enabled is set to
> true, mod_timer() is called for the eee_ctrl_timer, but because
> timer_setup() was never called, we hit the BUG defined at
> kernel/time/timer.c:952, because no function is defined for the
> timer.
>
> The following fixes it for me ...
>
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -399,10 +399,13 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
> mutex_lock(&priv->lock);
>
> /* Check if it needs to be deactivated */
> - if (!priv->eee_active && priv->eee_enabled) {
> - netdev_dbg(priv->dev, "disable EEE\n");
> - del_timer_sync(&priv->eee_ctrl_timer);
> - stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
> + if (!priv->eee_active) {
> + if (priv->eee_enabled) {
> + netdev_dbg(priv->dev, "disable EEE\n");
> + del_timer_sync(&priv->eee_ctrl_timer);
> + stmmac_set_eee_timer(priv, priv->hw, 0, tx_lpi_timer);
> + }
> + mutex_unlock(&priv->lock);
> return false;
> }
>
> It also looks like you have a potention deadlock in the current code
> because in the case of if (!priv->eee_active && priv->eee_enabled)
> you don't unlock the mutex. The above fixes this as well. I can send a
> formal patch if this looks correct.
Thanks for looking into this! The fix looks correct so if you could
submit a patch it would be great!
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Jesper Dangaard Brouer @ 2019-06-25 11:27 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce, brouer
In-Reply-To: <20190625105013.GA6485@khorivan>
On Tue, 25 Jun 2019 13:50:14 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> Hi Jesper,
>
> Could you please clarify one question.
>
> On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
> >This patch is needed before we can allow drivers to use page_pool for
> >DMA-mappings. Today with page_pool and XDP return API, it is possible to
> >remove the page_pool object (from rhashtable), while there are still
> >in-flight packet-pages. This is safely handled via RCU and failed lookups in
> >__xdp_return() fallback to call put_page(), when page_pool object is gone.
> >In-case page is still DMA mapped, this will result in page note getting
> >correctly DMA unmapped.
> >
> >To solve this, the page_pool is extended with tracking in-flight pages. And
> >XDP disconnect system queries page_pool and waits, via workqueue, for all
> >in-flight pages to be returned.
> >
> >To avoid killing performance when tracking in-flight pages, the implement
> >use two (unsigned) counters, that in placed on different cache-lines, and
> >can be used to deduct in-flight packets. This is done by mapping the
> >unsigned "sequence" counters onto signed Two's complement arithmetic
> >operations. This is e.g. used by kernel's time_after macros, described in
> >kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
> >
> >The trick is these two incrementing counters only need to be read and
> >compared, when checking if it's safe to free the page_pool structure. Which
> >will only happen when driver have disconnected RX/alloc side. Thus, on a
> >non-fast-path.
> >
> >It is chosen that page_pool tracking is also enabled for the non-DMA
> >use-case, as this can be used for statistics later.
> >
> >After this patch, using page_pool requires more strict resource "release",
> >e.g. via page_pool_release_page() that was introduced in this patchset, and
> >previous patches implement/fix this more strict requirement.
> >
> >Drivers no-longer call page_pool_destroy(). Drivers already call
> >xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
> >attempt to disconnect the mem id, and if attempt fails schedule the
> >disconnect for later via delayed workqueue.
> >
> >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >---
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
> > include/net/page_pool.h | 41 ++++++++++---
> > net/core/page_pool.c | 62 +++++++++++++++-----
> > net/core/xdp.c | 65 +++++++++++++++++++--
> > 4 files changed, 136 insertions(+), 35 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >index 2f647be292b6..6c9d4d7defbc 100644
> >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> > }
> >
> > xdp_rxq_info_unreg(&rq->xdp_rxq);
> >- if (rq->page_pool)
> >- page_pool_destroy(rq->page_pool);
> >-
> > mlx5_wq_destroy(&rq->wq_ctrl);
> > }
> >
> >diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> >index 754d980700df..f09b3f1994e6 100644
> >--- a/include/net/page_pool.h
> >+++ b/include/net/page_pool.h
> >@@ -16,14 +16,16 @@
> > * page_pool_alloc_pages() call. Drivers should likely use
> > * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
> > *
> >- * If page_pool handles DMA mapping (use page->private), then API user
> >- * is responsible for invoking page_pool_put_page() once. In-case of
> >- * elevated refcnt, the DMA state is released, assuming other users of
> >- * the page will eventually call put_page().
> >+ * API keeps track of in-flight pages, in-order to let API user know
> >+ * when it is safe to dealloactor page_pool object. Thus, API users
> >+ * must make sure to call page_pool_release_page() when a page is
> >+ * "leaving" the page_pool. Or call page_pool_put_page() where
> >+ * appropiate. For maintaining correct accounting.
> > *
> >- * If no DMA mapping is done, then it can act as shim-layer that
> >- * fall-through to alloc_page. As no state is kept on the page, the
> >- * regular put_page() call is sufficient.
> >+ * API user must only call page_pool_put_page() once on a page, as it
> >+ * will either recycle the page, or in case of elevated refcnt, it
> >+ * will release the DMA mapping and in-flight state accounting. We
> >+ * hope to lift this requirement in the future.
> > */
> > #ifndef _NET_PAGE_POOL_H
> > #define _NET_PAGE_POOL_H
> >@@ -66,9 +68,10 @@ struct page_pool_params {
> > };
> >
> > struct page_pool {
> >- struct rcu_head rcu;
> > struct page_pool_params p;
> >
> >+ u32 pages_state_hold_cnt;
> >+
> > /*
> > * Data structure for allocation side
> > *
> >@@ -96,6 +99,8 @@ struct page_pool {
> > * TODO: Implement bulk return pages into this structure.
> > */
> > struct ptr_ring ring;
> >+
> >+ atomic_t pages_state_release_cnt;
> > };
> >
> > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> >@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
> >
> > struct page_pool *page_pool_create(const struct page_pool_params *params);
> >
> >-void page_pool_destroy(struct page_pool *pool);
> >-
> > void __page_pool_free(struct page_pool *pool);
> > static inline void page_pool_free(struct page_pool *pool)
> > {
> >@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> > __page_pool_put_page(pool, page, true);
> > }
> >
> >+/* API user MUST have disconnected alloc-side (not allowed to call
> >+ * page_pool_alloc_pages()) before calling this. The free-side can
> >+ * still run concurrently, to handle in-flight packet-pages.
> >+ *
> >+ * A request to shutdown can fail (with false) if there are still
> >+ * in-flight packet-pages.
> >+ */
> >+bool __page_pool_request_shutdown(struct page_pool *pool);
> >+static inline bool page_pool_request_shutdown(struct page_pool *pool)
> >+{
> >+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
> >+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
> >+ */
> >+#ifdef CONFIG_PAGE_POOL
> >+ return __page_pool_request_shutdown(pool);
> >+#endif
> >+}
>
> The free side can ran in softirq, that means fast cache recycle is accessed.
> And it increments not atomic pool->alloc.count.
>
> For instance While redirect, for remote interface, while .ndo_xdp_xmit the
> xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
>
> In the same time, simultaneously, the work queue can try one more
> time to clear cash, calling __page_pool_request_shutdown()....
>
> Question, what prevents pool->alloc.count to be corrupted by race,
> causing to wrong array num and as result wrong page to be unmapped/put ....or
> even page leak. alloc.count usage is not protected,
> __page_pool_request_shutdown() is called not from same rx NAPI, even not from
> NAPI.
>
> Here, while alloc cache empty procedure in __page_pool_request_shutdown():
You forgot to copy this comment, which explains:
/* Empty alloc cache, assume caller made sure this is
* no-longer in use, and page_pool_alloc_pages() cannot be
* call concurrently.
*/
> while (pool->alloc.count) {
> page = pool->alloc.cache[--pool->alloc.count];
> __page_pool_return_page(pool, page);
> }
>
> For me seems all works fine, but I can't find what have I missed?
You have missed that, it is the drivers responsibility to "disconnect"
the xdp_rxq_info before calling shutdown. Which means that it is not
allowed to be used for RX, while the driver is shutting down a
RX-queue. For drivers this is very natural, else other things will
break.
> ...
>
> Same question about how xdp frame should be returned for drivers running
> tx napi exclusively, it can be still softirq but another CPU? What API
> should be used to return xdp frame.
You have to use the normal xdp_return_frame() which doesn't do "direct"
return.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [RFC PATCH 0/2] enable broadcom tagging for bcm531x5 switches
From: Benedikt Spranger @ 2019-06-25 11:20 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach
In-Reply-To: <f1f45e1d-5079-ab24-87d8-99b8c6710a08@gmail.com>
On Sat, 22 Jun 2019 19:24:10 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:
> Something like this should take care of that (untested). You might
> have to explicitly set the IMP port (port 8) in B53_UC_FWD_EN and
> B53_MC_FWD_EN, though since you turn on management mode, this may not
> be required. I might have some time tomorrow to test that on a Lamobo
> R1.
The patch work fine in a manual setup, but not with automagicaly setup
of $DISTRO. But that is a very different story. Trying to debug whats
going wrong there.
Thanks for the patch.
The whole setup of the switch seem to be desperate fragile. Any chance
to get that more robust?
Regards
Bene Spranger
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: René van Dorst @ 2019-06-25 11:31 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624153950.hdsuhrvfd77heyor@shell.armlinux.org.uk>
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:
Hi Russel,
Thanks for your review, see also my comments and questions below.
> Hi,
>
> On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>> drivers/net/dsa/mt7530.h | 9 ++
>> 2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
>> int port, int sset)
>> return ARRAY_SIZE(mt7530_mib);
>> }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> - struct phy_device *phydev)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - if (phy_is_pseudo_fixed_link(phydev)) {
>> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> - phydev->interface);
>> -
>> - /* Setup TX circuit incluing relevant PAD and driving */
>> - mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> - if (priv->id == ID_MT7530) {
>> - /* Setup RX circuit, relevant PAD and driving on the
>> - * host which must be placed after the setup on the
>> - * device side is all finished.
>> - */
>> - mt7623_pad_clk_setup(ds);
>> - }
>> - } else {
>> - u16 lcl_adv = 0, rmt_adv = 0;
>> - u8 flowctrl;
>> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> - switch (phydev->speed) {
>> - case SPEED_1000:
>> - mcr |= PMCR_FORCE_SPEED_1000;
>> - break;
>> - case SPEED_100:
>> - mcr |= PMCR_FORCE_SPEED_100;
>> - break;
>> - }
>> -
>> - if (phydev->link)
>> - mcr |= PMCR_FORCE_LNK;
>> -
>> - if (phydev->duplex) {
>> - mcr |= PMCR_FORCE_FDX;
>> -
>> - 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)
>> - mcr |= PMCR_TX_FC_EN;
>> - if (flowctrl & FLOW_CTRL_RX)
>> - mcr |= PMCR_RX_FC_EN;
>> - }
>> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> - }
>> -}
>> -
>> static int
>> mt7530_cpu_port_enable(struct mt7530_priv *priv,
>> int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>> return 0;
>> }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + const struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 is not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> +
>> + /* Setup TX circuit incluing relevant PAD and driving */
>> + mt7530_pad_clk_setup(ds, state->interface);
>> +
>> + if (priv->id == ID_MT7530) {
>> + /* Setup RX circuit, relevant PAD and driving on the
>> + * host which must be placed after the setup on the
>> + * device side is all finished.
>> + */
>> + mt7623_pad_clk_setup(ds);
>> + }
>> + break;
>> + default:
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> + mcr |= PMCR_FORCE_MODE;
>> +
>> + if (state->speed == SPEED_1000)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>> + if (state->speed == SPEED_100)
>> + mcr |= PMCR_FORCE_SPEED_100;
>> + if (state->duplex == DUPLEX_FULL)
>> + mcr |= PMCR_FORCE_FDX;
>> + if (state->link || mode == MLO_AN_FIXED)
>> + mcr |= PMCR_FORCE_LNK;
>
> This should be removed - state->link is not for use in mac_config.
> Even in fixed mode, the link can be brought up/down by means of a
> gpio, and this should be dealt with via the mac_link_* functions.
Maybe I understand it wrong, but is it the intention that in
phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
forces into a certain speed/mode/interface. So it never auto-negotiate because
phylink select the best configuration for you?
Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
and do nothing phylink_link_up()?
Other question:
Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
Or only mac_config() and port_disable?
>
>> + if (state->pause || phylink_test(state->advertising, Pause))
>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> + if (state->pause & MLO_PAUSE_TX)
>> + mcr |= PMCR_TX_FC_EN;
>> + if (state->pause & MLO_PAUSE_RX)
>> + mcr |= PMCR_RX_FC_EN;
>
> This is clearly wrong - if any bit in state->pause is set, then we
> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
> Pause set in the advertising mask, then both are set. This doesn't
> seem right - are these bits setting the advertisement, or are they
> telling the MAC to use flow control?
Last one, tell the MAC to use flow control.
Qoute of the datasheet:
If you want to disable TX and RX flow control disable bit 4 and 5
MAC Control Register:
BIT(4): Force TX FC:
0: Disabled
1: Let the MAC transmit a pause frame when operating in full-duplex
mode with low internal free memory page count.
BIT(5): Force RX FC:
0: Disabled
1: Let the MA accept a pause frame when operating in full-duplex
mode.
On the current driver both bits are set in a forced-link situation.
If we always forces the MAC mode I think I always set these bits and don't
anything with the Pause modes? Is that the right way to do it?
>
>> + }
>> +
>> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> + return;
>> +
>> +unsupported:
>> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> + __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface,
>> + struct phy_device *phydev)
>> +{
>> + /* Do nothing */
>> +}
>
> These two are where you should be forcing the link up or down if
> required (basically, inband modes should let the link come up/down
> irrespective of these functions, otherwise it should be forced.)
>
OK
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>
> PHY_INTERFACE_MODE_NA ?
You mean PHY_INTERFACE_MODE_NA is missing?
PHY_INTERFACE_MODE_TRGMII is a valid mode for this port.
>
>> + goto unsupported;
>> + break;
>> + default:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> + phylink_set(mask, MII);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>
> You seem to be missing phylink_set_port_modes() here.
OK
>
>> +
>> + linkmode_and(supported, supported, mask);
>> + linkmode_and(state->advertising, state->advertising, mask);
>> + return;
>> +
>> +unsupported:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> + __func__, state->interface, phy_modes(state->interface));
>
> Not a good idea to print this at error level; sometimes we just probe
> for support.
>
> Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
> interface mode that the MAC can't support - we detect that by the
> validation failing, and printing a more meaningful message in phylink
> itself.
OK, It will be removed.
>
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> + struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 pmsr;
>> +
>> + if (port < 0 || port >= MT7530_NUM_PORTS)
>> + return -EINVAL;
>> +
>> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> + state->link = (pmsr & PMSR_LINK);
>> + state->an_complete = state->link;
>> + state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> + case 0:
>> + state->speed = SPEED_10;
>> + break;
>> + case PMSR_SPEED_100:
>> + state->speed = SPEED_100;
>> + break;
>> + case PMSR_SPEED_1000:
>> + state->speed = SPEED_1000;
>> + break;
>> + default:
>> + state->speed = SPEED_UNKNOWN;
>> + break;
>> + }
>> +
>> + state->pause = 0;
>> + if (pmsr & PMSR_RX_FC)
>> + state->pause |= MLO_PAUSE_RX;
>> + if (pmsr & PMSR_TX_FC)
>> + state->pause |= MLO_PAUSE_TX;
>> +
>> + return 1;
>> +}
>> +
>> static const struct dsa_switch_ops mt7530_switch_ops = {
>> .get_tag_protocol = mtk_get_tag_protocol,
>> .setup = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .phy_write = mt7530_phy_write,
>> .get_ethtool_stats = mt7530_get_ethtool_stats,
>> .get_sset_count = mt7530_get_sset_count,
>> - .adjust_link = mt7530_adjust_link,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> .port_stp_state_set = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .port_vlan_prepare = mt7530_port_vlan_prepare,
>> .port_vlan_add = mt7530_port_vlan_add,
>> .port_vlan_del = mt7530_port_vlan_del,
>> + .phylink_validate = mt7530_phylink_validate,
>> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> + .phylink_mac_config = mt7530_phylink_mac_config,
>> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
>> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
>> };
>>
>> static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>> #define PMCR_FORCE_SPEED_100 BIT(2)
>> #define PMCR_FORCE_FDX BIT(1)
>> #define PMCR_FORCE_LNK BIT(0)
>> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
>> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>> PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
>> +#define PMSR_EEE1G BIT(7)
>> +#define PMSR_EEE100M BIT(6)
>> +#define PMSR_RX_FC BIT(5)
>> +#define PMSR_TX_FC BIT(4)
>> +#define PMSR_SPEED_1000 BIT(3)
>> +#define PMSR_SPEED_100 BIT(2)
>> +#define PMSR_DPX BIT(1)
>> +#define PMSR_LINK BIT(0)
>>
>> /* Register for MIB */
>> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
>> --
>> 2.20.1
>>
>>
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
Greats,
René
^ permalink raw reply
* Re: [PATCH iproute2] iproute: Pass RTM_F_CLONED on dump to fetch cached routes to be flushed
From: Stefano Brivio @ 2019-06-25 11:40 UTC (permalink / raw)
To: David Ahern
Cc: Stephen Hemminger, David Miller, Jianlin Shi, Wei Wang,
Martin KaFai Lau, Eric Dumazet, Matti Vaittinen, netdev
In-Reply-To: <209bcb66-2d57-eecf-d1a0-cc86af034e95@gmail.com>
On Mon, 24 Jun 2019 15:55:49 -0600
David Ahern <dsahern@gmail.com> wrote:
> On 6/14/19 7:33 PM, Stefano Brivio wrote:
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 2b3dcc5dbd53..192442b42062 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1602,6 +1602,16 @@ static int save_route_prep(void)
> > return 0;
> > }
> >
> > +static int iproute_flush_flags(struct nlmsghdr *nlh, int reqlen)
>
> rename that to iproute_flush_filter to be consistent with
> iproute_dump_filter.
I originally wanted to make it obvious that it's not an actual filter,
but:
> Actually, why can't the flush code use iproute_dump_filter?
...come on. That would be too simple.
No, my original understanding was that strict checking didn't imply
filtering. It does, and the current kernel implementation matches,
now also for RTM_F_CACHED. So yes, we can use it, and it doesn't cause
any unexpected behaviours with older kernels either. Sending v2. Thanks
for pointing this out.
--
Stefano
^ permalink raw reply
* [PATCH iproute2 v2] iproute: Set flags and attributes on dump to get IPv6 cached routes to be flushed
From: Stefano Brivio @ 2019-06-25 11:41 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David Miller, Jianlin Shi, Wei Wang, David Ahern,
Martin KaFai Lau, Eric Dumazet, Matti Vaittinen, netdev
With a current (5.1) kernel version, IPv6 exception routes can't be listed
(ip -6 route list cache) or flushed (ip -6 route flush cache). Kernel
support for this is being added back. Relevant net-next commits:
564c91f7e563 fib_frontend, ip6_fib: Select routes or exceptions dump from RTM_F_CLONED
ef11209d4219 Revert "net/ipv6: Bail early if user only wants cloned entries"
3401bfb1638e ipv6/route: Don't match on fc_nh_id if not set in ip6_route_del()
bf9a8a061ddc ipv6/route: Change return code of rt6_dump_route() for partial node dumps
1e47b4837f3b ipv6: Dump route exceptions if requested
40cb35d5dc04 ip6_fib: Don't discard nodes with valid routing information in fib6_locate_1()
However, to allow the kernel to filter routes based on the RTM_F_CLONED
flag, we need to make sure this flag is always passed when we want cached
routes to be dumped, and we can also pass table and output interface
attributes to have the kernel filtering on them, if requested by the user.
Use the existing iproute_dump_filter() as a filter for the dump request in
iproute_flush(). This way, 'ip -6 route flush cache' works again.
v2: Instead of creating a separate 'filter' function dealing with
RTM_F_CACHED only, use the existing iproute_dump_filter() and get
table and oif kernel filtering for free. Suggested by David Ahern.
Fixes: aba5acdfdb34 ("(Logical change 1.3)")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
ip/iproute.c | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/ip/iproute.c b/ip/iproute.c
index 2b3dcc5dbd53..1669e0138259 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1602,6 +1602,30 @@ static int save_route_prep(void)
return 0;
}
+static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
+{
+ struct rtmsg *rtm = NLMSG_DATA(nlh);
+ int err;
+
+ rtm->rtm_protocol = filter.protocol;
+ if (filter.cloned)
+ rtm->rtm_flags |= RTM_F_CLONED;
+
+ if (filter.tb) {
+ err = addattr32(nlh, reqlen, RTA_TABLE, filter.tb);
+ if (err)
+ return err;
+ }
+
+ if (filter.oif) {
+ err = addattr32(nlh, reqlen, RTA_OIF, filter.oif);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
static int iproute_flush(int family, rtnl_filter_t filter_fn)
{
time_t start = time(0);
@@ -1624,7 +1648,7 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
filter.flushe = sizeof(flushb);
for (;;) {
- if (rtnl_routedump_req(&rth, family, NULL) < 0) {
+ if (rtnl_routedump_req(&rth, family, iproute_dump_filter) < 0) {
perror("Cannot send dump request");
return -2;
}
@@ -1664,30 +1688,6 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
}
}
-static int iproute_dump_filter(struct nlmsghdr *nlh, int reqlen)
-{
- struct rtmsg *rtm = NLMSG_DATA(nlh);
- int err;
-
- rtm->rtm_protocol = filter.protocol;
- if (filter.cloned)
- rtm->rtm_flags |= RTM_F_CLONED;
-
- if (filter.tb) {
- err = addattr32(nlh, reqlen, RTA_TABLE, filter.tb);
- if (err)
- return err;
- }
-
- if (filter.oif) {
- err = addattr32(nlh, reqlen, RTA_OIF, filter.oif);
- if (err)
- return err;
- }
-
- return 0;
-}
-
static int iproute_list_flush_or_save(int argc, char **argv, int action)
{
int dump_family = preferred_family;
--
2.20.1
^ permalink raw reply related
* [[PATCH net-next]] net: core: xdp: make __mem_id_disconnect to be static
From: Ivan Khoronzhuk @ 2019-06-25 11:42 UTC (permalink / raw)
To: netdev, davem; +Cc: xdp-newbies, linux-kernel, ast, hawk, Ivan Khoronzhuk
Add missed static for local __mem_id_disconnect().
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
net/core/xdp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b513a18..829377cc83db 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -85,7 +85,7 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
kfree(xa);
}
-bool __mem_id_disconnect(int id, bool force)
+static bool __mem_id_disconnect(int id, bool force)
{
struct xdp_mem_allocator *xa;
bool safe_to_remove = true;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: René van Dorst @ 2019-06-25 11:43 UTC (permalink / raw)
To: Daniel Santos
Cc: sean.wang, f.fainelli, linux, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <13c67cb7-b33e-f2b1-9d1e-d2882e0ff076@pobox.com>
Quoting Daniel Santos <daniel.santos@pobox.com>:
Hi Daniel,
> On 6/24/19 9:52 AM, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>> drivers/net/dsa/mt7530.h | 9 ++
>> 2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
>> int port, int sset)
>> return ARRAY_SIZE(mt7530_mib);
>> }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> - struct phy_device *phydev)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - if (phy_is_pseudo_fixed_link(phydev)) {
>> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> - phydev->interface);
>> -
>> - /* Setup TX circuit incluing relevant PAD and driving */
>> - mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> - if (priv->id == ID_MT7530) {
>> - /* Setup RX circuit, relevant PAD and driving on the
>> - * host which must be placed after the setup on the
>> - * device side is all finished.
>> - */
>> - mt7623_pad_clk_setup(ds);
>> - }
>> - } else {
>> - u16 lcl_adv = 0, rmt_adv = 0;
>> - u8 flowctrl;
>> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> - switch (phydev->speed) {
>> - case SPEED_1000:
>> - mcr |= PMCR_FORCE_SPEED_1000;
>> - break;
>> - case SPEED_100:
>> - mcr |= PMCR_FORCE_SPEED_100;
>> - break;
>> - }
>> -
>> - if (phydev->link)
>> - mcr |= PMCR_FORCE_LNK;
>> -
>> - if (phydev->duplex) {
>> - mcr |= PMCR_FORCE_FDX;
>> -
>> - 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)
>> - mcr |= PMCR_TX_FC_EN;
>> - if (flowctrl & FLOW_CTRL_RX)
>> - mcr |= PMCR_RX_FC_EN;
>> - }
>> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> - }
>> -}
>> -
>> static int
>> mt7530_cpu_port_enable(struct mt7530_priv *priv,
>> int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>> return 0;
>> }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + const struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 is not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> +
>> + /* Setup TX circuit incluing relevant PAD and driving */
>> + mt7530_pad_clk_setup(ds, state->interface);
>> +
>> + if (priv->id == ID_MT7530) {
>> + /* Setup RX circuit, relevant PAD and driving on the
>> + * host which must be placed after the setup on the
>> + * device side is all finished.
>> + */
>> + mt7623_pad_clk_setup(ds);
>> + }
>> + break;
>> + default:
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> + mcr |= PMCR_FORCE_MODE;
>> +
>> + if (state->speed == SPEED_1000)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>> + if (state->speed == SPEED_100)
>> + mcr |= PMCR_FORCE_SPEED_100;
>
> I would suggest using the defines
>
> #define PMCR_FORCE_SPEED 0x0000000c /* or PMCR_FORCE_SPEED_MASK */
> #define PMCR_FORCE_SPEED_10 0x00000000
> #define PMCR_FORCE_SPEED_100 0x00000004
> #define PMCR_FORCE_SPEED_1000 0x00000008
>
> and a switch statement such as
>
> switch (state->speed) {
> case SPEED_10:
> mcr |= PMCR_FORCE_SPEED_10;
> break;
> case SPEED_100:
> mcr |= PMCR_FORCE_SPEED_100;
> break;
> case SPEED_1000:
> mcr |= PMCR_FORCE_SPEED_1000;
> break;
> }
>
> This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
> while alleviating the need to intimately know the hardware in order to
> easily understand what the code is doing at a glance. It's also better
> form as we're treating the two bits as a composite value, rather than
> two separate bits.
I will change it based on your surgestion.
>
>
>> + if (state->duplex == DUPLEX_FULL)
>> + mcr |= PMCR_FORCE_FDX;
>> + if (state->link || mode == MLO_AN_FIXED)
>> + mcr |= PMCR_FORCE_LNK;
>> + if (state->pause || phylink_test(state->advertising, Pause))
>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> + if (state->pause & MLO_PAUSE_TX)
>> + mcr |= PMCR_TX_FC_EN;
>> + if (state->pause & MLO_PAUSE_RX)
>> + mcr |= PMCR_RX_FC_EN;
>> + }
>> +
>> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> + return;
>> +
>> +unsupported:
>> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> + __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface,
>> + struct phy_device *phydev)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> + break;
>> + default:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> + phylink_set(mask, MII);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>> +
>> + linkmode_and(supported, supported, mask);
>> + linkmode_and(state->advertising, state->advertising, mask);
>> + return;
>> +
>> +unsupported:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> + __func__, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> + struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 pmsr;
>> +
>> + if (port < 0 || port >= MT7530_NUM_PORTS)
>> + return -EINVAL;
>> +
>> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> + state->link = (pmsr & PMSR_LINK);
>> + state->an_complete = state->link;
>> + state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> + case 0:
>> + state->speed = SPEED_10;
>> + break;
>> + case PMSR_SPEED_100:
>> + state->speed = SPEED_100;
>> + break;
>> + case PMSR_SPEED_1000:
>> + state->speed = SPEED_1000;
>> + break;
>> + default:
>> + state->speed = SPEED_UNKNOWN;
>> + break;
>> + }
>> +
>
> Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
> PMSR_SPEED_MASK if you prefer).
Same as above.
>
>> + state->pause = 0;
>> + if (pmsr & PMSR_RX_FC)
>> + state->pause |= MLO_PAUSE_RX;
>> + if (pmsr & PMSR_TX_FC)
>> + state->pause |= MLO_PAUSE_TX;
>> +
>> + return 1;
>> +}
>> +
>> static const struct dsa_switch_ops mt7530_switch_ops = {
>> .get_tag_protocol = mtk_get_tag_protocol,
>> .setup = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .phy_write = mt7530_phy_write,
>> .get_ethtool_stats = mt7530_get_ethtool_stats,
>> .get_sset_count = mt7530_get_sset_count,
>> - .adjust_link = mt7530_adjust_link,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> .port_stp_state_set = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>> .port_vlan_prepare = mt7530_port_vlan_prepare,
>> .port_vlan_add = mt7530_port_vlan_add,
>> .port_vlan_del = mt7530_port_vlan_del,
>> + .phylink_validate = mt7530_phylink_validate,
>> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> + .phylink_mac_config = mt7530_phylink_mac_config,
>> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
>> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
>> };
>>
>> static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>> #define PMCR_FORCE_SPEED_100 BIT(2)
>> #define PMCR_FORCE_FDX BIT(1)
>> #define PMCR_FORCE_LNK BIT(0)
>> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
>> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>> PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
>> +#define PMSR_EEE1G BIT(7)
>> +#define PMSR_EEE100M BIT(6)
>> +#define PMSR_RX_FC BIT(5)
>> +#define PMSR_TX_FC BIT(4)
>> +#define PMSR_SPEED_1000 BIT(3)
>> +#define PMSR_SPEED_100 BIT(2)
>> +#define PMSR_DPX BIT(1)
>> +#define PMSR_LINK BIT(0)
>>
>> /* Register for MIB */
>> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
>
> Cheers,
> Daniel
Greats,
René
^ 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