* Re: [PATCH net-next 3/3] net: sysfs: Document PHY device sysfs attributes
From: Andrew Lunn @ 2017-05-24 20:10 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem
In-Reply-To: <20170524193354.10739-4-f.fainelli@gmail.com>
> +What: /sys/class/mdio_bus/<bus>/<device>/phy_interface
> +Date: February 2014
> +KernelVersion: 3.15
> +Contact: netdev@vger.kernel.org
> +Description:
> + String value indicating the PHY interface, possible
> + values are in include/linux/phy.h function phy_modes.
Hi Florian
Does this suggest that these strings should be moved to
include/uapi/linux/phy.h?
Andrew
^ permalink raw reply
* Re: [PATCH net 1/3] bpf: fix incorrect pruning decision when alignment must be tracked
From: Daniel Borkmann @ 2017-05-24 20:17 UTC (permalink / raw)
To: David Miller; +Cc: ast, netdev
In-Reply-To: <20170524.160733.216900339299848648.davem@davemloft.net>
On 05/24/2017 10:07 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Tue, 23 May 2017 18:30:41 +0200
>
>> + if (!env->strict_alignment && old->off <= cur->off &&
>
> You can't just test env->strict_alignment by itself, that's just an
> override and doesn't determine the actual "strict" value we use which
> is a combination of env->strict_alignment and
> "!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS".
>
> So you'll have to update this test.
Argh, good point, true. Will respin with a v2.
^ permalink raw reply
* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
From: Jiri Pirko @ 2017-05-24 20:23 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <CAM_iQpV7i1Z6rJke8Zr_j1MSxA2R696+kXvcU7a72LLFxKe87Q@mail.gmail.com>
Wed, May 24, 2017 at 05:53:42PM CEST, xiyou.wangcong@gmail.com wrote:
>On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@gmail.com wrote:
>>>tcf_chain_get() always creates a new filter chain if not found
>>>in existing ones. This is totally unnecessary when we get or
>>>delete filters, new chain should be only created for new filters
>>>(or new actions).
>>>
>>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>>Cc: Jiri Pirko <jiri@mellanox.com>
>>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>---
>>> include/net/pkt_cls.h | 3 ++-
>>> net/sched/act_api.c | 2 +-
>>> net/sched/cls_api.c | 13 +++++++++----
>>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>>index 2c213a6..f776229 100644
>>>--- a/include/net/pkt_cls.h
>>>+++ b/include/net/pkt_cls.h
>>>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
>>> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>>>
>>> #ifdef CONFIG_NET_CLS
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+ bool create);
>>> void tcf_chain_put(struct tcf_chain *chain);
>>> int tcf_block_get(struct tcf_block **p_block,
>>> struct tcf_proto __rcu **p_filter_chain);
>>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>>index 0ecf2a8..aed6cf2 100644
>>>--- a/net/sched/act_api.c
>>>+++ b/net/sched/act_api.c
>>>@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
>>>
>>> if (!tp)
>>> return -EINVAL;
>>>- a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
>>>+ a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
>>> if (!a->goto_chain)
>>> return -ENOMEM;
>>> return 0;
>>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>>index 01a8b8b..23d2236 100644
>>>--- a/net/sched/cls_api.c
>>>+++ b/net/sched/cls_api.c
>>>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
>>> kfree(chain);
>>> }
>>>
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+ bool create)
>>> {
>>> struct tcf_chain *chain;
>>>
>>>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>> return chain;
>>> }
>>> }
>>>- return tcf_chain_create(block, chain_index);
>>>+ if (create)
>>>+ return tcf_chain_create(block, chain_index);
>>>+ else
>>>+ return NULL;
>>> }
>>> EXPORT_SYMBOL(tcf_chain_get);
>>>
>>>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>> err = -EINVAL;
>>> goto errout;
>>> }
>>>- chain = tcf_chain_get(block, chain_index);
>>>+ chain = tcf_chain_get(block, chain_index,
>>>+ n->nlmsg_type == RTM_NEWTFILTER);
>>
>> First of all, I really hate all these true/false arg dances. Totaly
>> confusing all the time.
>
>Sounds like you are able to understand the code at all.
>Sigh, I bet you never even read the changelog. ;)
>
>
>>
>>
>>
>>> if (!chain) {
>>>- err = -ENOMEM;
>>>+ err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>>
>> Confusing. Please do not obfuscate the code for a corner cases. Thanks.
>
>Either you don't understand the changelog or you don't understand
>ternary conditional operator. I can't help you if the latter.
>
>Sorry.
Heh. Really? All I say is, your patch is not needed at all. All it adds
makes to code harder to understand and no benefit.
^ permalink raw reply
* Re: [patch net-next v2 0/5] add tcp flags match support to flower and offload it
From: David Miller @ 2017-05-24 20:24 UTC (permalink / raw)
To: jiri; +Cc: netdev, jhs, xiyou.wangcong, simon.horman, mlxsw, idosch
In-Reply-To: <20170523164048.16514-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 23 May 2017 18:40:43 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> This patch adds support to dissect tcp flags, match on them using
> flower classifier and offload such rules to mlxsw Spectrum devices.
>
> ---
> v1->v2:
> - removed no longer relevant comment from patch 1 as suggested by Or
> - sent correct patches this time
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH V3 net 0/3] Fix checksum issues with Q-in-Q vlans
From: David Miller @ 2017-05-24 20:25 UTC (permalink / raw)
To: vyasevich; +Cc: netdev, alexander.duyck, makita.toshiaki, vyasevic
In-Reply-To: <1495561123-1819-1-git-send-email-vyasevic@redhat.com>
From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Tue, 23 May 2017 13:38:40 -0400
> TCP checksum appear broken on a lot of devices that
> advertise NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM. This problem
> becomes very visible/reproducable since the series
> commit afb0bc972b526 ("Merge branch 'stacked_vlan_tso'").
>
> In particular, the issue appeared consistently on bnx2 and be2net
> drivers (not all drivers were tested).
>
> This short series corrects this by disabling checksum offload
> support on packets sent through Q-in-Q vlans if the underlying HW only
> enables IP specific checksum features. We currently 'assume' that
> any drivers setting NETIF_F_HW_CSUM can correclty pass checksum offsets
> to HW. It is up to individual drivers to enable it properly through
> ndo_features_check if they have some support for Q-in-Q vlans.
>
> Additionally, be2net driver was fixed to make the proper call.
>
> While looking at the drivers, it was also found that virtio-net ended
> up disabling accelerations, which is unnecessary.
>
> V3: Fixed checkpatch errors.
>
> V2: Instead of disabling checksuming for all devices, only devices using
> NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM are now affected by this change.
> For drivers using NETIF_F_HW_CSUM, we will continue to use checksum
> offloading. If any drivers are found to be broken, they would need
> be fixed individually.
Series applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: support cross-chip ageing time
From: David Miller @ 2017-05-24 20:28 UTC (permalink / raw)
To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20170523192059.1720-1-vivien.didelot@savoirfairelinux.com>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 23 May 2017 15:20:59 -0400
> Now that the switchdev bridge ageing time attribute is propagated to all
> switch chips of the fabric, each switch can check if the requested value
> is valid and program itself, so that the whole fabric shares a common
> ageing time setting.
>
> This is especially needed for switch chips in between others, containing
> no bridge port members but evidently used in the data path.
>
> To achieve that, remove the condition which skips the other switches. We
> also don't need to identify the target switch anymore, thus remove the
> sw_index member of the dsa_notifier_ageing_time_info notifier structure.
>
> On ZII Dev Rev B (with two 88E6352 and one 88E6185) and ZII Dev Rev C
> (with two 88E6390X), we have the following hardware configuration:
...
> Before this patch:
...
> After this patch:
...
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] tcp: fix TCP_SYNCNT flakes
From: David Miller @ 2017-05-24 20:30 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, soheil, ycheng
In-Reply-To: <1495568315.6465.71.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 23 May 2017 12:38:35 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> After the mentioned commit, some of our packetdrill tests became flaky.
>
> TCP_SYNCNT socket option can limit the number of SYN retransmits.
>
> retransmits_timed_out() has to compare times computations based on
> local_clock() while timers are based on jiffies. With NTP adjustments
> and roundings we can observe 999 ms delay for 1000 ms timers.
> We end up sending one extra SYN packet.
>
> Gimmick added in commit 6fa12c850314 ("Revert Backoff [v3]: Calculate
> TCP's connection close threshold as a time value") makes no
> real sense for TCP_SYN_SENT sockets where no RTO backoff can happen at
> all.
>
> Lets use a simpler logic for TCP_SYN_SENT sockets and remove @syn_set
> parameter from retransmits_timed_out()
>
> Fixes: 9a568de4818d ("tcp: switch TCP TS option (RFC 7323) to 1ms clock")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [patch net-next v2 0/8] mlxsw: Support firmware flash
From: David Miller @ 2017-05-24 20:33 UTC (permalink / raw)
To: jiri; +Cc: netdev, idosch, yotamg, mlxsw, Yuval.Mintz
In-Reply-To: <20170523195630.6460-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 23 May 2017 21:56:22 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> Add support for device firmware flash on mlxsw spectrum. The firmware files
> are expected to be in the Mellanox Firmware Archive version 2 (MFA2)
> format.
>
> The firmware flash is triggered on driver initialization time if the device
> firmware version does not meet the minimum firmware version supported by
> the driver.
>
> Currently, to activate the newly flashed firmware, the user needs to
> reboot his system.
>
> The first patch introduces the mlxfw module, which implements common logic
> needed for the firmware flash process on Mellanox products, such as the
> MFA2 format parsing and the firmware flash state machine logic. As the
> module implements common logic which will be needed by various different
> Mellanox drivers, it defines a set of callbacks needed to interact with the
> specific device.
>
> Patches 1-5 implement the needed mlxfw callbacks in the mlxsw spectrum
> driver.
>
> Patches 6 and 7 add boot-time firmware upgrade on the mlxsw spectrum
> driver.
>
> Patch 8 adds a fix needed for new firmware versions.
Series applied, although I hope you sort out the user interface
soon otherwise most of this is completely dead unused code.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
From: Jason A. Donenfeld @ 2017-05-24 20:39 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang
In-Reply-To: <decd478f-0d5d-af6a-5b59-b710658ea16b@cogentembedded.com>
On Wed, May 24, 2017 at 6:41 PM, Sergei Shtylyov
> I've only looked on the last 2 patches. You can add my:
>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> if you want. :-)
Will do. For the series, or just for 5/5?
^ permalink raw reply
* Re: [PATCH v6 net-next 01/17] net: qualcomm: remove unnecessary includes
From: David Miller @ 2017-05-24 20:42 UTC (permalink / raw)
To: stefan.wahren
Cc: linux-serial, jslaby, gregkh, netdev, robh+dt, linux-kernel,
kubakici, mark.rutland, LinoSanfilippo, devicetree
In-Reply-To: <1746915556.279475.1495656327010@email.1und1.de>
From: Stefan Wahren <stefan.wahren@i2se.com>
Date: Wed, 24 May 2017 22:05:26 +0200 (CEST)
> AFAIK these ones above aren't necessary (no init, no kernel module,
> no kernel parameter, no kernel version) for this C file. So i will
> double check it.
You need the endianness translators like cpu_to_be32() or whatever,
so you need to figure out where you are getting that once these
explicit headers are removed.
And see, it's probably hidden inside of the private header's includes.
So we can't tell.
^ permalink raw reply
* Re: [PATCH net-next v9 5/5] virtio_net: check return value of skb_to_sgvec always
From: Sergei Shtylyov @ 2017-05-24 20:46 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Netdev, LKML, David Miller, Michael S. Tsirkin, Jason Wang
In-Reply-To: <CAHmME9obros-WAMC69VWrg=Djb_ZwKz9WyBVvKkAMHmC3N87SA@mail.gmail.com>
On 05/24/2017 11:39 PM, Jason A. Donenfeld wrote:
>> I've only looked on the last 2 patches. You can add my:
>>
>> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> if you want. :-)
>
> Will do. For the series, or just for 5/5?
5/5 only. :-)
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-24 20:57 UTC (permalink / raw)
To: Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, f.fainelli, David Miller,
Manoj Iyer, jhugo
In-Reply-To: <20170524193450.GC1788@lunn.ch>
On 05/24/2017 02:34 PM, Andrew Lunn wrote:
>> Ok, I'm going to debug this some more. It turns out that the MAC side of
>> the SGMII link can send an interrupt when it thinks that auto-negotiation is
>> done. I might be able to use this.
>
> You can use this for your board. But it still leaves the phy driver
> broken for everybody else.
Wait, I thought you said the at803x driver was not broken, since it
returns 0 when the SGMII side of the link hasn't finished auto-negotiating?
>> What function should my MAC driver call when it wants the phy core to call
>> at803x_aneg_done again to see if autonegotiation is done?
>
> You want to trigger the PHY state machine. There is only one exported
> API call to do this, phy_mac_interrupt(). But you are supposed to pass
> the new link state. And your MAC driver has no idea of that, it does
> not know if the copper side of the PHY is up.
My NIC has a feature called autopolling where it takes over the MDIO bus
and regularly polls the link state. When it detects that the link state
has changed, it generates a MAC interrupt. This is when I call
phy_mac_interrupt() normally.
I think I can use the SGMII interrupt to also call phy_mac_interrupt().
The problem is the from the copper side, the link is already up, so if I
call phy_mac_interrupt() again and say the link is up, the phy core is
going to ignore that.
> So it might be better if you export phy_trigger_machine().
I'll test that, but that does seem a bit hackish.
>> Also, is there a way for the MAC driver to know that at803x_aneg_done()
>> previously returned 0, and that it needs to tell the phy core to check again?
>
> Not that i know of. The MAC layer is not supposed to be messing around
> in the PHY layer. However, just triggering the PHY state machine
> should be enough.
Can you tell my how PHY_HAS_INTERRUPT is supposed to work? How does the
PHY send an interrupt?
I'm starting to think that my NIC's autopolling feature is not
compatible with phylib, and that I should use polling mode.
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Andrew Lunn @ 2017-05-24 21:15 UTC (permalink / raw)
To: Timur Tabi
Cc: Matthias May, Zefir Kurtisi, netdev, f.fainelli, David Miller,
Manoj Iyer, jhugo
In-Reply-To: <d6a23db1-2465-b315-f520-63aba0341fbe@quicinc.com>
On Wed, May 24, 2017 at 03:57:06PM -0500, Timur Tabi wrote:
> On 05/24/2017 02:34 PM, Andrew Lunn wrote:
> >>Ok, I'm going to debug this some more. It turns out that the MAC side of
> >>the SGMII link can send an interrupt when it thinks that auto-negotiation is
> >>done. I might be able to use this.
> >
> >You can use this for your board. But it still leaves the phy driver
> >broken for everybody else.
>
> Wait, I thought you said the at803x driver was not broken, since it
> returns 0 when the SGMII side of the link hasn't finished
> auto-negotiating?
It is correct so far. But to work, it needs to interrupt again once
the SGMII side has come up. Only then have we link.
> My NIC has a feature called autopolling where it takes over the MDIO
> bus and regularly polls the link state. When it detects that the
> link state has changed, it generates a MAC interrupt. This is when
> I call phy_mac_interrupt() normally.
Unfortunately, you need to keep this feature turned off. It will not
respect the phydev mutex. It has no idea what page has been currently
selected. It probably has no way to flip the page and see if the SGMII
link is up. etc.
> Can you tell my how PHY_HAS_INTERRUPT is supposed to work? How does
> the PHY send an interrupt?
Generally, the PHY interrupt pin is connected to a GPIO. You then use
the GPIO as an interrupt source. So it has an interrupt number. Put
that in phydev->irq, eg using the interrupts property in device tree.
The core will register an interrupt handler, and enable the
interrupt. When it receives an interrupt, it calls the phy driver to
service the interrupt.
Andrew
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Florian Fainelli @ 2017-05-24 21:19 UTC (permalink / raw)
To: Timur Tabi, Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <d6a23db1-2465-b315-f520-63aba0341fbe@quicinc.com>
On 05/24/2017 01:57 PM, Timur Tabi wrote:
> On 05/24/2017 02:34 PM, Andrew Lunn wrote:
>>> Ok, I'm going to debug this some more. It turns out that the MAC
>>> side of
>>> the SGMII link can send an interrupt when it thinks that
>>> auto-negotiation is
>>> done. I might be able to use this.
>>
>> You can use this for your board. But it still leaves the phy driver
>> broken for everybody else.
>
> Wait, I thought you said the at803x driver was not broken, since it
> returns 0 when the SGMII side of the link hasn't finished auto-negotiating?
>
>>> What function should my MAC driver call when it wants the phy core to
>>> call
>>> at803x_aneg_done again to see if autonegotiation is done?
>>
>> You want to trigger the PHY state machine. There is only one exported
>> API call to do this, phy_mac_interrupt(). But you are supposed to pass
>> the new link state. And your MAC driver has no idea of that, it does
>> not know if the copper side of the PHY is up.
>
> My NIC has a feature called autopolling where it takes over the MDIO bus
> and regularly polls the link state. When it detects that the link state
> has changed, it generates a MAC interrupt. This is when I call
> phy_mac_interrupt() normally.
>
> I think I can use the SGMII interrupt to also call phy_mac_interrupt().
> The problem is the from the copper side, the link is already up, so if I
> call phy_mac_interrupt() again and say the link is up, the phy core is
> going to ignore that.
The question is what the HW is auto-polling on? Most HW implementations
that I have seen either auto-poll and assume that BMSR.LINKSTATUS will
reflect what they want to, or they even let you specify which register
and bit(s) to monitor for link status. So which one is it here?
As Andrew already responded, this clashes with any reads/writes that the
PHY library could do, unless your HW is smart enough to serialize MDIO
reads/writes?
>
>> So it might be better if you export phy_trigger_machine().
>
> I'll test that, but that does seem a bit hackish.
>
>>> Also, is there a way for the MAC driver to know that at803x_aneg_done()
>>> previously returned 0, and that it needs to tell the phy core to
>>> check again?
>>
>> Not that i know of. The MAC layer is not supposed to be messing around
>> in the PHY layer. However, just triggering the PHY state machine
>> should be enough.
>
> Can you tell my how PHY_HAS_INTERRUPT is supposed to work? How does the
> PHY send an interrupt?
PHY_HAS_INTERRUPT indicates that you have a dedicated interrupt line for
your PHY device and that you have the ability to implement a custom
interrupt handling callback in the PHY driver (ack_interrupt) that lets
you deal with all possible sorts of the interrupts that the PHY could
generate.
>
> I'm starting to think that my NIC's autopolling feature is not
> compatible with phylib, and that I should use polling mode.
That's pretty much what Andrew told you about 5 emails ago...
--
Florian
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-24 21:20 UTC (permalink / raw)
To: Andrew Lunn, Timur Tabi
Cc: Matthias May, Zefir Kurtisi, netdev, f.fainelli, David Miller,
Manoj Iyer, jhugo
In-Reply-To: <20170524211520.GJ1788@lunn.ch>
On 05/24/2017 04:15 PM, Andrew Lunn wrote:
>> My NIC has a feature called autopolling where it takes over the MDIO
>> bus and regularly polls the link state. When it detects that the
>> link state has changed, it generates a MAC interrupt. This is when
>> I call phy_mac_interrupt() normally.
> Unfortunately, you need to keep this feature turned off. It will not
> respect the phydev mutex. It has no idea what page has been currently
> selected. It probably has no way to flip the page and see if the SGMII
> link is up. etc.
phydev mutex? And what do you mean by page?
I forgot one detail. Every time you do an MDIO read/write, it
temporarily disables the feature. Although, I think that's not relevant
to your point.
Disabling this feature and switching from PHY_IGNORE_INTERRUPT to
PHY_POLL might fix everything. I will try it.
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Florian Fainelli @ 2017-05-24 21:28 UTC (permalink / raw)
To: Timur Tabi, Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <45f80817-ec17-51c0-3816-f7c93e6aeeb9@quicinc.com>
On 05/24/2017 02:20 PM, Timur Tabi wrote:
> On 05/24/2017 04:15 PM, Andrew Lunn wrote:
>>> My NIC has a feature called autopolling where it takes over the MDIO
>>> bus and regularly polls the link state. When it detects that the
>>> link state has changed, it generates a MAC interrupt. This is when
>>> I call phy_mac_interrupt() normally.
>
>> Unfortunately, you need to keep this feature turned off. It will not
>> respect the phydev mutex. It has no idea what page has been currently
>> selected. It probably has no way to flip the page and see if the SGMII
>> link is up. etc.
>
> phydev mutex? And what do you mean by page?
Yes phydev->lock which is used to serialize the state machine state changes.
Most PHYs have many more registers than the 15 standard exposed
directly, and so you need indirect reads/writes to access these
registers, which typically involve switching a particular page, doing
the indirect register access, and then flipping the page back. If you
interrupt that scheme one way or another, your reads and writes are all
messed up.
>
> I forgot one detail. Every time you do an MDIO read/write, it
> temporarily disables the feature. Although, I think that's not relevant
> to your point.
Is that done by the HW itself, or is this under SW control exclusively.
>
> Disabling this feature and switching from PHY_IGNORE_INTERRUPT to
> PHY_POLL might fix everything. I will try it.
>
Humm yes, that seems like a worthwhile exercise at least.
--
Florian
^ permalink raw reply
* Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP
From: Ihar Hrachyshka @ 2017-05-24 21:32 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, ja, Networking
In-Reply-To: <CAK8P3a162_RdXHLCPzg47+F7yadRGMDgFG_z5R6v65-heY1KAw@mail.gmail.com>
On 05/23/2017 01:56 PM, Arnd Bergmann wrote:
> This seems to have caused a build warning:
>
> net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>
Not sure. How do you reproduce it? I just did 'make net' in the latest
tree that includes the patch, and it doesn't trigger the failure. Also
the code logic prevents it to be uninitialized (it's always set to -1 or
the actual type value).
Please advise,
Ihar
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-24 21:32 UTC (permalink / raw)
To: Florian Fainelli, Timur Tabi, Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <27f6d81d-50b1-a585-e251-2303957130d9@gmail.com>
On 05/24/2017 04:28 PM, Florian Fainelli wrote:
> Yes phydev->lock which is used to serialize the state machine state changes.
>
> Most PHYs have many more registers than the 15 standard exposed
> directly, and so you need indirect reads/writes to access these
> registers, which typically involve switching a particular page, doing
> the indirect register access, and then flipping the page back. If you
> interrupt that scheme one way or another, your reads and writes are all
> messed up.
Ah, and the at803x is a device like that.
At worst, the autopoll feature could read a register from the wrong
page, and think that the link state has changed when it hasn't. But
that's still bad, and all my problems do revolve around link states.
>> I forgot one detail. Every time you do an MDIO read/write, it
>> temporarily disables the feature. Although, I think that's not relevant
>> to your point.
>
> Is that done by the HW itself, or is this under SW control exclusively.
Software.
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Florian Fainelli @ 2017-05-24 21:36 UTC (permalink / raw)
To: Timur Tabi, Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <bde05752-8dcf-9c69-0677-39a12e67edeb@quicinc.com>
On 05/24/2017 02:32 PM, Timur Tabi wrote:
> On 05/24/2017 04:28 PM, Florian Fainelli wrote:
>
>> Yes phydev->lock which is used to serialize the state machine state
>> changes.
>>
>> Most PHYs have many more registers than the 15 standard exposed
>> directly, and so you need indirect reads/writes to access these
>> registers, which typically involve switching a particular page, doing
>> the indirect register access, and then flipping the page back. If you
>> interrupt that scheme one way or another, your reads and writes are all
>> messed up.
>
> Ah, and the at803x is a device like that.
>
> At worst, the autopoll feature could read a register from the wrong
> page, and think that the link state has changed when it hasn't. But
> that's still bad, and all my problems do revolve around link states.
Absolutely, just like it's not clear whether autopoll does read
BMSR.LINKSTAT or another at803x specific register? Also how is
BMSR.LINKSTAT defined on at803x when there is SGMII + Copper involved?
>
>>> I forgot one detail. Every time you do an MDIO read/write, it
>>> temporarily disables the feature. Although, I think that's not relevant
>>> to your point.
>>
>> Is that done by the HW itself, or is this under SW control exclusively.
>
> Software.
OK, and there is no way you can run into the following race condition:
CPU HW
MDIO read intent
polling starts
disable HW autopoll
polling continues
MDIO read is done
MDIO read done
polling stops
MDIO read value returned
if you disable autopolling in HW this is guaranteed to immediately stop
by the time the register value is seen in HW and your I/O read/write
returns?
--
Florian
^ permalink raw reply
* Re: [PATCH v2 2/4] arp: decompose is_garp logic into a separate function
From: Ihar Hrachyshka @ 2017-05-24 21:38 UTC (permalink / raw)
To: Julian Anastasov; +Cc: davem, netdev
In-Reply-To: <alpine.LFD.2.20.1705182342140.4645@ja.home.ssi.bg>
On 05/18/2017 01:49 PM, Julian Anastasov wrote:
> 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);
I can easily miss something substantial, so please correct me, but...
If it's of REPLY type, the RFC 2002 requires that target hardware
address field equals to source address field for a packet to be
considered gratuitous. Since IEEE 1394 ARP standard defines its payload
without target field, it seems to me that there is no such thing as a
gratuitous ARP reply for IEEE 1394. That's why I think resetting is_garp
to 0 for those packets is justified.
Ihar
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH 2/4] [next-queue]net: i40e: Add infrastructure for queue channel support with the TCs and queue configurations offloaded via mqprio scheduler
From: Alexander Duyck @ 2017-05-24 21:45 UTC (permalink / raw)
To: Amritha Nambiar; +Cc: intel-wired-lan, Netdev
In-Reply-To: <149524190808.11022.3222127507844771494.stgit@anamdev.jf.intel.com>
On Fri, May 19, 2017 at 5:58 PM, Amritha Nambiar
<amritha.nambiar@intel.com> wrote:
> This patch sets up the infrastructure for offloading TCs and
> queue configurations to the hardware by creating HW channels(VSI).
> A new channel is created for each of the traffic class
> configuration offloaded via mqprio framework except for the first TC
> (TC0). TC0 for the main VSI is also reconfigured as per user provided
> queue parameters. Queue counts that are not power-of-2 are handled by
> reconfiguring RSS by reprogramming LUTs using the queue count value.
> This patch also handles configuring the TX rings for the channels,
> setting up the RX queue map for channel.
>
> Also, the channels so created are removed and all the queue
> configuration is set to default when the qdisc is detached from the
> root of the device.
>
> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 36 +
> drivers/net/ethernet/intel/i40e/i40e_main.c | 740 +++++++++++++++++++++++++++
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 2
> 3 files changed, 771 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 395ca94..0915b02 100644
[...]
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 8d1d3b85..e1bea45 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
[...]
> +/**
> + * i40e_create_queue_channel - function to create channel
> + * @vsi: VSI to be configured
> + * @ch: ptr to channel (it contains channel specific params)
> + *
> + * This function creates channel (VSI) using num_queues specified by user,
> + * reconfigs RSS if needed.
> + **/
> +int i40e_create_queue_channel(struct i40e_vsi *vsi,
> + struct i40e_channel *ch)
> +{
> + struct i40e_pf *pf = vsi->back;
> + bool reconfig_rss;
> + int err;
> +
> + if (!ch)
> + return -EINVAL;
> +
> + if (!ch->num_queue_pairs) {
> + dev_err(&pf->pdev->dev, "Invalid num_queues requested: %d\n",
> + ch->num_queue_pairs);
> + return -EINVAL;
> + }
> +
> + /* validate user requested num_queues for channel */
> + err = i40e_validate_num_queues(pf, ch->num_queue_pairs, vsi,
> + &reconfig_rss);
> + if (err) {
> + dev_info(&pf->pdev->dev, "Failed to validate num_queues (%d)\n",
> + ch->num_queue_pairs);
> + return -EINVAL;
> + }
> +
> + /* By default we are in VEPA mode, if this is the first VF/VMDq
> + * VSI to be added switch to VEB mode.
> + */
> + if ((!(pf->flags & I40E_FLAG_VEB_MODE_ENABLED)) ||
> + (!i40e_is_any_channel(vsi))) {
> + if (!is_power_of_2(vsi->tc_config.tc_info[0].qcount)) {
> + dev_info(&pf->pdev->dev,
> + "Failed to create channel. Override queues (%u) not power of 2\n",
> + vsi->tc_config.tc_info[0].qcount);
> + return -EINVAL;
> + }
> +
> + if (vsi->type == I40E_VSI_SRIOV) {
> + if (!(pf->flags & I40E_FLAG_VEB_MODE_ENABLED)) {
> + dev_info(&pf->pdev->dev,
> + "Expected to be VEB mode by this time\n");
> + return -EINVAL;
> + }
> + }
> + if (!(pf->flags & I40E_FLAG_VEB_MODE_ENABLED)) {
> + pf->flags |= I40E_FLAG_VEB_MODE_ENABLED;
> +
> + if (vsi->type == I40E_VSI_MAIN) {
> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
> + i40e_do_reset(pf,
> + BIT_ULL(__I40E_PF_RESET_REQUESTED),
> + true);
> + else
> + i40e_do_reset_safe(pf,
> + BIT_ULL(__I40E_PF_RESET_REQUESTED));
So these BIT_ULL lines are triggering a check in checkpatch, and I
have to say I don't really like this as it really is messed up in
terms of formatting.
If nothing else you might want to look at defining a macro that
replaces the line. That way you could still represent the same data
without having to resort to misaligning things to make it under 80
characters.
^ permalink raw reply
* Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP
From: Arnd Bergmann @ 2017-05-24 21:57 UTC (permalink / raw)
To: Ihar Hrachyshka; +Cc: David Miller, ja, Networking
In-Reply-To: <23f8a3fa-3e7a-339a-2deb-76d20fe6483b@redhat.com>
On Wed, May 24, 2017 at 11:32 PM, Ihar Hrachyshka <ihrachys@redhat.com> wrote:
> On 05/23/2017 01:56 PM, Arnd Bergmann wrote:
>>
>> This seems to have caused a build warning:
>>
>> net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
>> in this function [-Wmaybe-uninitialized]
>>
> Not sure. How do you reproduce it? I just did 'make net' in the latest tree
> that includes the patch, and it doesn't trigger the failure. Also the code
> logic prevents it to be uninitialized (it's always set to -1 or the actual
> type value).
I saw it reported by the kernelci build bot:
https://kernelci.org/build/id/5925b81859b514fb106b9590/logs/
It happened on arm64 with 'make allmodconfig', but none of the other
architectures or the other configurations on that architecture.
This kind of warning can be a real pain to shut up when the compiler
gets it wrong. I haven't tested the latest kernel with your patch on my
own build box yet, so I also haven't been able to reproduce it.
Most likely, gcc gets confused when it needs to track the state of
too many variables here (I certainly get confused when I read it too).
My first attempt to resolve it would be:
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ae96e6f3e0cb..39f26980a565 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -663,6 +663,8 @@ static bool arp_is_garp(struct net *net, struct
net_device *dev,
*addr_type = inet_addr_type_dev_table(net, dev, sip);
if (*addr_type != RTN_UNICAST)
is_garp = false;
+ } else {
+ *addr_type = -1;
}
return is_garp;
}
@@ -864,7 +866,6 @@ static int arp_process(struct net *net, struct
sock *sk, struct sk_buff *skb)
n = __neigh_lookup(&arp_tbl, &sip, dev, 0);
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);
}
but this clearly needs a lot of build testing to see if it's sufficient. Moving
the initialization of addr_type=1 to the start of the function would
certainly avoid the warning, but I suspect this would be incorrect here.
Arnd
^ permalink raw reply related
* Re: [Intel-wired-lan] [PATCH 1/4] [next-queue]net: mqprio: Introduce new hardware offload mode in mqprio for offloading full TC configurations
From: Alexander Duyck @ 2017-05-24 21:59 UTC (permalink / raw)
To: Amritha Nambiar; +Cc: intel-wired-lan, Netdev
In-Reply-To: <149524190301.11022.16999010740843369111.stgit@anamdev.jf.intel.com>
On Fri, May 19, 2017 at 5:58 PM, Amritha Nambiar
<amritha.nambiar@intel.com> wrote:
> This patch introduces a new hardware offload mode in mqprio
> which makes full use of the mqprio options, the TCs, the
> queue configurations and the bandwidth rates for the TCs.
> This is achieved by setting the value 2 for the "hw" option.
> This new offload mode supports new attributes for traffic
> class such as minimum and maximum values for bandwidth rate limits.
>
> Introduces a new datastructure 'tc_mqprio_qopt_offload' for offloading
> mqprio queue options and use this to be shared between the kernel and
> device driver. This contains a copy of the exisiting datastructure
> for mqprio queue options. This new datastructure can be extended when
> adding new attributes for traffic class such as bandwidth rate limits. The
> existing datastructure for mqprio queue options will be shared between the
> kernel and userspace.
>
> This patch enables configuring additional attributes associated
> with a traffic class such as minimum and maximum bandwidth
> rates and can be offloaded to the hardware in the new offload mode.
> The min and max limits for bandwidth rates are provided
> by the user along with the the TCs and the queue configurations
> when creating the mqprio qdisc.
>
> Example:
> # tc qdisc add dev eth0 root mqprio num_tc 2 map 0 0 0 0 1 1 1 1\
> queues 4@0 4@4 min_rate 0Mbit 0Mbit max_rate 55Mbit 60Mbit hw 2
>
> To dump the bandwidth rates:
>
> # tc qdisc show dev eth0
> qdisc mqprio 804a: root tc 2 map 0 0 0 0 1 1 1 1 0 0 0 0 0 0 0 0
> queues:(0:3) (4:7)
> min rates:0bit 0bit
> max rates:55Mbit 60Mbit
>
> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> ---
> include/linux/netdevice.h | 2
> include/net/pkt_cls.h | 7 ++
> include/uapi/linux/pkt_sched.h | 13 +++
> net/sched/sch_mqprio.c | 169 +++++++++++++++++++++++++++++++++++++---
> 4 files changed, 180 insertions(+), 11 deletions(-)
>
[...]
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 0a4cf27..6457ec9 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -18,10 +18,13 @@
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> #include <net/sch_generic.h>
> +#include <net/pkt_cls.h>
>
> struct mqprio_sched {
> struct Qdisc **qdiscs;
> int hw_offload;
> + u32 flags;
> + u64 min_rate[TC_QOPT_MAX_QUEUE], max_rate[TC_QOPT_MAX_QUEUE];
> };
>
> static void mqprio_destroy(struct Qdisc *sch)
> @@ -39,10 +42,21 @@ static void mqprio_destroy(struct Qdisc *sch)
> }
>
> if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) {
> - struct tc_mqprio_qopt offload = { 0 };
> - struct tc_to_netdev tc = { .type = TC_SETUP_MQPRIO,
> - { .mqprio = &offload } };
> + struct tc_mqprio_qopt_offload offload = { 0 };
So this is currently throwing a warning when I pull these patches and
build this with gcc 6.2. I think the correct setup is "offload = {{ 0
}}" in order to indicate that we are initializing the inner structure
and all other data to 0.
> + struct tc_to_netdev tc = { 0 };
>
> + switch (priv->hw_offload) {
> + case TC_MQPRIO_HW_OFFLOAD_TCS:
> + tc.type = TC_SETUP_MQPRIO;
> + tc.mqprio = &offload.qopt;
> + break;
> + case TC_MQPRIO_HW_OFFLOAD:
> + tc.type = TC_SETUP_MQPRIO_EXT;
> + tc.mqprio_qopt = &offload;
> + break;
> + default:
> + return;
> + }
> dev->netdev_ops->ndo_setup_tc(dev, sch->handle, 0, &tc);
> } else {
> netdev_set_num_tc(dev, 0);
^ permalink raw reply
* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-05-24 22:03 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: Matthias May, Zefir Kurtisi, netdev, David Miller, Manoj Iyer,
jhugo
In-Reply-To: <500e4d46-977b-bb6f-9252-cb6990288a84@gmail.com>
On 05/24/2017 04:36 PM, Florian Fainelli wrote:
> OK, and there is no way you can run into the following race condition:
>
> CPU HW
> MDIO read intent
> polling starts
> disable HW autopoll
> polling continues
Disabling of the HW autopoll waits for the poll to actually stop before
continuing. You can see the code here:
http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-phy.c#L102
> MDIO read is done
> MDIO read done
> polling stops
> MDIO read value returned
>
>
> if you disable autopolling in HW this is guaranteed to immediately stop
> by the time the register value is seen in HW and your I/O read/write
> returns?
It doesn't immediately stop, but the emac_phy_mdio_autopoll_disable()
function waits for the MDIO bus to not be busy. But low-level details
of this feature are not documented, so who knows what it does exactly?
The original code that used this feature only supported one PHY and
never expected there to be any asynchronous MDIO transactios.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH 3/4] [next-queue]net: i40e: Enable mqprio full offload mode in the i40e driver for configuring TCs and queue mapping
From: Alexander Duyck @ 2017-05-24 22:05 UTC (permalink / raw)
To: Amritha Nambiar; +Cc: intel-wired-lan, Netdev
In-Reply-To: <149524191318.11022.8959648104747338042.stgit@anamdev.jf.intel.com>
On Fri, May 19, 2017 at 5:58 PM, Amritha Nambiar
<amritha.nambiar@intel.com> wrote:
> The i40e driver is modified to enable the new mqprio hardware
> offload mode and factor the TCs and queue configuration by
> creating channel VSIs. In this mode, the priority to traffic
> class mapping and the user specified queue ranges are used
> to configure the traffic classes when the 'hw' option is set
> to 2.
>
> Example:
> # tc qdisc add dev eth0 root mqprio num_tc 4\
> map 0 0 0 0 1 2 2 3 queues 2@0 2@2 1@4 1@5 hw 2
>
> # tc qdisc show dev eth0
> qdisc mqprio 8038: root tc 4 map 0 0 0 0 1 2 2 3 0 0 0 0 0 0 0 0
> queues:(0:1) (2:3) (4:4) (5:5)
>
> The HW channels created are removed and all the queue configuration
> is set to default when the qdisc is detached from the root of the
> device.
>
> #tc qdisc del dev eth0 root
>
> This patch also disables setting up channels via ethtool (ethtool -L)
> when the TCs are confgured using mqprio scheduler.
>
> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 4
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 6
> drivers/net/ethernet/intel/i40e/i40e_main.c | 311 ++++++++++++++++++++++--
> 3 files changed, 292 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 0915b02..a62f65a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -54,6 +54,8 @@
> #include <linux/clocksource.h>
> #include <linux/net_tstamp.h>
> #include <linux/ptp_clock_kernel.h>
> +#include <net/pkt_cls.h>
> +
> #include "i40e_type.h"
> #include "i40e_prototype.h"
> #include "i40e_client.h"
> @@ -685,6 +687,7 @@ struct i40e_vsi {
> enum i40e_vsi_type type; /* VSI type, e.g., LAN, FCoE, etc */
> s16 vf_id; /* Virtual function ID for SRIOV VSIs */
>
> + struct tc_mqprio_qopt_offload mqprio_qopt; /* queue parameters */
> struct i40e_tc_configuration tc_config;
> struct i40e_aqc_vsi_properties_data info;
>
> @@ -710,6 +713,7 @@ struct i40e_vsi {
> u16 cnt_q_avail; /* num of queues available for channel usage */
> u16 orig_rss_size;
> u16 current_rss_size;
> + bool reconfig_rss;
>
> /* keeps track of next_base_queue to be used for channel setup */
> atomic_t next_base_queue;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 3d58762..ab52979 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -3841,6 +3841,12 @@ static int i40e_set_channels(struct net_device *dev,
> if (vsi->type != I40E_VSI_MAIN)
> return -EINVAL;
>
> + /* We do not support setting channels via ethtool when TCs are
> + * configured through mqprio
> + */
> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
> + return -EINVAL;
> +
> /* verify they are not requesting separate vectors */
> if (!count || ch->rx_count || ch->tx_count)
> return -EINVAL;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index e1bea45..7f61d4f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -68,6 +68,7 @@ static int i40e_reset(struct i40e_pf *pf);
> static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired);
> static void i40e_fdir_sb_setup(struct i40e_pf *pf);
> static int i40e_veb_get_bw_info(struct i40e_veb *veb);
> +static int i40e_vsi_config_rss(struct i40e_vsi *vsi);
>
> /* i40e_pci_tbl - PCI Device ID Table
> *
> @@ -1560,6 +1561,105 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
> }
>
> /**
> + * i40e_vsi_setup_queue_map_mqprio - Prepares VSI tc_config to have queue
> + * configurations based on MQPRIO options.
> + * @vsi: the VSI being configured,
> + * @ctxt: VSI context structure
> + * @enabled_tc: number of traffic classes to enable
> + **/
> +static int i40e_vsi_setup_queue_map_mqprio(struct i40e_vsi *vsi,
> + struct i40e_vsi_context *ctxt,
> + u8 enabled_tc)
> +{
> + u8 netdev_tc = 0, offset = 0;
> + u16 qcount = 0, max_qcount, qmap, sections = 0;
> + int i, override_q, pow, num_qps, ret;
> +
> + if (vsi->type != I40E_VSI_MAIN)
> + return -EINVAL;
> +
> + sections = I40E_AQ_VSI_PROP_QUEUE_MAP_VALID;
> + sections |= I40E_AQ_VSI_PROP_SCHED_VALID;
> +
> + vsi->tc_config.numtc = vsi->mqprio_qopt.qopt.num_tc;
> + vsi->tc_config.enabled_tc = enabled_tc ? enabled_tc : 1;
> +
> + num_qps = vsi->mqprio_qopt.qopt.count[0];
> +
> + /* find the next higher power-of-2 of num queue pairs */
> + pow = ilog2(num_qps);
> + if (!is_power_of_2(num_qps))
> + pow++;
> +
> + qmap = (offset << I40E_AQ_VSI_TC_QUE_OFFSET_SHIFT) |
> + (pow << I40E_AQ_VSI_TC_QUE_NUMBER_SHIFT);
> +
> + /* Setup queue offset/count for all TCs for given VSI */
> + max_qcount = vsi->mqprio_qopt.qopt.count[0];
> +
> + for (i = 0; i < I40E_MAX_TRAFFIC_CLASS; i++) {
> + /* See if the given TC is enabled for the given VSI */
> + if (vsi->tc_config.enabled_tc & BIT(i)) {
> + offset = vsi->mqprio_qopt.qopt.offset[i];
> + qcount = vsi->mqprio_qopt.qopt.count[i];
> +
> + if (qcount > max_qcount)
> + max_qcount = qcount;
> +
> + vsi->tc_config.tc_info[i].qoffset = offset;
> + vsi->tc_config.tc_info[i].qcount = qcount;
> + vsi->tc_config.tc_info[i].netdev_tc = netdev_tc++;
> +
> + } else {
> + /* TC is not enabled so set the offset to
> + * default queue and allocate one queue
> + * for the given TC.
> + */
> + vsi->tc_config.tc_info[i].qoffset = 0;
> + vsi->tc_config.tc_info[i].qcount = 1;
> + vsi->tc_config.tc_info[i].netdev_tc = 0;
> + }
> + }
> +
> + /* Set actual Tx/Rx queue pairs */
> + vsi->num_queue_pairs = offset + qcount;
> +
> + /* Setup queue TC[0].qmap for given VSI context */
> + ctxt->info.tc_mapping[0] = cpu_to_le16(qmap);
> +
> + ctxt->info.mapping_flags |=
> + cpu_to_le16(I40E_AQ_VSI_QUE_MAP_CONTIG);
> +
> + ctxt->info.queue_mapping[0] = cpu_to_le16(vsi->base_queue);
> + ctxt->info.valid_sections |= cpu_to_le16(sections);
> +
> + /* Reconfigure RSS for main VSI with max queue count */
> + vsi->rss_size = max_qcount;
> +
> + ret = i40e_vsi_config_rss(vsi);
> + if (ret) {
> + dev_info(&vsi->back->pdev->dev,
> + "Failed to reconfig rss for num_queues (%u)\n",
> + max_qcount);
> + return ret;
> + }
> + vsi->reconfig_rss = true;
> + dev_dbg(&vsi->back->pdev->dev,
> + "Reconfigured rss with num_queues (%u)\n", max_qcount);
> +
> + /* Find queue count available for channel VSIs and starting offset
> + * for channel VSIs
> + */
> + override_q = vsi->mqprio_qopt.qopt.count[0];
> + if (override_q && (override_q < vsi->num_queue_pairs)) {
> + vsi->cnt_q_avail = vsi->num_queue_pairs - override_q;
> + atomic_set(&vsi->next_base_queue, override_q);
> + }
> +
> + return 0;
> +}
> +
> +/**
> * i40e_vsi_setup_queue_map - Setup a VSI queue map based on enabled_tc
> * @vsi: the VSI being setup
> * @ctxt: VSI context structure
> @@ -1597,7 +1697,7 @@ static void i40e_vsi_setup_queue_map(struct i40e_vsi *vsi,
> numtc = 1;
> }
> } else {
> - /* At least TC0 is enabled in case of non-DCB case */
> + /* At least TC0 is enabled in non-DCB, non-MQPRIO case */
> numtc = 1;
> }
>
> @@ -3150,6 +3250,7 @@ static void i40e_vsi_config_dcb_rings(struct i40e_vsi *vsi)
> rx_ring->dcb_tc = 0;
> tx_ring->dcb_tc = 0;
> }
> + return;
> }
>
> for (n = 0; n < I40E_MAX_TRAFFIC_CLASS; n++) {
> @@ -4777,6 +4878,25 @@ static u8 i40e_dcb_get_enabled_tc(struct i40e_dcbx_config *dcbcfg)
> }
>
> /**
> + * i40e_mqprio_get_enabled_tc - Get enabled traffic classes
> + * @pf: PF being queried
> + *
> + * Query the current MQPRIO configuration and return the number of
> + * traffic classes enabled.
> + **/
> +static u8 i40e_mqprio_get_enabled_tc(struct i40e_pf *pf)
> +{
> + struct i40e_vsi *vsi = pf->vsi[pf->lan_vsi];
> + u8 num_tc = vsi->mqprio_qopt.qopt.num_tc;
> + u8 enabled_tc = 1, i;
> +
> + for (i = 1; i < num_tc; i++)
> + enabled_tc |= BIT(i);
> +
> + return enabled_tc;
> +}
> +
> +/**
> * i40e_pf_get_num_tc - Get enabled traffic classes for PF
> * @pf: PF being queried
> *
> @@ -4789,7 +4909,10 @@ static u8 i40e_pf_get_num_tc(struct i40e_pf *pf)
> u8 num_tc = 0;
> struct i40e_dcbx_config *dcbcfg = &hw->local_dcbx_config;
>
> - /* If DCB is not enabled then always in single TC */
> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
> + return pf->vsi[pf->lan_vsi]->mqprio_qopt.qopt.num_tc;
> +
> + /* If neither MQPRIO nor DCB is enabled, then always in single TC */
> if (!(pf->flags & I40E_FLAG_DCB_ENABLED))
> return 1;
>
> @@ -4818,7 +4941,12 @@ static u8 i40e_pf_get_num_tc(struct i40e_pf *pf)
> **/
> static u8 i40e_pf_get_tc_map(struct i40e_pf *pf)
> {
> - /* If DCB is not enabled for this PF then just return default TC */
> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
> + return i40e_mqprio_get_enabled_tc(pf);
> +
> + /* If neither MQPRIO nor DCB is enabled for this PF then just return
> + * default TC
> + */
> if (!(pf->flags & I40E_FLAG_DCB_ENABLED))
> return I40E_DEFAULT_TRAFFIC_CLASS;
>
> @@ -4912,6 +5040,10 @@ static int i40e_vsi_configure_bw_alloc(struct i40e_vsi *vsi, u8 enabled_tc,
> for (i = 0; i < I40E_MAX_TRAFFIC_CLASS; i++)
> bw_data.tc_bw_credits[i] = bw_share[i];
>
> + if ((vsi->back->flags & I40E_FLAG_TC_MQPRIO) ||
> + !vsi->mqprio_qopt.qopt.hw)
> + return 0;
> +
> ret = i40e_aq_config_vsi_tc_bw(&vsi->back->hw, vsi->seid, &bw_data,
> NULL);
> if (ret) {
> @@ -4970,6 +5102,9 @@ static void i40e_vsi_config_netdev_tc(struct i40e_vsi *vsi, u8 enabled_tc)
> vsi->tc_config.tc_info[i].qoffset);
> }
>
> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
> + return;
> +
> /* Assign UP2TC map for the VSI */
> for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) {
> /* Get the actual TC# for the UP */
> @@ -5020,7 +5155,8 @@ static int i40e_vsi_config_tc(struct i40e_vsi *vsi, u8 enabled_tc)
> int i;
>
> /* Check if enabled_tc is same as existing or new TCs */
> - if (vsi->tc_config.enabled_tc == enabled_tc)
> + if (vsi->tc_config.enabled_tc == enabled_tc &&
> + vsi->mqprio_qopt.qopt.hw != TC_MQPRIO_HW_OFFLOAD)
> return ret;
>
> /* Enable ETS TCs with equal BW Share for now across all VSIs */
> @@ -5043,7 +5179,30 @@ static int i40e_vsi_config_tc(struct i40e_vsi *vsi, u8 enabled_tc)
> ctxt.vf_num = 0;
> ctxt.uplink_seid = vsi->uplink_seid;
> ctxt.info = vsi->info;
> - i40e_vsi_setup_queue_map(vsi, &ctxt, enabled_tc, false);
> +
> + if (vsi->back->flags & I40E_FLAG_TC_MQPRIO) {
> + ret = i40e_vsi_setup_queue_map_mqprio(vsi, &ctxt, enabled_tc);
> + if (ret)
> + goto out;
> +
> + } else {
> + i40e_vsi_setup_queue_map(vsi, &ctxt, enabled_tc, false);
> + }
> +
> + /* On destroying the qdisc, reset vsi->rss_size, as number of enabled
> + * queues changed.
> + */
> + if (!vsi->mqprio_qopt.qopt.hw && vsi->reconfig_rss) {
> + vsi->rss_size = min_t(int, vsi->back->alloc_rss_size,
> + vsi->num_queue_pairs);
> + ret = i40e_vsi_config_rss(vsi);
> + if (ret) {
> + dev_info(&vsi->back->pdev->dev,
> + "Failed to reconfig rss for num_queues\n");
> + return ret;
> + }
> + vsi->reconfig_rss = false;
> + }
>
> if (vsi->back->flags & I40E_FLAG_IWARP_ENABLED) {
> ctxt.info.valid_sections |=
> @@ -5051,7 +5210,9 @@ static int i40e_vsi_config_tc(struct i40e_vsi *vsi, u8 enabled_tc)
> ctxt.info.queueing_opt_flags |= I40E_AQ_VSI_QUE_OPT_TCP_ENA;
> }
>
> - /* Update the VSI after updating the VSI queue-mapping information */
> + /* Update the VSI after updating the VSI queue-mapping
> + * information
> + */
> ret = i40e_aq_update_vsi_params(&vsi->back->hw, &ctxt, NULL);
> if (ret) {
> dev_info(&vsi->back->pdev->dev,
> @@ -6168,48 +6329,142 @@ void i40e_down(struct i40e_vsi *vsi)
> }
>
> /**
> + * i40e_validate_mqprio_queue_mapping - validate queue mapping info
> + * @vsi: the VSI being configured
> + * @mqprio_qopt: queue parametrs
> + **/
> +int i40e_validate_mqprio_queue_mapping(struct i40e_vsi *vsi,
> + struct tc_mqprio_qopt_offload *mqprio_qopt)
So I see a few issues with this. From what I can tell this can be
static. Second is that it is triggering a check in checkpatch for
unaligned parameters.
I would recommend using the gcc coding style if needed so that you
could move the "static int" into a separate line if needed in order to
avoid issues with the line being too long. In addition you might look
at reducing the length of the function name if you still have issues
with being over 80 characters. Maybe something like
i40e_validate_mqprio_qopt instead of calling out the mapping and all
that since it seems like most of this is just about validating the
qopt fields provided in the mqprio_qopt structure anyway.
> +{
> + int i;
> +
> + if ((mqprio_qopt->qopt.offset[0] != 0) ||
> + (mqprio_qopt->qopt.num_tc < 1))
> + return -EINVAL;
> +
> + for (i = 0; ; i++) {
> + if (!mqprio_qopt->qopt.count[i])
> + return -EINVAL;
> +
> + if (mqprio_qopt->min_rate[i] || mqprio_qopt->max_rate[i])
> + return -EINVAL;
> +
> + if (i >= mqprio_qopt->qopt.num_tc - 1)
> + break;
> +
> + if (mqprio_qopt->qopt.offset[i + 1] !=
> + (mqprio_qopt->qopt.offset[i] + mqprio_qopt->qopt.count[i]))
> + return -EINVAL;
> + }
> +
> + if (vsi->num_queue_pairs <
> + (mqprio_qopt->qopt.offset[i] + mqprio_qopt->qopt.count[i])) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/**
> * i40e_setup_tc - configure multiple traffic classes
> * @netdev: net device to configure
> - * @tc: number of traffic classes to enable
> + * @tc: pointer to struct tc_to_netdev
> **/
> -static int i40e_setup_tc(struct net_device *netdev, u8 tc)
> +static int i40e_setup_tc(struct net_device *netdev, struct tc_to_netdev *tc)
> {
> struct i40e_netdev_priv *np = netdev_priv(netdev);
> struct i40e_vsi *vsi = np->vsi;
> struct i40e_pf *pf = vsi->back;
> - u8 enabled_tc = 0;
> + u8 enabled_tc = 0, num_tc = 0, hw = 0;
> int ret = -EINVAL;
> int i;
>
> - /* Check if DCB enabled to continue */
> - if (!(pf->flags & I40E_FLAG_DCB_ENABLED)) {
> - netdev_info(netdev, "DCB is not enabled for adapter\n");
> - goto exit;
> + if (tc->type == TC_SETUP_MQPRIO) {
> + hw = tc->mqprio->hw;
> + num_tc = tc->mqprio->num_tc;
> + } else if (tc->type == TC_SETUP_MQPRIO_EXT) {
> + hw = tc->mqprio_qopt->qopt.hw;
> + num_tc = tc->mqprio_qopt->qopt.num_tc;
> }
>
> - /* Check if MFP enabled */
> - if (pf->flags & I40E_FLAG_MFP_ENABLED) {
> - netdev_info(netdev, "Configuring TC not supported in MFP mode\n");
> - goto exit;
> + if (!hw) {
> + pf->flags &= ~I40E_FLAG_TC_MQPRIO;
> + if (tc->type == TC_SETUP_MQPRIO_EXT)
> + memcpy(&vsi->mqprio_qopt, tc->mqprio_qopt,
> + sizeof(*tc->mqprio_qopt));
> + goto config_tc;
> }
>
> - /* Check whether tc count is within enabled limit */
> - if (tc > i40e_pf_get_num_tc(pf)) {
> - netdev_info(netdev, "TC count greater than enabled on link for adapter\n");
> - goto exit;
> + switch (hw) {
> + case TC_MQPRIO_HW_OFFLOAD_TCS:
> + pf->flags &= ~I40E_FLAG_TC_MQPRIO;
> + /* Check if DCB enabled to continue */
> + if (!(pf->flags & I40E_FLAG_DCB_ENABLED)) {
> + netdev_info(netdev,
> + "DCB is not enabled for adapter\n");
> + goto exit;
> + }
> +
> + /* Check if MFP enabled */
> + if (pf->flags & I40E_FLAG_MFP_ENABLED) {
> + netdev_info(netdev,
> + "Configuring TC not supported in MFP mode\n");
> + goto exit;
> + }
> +
> + /* Check whether tc count is within enabled limit */
> + if (num_tc > i40e_pf_get_num_tc(pf)) {
> + netdev_info(netdev,
> + "TC count greater than enabled on link for adapter\n");
> + goto exit;
> + }
> + break;
> + case TC_MQPRIO_HW_OFFLOAD:
> + if (pf->flags & I40E_FLAG_DCB_ENABLED) {
> + netdev_info(netdev,
> + "Full offload of TC Mqprio options is not supported when DCB is enabled\n");
> + goto exit;
> + }
> +
> + /* Check if MFP enabled */
> + if (pf->flags & I40E_FLAG_MFP_ENABLED) {
> + netdev_info(netdev,
> + "Configuring TC not supported in MFP mode\n");
> + goto exit;
> + }
> +
> + ret = i40e_validate_mqprio_queue_mapping(vsi,
> + tc->mqprio_qopt);
> + if (ret)
> + goto exit;
> +
> + memcpy(&vsi->mqprio_qopt, tc->mqprio_qopt,
> + sizeof(*tc->mqprio_qopt));
> +
> + pf->flags |= I40E_FLAG_TC_MQPRIO;
> + pf->flags &= ~I40E_FLAG_DCB_ENABLED;
> +
> + break;
> + default:
> + return -EINVAL;
> }
>
> +config_tc:
> /* Generate TC map for number of tc requested */
> - for (i = 0; i < tc; i++)
> + for (i = 0; i < num_tc; i++)
> enabled_tc |= BIT(i);
>
> /* Requesting same TC configuration as already enabled */
> - if (enabled_tc == vsi->tc_config.enabled_tc)
> + if (enabled_tc == vsi->tc_config.enabled_tc &&
> + hw != TC_MQPRIO_HW_OFFLOAD)
> return 0;
>
> /* Quiesce VSI queues */
> i40e_quiesce_vsi(vsi);
>
> + if (!hw && !(pf->flags & I40E_FLAG_TC_MQPRIO))
> + i40e_remove_queue_channel(vsi);
> +
> /* Configure VSI for enabled TCs */
> ret = i40e_vsi_config_tc(vsi, enabled_tc);
> if (ret) {
> @@ -6229,8 +6484,11 @@ static int i40e_setup_tc(struct net_device *netdev, u8 tc)
>
> /* Unquiesce VSI */
> i40e_unquiesce_vsi(vsi);
> + return ret;
>
> exit:
> + /* Reset the configuration data */
> + memset(&vsi->tc_config, 0, sizeof(vsi->tc_config));
> i40e_unquiesce_vsi(vsi);
> return ret;
> }
> @@ -6238,12 +6496,7 @@ static int i40e_setup_tc(struct net_device *netdev, u8 tc)
> static int __i40e_setup_tc(struct net_device *netdev, u32 handle, __be16 proto,
> struct tc_to_netdev *tc)
> {
> - if (tc->type != TC_SETUP_MQPRIO)
> - return -EINVAL;
> -
> - tc->mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
> -
> - return i40e_setup_tc(netdev, tc->mqprio->num_tc);
> + return i40e_setup_tc(netdev, tc);
> }
>
> /**
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ 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