* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
From: David Ahern @ 2017-12-05 16:41 UTC (permalink / raw)
To: Johannes Berg, David Miller; +Cc: linux-wireless, netdev, j
In-Reply-To: <1512491661.26976.19.camel@sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]
On 12/5/17 9:34 AM, Johannes Berg wrote:
> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>>
>>> We could try to fix up the big endian problem here, but we
>>> don't know *how* userspace misbehaved - if using nla_put_u32
>>> then we could, but we also found a debug tool (which we'll
>>> ignore for the purposes of this regression) that was putting
>>> the padding into the length.
>
>> We're stuck with this thing forever... I'd like to consider other
>> options.
>>
>> I've seen this problem at least one time before, therefore I
>> suggest when we see a U8 attribute with a U32's length:
>>
>> 1) We access it as a u32, this takes care of all endianness
>> issues.
>
> Possible, but as I said above, I've seen at least one tool (a debug
> only script) now that will actually emit a U8 followed by 3 bytes of
> padding to make it netlink-aligned, but set the length to 4. That would
> be broken by making this change.
>
> I'm not saying this is bad - but there are different levels of
> compatibility and I'd probably go for "bug compatibility" here rather
> than "fix-it-up compatibility".
>
> Your call, ultimately - I've already fixed the tool I had found :-)
>
>> 2) We emit a warning so that the app gets fixes.
>
The attached is my proposal. Basically, allow it the length mismatch but
print a warning. This restores previous behavior and tells users of bad
commands.
[-- Attachment #2: 0001-netlink-Relax-attr-validation-for-fixed-length-types.patch --]
[-- Type: text/plain, Size: 2222 bytes --]
From 29a504c8de553c17d4aafd5a2cb9384a28672fe4 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Sun, 3 Dec 2017 07:22:20 -0800
Subject: [PATCH] netlink: Relax attr validation for fixed length types
Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
exact length for some types") requires attributes using types NLA_U* and
NLA_S* to have an exact length. This change is exposing bugs in various
userspace commands that are sending attributes with an invalid length
(e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
the commands are clearly broken and need to be fixed, users are arguing
that the sudden change in enforcement is breaking older commands on
newer kernels for use cases that otherwise "worked".
Relax the validation to print a warning mesage similar to what is done
for messages containing extra bytes after parsing.
Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
lib/nlattr.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..6122662906c8 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -28,8 +28,16 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
};
static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+ [NLA_U8] = sizeof(u8),
+ [NLA_U16] = sizeof(u16),
+ [NLA_U32] = sizeof(u32),
+ [NLA_U64] = sizeof(u64),
[NLA_MSECS] = sizeof(u64),
[NLA_NESTED] = NLA_HDRLEN,
+ [NLA_S8] = sizeof(s8),
+ [NLA_S16] = sizeof(s16),
+ [NLA_S32] = sizeof(s32),
+ [NLA_S64] = sizeof(s64),
};
static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
/* for data types NLA_U* and NLA_S* require exact length */
- if (nla_attr_len[pt->type]) {
- if (attrlen != nla_attr_len[pt->type])
- return -ERANGE;
- return 0;
+ if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
+ pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
+ current->comm, type);
}
switch (pt->type) {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH V2 net-next 0/3] net: hns3: Refactors "reset" handling code in HCLGE layer of HNS3 driver
From: David Miller @ 2017-12-05 16:48 UTC (permalink / raw)
To: salil.mehta-hv44wF8Li93QT0dZR+AlfA
Cc: yisen.zhuang-hv44wF8Li93QT0dZR+AlfA,
lipeng321-hv44wF8Li93QT0dZR+AlfA,
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linuxarm-hv44wF8Li93QT0dZR+AlfA
In-Reply-To: <20171204012955.17560-1-salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
From: Salil Mehta <salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Date: Mon, 4 Dec 2017 01:29:52 +0000
> This patch refactors the code of the reset feature in HCLGE layer
> of HNS3 PF driver. Prime motivation to do this change is:
> 1. To reduce the time for which common miscellaneous Vector 0
> interrupt is disabled because of the reset. Simplification
> of the common miscellaneous interrupt handler routine(for
> Vector 0) used to handle reset and other sources of Vector
> 0 interrupt.
> 2. Separate the task for handling the reset
> 3. Simplification of reset request submission and pending reset
> logic.
>
>
> To achieve above below few things have been done:
> 1. Interrupt is disabled while common miscellaneous interrupt
> handler is entered and re-enabled before it is exit. This
> reduces the interrupt handling latency as compared to older
> interrupt handling scheme where interrupt was being disabled
> in interrupt handler context and re-enabled in task context
> some time later. Made Miscellaneous interrupt handler more
> generic to handle all sources including reset interrupt source.
> 2. New reset service task has been introduced to service the
> reset handling.
> 3. Introduces new reset service task for honoring software reset
> requests like from network stack related to timeout and serving
> the pending reset request(to reset the driver and associated
> clients).
>
>
> Change Log:
> Patch V2: Addressed comment by Andrew Lunn
> Link: https://lkml.org/lkml/2017/12/1/366
> Patch V1: Initial Submit
Series applied, thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/1] gianfar: fix a flooded alignment reports because of padding issue.
From: David Miller @ 2017-12-05 16:48 UTC (permalink / raw)
To: zumeng.chen; +Cc: netdev, linux-kernel, claudiu.manoil
In-Reply-To: <1512357722-12124-1-git-send-email-zumeng.chen@gmail.com>
From: Zumeng Chen <zumeng.chen@gmail.com>
Date: Mon, 4 Dec 2017 11:22:02 +0800
> According to LS1021A RM, the value of PAL can be set so that the start of the
> IP header in the receive data buffer is aligned to a 32-bit boundary. Normally,
> setting PAL = 2 provides minimal padding to ensure such alignment of the IP
> header.
>
> However every incoming packet's 8-byte time stamp will be inserted into the
> packet data buffer as padding alignment bytes when hardware time stamping is
> enabled.
>
> So we set the padding 8+2 here to avoid the flooded alignment faults:
>
> root@128:~# cat /proc/cpu/alignment
> User: 0
> System: 17539 (inet_gro_receive+0x114/0x2c0)
> Skipped: 0
> Half: 0
> Word: 0
> DWord: 0
> Multi: 17539
> User faults: 2 (fixup)
>
> Also shown when exception report enablement
...
> Signed-off-by: Zumeng Chen <zumeng.chen@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
From: David Miller @ 2017-12-05 16:51 UTC (permalink / raw)
To: dsahern-Re5JQEeQqe8AvxtiuMwx3w
Cc: johannes-cdvu00un1VgdHxzADdlk8Q,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, j
In-Reply-To: <fe8e7ad5-9579-31ce-0d02-a6c8522ba366-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: David Ahern <dsahern-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Tue, 5 Dec 2017 09:41:21 -0700
> On 12/5/17 9:34 AM, Johannes Berg wrote:
>> On Tue, 2017-12-05 at 11:31 -0500, David Miller wrote:
>>>
>>>> We could try to fix up the big endian problem here, but we
>>>> don't know *how* userspace misbehaved - if using nla_put_u32
>>>> then we could, but we also found a debug tool (which we'll
>>>> ignore for the purposes of this regression) that was putting
>>>> the padding into the length.
>>
>>> We're stuck with this thing forever... I'd like to consider other
>>> options.
>>>
>>> I've seen this problem at least one time before, therefore I
>>> suggest when we see a U8 attribute with a U32's length:
>>>
>>> 1) We access it as a u32, this takes care of all endianness
>>> issues.
>>
>> Possible, but as I said above, I've seen at least one tool (a debug
>> only script) now that will actually emit a U8 followed by 3 bytes of
>> padding to make it netlink-aligned, but set the length to 4. That would
>> be broken by making this change.
>>
>> I'm not saying this is bad - but there are different levels of
>> compatibility and I'd probably go for "bug compatibility" here rather
>> than "fix-it-up compatibility".
>>
>> Your call, ultimately - I've already fixed the tool I had found :-)
>>
>>> 2) We emit a warning so that the app gets fixes.
>>
>
> The attached is my proposal. Basically, allow it the length mismatch but
> print a warning. This restores previous behavior and tells users of bad
> commands.
Where is the "access the U8 attribute as a U32 if length is 4" part
of my #1 above?
That's essential to handle this properly on all endianness.
^ permalink raw reply
* Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map_results enum
From: David Miller @ 2017-12-05 16:53 UTC (permalink / raw)
To: subashab; +Cc: netdev
In-Reply-To: <1512369428-20455-2-git-send-email-subashab@codeaurora.org>
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Date: Sun, 3 Dec 2017 23:37:03 -0700
> Only the success and consumed entries were actually in use.
> Use standard error codes instead.
>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
That is definitely not the only thing this change is doing:
> @@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>
> if (skb_headroom(skb) < required_headroom) {
> if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL))
> - return RMNET_MAP_CONSUMED;
> + goto fail;
> }
>
> map_header = rmnet_map_add_map_header(skb, additional_header_len, 0);
> if (!map_header)
> - return RMNET_MAP_CONSUMED;
> + goto fail;
>
Previously these paths would return "RMNET_MAP_CONSUMED":
> @@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb)
> }
>
> if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) {
> - switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) {
> - case RMNET_MAP_CONSUMED:
> - return;
> -
> - case RMNET_MAP_SUCCESS:
> - break;
> -
> - default:
> - kfree_skb(skb);
> + if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
> return;
> - }
> }
Causing this code to return.
Now, instead the code jumps to fail:
> @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>
> skb->protocol = htons(ETH_P_MAP);
>
> - return RMNET_MAP_SUCCESS;
> + return 0;
> +
> +fail:
> + kfree_skb(skb);
> + return -ENOMEM;
> }
>
Which frees the SKB.
This is a behavioral change, perhaps a bug fix.
Well, nobody knows because you do not explain this at all in your
commit message.
Do not mix functional changes with cleanups. If you want to change how
freeing the SKB is done, do it in a separate change from the patch that
removes this enumeration.
Thank you.
^ permalink raw reply
* Re: [PATCH net-next 4/6] net: qualcomm: rmnet: Process packets over ethernet
From: David Miller @ 2017-12-05 16:55 UTC (permalink / raw)
To: subashab; +Cc: netdev
In-Reply-To: <1512369428-20455-5-git-send-email-subashab@codeaurora.org>
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Date: Sun, 3 Dec 2017 23:37:06 -0700
> Add support to send and receive packets over ethernet.
> An example of usage is testing the data path on UML. This can be
> achieved by setting up two UML instances in multicast mode and
> associating rmnet over the UML ethernet device.
>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
> drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index a46053c..ee20f23 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -15,6 +15,7 @@
>
> #include <linux/netdevice.h>
> #include <linux/netdev_features.h>
> +#include <linux/if_arp.h>
> #include "rmnet_private.h"
> #include "rmnet_config.h"
> #include "rmnet_vnd.h"
> @@ -104,6 +105,12 @@ static void rmnet_set_skb_proto(struct sk_buff *skb)
> {
> struct sk_buff *skbn;
>
> + if (skb->dev->type == ARPHRD_ETHER) {
> + if (skb_headroom(skb) < ETH_HLEN)
> + kfree_skb(skb);
> + skb_push(skb, ETH_HLEN);
> + }
This will crash, you didn't test the new code path that does the
kfree_skb().
You should use an SKB helper function which will realloc the headroom
if ETH_HLEN is not available, instead of failing.
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 02/10] ixgbe: add ipsec register access routines
From: Alexander Duyck @ 2017-12-05 16:56 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1512452116-14795-3-git-send-email-shannon.nelson@oracle.com>
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> Add a few routines to make access to the ipsec registers just a little
> easier, and throw in the beginnings of an initialization.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/Makefile | 1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 6 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 157 +++++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h | 50 ++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
> 5 files changed, 215 insertions(+)
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
> index 35e6fa6..8319465 100644
> --- a/drivers/net/ethernet/intel/ixgbe/Makefile
> +++ b/drivers/net/ethernet/intel/ixgbe/Makefile
> @@ -42,3 +42,4 @@ ixgbe-$(CONFIG_IXGBE_DCB) += ixgbe_dcb.o ixgbe_dcb_82598.o \
> ixgbe-$(CONFIG_IXGBE_HWMON) += ixgbe_sysfs.o
> ixgbe-$(CONFIG_DEBUG_FS) += ixgbe_debugfs.o
> ixgbe-$(CONFIG_FCOE:m=y) += ixgbe_fcoe.o
> +ixgbe-$(CONFIG_XFRM_OFFLOAD) += ixgbe_ipsec.o
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index dd55787..1e11462 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -52,6 +52,7 @@
> #ifdef CONFIG_IXGBE_DCA
> #include <linux/dca.h>
> #endif
> +#include "ixgbe_ipsec.h"
>
> #include <net/busy_poll.h>
>
> @@ -1001,4 +1002,9 @@ void ixgbe_store_key(struct ixgbe_adapter *adapter);
> void ixgbe_store_reta(struct ixgbe_adapter *adapter);
> s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32 adv_reg, u32 lp_reg,
> u32 adv_sym, u32 adv_asm, u32 lp_sym, u32 lp_asm);
> +#ifdef CONFIG_XFRM_OFFLOAD
> +void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
> +#else
> +static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
> +#endif /* CONFIG_XFRM_OFFLOAD */
> #endif /* _IXGBE_H_ */
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> new file mode 100644
> index 0000000..14dd011
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -0,0 +1,157 @@
> +/*******************************************************************************
> + *
> + * Intel 10 Gigabit PCI Express Linux driver
> + * Copyright(c) 2017 Oracle and/or its affiliates. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Linux NICS <linux.nics@intel.com>
> + * e1000-devel Mailing List <e1000-devel@lists.sourceforge.net>
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
> + *
> + ******************************************************************************/
> +
> +#include "ixgbe.h"
> +
> +/**
> + * ixgbe_ipsec_set_tx_sa - set the Tx SA registers
> + * @hw: hw specific details
> + * @idx: register index to write
> + * @key: key byte array
> + * @salt: salt bytes
> + **/
> +static void ixgbe_ipsec_set_tx_sa(struct ixgbe_hw *hw, u16 idx,
> + u32 key[], u32 salt)
> +{
> + u32 reg;
> + int i;
> +
> + for (i = 0; i < 4; i++)
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(i), cpu_to_be32(key[3-i]));
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, cpu_to_be32(salt));
> + IXGBE_WRITE_FLUSH(hw);
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_IPSTXIDX);
> + reg &= IXGBE_RXTXIDX_IPS_EN;
> + reg |= idx << 3 | IXGBE_RXTXIDX_IDX_WRITE;
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, reg);
> + IXGBE_WRITE_FLUSH(hw);
> +}
> +
So there are a few things here to unpack.
The first is the carry-forward of the IPS bit. I'm not sure that is
the best way to go. Do we really expect to be updating SA values if
IPsec offload is not enabled? If so we may just want to carry a bit
flag somewhere in the ixgbe_hw struct indicating if Tx IPsec offload
is enabled and use that to determine the value for this bit.
Also we should probably replace "3" with a value indicating that it is
the SA index shift.
Also technically the WRITE_FLUSH isn't needed if you are doing a PCIe
read anyway to get IPSTXIDX.
> +/**
> + * ixgbe_ipsec_set_rx_item - set an Rx table item
> + * @hw: hw specific details
> + * @idx: register index to write
> + * @tbl: table selector
> + *
> + * Trigger the device to store into a particular Rx table the
> + * data that has already been loaded into the input register
> + **/
> +static void ixgbe_ipsec_set_rx_item(struct ixgbe_hw *hw, u16 idx, u32 tbl)
> +{
> + u32 reg;
> +
> + reg = IXGBE_READ_REG(hw, IXGBE_IPSRXIDX);
> + reg &= IXGBE_RXTXIDX_IPS_EN;
> + reg |= tbl | idx << 3 | IXGBE_RXTXIDX_IDX_WRITE;
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, reg);
> + IXGBE_WRITE_FLUSH(hw);
> +}
> +
The Rx version of this gets a bit trickier since the datasheet
actually indicates there are a few different types of tables that can
be indexed via this. Also why is the tbl value not being shifted? It
seems like it should be shifted by 1 to avoid overwriting the IPS_EN
bit. Really I would like to see the tbl value converted to an enum and
shifted by 1 in order to generate the table reference.
Here the "3" is a table index. It might be nice to call that out with
a name instead of using the magic number.
> +/**
> + * ixgbe_ipsec_set_rx_sa - set up the register bits to save SA info
> + * @hw: hw specific details
> + * @idx: register index to write
> + * @spi: security parameter index
> + * @key: key byte array
> + * @salt: salt bytes
> + * @mode: rx decrypt control bits
> + * @ip_idx: index into IP table for related IP address
> + **/
> +static void ixgbe_ipsec_set_rx_sa(struct ixgbe_hw *hw, u16 idx, __be32 spi,
> + u32 key[], u32 salt, u32 mode, u32 ip_idx)
> +{
> + int i;
> +
> + /* store the SPI (in bigendian) and IPidx */
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, spi);
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
> + IXGBE_WRITE_FLUSH(hw);
> +
> + ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_SPI);
> +
> + /* store the key, salt, and mode */
> + for (i = 0; i < 4; i++)
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(i), cpu_to_be32(key[3-i]));
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, cpu_to_be32(salt));
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
> + IXGBE_WRITE_FLUSH(hw);
> +
> + ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_KEY);
> +}
Is there any reason why you could write the SPI, key, salt, and mode,
then flush, and trigger the writes via the IPSRXIDX? Just wondering
since it would likely save you a few cycles avoiding PCIe bus stalls.
> +
> +/**
> + * ixgbe_ipsec_set_rx_ip - set up the register bits to save SA IP addr info
> + * @hw: hw specific details
> + * @idx: register index to write
> + * @addr: IP address byte array
> + **/
> +static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32 addr[])
> +{
> + int i;
> +
> + /* store the ip address */
> + for (i = 0; i < 4; i++)
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(i), addr[i]);
> + IXGBE_WRITE_FLUSH(hw);
> +
> + ixgbe_ipsec_set_rx_item(hw, idx, IXGBE_RXIDX_TBL_IP);
> +}
> +
This piece is kind of confusing. I would suggest storing the address
as a __be32 pointer instead of a u32 array. That way you start with
either an IPv6 or an IPv4 address at offset 0 instead of the way the
hardware is defined which has you writing it at either 0 or 3
depending on if the address is IPv6 or IPv4.
> +/**
> + * ixgbe_ipsec_clear_hw_tables - because some tables don't get cleared on reset
> + * @adapter: board private structure
> + **/
> +void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
> +{
> + struct ixgbe_hw *hw = &adapter->hw;
> + u32 buf[4] = {0, 0, 0, 0};
> + u16 idx;
> +
> + /* disable Rx and Tx SA lookup */
> + IXGBE_WRITE_REG(hw, IXGBE_IPSRXIDX, 0);
> + IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0);
> +
> + /* scrub the tables */
> + for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
> + ixgbe_ipsec_set_tx_sa(hw, idx, buf, 0);
> +
> + for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
> + ixgbe_ipsec_set_rx_sa(hw, idx, 0, buf, 0, 0, 0);
> +
> + for (idx = 0; idx < IXGBE_IPSEC_MAX_RX_IP_COUNT; idx++)
> + ixgbe_ipsec_set_rx_ip(hw, idx, buf);
> +}
> +
> +/**
> + * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
> + * @adapter: board private structure
> + **/
> +void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
> +{
> + ixgbe_ipsec_clear_hw_tables(adapter);
> +}
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
> new file mode 100644
> index 0000000..017b13f
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
> @@ -0,0 +1,50 @@
> +/*******************************************************************************
> +
> + Intel 10 Gigabit PCI Express Linux driver
> + Copyright(c) 2017 Oracle and/or its affiliates. All rights reserved.
> +
> + This program is free software; you can redistribute it and/or modify it
> + under the terms and conditions of the GNU General Public License,
> + version 2, as published by the Free Software Foundation.
> +
> + This program is distributed in the hope it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + more details.
> +
> + You should have received a copy of the GNU General Public License along with
> + this program. If not, see <http://www.gnu.org/licenses/>.
> +
> + The full GNU General Public License is included in this distribution in
> + the file called "COPYING".
> +
> + Contact Information:
> + Linux NICS <linux.nics@intel.com>
> + e1000-devel Mailing List <e1000-devel@lists.sourceforge.net>
> + Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
> +
> +*******************************************************************************/
> +
> +#ifndef _IXGBE_IPSEC_H_
> +#define _IXGBE_IPSEC_H_
> +
> +#define IXGBE_IPSEC_MAX_SA_COUNT 1024
> +#define IXGBE_IPSEC_MAX_RX_IP_COUNT 128
> +#define IXGBE_IPSEC_BASE_RX_INDEX IXGBE_IPSEC_MAX_SA_COUNT
> +#define IXGBE_IPSEC_BASE_TX_INDEX (IXGBE_IPSEC_MAX_SA_COUNT * 2)
> +
> +#define IXGBE_RXTXIDX_IPS_EN 0x00000001
> +#define IXGBE_RXIDX_TBL_MASK 0x00000006
> +#define IXGBE_RXIDX_TBL_IP 0x00000002
> +#define IXGBE_RXIDX_TBL_SPI 0x00000004
> +#define IXGBE_RXIDX_TBL_KEY 0x00000006
You might look at converting these table entries into an enum and add
a shift value. It will make things much easier to read.
> +#define IXGBE_RXTXIDX_IDX_MASK 0x00001ff8
> +#define IXGBE_RXTXIDX_IDX_READ 0x40000000
> +#define IXGBE_RXTXIDX_IDX_WRITE 0x80000000
> +
> +#define IXGBE_RXMOD_VALID 0x00000001
> +#define IXGBE_RXMOD_PROTO_ESP 0x00000004
> +#define IXGBE_RXMOD_DECRYPT 0x00000008
> +#define IXGBE_RXMOD_IPV6 0x00000010
> +
> +#endif /* _IXGBE_IPSEC_H_ */
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 6d5f31e..51fb3cf 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -10327,6 +10327,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> NETIF_F_FCOE_MTU;
> }
> #endif /* IXGBE_FCOE */
> + ixgbe_init_ipsec_offload(adapter);
>
> if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
> netdev->hw_features |= NETIF_F_LRO;
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* RE: [PATCH V2 net-next 0/3] net: hns3: Refactors "reset" handling code in HCLGE layer of HNS3 driver
From: Salil Mehta @ 2017-12-05 16:59 UTC (permalink / raw)
To: David Miller
Cc: Zhuangyuzeng (Yisen), lipeng (Y), mehta.salil.lnk@gmail.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-rdma@vger.kernel.org, Linuxarm
In-Reply-To: <20171205.114833.2112478522356368321.davem@davemloft.net>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, December 05, 2017 4:49 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: Zhuangyuzeng (Yisen) <yisen.zhuang@huawei.com>; lipeng (Y)
> <lipeng321@huawei.com>; mehta.salil.lnk@gmail.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> rdma@vger.kernel.org; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH V2 net-next 0/3] net: hns3: Refactors "reset"
> handling code in HCLGE layer of HNS3 driver
>
> From: Salil Mehta <salil.mehta@huawei.com>
> Date: Mon, 4 Dec 2017 01:29:52 +0000
>
> > This patch refactors the code of the reset feature in HCLGE layer
> > of HNS3 PF driver. Prime motivation to do this change is:
> > 1. To reduce the time for which common miscellaneous Vector 0
> > interrupt is disabled because of the reset. Simplification
> > of the common miscellaneous interrupt handler routine(for
> > Vector 0) used to handle reset and other sources of Vector
> > 0 interrupt.
> > 2. Separate the task for handling the reset
> > 3. Simplification of reset request submission and pending reset
> > logic.
> >
> >
> > To achieve above below few things have been done:
> > 1. Interrupt is disabled while common miscellaneous interrupt
> > handler is entered and re-enabled before it is exit. This
> > reduces the interrupt handling latency as compared to older
> > interrupt handling scheme where interrupt was being disabled
> > in interrupt handler context and re-enabled in task context
> > some time later. Made Miscellaneous interrupt handler more
> > generic to handle all sources including reset interrupt source.
> > 2. New reset service task has been introduced to service the
> > reset handling.
> > 3. Introduces new reset service task for honoring software reset
> > requests like from network stack related to timeout and serving
> > the pending reset request(to reset the driver and associated
> > clients).
> >
> >
> > Change Log:
> > Patch V2: Addressed comment by Andrew Lunn
> > Link: https://lkml.org/lkml/2017/12/1/366
> > Patch V1: Initial Submit
>
> Series applied, thank you.
Thanks Dave.
^ permalink raw reply
* Re: [PATCH v3 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
From: Marcelo Ricardo Leitner @ 2017-12-05 17:00 UTC (permalink / raw)
To: Yafang Shao
Cc: davem, songliubraving, kuznet, yoshfuji, rostedt, bgregg, netdev,
linux-kernel
In-Reply-To: <1512483162-5557-1-git-send-email-laoar.shao@gmail.com>
On Tue, Dec 05, 2017 at 02:12:42PM +0000, Yafang Shao wrote:
> The TCP/IP transition from TCP_LISTEN to TCP_SYN_RECV and some other
> transitions are not traced with tcp_set_state tracepoint.
>
> In order to trace the whole tcp lifespans, two helpers are introduced,
> void __tcp_set_state(struct sock *sk, int state);
> void sk_state_store(struct sock *sk, int newstate);
>
> When do TCP/IP state transition, we should use these two helpers or use
> tcp_set_state() other than assigning a value to sk_state directly.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks
> ---
> v2->v3: Per suggestion from Marcelo Ricardo Leitner, inverting __
> to sk_state_store.
> ---
> include/net/sock.h | 7 +++++--
> include/net/tcp.h | 1 +
> net/ipv4/inet_connection_sock.c | 4 ++--
> net/ipv4/inet_hashtables.c | 2 +-
> net/ipv4/tcp.c | 14 +++++++++++++-
> 5 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 79e1a2c..e89a098 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2349,18 +2349,21 @@ static inline int sk_state_load(const struct sock *sk)
> }
>
> /**
> - * sk_state_store - update sk->sk_state
> + * __sk_state_store - update sk->sk_state
> * @sk: socket pointer
> * @newstate: new state
> *
> * Paired with sk_state_load(). Should be used in contexts where
> * state change might impact lockless readers.
> */
> -static inline void sk_state_store(struct sock *sk, int newstate)
> +static inline void __sk_state_store(struct sock *sk, int newstate)
> {
> smp_store_release(&sk->sk_state, newstate);
> }
>
> +/* For tcp_set_state tracepoint */
> +void sk_state_store(struct sock *sk, int newstate);
> +
> void sock_enable_timestamp(struct sock *sk, int flag);
> int sock_get_timestamp(struct sock *, struct timeval __user *);
> int sock_get_timestampns(struct sock *, struct timespec __user *);
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4e09398..cc43ce9 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1247,6 +1247,7 @@ static inline bool tcp_checksum_complete(struct sk_buff *skb)
> "Close Wait","Last ACK","Listen","Closing"
> };
> #endif
> +void __tcp_set_state(struct sock *sk, int state);
> void tcp_set_state(struct sock *sk, int state);
>
> void tcp_done(struct sock *sk);
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 4ca46dc..74523c4 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -783,7 +783,7 @@ struct sock *inet_csk_clone_lock(const struct sock *sk,
> if (newsk) {
> struct inet_connection_sock *newicsk = inet_csk(newsk);
>
> - newsk->sk_state = TCP_SYN_RECV;
> + __tcp_set_state(newsk, TCP_SYN_RECV);
> newicsk->icsk_bind_hash = NULL;
>
> inet_sk(newsk)->inet_dport = inet_rsk(req)->ir_rmt_port;
> @@ -888,7 +888,7 @@ int inet_csk_listen_start(struct sock *sk, int backlog)
> return 0;
> }
>
> - sk->sk_state = TCP_CLOSE;
> + __tcp_set_state(sk, TCP_CLOSE);
> return err;
> }
> EXPORT_SYMBOL_GPL(inet_csk_listen_start);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index f6f5810..b24cc12 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -544,7 +544,7 @@ bool inet_ehash_nolisten(struct sock *sk, struct sock *osk)
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
> } else {
> percpu_counter_inc(sk->sk_prot->orphan_count);
> - sk->sk_state = TCP_CLOSE;
> + __tcp_set_state(sk, TCP_CLOSE);
> sock_set_flag(sk, SOCK_DEAD);
> inet_csk_destroy_sock(sk);
> }
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1803116..4ade887 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2036,6 +2036,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
> }
> EXPORT_SYMBOL(tcp_recvmsg);
>
> +void sk_state_store(struct sock *sk, int newstate)
> +{
> + trace_tcp_set_state(sk, sk->sk_state, newstate);
> + __sk_state_store(sk, newstate);
> +}
> +
> +void __tcp_set_state(struct sock *sk, int state)
> +{
> + trace_tcp_set_state(sk, sk->sk_state, state);
> + sk->sk_state = state;
> +}
> +
> void tcp_set_state(struct sock *sk, int state)
> {
> int oldstate = sk->sk_state;
> @@ -2065,7 +2077,7 @@ void tcp_set_state(struct sock *sk, int state)
> /* Change state AFTER socket is unhashed to avoid closed
> * socket sitting in hash tables.
> */
> - sk_state_store(sk, state);
> + __sk_state_store(sk, state);
>
> #ifdef STATE_TRACE
> SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n", sk, statename[oldstate], statename[state]);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH net-next V3] tun: add eBPF based queue selection method
From: David Miller @ 2017-12-05 17:02 UTC (permalink / raw)
To: jasowang
Cc: netdev, linux-kernel, mst, willemdebruijn.kernel, tom, aconole,
wexu
In-Reply-To: <1512379883-11887-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Mon, 4 Dec 2017 17:31:23 +0800
> This patch introduces an eBPF based queue selection method. With this,
> the policy could be offloaded to userspace completely through a new
> ioctl TUNSETSTEERINGEBPF.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V2:
> - call rtnl during netdev free
> - switch to use call_rcu() to prevent DOS from userspace
> - drop the policies setting/getting ioctls and allow detach through
> passing -1 as fd
Applied, thanks Jason.
I really wish this driver had newlink/changelink support rather than
us adding all of these ioctls...
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 04/10] ixgbe: add ipsec data structures
From: Alexander Duyck @ 2017-12-05 17:03 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1512452116-14795-5-git-send-email-shannon.nelson@oracle.com>
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> Set up the data structures to be used by the ipsec offload.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 5 ++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h | 40 ++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 1e11462..9487750 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -622,6 +622,7 @@ struct ixgbe_adapter {
> #define IXGBE_FLAG2_EEE_CAPABLE BIT(14)
> #define IXGBE_FLAG2_EEE_ENABLED BIT(15)
> #define IXGBE_FLAG2_RX_LEGACY BIT(16)
> +#define IXGBE_FLAG2_IPSEC_ENABLED BIT(17)
>
> /* Tx fast path data */
> int num_tx_queues;
> @@ -772,6 +773,10 @@ struct ixgbe_adapter {
>
> #define IXGBE_RSS_KEY_SIZE 40 /* size of RSS Hash Key in bytes */
> u32 *rss_key;
> +
> +#ifdef CONFIG_XFRM
> + struct ixgbe_ipsec *ipsec;
> +#endif /* CONFIG_XFRM */
> };
>
> static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
> index 017b13f..cb9a4be 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
> @@ -47,4 +47,44 @@
> #define IXGBE_RXMOD_DECRYPT 0x00000008
> #define IXGBE_RXMOD_IPV6 0x00000010
>
> +struct rx_sa {
> + struct hlist_node hlist;
> + struct xfrm_state *xs;
> + u32 ipaddr[4];
ipaddr should be stored as a __be32, not a u32.
> + u32 key[4];
> + u32 salt;
> + u32 mode;
> + u8 iptbl_ind;
> + bool used;
> + bool decrypt;
> +};
> +
> +struct rx_ip_sa {
> + u32 ipaddr[4];
Same thing here.
> + u32 ref_cnt;
> + bool used;
> +};
> +
> +struct tx_sa {
> + struct xfrm_state *xs;
> + u32 key[4];
> + u32 salt;
> + bool encrypt;
> + bool used;
> +};
> +
> +struct ixgbe_ipsec_tx_data {
> + u32 flags;
> + u16 trailer_len;
> + u16 sa_idx;
> +};
> +
> +struct ixgbe_ipsec {
> + u16 num_rx_sa;
> + u16 num_tx_sa;
> + struct rx_ip_sa *ip_tbl;
> + struct rx_sa *rx_tbl;
> + struct tx_sa *tx_tbl;
> + DECLARE_HASHTABLE(rx_sa_list, 8);
The hash table seems a bit on the small side. You might look at
increasing this to something like 32 in order to try and cut down on
the load in each bucket since the upper limit is 1K or so isn't it?
> +};
> #endif /* _IXGBE_IPSEC_H_ */
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [PATCH net-next] flow_dissector: dissect tunnel info outside __skb_flow_dissect()
From: David Miller @ 2017-12-05 17:10 UTC (permalink / raw)
To: simon.horman
Cc: jiri, jhs, xiyou.wangcong, netdev, oss-drivers, tom,
willemdebruijn.kernel
In-Reply-To: <20171204103148.4287-1-simon.horman@netronome.com>
From: Simon Horman <simon.horman@netronome.com>
Date: Mon, 4 Dec 2017 11:31:48 +0100
> Move dissection of tunnel info to outside of the main flow dissection
> function, __skb_flow_dissect(). The sole user of this feature, the flower
> classifier, is updated to call tunnel info dissection directly, using
> skb_flow_dissect_tunnel_info().
>
> This results in a slightly less complex implementation of
> __skb_flow_dissect(), in particular removing logic from that call path
> which is not used by the majority of users. The expense of this is borne by
> the flower classifier which now has to make an extra call for tunnel info
> dissection.
>
> This patch should not result in any behavioural change.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Ok, let's see where this goes :-)
Applied, thanks.
^ permalink raw reply
* RE: [PATCH net-next 1/8] net: hns3: Add HNS3 VF IMP(Integrated Management Proc) cmd interface
From: Salil Mehta @ 2017-12-05 17:11 UTC (permalink / raw)
To: David Miller
Cc: Zhuangyuzeng (Yisen), lipeng (Y), mehta.salil.lnk@gmail.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-rdma@vger.kernel.org, Linuxarm
In-Reply-To: <20171205.113744.2040678170803031734.davem@davemloft.net>
Hi Dave,
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, December 05, 2017 4:38 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: Zhuangyuzeng (Yisen) <yisen.zhuang@huawei.com>; lipeng (Y)
> <lipeng321@huawei.com>; mehta.salil.lnk@gmail.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> rdma@vger.kernel.org; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH net-next 1/8] net: hns3: Add HNS3 VF IMP(Integrated
> Management Proc) cmd interface
>
> From: Salil Mehta <salil.mehta@huawei.com>
> Date: Sun, 3 Dec 2017 12:33:00 +0000
>
> > +static int hclgevf_ring_space(struct hclgevf_cmq_ring *ring)
> > +{
> > + int ntu = ring->next_to_use;
> > + int ntc = ring->next_to_clean;
> > + int used = (ntu - ntc + ring->desc_num) % ring->desc_num;
>
> Order local variables from longest to shortest line, please.
This one skipped my eyes. Will fix. Thanks.
>
> Audit your entire submission for this issue.
Sure, will do.
>
> > +static int hclgevf_cmd_csq_done(struct hclgevf_hw *hw)
> > +{
> > + u32 head = hclgevf_read_dev(hw, HCLGEVF_NIC_CSQ_HEAD_REG);
> > + return head == hw->cmq.csq.next_to_use;
> > +}
>
> Please return bool from this function.
Agreed. Should have been bool here. Will fix.
>
> > +void hclgevf_cmd_setup_basic_desc(struct hclgevf_desc *desc,
> > + enum hclgevf_opcode_type opcode, bool
> is_read)
> > +{
> > + memset((void *)desc, 0, sizeof(struct hclgevf_desc));
>
> You never need casts like this, when the functions argument is void any
> pointer can be passed in as-is.
Agreed. Will remove this.
>
> > +
> > + /* If the command is sync, wait for the firmware to write back,
> > + * if multi descriptors to be sent, use the first one to check
> > + */
> > + if (HCLGEVF_SEND_SYNC(le16_to_cpu(desc->flag))) {
> > + do {
> > + if (hclgevf_cmd_csq_done(hw))
> > + break;
> > + udelay(1);
> > + timeout++;
> > + } while (timeout < hw->cmq.tx_timeout);
> > + }
>
> This is potentially a long timeout with a spinlock held. Consider
> using
> sleeping and completions.
I guess you meant 1 usec X (hw->cmq.tx_timeout = 200) times the delay
is large - right?
Will review it again.
Thanks.
Salil
^ permalink raw reply
* Re: [PATCH net-next 4/4] qede: Use NETIF_F_GRO_HW.
From: Michael Chan @ 2017-12-05 17:13 UTC (permalink / raw)
To: Chopra, Manish
Cc: Yuval Mintz, davem@davemloft.net, netdev@vger.kernel.org,
Elior, Ariel, Dept-Eng Everest Linux L2
In-Reply-To: <BN3PR0701MB1412F0BAAFA95465F32BB257893D0@BN3PR0701MB1412.namprd07.prod.outlook.com>
On Tue, Dec 5, 2017 at 4:32 AM, Chopra, Manish <Manish.Chopra@cavium.com> wrote:
>
> Since we are now differentiating HW gro with distinct feature bit, I think we should consider this feature bit everywhere where driver disables
> HW gro internally [not explicitly using ethtool]. This is not just specific to XDP but on some other conditions also driver disables HW gro in load flow.
> I think with just this change we would end up with showing HW gro feature enabled but actually driver has disabled it in XDP or other scenarios internally.
>
> I don't know if it's a good thing to do but just a suggestion -
> What if in driver load flow that is in the end of function qede_alloc_mem_rxq() we do something like below so that it will actually represent
> the actual state of the feature ?
>
> If (edev->gro_disable) {
> ndev->hw_features &= ~ NETIF_F_GRO_HW;
> ndev->features &= ~ NETIF_F_GRO_HW;
> }
If edev->gro_disable means that the current configuration cannot
support GRO_HW, then this makes sense. But I think it is best to
centralize this logic in ndo_fix_features(). The driver can then call
netdev_update_features() to update all features whenever there are
changes that can affect features.
^ permalink raw reply
* Re: [RFC v2 6/6] flow_dissector: Parse batman-adv unicast headers
From: Tom Herbert @ 2017-12-05 17:19 UTC (permalink / raw)
To: Sven Eckelmann
Cc: Linux Kernel Network Developers, David S . Miller, Jiri Pirko,
Eric Dumazet, LKML, b.a.t.m.a.n
In-Reply-To: <20171205143514.4441-7-sven.eckelmann@openmesh.com>
On Tue, Dec 5, 2017 at 6:35 AM, Sven Eckelmann
<sven.eckelmann@openmesh.com> wrote:
> The batman-adv unicast packets contain a full layer 2 frame in encapsulated
> form. The flow dissector must therefore be able to parse the batman-adv
> unicast header to reach the layer 2+3 information.
>
> +--------------------+
> | ip(v6)hdr |
> +--------------------+
> | inner ethhdr |
> +--------------------+
> | batadv unicast hdr |
> +--------------------+
> | outer ethhdr |
> +--------------------+
>
> The obtained information from the upper layer can then be used by RPS to
> schedule the processing on separate cores. This allows better distribution
> of multiple flows from the same neighbor to different cores.
>
> Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
> ---
> net/core/flow_dissector.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 15ce30063765..784cc07fc58e 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -24,6 +24,7 @@
> #include <linux/tcp.h>
> #include <net/flow_dissector.h>
> #include <scsi/fc/fc_fcoe.h>
> +#include <uapi/linux/batadv.h>
>
> static void dissector_set_key(struct flow_dissector *flow_dissector,
> enum flow_dissector_key_id key_id)
> @@ -696,6 +697,35 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>
> break;
> }
> + case htons(ETH_P_BATMAN): {
> + struct {
> + struct batadv_unicast_packet batadv_unicast;
> + struct ethhdr eth;
> + } *hdr, _hdr;
> +
> + hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen,
> + &_hdr);
> + if (!hdr) {
> + fdret = FLOW_DISSECT_RET_OUT_BAD;
> + break;
> + }
> +
> + if (hdr->batadv_unicast.version != BATADV_COMPAT_VERSION) {
> + fdret = FLOW_DISSECT_RET_OUT_BAD;
> + break;
> + }
> +
> + if (hdr->batadv_unicast.packet_type != BATADV_UNICAST) {
> + fdret = FLOW_DISSECT_RET_OUT_BAD;
> + break;
> + }
> +
> + proto = hdr->eth.h_proto;
> + nhoff += sizeof(*hdr);
> +
> + fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> + break;
> + }
> case htons(ETH_P_8021AD):
> case htons(ETH_P_8021Q): {
> const struct vlan_hdr *vlan;
> --
> 2.11.0
>
Switch statements with cases having many LOC is hard to read and
__skb_flow_dissect is aleady quite convoluted to begin with.
I suggest putting this in a static function similar to how MPLS and
GRE are handled.
Tom
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 05/10] ixgbe: implement ipsec add and remove of offloaded SA
From: Alexander Duyck @ 2017-12-05 17:26 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1512452116-14795-6-git-send-email-shannon.nelson@oracle.com>
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> Add the functions for setting up and removing offloaded SAs (Security
> Associations) with the x540 hardware. We set up the callback structure
> but we don't yet set the hardware feature bit to be sure the XFRM service
> won't actually try to use us for an offload yet.
>
> The software tables are made up to mimic the hardware tables to make it
> easier to track what's in the hardware, and the SA table index is used
> for the XFRM offload handle. However, there is a hashing field in the
> Rx SA tracking that will be used to facilitate faster table searches in
> the Rx fast path.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 377 +++++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +
> 2 files changed, 383 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 38a1a16..7b01d92 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -26,6 +26,8 @@
> ******************************************************************************/
>
> #include "ixgbe.h"
> +#include <net/xfrm.h>
> +#include <crypto/aead.h>
>
> /**
> * ixgbe_ipsec_set_tx_sa - set the Tx SA registers
> @@ -128,6 +130,7 @@ static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32 addr[])
> **/
> void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
> {
> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
> struct ixgbe_hw *hw = &adapter->hw;
> u32 buf[4] = {0, 0, 0, 0};
> u16 idx;
> @@ -139,9 +142,11 @@ void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
> /* scrub the tables */
> for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
> ixgbe_ipsec_set_tx_sa(hw, idx, buf, 0);
> + ipsec->num_tx_sa = 0;
>
> for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
> ixgbe_ipsec_set_rx_sa(hw, idx, 0, buf, 0, 0, 0);
> + ipsec->num_rx_sa = 0;
>
> for (idx = 0; idx < IXGBE_IPSEC_MAX_RX_IP_COUNT; idx++)
> ixgbe_ipsec_set_rx_ip(hw, idx, buf);
> @@ -287,11 +292,383 @@ static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
> }
>
> /**
> + * ixgbe_ipsec_find_empty_idx - find the first unused security parameter index
> + * @ipsec: pointer to ipsec struct
> + * @rxtable: true if we need to look in the Rx table
> + *
> + * Returns the first unused index in either the Rx or Tx SA table
> + **/
> +static int ixgbe_ipsec_find_empty_idx(struct ixgbe_ipsec *ipsec, bool rxtable)
> +{
> + u32 i;
> +
> + if (rxtable) {
> + if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
> + return -ENOSPC;
> +
> + /* search rx sa table */
> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
> + if (!ipsec->rx_tbl[i].used)
> + return i;
> + }
> + } else {
> + if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
> + return -ENOSPC;
Should this bi num_tx_sa?
> +
> + /* search tx sa table */
> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
> + if (!ipsec->tx_tbl[i].used)
> + return i;
> + }
> + }
> +
> + return -ENOSPC;
> +}
> +
> +/**
> + * ixgbe_ipsec_parse_proto_keys - find the key and salt based on the protocol
> + * @xs: pointer to xfrm_state struct
> + * @mykey: pointer to key array to populate
> + * @mysalt: pointer to salt value to populate
> + *
> + * This copies the protocol keys and salt to our own data tables. The
> + * 82599 family only supports the one algorithm.
> + **/
> +static int ixgbe_ipsec_parse_proto_keys(struct xfrm_state *xs,
> + u32 *mykey, u32 *mysalt)
> +{
> + struct net_device *dev = xs->xso.dev;
> + unsigned char *key_data;
> + char *alg_name = NULL;
> + char *aes_gcm_name = "rfc4106(gcm(aes))";
aes_gcm_name should probably be a static const char array instead of a pointer.
> + int key_len;
> +
> + if (xs->aead) {
> + key_data = &xs->aead->alg_key[0];
> + key_len = xs->aead->alg_key_len;
> + alg_name = xs->aead->alg_name;
> + } else {
> + netdev_err(dev, "Unsupported IPsec algorithm\n");
> + return -EINVAL;
> + }
> +
> + if (strcmp(alg_name, aes_gcm_name)) {
> + netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
> + aes_gcm_name);
> + return -EINVAL;
> + }
> +
> + /* 160 accounts for 16 byte key and 4 byte salt */
> + if (key_len == 128) {
> + netdev_info(dev, "IPsec hw offload parameters missing 32 bit salt value\n");
> + } else if (key_len != 160) {
> + netdev_err(dev, "IPsec hw offload only supports keys up to 128 bits with a 32 bit salt\n");
> + return -EINVAL;
> + }
> +
> + /* The key bytes come down in a bigendian array of bytes, and
> + * salt is always the last 4 bytes of the key array.
> + * We don't need to do any byteswapping.
> + */
> + memcpy(mykey, key_data, 16);
> + if (key_len == 160)
> + *mysalt = ((u32 *)key_data)[4];
> + else
> + *mysalt = 0;
You could combine these key_len checks into a single if/else set.
Basically just do something like the following:
/* 160 accounts for 16 byte key and 4 byte salt */
if (key_len == 160) {
*mysalt = ((u32 *)key_data)[4];
} else if (key_len != 128) {
netdev_err(dev, "IPsec hw offload only supports keys up to 128
bits with a 32 bit salt\n");
return -EINVAL;
} else {
netdev_info(dev, "IPsec hw offload parameters missing 32 bit
salt value\n");
*mysalt = 0;
}
/* The key bytes come down in a bigendian array of bytes, and
* salt is always the last 4 bytes of the key array.
* We don't need to do any byteswapping.
*/
memcpy(mykey, key_data, 16);
> +
> + return 0;
> +}
> +
> +/**
> + * ixgbe_ipsec_add_sa - program device with a security association
> + * @xs: pointer to transformer state struct
> + **/
> +static int ixgbe_ipsec_add_sa(struct xfrm_state *xs)
> +{
> + struct net_device *dev = xs->xso.dev;
> + struct ixgbe_adapter *adapter = netdev_priv(dev);
> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
> + struct ixgbe_hw *hw = &adapter->hw;
> + int checked, match, first;
> + u16 sa_idx;
> + int ret;
> + int i;
> +
> + if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
> + netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
> + xs->id.proto);
> + return -EINVAL;
> + }
> +
> + if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
> + struct rx_sa rsa;
> +
> + if (xs->calg) {
> + netdev_err(dev, "Compression offload not supported\n");
> + return -EINVAL;
> + }
> +
> + /* find the first unused index */
> + ret = ixgbe_ipsec_find_empty_idx(ipsec, true);
> + if (ret < 0) {
> + netdev_err(dev, "No space for SA in Rx table!\n");
> + return ret;
> + }
> + sa_idx = (u16)ret;
> +
> + memset(&rsa, 0, sizeof(rsa));
> + rsa.used = true;
> + rsa.xs = xs;
> +
> + if (rsa.xs->id.proto & IPPROTO_ESP)
> + rsa.decrypt = xs->ealg || xs->aead;
> +
> + /* get the key and salt */
> + ret = ixgbe_ipsec_parse_proto_keys(xs, rsa.key, &rsa.salt);
> + if (ret) {
> + netdev_err(dev, "Failed to get key data for Rx SA table\n");
> + return ret;
> + }
> +
> + /* get ip for rx sa table */
> + if (xs->xso.flags & XFRM_OFFLOAD_IPV6)
> + memcpy(rsa.ipaddr, &xs->id.daddr.a6, 16);
> + else
> + memcpy(&rsa.ipaddr[3], &xs->id.daddr.a4, 4);
> +
> + /* The HW does not have a 1:1 mapping from keys to IP addrs, so
> + * check for a matching IP addr entry in the table. If the addr
> + * already exists, use it; else find an unused slot and add the
> + * addr. If one does not exist and there are no unused table
> + * entries, fail the request.
> + */
> +
> + /* Find an existing match or first not used, and stop looking
> + * after we've checked all we know we have.
> + */
> + checked = 0;
> + match = -1;
> + first = -1;
> + for (i = 0;
> + i < IXGBE_IPSEC_MAX_RX_IP_COUNT &&
> + (checked < ipsec->num_rx_sa || first < 0);
> + i++) {
> + if (ipsec->ip_tbl[i].used) {
> + if (!memcmp(ipsec->ip_tbl[i].ipaddr,
> + rsa.ipaddr, sizeof(rsa.ipaddr))) {
> + match = i;
> + break;
> + }
> + checked++;
> + } else if (first < 0) {
> + first = i; /* track the first empty seen */
> + }
> + }
> +
> + if (ipsec->num_rx_sa == 0)
> + first = 0;
> +
> + if (match >= 0) {
> + /* addrs are the same, we should use this one */
> + rsa.iptbl_ind = match;
> + ipsec->ip_tbl[match].ref_cnt++;
> +
> + } else if (first >= 0) {
> + /* no matches, but here's an empty slot */
> + rsa.iptbl_ind = first;
> +
> + memcpy(ipsec->ip_tbl[first].ipaddr,
> + rsa.ipaddr, sizeof(rsa.ipaddr));
> + ipsec->ip_tbl[first].ref_cnt = 1;
> + ipsec->ip_tbl[first].used = true;
> +
> + ixgbe_ipsec_set_rx_ip(hw, rsa.iptbl_ind, rsa.ipaddr);
> +
> + } else {
> + /* no match and no empty slot */
> + netdev_err(dev, "No space for SA in Rx IP SA table\n");
> + memset(&rsa, 0, sizeof(rsa));
> + return -ENOSPC;
> + }
> +
> + rsa.mode = IXGBE_RXMOD_VALID;
> + if (rsa.xs->id.proto & IPPROTO_ESP)
> + rsa.mode |= IXGBE_RXMOD_PROTO_ESP;
> + if (rsa.decrypt)
> + rsa.mode |= IXGBE_RXMOD_DECRYPT;
> + if (rsa.xs->xso.flags & XFRM_OFFLOAD_IPV6)
> + rsa.mode |= IXGBE_RXMOD_IPV6;
> +
> + /* the preparations worked, so save the info */
> + memcpy(&ipsec->rx_tbl[sa_idx], &rsa, sizeof(rsa));
> +
> + ixgbe_ipsec_set_rx_sa(hw, sa_idx, rsa.xs->id.spi, rsa.key,
> + rsa.salt, rsa.mode, rsa.iptbl_ind);
> + xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_RX_INDEX;
> +
> + ipsec->num_rx_sa++;
> +
> + /* hash the new entry for faster search in Rx path */
> + hash_add_rcu(ipsec->rx_sa_list, &ipsec->rx_tbl[sa_idx].hlist,
> + rsa.xs->id.spi);
> + } else {
> + struct tx_sa tsa;
> +
> + /* find the first unused index */
> + ret = ixgbe_ipsec_find_empty_idx(ipsec, false);
> + if (ret < 0) {
> + netdev_err(dev, "No space for SA in Tx table\n");
> + return ret;
> + }
> + sa_idx = (u16)ret;
> +
> + memset(&tsa, 0, sizeof(tsa));
> + tsa.used = true;
> + tsa.xs = xs;
> +
> + if (xs->id.proto & IPPROTO_ESP)
> + tsa.encrypt = xs->ealg || xs->aead;
> +
> + ret = ixgbe_ipsec_parse_proto_keys(xs, tsa.key, &tsa.salt);
> + if (ret) {
> + netdev_err(dev, "Failed to get key data for Tx SA table\n");
> + memset(&tsa, 0, sizeof(tsa));
> + return ret;
> + }
> +
> + /* the preparations worked, so save the info */
> + memcpy(&ipsec->tx_tbl[sa_idx], &tsa, sizeof(tsa));
> +
> + ixgbe_ipsec_set_tx_sa(hw, sa_idx, tsa.key, tsa.salt);
> +
> + xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_TX_INDEX;
> +
> + ipsec->num_tx_sa++;
> + }
> +
> + /* enable the engine if not already warmed up */
> + if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED)) {
> + ixgbe_ipsec_start_engine(adapter);
> + adapter->flags2 |= IXGBE_FLAG2_IPSEC_ENABLED;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ixgbe_ipsec_del_sa - clear out this specific SA
> + * @xs: pointer to transformer state struct
> + **/
> +static void ixgbe_ipsec_del_sa(struct xfrm_state *xs)
> +{
> + struct net_device *dev = xs->xso.dev;
> + struct ixgbe_adapter *adapter = netdev_priv(dev);
> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
> + struct ixgbe_hw *hw = &adapter->hw;
> + u32 zerobuf[4] = {0, 0, 0, 0};
> + u16 sa_idx;
> +
> + if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
> + struct rx_sa *rsa;
> + u8 ipi;
> +
> + sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_RX_INDEX;
> + rsa = &ipsec->rx_tbl[sa_idx];
> +
> + if (!rsa->used) {
> + netdev_err(dev, "Invalid Rx SA selected sa_idx=%d offload_handle=%lu\n",
> + sa_idx, xs->xso.offload_handle);
> + return;
> + }
> +
> + ixgbe_ipsec_set_rx_sa(hw, sa_idx, 0, zerobuf, 0, 0, 0);
> + hash_del_rcu(&rsa->hlist);
> +
> + /* if the IP table entry is referenced by only this SA,
> + * i.e. ref_cnt is only 1, clear the IP table entry as well
> + */
> + ipi = rsa->iptbl_ind;
> + if (ipsec->ip_tbl[ipi].ref_cnt > 0) {
> + ipsec->ip_tbl[ipi].ref_cnt--;
> +
> + if (!ipsec->ip_tbl[ipi].ref_cnt) {
> + memset(&ipsec->ip_tbl[ipi], 0,
> + sizeof(struct rx_ip_sa));
> + ixgbe_ipsec_set_rx_ip(hw, ipi, zerobuf);
> + }
> + }
> +
> + memset(rsa, 0, sizeof(struct rx_sa));
> + ipsec->num_rx_sa--;
> + } else {
> + sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
> +
> + if (!ipsec->tx_tbl[sa_idx].used) {
> + netdev_err(dev, "Invalid Tx SA selected sa_idx=%d offload_handle=%lu\n",
> + sa_idx, xs->xso.offload_handle);
> + return;
> + }
> +
> + ixgbe_ipsec_set_tx_sa(hw, sa_idx, zerobuf, 0);
> + memset(&ipsec->tx_tbl[sa_idx], 0, sizeof(struct tx_sa));
> + ipsec->num_tx_sa--;
> + }
> +
> + /* if there are no SAs left, stop the engine to save energy */
> + if (ipsec->num_rx_sa == 0 && ipsec->num_tx_sa == 0) {
> + adapter->flags2 &= ~IXGBE_FLAG2_IPSEC_ENABLED;
> + ixgbe_ipsec_stop_engine(adapter);
> + }
> +}
> +
> +static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
> + .xdo_dev_state_add = ixgbe_ipsec_add_sa,
> + .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
> +};
> +
> +/**
> * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
> * @adapter: board private structure
> **/
> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
> {
> + struct ixgbe_ipsec *ipsec;
> + size_t size;
> +
> + ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL);
> + if (!ipsec)
> + goto err;
I would say just add another label to skip over the if statement you
added below.
> + hash_init(ipsec->rx_sa_list);
> +
> + size = sizeof(struct rx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
> + ipsec->rx_tbl = kzalloc(size, GFP_KERNEL);
> + if (!ipsec->rx_tbl)
> + goto err;
> +
> + size = sizeof(struct tx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
> + ipsec->tx_tbl = kzalloc(size, GFP_KERNEL);
> + if (!ipsec->tx_tbl)
> + goto err;
> +
> + size = sizeof(struct rx_ip_sa) * IXGBE_IPSEC_MAX_RX_IP_COUNT;
> + ipsec->ip_tbl = kzalloc(size, GFP_KERNEL);
> + if (!ipsec->ip_tbl)
> + goto err;
Do all these tables need to be allocated separately? I'm just
wondering if we can get away with doing something like what we did
with the ixgbe_q_vector structure where you just allocate this as one
physical block of memory and just split it up into multiple chunks
with a separate pointer to each chunk. Doing that would cut down on
the exception handling needed since it would be a single allocation
failure you would have to deal with.
> + ipsec->num_rx_sa = 0;
> + ipsec->num_tx_sa = 0;
> +
> + adapter->ipsec = ipsec;
> ixgbe_ipsec_clear_hw_tables(adapter);
> ixgbe_ipsec_stop_engine(adapter);
> +
> + return;
> +err:
> + if (ipsec) {
> + kfree(ipsec->ip_tbl);
> + kfree(ipsec->rx_tbl);
> + kfree(ipsec->tx_tbl);
> + kfree(adapter->ipsec);
> + }
> + netdev_err(adapter->netdev, "Unable to allocate memory for SA tables");
> }
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 51fb3cf..01fd89b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -10542,6 +10542,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
> set_bit(__IXGBE_REMOVING, &adapter->state);
> cancel_work_sync(&adapter->service_task);
>
> +#ifdef CONFIG_XFRM
> + kfree(adapter->ipsec->ip_tbl);
> + kfree(adapter->ipsec->rx_tbl);
> + kfree(adapter->ipsec->tx_tbl);
> + kfree(adapter->ipsec);
> +#endif /* CONFIG_XFRM */
It might be useful if you were to move this into a function of its
own. Also you should probably check for adapter->ipsec first,
otherwise you are going to cause NULL pointer dereference any time
adapter->ipsec isn't defined. because you are dereferencing it when
you go to free each of those tables.
>
> #ifdef CONFIG_IXGBE_DCA
> if (adapter->flags & IXGBE_FLAG_DCA_ENABLED) {
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [PATCH net-next 04/12] sctp: implement make_datafrag for sctp_stream_interleave
From: Marcelo Ricardo Leitner @ 2017-12-05 17:26 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem
In-Reply-To: <b8551388363da7b56f7e56132414c1399557a12d.1512486606.git.lucien.xin@gmail.com>
On Tue, Dec 05, 2017 at 11:16:01PM +0800, Xin Long wrote:
> To avoid hundreds of checks for the different process on I-DATA chunk,
> struct sctp_stream_interleave is defined as a group of functions used
> to replace the codes in some place where it needs to do different job
> according to if the asoc intl_enabled is set.
>
> With these ops, it only needs to initialize asoc->stream.si with
> sctp_stream_interleave_0 for normal data if asoc intl_enable is 0,
> or sctp_stream_interleave_1 for idata if asoc intl_enable is set in
> sctp_stream_init.
>
> After that, the members in asoc->stream.si can be used directly in
> some special places without checking asoc intl_enable.
>
> make_datafrag is the first member for sctp_stream_interleave, it's
> used to make data or idata frags, called in sctp_datamsg_from_user.
> The old function sctp_make_datafrag_empty needs to be adjust some
> to fit in this ops.
>
> Note that as idata and data chunks have different length, it also
> defines data_chunk_len for sctp_stream_interleave to describe the
> chunk size.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> include/net/sctp/sm.h | 5 +--
> include/net/sctp/stream_interleave.h | 44 ++++++++++++++++++++
> include/net/sctp/structs.h | 11 +++++
> net/sctp/Makefile | 2 +-
> net/sctp/chunk.c | 6 +--
> net/sctp/sm_make_chunk.c | 21 ++++------
> net/sctp/stream.c | 1 +
> net/sctp/stream_interleave.c | 79 ++++++++++++++++++++++++++++++++++++
> 8 files changed, 148 insertions(+), 21 deletions(-)
> create mode 100644 include/net/sctp/stream_interleave.h
> create mode 100644 net/sctp/stream_interleave.c
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 5389ae0..f950186 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -199,10 +199,9 @@ struct sctp_chunk *sctp_make_cwr(const struct sctp_association *asoc,
> const struct sctp_chunk *chunk);
> struct sctp_chunk *sctp_make_idata(const struct sctp_association *asoc,
> __u8 flags, int paylen, gfp_t gfp);
> -struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc,
> +struct sctp_chunk *sctp_make_datafrag_empty(const struct sctp_association *asoc,
> const struct sctp_sndrcvinfo *sinfo,
> - int len, const __u8 flags,
> - __u16 ssn, gfp_t gfp);
> + int len, __u8 flags, gfp_t gfp);
> struct sctp_chunk *sctp_make_ecne(const struct sctp_association *asoc,
> const __u32 lowest_tsn);
> struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc);
> diff --git a/include/net/sctp/stream_interleave.h b/include/net/sctp/stream_interleave.h
> new file mode 100644
> index 0000000..7b9fa8d
> --- /dev/null
> +++ b/include/net/sctp/stream_interleave.h
> @@ -0,0 +1,44 @@
> +/* SCTP kernel implementation
> + * (C) Copyright Red Hat Inc. 2017
> + *
> + * These are definitions used by the stream schedulers, defined in RFC
> + * draft ndata (https://tools.ietf.org/html/draft-ietf-tsvwg-sctp-ndata-11)
> + *
> + * This SCTP implementation is free software;
> + * you can redistribute it and/or modify it under the terms of
> + * the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This SCTP implementation is distributed in the hope that it
> + * will be useful, but WITHOUT ANY WARRANTY; without even the implied
> + * ************************
> + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GNU CC; see the file COPYING. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Please send any bug reports or fixes you make to the
> + * email addresses:
> + * lksctp developers <linux-sctp@vger.kernel.org>
> + *
> + * Written or modified by:
> + * Xin Long <lucien.xin@gmail.com>
> + */
> +
> +#ifndef __sctp_stream_interleave_h__
> +#define __sctp_stream_interleave_h__
> +
> +struct sctp_stream_interleave {
> + __u16 data_chunk_len;
> + /* (I-)DATA process */
> + struct sctp_chunk *(*make_datafrag)(const struct sctp_association *asoc,
> + const struct sctp_sndrcvinfo *sinfo,
> + int len, __u8 flags, gfp_t gfp);
> +};
> +
> +void sctp_stream_interleave_init(struct sctp_stream *stream);
> +
> +#endif /* __sctp_stream_interleave_h__ */
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 15183bf..ae7225b 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -89,6 +89,7 @@ struct sctp_stream;
> #include <net/sctp/tsnmap.h>
> #include <net/sctp/ulpevent.h>
> #include <net/sctp/ulpqueue.h>
> +#include <net/sctp/stream_interleave.h>
>
> /* Structures useful for managing bind/connect. */
>
> @@ -1388,11 +1389,21 @@ struct sctp_stream {
> struct sctp_stream_out_ext *rr_next;
> };
> };
> + struct sctp_stream_interleave *si;
> };
>
> #define SCTP_STREAM_CLOSED 0x00
> #define SCTP_STREAM_OPEN 0x01
>
> +static inline __u16 sctp_datachk_len(const struct sctp_stream *stream)
> +{
> + return stream->si->data_chunk_len;
> +}
checkpatch complained about a missing blank line here.
> +static inline __u16 sctp_datahdr_len(const struct sctp_stream *stream)
> +{
> + return stream->si->data_chunk_len - sizeof(struct sctp_chunkhdr);
> +}
> +
> /* SCTP_GET_ASSOC_STATS counters */
> struct sctp_priv_assoc_stats {
> /* Maximum observed rto in the association during subsequent
> diff --git a/net/sctp/Makefile b/net/sctp/Makefile
> index 1ca84a2..54bd9c1 100644
> --- a/net/sctp/Makefile
> +++ b/net/sctp/Makefile
> @@ -14,7 +14,7 @@ sctp-y := sm_statetable.o sm_statefuns.o sm_sideeffect.o \
> tsnmap.o bind_addr.o socket.o primitive.o \
> output.o input.o debug.o stream.o auth.o \
> offload.o stream_sched.o stream_sched_prio.o \
> - stream_sched_rr.o
> + stream_sched_rr.o stream_interleave.o
>
> sctp_probe-y := probe.o
>
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index 7b261af..bded6af 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -190,7 +190,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> */
> max_data = asoc->pathmtu -
> sctp_sk(asoc->base.sk)->pf->af->net_header_len -
> - sizeof(struct sctphdr) - sizeof(struct sctp_data_chunk);
> + sizeof(struct sctphdr) - sctp_datachk_len(&asoc->stream);
> max_data = SCTP_TRUNC4(max_data);
>
> /* If the the peer requested that we authenticate DATA chunks
> @@ -263,8 +263,8 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> frag |= SCTP_DATA_SACK_IMM;
> }
>
> - chunk = sctp_make_datafrag_empty(asoc, sinfo, len, frag,
> - 0, GFP_KERNEL);
> + chunk = asoc->stream.si->make_datafrag(asoc, sinfo, len, frag,
> + GFP_KERNEL);
> if (!chunk) {
> err = -ENOMEM;
> goto errout;
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index b969397..23a7313 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -721,38 +721,31 @@ struct sctp_chunk *sctp_make_ecne(const struct sctp_association *asoc,
> /* Make a DATA chunk for the given association from the provided
> * parameters. However, do not populate the data payload.
> */
> -struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc,
> +struct sctp_chunk *sctp_make_datafrag_empty(const struct sctp_association *asoc,
> const struct sctp_sndrcvinfo *sinfo,
> - int data_len, __u8 flags, __u16 ssn,
> - gfp_t gfp)
> + int len, __u8 flags, gfp_t gfp)
> {
> struct sctp_chunk *retval;
> struct sctp_datahdr dp;
> - int chunk_len;
>
> /* We assign the TSN as LATE as possible, not here when
> * creating the chunk.
> */
> - dp.tsn = 0;
> + memset(&dp, 0, sizeof(dp));
> + dp.ppid = sinfo->sinfo_ppid;
> dp.stream = htons(sinfo->sinfo_stream);
> - dp.ppid = sinfo->sinfo_ppid;
>
> /* Set the flags for an unordered send. */
> - if (sinfo->sinfo_flags & SCTP_UNORDERED) {
> + if (sinfo->sinfo_flags & SCTP_UNORDERED)
> flags |= SCTP_DATA_UNORDERED;
> - dp.ssn = 0;
> - } else
> - dp.ssn = htons(ssn);
>
> - chunk_len = sizeof(dp) + data_len;
> - retval = sctp_make_data(asoc, flags, chunk_len, gfp);
> + retval = sctp_make_data(asoc, flags, sizeof(dp) + len, gfp);
> if (!retval)
> - goto nodata;
> + return NULL;
>
> retval->subh.data_hdr = sctp_addto_chunk(retval, sizeof(dp), &dp);
> memcpy(&retval->sinfo, sinfo, sizeof(struct sctp_sndrcvinfo));
>
> -nodata:
> return retval;
> }
>
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 76ea66b..8370e6c 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -167,6 +167,7 @@ int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt,
> sched->init(stream);
>
> in:
> + sctp_stream_interleave_init(stream);
> if (!incnt)
> goto out;
>
> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> new file mode 100644
> index 0000000..397c3c1
> --- /dev/null
> +++ b/net/sctp/stream_interleave.c
> @@ -0,0 +1,79 @@
> +/* SCTP kernel implementation
> + * (C) Copyright Red Hat Inc. 2017
> + *
> + * This file is part of the SCTP kernel implementation
> + *
> + * These functions manipulate sctp stream queue/scheduling.
> + *
> + * This SCTP implementation is free software;
> + * you can redistribute it and/or modify it under the terms of
> + * the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This SCTP implementation is distributed in the hope that it
> + * will be useful, but WITHOUT ANY WARRANTY; without even the implied
> + * ************************
> + * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GNU CC; see the file COPYING. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Please send any bug reports or fixes you make to the
> + * email addresched(es):
> + * lksctp developers <linux-sctp@vger.kernel.org>
> + *
> + * Written or modified by:
> + * Xin Long <lucien.xin@gmail.com>
> + */
> +
> +#include <net/sctp/sctp.h>
> +#include <net/sctp/sm.h>
> +#include <linux/sctp.h>
> +
> +static struct sctp_chunk *sctp_make_idatafrag_empty(
And that the line shouldn't end with the (
but the parameter is big.. I don't see a good way here.
> + const struct sctp_association *asoc,
> + const struct sctp_sndrcvinfo *sinfo,
> + int len, __u8 flags, gfp_t gfp)
> +{
> + struct sctp_chunk *retval;
> + struct sctp_idatahdr dp;
> +
> + memset(&dp, 0, sizeof(dp));
> + dp.stream = htons(sinfo->sinfo_stream);
> +
> + if (sinfo->sinfo_flags & SCTP_UNORDERED)
> + flags |= SCTP_DATA_UNORDERED;
> +
> + retval = sctp_make_idata(asoc, flags, sizeof(dp) + len, gfp);
> + if (!retval)
> + return NULL;
> +
> + retval->subh.idata_hdr = sctp_addto_chunk(retval, sizeof(dp), &dp);
> + memcpy(&retval->sinfo, sinfo, sizeof(struct sctp_sndrcvinfo));
> +
> + return retval;
> +}
> +
> +static struct sctp_stream_interleave sctp_stream_interleave_0 = {
> + .data_chunk_len = sizeof(struct sctp_data_chunk),
> + /* DATA process functions */
> + .make_datafrag = sctp_make_datafrag_empty,
> +};
> +
> +static struct sctp_stream_interleave sctp_stream_interleave_1 = {
> + .data_chunk_len = sizeof(struct sctp_idata_chunk),
> + /* I-DATA process functions */
> + .make_datafrag = sctp_make_idatafrag_empty,
> +};
> +
> +void sctp_stream_interleave_init(struct sctp_stream *stream)
> +{
> + struct sctp_association *asoc;
> +
> + asoc = container_of(stream, struct sctp_association, stream);
> + stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
> + : &sctp_stream_interleave_0;
> +}
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support
From: Andrew Lunn @ 2017-12-05 17:28 UTC (permalink / raw)
To: Richard Leitner
Cc: robh+dt, mark.rutland, fugang.duan, f.fainelli, frowand.list,
davem, geert+renesas, sergei.shtylyov, baruch, david.wu, lukma,
netdev, devicetree, linux-kernel, richard.leitner
In-Reply-To: <20171205132600.13796-2-dev@g0hl1n.net>
Hi Richard
> +++ b/drivers/of/of_mdio.c
> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
> if (of_property_read_bool(child, "broken-turn-around"))
> mdio->phy_ignore_ta_mask |= 1 << addr;
>
> + if (of_property_read_u32(child, "reset-delay-us",
> + &phy->mdio.reset_delay))
> + phy->mdio.reset_delay = 0;
> +
> + if (of_property_read_u32(child, "reset-post-delay-us",
> + &phy->mdio.reset_post_delay))
> + phy->mdio.reset_post_delay = 0;
of_property_read_u32() should not change the variable you pass to it,
if it does not find the property. So you can change this to:
phy->mdio.reset_delay = 0;
phy->mdio.reset_post_delay = 0;
of_property_read_u32(child, "reset-delay-us",
&phy->mdio.reset_delay);
of_property_read_u32(child, "reset-post-delay-us",
&phy->mdio.reset_post_delay);
Andrew
^ permalink raw reply
* Re: [PATCH net-next 00/12] sctp: Implement Stream Interleave: The I-DATA Chunk Supporting User Message Interleaving
From: Marcelo Ricardo Leitner @ 2017-12-05 17:30 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem
In-Reply-To: <cover.1512486606.git.lucien.xin@gmail.com>
On Tue, Dec 05, 2017 at 11:15:57PM +0800, Xin Long wrote:
> Stream Interleave would be Implemented in two Parts:
> 1. The I-DATA Chunk Supporting User Message Interleaving
> 2. Interaction with Other SCTP Extensions
>
I have reviewed this patchset a couple of times already before the
posting and other than the missing blank line (heh), it looks good to
me. Would ack it now but we'll need a respin for the newline.
Xin, please wait a bit before respining it. Maybe Neil and others have
more comments on it.
Thanks,
Marcelo
^ permalink raw reply
* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
From: Johannes Berg @ 2017-12-05 17:30 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, j, dsahern
In-Reply-To: <20171205.114106.1013322969674769159.davem@davemloft.net>
On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
>
> There is no reasonable interpretation for what that application is
> doing, so I think we can safely call that case as buggy.
>
> We are only trying to handle the situation where a U8 attribute
> is presented as a bonafide U32 or a correct U8.
>
> Does this make sense?
Well the application is buggy, but we don't really know in what way?
Perhaps somebody even did the equivalent of
nla_put_u32(ATTR, cpu_to_le32(x))
when they noticed it was broken on BE, and end up with a similar case
as I had above.
I don't think there's a good solution to this, applications must be
fixed anyhow. I'm just saying that I'd save the extra code and stay
compatible with applications as written today, even if they're now
broken on BE - and rely on the warning to fix it. Trying to fix it up
seems to have the potential to just break something else.
johannes
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Alexander Duyck @ 2017-12-05 17:30 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1512452116-14795-7-git-send-email-shannon.nelson@oracle.com>
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On a chip reset most of the table contents are lost, so must be
> restored. This scans the driver's ipsec tables and restores both
> the filled and empty table slots to their pre-reset values.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 53 ++++++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
> 3 files changed, 56 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 9487750..7e8bca7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -1009,7 +1009,9 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32 adv_reg, u32 lp_reg,
> u32 adv_sym, u32 adv_asm, u32 lp_sym, u32 lp_asm);
> #ifdef CONFIG_XFRM_OFFLOAD
> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
> #else
> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
> +static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) { };
> #endif /* CONFIG_XFRM_OFFLOAD */
> #endif /* _IXGBE_H_ */
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 7b01d92..b93ee7f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -292,6 +292,59 @@ static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
> }
>
> /**
> + * ixgbe_ipsec_restore - restore the ipsec HW settings after a reset
> + * @adapter: board private structure
> + **/
> +void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter)
> +{
> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
> + struct ixgbe_hw *hw = &adapter->hw;
> + u32 zbuf[4] = {0, 0, 0, 0};
zbuf should be a static const.
> + int i;
> +
> + if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED))
> + return;
> +
> + /* clean up the engine settings */
> + ixgbe_ipsec_stop_engine(adapter);
> +
> + /* start the engine */
> + ixgbe_ipsec_start_engine(adapter);
> +
> + /* reload the IP addrs */
> + for (i = 0; i < IXGBE_IPSEC_MAX_RX_IP_COUNT; i++) {
> + struct rx_ip_sa *ipsa = &ipsec->ip_tbl[i];
> +
> + if (ipsa->used)
> + ixgbe_ipsec_set_rx_ip(hw, i, ipsa->ipaddr);
> + else
> + ixgbe_ipsec_set_rx_ip(hw, i, zbuf);
If we are doing a restore do we actually need to write the zero
values? If we did a reset I thought you had a function that was going
through and zeroing everything out. If so this now becomes redundant.
> + }
> +
> + /* reload the Rx keys */
> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
> + struct rx_sa *rsa = &ipsec->rx_tbl[i];
> +
> + if (rsa->used)
> + ixgbe_ipsec_set_rx_sa(hw, i, rsa->xs->id.spi,
> + rsa->key, rsa->salt,
> + rsa->mode, rsa->iptbl_ind);
> + else
> + ixgbe_ipsec_set_rx_sa(hw, i, 0, zbuf, 0, 0, 0);
same here
> + }
> +
> + /* reload the Tx keys */
> + for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
> + struct tx_sa *tsa = &ipsec->tx_tbl[i];
> +
> + if (tsa->used)
> + ixgbe_ipsec_set_tx_sa(hw, i, tsa->key, tsa->salt);
> + else
> + ixgbe_ipsec_set_tx_sa(hw, i, zbuf, 0);
and here
> + }
> +}
> +
> +/**
> * ixgbe_ipsec_find_empty_idx - find the first unused security parameter index
> * @ipsec: pointer to ipsec struct
> * @rxtable: true if we need to look in the Rx table
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 01fd89b..6eabf92 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -5347,6 +5347,7 @@ static void ixgbe_configure(struct ixgbe_adapter *adapter)
>
> ixgbe_set_rx_mode(adapter->netdev);
> ixgbe_restore_vlan(adapter);
> + ixgbe_ipsec_restore(adapter);
>
> switch (hw->mac.type) {
> case ixgbe_mac_82599EB:
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [PATCH net-next v3 2/4] phylib: add reset after clk enable support
From: Andrew Lunn @ 2017-12-05 17:34 UTC (permalink / raw)
To: Richard Leitner
Cc: robh+dt, mark.rutland, fugang.duan, f.fainelli, frowand.list,
davem, geert+renesas, sergei.shtylyov, baruch, david.wu, lukma,
netdev, devicetree, linux-kernel, richard.leitner
In-Reply-To: <20171205132600.13796-3-dev@g0hl1n.net>
On Tue, Dec 05, 2017 at 02:25:58PM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
>
> Some PHYs need the refclk to be a continuous clock. Therefore they don't
> allow turning it off and on again during operation. Nonetheless such a
> clock switching is performed by some ETH drivers (namely FEC [1]) for
> power saving reasons. An example for an affected PHY is the
> SMSC/Microchip LAN8720 in "REF_CLK In Mode".
>
> In order to provide a uniform method to overcome this problem this patch
> adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
> function phy_reset_after_clk_enable() to the phylib. These should be
> used to trigger reset of the PHY after the refclk is switched on again.
>
> This patch depends on the "phylib: Add device reset GPIO support" patch
> submitted by Geert Uytterhoeven/Sergei Shtylyov [2].
>
> [1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> [2] https://patchwork.kernel.org/patch/10090149/
>
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
Hi Richard
Same comment about moving text below the ---
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
From: Andrew Lunn @ 2017-12-05 17:35 UTC (permalink / raw)
To: Richard Leitner
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
fugang.duan-3arQi8VN3Tc, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8,
baruch-NswTu9S1W3P6gbPvEgmw2w, david.wu-TNX95d0MmH7DzftRWevZcw,
lukma-ynQEQJNshbs, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
richard.leitner-WcANXNA0UjBBDgjK7y7TUQ
In-Reply-To: <20171205132600.13796-4-dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org>
On Tue, Dec 05, 2017 at 02:25:59PM +0100, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>
> The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
> datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
> Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
> ETH driver reset them after the REF_CLK is enabled.
>
> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/00002165B.pdf
>
> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 07/10] ixgbe: process the Rx ipsec offload
From: Alexander Duyck @ 2017-12-05 17:40 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <1512452116-14795-8-git-send-email-shannon.nelson@oracle.com>
On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> If the chip sees and decrypts an ipsec offload, set up the skb
> sp pointer with the ralated SA info. Since the chip is rude
> enough to keep to itself the table index it used for the
> decryption, we have to do our own table lookup, using the
> hash for speed.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 6 ++
> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 89 ++++++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +
> 3 files changed, 98 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 7e8bca7..77f07dc 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -1009,9 +1009,15 @@ s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32 adv_reg, u32 lp_reg,
> u32 adv_sym, u32 adv_asm, u32 lp_sym, u32 lp_asm);
> #ifdef CONFIG_XFRM_OFFLOAD
> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter);
> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
> + union ixgbe_adv_rx_desc *rx_desc,
> + struct sk_buff *skb);
> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter);
> #else
> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { };
> +static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
> + union ixgbe_adv_rx_desc *rx_desc,
> + struct sk_buff *skb) { };
> static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) { };
> #endif /* CONFIG_XFRM_OFFLOAD */
> #endif /* _IXGBE_H_ */
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index b93ee7f..fd06d9b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -379,6 +379,35 @@ static int ixgbe_ipsec_find_empty_idx(struct ixgbe_ipsec *ipsec, bool rxtable)
> }
>
> /**
> + * ixgbe_ipsec_find_rx_state - find the state that matches
> + * @ipsec: pointer to ipsec struct
> + * @daddr: inbound address to match
> + * @proto: protocol to match
> + * @spi: SPI to match
> + *
> + * Returns a pointer to the matching SA state information
> + **/
> +static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec *ipsec,
> + __be32 daddr, u8 proto,
> + __be32 spi)
> +{
> + struct rx_sa *rsa;
> + struct xfrm_state *ret = NULL;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist, spi)
> + if (spi == rsa->xs->id.spi &&
> + daddr == rsa->xs->id.daddr.a4 &&
> + proto == rsa->xs->id.proto) {
> + ret = rsa->xs;
> + xfrm_state_hold(ret);
> + break;
> + }
> + rcu_read_unlock();
> + return ret;
> +}
> +
You need to choose a bucket, not just walk through all buckets.
Otherwise you might as well have just used a linked list. You might
look at using something like jhash_3words to generate a hash which you
then use to choose the bucket.
> +/**
> * ixgbe_ipsec_parse_proto_keys - find the key and salt based on the protocol
> * @xs: pointer to xfrm_state struct
> * @mykey: pointer to key array to populate
> @@ -680,6 +709,66 @@ static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
> };
>
> /**
> + * ixgbe_ipsec_rx - decode ipsec bits from Rx descriptor
> + * @rx_ring: receiving ring
> + * @rx_desc: receive data descriptor
> + * @skb: current data packet
> + *
> + * Determine if there was an ipsec encapsulation noticed, and if so set up
> + * the resulting status for later in the receive stack.
> + **/
> +void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
> + union ixgbe_adv_rx_desc *rx_desc,
> + struct sk_buff *skb)
> +{
> + struct ixgbe_adapter *adapter = netdev_priv(rx_ring->netdev);
> + u16 pkt_info = le16_to_cpu(rx_desc->wb.lower.lo_dword.hs_rss.pkt_info);
> + u16 ipsec_pkt_types = IXGBE_RXDADV_PKTTYPE_IPSEC_AH |
> + IXGBE_RXDADV_PKTTYPE_IPSEC_ESP;
> + struct ixgbe_ipsec *ipsec = adapter->ipsec;
> + struct xfrm_offload *xo = NULL;
> + struct xfrm_state *xs = NULL;
> + struct iphdr *iph;
> + u8 *c_hdr;
> + __be32 spi;
> + u8 proto;
> +
> + /* we can assume no vlan header in the way, b/c the
> + * hw won't recognize the IPsec packet and anyway the
> + * currently vlan device doesn't support xfrm offload.
> + */
> + /* TODO: not supporting IPv6 yet */
> + iph = (struct iphdr *)(skb->data + ETH_HLEN);
> + c_hdr = (u8 *)iph + iph->ihl * 4;
> + switch (pkt_info & ipsec_pkt_types) {
> + case IXGBE_RXDADV_PKTTYPE_IPSEC_AH:
> + spi = ((struct ip_auth_hdr *)c_hdr)->spi;
> + proto = IPPROTO_AH;
> + break;
> + case IXGBE_RXDADV_PKTTYPE_IPSEC_ESP:
> + spi = ((struct ip_esp_hdr *)c_hdr)->spi;
> + proto = IPPROTO_ESP;
> + break;
> + default:
> + return;
> + }
> +
> + xs = ixgbe_ipsec_find_rx_state(ipsec, iph->daddr, proto, spi);
> + if (unlikely(!xs))
> + return;
> +
> + skb->sp = secpath_dup(skb->sp);
> + if (unlikely(!skb->sp))
> + return;
> +
> + skb->sp->xvec[skb->sp->len++] = xs;
> + skb->sp->olen++;
> + xo = xfrm_offload(skb);
> + xo->flags = CRYPTO_DONE;
> + xo->status = CRYPTO_SUCCESS;
> +}
> +
> +/**
> * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
> * @adapter: board private structure
> **/
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 6eabf92..60f9f2d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -1755,6 +1755,9 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
>
> skb_record_rx_queue(skb, rx_ring->queue_index);
>
> + if (ixgbe_test_staterr(rx_desc, IXGBE_RXDADV_STAT_SECP))
> + ixgbe_ipsec_rx(rx_ring, rx_desc, skb);
> +
> skb->protocol = eth_type_trans(skb, dev);
> }
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [PATCH net 1/2] netlink: add NLA_U8_BUGGY attribute type
From: David Miller @ 2017-12-05 17:40 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, netdev, j, dsahern
In-Reply-To: <1512495010.26976.24.camel@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 05 Dec 2017 18:30:10 +0100
> On Tue, 2017-12-05 at 11:41 -0500, David Miller wrote:
>>
>> There is no reasonable interpretation for what that application is
>> doing, so I think we can safely call that case as buggy.
>>
>> We are only trying to handle the situation where a U8 attribute
>> is presented as a bonafide U32 or a correct U8.
>>
>> Does this make sense?
>
> Well the application is buggy, but we don't really know in what way?
> Perhaps somebody even did the equivalent of
> nla_put_u32(ATTR, cpu_to_le32(x))
> when they noticed it was broken on BE, and end up with a similar case
> as I had above.
>
> I don't think there's a good solution to this, applications must be
> fixed anyhow. I'm just saying that I'd save the extra code and stay
> compatible with applications as written today, even if they're now
> broken on BE - and rely on the warning to fix it. Trying to fix it up
> seems to have the potential to just break something else.
You might be right.
Ok let's just go with the warning + existing behavior for now.
^ 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