* Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions
From: SF Markus Elfring @ 2016-09-28 16:38 UTC (permalink / raw)
To: Paul Bolle
Cc: gigaset307x-common, netdev, Karsten Keil, LKML, kernel-janitors,
Julia Lawall
In-Reply-To: <1475062635.16606.27.camel@tiscali.nl>
>> * Multiplications for the size determination of memory allocations
>> indicated that array data structures should be processed.
>> Thus use the corresponding function "kmalloc_array".
>
> Was the current code incorrect?
I suggest to use a safer interface for array allocations.
> What makes kmalloc_array() better?
1. How do you think about the safety checks that this function provides?
2. Will you be also affected by further software evolution here?
2016-07-26
mm: faster kmalloc_array(), kcalloc()
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=91c6a05f72a996bee5133e76374ab3ad7d3b9b72
> I'm not going to change code just because some checker suggests to do so.
The script "checkpatch.pl" can point information out like the following.
WARNING: Prefer kmalloc_array over kmalloc with multiply
>> This issue was detected by using the Coccinelle software.
>
> So? And which coccinelle script was actually used?
How do you think about to look into related information sources?
https://github.com/coccinelle/coccinelle/issues/81
Would you like to experiment any further with an excerpt?
@replacement1@
expression count, target;
type T;
@@
target =
- kmalloc(sizeof(T) * (count)
+ kmalloc_array(count, sizeof(T)
, ...);
@replacement2@
expression count, pointer, target;
@@
target =
- kmalloc(sizeof(*pointer) * (count)
+ kmalloc_array(count, sizeof(*pointer)
, ...);
> I couldn't spot a coccinelle script doing that in the current tree.
This is true for such a software update opportunity.
>> * Replace the specification of a data structure by a pointer dereference
>> to make the corresponding size determination a bit safer according to
>> the Linux coding style convention.
>
> I'm not happy with you mixing this with the above, less trivial, change.
I find that it is a useful combination. - A parameter is adjusted together
with a special function name.
>> - drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
>> + drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
>
> For "minors" the same holds as for "channels", above.
>
> And you snuck in a parentheses change. That should have probably been
> merged with 5/5.
Would you prefer to add them in another update step?
Regards,
Markus
^ permalink raw reply
* RE: [PATCH 3/3] net: fec: align IP header in hardware
From: David Laight @ 2016-09-28 16:42 UTC (permalink / raw)
To: 'Eric Nelson', netdev@vger.kernel.org
Cc: linux@arm.linux.org.uk, andrew@lunn.ch, fugang.duan@nxp.com,
otavio@ossystems.com.br, edumazet@google.com,
troy.kisky@boundarydevices.com, davem@davemloft.net,
u.kleine-koenig@pengutronix.de
In-Reply-To: <4d199f32-092c-1c0a-7a01-6d0d317ef676@nelint.com>
From: Eric Nelson
> Sent: 26 September 2016 19:40
> Hi David,
>
> On 09/26/2016 02:26 AM, David Laight wrote:
> > From: Eric Nelson
> >> Sent: 24 September 2016 15:42
> >> The FEC receive accelerator (RACC) supports shifting the data payload of
> >> received packets by 16-bits, which aligns the payload (IP header) on a
> >> 4-byte boundary, which is, if not required, at least strongly suggested
> >> by the Linux networking layer.
> > ...
> >> + /* align IP header */
> >> + val |= FEC_RACC_SHIFT16;
> >
> > I can't help feeling that there needs to be corresponding
> > changes to increase the buffer size by 2 (maybe for large mtu)
> > and to discard two bytes from the frame length.
> >
>
> In the normal case, the fec driver over-allocates all receive packets to
> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
> which is either 0x0f (ARM) or 0x03 (PPC).
>
> If the frame length is less than rx_copybreak (typically 256), then
> the frame length from the receive buffer descriptor is used to
> control the allocation size for a copied buffer, and this will include
> the two bytes of padding if RACC_SHIFT16 is set.
>
> > If probably ought to be predicated on NET_IP_ALIGN as well.
> >
> Can you elaborate?
>From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
add two bytes of 'junk' to the start of every receive frame.
In the 'copybreak' case the new skb would need to be 2 bytes shorter
than the length reported by the hardware, and the data copied from
2 bytes into the dma buffer.
The extra 2 bytes also mean the that maximum mtu that can be received
into a buffer is two bytes less.
If someone sets the mtu to (say) 9k for jumbo frames this might matter.
Even with fixed 2048 byte buffers it reduces the maximum value the mtu
can be set to by 2.
Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
on a 4n boundary, and the skb are likely to be allocated that way.
In this case you don't want to extra two bytes of 'junk'.
OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
the data is dma'd to offset -2 in the skb and then ensure that the
end of frame is set correctly.
David
^ permalink raw reply
* Re: ISDN-Gigaset: Fine-tuning for three function implementations
From: SF Markus Elfring @ 2016-09-28 16:50 UTC (permalink / raw)
To: Paul Bolle
Cc: gigaset307x-common, netdev, Karsten Keil, LKML, kernel-janitors,
Julia Lawall
In-Reply-To: <1475063766.16606.39.camel@tiscali.nl>
> Two of the five patches introduced bugs. The rest of the series isn't
> free of various nits either. Of course, I was in no mood to be lenient
> when I looked at those three patches.
>
> I won't take any of these patches, sorry.
Would you like to look once more into an improved patch series for this
software module a bit later?
Regards,
Markus
^ permalink raw reply
* [PATCH 0/3] [v2] Add basic ACPI support to the Qualcomm Technologies EMAC driver
From: Timur Tabi @ 2016-09-28 16:58 UTC (permalink / raw)
To: netdev, linaro-acpi, linux-arm-kernel, shankerd, vikrams, davem
This patch series adds support to the EMAC driver for extracting addresses,
interrupts, and some _DSDs (properties) from ACPI. The first two patches
clean up the code, and the third patch adds ACPI-specific functionality.
The first patch fixes a bug with handling the platform_device for the
internal PHY. This phy is treated as a separate device in both DT and
ACPI, but since the platform is not released automatically when the
driver unloads, managed functions like devm_ioremap_resource cannot be
used.
The second patch replaces of_get_mac_address with its platform-independent
equivalent device_get_mac_address.
The third patch parses the ACPI tables to obtain the platform_device for
the primary EMAC node ("QCOM8070") and the internal phy node ("QCOM8071").
Timur Tabi (3):
[v2] net: qcom/emac: do not use devm on internal phy pdev
[v2] net: qcom/emac: use device_get_mac_address
[v2] net: qcom/emac: initial ACPI support
drivers/net/ethernet/qualcomm/emac/emac-phy.c | 37 ++++++--
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 110 ++++++++++++++++++------
drivers/net/ethernet/qualcomm/emac/emac.c | 26 ++++--
3 files changed, 134 insertions(+), 39 deletions(-)
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH 1/3] [v2] net: qcom/emac: do not use devm on internal phy pdev
From: Timur Tabi @ 2016-09-28 16:58 UTC (permalink / raw)
To: netdev, linaro-acpi, linux-arm-kernel, shankerd, vikrams, davem
In-Reply-To: <1475081924-12362-1-git-send-email-timur@codeaurora.org>
The platform_device returned by of_find_device_by_node() is not
automatically released when the driver unprobes. Therefore,
managed calls like devm_ioremap_resource() should not be used.
Instead, we manually allocate the resources and then free them
on driver release.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 42 +++++++++++++++++++------
drivers/net/ethernet/qualcomm/emac/emac.c | 4 +++
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 6ab0a3c..ad0e420 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -681,6 +681,7 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
struct resource *res;
const struct of_device_id *match;
struct device_node *np;
+ int ret;
np = of_parse_phandle(pdev->dev.of_node, "internal-phy", 0);
if (!np) {
@@ -697,25 +698,48 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
match = of_match_device(emac_sgmii_dt_match, &sgmii_pdev->dev);
if (!match) {
dev_err(&pdev->dev, "unrecognized internal phy node\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto error_put_device;
}
phy->initialize = (emac_sgmii_initialize)match->data;
/* Base address is the first address */
res = platform_get_resource(sgmii_pdev, IORESOURCE_MEM, 0);
- phy->base = devm_ioremap_resource(&sgmii_pdev->dev, res);
- if (IS_ERR(phy->base))
- return PTR_ERR(phy->base);
+ phy->base = ioremap(res->start, resource_size(res));
+ if (IS_ERR(phy->base)) {
+ ret = PTR_ERR(phy->base);
+ goto error_put_device;
+ }
/* v2 SGMII has a per-lane digital digital, so parse it if it exists */
res = platform_get_resource(sgmii_pdev, IORESOURCE_MEM, 1);
if (res) {
- phy->digital = devm_ioremap_resource(&sgmii_pdev->dev, res);
- if (IS_ERR(phy->base))
- return PTR_ERR(phy->base);
-
+ phy->digital = ioremap(res->start, resource_size(res));
+ if (IS_ERR(phy->digital)) {
+ ret = PTR_ERR(phy->digital);
+ goto error_unmap_base;
+ }
}
- return phy->initialize(adpt);
+ ret = phy->initialize(adpt);
+ if (ret)
+ goto error;
+
+ /* We've remapped the addresses, so we don't need the device any
+ * more. of_find_device_by_node() says we should release it.
+ */
+ put_device(&sgmii_pdev->dev);
+
+ return 0;
+
+error:
+ if (phy->digital)
+ iounmap(phy->digital);
+error_unmap_base:
+ iounmap(phy->base);
+error_put_device:
+ put_device(&sgmii_pdev->dev);
+
+ return ret;
}
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index e47d387..429b4cb 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -723,6 +723,10 @@ static int emac_remove(struct platform_device *pdev)
mdiobus_unregister(adpt->mii_bus);
free_netdev(netdev);
+ if (adpt->phy.digital)
+ iounmap(adpt->phy.digital);
+ iounmap(adpt->phy.base);
+
return 0;
}
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH 2/3] [v2] net: qcom/emac: use device_get_mac_address
From: Timur Tabi @ 2016-09-28 16:58 UTC (permalink / raw)
To: netdev, linaro-acpi, linux-arm-kernel, shankerd, vikrams, davem
In-Reply-To: <1475081924-12362-1-git-send-email-timur@codeaurora.org>
Replace the DT-specific of_get_mac_address() function with
device_get_mac_address, which works on both DT and ACPI platforms. This
change makes it easier to add ACPI support.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 429b4cb..551df1c 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -531,18 +531,16 @@ static void emac_clks_teardown(struct emac_adapter *adpt)
static int emac_probe_resources(struct platform_device *pdev,
struct emac_adapter *adpt)
{
- struct device_node *node = pdev->dev.of_node;
struct net_device *netdev = adpt->netdev;
struct resource *res;
- const void *maddr;
+ char maddr[ETH_ALEN];
int ret = 0;
/* get mac address */
- maddr = of_get_mac_address(node);
- if (!maddr)
- eth_hw_addr_random(netdev);
- else
+ if (device_get_mac_address(&pdev->dev, maddr, ETH_ALEN))
ether_addr_copy(netdev->dev_addr, maddr);
+ else
+ eth_hw_addr_random(netdev);
/* Core 0 interrupt */
ret = platform_get_irq(pdev, 0);
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH 3/3] [v2] net: qcom/emac: initial ACPI support
From: Timur Tabi @ 2016-09-28 16:58 UTC (permalink / raw)
To: netdev, linaro-acpi, linux-arm-kernel, shankerd, vikrams, davem
In-Reply-To: <1475081924-12362-1-git-send-email-timur@codeaurora.org>
Add support for reading addresses, interrupts, and _DSD properties
from ACPI tables, just like with device tree. The HID for the
EMAC device itself is QCOM8070. The internal PHY is represented
by a child node with a HID of QCOM8071.
The EMAC also has some complex clock initialization requirements
that are not represented by this patch. This will be addressed
in a future patch.
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/net/ethernet/qualcomm/emac/emac-phy.c | 37 ++++++++++---
drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 72 ++++++++++++++++++-------
drivers/net/ethernet/qualcomm/emac/emac.c | 12 +++++
3 files changed, 95 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index c412ba9..da4e90d 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -19,6 +19,7 @@
#include <linux/of_mdio.h>
#include <linux/phy.h>
#include <linux/iopoll.h>
+#include <linux/acpi.h>
#include "emac.h"
#include "emac-mac.h"
#include "emac-phy.h"
@@ -167,7 +168,6 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
{
struct device_node *np = pdev->dev.of_node;
- struct device_node *phy_np;
struct mii_bus *mii_bus;
int ret;
@@ -183,14 +183,37 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
mii_bus->parent = &pdev->dev;
mii_bus->priv = adpt;
- ret = of_mdiobus_register(mii_bus, np);
- if (ret) {
- dev_err(&pdev->dev, "could not register mdio bus\n");
- return ret;
+ if (has_acpi_companion(&pdev->dev)) {
+ u32 phy_addr;
+
+ ret = mdiobus_register(mii_bus);
+ if (ret) {
+ dev_err(&pdev->dev, "could not register mdio bus\n");
+ return ret;
+ }
+ ret = device_property_read_u32(&pdev->dev, "phy-channel",
+ &phy_addr);
+ if (ret)
+ /* If we can't read a valid phy address, then assume
+ * that there is only one phy on this mdio bus.
+ */
+ adpt->phydev = phy_find_first(mii_bus);
+ else
+ adpt->phydev = mdiobus_get_phy(mii_bus, phy_addr);
+
+ } else {
+ struct device_node *phy_np;
+
+ ret = of_mdiobus_register(mii_bus, np);
+ if (ret) {
+ dev_err(&pdev->dev, "could not register mdio bus\n");
+ return ret;
+ }
+
+ phy_np = of_parse_phandle(np, "phy-handle", 0);
+ adpt->phydev = of_phy_find_device(phy_np);
}
- phy_np = of_parse_phandle(np, "phy-handle", 0);
- adpt->phydev = of_phy_find_device(phy_np);
if (!adpt->phydev) {
dev_err(&pdev->dev, "could not find external phy\n");
mdiobus_unregister(mii_bus);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index ad0e420..3d2c05a 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -14,6 +14,7 @@
*/
#include <linux/iopoll.h>
+#include <linux/acpi.h>
#include <linux/of_device.h>
#include "emac.h"
#include "emac-mac.h"
@@ -662,6 +663,24 @@ void emac_sgmii_reset(struct emac_adapter *adpt)
clk_set_rate(adpt->clk[EMAC_CLK_HIGH_SPEED], 125000000);
}
+static int emac_sgmii_acpi_match(struct device *dev, void *data)
+{
+ static const struct acpi_device_id match_table[] = {
+ {
+ .id = "QCOM8071",
+ .driver_data = (kernel_ulong_t)emac_sgmii_init_v2,
+ },
+ {}
+ };
+ const struct acpi_device_id *id = acpi_match_device(match_table, dev);
+ emac_sgmii_initialize *initialize = data;
+
+ if (id)
+ *initialize = (emac_sgmii_initialize)id->driver_data;
+
+ return !!id;
+}
+
static const struct of_device_id emac_sgmii_dt_match[] = {
{
.compatible = "qcom,fsm9900-emac-sgmii",
@@ -679,30 +698,45 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
struct platform_device *sgmii_pdev = NULL;
struct emac_phy *phy = &adpt->phy;
struct resource *res;
- const struct of_device_id *match;
- struct device_node *np;
int ret;
- np = of_parse_phandle(pdev->dev.of_node, "internal-phy", 0);
- if (!np) {
- dev_err(&pdev->dev, "missing internal-phy property\n");
- return -ENODEV;
- }
+ if (has_acpi_companion(&pdev->dev)) {
+ struct device *dev;
- sgmii_pdev = of_find_device_by_node(np);
- if (!sgmii_pdev) {
- dev_err(&pdev->dev, "invalid internal-phy property\n");
- return -ENODEV;
- }
+ dev = device_find_child(&pdev->dev, &phy->initialize,
+ emac_sgmii_acpi_match);
- match = of_match_device(emac_sgmii_dt_match, &sgmii_pdev->dev);
- if (!match) {
- dev_err(&pdev->dev, "unrecognized internal phy node\n");
- ret = -ENODEV;
- goto error_put_device;
- }
+ if (!dev) {
+ dev_err(&pdev->dev, "cannot find internal phy node\n");
+ return -ENODEV;
+ }
- phy->initialize = (emac_sgmii_initialize)match->data;
+ sgmii_pdev = to_platform_device(dev);
+ } else {
+ const struct of_device_id *match;
+ struct device_node *np;
+
+ np = of_parse_phandle(pdev->dev.of_node, "internal-phy", 0);
+ if (!np) {
+ dev_err(&pdev->dev, "missing internal-phy property\n");
+ return -ENODEV;
+ }
+
+ sgmii_pdev = of_find_device_by_node(np);
+ if (!sgmii_pdev) {
+ dev_err(&pdev->dev, "invalid internal-phy property\n");
+ return -ENODEV;
+ }
+
+ match = of_match_device(emac_sgmii_dt_match, &sgmii_pdev->dev);
+ if (!match) {
+ dev_err(&pdev->dev, "unrecognized internal phy node\n");
+ ret = -ENODEV;
+ goto error_put_device;
+ }
+
+ phy->initialize = (emac_sgmii_initialize)match->data;
+ }
/* Base address is the first address */
res = platform_get_resource(sgmii_pdev, IORESOURCE_MEM, 0);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 551df1c..9bf3b2b 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -22,6 +22,7 @@
#include <linux/of_device.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/acpi.h>
#include "emac.h"
#include "emac-mac.h"
#include "emac-phy.h"
@@ -575,6 +576,16 @@ static const struct of_device_id emac_dt_match[] = {
{}
};
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id emac_acpi_match[] = {
+ {
+ .id = "QCOM8070",
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, emac_acpi_match);
+#endif
+
static int emac_probe(struct platform_device *pdev)
{
struct net_device *netdev;
@@ -734,6 +745,7 @@ static struct platform_driver emac_platform_driver = {
.driver = {
.name = "qcom-emac",
.of_match_table = emac_dt_match,
+ .acpi_match_table = ACPI_PTR(emac_acpi_match),
},
};
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* Re: [PATCH net-next] net/vxlan: Avoid unaligned access in vxlan_build_skb()
From: Sowmini Varadhan @ 2016-09-28 17:03 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Netdev, sowmini.varadhan
In-Reply-To: <CAKgT0Uenb+D44D4g2SieNDf3+P_wnv2WYSBZ+NU_87e011PRjA@mail.gmail.com>
On (09/23/16 17:43), Alexander Duyck wrote:
> > On (09/23/16 10:38), Alexander Duyck wrote:
;
> >> almost think of it as us doing something like the inverse of
> >> pskb_pull_tail. The general idea here is we want to actually leave
> >> the data in skb->data, but just reference it from frag[0] so that we
> >> don't accidentally pull in the 2 byte padding for alignment when
> >> transmitting the frame.
Some additional findings..
Just to recap how we got here: for the Rx path, the inner packet has
been set up as an ethernet frame with the IP header at an aligned address
when it hits vxlan_build_skb. But that means the (inner) mac address
was offset by NET_IP_ALIGN so vxlan_build_skb needs to pad the data
by NET_IP_ALIGN to make the vxh outer ip header align.
Then we'd need to do something like the suggestion above (keep some
pointers in frag[0]? do the reverse of a pskb_expand_head to push out
the inner ip header to the skb_frag_t?), to have the driver skip over the
pad..
I tried the following for a hack, and it takes care of the tx side
unaligned access, though, clearly, the memmove needs to be avoided
@@ -1750,10 +1825,38 @@ static int vxlan_build_skb(struct sk_buff *skb, struct d
if (err)
goto out_free;
+
+#if (NET_IP_ALIGN != 0)
+ {
+ unsigned char *data;
+
+ /* inner packet is an ethernet frame that was set up
+ * so that the IP header is aligned. But that means the
+ * mac address was offset by NET_IP_ALIGN, so we need
+ * to move things up so that the vxh and outer ip header
+ * are now aligned
+ * XXX The Alexander Duyck idea was to only do the
+ * extra __skb_push() for NET_IP_ALIGN, and avoid the
+ * extram memmove and ->inner* adjustments. Plus keep
+ * additional pointers in frag[0] and have the driver pick
+ * up pointers from frag[0] .. need to investigate
+ * that suggestion further.
+ */
+ data = skb->data;
+ skb->data -= NET_IP_ALIGN;
+ memmove(skb->data, data, skb_headlen(skb));
+ skb->inner_network_header -= NET_IP_ALIGN;
+ skb->inner_mac_header -= NET_IP_ALIGN;
+ skb->inner_transport_header -= NET_IP_ALIGN;
+ }
+#endif
vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
vxh->vx_flags = VXLAN_HF_VNI;
vxh->vx_vni = vxlan_vni_field(vni);
In the general case (when the skb passed to vxlan_build_skb is already
non-linear), wouldn't we end up having to shift all the frags by 1 and/or
do some type of memory copy of the inner packet? However, I think
there are some clever things we can do in general, to avoid the memmove..
I also looked at the Rx path. Here the suggestion was:
"we should only pull the outer headers from the page frag, and then
when the time is right we drop the outer headers and pull the inner
headers from the page frag. That way we can keep all the headers
aligned."
I hacked up ixgbe_add_rx_frag to always only create nonlinear skb's,
i.e., always avoid the
memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
and then I end up with
- ixgbe_clean_rx_irq copies outer header ether/ip/udp headers
into linear part as needed,
- then udp_gro_receive -> vxlan_gro_receive pulls up vxlan header
into linear part, and then..
- eth_gro_receive pulls up another 34 bytes for the eth + ip header.
This last pull ends up being unaligned.
I dont know if we can safely drop the outer ip headers at this point
(have not tried this yet, and I'm not sure we can do this in all udp
encaps cases..)
one other possibility is to set up the inner frame as part of the
->frag_list (note, this is *not* skb_frag_t). I suspect that is going
to cause other inefficiencies.
but, as tom has been saying all along, a big part of this problem is that
we are tripping up on the ethernet header in the middle of an
IP packet. Unfortunately I dont think the ietf is going to agree
to never ever do that, so I'm not sure we can win that architectural battle.
^ permalink raw reply
* Re: [PATCH v2 net] net: skbuff: skb_vlan_push: Fix wrong unwinding of skb->data after __vlan_insert_tag call
From: Shmulik Ladkani @ 2016-09-28 17:11 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Pravin Shelar, netdev, Shmulik Ladkani,
Jiri Pirko
In-Reply-To: <57EBD71A.90104@iogearbox.net>
On Wed, 28 Sep 2016 16:43:38 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:
> > (1) suppose upon entry we have
> >
> > DA,SA,0x8100,TCI,0x0800,
> > ^ ^
> > mac_hdr data
> >
> > initial offset is 18, and after current unwinding code we'll get
>
> You mean data points after the 0x0800, right?
Sorry. Yes, exactly as you say. Initially 18 bytes ahead:
DA,SA,0x8100,TCI,0x0800,
^ ^
mac_hdr data
^ permalink raw reply
* Re: [PATCH RFC 3/6] net: phy: Threaded interrupts allow some simplification
From: Florian Fainelli @ 2016-09-28 17:14 UTC (permalink / raw)
To: Sergei Shtylyov, Andrew Lunn; +Cc: Vivien Didelot, netdev
In-Reply-To: <891f6b60-47a0-ba03-d6d3-8284993d1896@cogentembedded.com>
On 09/28/2016 06:38 AM, Sergei Shtylyov wrote:
> On 09/28/2016 03:28 PM, Andrew Lunn wrote:
>
>>>>> The PHY interrupts are now handled in a threaded interrupt handler,
>>>>> which can sleep. The work queue is no longer needed, phy_change() can
>>>>> be called directly. Additionally, none of the callers of
>>>>> phy_mac_interrupt() did so in interrupt context, so fully remove the
>>>>
>>>> I did intend to call it from interrupt context (from the ravb
>>>> driver).
>>>>
>>>>> work queue, and document that phy_mac_interrupt() should not be called
>>>>> in interrupt context.
>>>>
>>>> It was intentionally made callable from the interrupt context, I'd
>>>> prefer
>>>> if you wouldn't change that.
>>>
>>> OTOH, it's still not very handy to call because of the 'new_link'
>>> parameter which I'm not sure I can provide...
>>
>> Hi Sergei
>>
>> If there is a need for it, i will leave the work queue and keep this
>> code unchanged.
>
> Let's hear what Florian says...
The intent is really to have phy_mac_interrupt() callable from hard IRQ
context, not that this matters really too much because link events
already occur in the slow path, but it's nice to have that property
retained IMHO.
--
Florian
^ permalink raw reply
* Re: [PATCH 3/3] net: fec: align IP header in hardware
From: Eric Nelson @ 2016-09-28 17:14 UTC (permalink / raw)
To: David Laight, netdev@vger.kernel.org
Cc: linux@arm.linux.org.uk, andrew@lunn.ch, fugang.duan@nxp.com,
otavio@ossystems.com.br, edumazet@google.com,
troy.kisky@boundarydevices.com, davem@davemloft.net,
u.kleine-koenig@pengutronix.de
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB010A73F@AcuExch.aculab.com>
Thanks David,
On 09/28/2016 09:42 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 26 September 2016 19:40
>> Hi David,
>>
>> On 09/26/2016 02:26 AM, David Laight wrote:
>>> From: Eric Nelson
>>>> Sent: 24 September 2016 15:42
>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
>>>> received packets by 16-bits, which aligns the payload (IP header) on a
>>>> 4-byte boundary, which is, if not required, at least strongly suggested
>>>> by the Linux networking layer.
>>> ...
>>>> + /* align IP header */
>>>> + val |= FEC_RACC_SHIFT16;
>>>
>>> I can't help feeling that there needs to be corresponding
>>> changes to increase the buffer size by 2 (maybe for large mtu)
>>> and to discard two bytes from the frame length.
>>>
>>
>> In the normal case, the fec driver over-allocates all receive packets to
>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
>> which is either 0x0f (ARM) or 0x03 (PPC).
>>
>> If the frame length is less than rx_copybreak (typically 256), then
>> the frame length from the receive buffer descriptor is used to
>> control the allocation size for a copied buffer, and this will include
>> the two bytes of padding if RACC_SHIFT16 is set.
>>
>>> If probably ought to be predicated on NET_IP_ALIGN as well.
>>>
>> Can you elaborate?
>
> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> add two bytes of 'junk' to the start of every receive frame.
>
That's right. Two bytes of junk between the MAC header and the
IP header.
> In the 'copybreak' case the new skb would need to be 2 bytes shorter
> than the length reported by the hardware, and the data copied from
> 2 bytes into the dma buffer.
>
As it stands, the skb allocated by the copybreak routine will include
the two bytes of padding, and the call to skb_pull_inline will ignore
them.
> The extra 2 bytes also mean the that maximum mtu that can be received
> into a buffer is two bytes less.
>
Right, but I think the max is already high enough that this isn't a
problem.
> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
> can be set to by 2.
>
As far as I can tell, the fec driver doesn't support jumbo frames, and
the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).
This is well within the 2048-byte allocation, even with optional headers
for VLAN etc.
> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
> on a 4n boundary, and the skb are likely to be allocated that way.
> In this case you don't want to extra two bytes of 'junk'.
>
NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h
> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
> the data is dma'd to offset -2 in the skb and then ensure that the
> end of frame is set correctly.
>
That's what the RACC SHIFT16 bit does.
The FEC hardware isn't capable of DMA'ing to an un-aligned address.
On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.
On other (PPC?) architectures, it requires 32-bit alignment. This is
handled by the rx_align field.
Regards,
Eric
^ permalink raw reply
* Re: [PATCH RFC 4/6] net: phy: Use phy name when requesting the interrupt
From: Florian Fainelli @ 2016-09-28 17:18 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot; +Cc: netdev
In-Reply-To: <1475051544-18561-5-git-send-email-andrew@lunn.ch>
On 09/28/2016 01:32 AM, Andrew Lunn wrote:
> Using the fixed name "phy_interrupt" is not very informative in
> /proc/interrupts when there are a lot of phys, e.g. a device with an
> Ethernet switch. So when requesting the interrupt, use the name of the
> phy.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 3/3] net: fec: align IP header in hardware
From: Russell King - ARM Linux @ 2016-09-28 17:25 UTC (permalink / raw)
To: Eric Nelson
Cc: David Laight, netdev@vger.kernel.org, andrew@lunn.ch,
fugang.duan@nxp.com, otavio@ossystems.com.br, edumazet@google.com,
troy.kisky@boundarydevices.com, davem@davemloft.net,
u.kleine-koenig@pengutronix.de
In-Reply-To: <5cf173c4-84e0-f309-f356-35b114cc166e@nelint.com>
On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote:
> Thanks David,
>
> On 09/28/2016 09:42 AM, David Laight wrote:
> > From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> > add two bytes of 'junk' to the start of every receive frame.
>
> That's right. Two bytes of junk between the MAC header and the
> IP header.
That's wrong. FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of
the packet, not in the middle of the packet:
7 RX FIFO Shift-16
SHIFT16
When this field is set, the actual frame data starts at bit 16
of the first word read from the RX FIFO aligning the Ethernet
payload on a 32-bit boundary.
NOTE: This function only affects the FIFO storage and has no
influence on the statistics, which use the actual length
of the frame received.
0 Disabled.
1 Instructs the MAC to write two additional bytes in front
of each frame received into the RX FIFO.
*in front* of the frame - that's before the Ethernet header. Not between
the ethernet and IP headers.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: Add Wake-on-LAN driver for Microsemi PHYs.
From: Florian Fainelli @ 2016-09-28 17:37 UTC (permalink / raw)
To: Raju Lakkaraju, netdev; +Cc: Allan.Nielsen, andrew
In-Reply-To: <1475064078-22310-2-git-send-email-Raju.Lakkaraju@microsemi.com>
On 09/28/2016 05:01 AM, Raju Lakkaraju wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
>
> Wake-on-LAN (WoL) is an Ethernet networking standard that allows
> a computer/device to be turned on or awakened by a network message.
> VSC8531 PHY can support this feature configure by driver set function.
> WoL status get by driver get function.
>
> Tested on Beaglebone Black with VSC 8531 PHY.
>
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> ---
> drivers/net/phy/mscc.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 132 insertions(+)
>
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index d350deb..ca6ea23 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -11,6 +11,7 @@
> #include <linux/mdio.h>
> #include <linux/mii.h>
> #include <linux/phy.h>
> +#include <linux/netdevice.h>
>
> enum rgmii_rx_clock_delay {
> RGMII_RX_CLK_DELAY_0_2_NS = 0,
> @@ -35,6 +36,7 @@ enum rgmii_rx_clock_delay {
>
> #define MII_VSC85XX_INT_MASK 25
> #define MII_VSC85XX_INT_MASK_MASK 0xa000
> +#define MII_VSC85XX_INT_MASK_WOL 0x0040
> #define MII_VSC85XX_INT_STATUS 26
>
> #define MSCC_EXT_PAGE_ACCESS 31
> @@ -46,6 +48,19 @@ enum rgmii_rx_clock_delay {
> #define RGMII_RX_CLK_DELAY_MASK 0x0070
> #define RGMII_RX_CLK_DELAY_POS 4
>
> +#define MSCC_PHY_WOL_LOWER_MAC_ADDR 21
> +#define MSCC_PHY_WOL_MID_MAC_ADDR 22
> +#define MSCC_PHY_WOL_UPPER_MAC_ADDR 23
> +#define MSCC_PHY_WOL_LOWER_PASSWD 24
> +#define MSCC_PHY_WOL_MID_PASSWD 25
> +#define MSCC_PHY_WOL_UPPER_PASSWD 26
> +
> +#define MSCC_PHY_WOL_MAC_CONTROL 27
> +#define EDGE_RATE_CNTL_POS 5
> +#define EDGE_RATE_CNTL_MASK 0x00E0
> +#define SECURE_ON_ENABLE 0x8000
> +#define SECURE_ON_PASSWD_LEN_4 0x4000
> +
> /* Microsemi PHY ID's */
> #define PHY_ID_VSC8531 0x00070570
> #define PHY_ID_VSC8541 0x00070770
> @@ -58,6 +73,119 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> return rc;
> }
>
> +static int vsc85xx_wol_set(struct phy_device *phydev,
> + struct ethtool_wolinfo *wol)
> +{
> + int rc;
> + u16 reg_val;
> + struct ethtool_wolinfo *wol_conf = wol;
> +
> + mutex_lock(&phydev->lock);
This mutex is used here because you are using an indirect page access,
right? This is not to protect against multiple calls of wol_set from
different executing threads?
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> + if (rc != 0)
> + goto out_unlock;
> +
> + if (wol->wolopts & WAKE_MAGIC) {
> + /* Store the device address for the magic packet */
> + reg_val = phydev->attached_dev->dev_addr[4] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[5];
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, reg_val);
> + reg_val = phydev->attached_dev->dev_addr[2] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[3];
> + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, reg_val);
> + reg_val = phydev->attached_dev->dev_addr[0] << 8;
> + reg_val |= phydev->attached_dev->dev_addr[1];
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, reg_val);
> + } else {
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_MAC_ADDR, 0);
> + phy_write(phydev, MSCC_PHY_WOL_MID_MAC_ADDR, 0);
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_MAC_ADDR, 0);
> + }
> +
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> + if (wol_conf->wolopts & WAKE_MAGICSECURE)
> + reg_val |= SECURE_ON_ENABLE;
> + else
> + reg_val &= ~SECURE_ON_ENABLE;
> + phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
> +
> + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> + reg_val = wol_conf->sopass[4] << 8;
> + reg_val |= wol_conf->sopass[5];
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[2] << 8;
> + reg_val |= wol_conf->sopass[3];
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, reg_val);
> + reg_val = wol_conf->sopass[0] << 8;
> + reg_val |= wol_conf->sopass[1];
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, reg_val);
> + } else {
> + phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, 0);
> + phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, 0);
> + }
How about making the code a little simpler in both cases with something
like this the following:
u16 pwd = { };
unsigned int i;
if (wol_conf->wolopts & WAKE_MAGICECURE)
for (i = 0; i < ARRAY_SIZE(pwd); i++)
pwd[i] = wol_conf->so_pass[5 - (i * 2 + 1)] << 8|
wol_conf->so_pass[5 - i * 2 ]
phy_write(phydev, MSCC_PHY_WOL_LOWER_PASSWD, pwd[0]);
phy_write(phydev, MSCC_PHY_WOL_MID_PASSWD, pwd[1]);
phy_write(phydev, MSCC_PHY_WOL_UPPER_PASSWD, pwd[2]);
> +
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> + if (rc != 0)
> + goto out_unlock;
> +
> + if (wol->wolopts & WAKE_MAGIC) {
Don't you also need to check WAKE_MAGICSECURE here as well? Or is the
interrupt going to be generated only if there is no password defined?
> + /* Enable the WOL interrupt */
> + reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> + reg_val |= MII_VSC85XX_INT_MASK_WOL;
> + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> + if (rc != 0)
> + goto out_unlock;
> + } else {
> + /* Disable the WOL interrupt */
> + reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
> + reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
> + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
> + if (rc != 0)
> + goto out_unlock;
> + }
> + /* Clear WOL iterrupt status */
> + reg_val = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> +
> +out_unlock:
> + mutex_unlock(&phydev->lock);
> +
> + return rc;
> +}
> +
> +static void vsc85xx_wol_get(struct phy_device *phydev,
> + struct ethtool_wolinfo *wol)
> +{
> + int rc;
> + u16 reg_val;
> + struct ethtool_wolinfo *wol_conf = wol;
> +
> + mutex_lock(&phydev->lock);
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> + if (rc != 0)
> + goto out_unlock;
> +
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
> + if (reg_val & SECURE_ON_ENABLE)
> + wol_conf->wolopts |= WAKE_MAGICSECURE;
> + if (wol_conf->wolopts & WAKE_MAGICSECURE) {
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_LOWER_PASSWD);
> + wol_conf->sopass[5] = reg_val & 0x00ff;
> + wol_conf->sopass[4] = (reg_val & 0xff00) >> 8;
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_MID_PASSWD);
> + wol_conf->sopass[3] = reg_val & 0x00ff;
> + wol_conf->sopass[2] = (reg_val & 0xff00) >> 8;
> + reg_val = phy_read(phydev, MSCC_PHY_WOL_UPPER_PASSWD);
> + wol_conf->sopass[1] = reg_val & 0x00ff;
> + wol_conf->sopass[0] = (reg_val & 0xff00) >> 8;
> + }
> +
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> + mutex_unlock(&phydev->lock);
> +}
> +
> static int vsc85xx_mac_if_set(struct phy_device *phydev,
> phy_interface_t interface)
> {
> @@ -177,6 +305,8 @@ static struct phy_driver vsc85xx_driver[] = {
> .config_intr = &vsc85xx_config_intr,
> .suspend = &genphy_suspend,
> .resume = &genphy_resume,
> + .set_wol = &vsc85xx_wol_set,
> + .get_wol = &vsc85xx_wol_get,
> },
> {
> .phy_id = PHY_ID_VSC8541,
> @@ -193,6 +323,8 @@ static struct phy_driver vsc85xx_driver[] = {
> .config_intr = &vsc85xx_config_intr,
> .suspend = &genphy_suspend,
> .resume = &genphy_resume,
> + .set_wol = &vsc85xx_wol_set,
> + .get_wol = &vsc85xx_wol_get,
> }
>
> };
>
--
Florian
^ permalink raw reply
* Re: [PATCH v2 net] net: skbuff: skb_vlan_push: Fix wrong unwinding of skb->data after __vlan_insert_tag call
From: Shmulik Ladkani @ 2016-09-28 17:42 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Pravin Shelar, netdev, Shmulik Ladkani,
Jiri Pirko
In-Reply-To: <57EBD71A.90104@iogearbox.net>
On Wed, 28 Sep 2016 16:43:38 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:
> Couldn't we end up with 1) for the act_vlan case when we'd have the
> offset-adjusted skb_vlan_push() fix from here, where we'd then redirect
> to ingress where skb_vlan_pop() would be called? If I'm not missing
> something, skb_vlan_push() would then point to the data location of 1)
> and with your other proposed direct netif_receive_skb() patch, no
> further skb->data adjustments would be done, right?
Right. Then skb_vlan_pop() should expect either (1) or (2).
> Another potential issue (but unrelated to this fix here) I just noticed
> is, whether act_vlan might have the same problem as we fixed in 8065694e6519
> ("bpf: fix checksum for vlan push/pop helper"). So potentially, we could
> end up fixing CHECKSUM_COMPLETE wrongly on ingress, since these 14 bytes
> are already pulled out of the sum at that point.
>
> > Should we adjust "offset" back, only if resulting offset is >=14 ?
>
> If also the checksum one might end up as an issue, maybe it's just best
> to go through the pain and do the push/pull for data plus csum, so both
> skb_vlan_*() functions see the frame starting from mac header temporarily?
Although not related to this specific fix, I see 2 ways addressing the
rcsum problem:
1. Per your suggestion, skb_vlan_*() to expect 'data' at mac_header
That would simplify things; for this suggested 'data unwind' fix as well
2. Within skb_vlan_*(), deduct (according to initial offset) whether
we're already "pulled out" of the rcsum, and not invoke the
skb_postpull/push_rcsum update.
Will meditate some more.
Thanks
Shmulik
^ permalink raw reply
* Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions
From: Paul Bolle @ 2016-09-28 17:44 UTC (permalink / raw)
To: SF Markus Elfring
Cc: gigaset307x-common, netdev, Karsten Keil, LKML, kernel-janitors,
Julia Lawall
In-Reply-To: <1a101dea-006f-f507-54e5-5d05ccd996ec@users.sourceforge.net>
On Wed, 2016-09-28 at 18:38 +0200, SF Markus Elfring wrote:
> > I'm not going to change code just because some checker suggests to
> > do so.
>
> The script "checkpatch.pl" can point information out like the
> following.
>
> WARNING: Prefer kmalloc_array over kmalloc with multiply
Am I being trolled?
Paul Bolle
^ permalink raw reply
* Re: ISDN-Gigaset: Fine-tuning for three function implementations
From: Paul Bolle @ 2016-09-28 17:57 UTC (permalink / raw)
To: SF Markus Elfring
Cc: gigaset307x-common, netdev, Karsten Keil, LKML, kernel-janitors,
Julia Lawall
In-Reply-To: <15e9e3f5-ef06-fb3d-4d6a-3df9c0378a2b@users.sourceforge.net>
On Wed, 2016-09-28 at 18:50 +0200, SF Markus Elfring wrote:
> Would you like to look once more into an improved patch series for this
> software module a bit later?
I'm afraid I'm not looking forward to receiving an update of this
series, sorry.
Thanks,
Paul Bolle
^ permalink raw reply
* [PATCH v1] mlx4: remove unused fields
From: David Decotigny @ 2016-09-28 18:00 UTC (permalink / raw)
To: Yishai Hadas, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: edumazet-hpIqsD4AKlfQT0dZR+AlfA, David Decotigny
From: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>
This also can address following UBSAN warnings:
[ 36.640343] ================================================================================
[ 36.648772] UBSAN: Undefined behaviour in drivers/net/ethernet/mellanox/mlx4/fw.c:857:26
[ 36.656853] shift exponent 64 is too large for 32-bit type 'int'
[ 36.663348] ================================================================================
[ 36.671783] ================================================================================
[ 36.680213] UBSAN: Undefined behaviour in drivers/net/ethernet/mellanox/mlx4/fw.c:861:27
[ 36.688297] shift exponent 35 is too large for 32-bit type 'int'
[ 36.694702] ================================================================================
Tested:
reboot with UBSAN, no warning.
Signed-off-by: David Decotigny <decot-Ypc/8FJVVoBWk0Htik3J/w@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx4/fw.c | 4 ----
drivers/net/ethernet/mellanox/mlx4/fw.h | 2 --
2 files changed, 6 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.c b/drivers/net/ethernet/mellanox/mlx4/fw.c
index 090bf81..f9cbc67 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.c
@@ -853,12 +853,8 @@ int mlx4_QUERY_DEV_CAP(struct mlx4_dev *dev, struct mlx4_dev_cap *dev_cap)
dev_cap->max_eqs = 1 << (field & 0xf);
MLX4_GET(field, outbox, QUERY_DEV_CAP_RSVD_MTT_OFFSET);
dev_cap->reserved_mtts = 1 << (field >> 4);
- MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_MRW_SZ_OFFSET);
- dev_cap->max_mrw_sz = 1 << field;
MLX4_GET(field, outbox, QUERY_DEV_CAP_RSVD_MRW_OFFSET);
dev_cap->reserved_mrws = 1 << (field & 0xf);
- MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_MTT_SEG_OFFSET);
- dev_cap->max_mtt_seg = 1 << (field & 0x3f);
MLX4_GET(size, outbox, QUERY_DEV_CAP_NUM_SYS_EQ_OFFSET);
dev_cap->num_sys_eqs = size & 0xfff;
MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_REQ_QP_OFFSET);
diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.h b/drivers/net/ethernet/mellanox/mlx4/fw.h
index f11614f..5343a05 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.h
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.h
@@ -80,9 +80,7 @@ struct mlx4_dev_cap {
int max_eqs;
int num_sys_eqs;
int reserved_mtts;
- int max_mrw_sz;
int reserved_mrws;
- int max_mtt_seg;
int max_requester_per_qp;
int max_responder_per_qp;
int max_rdma_global;
--
2.8.0.rc3.226.g39d4020
--
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 related
* Re: [PATCH 3/3] net: fec: align IP header in hardware
From: Eric Nelson @ 2016-09-28 18:01 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: David Laight, netdev@vger.kernel.org, andrew@lunn.ch,
fugang.duan@nxp.com, otavio@ossystems.com.br, edumazet@google.com,
troy.kisky@boundarydevices.com, davem@davemloft.net,
u.kleine-koenig@pengutronix.de
In-Reply-To: <20160928172530.GM1041@n2100.armlinux.org.uk>
Thanks Russell,
On 09/28/2016 10:25 AM, Russell King - ARM Linux wrote:
> On Wed, Sep 28, 2016 at 10:14:52AM -0700, Eric Nelson wrote:
>> Thanks David,
>>
>> On 09/28/2016 09:42 AM, David Laight wrote:
>>> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
>>> add two bytes of 'junk' to the start of every receive frame.
>>
>> That's right. Two bytes of junk between the MAC header and the
>> IP header.
>
> That's wrong. FEC_RACC_SHIFT16 adds two bytes to the _beginning_ of
> the packet, not in the middle of the packet:
>
> 7 RX FIFO Shift-16
> SHIFT16
> When this field is set, the actual frame data starts at bit 16
> of the first word read from the RX FIFO aligning the Ethernet
> payload on a 32-bit boundary.
>
> NOTE: This function only affects the FIFO storage and has no
> influence on the statistics, which use the actual length
> of the frame received.
>
> 0 Disabled.
> 1 Instructs the MAC to write two additional bytes in front
> of each frame received into the RX FIFO.
>
> *in front* of the frame - that's before the Ethernet header. Not between
> the ethernet and IP headers.
>
I obviously mis-read this, and haven't dumped any packets to straighten
myself out.
^ permalink raw reply
* [PATCH] Add netdev all_adj_list refcnt propagation to fix panic
From: David Ahern @ 2016-09-28 18:06 UTC (permalink / raw)
To: netdev, acollins, davem; +Cc: vfalico, nikolay, David Ahern
Andrew Collins posted this patch as RFC in March:
http://patchwork.ozlabs.org/patch/603101/
It has apparently fallen through the cracks and never applied.
It solves a refcnt problem (thanks Nik for pointing out this patch)
with stacked devices that involves macvlan on a bridge, a bond into
the bridge, and the bridge and macvlan are enslaved to a vrf:
+--------+
| myvrf |
+--------+
| |
| +---------+
| | macvlan |
| +---------+
| |
+----------+
| bridge |
+----------+
|
+--------+
| bond0 |
+--------+
|
+--------+
| swp3 |
+--------+
Deleting bond0 hangs waiting for bond0 to become free. The splat in dmesg
is:
[ 206.485340] ------------[ cut here ]------------
[ 206.486052] WARNING: CPU: 0 PID: 746 at /home/dsa/kernel-3.git/net/core/dev.c:6772 rollback_registered_many+0x28a/0x2da
[ 206.487563] Modules linked in: macvlan bonding bridge stp llc vrf
[ 206.488946] CPU: 0 PID: 746 Comm: ifdown Not tainted 4.8.0-rc7+ #144
[ 206.489768] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 206.490973] 0000000000000000 ffff88003b3a3ca8 ffffffff81287ad3 0000000000000000
[ 206.491939] 0000000000000000 ffff88003b3a3ce8 ffffffff8104f19d 00001a743b3a3b58
[ 206.492914] ffff88003c93e000 ffff88003b3a3d48 ffff88003c877400 ffff88003b3a3d48
[ 206.493873] Call Trace:
[ 206.494184] [<ffffffff81287ad3>] dump_stack+0x81/0xb6
[ 206.494805] [<ffffffff8104f19d>] __warn+0xc5/0xe0
[ 206.495382] [<ffffffff8104f265>] warn_slowpath_null+0x18/0x1a
[ 206.496094] [<ffffffff81412703>] rollback_registered_many+0x28a/0x2da
[ 206.496888] [<ffffffff8141277c>] rollback_registered+0x29/0x36
[ 206.497596] [<ffffffff81412950>] unregister_netdevice_queue+0x6a/0xa6
[ 206.498383] [<ffffffffa004ff10>] bonding_store_bonds+0x132/0x186 [bonding]
[ 206.499209] [<ffffffff8133baff>] class_attr_store+0x1e/0x20
[ 206.499893] [<ffffffff811c21ce>] sysfs_kf_write+0x44/0x4b
[ 206.500558] [<ffffffff811c14cb>] kernfs_fop_write+0x113/0x15d
[ 206.501269] [<ffffffff8116216b>] __vfs_write+0x21/0xa0
[ 206.501897] [<ffffffff810833cf>] ? percpu_down_read+0x4e/0x7a
[ 206.502595] [<ffffffff81164f25>] ? __sb_start_write+0x5a/0xab
[ 206.503302] [<ffffffff81164f25>] ? __sb_start_write+0x5a/0xab
[ 206.504015] [<ffffffff81162d7f>] vfs_write+0xa2/0xc6
[ 206.504637] [<ffffffff8116397a>] SyS_write+0x4b/0x79
[ 206.505251] [<ffffffff8150833c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
[ 206.506054] ---[ end trace a578aa9ea7e7176a ]---
[ 206.512017] PF_BRIDGE: RTM_SETLINK with unknown ifindex
[ 216.765567] unregister_netdevice: waiting for bond0 to become free. Usage count = 1
The splate is from this line in rollback_registered_many():
WARN_ON(netdev_has_any_upper_dev(dev));
---
This is the original commit message from Andrew:
This is an RFC patch to fix a relatively easily reproducible kernel
panic related to the all_adj_list handling for netdevs in recent kernels.
This is more to generate discussion than anything else. I don't
particularly like this approach, I'm hoping someone has a better idea.
The following sequence of commands will reproduce the issue:
ip link add link eth0 name eth0.100 type vlan id 100
ip link add link eth0 name eth0.200 type vlan id 200
ip link add name testbr type bridge
ip link set eth0.100 master testbr
ip link set eth0.200 master testbr
ip link add link testbr mac0 type macvlan
ip link delete dev testbr
This creates an upper/lower tree of (excuse the poor ASCII art):
/---eth0.100-eth0
mac0-testbr-
\---eth0.200-eth0
When testbr is deleted, the all_adj_lists are walked, and eth0 is deleted twice from
the mac0 list. Unfortunately, during setup in __netdev_upper_dev_link, only one
reference to eth0 is added, so this results in a panic.
This change adds reference count propagation so things are handled properly.
Matthias Schiffer reported a similar crash in batman-adv:
https://github.com/freifunk-gluon/gluon/issues/680
https://www.open-mesh.org/issues/247
which this patch also seems to resolve.
Patch is from Andrew Collins <acollins@cradlepoint.com>, but did not have
a formal sign-off. The patch applies to top of tree, so no change made on
my part I just pulled it off the web.
From: Andrew Collins <acollins@cradlepoint.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
net/core/dev.c | 69 ++++++++++++++++++++++++++++++++--------------------------
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index c0c291f721d6..84963c99763a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5589,6 +5589,7 @@ static inline bool netdev_adjacent_is_neigh_list(struct net_device *dev,
static int __netdev_adjacent_dev_insert(struct net_device *dev,
struct net_device *adj_dev,
+ u16 ref_nr,
struct list_head *dev_list,
void *private, bool master)
{
@@ -5598,7 +5599,7 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
adj = __netdev_find_adj(adj_dev, dev_list);
if (adj) {
- adj->ref_nr++;
+ adj->ref_nr += ref_nr;
return 0;
}
@@ -5608,7 +5609,7 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
adj->dev = adj_dev;
adj->master = master;
- adj->ref_nr = 1;
+ adj->ref_nr = ref_nr;
adj->private = private;
dev_hold(adj_dev);
@@ -5647,6 +5648,7 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
static void __netdev_adjacent_dev_remove(struct net_device *dev,
struct net_device *adj_dev,
+ u16 ref_nr,
struct list_head *dev_list)
{
struct netdev_adjacent *adj;
@@ -5659,10 +5661,10 @@ static void __netdev_adjacent_dev_remove(struct net_device *dev,
BUG();
}
- if (adj->ref_nr > 1) {
- pr_debug("%s to %s ref_nr-- = %d\n", dev->name, adj_dev->name,
- adj->ref_nr-1);
- adj->ref_nr--;
+ if (adj->ref_nr > ref_nr) {
+ pr_debug("%s to %s ref_nr-%d = %d\n", dev->name, adj_dev->name,
+ ref_nr, adj->ref_nr - ref_nr);
+ adj->ref_nr -= ref_nr;
return;
}
@@ -5681,21 +5683,22 @@ static void __netdev_adjacent_dev_remove(struct net_device *dev,
static int __netdev_adjacent_dev_link_lists(struct net_device *dev,
struct net_device *upper_dev,
+ u16 ref_nr,
struct list_head *up_list,
struct list_head *down_list,
void *private, bool master)
{
int ret;
- ret = __netdev_adjacent_dev_insert(dev, upper_dev, up_list, private,
- master);
+ ret = __netdev_adjacent_dev_insert(dev, upper_dev, ref_nr, up_list,
+ private, master);
if (ret)
return ret;
- ret = __netdev_adjacent_dev_insert(upper_dev, dev, down_list, private,
- false);
+ ret = __netdev_adjacent_dev_insert(upper_dev, dev, ref_nr, down_list,
+ private, false);
if (ret) {
- __netdev_adjacent_dev_remove(dev, upper_dev, up_list);
+ __netdev_adjacent_dev_remove(dev, upper_dev, ref_nr, up_list);
return ret;
}
@@ -5703,9 +5706,10 @@ static int __netdev_adjacent_dev_link_lists(struct net_device *dev,
}
static int __netdev_adjacent_dev_link(struct net_device *dev,
- struct net_device *upper_dev)
+ struct net_device *upper_dev,
+ u16 ref_nr)
{
- return __netdev_adjacent_dev_link_lists(dev, upper_dev,
+ return __netdev_adjacent_dev_link_lists(dev, upper_dev, ref_nr,
&dev->all_adj_list.upper,
&upper_dev->all_adj_list.lower,
NULL, false);
@@ -5713,17 +5717,19 @@ static int __netdev_adjacent_dev_link(struct net_device *dev,
static void __netdev_adjacent_dev_unlink_lists(struct net_device *dev,
struct net_device *upper_dev,
+ u16 ref_nr,
struct list_head *up_list,
struct list_head *down_list)
{
- __netdev_adjacent_dev_remove(dev, upper_dev, up_list);
- __netdev_adjacent_dev_remove(upper_dev, dev, down_list);
+ __netdev_adjacent_dev_remove(dev, upper_dev, ref_nr, up_list);
+ __netdev_adjacent_dev_remove(upper_dev, dev, ref_nr, down_list);
}
static void __netdev_adjacent_dev_unlink(struct net_device *dev,
- struct net_device *upper_dev)
+ struct net_device *upper_dev,
+ u16 ref_nr)
{
- __netdev_adjacent_dev_unlink_lists(dev, upper_dev,
+ __netdev_adjacent_dev_unlink_lists(dev, upper_dev, ref_nr,
&dev->all_adj_list.upper,
&upper_dev->all_adj_list.lower);
}
@@ -5732,17 +5738,17 @@ static int __netdev_adjacent_dev_link_neighbour(struct net_device *dev,
struct net_device *upper_dev,
void *private, bool master)
{
- int ret = __netdev_adjacent_dev_link(dev, upper_dev);
+ int ret = __netdev_adjacent_dev_link(dev, upper_dev, 1);
if (ret)
return ret;
- ret = __netdev_adjacent_dev_link_lists(dev, upper_dev,
+ ret = __netdev_adjacent_dev_link_lists(dev, upper_dev, 1,
&dev->adj_list.upper,
&upper_dev->adj_list.lower,
private, master);
if (ret) {
- __netdev_adjacent_dev_unlink(dev, upper_dev);
+ __netdev_adjacent_dev_unlink(dev, upper_dev, 1);
return ret;
}
@@ -5752,8 +5758,8 @@ static int __netdev_adjacent_dev_link_neighbour(struct net_device *dev,
static void __netdev_adjacent_dev_unlink_neighbour(struct net_device *dev,
struct net_device *upper_dev)
{
- __netdev_adjacent_dev_unlink(dev, upper_dev);
- __netdev_adjacent_dev_unlink_lists(dev, upper_dev,
+ __netdev_adjacent_dev_unlink(dev, upper_dev, 1);
+ __netdev_adjacent_dev_unlink_lists(dev, upper_dev, 1,
&dev->adj_list.upper,
&upper_dev->adj_list.lower);
}
@@ -5806,7 +5812,8 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(j, &upper_dev->all_adj_list.upper, list) {
pr_debug("Interlinking %s with %s, non-neighbour\n",
i->dev->name, j->dev->name);
- ret = __netdev_adjacent_dev_link(i->dev, j->dev);
+ ret = __netdev_adjacent_dev_link(i->dev, j->dev,
+ i->ref_nr);
if (ret)
goto rollback_mesh;
}
@@ -5816,7 +5823,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(i, &upper_dev->all_adj_list.upper, list) {
pr_debug("linking %s's upper device %s with %s\n",
upper_dev->name, i->dev->name, dev->name);
- ret = __netdev_adjacent_dev_link(dev, i->dev);
+ ret = __netdev_adjacent_dev_link(dev, i->dev, i->ref_nr);
if (ret)
goto rollback_upper_mesh;
}
@@ -5825,7 +5832,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(i, &dev->all_adj_list.lower, list) {
pr_debug("linking %s's lower device %s with %s\n", dev->name,
i->dev->name, upper_dev->name);
- ret = __netdev_adjacent_dev_link(i->dev, upper_dev);
+ ret = __netdev_adjacent_dev_link(i->dev, upper_dev, i->ref_nr);
if (ret)
goto rollback_lower_mesh;
}
@@ -5843,7 +5850,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(i, &dev->all_adj_list.lower, list) {
if (i == to_i)
break;
- __netdev_adjacent_dev_unlink(i->dev, upper_dev);
+ __netdev_adjacent_dev_unlink(i->dev, upper_dev, i->ref_nr);
}
i = NULL;
@@ -5853,7 +5860,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(i, &upper_dev->all_adj_list.upper, list) {
if (i == to_i)
break;
- __netdev_adjacent_dev_unlink(dev, i->dev);
+ __netdev_adjacent_dev_unlink(dev, i->dev, i->ref_nr);
}
i = j = NULL;
@@ -5865,7 +5872,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
list_for_each_entry(j, &upper_dev->all_adj_list.upper, list) {
if (i == to_i && j == to_j)
break;
- __netdev_adjacent_dev_unlink(i->dev, j->dev);
+ __netdev_adjacent_dev_unlink(i->dev, j->dev, i->ref_nr);
}
if (i == to_i)
break;
@@ -5945,16 +5952,16 @@ void netdev_upper_dev_unlink(struct net_device *dev,
*/
list_for_each_entry(i, &dev->all_adj_list.lower, list)
list_for_each_entry(j, &upper_dev->all_adj_list.upper, list)
- __netdev_adjacent_dev_unlink(i->dev, j->dev);
+ __netdev_adjacent_dev_unlink(i->dev, j->dev, i->ref_nr);
/* remove also the devices itself from lower/upper device
* list
*/
list_for_each_entry(i, &dev->all_adj_list.lower, list)
- __netdev_adjacent_dev_unlink(i->dev, upper_dev);
+ __netdev_adjacent_dev_unlink(i->dev, upper_dev, i->ref_nr);
list_for_each_entry(i, &upper_dev->all_adj_list.upper, list)
- __netdev_adjacent_dev_unlink(dev, i->dev);
+ __netdev_adjacent_dev_unlink(dev, i->dev, i->ref_nr);
call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
&changeupper_info.info);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net-next] net/vxlan: Avoid unaligned access in vxlan_build_skb()
From: Alexander Duyck @ 2016-09-28 18:08 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: Netdev
In-Reply-To: <20160928170346.GA9263@oracle.com>
On Wed, Sep 28, 2016 at 10:03 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (09/23/16 17:43), Alexander Duyck wrote:
>> > On (09/23/16 10:38), Alexander Duyck wrote:
> ;
>> >> almost think of it as us doing something like the inverse of
>> >> pskb_pull_tail. The general idea here is we want to actually leave
>> >> the data in skb->data, but just reference it from frag[0] so that we
>> >> don't accidentally pull in the 2 byte padding for alignment when
>> >> transmitting the frame.
>
> Some additional findings..
>
> Just to recap how we got here: for the Rx path, the inner packet has
> been set up as an ethernet frame with the IP header at an aligned address
> when it hits vxlan_build_skb. But that means the (inner) mac address
> was offset by NET_IP_ALIGN so vxlan_build_skb needs to pad the data
> by NET_IP_ALIGN to make the vxh outer ip header align.
>
> Then we'd need to do something like the suggestion above (keep some
> pointers in frag[0]? do the reverse of a pskb_expand_head to push out
> the inner ip header to the skb_frag_t?), to have the driver skip over the
> pad..
>
> I tried the following for a hack, and it takes care of the tx side
> unaligned access, though, clearly, the memmove needs to be avoided
>
> @@ -1750,10 +1825,38 @@ static int vxlan_build_skb(struct sk_buff *skb, struct d
> if (err)
> goto out_free;
>
> +
> +#if (NET_IP_ALIGN != 0)
> + {
> + unsigned char *data;
> +
> + /* inner packet is an ethernet frame that was set up
> + * so that the IP header is aligned. But that means the
> + * mac address was offset by NET_IP_ALIGN, so we need
> + * to move things up so that the vxh and outer ip header
> + * are now aligned
> + * XXX The Alexander Duyck idea was to only do the
> + * extra __skb_push() for NET_IP_ALIGN, and avoid the
> + * extram memmove and ->inner* adjustments. Plus keep
> + * additional pointers in frag[0] and have the driver pick
> + * up pointers from frag[0] .. need to investigate
> + * that suggestion further.
> + */
> + data = skb->data;
> + skb->data -= NET_IP_ALIGN;
> + memmove(skb->data, data, skb_headlen(skb));
> + skb->inner_network_header -= NET_IP_ALIGN;
> + skb->inner_mac_header -= NET_IP_ALIGN;
> + skb->inner_transport_header -= NET_IP_ALIGN;
> + }
> +#endif
> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> vxh->vx_flags = VXLAN_HF_VNI;
> vxh->vx_vni = vxlan_vni_field(vni);
>
> In the general case (when the skb passed to vxlan_build_skb is already
> non-linear), wouldn't we end up having to shift all the frags by 1 and/or
> do some type of memory copy of the inner packet? However, I think
> there are some clever things we can do in general, to avoid the memmove..
Right, basically my idea was to just skip the memmove, pull the data
out and add pointers to this spot in the fraglist. Doing that you
should be pulling tail back so it is equal to data. Then you just do
an skb_reserve(skb, -NET_IP_ALIGN) and you should be all set to start
adding outer headers. The problem is you end up having to disable any
offsets such as GSO or checksum offload since you can't really use the
inner header offsets anymore.
> I also looked at the Rx path. Here the suggestion was:
> "we should only pull the outer headers from the page frag, and then
> when the time is right we drop the outer headers and pull the inner
> headers from the page frag. That way we can keep all the headers
> aligned."
> I hacked up ixgbe_add_rx_frag to always only create nonlinear skb's,
> i.e., always avoid the
> memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
> and then I end up with
> - ixgbe_clean_rx_irq copies outer header ether/ip/udp headers
> into linear part as needed,
> - then udp_gro_receive -> vxlan_gro_receive pulls up vxlan header
> into linear part, and then..
This is the point where we need to stop, drop the existing headers,
call skb_reserve(NET_IP_ALIGN), and then pick back up where we left
off. We just have to make sure the skbuff isn't shared.
> - eth_gro_receive pulls up another 34 bytes for the eth + ip header.
> This last pull ends up being unaligned.
> I dont know if we can safely drop the outer ip headers at this point
> (have not tried this yet, and I'm not sure we can do this in all udp
> encaps cases..)
In general the interface behind the tunnel shouldn't need to know
about any of the data in front of the tunnel, so you should be able to
drop the original header offsets and reset things if you want so you
could overwrite the old headers.
> one other possibility is to set up the inner frame as part of the
> ->frag_list (note, this is *not* skb_frag_t). I suspect that is going
> to cause other inefficiencies.
Actually I'm kind of wondering if this might not be the way to go
myself. The overhead for making this kind of transition is sure to be
ugly though as it would require us to update a number of drivers to
support transmitting a fraglist, and we would have to update all the
header manipulation code so that we could realize that inner and outer
header existed in two separate buffers. One advantage though would be
that we could get rid of all the "inner_" header bits from the sk_buff
since we could just use the header offsets stored in the frame hanging
off of the frag_list.
> but, as tom has been saying all along, a big part of this problem is that
> we are tripping up on the ethernet header in the middle of an
> IP packet. Unfortunately I dont think the ietf is going to agree
> to never ever do that, so I'm not sure we can win that architectural battle.
If I am not mistaken I think the multi-buffer approach is the approach
taken by other OSes, although for us it is more difficult since we
have the scatter-gather list that is frags, and then the chained
buffer list which is frag_list. The other gotcha is determining how
many hardware vendors can support having the headers split over 2 DMA
requests. I know in the case of i40e we would have to update the
driver so that the workaround to avoid exceeding 8 descriptors would
have to factor in the inner headers being split off.
I'm sure we are all going to be talking about this in great detail
next week at netdev/netconf.. :-)
- Alex
^ permalink raw reply
* Re: [PATCH v1] mlx4: remove unused fields
From: Eric Dumazet @ 2016-09-28 18:19 UTC (permalink / raw)
To: David Decotigny, Tariq Toukan
Cc: Yishai Hadas, netdev, linux-rdma, linux-kernel, edumazet,
David Decotigny
In-Reply-To: <1475085604-101493-1-git-send-email-ddecotig@gmail.com>
On Wed, 2016-09-28 at 11:00 -0700, David Decotigny wrote:
> From: David Decotigny <decot@googlers.com>
>
> This also can address following UBSAN warnings:
> [ 36.640343] ================================================================================
> [ 36.648772] UBSAN: Undefined behaviour in drivers/net/ethernet/mellanox/mlx4/fw.c:857:26
> [ 36.656853] shift exponent 64 is too large for 32-bit type 'int'
> [ 36.663348] ================================================================================
> [ 36.671783] ================================================================================
> [ 36.680213] UBSAN: Undefined behaviour in drivers/net/ethernet/mellanox/mlx4/fw.c:861:27
> [ 36.688297] shift exponent 35 is too large for 32-bit type 'int'
> [ 36.694702] ================================================================================
>
> Tested:
> reboot with UBSAN, no warning.
>
> Signed-off-by: David Decotigny <decot@googlers.com>
> ---
CC: Tariq Toukan <tariqt@mellanox.com> (mlx4 maintainer)
Note this patch was cooked/tested using net-next, but can be applied to
net tree, with minor fuzz.
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks David.
^ permalink raw reply
* [PATCH] tc: f_u32: Fill in 'linkid' provided by user
From: Sushma Sitaram @ 2016-09-28 18:30 UTC (permalink / raw)
To: stephen, netdev; +Cc: john.r.fastabend
Currently, 'linkid' input by the user is parsed but 'handle' is appended to the netlink message.
# tc filter add dev enp1s0f1 protocol ip parent ffff: prio 99 u32 ht 800: \
order 1 link 1: offset at 0 mask 0f00 shift 6 plus 0 eat match ip \
protocol 6 ff
resulted in:
filter protocol ip pref 99 u32 fh 800::1 order 1 key ht 800 bkt 0
match 00060000/00ff0000 at 8
offset 0f00>>6 at 0 eat
This patch results in:
filter protocol ip pref 99 u32 fh 800::1 order 1 key ht 800 bkt 0 link 1:
match 00060000/00ff0000 at 8
offset 0f00>>6 at 0 eat
Signed-off-by Sushma Sitaram: Sushma Sitaram <sushma.sitaram@intel.com>
---
tc/f_u32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tc/f_u32.c b/tc/f_u32.c
index 0ad7ed2..92c1fcd 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1071,7 +1071,7 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
fprintf(stderr, "\"link\" must be a hash table.\n");
return -1;
}
- addattr_l(n, MAX_MSG, TCA_U32_LINK, &handle, 4);
+ addattr_l(n, MAX_MSG, TCA_U32_LINK, &linkid, 4);
} else if (strcmp(*argv, "ht") == 0) {
unsigned int ht;
^ permalink raw reply related
* [PATCHv3 net 0/3] sctp: a bunch of fixes for prsctp polices
From: Xin Long @ 2016-09-28 18:37 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Vlad Yasevich, daniel
This patchset is to fix 2 issues for prsctp polices:
1. patch 1 and 2 fix "netperf-Throughput_Mbps -37.2% regression" issue
when overloading the CPU.
2. patch 3 fix "prsctp polices should check both sides' prsctp_capable,
instead of only local side".
Xin Long (3):
sctp: move sent_count to the memory hole in sctp_chunk
sctp: remove prsctp_param from sctp_chunk
sctp: change to check peer prsctp_capable when using prsctp polices
include/net/sctp/structs.h | 13 +++----------
net/sctp/chunk.c | 11 ++++++++---
net/sctp/outqueue.c | 12 ++++++------
net/sctp/sm_make_chunk.c | 15 ---------------
4 files changed, 17 insertions(+), 34 deletions(-)
--
2.1.0
^ permalink raw reply
* [PATCHv3 net 1/3] sctp: move sent_count to the memory hole in sctp_chunk
From: Xin Long @ 2016-09-28 18:37 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Vlad Yasevich, daniel
In-Reply-To: <cover.1475087147.git.lucien.xin@gmail.com>
Now pahole sctp_chunk, it has 2 memory holes:
struct sctp_chunk {
struct list_head list;
atomic_t refcnt;
/* XXX 4 bytes hole, try to pack */
...
long unsigned int prsctp_param;
int sent_count;
/* XXX 4 bytes hole, try to pack */
This patch is to move up sent_count to fill the 1st one and eliminate
the 2nd one.
It's not just another struct compaction, it also fixes the "netperf-
Throughput_Mbps -37.2% regression" issue when overloading the CPU.
Fixes: a6c2f792873a ("sctp: implement prsctp TTL policy")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/structs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index ce93c4b..4f097f5 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -554,6 +554,9 @@ struct sctp_chunk {
atomic_t refcnt;
+ /* How many times this chunk have been sent, for prsctp RTX policy */
+ int sent_count;
+
/* This is our link to the per-transport transmitted list. */
struct list_head transmitted_list;
@@ -610,9 +613,6 @@ struct sctp_chunk {
*/
unsigned long prsctp_param;
- /* How many times this chunk have been sent, for prsctp RTX policy */
- int sent_count;
-
/* Which association does this belong to? */
struct sctp_association *asoc;
--
2.1.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox