* Re: [PATCH net-next] qed: Remove unused including <linux/version.h>
From: David Miller @ 2017-05-18 19:19 UTC (permalink / raw)
To: weiyj.lk; +Cc: Yuval.Mintz, Ariel.Elior, weiyongjun1, everest-linux-l2, netdev
In-Reply-To: <20170518152629.12503-1-weiyj.lk@gmail.com>
From: Wei Yongjun <weiyj.lk@gmail.com>
Date: Thu, 18 May 2017 15:26:29 +0000
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Remove including <linux/version.h> that is not needed.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net/mlx5e: Fix possible memory leak
From: David Miller @ 2017-05-18 19:19 UTC (permalink / raw)
To: weiyj.lk-Re5JQEeQqe8AvxtiuMwx3w
Cc: saeedm-VPRAkNaXOzVWk0Htik3J/w, matanb-VPRAkNaXOzVWk0Htik3J/w,
leonro-VPRAkNaXOzVWk0Htik3J/w, hadarh-VPRAkNaXOzVWk0Htik3J/w,
weiyongjun1-hv44wF8Li93QT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170518153441.24398-1-weiyj.lk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Wei Yongjun <weiyj.lk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Thu, 18 May 2017 15:34:41 +0000
> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> 'encap_header' is malloced and should be freed before leaving from
> the error handling cases, otherwise it will cause memory leak.
>
> Fixes: 232c001398ae ("net/mlx5e: Add support to neighbour update flow")
> Signed-off-by: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
From: Andrew Lunn @ 2017-05-18 19:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Florian Fainelli, Geert Uytterhoeven, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdWYeSzofU57LmT_M3KkcHhvjfJy7nkHvcXkKzOYmvhqqA@mail.gmail.com>
> > This most certainly works fine in the simple case where you have one PHY
> > hanging off the MDIO bus, now what happens if you have several?
> >
> > Presumably, the first PHY that returns EPROBE_DEFER will make the entire
> > bus registration return EPROB_DEFER as well, and so on, and so forth,
> > but I am not sure if we will be properly unwinding the successful
> > registration of PHYs that either don't have an interrupt, or did not
> > return EPROBE_DEFER.
> >
> > It should be possible to mimic this behavior by using the fixed PHY, and
> > possibly the dsa_loop.c driver which would create 4 ports, expecting 4
> > fixed PHYs to be present.
>
> mdiobus_unregister(), called from of_mdiobus_register() on failure,
> should do the unwinding, right?
>
> And when the driver is reprobed, all PHYs are reprobed, until they all
> succeed.
That is the theory. I looked at that while reviewing the patch. But
this has probably not been tested in anger. It would be good to test
this properly, with not just the first PHY returning -EPROBE_DEFER, to
really test the unwind.
Andrew
^ permalink raw reply
* Re: [PATCH v5 net-next 5/7] net: fix documentation of struct scm_timestamping
From: Willem de Bruijn @ 2017-05-18 19:38 UTC (permalink / raw)
To: Miroslav Lichvar; +Cc: Network Development, Richard Cochran, Willem de Bruijn
In-Reply-To: <20170518140738.19617-6-mlichvar@redhat.com>
On Thu, May 18, 2017 at 10:07 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> The scm_timestamping struct may return multiple non-zero fields, e.g.
> when both software and hardware RX timestamping is enabled, or when the
> SO_TIMESTAMP(NS) option is combined with SCM_TIMESTAMPING and a false
> software timestamp is generated in the recvmsg() call in order to always
> return a SCM_TIMESTAMP(NS) message.
>
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Thanks for adding this!
> +Note that if the SO_TIMESTAMP or SO_TIMESTAMPNS option is enabled
> +together with SO_TIMESTAMPING using SOF_TIMESTAMPING_SOFTWARE, a false
> +software timestamp will be generated in the recvmsg() call and passed
> +in ts[0] when a real software timestamp is missing.
With receive software timestamping this is expected behavior? I would make
explicit that this happens even on tx timestamps.
> For this reason it
> +is not recommended to combine SO_TIMESTAMP(NS) with SO_TIMESTAMPING.
And I'd remove this. The extra timestamp is harmless, and we may be missing
other reasons why someone would want to enable both on the same socket.
^ permalink raw reply
* [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP
From: Ihar Hrachyshka @ 2017-05-18 19:41 UTC (permalink / raw)
To: davem, ja; +Cc: Ihar Hrachyshka, netdev
This patchset is spurred by discussion started at
https://patchwork.ozlabs.org/patch/760372/ where we figured that there is no
real reason for enforcing override by gratuitous ARP packets only when
arp_accept is 1. Same should happen when it's 0 (the default value).
changelog v2: handled review comments by Julian Anastasov
- fixed a mistake in a comment;
- postponed addr_type calculation to as late as possible.
Ihar Hrachyshka (4):
arp: fixed error in a comment
arp: decompose is_garp logic into a separate function
arp: postpone addr_type calculation to as late as possible
arp: always override existing neigh entries with gratuitous ARP
net/ipv4/arp.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 17 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v2 1/4] arp: fixed error in a comment
From: Ihar Hrachyshka @ 2017-05-18 19:41 UTC (permalink / raw)
To: davem, ja; +Cc: Ihar Hrachyshka, netdev
In-Reply-To: <cover.1495136258.git.ihrachys@redhat.com>
the is_garp code deals just with gratuitous ARP packets, not every
unsolicited packet.
This patch is a result of a discussion in netdev:
http://marc.info/?l=linux-netdev&m=149506354216994
Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
net/ipv4/arp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index d54345a..053492a 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -846,7 +846,7 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
*/
is_garp = tip == sip && addr_type == RTN_UNICAST;
- /* Unsolicited ARP _replies_ also require target hwaddr to be
+ /* Gratuitous ARP _replies_ also require target hwaddr to be
* the same as source.
*/
if (is_garp && arp->ar_op == htons(ARPOP_REPLY))
--
2.9.3
^ permalink raw reply related
* [PATCH v2 2/4] arp: decompose is_garp logic into a separate function
From: Ihar Hrachyshka @ 2017-05-18 19:41 UTC (permalink / raw)
To: davem, ja; +Cc: Ihar Hrachyshka, netdev
In-Reply-To: <cover.1495136258.git.ihrachys@redhat.com>
The code is quite involving already to earn a separate function for
itself. If anything, it helps arp_process readability.
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
net/ipv4/arp.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 053492a..ca6e1e6 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -641,6 +641,27 @@ void arp_xmit(struct sk_buff *skb)
}
EXPORT_SYMBOL(arp_xmit);
+static bool arp_is_garp(struct net_device *dev, int addr_type,
+ __be16 ar_op,
+ __be32 sip, __be32 tip,
+ unsigned char *sha, unsigned char *tha)
+{
+ bool is_garp = tip == sip && addr_type == RTN_UNICAST;
+
+ /* Gratuitous ARP _replies_ also require target hwaddr to be
+ * the same as source.
+ */
+ if (is_garp && ar_op == htons(ARPOP_REPLY))
+ is_garp =
+ /* IPv4 over IEEE 1394 doesn't provide target
+ * hardware address field in its ARP payload.
+ */
+ tha &&
+ !memcmp(tha, sha, dev->addr_len);
+
+ return is_garp;
+}
+
/*
* Process an arp request.
*/
@@ -844,18 +865,8 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
It is possible, that this option should be enabled for some
devices (strip is candidate)
*/
- is_garp = tip == sip && addr_type == RTN_UNICAST;
-
- /* Gratuitous ARP _replies_ also require target hwaddr to be
- * the same as source.
- */
- if (is_garp && arp->ar_op == htons(ARPOP_REPLY))
- is_garp =
- /* IPv4 over IEEE 1394 doesn't provide target
- * hardware address field in its ARP payload.
- */
- tha &&
- !memcmp(tha, sha, dev->addr_len);
+ is_garp = arp_is_garp(dev, addr_type, arp->ar_op,
+ sip, tip, sha, tha);
if (!n &&
((arp->ar_op == htons(ARPOP_REPLY) &&
--
2.9.3
^ permalink raw reply related
* [PATCH v2 3/4] arp: postpone addr_type calculation to as late as possible
From: Ihar Hrachyshka @ 2017-05-18 19:41 UTC (permalink / raw)
To: davem, ja; +Cc: Ihar Hrachyshka, netdev
In-Reply-To: <cover.1495136258.git.ihrachys@redhat.com>
The addr_type retrieval can be costly, so it's worth trying to avoid its
calculation as much as possible. This patch makes it calculated only
for gratuitous ARP packets. This is especially important since later we
may want to move is_garp calculation outside of arp_accept block, at
which point the costly operation will be executed for all setups.
The patch is the result of a discussion in net-dev:
http://marc.info/?l=linux-netdev&m=149506354216994
Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
net/ipv4/arp.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ca6e1e6..c22103c 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -641,12 +641,12 @@ void arp_xmit(struct sk_buff *skb)
}
EXPORT_SYMBOL(arp_xmit);
-static bool arp_is_garp(struct net_device *dev, int addr_type,
- __be16 ar_op,
+static bool arp_is_garp(struct net *net, struct net_device *dev,
+ int *addr_type, __be16 ar_op,
__be32 sip, __be32 tip,
unsigned char *sha, unsigned char *tha)
{
- bool is_garp = tip == sip && addr_type == RTN_UNICAST;
+ bool is_garp = tip == sip;
/* Gratuitous ARP _replies_ also require target hwaddr to be
* the same as source.
@@ -659,6 +659,11 @@ static bool arp_is_garp(struct net_device *dev, int addr_type,
tha &&
!memcmp(tha, sha, dev->addr_len);
+ if (is_garp) {
+ *addr_type = inet_addr_type_dev_table(net, dev, sip);
+ if (*addr_type != RTN_UNICAST)
+ is_garp = false;
+ }
return is_garp;
}
@@ -859,18 +864,23 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
n = __neigh_lookup(&arp_tbl, &sip, dev, 0);
if (IN_DEV_ARP_ACCEPT(in_dev)) {
- unsigned int addr_type = inet_addr_type_dev_table(net, dev, sip);
+ addr_type = -1;
/* Unsolicited ARP is not accepted by default.
It is possible, that this option should be enabled for some
devices (strip is candidate)
*/
- is_garp = arp_is_garp(dev, addr_type, arp->ar_op,
+ is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
sip, tip, sha, tha);
if (!n &&
- ((arp->ar_op == htons(ARPOP_REPLY) &&
- addr_type == RTN_UNICAST) || is_garp))
+ (is_garp ||
+ (arp->ar_op == htons(ARPOP_REPLY) &&
+ (addr_type == RTN_UNICAST ||
+ (addr_type < 0 &&
+ /* postpone calculation to as late as possible */
+ inet_addr_type_dev_table(net, dev, sip) ==
+ RTN_UNICAST)))))
n = __neigh_lookup(&arp_tbl, &sip, dev, 1);
}
--
2.9.3
^ permalink raw reply related
* [PATCH v2 4/4] arp: always override existing neigh entries with gratuitous ARP
From: Ihar Hrachyshka @ 2017-05-18 19:41 UTC (permalink / raw)
To: davem, ja; +Cc: Ihar Hrachyshka, netdev
In-Reply-To: <cover.1495136258.git.ihrachys@redhat.com>
Currently, when arp_accept is 1, we always override existing neigh
entries with incoming gratuitous ARP replies. Otherwise, we override
them only if new replies satisfy _locktime_ conditional (packets arrive
not earlier than _locktime_ seconds since the last update to the neigh
entry).
The idea behind locktime is to pick the very first (=> close) reply
received in a unicast burst when ARP proxies are used. This helps to
avoid ARP thrashing where Linux would switch back and forth from one
proxy to another.
This logic has nothing to do with gratuitous ARP replies that are
generally not aligned in time when multiple IP address carriers send
them into network.
This patch enforces overriding of existing neigh entries by all incoming
gratuitous ARP packets, irrespective of their time of arrival. This will
make the kernel honour all incoming gratuitous ARP packets.
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
net/ipv4/arp.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index c22103c..ae96e6f 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -863,16 +863,17 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
n = __neigh_lookup(&arp_tbl, &sip, dev, 0);
- if (IN_DEV_ARP_ACCEPT(in_dev)) {
+ if (n || IN_DEV_ARP_ACCEPT(in_dev)) {
addr_type = -1;
+ is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
+ sip, tip, sha, tha);
+ }
+ if (IN_DEV_ARP_ACCEPT(in_dev)) {
/* Unsolicited ARP is not accepted by default.
It is possible, that this option should be enabled for some
devices (strip is candidate)
*/
- is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
- sip, tip, sha, tha);
-
if (!n &&
(is_garp ||
(arp->ar_op == htons(ARPOP_REPLY) &&
--
2.9.3
^ permalink raw reply related
* [PATCH net-next] geneve: always fill CSUM6_RX configuration
From: Eric Garver @ 2017-05-18 19:59 UTC (permalink / raw)
To: netdev; +Cc: pravin shelar
CSMU6_RX is relevant for collect_metadata as well. As such leave it
outside of the dev's IPv4/IPv6 checks.
Fixes: 9b4437a5b870 ("geneve: Unify LWT and netdev handling.")
Signed-off-by: Eric Garver <e@erig.me>
---
drivers/net/geneve.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index dec5d563ab19..f557d1dc3f9b 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1311,13 +1311,13 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_TX,
!(info->key.tun_flags & TUNNEL_CSUM)))
goto nla_put_failure;
-
- if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX,
- !geneve->use_udp6_rx_checksums))
- goto nla_put_failure;
#endif
}
+ if (nla_put_u8(skb, IFLA_GENEVE_UDP_ZERO_CSUM6_RX,
+ !geneve->use_udp6_rx_checksums))
+ goto nla_put_failure;
+
if (nla_put_u8(skb, IFLA_GENEVE_TTL, info->key.ttl) ||
nla_put_u8(skb, IFLA_GENEVE_TOS, info->key.tos) ||
nla_put_be32(skb, IFLA_GENEVE_LABEL, info->key.label))
--
2.12.0
^ permalink raw reply related
* Re: [PATCH v1] samples/bpf: Add a .gitignore for binaries
From: David Ahern @ 2017-05-18 20:03 UTC (permalink / raw)
To: Alexander Alemayhu
Cc: Mickaël Salaün, Alexei Starovoitov, Daniel Borkmann,
linux-kernel, Arnaldo Carvalho de Melo, Wang Nan, netdev
In-Reply-To: <20170517081844.GA4447@gmail.com>
On 5/17/17 1:18 AM, Alexander Alemayhu wrote:
> I have looked into this but found it to be not easy and all attempts to
> change the Makefile has resulted in obscure errors :/
>
> Getting clang to output in a different directory was easy[0], but I guess
> this is not the right approach either. Have you tried making the change?
spent an hour so a few weeks back. It is not trivial, but someone needs
to find to fix it now.
perf is the example to use: you can build it from both top level kernel
directory (e.g, make -C tools/perf O=/tmp/perf) and the perf directory
(cd tools/perf; make O=/tmp/perf). Both are wanted for samples/bpf and
it would be nice to keep the O= option as well.
I don't have the time for the next few weeks. Perhaps mid-June I can
take a look.
^ permalink raw reply
* Re: [PATCH v4 net-next 6/7] net: allow simultaneous SW and HW transmit timestamping
From: Willem de Bruijn @ 2017-05-18 20:16 UTC (permalink / raw)
To: Miroslav Lichvar; +Cc: Network Development, Richard Cochran, Willem de Bruijn
In-Reply-To: <20170518130656.24163-7-mlichvar@redhat.com>
On Thu, May 18, 2017 at 9:06 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> Add SOF_TIMESTAMPING_OPT_TX_SWHW option to allow an outgoing packet to
> be looped to the socket's error queue with a software timestamp even
> when a hardware transmit timestamp is expected to be provided by the
> driver.
>
> Applications using this option will receive two separate messages from
> the error queue, one with a software timestamp and the other with a
> hardware timestamp. As the hardware timestamp is saved to the shared skb
> info, which may happen before the first message with software timestamp
> is received by the application, the hardware timestamp is copied to the
> SCM_TIMESTAMPING control message only when the skb has no software
> timestamp or it is an incoming packet.
>
> While changing sw_tx_timestamp(), inline it in skb_tx_timestamp() as
> there are no other users.
>
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
> +/* On transmit, software and hardware timestamps are returned independently.
> + * As the two skb clones share the hardware timestamp, which may be updated
> + * before the software timestamp is received, a hardware TX timestamp may be
> + * returned only if there is no software TX timestamp. A false software
> + * timestamp made for SOCK_RCVTSTAMP when a real timestamp is missing must
> + * be ignored.
Please expand on why this case can be ignored. It is quite subtle. How about
something like
*
* A false software timestamp is one made inside the __sock_recv_timestamp
* call itself. These are generated whenever SO_TIMESTAMP(NS) is enabled
* on the socket, even when the timestamp reported is for another option, such
* as hardware tx timestamp.
*
* Ignore these when deciding whether a timestamp source is hw or sw.
*/
And perhaps move the comment to the branch itself.
> + */
> +static bool skb_is_swtx_tstamp(const struct sk_buff *skb,
> + const struct sock *sk, int false_tstamp)
> +{
> + if (false_tstamp && sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
Also, why is it ignored only for the new mode?
> + return 0;
> +
> + return skb->tstamp && skb_is_err_queue(skb);
> +}
^ permalink raw reply
* Re: [PATCH v5 net-next 4/7] net: add new control message for incoming HW-timestamped packets
From: Willem de Bruijn @ 2017-05-18 20:20 UTC (permalink / raw)
To: Miroslav Lichvar; +Cc: Network Development, Richard Cochran, Willem de Bruijn
In-Reply-To: <20170518140738.19617-5-mlichvar@redhat.com>
On Thu, May 18, 2017 at 10:07 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> Add SOF_TIMESTAMPING_OPT_PKTINFO option to request a new control message
> for incoming packets with hardware timestamps. It contains the index of
> the real interface which received the packet and the length of the
> packet at layer 2.
>
> The index is useful with bonding, bridges and other interfaces, where
> IP_PKTINFO doesn't allow applications to determine which PHC made the
> timestamp. With the L2 length (and link speed) it is possible to
> transpose preamble timestamps to trailer timestamps, which are used in
> the NTP protocol.
>
> While this information could be provided by two new socket options
> independently from timestamping, it doesn't look like they would be very
> useful. With this option any performance impact is limited to hardware
> timestamping.
>
> Use dev_get_by_napi_id() to get the device and its index. On kernels
> with disabled CONFIG_NET_RX_BUSY_POLL or drivers not using NAPI, a zero
> index will be returned in the control message.
>
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Acked-by: Willem de Bruijn <willemb@google.com>
> +SOF_TIMESTAMPING_OPT_PKTINFO:
> +
> + Enable the SCM_TIMESTAMPING_PKTINFO control message for incoming
> + packets with hardware timestamps. The message contains struct
> + scm_ts_pktinfo, which supplies the index of the real interface which
> + received the packet and its length at layer 2. A valid (non-zero)
> + interface index will be returned only if CONFIG_NET_RX_BUSY_POLL is
> + enabled and the driver is using NAPI.
It is probably good to explicitly call out that the remaining two fields
are reserved and undefined. To stress that applications cannot be
overly pedantic and start failing if these become non-zero.
^ permalink raw reply
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
From: Geert Uytterhoeven @ 2017-05-18 20:36 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Geert Uytterhoeven, Rob Herring, Frank Rowand,
Thomas Petazzoni, Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
In-Reply-To: <20170518193453.GB13759@lunn.ch>
Hi Andrew,
On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > This most certainly works fine in the simple case where you have one PHY
>> > hanging off the MDIO bus, now what happens if you have several?
>> >
>> > Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>> > bus registration return EPROB_DEFER as well, and so on, and so forth,
>> > but I am not sure if we will be properly unwinding the successful
>> > registration of PHYs that either don't have an interrupt, or did not
>> > return EPROBE_DEFER.
>> >
>> > It should be possible to mimic this behavior by using the fixed PHY, and
>> > possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>> > fixed PHYs to be present.
>>
>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>> should do the unwinding, right?
>>
>> And when the driver is reprobed, all PHYs are reprobed, until they all
>> succeed.
>
> That is the theory. I looked at that while reviewing the patch. But
> this has probably not been tested in anger. It would be good to test
> this properly, with not just the first PHY returning -EPROBE_DEFER, to
> really test the unwind.
Unfortunately I don't have a board with multiple PHYs, so I cannot test
that case.
Does unbinding/rebinding a network driver with multiple PHYs currently
work? Or module unload/reload?
That should exercise a similar code path.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 2/4] arp: decompose is_garp logic into a separate function
From: Julian Anastasov @ 2017-05-18 20:49 UTC (permalink / raw)
To: Ihar Hrachyshka; +Cc: davem, netdev
In-Reply-To: <481737abe7a375a7efe125f4e76a998dd670a2df.1495136258.git.ihrachys@redhat.com>
Hello,
On Thu, 18 May 2017, Ihar Hrachyshka wrote:
> The code is quite involving already to earn a separate function for
> itself. If anything, it helps arp_process readability.
>
> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> ---
> net/ipv4/arp.c | 35 +++++++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 053492a..ca6e1e6 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -641,6 +641,27 @@ void arp_xmit(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(arp_xmit);
>
> +static bool arp_is_garp(struct net_device *dev, int addr_type,
> + __be16 ar_op,
> + __be32 sip, __be32 tip,
> + unsigned char *sha, unsigned char *tha)
> +{
> + bool is_garp = tip == sip && addr_type == RTN_UNICAST;
> +
> + /* Gratuitous ARP _replies_ also require target hwaddr to be
> + * the same as source.
> + */
> + if (is_garp && ar_op == htons(ARPOP_REPLY))
> + is_garp =
> + /* IPv4 over IEEE 1394 doesn't provide target
> + * hardware address field in its ARP payload.
> + */
> + tha &&
All 4 patches look ok to me with only a small problem
which comes from patch already included in kernel. I see
that GARP replies can not work for 1394, is_garp will be
cleared. May be 'tha' check should be moved in if expression,
for example:
if (is_garp && ar_op == htons(ARPOP_REPLY) && tha)
is_garp = !memcmp(tha, sha, dev->addr_len);
> + !memcmp(tha, sha, dev->addr_len);
> +
> + return is_garp;
> +}
Regards
^ permalink raw reply
* [PATCH v2 net-next 0/2] Check all RGMII phy mode variants
From: Iyappan Subramanian @ 2017-05-18 22:13 UTC (permalink / raw)
To: davem, netdev
Cc: f.fainelli, andrew, linux-arm-kernel, patches,
Iyappan Subramanian
This patch set,
- adds phy_interface_mode_is_rgmii() helper function
- addresses review comment from previous patch set, by calling
phy_interface_mode_is_rgmii() to address all RGMII variants
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
---
v2: Address review comments from v1
- adds phy_interface_mode_is_rgmii() helper function
- addresses review comment from previous patch set, by calling
phy_interface_mode_is_rgmii() to address all RGMII variants
v1:
- Initial version
---
Iyappan Subramanian (2):
include: linux: Add helper function to check phy interface mode
drivers: net: xgene: Check all RGMII phy mode variants
drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c | 6 +++---
drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 12 ++++++------
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 15 +++++++++------
include/linux/phy.h | 14 ++++++++++++--
4 files changed, 30 insertions(+), 17 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v2 net-next 1/2] include: linux: Add helper function to check phy interface mode
From: Iyappan Subramanian @ 2017-05-18 22:13 UTC (permalink / raw)
To: davem, netdev
Cc: f.fainelli, andrew, linux-arm-kernel, patches,
Iyappan Subramanian
In-Reply-To: <1495145624-29463-1-git-send-email-isubramanian@apm.com>
Added helper function that checks phy_mode is RGMII (all variants)
'bool phy_interface_mode_is_rgmii(phy_interface_t mode)'
Changed the following function, to use the above.
'bool phy_interface_is_rgmii(struct phy_device *phydev)'
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
Suggested-by: Andrew Lunn <andrew@lunn.ch>
---
include/linux/phy.h | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 54ef458..5a808a2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -716,14 +716,24 @@ static inline bool phy_is_internal(struct phy_device *phydev)
}
/**
+ * phy_interface_mode_is_rgmii - Convenience function for testing if a
+ * PHY interface mode is RGMII (all variants)
+ * @mode: the phy_interface_t enum
+ */
+static inline bool phy_interface_mode_is_rgmii(phy_interface_t mode)
+{
+ return mode >= PHY_INTERFACE_MODE_RGMII &&
+ mode <= PHY_INTERFACE_MODE_RGMII_TXID;
+};
+
+/**
* phy_interface_is_rgmii - Convenience function for testing if a PHY interface
* is RGMII (all variants)
* @phydev: the phy_device struct
*/
static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
{
- return phydev->interface >= PHY_INTERFACE_MODE_RGMII &&
- phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
+ return phy_interface_mode_is_rgmii(phydev->interface);
};
/*
--
1.9.1
^ permalink raw reply related
* [PATCH v2 net-next 2/2] drivers: net: xgene: Check all RGMII phy mode variants
From: Iyappan Subramanian @ 2017-05-18 22:13 UTC (permalink / raw)
To: davem, netdev
Cc: f.fainelli, andrew, linux-arm-kernel, patches,
Iyappan Subramanian, Quan Nguyen
In-Reply-To: <1495145624-29463-1-git-send-email-isubramanian@apm.com>
This patch addresses the review comment from the previous patch set,
by using phy_interface_mode_is_rgmii() helper function to address
all RGMII phy mode variants.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
Review comment reference:
http://www.spinics.net/lists/netdev/msg434649.html
---
drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c | 6 +++---
drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 12 ++++++------
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 15 +++++++++------
3 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c b/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
index 0fdec78..559963b 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_ethtool.c
@@ -127,7 +127,7 @@ static int xgene_get_link_ksettings(struct net_device *ndev,
struct phy_device *phydev = ndev->phydev;
u32 supported;
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode)) {
if (phydev == NULL)
return -ENODEV;
@@ -177,7 +177,7 @@ static int xgene_set_link_ksettings(struct net_device *ndev,
struct xgene_enet_pdata *pdata = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode)) {
if (!phydev)
return -ENODEV;
@@ -304,7 +304,7 @@ static int xgene_set_pauseparam(struct net_device *ndev,
struct phy_device *phydev = ndev->phydev;
u32 oldadv, newadv;
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII ||
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode) ||
pdata->phy_mode == PHY_INTERFACE_MODE_SGMII) {
if (!phydev)
return -EINVAL;
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
index 6ac27c7..e45b587 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
@@ -272,7 +272,7 @@ void xgene_enet_wr_mac(struct xgene_enet_pdata *pdata, u32 wr_addr, u32 wr_data)
u32 done;
if (pdata->mdio_driver && ndev->phydev &&
- pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
+ phy_interface_mode_is_rgmii(pdata->phy_mode)) {
struct mii_bus *bus = ndev->phydev->mdio.bus;
return xgene_mdio_wr_mac(bus->priv, wr_addr, wr_data);
@@ -326,12 +326,13 @@ static void xgene_enet_rd_mcx_csr(struct xgene_enet_pdata *pdata,
u32 xgene_enet_rd_mac(struct xgene_enet_pdata *pdata, u32 rd_addr)
{
void __iomem *addr, *rd, *cmd, *cmd_done;
+ struct net_device *ndev = pdata->ndev;
u32 done, rd_data;
u8 wait = 10;
- if (pdata->mdio_driver && pdata->ndev->phydev &&
- pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
- struct mii_bus *bus = pdata->ndev->phydev->mdio.bus;
+ if (pdata->mdio_driver && ndev->phydev &&
+ phy_interface_mode_is_rgmii(pdata->phy_mode)) {
+ struct mii_bus *bus = ndev->phydev->mdio.bus;
return xgene_mdio_rd_mac(bus->priv, rd_addr);
}
@@ -349,8 +350,7 @@ u32 xgene_enet_rd_mac(struct xgene_enet_pdata *pdata, u32 rd_addr)
udelay(1);
if (!done)
- netdev_err(pdata->ndev, "mac read failed, addr: %04x\n",
- rd_addr);
+ netdev_err(ndev, "mac read failed, addr: %04x\n", rd_addr);
rd_data = ioread32(rd);
iowrite32(0, cmd);
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 21cd4ef..d3906f6 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -1634,7 +1634,7 @@ static int xgene_enet_get_irqs(struct xgene_enet_pdata *pdata)
struct device *dev = &pdev->dev;
int i, ret, max_irqs;
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode))
max_irqs = 1;
else if (pdata->phy_mode == PHY_INTERFACE_MODE_SGMII)
max_irqs = 2;
@@ -1760,7 +1760,7 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
dev_err(dev, "Unable to get phy-connection-type\n");
return pdata->phy_mode;
}
- if (pdata->phy_mode != PHY_INTERFACE_MODE_RGMII &&
+ if (!phy_interface_mode_is_rgmii(pdata->phy_mode) &&
pdata->phy_mode != PHY_INTERFACE_MODE_SGMII &&
pdata->phy_mode != PHY_INTERFACE_MODE_XGMII) {
dev_err(dev, "Incorrect phy-connection-type specified\n");
@@ -1805,7 +1805,7 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
pdata->cle.base = base_addr + BLOCK_ETH_CLE_CSR_OFFSET;
pdata->eth_ring_if_addr = base_addr + BLOCK_ETH_RING_IF_OFFSET;
pdata->eth_diag_csr_addr = base_addr + BLOCK_ETH_DIAG_CSR_OFFSET;
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII ||
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode) ||
pdata->phy_mode == PHY_INTERFACE_MODE_SGMII) {
pdata->mcx_mac_addr = pdata->base_addr + BLOCK_ETH_MAC_OFFSET;
pdata->mcx_stats_addr =
@@ -1904,6 +1904,9 @@ static void xgene_enet_setup_ops(struct xgene_enet_pdata *pdata)
{
switch (pdata->phy_mode) {
case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
pdata->mac_ops = &xgene_gmac_ops;
pdata->port_ops = &xgene_gport_ops;
pdata->rm = RM3;
@@ -2100,7 +2103,7 @@ static int xgene_enet_probe(struct platform_device *pdev)
if (pdata->phy_mode == PHY_INTERFACE_MODE_XGMII) {
INIT_DELAYED_WORK(&pdata->link_work, link_state);
} else if (!pdata->mdio_driver) {
- if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_mode_is_rgmii(pdata->phy_mode))
ret = xgene_enet_mdio_config(pdata);
else
INIT_DELAYED_WORK(&pdata->link_work, link_state);
@@ -2131,7 +2134,7 @@ static int xgene_enet_probe(struct platform_device *pdev)
if (pdata->mdio_driver)
xgene_enet_phy_disconnect(pdata);
- else if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ else if (phy_interface_mode_is_rgmii(pdata->phy_mode))
xgene_enet_mdio_remove(pdata);
err1:
xgene_enet_delete_desc_rings(pdata);
@@ -2155,7 +2158,7 @@ static int xgene_enet_remove(struct platform_device *pdev)
if (pdata->mdio_driver)
xgene_enet_phy_disconnect(pdata);
- else if (pdata->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ else if (phy_interface_mode_is_rgmii(pdata->phy_mode))
xgene_enet_mdio_remove(pdata);
unregister_netdev(ndev);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 net-next 1/2] include: linux: Add helper function to check phy interface mode
From: Florian Fainelli @ 2017-05-18 22:19 UTC (permalink / raw)
To: Iyappan Subramanian, davem, netdev; +Cc: andrew, patches, linux-arm-kernel
In-Reply-To: <1495145624-29463-2-git-send-email-isubramanian@apm.com>
On 05/18/2017 03:13 PM, Iyappan Subramanian wrote:
> Added helper function that checks phy_mode is RGMII (all variants)
> 'bool phy_interface_mode_is_rgmii(phy_interface_t mode)'
>
> Changed the following function, to use the above.
> 'bool phy_interface_is_rgmii(struct phy_device *phydev)'
>
> Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
Not sure why you have chosen include: linux as the subject since all
changes done to that file typically had the "phy: " prefix, but the code
changes are fine, thanks!
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> include/linux/phy.h | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 54ef458..5a808a2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -716,14 +716,24 @@ static inline bool phy_is_internal(struct phy_device *phydev)
> }
>
> /**
> + * phy_interface_mode_is_rgmii - Convenience function for testing if a
> + * PHY interface mode is RGMII (all variants)
> + * @mode: the phy_interface_t enum
> + */
> +static inline bool phy_interface_mode_is_rgmii(phy_interface_t mode)
> +{
> + return mode >= PHY_INTERFACE_MODE_RGMII &&
> + mode <= PHY_INTERFACE_MODE_RGMII_TXID;
> +};
> +
> +/**
> * phy_interface_is_rgmii - Convenience function for testing if a PHY interface
> * is RGMII (all variants)
> * @phydev: the phy_device struct
> */
> static inline bool phy_interface_is_rgmii(struct phy_device *phydev)
> {
> - return phydev->interface >= PHY_INTERFACE_MODE_RGMII &&
> - phydev->interface <= PHY_INTERFACE_MODE_RGMII_TXID;
> + return phy_interface_mode_is_rgmii(phydev->interface);
> };
>
> /*
>
--
Florian
^ permalink raw reply
* Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral
From: Florian Fainelli @ 2017-05-18 22:21 UTC (permalink / raw)
To: Geert Uytterhoeven, Andrew Lunn
Cc: Geert Uytterhoeven, Rob Herring, Frank Rowand, Thomas Petazzoni,
Sergei Shtylyov, netdev@vger.kernel.org,
devicetree@vger.kernel.org, Linux-Renesas,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdV+jfh=EGt3SvjtrtVfWQ6P8ofp_JxeQZjRrSEfDWs5hA@mail.gmail.com>
On 05/18/2017 01:36 PM, Geert Uytterhoeven wrote:
> Hi Andrew,
>
> On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>> This most certainly works fine in the simple case where you have one PHY
>>>> hanging off the MDIO bus, now what happens if you have several?
>>>>
>>>> Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>>>> bus registration return EPROB_DEFER as well, and so on, and so forth,
>>>> but I am not sure if we will be properly unwinding the successful
>>>> registration of PHYs that either don't have an interrupt, or did not
>>>> return EPROBE_DEFER.
>>>>
>>>> It should be possible to mimic this behavior by using the fixed PHY, and
>>>> possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>>>> fixed PHYs to be present.
>>>
>>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>>> should do the unwinding, right?
>>>
>>> And when the driver is reprobed, all PHYs are reprobed, until they all
>>> succeed.
>>
>> That is the theory. I looked at that while reviewing the patch. But
>> this has probably not been tested in anger. It would be good to test
>> this properly, with not just the first PHY returning -EPROBE_DEFER, to
>> really test the unwind.
>
> Unfortunately I don't have a board with multiple PHYs, so I cannot test
> that case.
>
> Does unbinding/rebinding a network driver with multiple PHYs currently
> work? Or module unload/reload?
Usually there is a strict 1:1 mapping between a network device (not
driver) and a PHY device, switch drivers however, would have multiple
PHYs (one per port, aka net_deice).
NB: binding and unbinding of PHYs is pretty broken at the moment though,
because there is a complete disconnect between what the Ethernet MAC
expects, and the state in which the PHY is. I had some patches to fix
that, but this turned out to be playing whack-a-mole which I typically
suck at.
--
Florian
^ permalink raw reply
* [net-intel-e1000e] question about value overwrite
From: Gustavo A. R. Silva @ 2017-05-18 22:22 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: intel-wired-lan, netdev, linux-kernel
Hello everybody,
While looking into Coverity ID 1226905 I ran into the following piece
of code at drivers/net/ethernet/intel/e1000e/ich8lan.c:2400
2400/**
2401 * e1000_hv_phy_workarounds_ich8lan - A series of Phy workarounds to be
2402 * done after every PHY reset.
2403 **/
2404static s32 e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
2405{
2406 s32 ret_val = 0;
2407 u16 phy_data;
2408
2409 if (hw->mac.type != e1000_pchlan)
2410 return 0;
2411
2412 /* Set MDIO slow mode before any other MDIO access */
2413 if (hw->phy.type == e1000_phy_82577) {
2414 ret_val = e1000_set_mdio_slow_mode_hv(hw);
2415 if (ret_val)
2416 return ret_val;
2417 }
2418
2419 if (((hw->phy.type == e1000_phy_82577) &&
2420 ((hw->phy.revision == 1) || (hw->phy.revision == 2))) ||
2421 ((hw->phy.type == e1000_phy_82578) &&
(hw->phy.revision == 1))) {
2422 /* Disable generation of early preamble */
2423 ret_val = e1e_wphy(hw, PHY_REG(769, 25), 0x4431);
2424 if (ret_val)
2425 return ret_val;
2426
2427 /* Preamble tuning for SSC */
2428 ret_val = e1e_wphy(hw, HV_KMRN_FIFO_CTRLSTA, 0xA204);
2429 if (ret_val)
2430 return ret_val;
2431 }
2432
2433 if (hw->phy.type == e1000_phy_82578) {
2434 /* Return registers to default by doing a soft reset then
2435 * writing 0x3140 to the control register.
2436 */
2437 if (hw->phy.revision < 2) {
2438 e1000e_phy_sw_reset(hw);
2439 ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
2440 }
2441 }
2442
2443 /* Select page 0 */
2444 ret_val = hw->phy.ops.acquire(hw);
2445 if (ret_val)
2446 return ret_val;
2447
2448 hw->phy.addr = 1;
2449 ret_val = e1000e_write_phy_reg_mdic(hw,
IGP01E1000_PHY_PAGE_SELECT, 0);
2450 hw->phy.ops.release(hw);
2451 if (ret_val)
2452 return ret_val;
2453
2454 /* Configure the K1 Si workaround during phy reset
assuming there is
2455 * link so that it disables K1 if link is in 1Gbps.
2456 */
2457 ret_val = e1000_k1_gig_workaround_hv(hw, true);
2458 if (ret_val)
2459 return ret_val;
2460
2461 /* Workaround for link disconnects on a busy hub in half duplex */
2462 ret_val = hw->phy.ops.acquire(hw);
2463 if (ret_val)
2464 return ret_val;
2465 ret_val = e1e_rphy_locked(hw, BM_PORT_GEN_CFG, &phy_data);
2466 if (ret_val)
2467 goto release;
2468 ret_val = e1e_wphy_locked(hw, BM_PORT_GEN_CFG, phy_data & 0x00FF);
2469 if (ret_val)
2470 goto release;
2471
2472 /* set MSE higher to enable link to stay up when noise is high */
2473 ret_val = e1000_write_emi_reg_locked(hw,
I82577_MSE_THRESHOLD, 0x0034);
2474release:
2475 hw->phy.ops.release(hw);
2476
2477 return ret_val;
2478}
The issue is that the value stored in variable _ret_val_ at line 2439
is overwritten by the one stored at line 2444, before it can be used.
My question is if the original intention was to return this value
immediately after the assignment at line 2439, something like in the
following patch:
index 68ea8b4..d6d4ed7 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -2437,6 +2437,8 @@ static s32
e1000_hv_phy_workarounds_ich8lan(struct e1000_hw *hw)
if (hw->phy.revision < 2) {
e1000e_phy_sw_reset(hw);
ret_val = e1e_wphy(hw, MII_BMCR, 0x3140);
+ if (ret_val)
+ return ret_val;
}
}
What do you think?
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
^ permalink raw reply related
* Re: drivers/net/hamradio: divide error in hdlcdrv_ioctl
From: Andrey Konovalov @ 2017-05-19 0:29 UTC (permalink / raw)
To: Alan Cox
Cc: David S. Miller, Alan Cox, Thomas Osterried,
Javier Martinez Canillas, David Howells, Geliang Tang, netdev,
LKML, syzkaller, Dmitry Vyukov, Kostya Serebryany
In-Reply-To: <20170517210740.20cbbb82@alans-desktop>
On Wed, May 17, 2017 at 10:07 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> On Tue, 16 May 2017 17:05:32 +0200
> Andrey Konovalov <andreyknvl@google.com> wrote:
>
>> Hi,
>>
>> I've got the following error report while fuzzing the kernel with syzkaller.
>>
>> On commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6 (4.12-rc1).
>>
>> A reproducer and .config are attached.
>
> This should fix it.
Hi Alan,
Someone else has already sent a couple of versions of a similar fix.
https://patchwork.ozlabs.org/patch/763832/
Thanks!
>
> commit 37b3fa4b617681f00cfa1f76d6d7716cc6d9f79a
> Author: Alan Cox <alan@llwyncelyn.cymru>
> Date: Wed May 17 21:04:27 2017 +0100
>
> hdlcdrv: Fix division by zero when bitrate is unset
>
> The code attempts to check for out of range calibration. What it forgets to do
> is check for the 0 bitrate case. As a result the range check itself oopses the
> kernel.
>
> Found by Andrey Konovalov using Syzkaller.
>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
>
> diff --git a/drivers/net/hamradio/hdlcdrv.c b/drivers/net/hamradio/hdlcdrv.c
> index 8c3633c..9f34a48 100644
> --- a/drivers/net/hamradio/hdlcdrv.c
> +++ b/drivers/net/hamradio/hdlcdrv.c
> @@ -576,7 +576,7 @@ static int hdlcdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> case HDLCDRVCTL_CALIBRATE:
> if(!capable(CAP_SYS_RAWIO))
> return -EPERM;
> - if (bi.data.calibrate > INT_MAX / s->par.bitrate)
> + if (!s->par.bitrate || bi.data.calibrate > INT_MAX / s->par.bitrate)
> return -EINVAL;
> s->hdlctx.calibrate = bi.data.calibrate * s->par.bitrate / 16;
> return 0;
^ permalink raw reply
* Re: [[PATCH v1]] hdlcdrv: fix divide error bug if bitrate is 0
From: Andrey Konovalov @ 2017-05-19 0:31 UTC (permalink / raw)
To: Firo Yang
Cc: t.sailer, wharms, linux-hams, netdev, Dmitry Vyukov, syzkaller,
David S. Miller, Greg Kroah-Hartman
In-Reply-To: <1495080138-21695-1-git-send-email-firogm@gmail.com>
On Thu, May 18, 2017 at 6:02 AM, Firo Yang <firogm@gmail.com> wrote:
> The divisor s->par.bitrate will always be 0 until initialized by
> ndo_open() and hdlcdrv_open().
>
> In order to fix this divide zero error, check whether the netdevice was
> opened by ndo_open() before performing divide.And we also check the the
> value of bitrate in case of bad setting of it.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Firo Yang <firogm@gmail.com>
Hi Firo,
Please reply to the original report thread when you send a fix, so
other people won't start working on the same patch.
BTW, it was reported by me, but I don't think it's important.
Thanks!
> ---
> v0->v1:
> Reviewed by walter harms <wharms@bfs.de>.
> Return ENODEV instead of EPERM if !netif_running(dev)
> Check if s->par.bitrate > 0.
>
> drivers/net/hamradio/hdlcdrv.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/hamradio/hdlcdrv.c b/drivers/net/hamradio/hdlcdrv.c
> index 8c3633c..b0f417f 100644
> --- a/drivers/net/hamradio/hdlcdrv.c
> +++ b/drivers/net/hamradio/hdlcdrv.c
> @@ -576,6 +576,10 @@ static int hdlcdrv_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> case HDLCDRVCTL_CALIBRATE:
> if(!capable(CAP_SYS_RAWIO))
> return -EPERM;
> + if (!netif_running(dev))
> + return -ENODEV;
> + if (!(s->par.bitrate > 0))
> + return -EINVAL;
> if (bi.data.calibrate > INT_MAX / s->par.bitrate)
> return -EINVAL;
> s->hdlctx.calibrate = bi.data.calibrate * s->par.bitrate / 16;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.
From: Toshiaki Makita @ 2017-05-19 1:04 UTC (permalink / raw)
To: Vladislav Yasevich, netdev; +Cc: Vladislav Yasevich
In-Reply-To: <1495114265-23368-2-git-send-email-vyasevic@redhat.com>
On 2017/05/18 22:31, Vladislav Yasevich wrote:
> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken. The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans. However, most HW we support can't handle
> it. To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
>
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic. Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
>
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
The patch looks ok, but why do you think 8cb65d000 is wrong?
The same check was there before my patch set.
kernel v4.0:
> netdev_features_t netif_skb_features(struct sk_buff *skb)
...
> if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD))
> features = netdev_intersect_features(features,
> NETIF_F_SG |
> NETIF_F_HIGHDMA |
> NETIF_F_FRAGLIST |
> NETIF_F_GEN_CSUM |
> NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_STAG_TX);
The commit just moved the check into another function.
Toshiaki Makita
^ permalink raw reply
* Re: [PATCH v2 1/3] bpf: Use 1<<16 as ceiling for immediate alignment in verifier.
From: Alexei Starovoitov @ 2017-05-19 1:22 UTC (permalink / raw)
To: Edward Cree, David Miller, Daniel Borkmann; +Cc: alexei.starovoitov, netdev
In-Reply-To: <739ef342-728c-de57-c2a2-03fa85b3a246@solarflare.com>
On 5/18/17 9:38 AM, Edward Cree wrote:
> On 18/05/17 15:49, Edward Cree wrote:
>> Here's one idea that seemed to work when I did a couple of experiments:
>> let A = (a;am), B = (b;bm) where the m are the masks
>> Σ = am + bm + a + b
>> χ = Σ ^ (a + b) /* unknown carries */
>> μ = χ | am | bm /* mask of result */
>> then A + B = ((a + b) & ~μ; μ)
>>
>> The idea is that we find which bits change between the case "all x are
>> 1" and "all x are 0", and those become xs too.
> https://gist.github.com/ecree-solarflare/0665d5b46c2d8d08de2377fbd527de8d
I played with it quite a bit trying to break it and have to
agree that the above algorithm works.
At least for add and sub I think it's solid.
Still feels a bit magical, since it gave me better results
than I could envision for my test vectors.
In your .py I'd only change __str__(self) to print them in mask,value
as the order they're passed into constructor to make it easier to read.
The bin(self) output is the most useful, of course.
We should carry it into the kernel too for debugging.
> And now I've found a similar algorithm for subtraction, which (again) I
> can't prove but it seems to work.
> α = a + am - b
> β = a - b - bm
> χ = α ^ β
> μ = χ | α | β
> then A - B = ((a - b) & ~μ; μ)
> Again we're effectively finding the max. and min. values, and XORing
> them to find unknown carries.
>
> Bitwise operations are easy, of course;
> /* By assumption, a & am == b & bm == 0 */
> A & B = (a & b; (a | am) & (b | bm) & ~(a & b))
> A | B = (a | b; (am | bm) & ~(a | b))
> /* It bothers me that & and | aren't symmetric, but I can't fix it */
> A ^ B = (a ^ b; am | bm)
>
> as are shifts by a constant (just shift 0s into both number and mask).
>
> Multiplication by a constant can be done by decomposing into shifts
> and adds; but it can also be done directly; here we find (a;am) * k.
> π = a * k
> γ = am * k
> then A * k = (π; 0) + (0; γ), for which we use our addition algo.
>
> Multiplication of two unknown values is a nightmare, as unknown bits
> can propagate all over the place. We can do a shift-add
> decomposition where the adds for unknown bits have all the 1s in
> the addend replaced with xs. A few experiments suggest that this
> works, regardless of the order of operands. For instance
> 110x * x01 comes out as either
> 110x
> + xx0x
> = xxxx0x
> or
> x0x
> x01
> + x01
> = xxxx0x
> We can slightly optimise this by handling all the 1 bits in one go;
> that is, for (a;am) * (b;bm) we first find (a;am) * b using our
> multiplication-by-a-constant algo above, then for each bit in bm
> we find (a;am) * bit and force all its nonzero bits to unknown;
> finally we add all our components.
this mul algo I don't completely understand. It feels correct,
but I'm not sure we really need it for the kernel.
For all practical cases llvm will likely emit shifts or sequence
of adds and shifts, so multiplies by crazy non-optimizable constant
or variable are rare and likely the end result is going to be
outside of packet boundary, so it will be rejected anyway and
precise alignment tracking doesn't matter much.
What I love about the whole thing that it works for access into
packet, access into map values and in the future for any other
variable length access.
> Don't even ask about division; that scrambles bits so hard that the
yeah screw div and mod. We have an option to disable div/mod altogether
under some new 'prog_flags', since it has this ugly 'div by 0'
exception path. We don't even have 'signed division' instruction and
llvm errors like:
errs() << "Unsupport signed division for DAG: ";
errs() << "Please convert to unsigned div/mod.\n";
and no one complained. It just means that division is extremely rare.
Are you planning to work on the kernel patch for this algo?
Once we have it the verifier will be smarter regarding
alignment tracking than any compiler i know :)
^ 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