* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
From: Edward Cree @ 2018-11-06 22:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin Lau, Yonghong Song, Alexei Starovoitov,
daniel@iogearbox.net, netdev@vger.kernel.org, Kernel Team
In-Reply-To: <20181106215633.gi6v4fi2llmqo2rn@ast-mbp>
On 06/11/18 21:56, Alexei Starovoitov wrote:
> that looks very weird to me. Why split func name from argument names?
> The 'function name' as seen by the BTF may not be the symbol name
> as seen in elf file.
The symbol name will be in the symbol table, which is not the same
thing as the functions table in BTF that I'm proposing. (They do
look a little similar as I included an insn_idx for functions that
partially duplicates the offset given in the symbol table. But
that's necessary precisely for the reason you mention, that the
function name != the symbol name in general.)
"Splitting" func name from argument names is partly to potentially
save space — if we'd had "int bar(int x)" instead then 'bar' could
share its type record with 'foo'. And partly just because the
name of the function itself is no more part of its type than the
name of an integer variable is part of the integer's type.
(Whereas names of parameters are like names of struct members:
while they are not part of the 'pure type' from a language
perspective, they are part of the type from the perspective of
debugging, which is why they belong in the BTF type record.)
> There are C, bpftrace, p4 and python frontends. These languages
> should be free to put into BTF KIND_FUNC name that makes sense
> from the language point of view.
I'm paying attention to BTF because I'm adding support for it into
my ebpf_asm. Don't you think I *know* that frontends for BPF are
more than just C?
>> and in the 'variables' section we might have
>> 1 "quux" type=1 where=stack func=1 offset=-8
> that doesn't work. stack slots can be reused by compiler.
And who says that there can't be multiple records pointing to the
same stack slot with different types & names?
> Instead we will annotate every load/store with btf type id.
That's certainly more useful; but I think most useful of all is to
have *both* (though the stack slot types should be optional).
> The global variables for given .c file will look like single KIND_STRUCT
That's exactly the kind of superficially-clever but nasty hack
that results from the continued insistence on conflating types
and instances (objects). In the long run it will make
maintenance harder, and frustrate new features owing to the need
to find new hacks to shoehorn them into the same model.
Instead there should be entries for the globals in something like
the variables table I mentioned,
2 "fred" type=1 where=global func=0 offset=8
in which 'func' is unused and 'offset' gives offset in .bss.
'where' might also include indication of whether it's static.
Then for linkage you can extend this with index of which file it
came from.
But maybe discussing global variables is a bit premature as eBPF
doesn't have any such thing yet.
> yes we do see these things differently.
> To us function name is the debug info that fits well into BTF description.
> Whereas you see the function name part of function declaration
> as something 'entirely different'.
I'm not saying that the function name is 'entirely different'
to the rest of the type. (Though I do think it doesn't
belong in the type, that's a weaker and contingent point.)
I'm saying that the *function* is entirely different to its
*type*. It's a category error to conflate them:
f: x ↦ x + 1
is a function.
int → int
is a type, and specifically the type of the object named "f".
(And the nature of mathematical notation for functions happens
to put the name 'x' in the former, whereas we are putting the
parameter name in the latter, but that's irrelevant.)
Similarly, "1" is an integer, but "integer" is a type, and is
not itself an integer, while "1" is not a type. They are at
different meta-levels.
-Ed
^ permalink raw reply
* Re: [PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()
From: David Miller @ 2018-11-06 22:59 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <CANn89iJpP-41S0nhmyCanpK03z0ayShgfAqis0jgY7RhsqE=Ng@mail.gmail.com>
From: Eric Dumazet <edumazet@google.com>
Date: Tue, 6 Nov 2018 14:51:15 -0800
> On Tue, Nov 6, 2018 at 2:41 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Tue, 6 Nov 2018 14:25:52 -0800
>>
>> > + if (unlikely(nlen > sizeof(struct ipv6hdr))) {
>> > + if (memcmp(iph + 1, iph2 + 1,
>> > + nlen - sizeof(struct ipv6hdr)))
>> > + goto not_same_flow;
>> > + }
>>
>> Is this even possible?
>
> I believe that nlen can be indeed > sizeof(struct ipv6hdr) in presence
> of exthdrs,
> eg if ipv6_gso_pull_exthdrs() had to be called (line 201)
>
> I admit I have not checked if this was actually possible.
Indeed, that does make it possible.
Patch applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next 0/3] net: More extack messages
From: David Miller @ 2018-11-06 23:01 UTC (permalink / raw)
To: dsahern; +Cc: netdev, dsahern
In-Reply-To: <20181106205116.7718-1-dsahern@kernel.org>
From: David Ahern <dsahern@kernel.org>
Date: Tue, 6 Nov 2018 12:51:13 -0800
> From: David Ahern <dsahern@gmail.com>
>
> Add more extack messages for several link create errors (e.g., invalid
> number of queues, unknown link kind) and invalid metrics argument.
Series applied, thanks David.
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: dsa: bcm_sf2: Store rules in lists
From: David Miller @ 2018-11-06 23:06 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, vivien.didelot, pablo
In-Reply-To: <20181106205841.14308-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 6 Nov 2018 12:58:36 -0800
> Hi all,
>
> This patch series changes the bcm-sf2 driver to keep a copy of the
> inserted rules as opposed to using the HW as a storage area for a number
> of reasons:
>
> - this helps us with doing duplicate rule detection in a faster way, it
> would have required a full rule read before
>
> - this helps with Pablo's on-going work to convert ethtool_rx_flow_spec
> to a more generic flow rule structure by having fewer code paths to
> convert to the new structure/helpers
>
> - we need to cache copies to restore them during drive resumption,
> because depending on the low power mode the system has entered, the
> switch may have lost all of its context
Looks good to me, series applied and build testing right now.
I will say that the ethtool flow spec comparison should probably
eventually be broken out into a helper function places somewhere
common. It's very likely this approach, and thus the helper, can
be used by other drivers in a similar situation.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: dsa: bcm_sf2: Store rules in lists
From: Florian Fainelli @ 2018-11-06 23:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, andrew, vivien.didelot, pablo
In-Reply-To: <20181106.150619.2176282733992923198.davem@davemloft.net>
On 11/6/18 3:06 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 6 Nov 2018 12:58:36 -0800
>
>> Hi all,
>>
>> This patch series changes the bcm-sf2 driver to keep a copy of the
>> inserted rules as opposed to using the HW as a storage area for a number
>> of reasons:
>>
>> - this helps us with doing duplicate rule detection in a faster way, it
>> would have required a full rule read before
>>
>> - this helps with Pablo's on-going work to convert ethtool_rx_flow_spec
>> to a more generic flow rule structure by having fewer code paths to
>> convert to the new structure/helpers
>>
>> - we need to cache copies to restore them during drive resumption,
>> because depending on the low power mode the system has entered, the
>> switch may have lost all of its context
>
> Looks good to me, series applied and build testing right now.
>
> I will say that the ethtool flow spec comparison should probably
> eventually be broken out into a helper function places somewhere
> common. It's very likely this approach, and thus the helper, can
> be used by other drivers in a similar situation.
Sure, that could be done, I will check with Pablo how he wants to
approach that as well since he is reworking how flow rules are
represented. Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v4 1/3] net: emac: implement 802.1Q VLAN TX tagging support
From: David Miller @ 2018-11-06 23:09 UTC (permalink / raw)
To: chunkeey; +Cc: netdev, ivan, f.fainelli
In-Reply-To: <0f8f149bb526b911813cfe555f99ef00db2f1387.1541543231.git.chunkeey@gmail.com>
From: Christian Lamparter <chunkeey@gmail.com>
Date: Tue, 6 Nov 2018 23:27:49 +0100
> @@ -1435,6 +1436,22 @@ static inline netdev_tx_t emac_xmit_finish(struct emac_instance *dev, int len)
> return NETDEV_TX_OK;
> }
>
> +static inline u16 emac_tx_vlan(struct emac_instance *dev, struct sk_buff *skb)
> +{
> + /* Handle VLAN TPID and TCI insert if this is a VLAN skb */
> + if (emac_has_feature(dev, EMAC_FTR_HAS_VLAN_CTAG_TX) &&
> + skb_vlan_tag_present(skb)) {
> + struct emac_regs __iomem *p = dev->emacp;
> +
> + /* update the VLAN TCI */
> + out_be32(&p->vtci, (u32)skb_vlan_tag_get(skb));
Hmmm, how does this vtci register work?
How can you have a global piece of register state controlling the VLAN
tag that will be used for the TX frame?
What happens if you queue up several TX SKBs, each one with a different
VLAN tci?
Normally the TCI state is implemented on a per-tx-descriptor basis.
^ permalink raw reply
* Re: [RFC perf,bpf 1/5] perf, bpf: Introduce PERF_RECORD_BPF_EVENT
From: Peter Zijlstra @ 2018-11-07 8:40 UTC (permalink / raw)
To: Song Liu; +Cc: netdev, linux-kernel, kernel-team, ast, daniel, acme
In-Reply-To: <20181106205246.567448-2-songliubraving@fb.com>
On Tue, Nov 06, 2018 at 12:52:42PM -0800, Song Liu wrote:
> For better performance analysis of BPF programs, this patch introduces
> PERF_RECORD_BPF_EVENT, a new perf_event_type that exposes BPF program
> load/unload information to user space.
>
> /*
> * Record different types of bpf events:
> * enum perf_bpf_event_type {
> * PERF_BPF_EVENT_UNKNOWN = 0,
> * PERF_BPF_EVENT_PROG_LOAD = 1,
> * PERF_BPF_EVENT_PROG_UNLOAD = 2,
> * };
> *
> * struct {
> * struct perf_event_header header;
> * u16 type;
> * u16 flags;
> * u32 id; // prog_id or map_id
> * };
> */
> PERF_RECORD_BPF_EVENT = 17,
>
> PERF_RECORD_BPF_EVENT contains minimal information about the BPF program.
> Perf utility (or other user space tools) should listen to this event and
> fetch more details about the event via BPF syscalls
> (BPF_PROG_GET_FD_BY_ID, BPF_OBJ_GET_INFO_BY_FD, etc.).
Why !? You're failing to explain why it cannot provide the full
information there.
^ permalink raw reply
* [PATCH net-next 0/3] net: systemport: Unmap queues upon DSA unregister event
From: Florian Fainelli @ 2018-11-06 23:15 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, vivien.didelot, Florian Fainelli
Hi all,
This patch series fixes the unbinding/binding of the bcm_sf2 switch
driver along with bcmsysport which monitors the switch port queues.
Because the driver was not processing the DSA_PORT_UNREGISTER event, we
would not be unmapping switch port/queues, which could cause incorrect
decisions to be made by the HW (e.g: queue always back-pressured).
Florian Fainelli (3):
net: dsa: bcm_sf2: Turn on PHY to allow successful registration
net: systemport: Simplify queue mapping logic
net: systemport: Unmap queues upon DSA unregister event
drivers/net/dsa/bcm_sf2.c | 4 ++
drivers/net/ethernet/broadcom/bcmsysport.c | 71 ++++++++++++++++++----
drivers/net/ethernet/broadcom/bcmsysport.h | 1 -
3 files changed, 62 insertions(+), 14 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH net-next 1/3] net: dsa: bcm_sf2: Turn on PHY to allow successful registration
From: Florian Fainelli @ 2018-11-06 23:15 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, vivien.didelot, Florian Fainelli
In-Reply-To: <20181106231518.16314-1-f.fainelli@gmail.com>
We are binding to the PHY using the SF2 slave MDIO bus that we create,
binding involves reading the PHY's MII_PHYSID1/2 which won't be possible
if the PHY is turned off. Temporarily turn it on/off for the bus probing
to succeeed. This fixes unbind/bind problems where the port connecting
to that PHY would be in error since it could not connect to it.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/dsa/bcm_sf2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 2eb68769562c..2c664aac1e7b 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1090,12 +1090,16 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
return ret;
}
+ bcm_sf2_gphy_enable_set(priv->dev->ds, true);
+
ret = bcm_sf2_mdio_register(ds);
if (ret) {
pr_err("failed to register MDIO bus\n");
return ret;
}
+ bcm_sf2_gphy_enable_set(priv->dev->ds, false);
+
ret = bcm_sf2_cfp_rst(priv);
if (ret) {
pr_err("failed to reset CFP\n");
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/3] net: systemport: Simplify queue mapping logic
From: Florian Fainelli @ 2018-11-06 23:15 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, vivien.didelot, Florian Fainelli
In-Reply-To: <20181106231518.16314-1-f.fainelli@gmail.com>
The use of a bitmap speeds up the finding of the first available queue
to which we could start establishing the mapping for, but we still have
to loop over all slave network devices to set them up. Simplify the
logic to have a single loop, and use the fact that a correctly
configured ring has inspect set to true. This will make things simpler
to unwind during device unregistration.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 17 +++++++++--------
drivers/net/ethernet/broadcom/bcmsysport.h | 1 -
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 0e2d99c737e3..f620c647bb86 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2312,7 +2312,7 @@ static int bcm_sysport_map_queues(struct notifier_block *nb,
struct bcm_sysport_priv *priv;
struct net_device *slave_dev;
unsigned int num_tx_queues;
- unsigned int q, start, port;
+ unsigned int q, qp, port;
struct net_device *dev;
priv = container_of(nb, struct bcm_sysport_priv, dsa_notifier);
@@ -2351,20 +2351,21 @@ static int bcm_sysport_map_queues(struct notifier_block *nb,
priv->per_port_num_tx_queues = num_tx_queues;
- start = find_first_zero_bit(&priv->queue_bitmap, dev->num_tx_queues);
- for (q = 0; q < num_tx_queues; q++) {
- ring = &priv->tx_rings[q + start];
+ for (q = 0, qp = 0; q < dev->num_tx_queues && qp < num_tx_queues;
+ q++) {
+ ring = &priv->tx_rings[q];
+
+ if (ring->inspect)
+ continue;
/* Just remember the mapping actual programming done
* during bcm_sysport_init_tx_ring
*/
- ring->switch_queue = q;
+ ring->switch_queue = qp;
ring->switch_port = port;
ring->inspect = true;
priv->ring_map[q + port * num_tx_queues] = ring;
-
- /* Set all queues as being used now */
- set_bit(q + start, &priv->queue_bitmap);
+ qp++;
}
return 0;
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index a7a230884a87..94d64b203098 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -795,7 +795,6 @@ struct bcm_sysport_priv {
/* map information between switch port queues and local queues */
struct notifier_block dsa_notifier;
unsigned int per_port_num_tx_queues;
- unsigned long queue_bitmap;
struct bcm_sysport_tx_ring *ring_map[DSA_MAX_PORTS * 8];
};
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 3/3] net: systemport: Unmap queues upon DSA unregister event
From: Florian Fainelli @ 2018-11-06 23:15 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, vivien.didelot, Florian Fainelli
In-Reply-To: <20181106231518.16314-1-f.fainelli@gmail.com>
Binding and unbinding the switch driver which creates the DSA slave
network devices for which we set-up inspection would lead to
undesireable effects since we were not clearing the port/queue mapping
to the SYSTEMPORT TX queue.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 56 +++++++++++++++++++---
1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index f620c647bb86..f8f0a027b3ae 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2371,17 +2371,61 @@ static int bcm_sysport_map_queues(struct notifier_block *nb,
return 0;
}
+static int bcm_sysport_unmap_queues(struct notifier_block *nb,
+ struct dsa_notifier_register_info *info)
+{
+ struct bcm_sysport_tx_ring *ring;
+ struct bcm_sysport_priv *priv;
+ struct net_device *slave_dev;
+ unsigned int num_tx_queues;
+ struct net_device *dev;
+ unsigned int q, port;
+
+ priv = container_of(nb, struct bcm_sysport_priv, dsa_notifier);
+ if (priv->netdev != info->master)
+ return 0;
+
+ dev = info->master;
+
+ if (dev->netdev_ops != &bcm_sysport_netdev_ops)
+ return 0;
+
+ port = info->port_number;
+ slave_dev = info->info.dev;
+
+ num_tx_queues = slave_dev->real_num_tx_queues;
+
+ for (q = 0; q < dev->num_tx_queues; q++) {
+ ring = &priv->tx_rings[q];
+
+ if (ring->switch_port != port)
+ continue;
+
+ if (!ring->inspect)
+ continue;
+
+ ring->inspect = false;
+ priv->ring_map[q + port * num_tx_queues] = NULL;
+ }
+
+ return 0;
+}
+
static int bcm_sysport_dsa_notifier(struct notifier_block *nb,
unsigned long event, void *ptr)
{
- struct dsa_notifier_register_info *info;
+ int ret = NOTIFY_DONE;
- if (event != DSA_PORT_REGISTER)
- return NOTIFY_DONE;
-
- info = ptr;
+ switch (event) {
+ case DSA_PORT_REGISTER:
+ ret = bcm_sysport_map_queues(nb, ptr);
+ break;
+ case DSA_PORT_UNREGISTER:
+ ret = bcm_sysport_unmap_queues(nb, ptr);
+ break;
+ }
- return notifier_from_errno(bcm_sysport_map_queues(nb, info));
+ return notifier_from_errno(ret);
}
#define REV_FMT "v%2x.%02x"
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 00/11] ICMP error handling for UDP tunnels
From: David Miller @ 2018-11-06 23:24 UTC (permalink / raw)
To: sbrivio; +Cc: sd, lucien.xin, netdev
In-Reply-To: <cover.1541533786.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Tue, 6 Nov 2018 22:38:56 +0100
> This series introduces ICMP error handling for UDP tunnels and
> encapsulations and related selftests. We need to handle ICMP errors to
> support PMTU discovery and route redirection -- this support is entirely
> missing right now:
>
> - patch 1/11 adds a socket lookup for UDP tunnels that use, by design,
> the same destination port on both endpoints -- i.e. VxLAN and GENEVE
> - patches 2/11 to 7/11 are specific to VxLAN and GENEVE
> - patches 8/11 and 9/11 add infrastructure for lookup of encapsulations
> where sent packets cannot be matched via receiving socket lookup, i.e.
> FoU and GUE
> - patches 10/11 and 11/11 are specific to FoU and GUE
I like this series, especially the testcases.
But I have a minor coding style issue or two I'd like you
to fixup before I apply this.
I'll reply to individual patches as needed.
^ permalink raw reply
* Re: [PATCH net-next 01/11] udp: Handle ICMP errors for tunnels with same destination port on both endpoints
From: David Miller @ 2018-11-06 23:25 UTC (permalink / raw)
To: sbrivio; +Cc: sd, lucien.xin, netdev
In-Reply-To: <9f1e659e3745bbc8f29a9debb9bb8c0d8918fa24.1541533786.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Tue, 6 Nov 2018 22:38:57 +0100
> + /* Network header needs to point to the outer IPv4 header inside ICMP */
> + skb_reset_network_header(skb);
> + iph = ip_hdr(skb);
> + /* Transport header needs to point to the UDP header */
> + skb_set_transport_header(skb, iph->ihl << 2);
Please put an empty line before the second comment.
^ permalink raw reply
* Re: [PATCH net-next 09/11] udp: Support for error handlers of tunnels with arbitrary destination port
From: David Miller @ 2018-11-06 23:26 UTC (permalink / raw)
To: sbrivio; +Cc: sd, lucien.xin, netdev
In-Reply-To: <d4649d12308c6d51f36a809e63514a39c75564af.1541533786.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Tue, 6 Nov 2018 22:39:05 +0100
> diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
> index 236e40ba06bf..7855966b4a19 100644
> --- a/include/net/ip6_tunnel.h
> +++ b/include/net/ip6_tunnel.h
> @@ -69,6 +69,8 @@ struct ip6_tnl_encap_ops {
> size_t (*encap_hlen)(struct ip_tunnel_encap *e);
> int (*build_header)(struct sk_buff *skb, struct ip_tunnel_encap *e,
> u8 *protocol, struct flowi6 *fl6);
> + int (*err_handler)(struct sk_buff *, struct inet6_skb_parm *opt,
> + u8 type, u8 code, int offset, __be32 info);
> };
Please give names to all of the arguments in this new method.
...
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index b0d022ff6ea1..5980659312e5 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -311,6 +311,7 @@ struct ip_tunnel_encap_ops {
> size_t (*encap_hlen)(struct ip_tunnel_encap *e);
> int (*build_header)(struct sk_buff *skb, struct ip_tunnel_encap *e,
> u8 *protocol, struct flowi4 *fl4);
> + int (*err_handler)(struct sk_buff *, u32);
Likewise.
^ permalink raw reply
* [PATCH RFC net-next 0/3] net: phy: sfp: Warn when using generic PHY driver
From: Florian Fainelli @ 2018-11-06 23:29 UTC (permalink / raw)
To: netdev; +Cc: andrew, rmk+kernel, davem, Florian Fainelli
Hi all,
This patch series allows warning an user that the generic PHY driver(s)
are used when a SFP incorporates a PHY (e.g: 1000BaseT SFP) which is
likely not going to work at all.
Let me know if you would want to do that differently.
Florian Fainelli (3):
net: phy: Add helpers to determine if PHY driver is generic
net: phy: sfp: Issue warning when using Generic PHY driver(s)
net: phy: Default MARVELL_PHY to the value of SFP
drivers/net/phy/Kconfig | 1 +
drivers/net/phy/phy_device.c | 34 ++++++++++++++++++++++++++++++++--
drivers/net/phy/sfp.c | 3 +++
include/linux/phy.h | 3 +++
4 files changed, 39 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH RFC net-next 1/3] net: phy: Add helpers to determine if PHY driver is generic
From: Florian Fainelli @ 2018-11-06 23:29 UTC (permalink / raw)
To: netdev; +Cc: andrew, rmk+kernel, davem, Florian Fainelli
In-Reply-To: <20181106232913.17216-1-f.fainelli@gmail.com>
We are already checking in phy_detach() that the PHY driver is of
generic kind (1G or 10G) and we are going to make use of that in the SFP
layer as well for 1000BaseT SFP modules, so expose helper functions to
return that information.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy_device.c | 34 ++++++++++++++++++++++++++++++++--
include/linux/phy.h | 3 +++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ab33d1777132..15de7a3263bf 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1262,6 +1262,36 @@ struct phy_device *phy_attach(struct net_device *dev, const char *bus_id,
}
EXPORT_SYMBOL(phy_attach);
+static bool phy_driver_is_genphy_kind(struct phy_device *phydev,
+ struct device_driver *driver)
+{
+ struct device *d = &phydev->mdio.dev;
+ bool ret = false;
+
+ if (!phydev->drv)
+ return ret;
+
+ get_device(d);
+ ret = d->driver == driver;
+ put_device(d);
+
+ return ret;
+}
+
+bool phy_driver_is_genphy(struct phy_device *phydev)
+{
+ return phy_driver_is_genphy_kind(phydev,
+ &genphy_driver.mdiodrv.driver);
+}
+EXPORT_SYMBOL_GPL(phy_driver_is_genphy);
+
+bool phy_driver_is_genphy_10g(struct phy_device *phydev)
+{
+ return phy_driver_is_genphy_kind(phydev,
+ &genphy_10g_driver.mdiodrv.driver);
+}
+EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
+
/**
* phy_detach - detach a PHY device from its network device
* @phydev: target phy_device struct
@@ -1293,8 +1323,8 @@ void phy_detach(struct phy_device *phydev)
* from the generic driver so that there's a chance a
* real driver could be loaded
*/
- if (phydev->mdio.dev.driver == &genphy_10g_driver.mdiodrv.driver ||
- phydev->mdio.dev.driver == &genphy_driver.mdiodrv.driver)
+ if (phy_driver_is_genphy(phydev) ||
+ phy_driver_is_genphy_10g(phydev))
device_release_driver(&phydev->mdio.dev);
/*
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ea87f774a76..84a6c7efef60 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1192,4 +1192,7 @@ module_exit(phy_module_exit)
#define module_phy_driver(__phy_drivers) \
phy_module_driver(__phy_drivers, ARRAY_SIZE(__phy_drivers))
+bool phy_driver_is_genphy(struct phy_device *phydev);
+bool phy_driver_is_genphy_10g(struct phy_device *phydev);
+
#endif /* __PHY_H */
--
2.17.1
^ permalink raw reply related
* [PATCH RFC net-next 2/3] net: phy: sfp: Issue warning when using Generic PHY driver(s)
From: Florian Fainelli @ 2018-11-06 23:29 UTC (permalink / raw)
To: netdev; +Cc: andrew, rmk+kernel, davem, Florian Fainelli
In-Reply-To: <20181106232913.17216-1-f.fainelli@gmail.com>
1000BaseT SFP modules typically include an Ethernet PHY device, and
while the Generic PHY driver will be able to bind to it, it usually will
not work at all without a specialized PHY driver. Issue a warning in
that case to help toubleshoot things.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/sfp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index fd8bb998ae52..228205d8ce84 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1203,6 +1203,9 @@ static void sfp_sm_probe_phy(struct sfp *sfp)
}
sfp->mod_phy = phy;
+ if (phy_driver_is_genphy(phy) || phy_driver_is_genphy_10g(phy))
+ dev_warn(sfp->dev, "Using Generic PHY driver with a SFP!\n");
+
phy_start(phy);
}
--
2.17.1
^ permalink raw reply related
* [PATCH RFC net-next 3/3] net: phy: Default MARVELL_PHY to the value of SFP
From: Florian Fainelli @ 2018-11-06 23:29 UTC (permalink / raw)
To: netdev; +Cc: andrew, rmk+kernel, davem, Florian Fainelli
In-Reply-To: <20181106232913.17216-1-f.fainelli@gmail.com>
Marvell PHYs are typically found in 1000BaseT SFP modules, so give a
chance for users to get the correct PHY driver when using SFP modules.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3d187cd50eb0..cf7d44ba20c5 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -350,6 +350,7 @@ config LXT_PHY
config MARVELL_PHY
tristate "Marvell PHYs"
+ default SFP
---help---
Currently has a driver for the 88E1011S
--
2.17.1
^ permalink raw reply related
* Re: [PATCH RFC net-next 0/3] net: phy: sfp: Warn when using generic PHY driver
From: David Miller @ 2018-11-06 23:38 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, rmk+kernel
In-Reply-To: <20181106232913.17216-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 6 Nov 2018 15:29:10 -0800
> This patch series allows warning an user that the generic PHY driver(s)
> are used when a SFP incorporates a PHY (e.g: 1000BaseT SFP) which is
> likely not going to work at all.
>
> Let me know if you would want to do that differently.
Is there ever a possibility that the generic PHY driver could work
in an SFP situation?
If not, yes emit the message but also fail the load and registry too
perhaps?
^ permalink raw reply
* Re: [PATCH net-next 0/3] net: systemport: Unmap queues upon DSA unregister event
From: David Miller @ 2018-11-06 23:40 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, vivien.didelot
In-Reply-To: <20181106231518.16314-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 6 Nov 2018 15:15:15 -0800
> This patch series fixes the unbinding/binding of the bcm_sf2 switch
> driver along with bcmsysport which monitors the switch port queues.
> Because the driver was not processing the DSA_PORT_UNREGISTER event, we
> would not be unmapping switch port/queues, which could cause incorrect
> decisions to be made by the HW (e.g: queue always back-pressured).
Series applied, thanks Florian.
^ permalink raw reply
* Re: [PATCH RFC net-next 0/3] net: phy: sfp: Warn when using generic PHY driver
From: Florian Fainelli @ 2018-11-06 23:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev, andrew, rmk+kernel
In-Reply-To: <20181106.153844.1612363235041286689.davem@davemloft.net>
On 11/6/18 3:38 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 6 Nov 2018 15:29:10 -0800
>
>> This patch series allows warning an user that the generic PHY driver(s)
>> are used when a SFP incorporates a PHY (e.g: 1000BaseT SFP) which is
>> likely not going to work at all.
>>
>> Let me know if you would want to do that differently.
>
> Is there ever a possibility that the generic PHY driver could work
> in an SFP situation?
Given the PHY has to operate in SGMII mode, I doubt it could work
without a specialized driver, Andrew, Russell, would you concur?
>
> If not, yes emit the message but also fail the load and registry too
> perhaps?
>
I was not sure this would be acceptable, but it is definitively an easy
change.
--
Florian
^ permalink raw reply
* Re: [PATCH RFC net-next 0/3] net: phy: sfp: Warn when using generic PHY driver
From: Russell King - ARM Linux @ 2018-11-07 0:03 UTC (permalink / raw)
To: David Miller; +Cc: f.fainelli, netdev, andrew
In-Reply-To: <20181106.153844.1612363235041286689.davem@davemloft.net>
On Tue, Nov 06, 2018 at 03:38:44PM -0800, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 6 Nov 2018 15:29:10 -0800
>
> > This patch series allows warning an user that the generic PHY driver(s)
> > are used when a SFP incorporates a PHY (e.g: 1000BaseT SFP) which is
> > likely not going to work at all.
> >
> > Let me know if you would want to do that differently.
>
> Is there ever a possibility that the generic PHY driver could work
> in an SFP situation?
I don't yet see the reason for Florian's patch series - all the Marvell
88e1111 based modules I have, or have come across in information from
manufacturers self-configure themselves and don't really need the
Marvell 1G PHY driver.
For example, the Source Photonics were offering a range of 1GbaseT
modules with the 88e1111 programmed in different modes, but published
instructions for the register accesses to configure them differently
(eg, SGMII vs 1000base-X interface facing the MAC). Depending on
the module part number determines which mode the PHY has been
programmed to come up in.
So in theory, you don't need any PHY driver for these modules - but
it's useful to have a functional PHY driver to be able to read out
the negotiated flow control results.
I'd like more information from Florian about the reasoning behind
this patch series before it's merged.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH RFC net-next 0/3] net: phy: sfp: Warn when using generic PHY driver
From: Florian Fainelli @ 2018-11-07 0:09 UTC (permalink / raw)
To: Russell King - ARM Linux, David Miller; +Cc: netdev, andrew
In-Reply-To: <20181107000322.GP30658@n2100.armlinux.org.uk>
On 11/6/18 4:03 PM, Russell King - ARM Linux wrote:
> On Tue, Nov 06, 2018 at 03:38:44PM -0800, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Tue, 6 Nov 2018 15:29:10 -0800
>>
>>> This patch series allows warning an user that the generic PHY driver(s)
>>> are used when a SFP incorporates a PHY (e.g: 1000BaseT SFP) which is
>>> likely not going to work at all.
>>>
>>> Let me know if you would want to do that differently.
>>
>> Is there ever a possibility that the generic PHY driver could work
>> in an SFP situation?
>
> I don't yet see the reason for Florian's patch series - all the Marvell
> 88e1111 based modules I have, or have come across in information from
> manufacturers self-configure themselves and don't really need the
> Marvell 1G PHY driver.
>
> For example, the Source Photonics were offering a range of 1GbaseT
> modules with the 88e1111 programmed in different modes, but published
> instructions for the register accesses to configure them differently
> (eg, SGMII vs 1000base-X interface facing the MAC). Depending on
> the module part number determines which mode the PHY has been
> programmed to come up in.
>
> So in theory, you don't need any PHY driver for these modules - but
> it's useful to have a functional PHY driver to be able to read out
> the negotiated flow control results.
>
> I'd like more information from Florian about the reasoning behind
> this patch series before it's merged.
>
The module that I am using [1] would not work, as in , no link up being
reported without turning on the Marvell PHY driver:
https://www.amazon.com/dp/B01LW2P72V/ref=twister_B07F3WQJQX?_encoding=UTF8&psc=1
this module uses a 88E1111 PHY as well (OUI: 0x01410cc2).
--
Florian
^ permalink raw reply
* Re: [RFC perf,bpf 5/5] perf util: generate bpf_prog_info_event for short living bpf programs
From: David Ahern @ 2018-11-07 0:23 UTC (permalink / raw)
To: Alexei Starovoitov, David Miller
Cc: Song Liu, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Kernel Team, ast@kernel.org, daniel@iogearbox.net,
peterz@infradead.org, acme@kernel.org
In-Reply-To: <39fe6abc-5c3e-bac3-0c0b-cf68bea23ab0@fb.com>
On 11/6/18 5:13 PM, Alexei Starovoitov wrote:
> On 11/6/18 3:36 PM, David Miller wrote:
>> From: Alexei Starovoitov <ast@fb.com>
>> Date: Tue, 6 Nov 2018 23:29:07 +0000
>>
>>> I think concerns with perf overhead from collecting bpf events
>>> are unfounded.
>>> I would prefer for this flag to be on by default.
>>
>> I will sit in userspace looping over bpf load/unload and see how the
>> person trying to monitor something else with perf feels about that.
>>
>> Really, it is inappropriate to turn this on by default, I completely
>> agree with David Ahern.
>>
>> It's hard enough, _AS IS_, for me to fight back all of the bloat that
>> is in perf right now and get it back to being able to handle simple
>> full workloads without dropping events..
>
> It's a separate perf thread and separate event with its own epoll.
> I don't see how it can affect main event collection.
> Let's put it this way. If it does affect somehow, then yes,
> it should not be on. If it is not, there is no downside to keep it on.
> Typical user expects to type 'perf record' and see everything that
> is happening on the system. Right now short lived bpf programs
> will not be seen. How user suppose to even know when to use the flag?
The default is profiling where perf record collects task events and
periodic samples. So for the default record/report, the bpf load /
unload events are not relevant.
> The only option is to always pass the flag 'just in case'
> which is unnecessary burden.
People who care about an event enable the event collection of the event.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf_load: add map name to load_maps error message
From: Song Liu @ 2018-11-07 0:26 UTC (permalink / raw)
To: John Fastabend
Cc: shannon.nelson, Alexei Starovoitov, Daniel Borkmann, Networking,
shannon.lee.nelson
In-Reply-To: <ad307fb7-0ea3-3929-1dfe-38dbf281e206@gmail.com>
On Mon, Oct 29, 2018 at 3:12 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> On 10/29/2018 02:14 PM, Shannon Nelson wrote:
> > To help when debugging bpf/xdp load issues, have the load_map()
> > error message include the number and name of the map that
> > failed.
> >
> > Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> > ---
> > samples/bpf/bpf_load.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> > index 89161c9..5de0357 100644
> > --- a/samples/bpf/bpf_load.c
> > +++ b/samples/bpf/bpf_load.c
> > @@ -282,8 +282,8 @@ static int load_maps(struct bpf_map_data *maps, int nr_maps,
> > numa_node);
> > }
> > if (map_fd[i] < 0) {
> > - printf("failed to create a map: %d %s\n",
> > - errno, strerror(errno));
> > + printf("failed to create map %d (%s): %d %s\n",
> > + i, maps[i].name, errno, strerror(errno));
> > return 1;
> > }
> > maps[i].fd = map_fd[i];
> >
>
> LGTM
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
^ 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