* Re: [PATCH] Cipso: cipso_v4_optptr enter infinite loop
From: Paul Moore @ 2017-07-31 20:13 UTC (permalink / raw)
To: Yujuan Qi
Cc: David S. Miller, Casey Schaufler, netdev, linux-mediatek,
linux-kernel, Ryder Lee
In-Reply-To: <1501471381-12808-1-git-send-email-yujuan.qi@mediatek.com>
On Sun, Jul 30, 2017 at 11:23 PM, Yujuan Qi <yujuan.qi@mediatek.com> wrote:
> From: "yujuan.qi" <yujuan.qi@mediatek.com>
>
> in for(),if((optlen > 0) && (optptr[1] == 0)), enter infinite loop.
>
> Test: receive a packet which the ip length > 20 and the first byte of ip option is 0, produce this issue
>
> Signed-off-by: yujuan.qi <yujuan.qi@mediatek.com>
> ---
> net/ipv4/cipso_ipv4.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Considering I gave you the code below I should probably ack it, right? ;)
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index ae20616..0d1e07d 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1523,9 +1523,17 @@ unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
> int taglen;
>
> for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 0; ) {
> - if (optptr[0] == IPOPT_CIPSO)
> + switch (optptr[0]) {
> + case IPOPT_CIPSO:
> return optptr;
> - taglen = optptr[1];
> + case IPOPT_END:
> + return NULL;
> + case IPOPT_NOOP:
> + taglen = 1;
> + break;
> + default:
> + taglen = optptr[1];
> + }
> optlen -= taglen;
> optptr += taglen;
> }
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [RFC net-next 0/6] tcp: remove prequeue and header prediction
From: Yuchung Cheng @ 2017-07-31 20:04 UTC (permalink / raw)
To: Neal Cardwell
Cc: Florian Westphal, Netdev, Eric Dumazet, Soheil Hassas Yeganeh,
Wei Wang, Lawrence Brakmo, David Miller, Lorenzo Colitti,
Van Jacobson
In-Reply-To: <CADVnQym-aDwv-NhcwM5eTW4XQZcKMS+64-VwObKFxT5maT=TGg@mail.gmail.com>
On Sat, Jul 29, 2017 at 7:25 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Thu, Jul 27, 2017 at 7:31 PM, Florian Westphal <fw@strlen.de> wrote:
>> This RFC removes tcp prequeueing and header prediction support.
>>
>> After a hallway discussion with Eric Dumazet some
>> maybe-not-so-useful-anymore TCP stack features came up, HP and
>> Prequeue among these.
>>
>> So this RFC proposes to axe both.
>>
>> In brief, TCP prequeue assumes a single-process-blocking-read
>> design, which is not that common anymore, and the most frequently
>> used high-performance networking program that does this is netperf :)
>>
>> With more commong (e)poll designs, prequeue doesn't work.
>>
>> The idea behind prequeueing isn't so bad in itself; it moves
>> part of tcp processing -- including ack processing (including
>> retransmit queue processing) into process context.
>> However, removing it would not just avoid some code, for most
>> programs it elimiates dead code.
>>
>> As processing then always occurs in BH context, it would allow us
>> to experiment e.g. with bulk-freeing of skb heads when a packet acks
>> data on the retransmit queue.
>>
>> Header prediction is also less useful nowadays.
>> For packet trains, GRO will aggregate packets so we do not get
>> a per-packet benefit.
>> Header prediction will also break down with light packet loss due to SACK.
>>
>> So, In short: What do others think?
>>
>> Florian Westphal (6):
>> tcp: remove prequeue support
>> tcp: reindent two spots after prequeue removal
>> tcp: remove low_latency sysctl
>> tcp: remove header prediction
>> tcp: remove CA_ACK_SLOWPATH
>> tcp: remove unused mib counters
>>
>> Documentation/networking/ip-sysctl.txt | 7
>> include/linux/tcp.h | 15 -
>> include/net/tcp.h | 40 ----
>> include/uapi/linux/snmp.h | 8
>> net/ipv4/proc.c | 8
>> net/ipv4/sysctl_net_ipv4.c | 3
>> net/ipv4/tcp.c | 109 -----------
>> net/ipv4/tcp_input.c | 303 +++------------------------------
>> net/ipv4/tcp_ipv4.c | 63 ------
>> net/ipv4/tcp_minisocks.c | 3
>> net/ipv4/tcp_output.c | 2
>> net/ipv4/tcp_timer.c | 12 -
>> net/ipv4/tcp_westwood.c | 31 ---
>> net/ipv6/tcp_ipv6.c | 3
>> 14 files changed, 43 insertions(+), 564 deletions(-)
>>
>
> I unconditionally support the removal of prequeue support.
>
> For the header prediction code: IMHO before removing the header
> prediction code it would be useful to do some kind of before-and-after
> benchmarking on a low-powered device where battery life is the main
> concern. I am thinking about ARM-based cell phones, IoT/embedded
> devices, raspberry pi, etc. You mention GRO helping to make header
> prediction obsolete, but in those devices packets arrive so slowly
> that probably GRO does not help. With slow CPUs and battery life the
> main concern, it seems conceivable to me that header prediction might
> still be a win (and worth keeping, since the complexity cost is
by the time these devices use 4.12 kernels they are likely powerful
enough to make header prediction irrelevant...
> largely in the past; the maintenance overhead has been low). Just a
> thought.
>
> thanks,
> neal
^ permalink raw reply
* Re: [PATCH net] Revert "net: bcmgenet: Remove init parameter from bcmgenet_mii_config"
From: Doug Berger @ 2017-07-31 19:55 UTC (permalink / raw)
To: Florian Fainelli, netdev; +Cc: davem, jaedon.shin
In-Reply-To: <20170731180532.19884-1-f.fainelli@gmail.com>
On 07/31/2017 11:05 AM, Florian Fainelli wrote:
> This reverts commit 28b45910ccda ("net: bcmgenet: Remove init parameter
> from bcmgenet_mii_config") because in the process of moving from
> dev_info() to dev_info_once() we essentially lost the helpful printed
> messages once the second instance of the driver is loaded.
> dev_info_once() does not actually print the message once per device
> instance, but once period.
>
> Fixes: 28b45910ccda ("net: bcmgenet: Remove init parameter from bcmgenet_mii_config")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
> drivers/net/ethernet/broadcom/genet/bcmgenet.h | 2 +-
> drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 ++++---
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7b0b399aaedd..a981c4ee9d72 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3669,7 +3669,7 @@ static int bcmgenet_resume(struct device *d)
>
> phy_init_hw(priv->phydev);
> /* Speed settings must be restored */
> - bcmgenet_mii_config(priv->dev);
> + bcmgenet_mii_config(priv->dev, false);
>
> /* disable ethernet MAC while updating its registers */
> umac_enable_set(priv, CMD_TX_EN | CMD_RX_EN, false);
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index b9344de669f8..3a34fdba5301 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -698,7 +698,7 @@ GENET_IO_MACRO(rbuf, GENET_RBUF_OFF);
>
> /* MDIO routines */
> int bcmgenet_mii_init(struct net_device *dev);
> -int bcmgenet_mii_config(struct net_device *dev);
> +int bcmgenet_mii_config(struct net_device *dev, bool init);
> int bcmgenet_mii_probe(struct net_device *dev);
> void bcmgenet_mii_exit(struct net_device *dev);
> void bcmgenet_mii_reset(struct net_device *dev);
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 071fcbd14e6a..30cb97b4a1d7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -238,7 +238,7 @@ static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
> bcmgenet_fixed_phy_link_update);
> }
>
> -int bcmgenet_mii_config(struct net_device *dev)
> +int bcmgenet_mii_config(struct net_device *dev, bool init)
> {
> struct bcmgenet_priv *priv = netdev_priv(dev);
> struct phy_device *phydev = priv->phydev;
> @@ -327,7 +327,8 @@ int bcmgenet_mii_config(struct net_device *dev)
> bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
> }
>
> - dev_info_once(kdev, "configuring instance for %s\n", phy_name);
> + if (init)
> + dev_info(kdev, "configuring instance for %s\n", phy_name);
>
> return 0;
> }
> @@ -375,7 +376,7 @@ int bcmgenet_mii_probe(struct net_device *dev)
> * PHY speed which is needed for bcmgenet_mii_config() to configure
> * things appropriately.
> */
> - ret = bcmgenet_mii_config(dev);
> + ret = bcmgenet_mii_config(dev, true);
> if (ret) {
> phy_disconnect(priv->phydev);
> return ret;
>
Looks good to me.
Reviewed-by: Doug Berger <opendmb@gmail.com>
^ permalink raw reply
* [PATCH v2] ss: Enclose IPv6 address in brackets
From: Florian Lehner @ 2017-07-31 19:50 UTC (permalink / raw)
To: netdev
In-Reply-To: <fc81757c-8dbb-dbed-7a24-7011cb8bb9e0@der-flo.net>
This updated patch adds support for RFC2732 IPv6 address format with
brackets for the tool ss. Resolved hostnames will not be enclosed in
brackets, therefore the global variable resolve_hosts is initialized and
checked.
Signed-off-by: Lehner Florian <dev@der-flo.net>
---
misc/ss.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index 12763c9..ac94537 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -88,7 +88,7 @@ static int security_get_initial_context(char *name,
char **context)
}
#endif
-int resolve_hosts;
+int resolve_hosts = 0;
int resolve_services = 1;
int preferred_family = AF_UNSPEC;
int show_options;
@@ -1059,7 +1059,11 @@ static void inet_addr_print(const inet_prefix *a,
int port, unsigned int ifindex
ap = format_host(AF_INET, 4, a->data);
}
} else {
- ap = format_host(a->family, 16, a->data);
+ if (a->family == AF_INET6 && !resolve_hosts) {
+ sprintf(buf, "[%s]", format_host(a->family, 16, a->data));
+ } else {
+ ap = format_host(a->family, 16, a->data);
+ }
est_len = strlen(ap);
if (est_len <= addr_width)
est_len = addr_width;
--
2.9.4
^ permalink raw reply related
* Re: [PATCH v2] ravb: add wake-on-lan support via magic packet
From: Sergei Shtylyov @ 2017-07-31 19:47 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: Geert Uytterhoeven, netdev, linux-renesas-soc
In-Reply-To: <20170730140646.11781-1-niklas.soderlund+renesas@ragnatech.se>
Hello!
On 07/30/2017 05:06 PM, Niklas Söderlund wrote:
> WoL is enabled in the suspend callback by setting MagicPacket detection
> and disabling all interrupts expect MagicPacket. In the resume path the
> driver needs to reset the hardware to rearm the WoL logic, this prevents
> the driver from simply restoring the registers and to take advantage of
> that ravb was not suspended to reduce resume time. To reset the
> hardware the driver closes the device, sets it in reset mode and reopens
> the device just like it would do in a normal suspend/resume scenario
> without WoL enabled, but it both closes and opens the device in the
> resume callback since the device needs to be reset for WoL to work.
> One quirk needed for WoL is that the module clock needs to be prevented
> from being switched off by Runtime PM. To keep the clock alive the
> suspend callback need to call clk_enable() directly to increase the
> usage count of the clock. Then when Runtime PM decreases the clock usage
> count it won't reach 0 and be switched off.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5931e859876c2aee..3d399f85417a83cf 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
> struct ravb_private *priv = netdev_priv(ndev);
> int ret = 0;
>
> + /* Reduce the usecount of the clock to zero and then
> + * restore it to its original value. This is done to force
> + * the clock to be re-enabled which is a workaround
> + * for renesas-cpg-mssr driver which do not enable clocks
> + * when resuming from PSCI suspend/resume.
> + *
> + * Without this workaround the driver fails to communicate
> + * with the hardware if WoL was enabled when the system
> + * entered PSCI suspend. This is due to that if WoL is enabled
> + * we explicitly keep the clock from being turned off when
> + * suspending, but in PSCI sleep power is cut so the clock
> + * is disabled anyhow, the clock driver is not aware of this
> + * so the clock is not turned back on when resuming.
> + *
> + * TODO: once the renesas-cpg-mssr suspend/resume is working
> + * this clock dance should be removed.
> + */
> + clk_disable(priv->clk);
> + clk_disable(priv->clk);
> + clk_enable(priv->clk);
> + clk_enable(priv->clk);
After a small chat with Niklas, it became clear that this dance should be
behind the *if* that follows. I'd also like to see this workaround as a
separate patch since the isse it addresses is R-Car gen3 specific (and thus
can be reverted once the CPG/MMSR driver is fixed)...
> +
> + /* If WoL is enabled set reset mode to rearm the WoL logic */
> + if (priv->wol_enabled)
> + ravb_write(ndev, CCC_OPC_RESET, CCC);
> +
> /* All register have been reset to default values.
> * Restore all registers which where setup at probe time and
> * reopen device if it was running before system suspended.
> @@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
> ravb_write(ndev, priv->desc_bat_dma, DBAT);
>
> if (netif_running(ndev)) {
> + if (priv->wol_enabled) {
> + ret = ravb_wol_restore(ndev);
> + if (ret)
> + return ret;
> + }
> ret = ravb_open(ndev);
Hm, perhaps worth calling sh_eth_open() outside sh_eth_wol_restore() as well?
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] ISDN: eicon: fix array-bounds warning properly
From: isdn @ 2017-07-31 19:38 UTC (permalink / raw)
To: Arnd Bergmann, Armin Schindler; +Cc: netdev, linux-kernel
In-Reply-To: <20170731090438.458392-1-arnd@arndb.de>
Hi Arnd,
I think you are right, but removing this is maybe the wrong fix.
The issue is, that CAPI messages are packed byte streams and yes the
64bit extension of CAPI is not very good designed for modern CPU
constrains with alignment, since the data pointer for the buffer is not
on a 64bit boundary. All hardware controller implementations I know do
ignore the data pointer, they are simple awaiting the data of the given
length directly after the message. Only the application interface on 64
bit systems really need the 64 bit data pointer value, which is usually
set in the HW driver to the mapped user space address of the used data
buffer if the user space process is 64bit.
The user space CAPI library correctly use byte stream access functions
to read/write the values. So the correct solution for this driver would
be to use stream access functions as well here and not add a 64 bit
pointer.
On the other hand I do not think the any people use this driver on 64
bit systems today, because they would run into this issue.
Best
Karsten
Am 31.07.2017 um 11:04 schrieb Arnd Bergmann:
> I patched a variant of this warning before, but now saw it come back
> in a different configuration with gcc-7 and UBSAN:
>
> drivers/isdn/hardware/eicon/message.c: In function 'mixer_notify_update':
> drivers/isdn/hardware/eicon/message.c:11162:54: error: array subscript is above array bounds [-Werror=array-bounds]
> ((CAPI_MSG *) msg)->info.facility_req.structs[1] = LI_REQ_SILENT_UPDATE & 0xff;
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/isdn/hardware/eicon/message.c:11163:54: error: array subscript is above array bounds [-Werror=array-bounds]
> ((CAPI_MSG *) msg)->info.facility_req.structs[2] = LI_REQ_SILENT_UPDATE >> 8;
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/isdn/hardware/eicon/message.c:11164:54: error: array subscript is above array bounds [-Werror=array-bounds]
> ((CAPI_MSG *) msg)->info.facility_req.structs[3] = 0;
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
>
> I spent a long time narrowing down what caused this, as I suspected
> yet another false-positive warning in gcc. However, this time it
> turned out to be an ancient kernel bug, which probably prevented
> this from ever working on 64-bit machines, causing a stack
> buffer overflow as indicated by the warning originally.
>
> The problem is that having a 64-bit pointer inside of the CAPI_MSG->info
> union leads to the start of the union to become 64-bit aligned by adding
> four padding bytes. The structure is however aliased to a fixed-length
> array on the stack in mixer_notify_update(), and later copied directly
> to the hardware, so both go wrong.
>
> This just removes the fields that were apparently added in a misguided
> attempt to make the driver work on 64-bit machines but never actually
> used.
>
> Fixes: 950eabbd6dde ("ISDN: eicon: silence misleading array-bounds warning")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/isdn/hardware/eicon/capi20.h | 18 ------------------
> 1 file changed, 18 deletions(-)
>
> diff --git a/drivers/isdn/hardware/eicon/capi20.h b/drivers/isdn/hardware/eicon/capi20.h
> index 391e4175b0b5..7b97cd576485 100644
> --- a/drivers/isdn/hardware/eicon/capi20.h
> +++ b/drivers/isdn/hardware/eicon/capi20.h
> @@ -301,14 +301,6 @@ typedef struct {
> word Number;
> word Flags;
> } _DAT_B3_REQP;
> -/* DATA-B3-REQUEST 64 BIT Systems */
> -typedef struct {
> - dword Data;
> - word Data_Length;
> - word Number;
> - word Flags;
> - void *pData;
> -} _DAT_B3_REQ64P;
> /* DATA-B3-CONFIRM */
> typedef struct {
> word Number;
> @@ -321,14 +313,6 @@ typedef struct {
> word Number;
> word Flags;
> } _DAT_B3_INDP;
> -/* DATA-B3-INDICATION 64 BIT Systems */
> -typedef struct {
> - dword Data;
> - word Data_Length;
> - word Number;
> - word Flags;
> - void *pData;
> -} _DAT_B3_IND64P;
> /* DATA-B3-RESPONSE */
> typedef struct {
> word Number;
> @@ -409,10 +393,8 @@ struct _API_MSG {
> _DIS_B3_INDP disconnect_b3_ind;
> _DIS_B3_RESP disconnect_b3_res;
> _DAT_B3_REQP data_b3_req;
> - _DAT_B3_REQ64P data_b3_req64;
> _DAT_B3_CONP data_b3_con;
> _DAT_B3_INDP data_b3_ind;
> - _DAT_B3_IND64P data_b3_ind64;
> _DAT_B3_RESP data_b3_res;
> _RES_B3_REQP reset_b3_req;
> _RES_B3_CONP reset_b3_con;
>
^ permalink raw reply
* Re: [RFC net-next] net ipv6: convert fib6_table rwlock to a percpu lock
From: Shaohua Li @ 2017-07-31 19:34 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, Kernel-team, Shaohua Li, Wei Wang
In-Reply-To: <1501525853.1876.22.camel@edumazet-glaptop3.roam.corp.google.com>
On Mon, Jul 31, 2017 at 11:30:53AM -0700, Eric Dumazet wrote:
> On Mon, 2017-07-31 at 10:18 -0700, Shaohua Li wrote:
> > From: Shaohua Li <shli@fb.com>
> >
> > In a syn flooding test, the fib6_table rwlock is a significant
> > bottleneck. While converting the rwlock to rcu sounds straighforward,
> > but is very challenging if it's possible. A percpu spinlock is quite
> > trival for this problem since updating the routing table is a rare
> > event. In my test, the server receives around 1.5 Mpps in syn flooding
> > test without the patch in a dual sockets and 56-CPU system. With the
> > patch, the server receives around 3.8Mpps, and perf report doesn't show
> > the locking issue.
> >
>
> > +static inline void fib6_table_write_lock_bh(struct fib6_table *table)
> > +{
> > + int i;
> > +
> > + spin_lock_bh(per_cpu_ptr(table->percpu_tb6_lock, 0));
> > + for_each_possible_cpu(i) {
> > + if (i == 0)
> > + continue;
> > + spin_lock_nest_lock(per_cpu_ptr(table->percpu_tb6_lock, i),
> > + per_cpu_ptr(table->percpu_tb6_lock, 0));
> > + }
> > +}
>
> Your code assumes that cpu 0 is valid.
This is unlikely not true especially for possible cpu map :)
> I would rather not hard code this knowledge.
Will fix it in next post
> Also this is not clear why you need the nested thing.
This is to avoid lockdep warnning. The locks have the same lockdep key, if we
don't use the nest lock annotation, lockdep assumes they are the same lock and
complain.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH net V3 2/2] net: fix tcp reset packet flowlabel for ipv6
From: Shaohua Li @ 2017-07-31 19:30 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, kernel-team,
Shaohua Li, Eric Dumazet, Florent Fourcot
In-Reply-To: <CAM_iQpV-=OxeGyS1ie62x-_+w2pN_V-yxQ0jP3HfjYxo+k_k-A@mail.gmail.com>
On Mon, Jul 31, 2017 at 11:10:38AM -0700, Cong Wang wrote:
> On Mon, Jul 31, 2017 at 10:08 AM, Shaohua Li <shli@kernel.org> wrote:
> > +/* Like ip6_make_flowlabel, but already has hash */
> > +static inline __be32 ip6_make_flowlabel_from_hash(struct net *net,
> > + bool autolabel, u32 hash)
> > +{
> > + __be32 flowlabel;
> > +
> > + if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
> > + (!autolabel &&
> > + net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
> > + return 0;
> > +
> > + flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
> > +
> > + if (net->ipv6.sysctl.flowlabel_state_ranges)
> > + flowlabel |= IPV6_FLOWLABEL_STATELESS_FLAG;
> > +
> > + return flowlabel;
> > +}
>
> I still don't see why you have to duplicate the code,
> for me you can just refactor ip6_make_flowlabel()
> and pass the hash as a parameter and pass
> 'flowlabel' as 0, and no run-time overhead.
Still need extra check. Ok, I updated the patch.
Thanks,
Shaohua
>From 373e23f5295ee4cb725109a9e58152451a9fb4cc Mon Sep 17 00:00:00 2001
Message-Id: <373e23f5295ee4cb725109a9e58152451a9fb4cc.1501529088.git.shli@fb.com>
From: Shaohua Li <shli@fb.com>
Date: Tue, 11 Jul 2017 21:09:48 -0700
Subject: [PATCH] net: fix tcp reset packet flowlabel for ipv6
Please see below tcpdump output:
21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 7], length 0
21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 30
21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 24
21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 0
21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options [nop,nop,TS val 2500904438 ecr 2500903438], length 24
21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload length: 20) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0
The tcp reset packet has a different flowlabel, which causes our router
doesn't correctly close tcp connection. The reason is the normal packet
gets the skb->hash from sk->sk_txhash, which is generated randomly.
ip6_make_flowlabel then uses the hash to create a flowlabel. The reset
packet doesn't get assigned a hash, so the flowlabel is calculated with
flowi6.
Since user can't change timewait sock flowlabel, we create a flowlabel
for timewait socket with the random generated hash (sk->sk_txhash), then
use it in reset packet. In this way, the reset packet will have the same
flowlabel as normal packets.
This also fixes the flowlabel issue for reset packet if user configures
flowlabel, which is ignored previously.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Florent Fourcot <flo@fourcot.fr>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
include/net/ipv6.h | 9 ++++-----
net/ipv4/tcp_minisocks.c | 8 +++++++-
net/ipv6/ip6_gre.c | 2 +-
net/ipv6/ip6_output.c | 4 ++--
net/ipv6/ip6_tunnel.c | 2 +-
net/ipv6/tcp_ipv6.c | 18 +++++++++++++++++-
6 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 7548367..f8713fd 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -773,10 +773,8 @@ static inline void iph_to_flow_copy_v6addrs(struct flow_keys *flow,
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
@@ -788,7 +786,8 @@ static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
return flowlabel;
- hash = skb_get_hash_flowi6(skb, fl6);
+ if (skb)
+ hash = skb_get_hash_flowi6(skb, fl6);
flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
@@ -814,7 +813,7 @@ static inline int ip6_default_np_autolabel(struct net *net)
static inline void ip6_set_txhash(struct sock *sk) { }
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
return flowlabel;
}
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0ff83c1..ac41b25 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -276,11 +276,17 @@ void tcp_time_wait(struct sock *sk, int state, int timeo)
#if IS_ENABLED(CONFIG_IPV6)
if (tw->tw_family == PF_INET6) {
struct ipv6_pinfo *np = inet6_sk(sk);
+ __be32 flowlabel;
tw->tw_v6_daddr = sk->sk_v6_daddr;
tw->tw_v6_rcv_saddr = sk->sk_v6_rcv_saddr;
tw->tw_tclass = np->tclass;
- tw->tw_flowlabel = be32_to_cpu(np->flow_label & IPV6_FLOWLABEL_MASK);
+ flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK;
+ if (flowlabel == 0)
+ flowlabel = ip6_make_flowlabel(
+ sock_net(sk), NULL, 0, np->autoflowlabel,
+ NULL, sk->sk_txhash);
+ tw->tw_flowlabel = be32_to_cpu(flowlabel);
tw->tw_ipv6only = sk->sk_ipv6only;
}
#endif
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 67ff2aa..36db44a 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -948,7 +948,7 @@ static int ip6gre_header(struct sk_buff *skb, struct net_device *dev,
ip6_flow_hdr(ipv6h, 0,
ip6_make_flowlabel(dev_net(dev), skb,
t->fl.u.ip6.flowlabel, true,
- &t->fl.u.ip6));
+ &t->fl.u.ip6, 0));
ipv6h->hop_limit = t->parms.hop_limit;
ipv6h->nexthdr = NEXTHDR_GRE;
ipv6h->saddr = t->parms.laddr;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 162efba..cefd249 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -230,7 +230,7 @@ int ip6_xmit(const struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
hlimit = ip6_dst_hoplimit(dst);
ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
- np->autoflowlabel, fl6));
+ np->autoflowlabel, fl6, 0));
hdr->payload_len = htons(seg_len);
hdr->nexthdr = proto;
@@ -1702,7 +1702,7 @@ struct sk_buff *__ip6_make_skb(struct sock *sk,
ip6_flow_hdr(hdr, v6_cork->tclass,
ip6_make_flowlabel(net, skb, fl6->flowlabel,
- np->autoflowlabel, fl6));
+ np->autoflowlabel, fl6, 0));
hdr->hop_limit = v6_cork->hop_limit;
hdr->nexthdr = proto;
hdr->saddr = fl6->saddr;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 3a0ba2a..9c5d129 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1201,7 +1201,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
skb_reset_network_header(skb);
ipv6h = ipv6_hdr(skb);
ip6_flow_hdr(ipv6h, dsfield,
- ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
+ ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6, 0));
ipv6h->hop_limit = hop_limit;
ipv6h->nexthdr = proto;
ipv6h->saddr = fl6->saddr;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2521690..bb47b6c 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -891,6 +891,8 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
struct sock *sk1 = NULL;
#endif
int oif;
+ u8 tclass = 0;
+ __be32 flowlabel = 0;
if (th->rst)
return;
@@ -939,7 +941,21 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
(th->doff << 2);
oif = sk ? sk->sk_bound_dev_if : 0;
- tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0);
+ if (sk) {
+ if (sk_fullsock(sk)) {
+ struct ipv6_pinfo *np = inet6_sk(sk);
+
+ tclass = np->tclass;
+ flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK;
+ } else {
+ struct inet_timewait_sock *tw = inet_twsk(sk);
+
+ tclass = tw->tw_tclass;
+ flowlabel = cpu_to_be32(tw->tw_flowlabel);
+ }
+ }
+ tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1,
+ tclass, flowlabel);
#ifdef CONFIG_TCP_MD5SIG
out:
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 7/7] net: bcmgenet: Utilize bcmgenet_mii_exit() for error path
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
bcmgenet_mii_init() has an error path which is strictly identical to the
unwinding that bcmgenet_mii_exit() does, so have bcmgenet_mii_init()
utilize bcmgenet_mii_exit() for that.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 7fdc352628f9..33d3f60ac74b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -563,7 +563,6 @@ static int bcmgenet_mii_bus_init(struct bcmgenet_priv *priv)
int bcmgenet_mii_init(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
- struct device_node *dn = priv->pdev->dev.of_node;
int ret;
ret = bcmgenet_mii_register(priv);
@@ -577,11 +576,7 @@ int bcmgenet_mii_init(struct net_device *dev)
return 0;
out:
- if (of_phy_is_fixed_link(dn))
- of_phy_deregister_fixed_link(dn);
- of_node_put(priv->phy_dn);
- platform_device_unregister(priv->mii_pdev);
- platform_device_put(priv->mii_pdev);
+ bcmgenet_mii_exit(dev);
return ret;
}
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 6/7] net: bcmgenet: Drop legacy MDIO code
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
Now that we have fully migrated to the mdio-bcm-unimac driver, drop the
legacy MDIO bus code which did duplicate a fair amount of code.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmmii.c | 125 ---------------------------
1 file changed, 125 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 368d5eab306b..7fdc352628f9 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -28,59 +28,6 @@
#include "bcmgenet.h"
-/* read a value from the MII */
-static int bcmgenet_mii_read(struct mii_bus *bus, int phy_id, int location)
-{
- int ret;
- struct net_device *dev = bus->priv;
- struct bcmgenet_priv *priv = netdev_priv(dev);
- u32 reg;
-
- bcmgenet_umac_writel(priv, (MDIO_RD | (phy_id << MDIO_PMD_SHIFT) |
- (location << MDIO_REG_SHIFT)), UMAC_MDIO_CMD);
- /* Start MDIO transaction*/
- reg = bcmgenet_umac_readl(priv, UMAC_MDIO_CMD);
- reg |= MDIO_START_BUSY;
- bcmgenet_umac_writel(priv, reg, UMAC_MDIO_CMD);
- wait_event_timeout(priv->wq,
- !(bcmgenet_umac_readl(priv, UMAC_MDIO_CMD)
- & MDIO_START_BUSY),
- HZ / 100);
- ret = bcmgenet_umac_readl(priv, UMAC_MDIO_CMD);
-
- /* Some broken devices are known not to release the line during
- * turn-around, e.g: Broadcom BCM53125 external switches, so check for
- * that condition here and ignore the MDIO controller read failure
- * indication.
- */
- if (!(bus->phy_ignore_ta_mask & 1 << phy_id) && (ret & MDIO_READ_FAIL))
- return -EIO;
-
- return ret & 0xffff;
-}
-
-/* write a value to the MII */
-static int bcmgenet_mii_write(struct mii_bus *bus, int phy_id,
- int location, u16 val)
-{
- struct net_device *dev = bus->priv;
- struct bcmgenet_priv *priv = netdev_priv(dev);
- u32 reg;
-
- bcmgenet_umac_writel(priv, (MDIO_WR | (phy_id << MDIO_PMD_SHIFT) |
- (location << MDIO_REG_SHIFT) | (0xffff & val)),
- UMAC_MDIO_CMD);
- reg = bcmgenet_umac_readl(priv, UMAC_MDIO_CMD);
- reg |= MDIO_START_BUSY;
- bcmgenet_umac_writel(priv, reg, UMAC_MDIO_CMD);
- wait_event_timeout(priv->wq,
- !(bcmgenet_umac_readl(priv, UMAC_MDIO_CMD) &
- MDIO_START_BUSY),
- HZ / 100);
-
- return 0;
-}
-
/* setup netdev link state when PHY link status change and
* update UMAC and RGMII block when link up
*/
@@ -393,78 +340,6 @@ int bcmgenet_mii_probe(struct net_device *dev)
return 0;
}
-/* Workaround for integrated BCM7xxx Gigabit PHYs which have a problem with
- * their internal MDIO management controller making them fail to successfully
- * be read from or written to for the first transaction. We insert a dummy
- * BMSR read here to make sure that phy_get_device() and get_phy_id() can
- * correctly read the PHY MII_PHYSID1/2 registers and successfully register a
- * PHY device for this peripheral.
- *
- * Once the PHY driver is registered, we can workaround subsequent reads from
- * there (e.g: during system-wide power management).
- *
- * bus->reset is invoked before mdiobus_scan during mdiobus_register and is
- * therefore the right location to stick that workaround. Since we do not want
- * to read from non-existing PHYs, we either use bus->phy_mask or do a manual
- * Device Tree scan to limit the search area.
- */
-static int bcmgenet_mii_bus_reset(struct mii_bus *bus)
-{
- struct net_device *dev = bus->priv;
- struct bcmgenet_priv *priv = netdev_priv(dev);
- struct device_node *np = priv->mdio_dn;
- struct device_node *child = NULL;
- u32 read_mask = 0;
- int addr = 0;
-
- if (!np) {
- read_mask = 1 << priv->phy_addr;
- } else {
- for_each_available_child_of_node(np, child) {
- addr = of_mdio_parse_addr(&dev->dev, child);
- if (addr < 0)
- continue;
-
- read_mask |= 1 << addr;
- }
- }
-
- for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
- if (read_mask & 1 << addr) {
- dev_dbg(&dev->dev, "Workaround for PHY @ %d\n", addr);
- mdiobus_read(bus, addr, MII_BMSR);
- }
- }
-
- return 0;
-}
-
-static int bcmgenet_mii_alloc(struct bcmgenet_priv *priv)
-{
- struct mii_bus *bus;
-
- if (priv->mii_bus)
- return 0;
-
- priv->mii_bus = mdiobus_alloc();
- if (!priv->mii_bus) {
- pr_err("failed to allocate\n");
- return -ENOMEM;
- }
-
- bus = priv->mii_bus;
- bus->priv = priv->dev;
- bus->name = "bcmgenet MII bus";
- bus->parent = &priv->pdev->dev;
- bus->read = bcmgenet_mii_read;
- bus->write = bcmgenet_mii_write;
- bus->reset = bcmgenet_mii_bus_reset;
- snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d",
- priv->pdev->name, priv->pdev->id);
-
- return 0;
-}
-
static struct device_node *bcmgenet_mii_of_find_mdio(struct bcmgenet_priv *priv)
{
struct device_node *dn = priv->pdev->dev.of_node;
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 5/7] net: bcmgenet: utilize generic Broadcom UniMAC MDIO controller driver
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
Update the GENET driver to register an UniMAC MDIO bus controller for
the GENET internal MDIO bus, update the platform data code to attach the
PHY to the correct MDIO bus controller.
The Device Tree portion of the code is mostly left unmodified since the
lookup/binding is done via phandles and Device Tree nodes which are much
more flexible in locating and binding PHYs to their respective MDIO bus
controllers.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/Kconfig | 1 +
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 148 +++++++++++++++++++------
3 files changed, 116 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 285f8bc25682..ec7a798c6bd1 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -65,6 +65,7 @@ config BCMGENET
select PHYLIB
select FIXED_PHY
select BCM7XXX_PHY
+ select MDIO_BCM_UNIMAC
help
This driver supports the built-in Ethernet MACs found in the
Broadcom BCM7xxx Set Top Box family chipset.
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index b9344de669f8..4775999ee016 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -657,6 +657,7 @@ struct bcmgenet_priv {
struct clk *clk;
struct platform_device *pdev;
+ struct platform_device *mii_pdev;
/* WOL */
struct clk *clk_wol;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 071fcbd14e6a..368d5eab306b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -24,6 +24,7 @@
#include <linux/of_net.h>
#include <linux/of_mdio.h>
#include <linux/platform_data/bcmgenet.h>
+#include <linux/platform_data/mdio-bcm-unimac.h>
#include "bcmgenet.h"
@@ -464,31 +465,120 @@ static int bcmgenet_mii_alloc(struct bcmgenet_priv *priv)
return 0;
}
-static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
+static struct device_node *bcmgenet_mii_of_find_mdio(struct bcmgenet_priv *priv)
{
struct device_node *dn = priv->pdev->dev.of_node;
struct device *kdev = &priv->pdev->dev;
- struct phy_device *phydev = NULL;
char *compat;
- int phy_mode;
- int ret;
compat = kasprintf(GFP_KERNEL, "brcm,genet-mdio-v%d", priv->version);
if (!compat)
- return -ENOMEM;
+ return NULL;
priv->mdio_dn = of_find_compatible_node(dn, NULL, compat);
kfree(compat);
if (!priv->mdio_dn) {
dev_err(kdev, "unable to find MDIO bus node\n");
- return -ENODEV;
+ return NULL;
}
- ret = of_mdiobus_register(priv->mii_bus, priv->mdio_dn);
- if (ret) {
- dev_err(kdev, "failed to register MDIO bus\n");
- return ret;
+ return priv->mdio_dn;
+}
+
+static void bcmgenet_mii_pdata_init(struct bcmgenet_priv *priv,
+ struct unimac_mdio_pdata *ppd)
+{
+ struct device *kdev = &priv->pdev->dev;
+ struct bcmgenet_platform_data *pd = kdev->platform_data;
+
+ if (pd->phy_interface != PHY_INTERFACE_MODE_MOCA && pd->mdio_enabled) {
+ /*
+ * Internal or external PHY with MDIO access
+ */
+ if (pd->phy_address >= 0 && pd->phy_address < PHY_MAX_ADDR)
+ ppd->phy_mask = 1 << pd->phy_address;
+ else
+ ppd->phy_mask = 0;
}
+}
+
+static int bcmgenet_mii_wait(void *wait_func_data)
+{
+ struct bcmgenet_priv *priv = wait_func_data;
+
+ wait_event_timeout(priv->wq,
+ !(bcmgenet_umac_readl(priv, UMAC_MDIO_CMD)
+ & MDIO_START_BUSY),
+ HZ / 100);
+ return 0;
+}
+
+static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
+{
+ struct platform_device *pdev = priv->pdev;
+ struct bcmgenet_platform_data *pdata = pdev->dev.platform_data;
+ struct device_node *dn = pdev->dev.of_node;
+ struct unimac_mdio_pdata ppd;
+ struct platform_device *ppdev;
+ struct resource *pres, res;
+ int id, ret;
+
+ pres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ memset(&res, 0, sizeof(res));
+ memset(&ppd, 0, sizeof(ppd));
+
+ ppd.wait_func = bcmgenet_mii_wait;
+ ppd.wait_func_data = priv;
+ ppd.bus_name = "bcmgenet MII bus";
+
+ /* Unimac MDIO bus controller starts at UniMAC offset + MDIO_CMD
+ * and is 2 * 32-bits word long, 8 bytes total.
+ */
+ res.start = pres->start + GENET_UMAC_OFF + UMAC_MDIO_CMD;
+ res.end = res.start + 8;
+ res.flags = IORESOURCE_MEM;
+
+ if (dn)
+ id = of_alias_get_id(dn, "eth");
+ else
+ id = pdev->id;
+
+ ppdev = platform_device_alloc(UNIMAC_MDIO_DRV_NAME, id);
+ if (!ppdev)
+ return -ENOMEM;
+
+ /* Retain this platform_device pointer for later cleanup */
+ priv->mii_pdev = ppdev;
+ ppdev->dev.parent = &pdev->dev;
+ ppdev->dev.of_node = bcmgenet_mii_of_find_mdio(priv);
+ if (pdata)
+ bcmgenet_mii_pdata_init(priv, &ppd);
+
+ ret = platform_device_add_resources(ppdev, &res, 1);
+ if (ret)
+ goto out;
+
+ ret = platform_device_add_data(ppdev, &ppd, sizeof(ppd));
+ if (ret)
+ goto out;
+
+ ret = platform_device_add(ppdev);
+ if (ret)
+ goto out;
+
+ return 0;
+out:
+ platform_device_put(ppdev);
+ return ret;
+}
+
+static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
+{
+ struct device_node *dn = priv->pdev->dev.of_node;
+ struct device *kdev = &priv->pdev->dev;
+ struct phy_device *phydev;
+ int phy_mode;
+ int ret;
/* Fetch the PHY phandle */
priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
@@ -536,33 +626,23 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
{
struct device *kdev = &priv->pdev->dev;
struct bcmgenet_platform_data *pd = kdev->platform_data;
- struct mii_bus *mdio = priv->mii_bus;
+ char phy_name[MII_BUS_ID_SIZE + 3];
+ char mdio_bus_id[MII_BUS_ID_SIZE];
struct phy_device *phydev;
- int ret;
+
+ snprintf(mdio_bus_id, MII_BUS_ID_SIZE, "%s-%d",
+ UNIMAC_MDIO_DRV_NAME, priv->pdev->id);
if (pd->phy_interface != PHY_INTERFACE_MODE_MOCA && pd->mdio_enabled) {
+ snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT,
+ mdio_bus_id, pd->phy_address);
+
/*
* Internal or external PHY with MDIO access
*/
- if (pd->phy_address >= 0 && pd->phy_address < PHY_MAX_ADDR)
- mdio->phy_mask = ~(1 << pd->phy_address);
- else
- mdio->phy_mask = 0;
-
- ret = mdiobus_register(mdio);
- if (ret) {
- dev_err(kdev, "failed to register MDIO bus\n");
- return ret;
- }
-
- if (pd->phy_address >= 0 && pd->phy_address < PHY_MAX_ADDR)
- phydev = mdiobus_get_phy(mdio, pd->phy_address);
- else
- phydev = phy_find_first(mdio);
-
+ phydev = phy_attach(priv->dev, phy_name, pd->phy_interface);
if (!phydev) {
dev_err(kdev, "failed to register PHY device\n");
- mdiobus_unregister(mdio);
return -ENODEV;
}
} else {
@@ -611,7 +691,7 @@ int bcmgenet_mii_init(struct net_device *dev)
struct device_node *dn = priv->pdev->dev.of_node;
int ret;
- ret = bcmgenet_mii_alloc(priv);
+ ret = bcmgenet_mii_register(priv);
if (ret)
return ret;
@@ -625,8 +705,8 @@ int bcmgenet_mii_init(struct net_device *dev)
if (of_phy_is_fixed_link(dn))
of_phy_deregister_fixed_link(dn);
of_node_put(priv->phy_dn);
- mdiobus_unregister(priv->mii_bus);
- mdiobus_free(priv->mii_bus);
+ platform_device_unregister(priv->mii_pdev);
+ platform_device_put(priv->mii_pdev);
return ret;
}
@@ -638,6 +718,6 @@ void bcmgenet_mii_exit(struct net_device *dev)
if (of_phy_is_fixed_link(dn))
of_phy_deregister_fixed_link(dn);
of_node_put(priv->phy_dn);
- mdiobus_unregister(priv->mii_bus);
- mdiobus_free(priv->mii_bus);
+ platform_device_unregister(priv->mii_pdev);
+ platform_device_put(priv->mii_pdev);
}
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 4/7] net: phy: mdio-bcm-unimac: Allow specifying platform data
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
In preparation for having the bcmgenet driver migrate over the
mdio-bcm-unimac driver, add a platform data structure which allows
passing integrating specific details like bus name, wait function to
complete MDIO operations and PHY mask.
We also define what the platform device name contract is by defining
UNIMAC_MDIO_DRV_NAME and moving it to the platform_data header.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
MAINTAINERS | 1 +
drivers/net/phy/mdio-bcm-unimac.c | 28 +++++++++++++++++++++------
include/linux/platform_data/mdio-bcm-unimac.h | 13 +++++++++++++
3 files changed, 36 insertions(+), 6 deletions(-)
create mode 100644 include/linux/platform_data/mdio-bcm-unimac.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 297e610c9163..7d72fdbed6e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5140,6 +5140,7 @@ L: netdev@vger.kernel.org
S: Maintained
F: include/linux/phy.h
F: include/linux/phy_fixed.h
+F: include/linux/platform_data/mdio-bcm-unimac.h
F: drivers/net/phy/
F: Documentation/networking/phy.txt
F: drivers/of/of_mdio.c
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 4e52692f9eea..89425ca48412 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -21,6 +21,8 @@
#include <linux/of_platform.h>
#include <linux/of_mdio.h>
+#include <linux/platform_data/mdio-bcm-unimac.h>
+
#define MDIO_CMD 0x00
#define MDIO_START_BUSY (1 << 29)
#define MDIO_READ_FAIL (1 << 28)
@@ -41,6 +43,8 @@
struct unimac_mdio_priv {
struct mii_bus *mii_bus;
void __iomem *base;
+ int (*wait_func) (void *wait_func_data);
+ void *wait_func_data;
};
static inline void unimac_mdio_start(struct unimac_mdio_priv *priv)
@@ -57,8 +61,9 @@ static inline unsigned int unimac_mdio_busy(struct unimac_mdio_priv *priv)
return __raw_readl(priv->base + MDIO_CMD) & MDIO_START_BUSY;
}
-static int unimac_mdio_poll(struct unimac_mdio_priv *priv)
+static int unimac_mdio_poll(void *wait_func_data)
{
+ struct unimac_mdio_priv *priv = wait_func_data;
unsigned int timeout = 1000;
do {
@@ -77,6 +82,7 @@ static int unimac_mdio_poll(struct unimac_mdio_priv *priv)
static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
{
struct unimac_mdio_priv *priv = bus->priv;
+ int ret;
u32 cmd;
/* Prepare the read operation */
@@ -86,7 +92,7 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
/* Start MDIO transaction */
unimac_mdio_start(priv);
- ret = unimac_mdio_poll(priv);
+ ret = priv->wait_func(priv->wait_func_data);
if (ret)
return ret;
@@ -116,7 +122,7 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
unimac_mdio_start(priv);
- return unimac_mdio_poll(priv);
+ return priv->wait_func(priv->wait_func_data);
}
/* Workaround for integrated BCM7xxx Gigabit PHYs which have a problem with
@@ -165,6 +171,7 @@ static int unimac_mdio_reset(struct mii_bus *bus)
static int unimac_mdio_probe(struct platform_device *pdev)
{
+ struct unimac_mdio_pdata *pdata = pdev->dev.platform_data;
struct unimac_mdio_priv *priv;
struct device_node *np;
struct mii_bus *bus;
@@ -194,7 +201,16 @@ static int unimac_mdio_probe(struct platform_device *pdev)
bus = priv->mii_bus;
bus->priv = priv;
- bus->name = "unimac MII bus";
+ if (pdata) {
+ bus->name = pdata->bus_name;
+ priv->wait_func = pdata->wait_func;
+ priv->wait_func_data = pdata->wait_func_data;
+ bus->phy_mask = ~pdata->phy_mask;
+ } else {
+ bus->name = "unimac MII bus";
+ priv->wait_func_data = priv;
+ priv->wait_func = unimac_mdio_poll;
+ }
bus->parent = &pdev->dev;
bus->read = unimac_mdio_read;
bus->write = unimac_mdio_write;
@@ -241,7 +257,7 @@ MODULE_DEVICE_TABLE(of, unimac_mdio_ids);
static struct platform_driver unimac_mdio_driver = {
.driver = {
- .name = "unimac-mdio",
+ .name = UNIMAC_MDIO_DRV_NAME,
.of_match_table = unimac_mdio_ids,
},
.probe = unimac_mdio_probe,
@@ -252,4 +268,4 @@ module_platform_driver(unimac_mdio_driver);
MODULE_AUTHOR("Broadcom Corporation");
MODULE_DESCRIPTION("Broadcom UniMAC MDIO bus controller");
MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:unimac-mdio");
+MODULE_ALIAS("platform:" UNIMAC_MDIO_DRV_NAME);
diff --git a/include/linux/platform_data/mdio-bcm-unimac.h b/include/linux/platform_data/mdio-bcm-unimac.h
new file mode 100644
index 000000000000..8a5f9f0b2c52
--- /dev/null
+++ b/include/linux/platform_data/mdio-bcm-unimac.h
@@ -0,0 +1,13 @@
+#ifndef __MDIO_BCM_UNIMAC_PDATA_H
+#define __MDIO_BCM_UNIMAC_PDATA_H
+
+struct unimac_mdio_pdata {
+ u32 phy_mask;
+ int (*wait_func)(void *data);
+ void *wait_func_data;
+ const char *bus_name;
+};
+
+#define UNIMAC_MDIO_DRV_NAME "unimac-mdio"
+
+#endif /* __MDIO_BCM_UNIMAC_PDATA_H */
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 3/7] net: phy: mdio-bcm-unimac: Add debug print for PHY workaround
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
In order to be stricly identical to what bcmgenet does, add a debug
print when a PHY workaround during bus->reset() is executed. Preliminary
change to moving bcmgenet towards mdio-bcm-unimac.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/mdio-bcm-unimac.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 97cac10bd0d5..4e52692f9eea 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -154,8 +154,10 @@ static int unimac_mdio_reset(struct mii_bus *bus)
}
for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
- if (read_mask & 1 << addr)
+ if (read_mask & 1 << addr) {
+ dev_dbg(&bus->dev, "Workaround for PHY @ %d\n", addr);
mdiobus_read(bus, addr, MII_BMSR);
+ }
}
return 0;
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 2/7] net: phy: mdio-bcm-unimac: create unique bus names
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
In preparation for having multiple GENET instances in a system (up to
3), make sure that we do include the bus instance number in the name of
the MDIO bus such that we change it from "unimac-mdio" to
"unimac-mdio-0" for instance.
So far, the only user of this driver is using Device Tree, which uses a
lookup/parenting based technique to map PHY devices to their respective
MDIO bus controllers, hence causing no additional changes.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/mdio-bcm-unimac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 226fdccfa1a8..97cac10bd0d5 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -197,7 +197,7 @@ static int unimac_mdio_probe(struct platform_device *pdev)
bus->read = unimac_mdio_read;
bus->write = unimac_mdio_write;
bus->reset = unimac_mdio_reset;
- snprintf(bus->id, MII_BUS_ID_SIZE, "%s", pdev->name);
+ snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", pdev->name, pdev->id);
ret = of_mdiobus_register(bus, np);
if (ret) {
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 1/7] net: phy: mdio-bcm-unimac: factor busy polling loop
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
In-Reply-To: <20170731190429.24204-1-f.fainelli@gmail.com>
Factor the code that does the busy polling on the MDIO_BUSY bit since we
will have different code-paths for for completion depending on whether
we are using interrupts or polling.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/mdio-bcm-unimac.c | 43 +++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 34395230ce70..226fdccfa1a8 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -57,10 +57,26 @@ static inline unsigned int unimac_mdio_busy(struct unimac_mdio_priv *priv)
return __raw_readl(priv->base + MDIO_CMD) & MDIO_START_BUSY;
}
+static int unimac_mdio_poll(struct unimac_mdio_priv *priv)
+{
+ unsigned int timeout = 1000;
+
+ do {
+ if (!unimac_mdio_busy(priv))
+ return 0;
+
+ usleep_range(1000, 2000);
+ } while (timeout--);
+
+ if (!timeout)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
{
struct unimac_mdio_priv *priv = bus->priv;
- unsigned int timeout = 1000;
u32 cmd;
/* Prepare the read operation */
@@ -70,15 +86,9 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
/* Start MDIO transaction */
unimac_mdio_start(priv);
- do {
- if (!unimac_mdio_busy(priv))
- break;
-
- usleep_range(1000, 2000);
- } while (timeout--);
-
- if (!timeout)
- return -ETIMEDOUT;
+ ret = unimac_mdio_poll(priv);
+ if (ret)
+ return ret;
cmd = __raw_readl(priv->base + MDIO_CMD);
@@ -97,7 +107,6 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
int reg, u16 val)
{
struct unimac_mdio_priv *priv = bus->priv;
- unsigned int timeout = 1000;
u32 cmd;
/* Prepare the write operation */
@@ -107,17 +116,7 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
unimac_mdio_start(priv);
- do {
- if (!unimac_mdio_busy(priv))
- break;
-
- usleep_range(1000, 2000);
- } while (timeout--);
-
- if (!timeout)
- return -ETIMEDOUT;
-
- return 0;
+ return unimac_mdio_poll(priv);
}
/* Workaround for integrated BCM7xxx Gigabit PHYs which have a problem with
--
2.9.3
^ permalink raw reply related
* [PATCH net-next 0/7] net: bcmgenet: utilize MDIO unimac driver
From: Florian Fainelli @ 2017-07-31 19:04 UTC (permalink / raw)
To: netdev; +Cc: davem, opendmb, jaedon.shin, pgynther, Florian Fainelli
Hi all,
This patch series migrates the Broadcom GENET driver to use the mdio-bcm-unimac
driver. This MDIO HW is the same as the one GENET internally embedds, yet for
historical reasons the two drivers lived their own lives. Because of the GENET
interrupt situation, we let it specify how it wants to signal MDIO operations
completion using its driver-private waitqueue.
The diffstat is not super impressive, but it's still negative! This would
make it easier in the future to absorb possible workarounds/bugs/features
within the same location.
This was tested on BCM7260 (GENETv5, single instance), BCM7439 (GENETv4, triple
instance) and BCM7445 (bcm_sf2 + mdio-bcm-unimac).
We also now have a nice /proc/iomem output:
f0b00000-f0b0fc4b : /rdb/ethernet@f0b00000
f0b00e14-f0b00e1c : unimac-mdio.0
f0b20000-f0b2fc4b : /rdb/ethernet@f0b20000
f0b20e14-f0b20e1c : unimac-mdio.1
f0b40000-f0b4fc4b : /rdb/ethernet@f0b40000
f0b40e14-f0b40e1c : unimac-mdio.2
Florian Fainelli (7):
net: phy: mdio-bcm-unimac: factor busy polling loop
net: phy: mdio-bcm-unimac: create unique bus names
net: phy: mdio-bcm-unimac: Add debug print for PHY workaround
net: phy: mdio-bcm-unimac: Allow specifying platform data
net: bcmgenet: utilize generic Broadcom UniMAC MDIO controller driver
net: bcmgenet: Drop legacy MDIO code
net: bcmgenet: Utilize bcmgenet_mii_exit() for error path
MAINTAINERS | 1 +
drivers/net/ethernet/broadcom/Kconfig | 1 +
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 256 ++++++++++---------------
drivers/net/phy/mdio-bcm-unimac.c | 71 ++++---
include/linux/platform_data/mdio-bcm-unimac.h | 13 ++
6 files changed, 163 insertions(+), 180 deletions(-)
create mode 100644 include/linux/platform_data/mdio-bcm-unimac.h
--
2.9.3
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Michael S. Tsirkin @ 2017-07-31 18:50 UTC (permalink / raw)
To: Euan Kemp; +Cc: seth.forshee, netdev
In-Reply-To: <20170731183800.4jg2qx42sheq7axw@multivac.euank.com>
On Mon, Jul 31, 2017 at 11:38:00AM -0700, Euan Kemp wrote:
> On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote:
> > I kept this around unchanged from
> > ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal
> > reason not to account for that space: not enough bits to do it. No
> > longer true so let's account for length exactly. I'll send a proper
> > patch after a bit of testing, would appreciate reports reports of
> > whether this helps very much.
>
> I can confirm your patch fixes the issue for me.
>
> - Euan
Patch posted, thanks everyone.
^ permalink raw reply
* [PATCH net] virtio_net: fix truesize for mergeable buffers
From: Michael S. Tsirkin @ 2017-07-31 18:49 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, virtualization, Seth Forshee, David Miller
Seth Forshee noticed a performance degradation with some workloads.
This turns out to be due to packet drops. Euan Kemp noticed that this
is because we drop all packets where length exceeds the truesize, but
for some packets we add in extra memory without updating the truesize.
This in turn was kept around unchanged from ab7db91705e95 ("virtio-net:
auto-tune mergeable rx buffer size for improved performance"). That
commit had an internal reason not to account for the extra space: not
enough bits to do it. No longer true so let's account for the allocated
length exactly.
Many thanks to Seth Forshee for the report and bisecting and Euan Kemp
for debugging the issue.
Fixes: 680557cf79f8 ("virtio_net: rework mergeable buffer handling")
Reported-by: Euan Kemp <euan.kemp@coreos.com>
Tested-by: Euan Kemp <euan.kemp@coreos.com>
Reported-by: Seth Forshee <seth.forshee@canonical.com>
Tested-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This patch is needed on stable as well.
drivers/net/virtio_net.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f41ab0e..98f17b0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -889,21 +889,20 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
buf += headroom; /* advance address leaving hole at front of pkt */
- ctx = (void *)(unsigned long)len;
get_page(alloc_frag->page);
alloc_frag->offset += len + headroom;
hole = alloc_frag->size - alloc_frag->offset;
if (hole < len + headroom) {
/* To avoid internal fragmentation, if there is very likely not
* enough space for another buffer, add the remaining space to
- * the current buffer. This extra space is not included in
- * the truesize stored in ctx.
+ * the current buffer.
*/
len += hole;
alloc_frag->offset += hole;
}
sg_init_one(rq->sg, buf, len);
+ ctx = (void *)(unsigned long)len;
err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
if (err < 0)
put_page(virt_to_head_page(buf));
--
MST
^ permalink raw reply related
* Re: [PATCH 1/1 v2] netfilter: constify nf_conntrack_l3/4proto parameters
From: Pablo Neira Ayuso @ 2017-07-31 18:44 UTC (permalink / raw)
To: Julia Lawall
Cc: David S. Miller, bhumirks, kernel-janitors, Jozsef Kadlecsik,
Florian Westphal, netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <1501443524-13201-2-git-send-email-Julia.Lawall@lip6.fr>
Hi Julia,
On Sun, Jul 30, 2017 at 09:38:44PM +0200, Julia Lawall wrote:
> When a nf_conntrack_l3/4proto parameter is not on the left hand side
> of an assignment, its address is not taken, and it is not passed to a
> function that may modify its fields, then it can be declared as const.
>
> This change is useful from a documentation point of view, and can
> possibly facilitate making some nf_conntrack_l3/4proto structures const
> subsequently.
>
> Done with the help of Coccinelle.
>
> Some spacing adjusted to fit within 80 characters.
patching file include/net/netfilter/nf_conntrack_l3proto.h
Hunk #1 FAILED at 76.
1 out of 1 hunk FAILED -- saving rejects to file
include/net/netfilter/nf_conntrack_l3proto.h.rej
patching file include/net/netfilter/nf_conntrack_l4proto.h
patching file include/net/netfilter/nf_conntrack_timeout.h
patching file net/netfilter/nf_conntrack_core.c
Hunk #1 succeeded at 1176 (offset -7 lines).
Hunk #2 succeeded at 1288 (offset -7 lines).
patching file net/netfilter/nf_conntrack_netlink.c
patching file net/netfilter/nf_conntrack_proto.c
Hunk #2 FAILED at 242.
Hunk #3 FAILED at 271.
Hunk #4 succeeded at 257 with fuzz 1 (offset -29 lines).
Hunk #5 succeeded at 272 (offset -29 lines).
Hunk #6 succeeded at 295 (offset -29 lines).
Hunk #7 succeeded at 366 (offset -29 lines).
Hunk #8 succeeded at 391 (offset -29 lines).
Hunk #9 succeeded at 404 (offset -29 lines).
Hunk #10 succeeded at 415 (offset -29 lines).
Hunk #11 succeeded at 449 (offset -29 lines).
Hunk #12 succeeded at 485 (offset -29 lines).
2 out of 12 hunks FAILED -- saving rejects to file
net/netfilter/nf_conntrack_proto.c.rej
patching file net/netfilter/nfnetlink_cttimeout.c
Could you rebase on top of nf-next git tree?
Thanks a lot!
^ permalink raw reply
* Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.
From: David Ahern @ 2017-07-31 18:40 UTC (permalink / raw)
To: Cong Wang; +Cc: Roopa Prabhu, Hangbin Liu, network dev
In-Reply-To: <CAM_iQpVobEXqE-CWVpQU4b8jnOrzq9EEfszj+SnyqX-RZ+B6Bw@mail.gmail.com>
On 7/31/17 12:37 PM, Cong Wang wrote:
>
>> So Roopa's fibmatch patches brings consistency between IPv4 and IPv6 at
>> the cost of breaking backwards compatibility for IPv6 when the prohibit
>> or blackhole routes are hit.
>>
> There are already many differences between IPv4 and
> IPv6 behaviors, I don't see why this one is so special
> that we have to make it consistent.
yes, and I have sent a number of patches closing the gap.
^ permalink raw reply
* [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
From: Nick Desaulniers @ 2017-07-31 18:39 UTC (permalink / raw)
Cc: mka, lorenzo, Nick Desaulniers, Pablo Neira Ayuso,
Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam,
netdev, linux-kernel
Clang produces the following warning:
net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
logical not is only applied to the left hand side of this comparison
[-Werror,-Wlogical-not-parentheses]
if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
^
add parentheses after the '!' to evaluate the comparison first
add parentheses around left hand side expression to silence this warning
There's not necessarily a bug here, but it's cleaner to use the form:
if (x != 0)
rather than:
if (!x == 0)
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Also, it's even cleaner to use the form:
if (x)
but then if the return codes change from treating 0 as success (unlikely),
then all call sites must be updated.
I'm happy to send v2 that changes to that form, and updates the other call
sites to be:
if (set_h225_addr())
handle_failures()
else
handle_success()
net/ipv4/netfilter/nf_nat_h323.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7ebba0b6..d8fb251fa6e3 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -550,9 +550,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
}
/* Modify signal */
- if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
- &ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
+ if (set_h225_addr(skb, protoff, data, dataoff, taddr,
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ htons(nated_port)) != 0) {
nf_ct_unexpect_related(exp);
return -1;
}
--
2.14.0.rc0.400.g1c36432dff-goog
^ permalink raw reply related
* Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.
From: Cong Wang @ 2017-07-31 18:37 UTC (permalink / raw)
To: David Ahern; +Cc: Roopa Prabhu, Hangbin Liu, network dev
In-Reply-To: <ea0c0319-1b74-da8f-e307-2bf02e674119@gmail.com>
On Fri, Jul 28, 2017 at 10:39 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/28/17 11:13 AM, Roopa Prabhu wrote:
>> for fibmatch, my original intent was to return with an error code.
>> This is similar
>> to the ipv4 behavior. One option is to keep the check in there and put
>> the 'fibmatch'
>> condition around it. But, i do want to make sure that for the fibmatch case,
>> it does not return an error directly on an existing prohibit route
>> entry in the fib.
>> This is probably doable by checking for appropriate
>> net->ipv6.ip6_prohibit_entry entries.
>>
>
> IPv4 does not have the notion of null_entry or prohibit route entries
> which makes IPv4 and IPv6 inconsistent - something we really need to be
> avoiding from a user experience.
>
> We have the following cases:
>
> # ip -4 rule add to 172.16.60.0/24 prohibit
> # ip -4 route add prohibit 172.16.50.0/24
> # ip -6 rule add to 6000::/120 prohibit
> # ip -6 route add prohibit 5000::/120
>
>
> Behavior before Roopa's patch set:
> Rule match:
> # ip ro get 172.16.60.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 6000::1
> prohibit 6000::1 from :: dev lo proto kernel src 2001:db8::3 metric
> 4294967295 error -13 pref medium
>
> Route match:
> # ip ro get 172.16.50.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 5000::1
> prohibit 5000::1 from :: dev lo table red src 2001:db8::3 metric
> 1024 error -13 pref medium
>
>
> Behavior after Roopa's patch set:
> Rule match:
> # ip ro get 172.16.60.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 6000::1
> RTNETLINK answers: Permission denied
>
> Route match:
> # ip ro get 172.16.50.1
> RTNETLINK answers: Permission denied
>
> # ip -6 ro get 5000::1
> RTNETLINK answers: Permission denied
>
There must be a reason why we allocate prohibit entries
dynamically for IPv6 despite we already have a (relatively)
static one.
>From this point of view, we need to dump them, that is,
restore the behavior before Roopa's patch.
>
> So Roopa's fibmatch patches brings consistency between IPv4 and IPv6 at
> the cost of breaking backwards compatibility for IPv6 when the prohibit
> or blackhole routes are hit.
>
There are already many differences between IPv4 and
IPv6 behaviors, I don't see why this one is so special
that we have to make it consistent.
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Euan Kemp @ 2017-07-31 18:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: seth.forshee, netdev
In-Reply-To: <20170731161903-mutt-send-email-mst@kernel.org>
On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote:
> I kept this around unchanged from
> ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal
> reason not to account for that space: not enough bits to do it. No
> longer true so let's account for length exactly. I'll send a proper
> patch after a bit of testing, would appreciate reports reports of
> whether this helps very much.
I can confirm your patch fixes the issue for me.
- Euan
^ permalink raw reply
* Re: Long stalls creating a new netns after a netns with a SMB client exits
From: Rolf Neugebauer @ 2017-07-31 18:37 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAM_iQpWascP6-6x824S60q1WUcLPBgjKrDh+hhyz7EjhYY=q7Q@mail.gmail.com>
On Mon, Jul 31, 2017 at 6:06 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Jul 28, 2017 at 11:58 AM, Rolf Neugebauer
> <rolf.neugebauer@docker.com> wrote:
>> On Fri, Jul 28, 2017 at 6:49 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> Hello,
>>>
>>> On Fri, Jul 28, 2017 at 9:47 AM, Rolf Neugebauer
>>> <rolf.neugebauer@docker.com> wrote:
>>>> Creating the new namespace is stalling for around 200 seconds and
>>>> there 20 odd messages on the console, like:
>>>>
>>>> [ 67.372603] unregister_netdevice: waiting for lo to become free.
>>>> Usage count = 1
>>>>
>>>
>>> Sounds like another netdev refcnt leak.
>>
>> I don't think it's a leak as such because the system eventually
>> recovers after around 200 seconds.
>>
>>>
>>>> Adding a 'sleep 1' before deleting the original network namespace
>>>> "solves" the issue, but that doesn't sound like a good fix. Not using
>>>> unmount also does not help (understandable).
>>>
>>>
>>> Interesting, if sleeping for 1sec help, why did you see the stall for
>>> 200sec? The "leak" should go away eventually without 'sleep 1',
>>> right?
>>
>> Yes. I suspect, that with a sleep some cleanup code (maybe umount)
>> gets run and the ref count gets decremented within the second. Without
>> the sleep, something gets yanked, and whatever operation needs to be
>> done can't get performed, times out after 200s and then the ref count
>> gets decremented.
>
>
> This reminds me of
>
> commit f186ce61bb8235d80068c390dc2aad7ca427a4c2
> Author: Krister Johansen <kjlx@templeofstupid.com>
> Date: Thu Jun 8 13:12:38 2017 -0700
>
> Fix an intermittent pr_emerg warning about lo becoming free.
>
> but this one is merged in 4.12 too, so must be something else.
I've done my last test on 4.9.40 not 4.12.x but another user reported
the same issue on 4.12 and 4.4.x and I just verified on 4.12.4 as
well.
I reported something with similar symptoms, i.e. stalls which
eventually stopped here:
http://marc.info/?l=linux-netdev&m=147870616302799&w=2 but judging by
the backtraces from that post it looks entirely different
Rolf
^ permalink raw reply
* Re: [RFC net-next] net ipv6: convert fib6_table rwlock to a percpu lock
From: Eric Dumazet @ 2017-07-31 18:30 UTC (permalink / raw)
To: Shaohua Li; +Cc: netdev, davem, Kernel-team, Shaohua Li, Wei Wang
In-Reply-To: <1a5ad2f0585aa66496b27e123d1c38b75552df4c.1501520674.git.shli@fb.com>
On Mon, 2017-07-31 at 10:18 -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
>
> In a syn flooding test, the fib6_table rwlock is a significant
> bottleneck. While converting the rwlock to rcu sounds straighforward,
> but is very challenging if it's possible. A percpu spinlock is quite
> trival for this problem since updating the routing table is a rare
> event. In my test, the server receives around 1.5 Mpps in syn flooding
> test without the patch in a dual sockets and 56-CPU system. With the
> patch, the server receives around 3.8Mpps, and perf report doesn't show
> the locking issue.
>
> +static inline void fib6_table_write_lock_bh(struct fib6_table *table)
> +{
> + int i;
> +
> + spin_lock_bh(per_cpu_ptr(table->percpu_tb6_lock, 0));
> + for_each_possible_cpu(i) {
> + if (i == 0)
> + continue;
> + spin_lock_nest_lock(per_cpu_ptr(table->percpu_tb6_lock, i),
> + per_cpu_ptr(table->percpu_tb6_lock, 0));
> + }
> +}
Your code assumes that cpu 0 is valid.
I would rather not hard code this knowledge.
Also this is not clear why you need the nested thing.
^ 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