* Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-07-02 21:24 UTC (permalink / raw)
To: Neal Cardwell
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <CADVnQy=3s4AAX-SyHaRURPgA=mN=m4DBP=Jz3HGt5pBPbiRaaA@mail.gmail.com>
On 7/2/18, 7:57 AM, "Neal Cardwell" <ncardwell@google.com> wrote:
On Sat, Jun 30, 2018 at 9:47 PM Lawrence Brakmo <brakmo@fb.com> wrote:
> I see two issues, one is that entering quickack mode as you
> mentioned does not insure that it will still be on when the CWR
> arrives. The second issue is that the problem occurs right after the
> receiver sends a small reply which results in entering pingpong mode
> right before the sender starts the new request by sending just one
> packet (i.e. forces delayed ack).
>
> I compiled and tested your patch. Both 99 and 99.9 percentile
> latencies are around 40ms. Looking at the packet traces shows that
> some CWR marked packets are not being ack immediately (delayed by
> 40ms).
Thanks, Larry! So your tests provide nice, specific evidence that it
is good to force an immediate ACK when a receiver receives a packet
with CWR marked. Given that, I am wondering what the simplest way is
to achieve that goal.
What if, rather than plumbing a new specific signal into
__tcp_ack_snd_check(), we use the existing general quick-ack
mechanism, where various parts of the TCP stack (like
__tcp_ecn_check_ce()) are already using the quick-ack mechanism to
"remotely" signal to __tcp_ack_snd_check() that they want an immediate
ACK.
For example, would it work to do something like:
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c53ae5fc834a5..8168d1938b376 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -262,6 +262,12 @@ static void __tcp_ecn_check_ce(struct sock *sk,
const struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
+ /* If the sender is telling us it has entered CWR, then its cwnd may be
+ * very low (even just 1 packet), so we should ACK immediately.
+ */
+ if (tcp_hdr(skb)->cwr)
+ tcp_enter_quickack_mode(sk, 2);
+
switch (TCP_SKB_CB(skb)->ip_dsfield & INET_ECN_MASK) {
case INET_ECN_NOT_ECT:
/* Insure that GCN will not continue to mark packets. */
And then since that broadens the mission of this function beyond
checking just the ECT/CE bits, I supposed we could rename the
__tcp_ecn_check_ce() and tcp_ecn_check_ce() functions to
__tcp_ecn_check() and tcp_ecn_check(), or something like that.
Would that work for this particular issue?
Neal
Thanks Neal, it does work and is cleaner than what I was doing. I will submit a revised patch set.
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-02 21:24 UTC (permalink / raw)
To: Neal Cardwell
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <CADVnQykHXktzKY_jXa9GRwPciTf-1CsAegGRPyUc+F1mdMMmBw@mail.gmail.com>
On 7/2/18, 8:18 AM, "netdev-owner@vger.kernel.org on behalf of Neal Cardwell" <netdev-owner@vger.kernel.org on behalf of ncardwell@google.com> wrote:
On Fri, Jun 29, 2018 at 9:48 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
> notifications to keep track if it needs to send an ACK for packets that
> were received with a particular ECN state but whose ACK was delayed.
>
> Under some circumstances, for example when a delayed ACK is sent with a
> data packet, DCTCP state was not being updated due to a lack of
> notification that the previously delayed ACK was sent. As a result, it
> would sometimes send a duplicate ACK when a new data packet arrived.
>
> This patch insures that DCTCP's state is correctly updated so it will
> not send the duplicate ACK.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> net/ipv4/tcp_output.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index f8f6129160dd..41f6ad7a21e4 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, unsigned int pkts)
> __sock_put(sk);
> }
> tcp_dec_quickack_mode(sk, pkts);
> + if (inet_csk_ack_scheduled(sk))
> + tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
> inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
> }
Thanks for this fix! Seems like this would work, but if I am reading
this correctly then it seems like this would cause a duplicate call to
tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) when we are sending a pure
ACK (delayed or non-delayed):
(1) once from tcp_send_ack() before we send the ACK:
tcp_send_ack(struct sock *sk)
-> tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
(2) then again from tcp_event_ack_sent() after we have sent the ACK:
tcp_event_ack_sent()
-> if (inet_csk_ack_scheduled(sk))
tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
What if we remove the original CA_EVENT_NON_DELAYED_ACK call and just
replace it with your new one? (not compiled, not tested):
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 3889dcd4868d4..bddb49617d9be 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -184,6 +184,8 @@ static inline void tcp_event_ack_sent(struct sock
*sk, unsigned int pkts)
__sock_put(sk);
}
tcp_dec_quickack_mode(sk, pkts);
+ if (inet_csk_ack_scheduled(sk))
+ tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
}
@@ -3836,8 +3838,6 @@ void tcp_send_ack(struct sock *sk)
if (sk->sk_state == TCP_CLOSE)
return;
- tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
-
/* We are not putting this on the write queue, so
* tcp_transmit_skb() will set the ownership to this
* sock.
Aside from lower CPU overhead, one nice benefit of that is that we
then only call tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) in one
place, which might be a little easier to reason about.
Does that work?
neal
Thanks Neal, good catch! I will resubmit the patch.
^ permalink raw reply
* Re: [PATCH net-next 0/7] net/ipv6: Fix route append and replace use cases
From: David Ahern @ 2018-07-02 21:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Thomas.Winter, idosch, sharpd, roopa
In-Reply-To: <6d61c529-5b51-dc3b-86ee-e912f1a8e0a8@gmail.com>
On 5/22/18 2:44 PM, David Ahern wrote:
> On 5/22/18 12:46 PM, David Miller wrote:
>>
>> Ok, I'll apply this series.
>>
>> But if this breaks things for anyone in a practical way, I am unfortunately
>> going to have to revert no matter how silly the current behavior may be.
>>
>
> Understood. I have to try the best option first. I'll look at
> regressions if they happen.
>
Debugging the problem Sowmini reported and I discovered NetworkManager
likes to manage link-local prefix routes (fe80::/64), and it likes to do
so with NLM_F_APPEND so the routes end up like this:
fe80::/64 dev eth0 proto kernel metric 100 pref medium
fe80::/64 dev eth1 proto kernel metric 101 pref medium
fe80::/64 proto kernel metric 256
nexthop dev eth1 weight 1
nexthop dev eth0 weight 1 pref medium
NM deletes the kernel installed routes, and then adds new ones -- twice:
once with the custom metric and then re-installs the route it deleted
but with the append flag.
Given the short amount of time left for 4.18 (and summer PTO), I think a
revert is the only option; we can try again in a future release.
^ permalink raw reply
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
From: Andrew Lunn @ 2018-07-02 21:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <fb72a450-8f15-2f34-38ae-26c7c3a5d159@gmail.com>
> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
This bit you probably want to keep. The PHY never says it support
Pause. The MAC needs to enable pause if the MAC supports pause.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Andrew Lunn @ 2018-07-02 21:20 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <5be3d523-bb26-c2d6-71f4-165e9c6e45a9@gmail.com>
On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
> Change rtl_speed_down() to use phylib.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 311321ee..807fbc75 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
> }
>
> + /* We may have called rtl_speed_down before */
> + dev->phydev->advertising = dev->phydev->supported;
> + genphy_config_aneg(dev->phydev);
> +
> genphy_soft_reset(dev->phydev);
>
> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
> }
> }
>
> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
> +
> static void rtl_speed_down(struct rtl8169_private *tp)
> {
> - u32 adv;
> - int lpa;
> + struct phy_device *phydev = tp->dev->phydev;
> + u32 adv = phydev->lp_advertising & phydev->supported;
>
> - rtl_writephy(tp, 0x1f, 0x0000);
> - lpa = rtl_readphy(tp, MII_LPA);
> + if (adv & BASET10)
> + phydev->advertising &= ~(BASET100 | BASET1000);
> + else if (adv & BASET100)
> + phydev->advertising &= ~BASET1000;
>
> - if (lpa & (LPA_10HALF | LPA_10FULL))
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
> - else if (lpa & (LPA_100HALF | LPA_100FULL))
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
> - else
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
> - (tp->mii.supports_gmii ?
> - ADVERTISED_1000baseT_Half |
> - ADVERTISED_1000baseT_Full : 0);
> -
> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> - adv);
> + genphy_config_aneg(phydev);
> }
It probably it is me being too tired, but i don't get what this is
doing? Changing the local advertisement based on what the remote is
advertising. Why?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 06/10] r8169: use phy_mii_ioctl
From: Andrew Lunn @ 2018-07-02 21:13 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <9da55dc3-6e96-14e2-ac1c-1d621da533fe@gmail.com>
On Mon, Jul 02, 2018 at 09:37:05PM +0200, Heiner Kallweit wrote:
> Switch to using phy_mii_ioctl().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH bpf 3/3] bpf: undo prog rejection on read-only lock failure
From: Daniel Borkmann @ 2018-07-02 21:12 UTC (permalink / raw)
To: Kees Cook; +Cc: Alexei Starovoitov, Network Development, Laura Abbott
In-Reply-To: <CAGXu5jLCFhxw2MVg4Gfz=yrYzxONr0ESHtePPeS=xgM3mQUhsQ@mail.gmail.com>
On 07/02/2018 08:48 PM, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 4:47 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 06/29/2018 08:42 PM, Kees Cook wrote:
>>> On Thu, Jun 28, 2018 at 2:34 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> Kees suggested that if set_memory_*() can fail, we should annotate it with
>>>> __must_check, and all callers need to deal with it gracefully given those
>>>> set_memory_*() markings aren't "advisory", but they're expected to actually
>>>> do what they say. This might be an option worth to move forward in future
>>>> but would at the same time require that set_memory_*() calls from supporting
>>>> archs are guaranteed to be "atomic" in that they provide rollback if part
>>>> of the range fails, once that happened, the transition from RW -> RO could
>>>> be made more robust that way, while subsequent RO -> RW transition /must/
>>>> continue guaranteeing to always succeed the undo part.
>>>
>>> Does this mean we can have BPF filters that aren't read-only then?
>>> What's the situation where set_memory_ro() fails? (Can it be induced
>>> by the user?)
>>
>> My understanding is that the cpa_process_alias() would attempt to also change
>> attributes of physmap ranges, and it found that a large page had to be split
>> for this but failed in doing so thus attributes couldn't be updated there due
>> to page alloc error. Attempting to change the primary mapping which would be
>> directly the addr passed to set_memory_ro() was however set to read-only
>> despite error. While for reproduction I had a toggle on the alloc_pages() in
>> split_large_page() to have it fail, I only could trigger it occasionally; I
>> used the selftest suite in a loop to stress test and it hit about or twice
>> over hours.
>
> Okay, so it's pretty rare; that's good! :P
>
> It really seems like this should be a situation that never fails, but
> if we ARE going to allow failures, then I think we need to propagate
> them up to callers. That means modules could fail to load in these
> cases, etc, etc. Since this is a fundamental protection, we need to
> either never fail to set things RO or we need to disallow operation
> continuing in the face of something NOT being RO.
Yeah, fully agree with you, and set_memory_*() would need to be reworked for the
archs implementing them to recover from failure and rollback any attribute changes
already done from the call. Today it's not the case, so this would be first step,
and second step to add the checks for error and bail out from various call-sites.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset
From: Andrew Lunn @ 2018-07-02 21:11 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <968b2379-9201-d4e5-502c-566dfef1a1ea@gmail.com>
On Mon, Jul 02, 2018 at 09:37:03PM +0200, Heiner Kallweit wrote:
> Switch to using phy_ethtool_nway_reset().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
From: Andrew Lunn @ 2018-07-02 21:08 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <ac6d5745-2c4c-d229-12f3-b6b45b95b8ed@gmail.com>
On Mon, Jul 02, 2018 at 09:36:58PM +0200, Heiner Kallweit wrote:
> Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
> to do an explicit PHY soft reset because some chips use the genphy driver
> which uses a no-op as soft_reset callback.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
From: Andrew Lunn @ 2018-07-02 21:06 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <318ae669-d89c-636d-94ac-d4c9f662400e@gmail.com>
> static void r8168_pll_power_down(struct rtl8169_private *tp)
> {
> if (r8168_check_dash(tp))
> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
> if (rtl_wol_pll_power_down(tp))
> return;
>
> - r8168_phy_power_down(tp);
> + /* cover the case that PHY isn't connected */
> + phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
This could do some more explanation. Why would it not be connected?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
From: Andrew Lunn @ 2018-07-02 21:02 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <60049e7e-b86d-1968-cdfd-7e0f91a25b88@gmail.com>
> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
> +{
> + struct rtl8169_private *tp = mii_bus->priv;
> +
> + return rtl_readphy(tp, phyreg);
So there is no support for phyaddr?
It would be better to trap the phyaddr which are not supported and
return 0xffff.
Andrew
^ permalink raw reply
* [PATCH] gen_stats: Fix netlink stats dumping in the presence of padding
From: Toke Høiland-Jørgensen @ 2018-07-02 20:52 UTC (permalink / raw)
To: netdev; +Cc: cake
The gen_stats facility will add a header for the toplevel nlattr of type
TCA_STATS2 that contains all stats added by qdisc callbacks. A reference
to this header is stored in the gnet_dump struct, and when all the
per-qdisc callbacks have finished adding their stats, the length of the
containing header will be adjusted to the right value.
However, on architectures that need padding (i.e., that don't set
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS), the padding nlattr is added
before the stats, which means that the stored pointer will point to the
padding, and so when the header is fixed up, the result is just a very
big padding nlattr. Because most qdiscs also supply the legacy TCA_STATS
struct, this problem has been mostly invisible, but we exposed it with
the netlink attribute-based statistics in CAKE.
Fix the issue by fixing up the stored pointer if it points to a padding
nlattr.
Tested-by: Pete Heist <pete@heistp.net>
Tested-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
net/core/gen_stats.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index b2b2323bdc84..188d693cb251 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -77,8 +77,20 @@ gnet_stats_start_copy_compat(struct sk_buff *skb, int type, int tc_stats_type,
d->lock = lock;
spin_lock_bh(lock);
}
- if (d->tail)
- return gnet_stats_copy(d, type, NULL, 0, padattr);
+ if (d->tail) {
+ int ret = gnet_stats_copy(d, type, NULL, 0, padattr);
+
+ /* The initial attribute added in gnet_stats_copy() may be
+ * preceded by a padding attribute, in which case d->tail will
+ * end up pointing at the padding instead of the real attribute.
+ * Fix this so gnet_stats_finish_copy() adjusts the length of
+ * the right attribute.
+ */
+ if (ret == 0 && d->tail->nla_type == padattr)
+ d->tail = (struct nlattr *)((char *)d->tail +
+ NLA_ALIGN(d->tail->nla_len));
+ return ret;
+ }
return 0;
}
^ permalink raw reply related
* [PATCH net] r8169: fix mac address change
From: Heiner Kallweit @ 2018-07-02 20:49 UTC (permalink / raw)
To: David Miller, Corinna Vinschen, netdev@vger.kernel.org
Network core refuses to change mac address because flag
IFF_LIVE_ADDR_CHANGE isn't set. Set this missing flag.
Fixes: 1f7aa2bc268e ("r8169: simplify rtl_set_mac_address")
Reported-by: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f80ac894..a390db27 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7607,6 +7607,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
NETIF_F_HW_VLAN_CTAG_RX;
dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
NETIF_F_HIGHDMA;
+ dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
tp->cp_cmd |= RxChkSum | RxVlan;
--
2.18.0
^ permalink raw reply related
* RE: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Ben Whitten @ 2018-07-02 20:43 UTC (permalink / raw)
To: Andreas Färber, Mark Brown
Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Jian-Hong Pan, Jiri Pirko,
Marcel Holtmann, David S . Miller, Matthias Brugger, Janus Piwek,
Michael Röder, Dollar Chen, Ken Yu, Steve deRosier,
linux-spi@vger.kernel.org, LoRa_Community_Support@semtech.com
In-Reply-To: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de>
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
>
> Hi Mark,
>
> This driver is still evolving, there's newer code on my lora-next branch
> already: https://github.com/afaerber/linux/commits/lora-next
>
> The reason you're in CC on this RFC is two-fold:
>
> 1) You applied Ben's patch to associate "semtech,sx1301" with spidev,
> whereas I am now preparing a new driver for the same compatible.
>
> 2) This SPI device is in turn exposing the two SPI masters that you
> already found below, and I didn't see a sane way to split that code out
> into drivers/spi/, so it's in drivers/net/lora/ here - has there been
> any precedence either way?
In my work in progress driver I just register one controller for the sx1301 with two chip selects and use the chip select information to choose the correct radio to send to, this is based on the DT reg information. No need to register two separate masters.
> More inline ...
>
> Am 02.07.2018 um 18:12 schrieb Mark Brown:
> > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote:
> >
> >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable)
> >> +{
> >> + int ret;
> >> +
> >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0");
> >> +
> >> + if (enable)
> >> + return;
> >> +
> >> + ret = sx1301_radio_set_cs(spi->controller, enable);
> >> + if (ret)
> >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret);
> >> +}
> >
> > So we never disable chip select?
>
> Not here, I instead did that in transfer_one below.
>
> Unfortunately there seems to be no documentation, only reference code:
>
> https://github.com/Lora-
> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121
> https://github.com/Lora-
> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165
>
> It sets CS to 0 before writing to address and data registers, then
> immediately sets CS to 1 and back to 0 before reading or ending the
> write transaction. I've tried to force the same behavior in this driver.
> My guess was that CS is high-active during the short 1-0 cycle, because
> if it's low-active during the register writes then why the heck is it
> set to 0 again in the end instead of keeping at 1... confusing.
>
> Maybe the Semtech folks CC'ed can comment how these registers work?
>
> >> + if (tx_buf) {
> >> + ret = sx1301_write(ssx->parent, ssx->regs +
> REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> >
> > This looks confused. We're in an if (tx_buf) block but there's a use of
> > the ternery operator that appears to be checking if we have a tx_buf?
>
> Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl
> will complain about lines too long, and TODOs are sprinkled all over or
> not even mentioned. It's a Proof of Concept that a net_device could work
> for a wide range of spi and serdev based drivers, and on top this device
> has more than one channel, which may influence network-level design
> discussions.
>
> That said, I'll happily drop the second check. Thanks for spotting!
>
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio address write
> failed\n");
> >> + return ret;
> >> + }
> >> +
> >> + ret = sx1301_write(ssx->parent, ssx->regs +
> REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio data write failed\n");
> >> + return ret;
> >> + }
> >
> > This looks awfully like you're coming in at the wrong abstraction layer
> > and the hardware actually implements a register abstraction rather than
> > a SPI one so you should be using regmap as the abstraction.
>
> I don't understand. Ben has suggested using regmap for the SPI _device_
> that we're talking to, which may be a good idea. But this SX1301 device
> in turn has two SPI _masters_ talking to an SX125x slave each. I don't
> see how using regmap instead of my wrappers avoids this spi_controller?
> The whole point of this spi_controller is to abstract and separate the
> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver,
> instead of mixing it into the SX1301 driver - to me that looks cleaner
> and more extensible. It also has the side-effect that we could configure
> the two radios via DT (frequencies, clk output, etc.).
You want an SPI controller in the SX1301 as the down stream radios are SPI and could be attached directly to a host SPI bus, makes sense to have one radio driver and talk through the SX1301.
But you should use the regmap to access the SX1301 master controller registers.
Example I use with one SPI master and some clock info:
eg:
sx1301: sx1301@0 {
compatible = "semtech,sx1301";
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <8000000>;
gpios-reset = <&pioA 26 GPIO_ACTIVE_HIGH>;
clocks = <&radio1 0>, <&clkhs 0>;
clock-names = "clk32m", "clkhs";
radio0: sx1257@0 {
compatible = "semtech,sx125x";
reg = <0>;
spi-max-frequency = <8000000>;
tx;
clocks = <&tcxo 0>;
clock-names = "tcxo";
};
radio1: sx1257@1 {
compatible = "semtech,sx125x";
reg = <1>;
spi-max-frequency = <8000000>;
#clock-cells = <0>;
clocks = <&tcxo 0>;
clock-names = "tcxo";
clock-output-names = "clk32m";
};
};
> You will find a datasheet with some diagrams mentioning "SPI" at:
> https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301
>
> >> + if (rx_buf) {
> >> + ret = sx1301_read(ssx->parent, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio data read failed\n");
> >> + return ret;
> >> + }
> >> + }
> >
> > For a read we never set an address?
>
> To read, you first write the address via tx_buf, then either in the same
> transfer in the third byte or in a subsequent one-byte transfer as first
> byte you get the data.
>
> If you have better ideas how to structure this, do let me know.
>
> >> +static void sx1301_radio_setup(struct spi_controller *ctrl)
> >> +{
> >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS;
> >
> > This controller has no chip select but we provided a set_cs operation?
>
> Oops, I played around with those two options and was hoping SPI_NO_CS
> would avoid the undesired set_cs invocations, but it didn't work as
> expected and so I added the "if (enabled)" check above.
>
> Thanks for your review,
>
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Alexander Aring @ 2018-07-02 20:43 UTC (permalink / raw)
To: Michael Scott
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <516fbc65-cc4b-8016-de5a-e2240b779d15@opensourcefoundries.com>
Hi,
On Mon, Jul 02, 2018 at 12:45:41PM -0700, Michael Scott wrote:
> Hello Alexander,
>
...
> > Question is for me: which upper layer wants access MAC header here on
> > receive path?
> > It cannot parsed anyhow because so far I know no upper layer can parse
> > at the moment 802.15.4 frames (which is a complex format). Maybe over
> > some header_ops callback?
>
> I was testing a C program which performs NAT64 handling on packets
> destined to a certain IPv6 subnet (64:ff9b::). To do this, the application
> opens a RAW socket like this: sniff_sock = socket(PF_PACKET, SOCK_RAW,
> htons(ETH_P_ALL)); It then sets promiscuous mode and enters a looping call
> of:
> length = recv(sniff_sock, buffer, PACKET_BUFFER, MSG_TRUNC); My host PC
> kernel would then promptly crash on me. (I'm going to purposely avoid the
> obvious point of: this probably isn't the best way to parse packets for
> NAT64 translation as you will get every single packet incoming or outgoing
> on the host.) Turns out, testing the program on an 802.15.4 6lowpan
> interface exposed some of the issues which this mailing list (but not
> myself) is well aware of (no L2 data in the RAW packets) and also led me to
> debugging this patch to stop the kernel crash. TL;DR: To summarize, any
> PF_PACKET SOCK_RAW socket which recv()'s IPv6 data from a 6lowpan node will
> cause this kernel crash eventually (checked on kernel 4.15, 4.16, 4.17 and
> 4.18-rc1). - Mike
> >
"any PF_PACKET SOCK_RAW" can't be otherwise I would also see it with my
sniffer programs e.g. wireshark or tcpdump which use libpcap.
There need to be some different in the handling. This is what I have
currently in my mind.
I currently not sure how to set skb->mac_header if interface is RAW_IP.
It seems there is an indicator that mac header is not set. Example:
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 6b1042e21656..e6ec2df3afe0 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
hdr.hop_limit, &hdr.daddr);
skb_push(skb, sizeof(hdr));
+ skb->mac_header = (typeof(skb->mac_header))~0U;
skb_reset_network_header(skb);
skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
Maybe we should lookup what skb->mac_header points to on tun interfaces
then do the same.
- Alex
^ permalink raw reply related
* Re: Regression introduced by "r8169: simplify rtl_set_mac_address"
From: Heiner Kallweit @ 2018-07-02 20:35 UTC (permalink / raw)
To: Corinna Vinschen; +Cc: netdev@vger.kernel.org
In-Reply-To: <20180702194812.GG3111@calimero.vinschen.de>
On 02.07.2018 21:48, Corinna Vinschen wrote:
> Hi,
>
> the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address",
> introduced a regression found by trying to team a r8169 NIC.
>
Thanks for reporting!
> Try the following (assuming the r8169 NIC is eth0):
>
> $ nmcli con add type team con-name team0 ifname nm-team config \
> '{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \
> ipv4.method disable ipv6.method ignore
> $ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team
> $ nmcli con up team0.0
> $ teamdctl nm-team port present eth0
> command call failed (No such device)
>
> Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify
> rtl_set_mac_address" as the culprit. Reverting this patch fixes
> the issue and the teamdctl call succeeds.
>
> The reason is apparently the usage of eth_mac_addr here. eth_mac_addr
> calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE.
> Debugging shows this flag not being set on r8169, thus
> eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims
> "No such device", rather than "Device or resource busy", but that's not
> the point here).
>
> Note that other devices like igb, don't call eth_mac_addr either, but
> rather call memcpy by themselves to copy the new MAC, just as the
> original r8169 code did, too. Consequentially this problem is not
> present on igb.
>
Doing the memcpy directly in the driver (like it was before) would be
a solution, however I'd consider this more a workaround because purpose
of the core functions is to encapsulate such details.
> I suggest to revert this change in the first place, but I wonder if
> we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers.
>
I'd prefer to keep the code and set flag IFF_LIVE_ADDR_CHANGE in the
driver. Setting this flag via ethtool --set-priv-flags in theory would
also be an option, however the r8169 driver doesn't implement the
ethtool_ops set_priv_flags callback.
>
> Thanks,
> Corinna
>
Rgds, Heiner
^ permalink raw reply
* [PATCH wpan 2/2] net: mac802154: tx: expand tailroom if necessary
From: Alexander Aring @ 2018-07-02 20:32 UTC (permalink / raw)
To: stefan; +Cc: linux-wpan, netdev, kernel, Alexander Aring
In-Reply-To: <20180702203203.21316-1-aring@mojatatu.com>
This patch is necessary if case of AF_PACKET or other socket interface
which I am aware of it and didn't allocated the necessary room.
Reported-by: David Palma <david.palma@ntnu.no>
Reported-by: Rabi Narayan Sahoo <rabinarayans0828@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
net/mac802154/tx.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 7e253455f9dd..bcd1a5e6ebf4 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -63,8 +63,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
int ret;
if (!(local->hw.flags & IEEE802154_HW_TX_OMIT_CKSUM)) {
- u16 crc = crc_ccitt(0, skb->data, skb->len);
+ struct sk_buff *nskb;
+ u16 crc;
+
+ if (unlikely(skb_tailroom(skb) < IEEE802154_FCS_LEN)) {
+ nskb = skb_copy_expand(skb, 0, IEEE802154_FCS_LEN,
+ GFP_ATOMIC);
+ if (likely(nskb)) {
+ consume_skb(skb);
+ skb = nskb;
+ } else {
+ goto err_tx;
+ }
+ }
+ crc = crc_ccitt(0, skb->data, skb->len);
put_unaligned_le16(crc, skb_put(skb, 2));
}
--
2.11.0
^ permalink raw reply related
* [PATCH wpan 1/2] net: 6lowpan: fix reserved space for single frames
From: Alexander Aring @ 2018-07-02 20:32 UTC (permalink / raw)
To: stefan; +Cc: linux-wpan, netdev, kernel, Alexander Aring
This patch fixes patch add handling to take care tail and headroom for
single 6lowpan frames. We need to be sure we have a skb with the right
head and tailroom for single frames. This patch do it by using
skb_copy_expand() if head and tailroom is not enough allocated by upper
layer.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195059
Reported-by: David Palma <david.palma@ntnu.no>
Reported-by: Rabi Narayan Sahoo <rabinarayans0828@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
net/ieee802154/6lowpan/tx.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index e6ff5128e61a..d0c4d220de08 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -265,9 +265,24 @@ netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *ldev)
/* We must take a copy of the skb before we modify/replace the ipv6
* header as the header could be used elsewhere
*/
- skb = skb_unshare(skb, GFP_ATOMIC);
- if (!skb)
- return NET_XMIT_DROP;
+ if (unlikely(skb_headroom(skb) < ldev->needed_headroom ||
+ skb_tailroom(skb) < ldev->needed_tailroom)) {
+ struct sk_buff *nskb;
+
+ nskb = skb_copy_expand(skb, ldev->needed_headroom,
+ ldev->needed_tailroom, GFP_ATOMIC);
+ if (likely(skb)) {
+ consume_skb(skb);
+ skb = nskb;
+ } else {
+ kfree_skb(skb);
+ return NET_XMIT_DROP;
+ }
+ } else {
+ skb = skb_unshare(skb, GFP_ATOMIC);
+ if (!skb)
+ return NET_XMIT_DROP;
+ }
ret = lowpan_header(skb, ldev, &dgram_size, &dgram_offset);
if (ret < 0) {
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net-next] openvswitch: kernel datapath clone action
From: Yifeng Sun @ 2018-07-02 15:18 UTC (permalink / raw)
To: pshelar, azhou, netdev; +Cc: Yifeng Sun
Add 'clone' action to kernel datapath by using existing functions.
When actions within clone don't modify the current flow, the flow
key is not cloned before executing clone actions.
This is a follow up patch for this incomplete work:
https://patchwork.ozlabs.org/patch/722096/
v1 -> v2:
Refactor as advised by reviewer.
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Andy Zhou <azhou@ovn.org>
---
include/linux/openvswitch.h | 5 +++
include/uapi/linux/openvswitch.h | 3 ++
net/openvswitch/actions.c | 33 ++++++++++++++++++
net/openvswitch/flow_netlink.c | 73 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 114 insertions(+)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e6b240b6..379affc 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -21,4 +21,9 @@
#include <uapi/linux/openvswitch.h>
+#define OVS_CLONE_ATTR_EXEC 0 /* Specify an u32 value. When nonzero,
+ * actions in clone will not change flow
+ * keys. False otherwise.
+ */
+
#endif /* _LINUX_OPENVSWITCH_H */
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 863aaba..dbe0cbe 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -840,6 +840,8 @@ struct ovs_action_push_eth {
* @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
* @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the
* packet, or modify the packet (e.g., change the DSCP field).
+ * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
+ * actions without affecting the original packet and key.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -873,6 +875,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */
OVS_ACTION_ATTR_POP_NSH, /* No argument. */
OVS_ACTION_ATTR_METER, /* u32 meter ID. */
+ OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
__OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df2..85ae53d 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1057,6 +1057,28 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
clone_flow_key);
}
+/* When 'last' is true, clone() should always consume the 'skb'.
+ * Otherwise, clone() should keep 'skb' intact regardless what
+ * actions are executed within clone().
+ */
+static int clone(struct datapath *dp, struct sk_buff *skb,
+ struct sw_flow_key *key, const struct nlattr *attr,
+ bool last)
+{
+ struct nlattr *actions;
+ struct nlattr *clone_arg;
+ int rem = nla_len(attr);
+ bool dont_clone_flow_key;
+
+ /* The first action is always 'OVS_CLONE_ATTR_ARG'. */
+ clone_arg = nla_data(attr);
+ dont_clone_flow_key = nla_get_u32(clone_arg);
+ actions = nla_next(clone_arg, &rem);
+
+ return clone_execute(dp, skb, key, 0, actions, rem, last,
+ !dont_clone_flow_key);
+}
+
static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key,
const struct nlattr *attr)
{
@@ -1336,6 +1358,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
consume_skb(skb);
return 0;
}
+ break;
+
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = clone(dp, skb, key, a, last);
+ if (last)
+ return err;
+
+ break;
+ }
}
if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 391c407..a70097e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2460,6 +2460,40 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
return 0;
}
+static int validate_and_copy_clone(struct net *net,
+ const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa,
+ __be16 eth_type, __be16 vlan_tci,
+ bool log, bool last)
+{
+ int start, err;
+ u32 exec;
+
+ if (nla_len(attr) && nla_len(attr) < NLA_HDRLEN)
+ return -EINVAL;
+
+ start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CLONE, log);
+ if (start < 0)
+ return start;
+
+ exec = last || !actions_may_change_flow(attr);
+
+ err = ovs_nla_add_action(sfa, OVS_CLONE_ATTR_EXEC, &exec,
+ sizeof(exec), log);
+ if (err)
+ return err;
+
+ err = __ovs_nla_copy_actions(net, attr, key, sfa,
+ eth_type, vlan_tci, log);
+ if (err)
+ return err;
+
+ add_nested_action_end(*sfa, start);
+
+ return 0;
+}
+
void ovs_match_init(struct sw_flow_match *match,
struct sw_flow_key *key,
bool reset_key,
@@ -2849,6 +2883,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
[OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1,
[OVS_ACTION_ATTR_POP_NSH] = 0,
[OVS_ACTION_ATTR_METER] = sizeof(u32),
+ [OVS_ACTION_ATTR_CLONE] = (u32)-1,
};
const struct ovs_action_push_vlan *vlan;
int type = nla_type(a);
@@ -3038,6 +3073,18 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
/* Non-existent meters are simply ignored. */
break;
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = validate_and_copy_clone(net, a, key, sfa,
+ eth_type, vlan_tci,
+ log, last);
+ if (err)
+ return err;
+ skip_copy = true;
+ break;
+ }
+
default:
OVS_NLERR(log, "Unknown Action type %d", type);
return -EINVAL;
@@ -3116,6 +3163,26 @@ static int sample_action_to_attr(const struct nlattr *attr,
return err;
}
+static int clone_action_to_attr(const struct nlattr *attr,
+ struct sk_buff *skb)
+{
+ struct nlattr *start;
+ int err = 0, rem = nla_len(attr);
+
+ start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE);
+ if (!start)
+ return -EMSGSIZE;
+
+ err = ovs_nla_put_actions(nla_data(attr), rem, skb);
+
+ if (err)
+ nla_nest_cancel(skb, start);
+ else
+ nla_nest_end(skb, start);
+
+ return err;
+}
+
static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
{
const struct nlattr *ovs_key = nla_data(a);
@@ -3204,6 +3271,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
return err;
break;
+ case OVS_ACTION_ATTR_CLONE:
+ err = clone_action_to_attr(a, skb);
+ if (err)
+ return err;
+ break;
+
default:
if (nla_put(skb, type, nla_len(a), nla_data(a)))
return -EMSGSIZE;
--
2.7.4
^ permalink raw reply related
* Regression introduced by "r8169: simplify rtl_set_mac_address"
From: Corinna Vinschen @ 2018-07-02 19:48 UTC (permalink / raw)
To: netdev, Heiner Kallweit
Hi,
the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address",
introduced a regression found by trying to team a r8169 NIC.
Try the following (assuming the r8169 NIC is eth0):
$ nmcli con add type team con-name team0 ifname nm-team config \
'{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \
ipv4.method disable ipv6.method ignore
$ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team
$ nmcli con up team0.0
$ teamdctl nm-team port present eth0
command call failed (No such device)
Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify
rtl_set_mac_address" as the culprit. Reverting this patch fixes
the issue and the teamdctl call succeeds.
The reason is apparently the usage of eth_mac_addr here. eth_mac_addr
calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE.
Debugging shows this flag not being set on r8169, thus
eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims
"No such device", rather than "Device or resource busy", but that's not
the point here).
Note that other devices like igb, don't call eth_mac_addr either, but
rather call memcpy by themselves to copy the new MAC, just as the
original r8169 code did, too. Consequentially this problem is not
present on igb.
I suggest to revert this change in the first place, but I wonder if
we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers.
Thanks,
Corinna
^ permalink raw reply
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Michael Scott @ 2018-07-02 19:45 UTC (permalink / raw)
To: Alexander Aring
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <20180702185438.dqqjg6k45iefj5is@x220t>
Hello Alexander,
On 07/02/2018 11:54 AM, Alexander Aring wrote:
> Hi,
>
> On Tue, Jun 19, 2018 at 04:44:06PM -0700, Michael Scott wrote:
>> After decompression of 6lowpan socket data, an IPv6 header is inserted
>> before the existing socket payload. After this, we reset the
>> network_header value of the skb to account for the difference in payload
>> size from prior to decompression + the addition of the IPv6 header.
>>
>> However, we fail to reset the mac_header value.
>>
>> Leaving the mac_header value untouched here, can cause a calculation
>> error in net/packet/af_packet.c packet_rcv() function when an
>> AF_PACKET socket is opened in SOCK_RAW mode for use on a 6lowpan
>> interface.
>>
>> On line 2088, the data pointer is moved backward by the value returned
>> from skb_mac_header(). If skb->data is adjusted so that it is before
>> the skb->head pointer (which can happen when an old value of mac_header
>> is left in place) the kernel generates a panic in net/core/skbuff.c
>> line 1717.
>>
>> This panic can be generated by BLE 6lowpan interfaces (such as bt0) and
>> 802.15.4 interfaces (such as lowpan0) as they both use the same 6lowpan
>> sources for compression and decompression.
>>
>> Signed-off-by: Michael Scott <michael@opensourcefoundries.com>
>> ---
>> net/6lowpan/iphc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index 6b1042e21656..52fad5dad9f7 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>> hdr.hop_limit, &hdr.daddr);
>>
>> skb_push(skb, sizeof(hdr));
>> + skb_reset_mac_header(skb);
>> skb_reset_network_header(skb);
>> skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>>
> I think it's good to make that if the mac_header gets a dangled pointer.
> But we don't have a mac header at this point anymore...
>
> There exists also some functionality that the MAC header is not set, I
> suppose this can be usefuly for tun like interfaces e.g. RAW IP what we
> have here.
>
> skb_mac_header_was_set
>
> which does:
>
> return skb->mac_header != (typeof(skb->mac_header))~0U;
>
> maybe we can set it as (typeof(skb->mac_header))~0U and then everything
> will run as far the kernel will not crash anymore.
>
> Question is for me: which upper layer wants access MAC header here on
> receive path?
> It cannot parsed anyhow because so far I know no upper layer can parse
> at the moment 802.15.4 frames (which is a complex format). Maybe over
> some header_ops callback?
I was testing a C program which performs NAT64 handling on packets
destined to a certain IPv6 subnet (64:ff9b::). To do this, the
application opens a RAW socket like this: sniff_sock = socket(PF_PACKET,
SOCK_RAW, htons(ETH_P_ALL)); It then sets promiscuous mode and enters a
looping call of:
length = recv(sniff_sock, buffer, PACKET_BUFFER, MSG_TRUNC); My host PC
kernel would then promptly crash on me. (I'm going to purposely avoid
the obvious point of: this probably isn't the best way to parse packets
for NAT64 translation as you will get every single packet incoming or
outgoing on the host.) Turns out, testing the program on an 802.15.4
6lowpan interface exposed some of the issues which this mailing list
(but not myself) is well aware of (no L2 data in the RAW packets) and
also led me to debugging this patch to stop the kernel crash. TL;DR: To
summarize, any PF_PACKET SOCK_RAW socket which recv()'s IPv6 data from a
6lowpan node will cause this kernel crash eventually (checked on kernel
4.15, 4.16, 4.17 and 4.18-rc1). - Mike
>
> - Alex
^ permalink raw reply
* [PATCH net-next 10/10] r8169: don't read chip phy status register
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Instead of accessing the PHYstatus register we can use the information
phylib stores in the phy_device structure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 48c0e77c..7b7de596 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1428,18 +1428,19 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
+ struct phy_device *phydev = dev->phydev;
if (!netif_running(dev))
return;
if (tp->mac_version == RTL_GIGA_MAC_VER_34 ||
tp->mac_version == RTL_GIGA_MAC_VER_38) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
ERIAR_EXGMAC);
- } else if (RTL_R8(tp, PHYstatus) & _100bps) {
+ } else if (phydev->speed == SPEED_100) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x0000001f,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1457,7 +1458,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
} else if (tp->mac_version == RTL_GIGA_MAC_VER_35 ||
tp->mac_version == RTL_GIGA_MAC_VER_36) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1469,7 +1470,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
}
} else if (tp->mac_version == RTL_GIGA_MAC_VER_37) {
- if (RTL_R8(tp, PHYstatus) & _10bps) {
+ if (phydev->speed == SPEED_10) {
rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x4d02,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_0011, 0x0060,
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
The only remaining usage of the struct mii_if_info member is to store the
information whether the chip is GMII-capable. So we can replace it with
a simple flag.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 38 +++++-----------------------
1 file changed, 7 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index b696b83d..48c0e77c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -15,7 +15,6 @@
#include <linux/etherdevice.h>
#include <linux/delay.h>
#include <linux/ethtool.h>
-#include <linux/mii.h>
#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
@@ -754,7 +753,7 @@ struct rtl8169_private {
struct work_struct work;
} wk;
- struct mii_if_info mii;
+ unsigned supports_gmii:1;
struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
@@ -1106,21 +1105,6 @@ static void rtl_w0w1_phy(struct rtl8169_private *tp, int reg_addr, int p, int m)
rtl_writephy(tp, reg_addr, (val & ~m) | p);
}
-static void rtl_mdio_write(struct net_device *dev, int phy_id, int location,
- int val)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- rtl_writephy(tp, location, val);
-}
-
-static int rtl_mdio_read(struct net_device *dev, int phy_id, int location)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return rtl_readphy(tp, location);
-}
-
DECLARE_RTL_COND(rtl_ephyar_cond)
{
return RTL_R32(tp, EPHYAR) & EPHYAR_FLAG;
@@ -2253,15 +2237,15 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp,
"unknown MAC, using family default\n");
tp->mac_version = default_version;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_42 :
RTL_GIGA_MAC_VER_43;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_45 :
RTL_GIGA_MAC_VER_47;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_46 :
RTL_GIGA_MAC_VER_48;
}
@@ -6714,14 +6698,14 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
phy_interface_t phy_mode;
int ret;
- phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
PHY_INTERFACE_MODE_MII;
phydev = mdiobus_get_phy(tp->mii_bus, 0);
if (!phydev)
return -ENODEV;
- if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
phy_set_max_speed(phydev, SPEED_100);
}
@@ -7317,7 +7301,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
struct rtl8169_private *tp;
- struct mii_if_info *mii;
struct net_device *dev;
int chipset, region, i;
int rc;
@@ -7337,14 +7320,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->dev = dev;
tp->pci_dev = pdev;
tp->msg_enable = netif_msg_init(debug.msg_enable, R8169_MSG_DEFAULT);
-
- mii = &tp->mii;
- mii->dev = dev;
- mii->mdio_read = rtl_mdio_read;
- mii->mdio_write = rtl_mdio_write;
- mii->phy_id_mask = 0x1f;
- mii->reg_num_mask = 0x1f;
- mii->supports_gmii = cfg->has_gmii;
+ tp->supports_gmii = cfg->has_gmii;
/* enable device (incl. PCI PM wakeup and hotplug setup) */
rc = pcim_enable_device(pdev);
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
We can remove rtl8169_set_speed_xmii() now that phylib handles all this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 90 ----------------------------
1 file changed, 90 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 807fbc75..b696b83d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1670,89 +1670,6 @@ static int rtl8169_get_regs_len(struct net_device *dev)
return R8169_REGS_SIZE;
}
-static int rtl8169_set_speed_xmii(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 adv)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int giga_ctrl, bmcr;
- int rc = -EINVAL;
-
- rtl_writephy(tp, 0x1f, 0x0000);
-
- if (autoneg == AUTONEG_ENABLE) {
- int auto_nego;
-
- auto_nego = rtl_readphy(tp, MII_ADVERTISE);
- auto_nego &= ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
- ADVERTISE_100HALF | ADVERTISE_100FULL);
-
- if (adv & ADVERTISED_10baseT_Half)
- auto_nego |= ADVERTISE_10HALF;
- if (adv & ADVERTISED_10baseT_Full)
- auto_nego |= ADVERTISE_10FULL;
- if (adv & ADVERTISED_100baseT_Half)
- auto_nego |= ADVERTISE_100HALF;
- if (adv & ADVERTISED_100baseT_Full)
- auto_nego |= ADVERTISE_100FULL;
-
- auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
-
- giga_ctrl = rtl_readphy(tp, MII_CTRL1000);
- giga_ctrl &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
-
- /* The 8100e/8101e/8102e do Fast Ethernet only. */
- if (tp->mii.supports_gmii) {
- if (adv & ADVERTISED_1000baseT_Half)
- giga_ctrl |= ADVERTISE_1000HALF;
- if (adv & ADVERTISED_1000baseT_Full)
- giga_ctrl |= ADVERTISE_1000FULL;
- } else if (adv & (ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full)) {
- netif_info(tp, link, dev,
- "PHY does not support 1000Mbps\n");
- goto out;
- }
-
- bmcr = BMCR_ANENABLE | BMCR_ANRESTART;
-
- rtl_writephy(tp, MII_ADVERTISE, auto_nego);
- rtl_writephy(tp, MII_CTRL1000, giga_ctrl);
- } else {
- if (speed == SPEED_10)
- bmcr = 0;
- else if (speed == SPEED_100)
- bmcr = BMCR_SPEED100;
- else
- goto out;
-
- if (duplex == DUPLEX_FULL)
- bmcr |= BMCR_FULLDPLX;
- }
-
- rtl_writephy(tp, MII_BMCR, bmcr);
-
- if (tp->mac_version == RTL_GIGA_MAC_VER_02 ||
- tp->mac_version == RTL_GIGA_MAC_VER_03) {
- if ((speed == SPEED_100) && (autoneg != AUTONEG_ENABLE)) {
- rtl_writephy(tp, 0x17, 0x2138);
- rtl_writephy(tp, 0x0e, 0x0260);
- } else {
- rtl_writephy(tp, 0x17, 0x2108);
- rtl_writephy(tp, 0x0e, 0x0000);
- }
- }
-
- rc = 0;
-out:
- return rc;
-}
-
-static int rtl8169_set_speed(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 advertising)
-{
- return rtl8169_set_speed_xmii(dev, autoneg, speed, duplex, advertising);
-}
-
static netdev_features_t rtl8169_fix_features(struct net_device *dev,
netdev_features_t features)
{
@@ -4245,13 +4162,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
genphy_config_aneg(dev->phydev);
genphy_soft_reset(dev->phydev);
-
- rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0));
}
static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Change rtl_speed_down() to use phylib.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 311321ee..807fbc75 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
+ /* We may have called rtl_speed_down before */
+ dev->phydev->advertising = dev->phydev->supported;
+ genphy_config_aneg(dev->phydev);
+
genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
@@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
}
}
+#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
+#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
+#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
+
static void rtl_speed_down(struct rtl8169_private *tp)
{
- u32 adv;
- int lpa;
+ struct phy_device *phydev = tp->dev->phydev;
+ u32 adv = phydev->lp_advertising & phydev->supported;
- rtl_writephy(tp, 0x1f, 0x0000);
- lpa = rtl_readphy(tp, MII_LPA);
+ if (adv & BASET10)
+ phydev->advertising &= ~(BASET100 | BASET1000);
+ else if (adv & BASET100)
+ phydev->advertising &= ~BASET1000;
- if (lpa & (LPA_10HALF | LPA_10FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
- else if (lpa & (LPA_100HALF | LPA_100FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
- else
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0);
-
- rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- adv);
+ genphy_config_aneg(phydev);
}
static void rtl_wol_suspend_quirk(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
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