* [PATCH] iproute2: Add support for CAN presume-ack feature
From: Nikita Edward Baruzdin @ 2014-12-05 8:41 UTC (permalink / raw)
To: linux-can, netdev
In-Reply-To: <1417768902-5404-1-git-send-email-nebaruzdin@gmail.com>
This patch makes CAN_CTRLMODE_PRESUME_ACK netlink feature configurable.
When enabled, the feature sets CAN controller in mode in which
acknowledgement absence is ignored.
Signed-off-by: Nikita Edward Baruzdin <nebaruzdin@gmail.com>
---
ip/iplink_can.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 5b92426..fb50332 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -37,6 +37,7 @@ static void print_usage(FILE *f)
"\t[ one-shot { on | off } ]\n"
"\t[ berr-reporting { on | off } ]\n"
"\t[ fd { on | off } ]\n"
+ "\t[ presume-ack { on | off } ]\n"
"\n"
"\t[ restart-ms TIME-MS ]\n"
"\t[ restart ]\n"
@@ -99,6 +100,7 @@ static void print_ctrlmode(FILE *f, __u32 cm)
_PF(CAN_CTRLMODE_ONE_SHOT, "ONE-SHOT");
_PF(CAN_CTRLMODE_BERR_REPORTING, "BERR-REPORTING");
_PF(CAN_CTRLMODE_FD, "FD");
+ _PF(CAN_CTRLMODE_PRESUME_ACK, "PRESUME-ACK");
#undef _PF
if (cm)
fprintf(f, "%x", cm);
@@ -201,6 +203,10 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
NEXT_ARG();
set_ctrlmode("fd", *argv, &cm,
CAN_CTRLMODE_FD);
+ } else if (matches(*argv, "presume-ack") == 0) {
+ NEXT_ARG();
+ set_ctrlmode("presume-ack", *argv, &cm,
+ CAN_CTRLMODE_PRESUME_ACK);
} else if (matches(*argv, "restart") == 0) {
__u32 val = 1;
--
2.1.3
^ permalink raw reply related
* CAN presume-ack feature
From: Nikita Edward Baruzdin @ 2014-12-05 8:41 UTC (permalink / raw)
To: linux-can, netdev
In-Reply-To: <1417766390-4587-1-git-send-email-nebaruzdin@gmail.com>
Hello, all.
I just remembered about this small patch I promised to submit.
It corresponds to an earlier introduced change in SocketCAN subsystem, see
http://thread.gmane.org/gmane.linux.can/6110
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Julia Lawall @ 2014-12-05 8:40 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Julia Lawall, Joe Perches, Sergei Shtylyov, Paul Mackerras,
linux-ppp, netdev, Eric Dumazet, LKML, kernel-janitors
In-Reply-To: <54816700.6090501@users.sourceforge.net>
On Fri, 5 Dec 2014, SF Markus Elfring wrote:
> > Markus, are you sure that you cannot use netdev_warn in this case?
>
> No. - I did not become familiar enough with the software infrastructure around
> the mppe_rekey() function.
>
> I do not see so far how I could determine a pointer for the first parameter there.
> The data structure "ppp_mppe_state" (which is referenced by the input variable
> "state") does not contain corresponding context information, does it?
Indeed, I also don't see anything promising.
julia
^ permalink raw reply
* Re: [PATCH] net: ethernet: rocker: Add select to CONFIG_BRIDGE in Kconfig
From: Jiri Pirko @ 2014-12-05 8:32 UTC (permalink / raw)
To: Andreas Ruprecht
Cc: David Miller, jim.epost, sfr, linux-next, linux-kernel, sfeldma,
netdev
In-Reply-To: <54816C93.2020009@rupran.de>
Fri, Dec 05, 2014 at 09:28:03AM CET, mail@rupran.de wrote:
>
>> On 05.12.2014 05:24, David Miller wrote:
>> Do not use select, please.
>>
>> You can only use select on leaf node Kconfig symbols, ie. those
>> which do not have any dependencies whatsoever.
>>
>> Select does not recursively walk down the dependency chain turning
>> things on for you when you say "select X".
>>
>
>Okay, sure, my bad for not looking that up properly.
>
>If we change this into a "depends on", this should not be a problem,
>right? If CONFIG_BRIDGE is selected as 'm', then CONFIG_ROCKER can only
>be 'n' or 'm', if CONFIG_BRIDGE is set to 'y', it can be 'n', 'm' or 'y'.
>
>This also means that the prompt associated with CONFIG_ROCKER will only
>show up in menuconfig when CONFIG_BRIDGE has been enabled (either 'm' or
>'y').
>
>I've attached an updated patch.
>From 931a36cc5ec67ec23ba2373d42840d968ed78120 Mon Sep 17 00:00:00 2001
>From: Andreas Ruprecht <rupran@einserver.de>
>Date: Thu, 4 Dec 2014 18:28:09 +0100
>Subject: [PATCH] net: ethernet: rocker: Add dependency to CONFIG_BRIDGE in
> Kconfig
>
>In a configuration with CONFIG_BRIDGE set to 'm' and CONFIG_ROCKER
>set to 'y', undefined references occur at link time:
>
>> drivers/built-in.o: In function `rocker_port_fdb_learn_work':
>> /home/jim/linux/drivers/net/ethernet/rocker/rocker.c:3014: undefined
>> reference to `br_fdb_external_learn_del'
>> /home/jim/linux/drivers/net/ethernet/rocker/rocker.c:3016: undefined
>> reference to `br_fdb_external_learn_add'
>
>This patch fixes these by declaring CONFIG_ROCKER as being dependent
>on CONFIG_BRIDGE.
>
>Reported-by: Jim Davis <jim.epost@gmail.com>
>Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
Acked-by: Jiri Pirko <jiri@resnulli.us>
>---
> drivers/net/ethernet/rocker/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/rocker/Kconfig b/drivers/net/ethernet/rocker/Kconfig
>index 11a850eab628..b9952ef040e4 100644
>--- a/drivers/net/ethernet/rocker/Kconfig
>+++ b/drivers/net/ethernet/rocker/Kconfig
>@@ -17,7 +17,7 @@ if NET_VENDOR_ROCKER
>
> config ROCKER
> tristate "Rocker switch driver (EXPERIMENTAL)"
>- depends on PCI && NET_SWITCHDEV
>+ depends on PCI && NET_SWITCHDEV && BRIDGE
> ---help---
> This driver supports Rocker switch device.
>
>--
>1.9.1
>
^ permalink raw reply
* Re: [RESEND PATCH] net: introduce helper macra CMSG_FOREACH_HDR
From: Gu Zheng @ 2014-12-05 8:29 UTC (permalink / raw)
To: Joe Perches; +Cc: davem, netdev, linux-kernel
In-Reply-To: <1417766552.2721.45.camel@perches.com>
Hi Joe,
Thanks for your comment.
On 12/05/2014 04:02 PM, Joe Perches wrote:
> On Fri, 2014-12-05 at 15:14 +0800, Gu Zheng wrote:
>> Introduce helper macra
>
> macro
Ah~, it's a typo.
>
>> CMSG_FOREACH_HDR as a wrapper of the enumerating
>> cmsghdr from msghdr, just cleanup.
>
> maybe better to use lower case "for_each_cmsg_hdr"
> or some such.
But this will make it out of the ordinary, as the existed ones
are all upper.
David, what's your opinion?
Regards,
Gu
>
> checkpatch would recognize that too as long as it
> uses "for_each".
>
>> diff --git a/include/linux/socket.h b/include/linux/socket.h
> []
>> @@ -94,6 +94,10 @@ struct cmsghdr {
>> (cmsg)->cmsg_len <= (unsigned long) \
>> ((mhdr)->msg_controllen - \
>> ((char *)(cmsg) - (char *)(mhdr)->msg_control)))
>> +#define CMSG_FOREACH_HDR(cmsg, msg) \
>> + for (cmsg = CMSG_FIRSTHDR(msg); \
>> + cmsg; \
>> + cmsg = CMSG_NXTHDR(msg, cmsg))
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* Re: [PATCH] net: ethernet: rocker: Add select to CONFIG_BRIDGE in Kconfig
From: Andreas Ruprecht @ 2014-12-05 8:28 UTC (permalink / raw)
To: David Miller
Cc: jim.epost, sfr, linux-next, linux-kernel, jiri, sfeldma, netdev
In-Reply-To: <20141204.202404.32587724475591669.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 766 bytes --]
> On 05.12.2014 05:24, David Miller wrote:
> Do not use select, please.
>
> You can only use select on leaf node Kconfig symbols, ie. those
> which do not have any dependencies whatsoever.
>
> Select does not recursively walk down the dependency chain turning
> things on for you when you say "select X".
>
Okay, sure, my bad for not looking that up properly.
If we change this into a "depends on", this should not be a problem,
right? If CONFIG_BRIDGE is selected as 'm', then CONFIG_ROCKER can only
be 'n' or 'm', if CONFIG_BRIDGE is set to 'y', it can be 'n', 'm' or 'y'.
This also means that the prompt associated with CONFIG_ROCKER will only
show up in menuconfig when CONFIG_BRIDGE has been enabled (either 'm' or
'y').
I've attached an updated patch.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-net-ethernet-rocker-Add-dependency-to-CONFIG_BRIDGE-.patch --]
[-- Type: text/x-patch; name="0001-net-ethernet-rocker-Add-dependency-to-CONFIG_BRIDGE-.patch", Size: 1439 bytes --]
From 931a36cc5ec67ec23ba2373d42840d968ed78120 Mon Sep 17 00:00:00 2001
From: Andreas Ruprecht <rupran@einserver.de>
Date: Thu, 4 Dec 2014 18:28:09 +0100
Subject: [PATCH] net: ethernet: rocker: Add dependency to CONFIG_BRIDGE in
Kconfig
In a configuration with CONFIG_BRIDGE set to 'm' and CONFIG_ROCKER
set to 'y', undefined references occur at link time:
> drivers/built-in.o: In function `rocker_port_fdb_learn_work':
> /home/jim/linux/drivers/net/ethernet/rocker/rocker.c:3014: undefined
> reference to `br_fdb_external_learn_del'
> /home/jim/linux/drivers/net/ethernet/rocker/rocker.c:3016: undefined
> reference to `br_fdb_external_learn_add'
This patch fixes these by declaring CONFIG_ROCKER as being dependent
on CONFIG_BRIDGE.
Reported-by: Jim Davis <jim.epost@gmail.com>
Signed-off-by: Andreas Ruprecht <rupran@einserver.de>
---
drivers/net/ethernet/rocker/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/rocker/Kconfig b/drivers/net/ethernet/rocker/Kconfig
index 11a850eab628..b9952ef040e4 100644
--- a/drivers/net/ethernet/rocker/Kconfig
+++ b/drivers/net/ethernet/rocker/Kconfig
@@ -17,7 +17,7 @@ if NET_VENDOR_ROCKER
config ROCKER
tristate "Rocker switch driver (EXPERIMENTAL)"
- depends on PCI && NET_SWITCHDEV
+ depends on PCI && NET_SWITCHDEV && BRIDGE
---help---
This driver supports Rocker switch device.
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: SF Markus Elfring @ 2014-12-05 8:04 UTC (permalink / raw)
To: Julia Lawall, Joe Perches
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors
In-Reply-To: <alpine.DEB.2.02.1412050726000.2059@localhost6.localdomain6>
> Markus, are you sure that you cannot use netdev_warn in this case?
No. - I did not become familiar enough with the software infrastructure around
the mppe_rekey() function.
I do not see so far how I could determine a pointer for the first parameter there.
The data structure "ppp_mppe_state" (which is referenced by the input variable
"state") does not contain corresponding context information, does it?
Regards,
Markus
^ permalink raw reply
* Re: [RESEND PATCH] net: introduce helper macra CMSG_FOREACH_HDR
From: Joe Perches @ 2014-12-05 8:02 UTC (permalink / raw)
To: Gu Zheng; +Cc: davem, netdev, linux-kernel
In-Reply-To: <54815B40.1050909@cn.fujitsu.com>
On Fri, 2014-12-05 at 15:14 +0800, Gu Zheng wrote:
> Introduce helper macra
macro
> CMSG_FOREACH_HDR as a wrapper of the enumerating
> cmsghdr from msghdr, just cleanup.
maybe better to use lower case "for_each_cmsg_hdr"
or some such.
checkpatch would recognize that too as long as it
uses "for_each".
> diff --git a/include/linux/socket.h b/include/linux/socket.h
[]
> @@ -94,6 +94,10 @@ struct cmsghdr {
> (cmsg)->cmsg_len <= (unsigned long) \
> ((mhdr)->msg_controllen - \
> ((char *)(cmsg) - (char *)(mhdr)->msg_control)))
> +#define CMSG_FOREACH_HDR(cmsg, msg) \
> + for (cmsg = CMSG_FIRSTHDR(msg); \
> + cmsg; \
> + cmsg = CMSG_NXTHDR(msg, cmsg))
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Joe Perches @ 2014-12-05 7:57 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors, Julia Lawall
In-Reply-To: <54815C5F.8090702@users.sourceforge.net>
On Fri, 2014-12-05 at 08:18 +0100, SF Markus Elfring wrote:
> >>> It's generally nicer to replace embedded function names
> >>> with "%s: ", __func__
> >>>
> >>> pr_warn("%s: cipher_encrypt failed\n", __func__);
> >>
> >> Do you want that I send a third patch series for the fine-tuning of these parameters?
> >
> > If you want.
>
> Would "a committer" fix such a small source code adjustment also without a resend of
> a patch series?
Depends on the committer. Some might, most wouldn't.
drivers/net/ppp doesn't have a specific maintainer.
The networking maintainer generally asks for resends
of patches that don't suit his taste, but lots of
non-perfect patches still get applied there.
It's a process, and it's not immediate. Wait to see
if these get applied as-is. If the embedded function
name use, which is trivial, bothers you, send another
patch later on that changes it.
> Does it make sense to express such implementation details in the Linux coding
> style documentation more explicitly (besides the fact that this update suggestion
> was also triggered by a warning from the script "checkpatch.pl").
Probably not.
Overly formalized coding style rules are perhaps
more of a barrier to entry than most want.
^ permalink raw reply
* Re: [PATCH iproute2] bridge link: add option 'self'
From: Jiri Pirko @ 2014-12-05 7:55 UTC (permalink / raw)
To: roopa
Cc: sfeldma, jhs, bcrl, tgraf, john.fastabend, stephen, linville,
nhorman, nicolas.dichtel, vyasevic, f.fainelli, buytenh, aviadr,
netdev, davem, shm, gospo
In-Reply-To: <1417746436-41023-1-git-send-email-roopa@cumulusnetworks.com>
Fri, Dec 05, 2014 at 03:27:16AM CET, roopa@cumulusnetworks.com wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>Currently self is set internally only if hwmode is set.
>This makes it necessary for the hw to have a mode.
>There is no hwmode really required to go to hardware. So, introduce
>self for anybody who wants to target hardware.
Signed-off line is missing.
Other than that, the patch looks fine.
Reviewed-by: Jiri Pirko <jiri@resnulli.us>
Feel free to add my review line to the repost.
>---
> bridge/link.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/bridge/link.c b/bridge/link.c
>index 90d9e7f..2b86141 100644
>--- a/bridge/link.c
>+++ b/bridge/link.c
>@@ -321,6 +321,9 @@ static int brlink_modify(int argc, char **argv)
> "\"veb\".\n");
> exit(-1);
> }
>+ } else if (strcmp(*argv, "self") == 0) {
>+ NEXT_ARG();
>+ flags = BRIDGE_FLAGS_SELF;
> } else {
> usage();
> }
>--
>1.7.10.4
>
^ permalink raw reply
* Re: [PATCH 1/3] netdev: introduce new NETIF_F_HW_SWITCH_OFFLOAD feature flag for switch device offloads
From: Jiri Pirko @ 2014-12-05 7:41 UTC (permalink / raw)
To: roopa
Cc: sfeldma, jhs, bcrl, tgraf, john.fastabend, stephen, linville,
nhorman, nicolas.dichtel, vyasevic, f.fainelli, buytenh, aviadr,
netdev, davem, shm, gospo
In-Reply-To: <1417746401-8140-2-git-send-email-roopa@cumulusnetworks.com>
Fri, Dec 05, 2014 at 03:26:39AM CET, roopa@cumulusnetworks.com wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>This is a generic high level feature flag for all switch asic features today.
>
>switch drivers set this flag on switch ports. Logical devices like
>bridge, bonds, vxlans can inherit this flag from their slaves/ports.
Can you please elaborate on how exactly would this inheritance look
like?
>
>I had to use SWITCH in the name to avoid ambiguity with other feature
>flags. But, since i have been harping about not calling it 'switch',
>I am welcome to any suggestions :)
>
>An alternative to using a feature flag is to use a IFF_HW_OFFLOAD
>in net_device_flags.
>---
> include/linux/netdev_features.h | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>index 8e30685..68db1de 100644
>--- a/include/linux/netdev_features.h
>+++ b/include/linux/netdev_features.h
>@@ -66,6 +66,7 @@ enum {
> NETIF_F_HW_VLAN_STAG_FILTER_BIT,/* Receive filtering on VLAN STAGs */
> NETIF_F_HW_L2FW_DOFFLOAD_BIT, /* Allow L2 Forwarding in Hardware */
> NETIF_F_BUSY_POLL_BIT, /* Busy poll */
>+ NETIF_F_HW_SWITCH_OFFLOAD_BIT, /* HW switch offload */
>
> /*
> * Add your fresh new feature above and remember to update
>@@ -124,6 +125,7 @@ enum {
> #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX)
> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD)
> #define NETIF_F_BUSY_POLL __NETIF_F(BUSY_POLL)
>+#define NETIF_F_HW_SWITCH_OFFLOAD __NETIF_F(HW_SWITCH_OFFLOAD)
>
> /* Features valid for ethtool to change */
> /* = all defined minus driver/device-class-related */
>--
>1.7.10.4
>
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Joe Perches @ 2014-12-05 7:41 UTC (permalink / raw)
To: Julia Lawall
Cc: SF Markus Elfring, Sergei Shtylyov, Paul Mackerras, linux-ppp,
netdev, Eric Dumazet, LKML, kernel-janitors
In-Reply-To: <alpine.DEB.2.02.1412050820290.2059@localhost6.localdomain6>
On Fri, 2014-12-05 at 08:21 +0100, Julia Lawall wrote:
> On Thu, 4 Dec 2014, Joe Perches wrote:
> > It's generally nicer to replace embedded function names
> > with "%s: ", __func__
> >
> > pr_warn("%s: cipher_encrypt failed\n", __func__);
>
> Doing so may potentially allow some strings to be shared, thus saving a
> little space. Perhaps not in this case, though.
It's not necessarily a code size savings in any case.
It can be, but the real benefits are stylistic
consistency and lack of mismatch between function
name and message.
If the code is refactored or copy/pasted into another
function, a moderately common defect is not modifying
the embedded function name in the message.
There may be some smallish savings if ever these
__func__ uses were converted to use %pf via some
internal standardized mechanism.
A negative to that approach is inlined functions would
take the function name of the parent not keep the
inlined function name.
^ permalink raw reply
* Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set
From: Jiri Pirko @ 2014-12-05 7:38 UTC (permalink / raw)
To: roopa
Cc: sfeldma, jhs, bcrl, tgraf, john.fastabend, stephen, linville,
nhorman, nicolas.dichtel, vyasevic, f.fainelli, buytenh, aviadr,
netdev, davem, shm, gospo
In-Reply-To: <1417746401-8140-3-git-send-email-roopa@cumulusnetworks.com>
Fri, Dec 05, 2014 at 03:26:40AM CET, roopa@cumulusnetworks.com wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>This allows offloading to switch asic without having the user to set
>any flag. And this is done in the bridge driver to rollback kernel settings
>on hw offload failure if required in the future.
>
>With this, it also makes sure a notification goes out only after the
>attributes are set both in the kernel and hw.
>---
> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
>diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>index 9f5eb55..ce173f0 100644
>--- a/net/bridge/br_netlink.c
>+++ b/net/bridge/br_netlink.c
>@@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
> afspec, RTM_SETLINK);
> }
>
>+ if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
>+ dev->netdev_ops->ndo_bridge_setlink) {
>+ int ret = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
This (and I suspect other patches as well) has many issues which
are pointed out by scripts/checkpatch.pl sctipts. For example
here, there should be an empty line. Please run your patches by
this script before you send them.
>+ if (ret && ret != -EOPNOTSUPP) {
>+ /* XXX Fix this in the future to rollback
>+ * kernel settings and return error
>+ */
>+ br_warn(p->br, "error offloading bridge attributes "
>+ "on port %u(%s)\n", (unsigned int) p->port_no,
>+ p->dev->name);
>+ }
>+ }
>+
> if (err == 0)
> br_ifinfo_notify(RTM_NEWLINK, p);
>-
> out:
> return err;
> }
>@@ -433,6 +445,19 @@ int br_dellink(struct net_device *dev, struct nlmsghdr *nlh)
> err = br_afspec((struct net_bridge *)netdev_priv(dev), p,
> afspec, RTM_DELLINK);
>
>+ if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD
>+ && dev->netdev_ops->ndo_bridge_setlink) {
>+ int ret = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
c&p issue, there should be check for dellink here.
>+ if (ret && ret != -EOPNOTSUPP) {
>+ /* XXX Fix this in the future to rollback
>+ * kernel settings and return error
>+ */
>+ br_warn(p->br, "error offloading bridge attributes "
>+ "on port %u(%s)\n", (unsigned int) p->port_no,
>+ p->dev->name);
>+ }
>+ }
>+
I agree with Scott that this code should be moved to rtnetlink.c
> return err;
> }
> static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>--
>1.7.10.4
>
^ permalink raw reply
* Re: [PATCH v3 net-next 2/2 tuntap: Increase the number of queues in tun.
From: Jason Wang @ 2014-12-05 7:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Pankaj Gupta, linux-kernel, netdev, davem, dgibson, vfalico,
edumazet, vyasevic, hkchu, wuzhy, xemul, therbert, bhutchings,
xii, stephen, jiri, sergei.shtylyov
In-Reply-To: <20141204102013.GC17122@redhat.com>
On 12/04/2014 06:20 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 04, 2014 at 03:03:34AM +0008, Jason Wang wrote:
>> >
>> >
>> > On Wed, Dec 3, 2014 at 5:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > >On Wed, Dec 03, 2014 at 12:49:37PM +0530, Pankaj Gupta wrote:
>>>> > >> Networking under kvm works best if we allocate a per-vCPU RX and TX
>>>> > >> queue in a virtual NIC. This requires a per-vCPU queue on the host
>>>> > >>side.
>>>> > >> It is now safe to increase the maximum number of queues.
>>>> > >> Preceding patche: 'net: allow large number of rx queues'
>>> > >
>>> > >s/patche/patch/
>>> > >
>>>> > >> made sure this won't cause failures due to high order memory
>>>> > >> allocations. Increase it to 256: this is the max number of vCPUs
>>>> > >> KVM supports.
>>>> > >> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>>> > >> Reviewed-by: David Gibson <dgibson@redhat.com>
>>> > >
>>> > >Hmm it's kind of nasty that each tun device is now using x16 memory.
>>> > >Maybe we should look at using a flex array instead, and removing the
>>> > >limitation altogether (e.g. make it INT_MAX)?
>> >
>> > But this only happens when IFF_MULTIQUEUE were used.
> I refer to this field:
> struct tun_file __rcu *tfiles[MAX_TAP_QUEUES];
> if we make MAX_TAP_QUEUES 256, this will use 4K bytes,
> apparently unconditionally.
>
>
How about just allocate one tfile if IFF_MULTIQUEUE were disabled?
^ permalink raw reply
* Re: [patch] ipvs: uninitialized data with IP_VS_IPV6
From: Dan Carpenter @ 2014-12-05 7:25 UTC (permalink / raw)
To: Julian Anastasov
Cc: Wensong Zhang, Simon Horman, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, netdev, lvs-devel,
netfilter-devel, coreteam, kernel-janitors
In-Reply-To: <alpine.LFD.2.11.1412042308100.4841@ja.home.ssi.bg>
On Thu, Dec 04, 2014 at 11:19:34PM +0200, Julian Anastasov wrote:
>
> I guess ip_vs_ftp_in() needs the same fix?
Good catch. Thanks for noticing that. I'll send a v2.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Julia Lawall @ 2014-12-05 7:21 UTC (permalink / raw)
To: Joe Perches
Cc: SF Markus Elfring, Sergei Shtylyov, Paul Mackerras, linux-ppp,
netdev, Eric Dumazet, LKML, kernel-janitors
In-Reply-To: <1417731809.2721.17.camel@perches.com>
On Thu, 4 Dec 2014, Joe Perches wrote:
> On Thu, 2014-12-04 at 23:10 +0100, SF Markus Elfring wrote:
> > The mppe_rekey() function contained a few update candidates.
> > * Curly brackets were still used around a single function call "printk".
> > * Unwanted space characters
> >
> > Let us improve these implementation details according to the current Linux
> > coding style convention.
>
> trivia:
>
> > diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> []
> > @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> > setup_sg(sg_in, state->sha1_digest, state->keylen);
> > setup_sg(sg_out, state->session_key, state->keylen);
> > if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> > - state->keylen) != 0) {
> > - printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> > - }
> > + state->keylen) != 0)
> > + pr_warn("mppe_rekey: cipher_encrypt failed\n");
>
> It's generally nicer to replace embedded function names
> with "%s: ", __func__
>
> pr_warn("%s: cipher_encrypt failed\n", __func__);
Doing so may potentially allow some strings to be shared, thus saving a
little space. Perhaps not in this case, though.
julia
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: SF Markus Elfring @ 2014-12-05 7:18 UTC (permalink / raw)
To: Joe Perches
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1417733101.2721.20.camel@perches.com>
>>> It's generally nicer to replace embedded function names
>>> with "%s: ", __func__
>>>
>>> pr_warn("%s: cipher_encrypt failed\n", __func__);
>>
>> Do you want that I send a third patch series for the fine-tuning of these parameters?
>
> If you want.
Would "a committer" fix such a small source code adjustment also without a resend of
a patch series?
> I just wanted you to be aware of it for future patches.
Thanks for your tip.
Does it make sense to express such implementation details in the Linux coding
style documentation more explicitly (besides the fact that this update suggestion
was also triggered by a warning from the script "checkpatch.pl").
Regards,
Markus
^ permalink raw reply
* [RESEND PATCH] net/socket.c : introduce helper function do_sock_sendmsg to replace reduplicate code
From: Gu Zheng @ 2014-12-05 7:14 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, Gu Zheng
In-Reply-To: <1416799125-13972-1-git-send-email-guz.fnst@cn.fujitsu.com>
Introduce helper function do_sock_sendmsg() to simplify sock_sendmsg{_nosec},
and replace reduplicate code.
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
net/socket.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index fe20c31..2a6f3c6 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -651,7 +651,8 @@ static inline int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,
return err ?: __sock_sendmsg_nosec(iocb, sock, msg, size);
}
-int sock_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
+static int do_sock_sendmsg(struct socket *sock, struct msghdr *msg,
+ size_t size, bool nosec)
{
struct kiocb iocb;
struct sock_iocb siocb;
@@ -659,25 +660,22 @@ int sock_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
init_sync_kiocb(&iocb, NULL);
iocb.private = &siocb;
- ret = __sock_sendmsg(&iocb, sock, msg, size);
+ ret = nosec ? __sock_sendmsg_nosec(&iocb, sock, msg, size) :
+ __sock_sendmsg(&iocb, sock, msg, size);
if (-EIOCBQUEUED == ret)
ret = wait_on_sync_kiocb(&iocb);
return ret;
}
+
+int sock_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
+{
+ return do_sock_sendmsg(sock, msg, size, false);
+}
EXPORT_SYMBOL(sock_sendmsg);
static int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
{
- struct kiocb iocb;
- struct sock_iocb siocb;
- int ret;
-
- init_sync_kiocb(&iocb, NULL);
- iocb.private = &siocb;
- ret = __sock_sendmsg_nosec(&iocb, sock, msg, size);
- if (-EIOCBQUEUED == ret)
- ret = wait_on_sync_kiocb(&iocb);
- return ret;
+ return do_sock_sendmsg(sock, msg, size, true);
}
int kernel_sendmsg(struct socket *sock, struct msghdr *msg,
--
1.7.7
.
^ permalink raw reply related
* [RESEND PATCH] net: introduce helper macra CMSG_FOREACH_HDR
From: Gu Zheng @ 2014-12-05 7:14 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, Gu Zheng
Introduce helper macra CMSG_FOREACH_HDR as a wrapper of the enumerating
cmsghdr from msghdr, just cleanup.
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
.../networking/timestamping/timestamping.c | 4 +---
.../networking/timestamping/txtimestamp.c | 4 +---
crypto/af_alg.c | 2 +-
include/linux/socket.h | 4 ++++
net/core/scm.c | 3 +--
net/dccp/proto.c | 5 ++---
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/datagram.c | 2 +-
net/iucv/af_iucv.c | 4 +---
net/rds/send.c | 4 ++--
net/rxrpc/ar-output.c | 2 +-
net/sctp/socket.c | 3 +--
12 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/Documentation/networking/timestamping/timestamping.c b/Documentation/networking/timestamping/timestamping.c
index 5cdfd74..3106e88 100644
--- a/Documentation/networking/timestamping/timestamping.c
+++ b/Documentation/networking/timestamping/timestamping.c
@@ -169,9 +169,7 @@ static void printpacket(struct msghdr *msg, int res,
res,
inet_ntoa(from_addr->sin_addr),
msg->msg_controllen);
- for (cmsg = CMSG_FIRSTHDR(msg);
- cmsg;
- cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
printf(" cmsg len %zu: ", cmsg->cmsg_len);
switch (cmsg->cmsg_level) {
case SOL_SOCKET:
diff --git a/Documentation/networking/timestamping/txtimestamp.c b/Documentation/networking/timestamping/txtimestamp.c
index b32fc2a..e44ef35 100644
--- a/Documentation/networking/timestamping/txtimestamp.c
+++ b/Documentation/networking/timestamping/txtimestamp.c
@@ -149,9 +149,7 @@ static void __recv_errmsg_cmsg(struct msghdr *msg, int payload_len)
struct scm_timestamping *tss = NULL;
struct cmsghdr *cm;
- for (cm = CMSG_FIRSTHDR(msg);
- cm && cm->cmsg_len;
- cm = CMSG_NXTHDR(msg, cm)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (cm->cmsg_level == SOL_SOCKET &&
cm->cmsg_type == SCM_TIMESTAMPING) {
tss = (void *) CMSG_DATA(cm);
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 6a3ad80..3df7d53 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -399,7 +399,7 @@ int af_alg_cmsg_send(struct msghdr *msg, struct af_alg_control *con)
{
struct cmsghdr *cmsg;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
if (cmsg->cmsg_level != SOL_ALG)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index bb9b836..d4b592f 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -94,6 +94,10 @@ struct cmsghdr {
(cmsg)->cmsg_len <= (unsigned long) \
((mhdr)->msg_controllen - \
((char *)(cmsg) - (char *)(mhdr)->msg_control)))
+#define CMSG_FOREACH_HDR(cmsg, msg) \
+ for (cmsg = CMSG_FIRSTHDR(msg); \
+ cmsg; \
+ cmsg = CMSG_NXTHDR(msg, cmsg))
/*
* Get the next cmsg header
diff --git a/net/core/scm.c b/net/core/scm.c
index b442e7e..e938c49 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -129,8 +129,7 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p)
struct cmsghdr *cmsg;
int err;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg))
- {
+ CMSG_FOREACH_HDR(cmsg, msg) {
err = -EINVAL;
/* Verify that cmsg_len is at least sizeof(struct cmsghdr) */
diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index 5ab6627..d449cc5 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -703,7 +703,7 @@ EXPORT_SYMBOL_GPL(compat_dccp_getsockopt);
static int dccp_msghdr_parse(struct msghdr *msg, struct sk_buff *skb)
{
- struct cmsghdr *cmsg = CMSG_FIRSTHDR(msg);
+ struct cmsghdr *cmsg;
/*
* Assign an (opaque) qpolicy priority value to skb->priority.
@@ -717,8 +717,7 @@ static int dccp_msghdr_parse(struct msghdr *msg, struct sk_buff *skb)
*/
skb->priority = 0;
- for (; cmsg != NULL; cmsg = CMSG_NXTHDR(msg, cmsg)) {
-
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 9daf217..14a6f71 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -192,7 +192,7 @@ int ip_cmsg_send(struct net *net, struct msghdr *msg, struct ipcm_cookie *ipc,
int err, val;
struct cmsghdr *cmsg;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
#if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 2cdc383..9895b98 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -640,7 +640,7 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
int len;
int err = 0;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
int addr_type;
if (!CMSG_OK(msg, cmsg)) {
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index a089b6b..eae4e08 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1070,9 +1070,7 @@ static int iucv_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
txmsg.class = 0;
/* iterate over control messages */
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg;
- cmsg = CMSG_NXTHDR(msg, cmsg)) {
-
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg)) {
err = -EINVAL;
goto out;
diff --git a/net/rds/send.c b/net/rds/send.c
index 0a64541..1accb3e 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -826,7 +826,7 @@ static int rds_rm_size(struct msghdr *msg, int data_len)
int cmsg_groups = 0;
int retval;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
@@ -878,7 +878,7 @@ static int rds_cmsg_send(struct rds_sock *rs, struct rds_message *rm,
struct cmsghdr *cmsg;
int ret = 0;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
diff --git a/net/rxrpc/ar-output.c b/net/rxrpc/ar-output.c
index 0b4b9a7..f915e7e 100644
--- a/net/rxrpc/ar-output.c
+++ b/net/rxrpc/ar-output.c
@@ -45,7 +45,7 @@ static int rxrpc_sendmsg_cmsg(struct rxrpc_sock *rx, struct msghdr *msg,
if (msg->msg_controllen == 0)
return -EINVAL;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, msg) {
if (!CMSG_OK(msg, cmsg))
return -EINVAL;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 634a2ab..58834d7 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -6592,8 +6592,7 @@ static int sctp_msghdr_parse(const struct msghdr *msg, sctp_cmsgs_t *cmsgs)
struct cmsghdr *cmsg;
struct msghdr *my_msg = (struct msghdr *)msg;
- for (cmsg = CMSG_FIRSTHDR(msg); cmsg != NULL;
- cmsg = CMSG_NXTHDR(my_msg, cmsg)) {
+ CMSG_FOREACH_HDR(cmsg, my_msg) {
if (!CMSG_OK(my_msg, cmsg))
return -EINVAL;
--
1.7.7
.
^ permalink raw reply related
* Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set
From: Roopa Prabhu @ 2014-12-05 7:10 UTC (permalink / raw)
To: John Fastabend
Cc: Scott Feldman, Jiří Pírko, Jamal Hadi Salim,
Benjamin LaHaise, Thomas Graf, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <548156C8.5060602@gmail.com>
On 12/4/14, 10:55 PM, John Fastabend wrote:
> On 12/04/2014 10:41 PM, Scott Feldman wrote:
>> On Thu, Dec 4, 2014 at 6:26 PM, <roopa@cumulusnetworks.com> wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> This allows offloading to switch asic without having the user to set
>>> any flag. And this is done in the bridge driver to rollback kernel
>>> settings
>>> on hw offload failure if required in the future.
>>>
>>> With this, it also makes sure a notification goes out only after the
>>> attributes are set both in the kernel and hw.
>>
>> I like this approach as it streamlines the steps for the user in
>> setting port flags. There is one case for FLOODING where you'll have
>> to turn off flooding for both, and then turn on flooding in hw. You
>> don't want flooding turned on on kernel and hw.
>>
>>> ---
>>> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++-
>>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index 9f5eb55..ce173f0 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct
>>> nlmsghdr *nlh)
>>> afspec, RTM_SETLINK);
>>> }
>>>
>>> + if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
>>> + dev->netdev_ops->ndo_bridge_setlink) {
>>> + int ret = dev->netdev_ops->ndo_bridge_setlink(dev,
>>> nlh);
>>
>> I think you want to up-level this to net/core/rtnetlink.c because
>> you're only enabling the feature for one instance of a driver that
>> implements ndo_bridge_setlink: the bridge driver. If another driver
>> was MASTER and implemented ndo_bridge_setlink, you'd want same check
>> to push setting down to SELF port driver.
>
> Also if the user set SELF && MASTER flags && had HW_SWITCH_OFFLOAD bit
> set we would call ndo_bridge_setlink twice on the dev. Maybe you can
> catch this case in rtnetlink.c and only call it once.
yes, thought about this and when i looked at iproute2 code, it is either
master
or self today and i don't think anybody else uses both flags with the
current
kernel implementation. But yes, that does not stop anybody from setting
both flags.
I will handle it better in v2.
>
>>
>>> + if (ret && ret != -EOPNOTSUPP) {
>>> + /* XXX Fix this in the future to rollback
>>> + * kernel settings and return error
>>> + */
>>
>> The future is now. Let's fix this now for the rollback case (again up
>> in rtnetlink.c). So then a general question comes to mind: for these
>> dual target sets, is it best to try HW first and then SW, or the other
>> way around? Either way, on failure on second you need to rollback
>> first. And, on failure, you need to know rollback value for first, so
>> you have to do a getlink on first before attempting set.
>
> It might be helpful to return some indication of what object failed as
> well.
ok...
thanks,
Roopa
^ permalink raw reply
* Re: [PATCH iproute2] bridge link: add option 'self'
From: Roopa Prabhu @ 2014-12-05 7:04 UTC (permalink / raw)
To: Scott Feldman
Cc: Jiří Pírko, Jamal Hadi Salim, Benjamin LaHaise,
Thomas Graf, john fastabend, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <CAE4R7bA3JSyHxe5ptVzpCzP5nHmKm8KyPqGvK7a_N2TWX-BNRw@mail.gmail.com>
On 12/4/14, 10:52 PM, Scott Feldman wrote:
> On Thu, Dec 4, 2014 at 6:27 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Currently self is set internally only if hwmode is set.
>> This makes it necessary for the hw to have a mode.
>> There is no hwmode really required to go to hardware. So, introduce
>> self for anybody who wants to target hardware.
> Can you include unwinding the hwmode 'swdev' setting for
> kernel/iproute2 in v2? With this "self" option, and your new
> NETIF_F_HW_SWITCH_OFFLOAD flag, we don't need the hwmde 'swdev'.
yep. will do. thanks
>> ---
>> bridge/link.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/bridge/link.c b/bridge/link.c
>> index 90d9e7f..2b86141 100644
>> --- a/bridge/link.c
>> +++ b/bridge/link.c
>> @@ -321,6 +321,9 @@ static int brlink_modify(int argc, char **argv)
>> "\"veb\".\n");
>> exit(-1);
>> }
>> + } else if (strcmp(*argv, "self") == 0) {
>> + NEXT_ARG();
>> + flags = BRIDGE_FLAGS_SELF;
>> } else {
>> usage();
>> }
>> --
>> 1.7.10.4
>>
^ permalink raw reply
* Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set
From: Roopa Prabhu @ 2014-12-05 7:02 UTC (permalink / raw)
To: Scott Feldman
Cc: Jiří Pírko, Jamal Hadi Salim, Benjamin LaHaise,
Thomas Graf, john fastabend, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <CAE4R7bBOCgoH0zL6+82jj2qjcP8n3kSo51x+i5SoqkN8Ff1CBQ@mail.gmail.com>
On 12/4/14, 10:41 PM, Scott Feldman wrote:
> On Thu, Dec 4, 2014 at 6:26 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This allows offloading to switch asic without having the user to set
>> any flag. And this is done in the bridge driver to rollback kernel settings
>> on hw offload failure if required in the future.
>>
>> With this, it also makes sure a notification goes out only after the
>> attributes are set both in the kernel and hw.
> I like this approach as it streamlines the steps for the user in
> setting port flags. There is one case for FLOODING where you'll have
> to turn off flooding for both, and then turn on flooding in hw. You
> don't want flooding turned on on kernel and hw.
ok, maybe using the higher bits as in
https://patchwork.ozlabs.org/patch/413211/
might help with that. Let me think some more.
>
>> ---
>> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 9f5eb55..ce173f0 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
>> afspec, RTM_SETLINK);
>> }
>>
>> + if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
>> + dev->netdev_ops->ndo_bridge_setlink) {
>> + int ret = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
> I think you want to up-level this to net/core/rtnetlink.c because
> you're only enabling the feature for one instance of a driver that
> implements ndo_bridge_setlink: the bridge driver. If another driver
> was MASTER and implemented ndo_bridge_setlink, you'd want same check
> to push setting down to SELF port driver.
yeah, i thought about that. But i moved it here so that rollback would
be easier.
>
>> + if (ret && ret != -EOPNOTSUPP) {
>> + /* XXX Fix this in the future to rollback
>> + * kernel settings and return error
>> + */
> The future is now. Let's fix this now for the rollback case (again up
> in rtnetlink.c). So then a general question comes to mind: for these
> dual target sets, is it best to try HW first and then SW, or the other
> way around? Either way, on failure on second you need to rollback
> first. And, on failure, you need to know rollback value for first, so
> you have to do a getlink on first before attempting set.
yep, exactly, I went through the same thought process yesterday when i
was trying to implement rollback.
>
>> + br_warn(p->br, "error offloading bridge attributes "
>> + "on port %u(%s)\n", (unsigned int) p->port_no,
>> + p->dev->name);
>> + }
>> + }
>> +
>> if (err == 0)
>> br_ifinfo_notify(RTM_NEWLINK, p);
>> -
>> out:
>> return err;
>> }
>> @@ -433,6 +445,19 @@ int br_dellink(struct net_device *dev, struct nlmsghdr *nlh)
>> err = br_afspec((struct net_bridge *)netdev_priv(dev), p,
>> afspec, RTM_DELLINK);
>>
>> + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD
>> + && dev->netdev_ops->ndo_bridge_setlink) {
>> + int ret = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
>> + if (ret && ret != -EOPNOTSUPP) {
>> + /* XXX Fix this in the future to rollback
>> + * kernel settings and return error
>> + */
>> + br_warn(p->br, "error offloading bridge attributes "
>> + "on port %u(%s)\n", (unsigned int) p->port_no,
>> + p->dev->name);
>> + }
>> + }
>> +
> Same comments as setlink above.
>
>> return err;
>> }
>> static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>> --
>> 1.7.10.4
>>
^ permalink raw reply
* Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set
From: John Fastabend @ 2014-12-05 6:55 UTC (permalink / raw)
To: Scott Feldman
Cc: Roopa Prabhu, Jiří Pírko, Jamal Hadi Salim,
Benjamin LaHaise, Thomas Graf, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <CAE4R7bBOCgoH0zL6+82jj2qjcP8n3kSo51x+i5SoqkN8Ff1CBQ@mail.gmail.com>
On 12/04/2014 10:41 PM, Scott Feldman wrote:
> On Thu, Dec 4, 2014 at 6:26 PM, <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This allows offloading to switch asic without having the user to set
>> any flag. And this is done in the bridge driver to rollback kernel settings
>> on hw offload failure if required in the future.
>>
>> With this, it also makes sure a notification goes out only after the
>> attributes are set both in the kernel and hw.
>
> I like this approach as it streamlines the steps for the user in
> setting port flags. There is one case for FLOODING where you'll have
> to turn off flooding for both, and then turn on flooding in hw. You
> don't want flooding turned on on kernel and hw.
>
>> ---
>> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 9f5eb55..ce173f0 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
>> afspec, RTM_SETLINK);
>> }
>>
>> + if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
>> + dev->netdev_ops->ndo_bridge_setlink) {
>> + int ret = dev->netdev_ops->ndo_bridge_setlink(dev, nlh);
>
> I think you want to up-level this to net/core/rtnetlink.c because
> you're only enabling the feature for one instance of a driver that
> implements ndo_bridge_setlink: the bridge driver. If another driver
> was MASTER and implemented ndo_bridge_setlink, you'd want same check
> to push setting down to SELF port driver.
Also if the user set SELF && MASTER flags && had HW_SWITCH_OFFLOAD bit
set we would call ndo_bridge_setlink twice on the dev. Maybe you can
catch this case in rtnetlink.c and only call it once.
>
>> + if (ret && ret != -EOPNOTSUPP) {
>> + /* XXX Fix this in the future to rollback
>> + * kernel settings and return error
>> + */
>
> The future is now. Let's fix this now for the rollback case (again up
> in rtnetlink.c). So then a general question comes to mind: for these
> dual target sets, is it best to try HW first and then SW, or the other
> way around? Either way, on failure on second you need to rollback
> first. And, on failure, you need to know rollback value for first, so
> you have to do a getlink on first before attempting set.
It might be helpful to return some indication of what object failed as
well.
>
>> + br_warn(p->br, "error offloading bridge attributes "
>> + "on port %u(%s)\n", (unsigned int) p->port_no,
>> + p->dev->name);
>> + }
>> + }
>> +
>> if (err == 0)
>> br_ifinfo_notify(RTM_NEWLINK, p);
>> -
>> out:
>> return err;
>> }
>> @@ -433,6 +445,19 @@ int br_dellink(struct net_device *dev, struct nlmsghdr *nlh)
>> err = br_afspec((struct net_bridge *)netdev_priv(dev), p,
>> afspec, RTM_DELLINK);
>>
>> + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD
>> + && dev->netdev_ops->ndo_bridge_setlink) {
>> + int ret = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
>> + if (ret && ret != -EOPNOTSUPP) {
>> + /* XXX Fix this in the future to rollback
>> + * kernel settings and return error
>> + */
>> + br_warn(p->br, "error offloading bridge attributes "
>> + "on port %u(%s)\n", (unsigned int) p->port_no,
>> + p->dev->name);
>> + }
>> + }
>> +
>
> Same comments as setlink above.
>
>> return err;
>> }
>> static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>> --
>> 1.7.10.4
>>
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH iproute2] bridge link: add option 'self'
From: Scott Feldman @ 2014-12-05 6:52 UTC (permalink / raw)
To: Roopa Prabhu
Cc: Jiří Pírko, Jamal Hadi Salim, Benjamin LaHaise,
Thomas Graf, john fastabend, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <1417746436-41023-1-git-send-email-roopa@cumulusnetworks.com>
On Thu, Dec 4, 2014 at 6:27 PM, <roopa@cumulusnetworks.com> wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Currently self is set internally only if hwmode is set.
> This makes it necessary for the hw to have a mode.
> There is no hwmode really required to go to hardware. So, introduce
> self for anybody who wants to target hardware.
Can you include unwinding the hwmode 'swdev' setting for
kernel/iproute2 in v2? With this "self" option, and your new
NETIF_F_HW_SWITCH_OFFLOAD flag, we don't need the hwmde 'swdev'.
> ---
> bridge/link.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/bridge/link.c b/bridge/link.c
> index 90d9e7f..2b86141 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -321,6 +321,9 @@ static int brlink_modify(int argc, char **argv)
> "\"veb\".\n");
> exit(-1);
> }
> + } else if (strcmp(*argv, "self") == 0) {
> + NEXT_ARG();
> + flags = BRIDGE_FLAGS_SELF;
> } else {
> usage();
> }
> --
> 1.7.10.4
>
^ permalink raw reply
* Re: [PATCH 1/3] netdev: introduce new NETIF_F_HW_SWITCH_OFFLOAD feature flag for switch device offloads
From: Scott Feldman @ 2014-12-05 6:47 UTC (permalink / raw)
To: John Fastabend
Cc: Roopa Prabhu, Jiří Pírko, Jamal Hadi Salim,
Benjamin LaHaise, Thomas Graf, stephen@networkplumber.org,
John Linville, nhorman@tuxdriver.com, Nicolas Dichtel,
vyasevic@redhat.com, Florian Fainelli, buytenh@wantstofly.org,
Aviad Raveh, Netdev, David S. Miller, shm, Andy Gospodarek
In-Reply-To: <54815180.30900@gmail.com>
On Thu, Dec 4, 2014 at 10:32 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 12/04/2014 10:08 PM, Scott Feldman wrote:
>>
>> On Thu, Dec 4, 2014 at 6:26 PM, <roopa@cumulusnetworks.com> wrote:
>>>
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> This is a generic high level feature flag for all switch asic features
>>> today.
>>>
>>> switch drivers set this flag on switch ports. Logical devices like
>>> bridge, bonds, vxlans can inherit this flag from their slaves/ports.
>>>
>>> I had to use SWITCH in the name to avoid ambiguity with other feature
>>> flags. But, since i have been harping about not calling it 'switch',
>>> I am welcome to any suggestions :)
>>>
>>> An alternative to using a feature flag is to use a IFF_HW_OFFLOAD
>>> in net_device_flags.
>>> ---
>>> include/linux/netdev_features.h | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/linux/netdev_features.h
>>> b/include/linux/netdev_features.h
>>> index 8e30685..68db1de 100644
>>> --- a/include/linux/netdev_features.h
>>> +++ b/include/linux/netdev_features.h
>>> @@ -66,6 +66,7 @@ enum {
>>> NETIF_F_HW_VLAN_STAG_FILTER_BIT,/* Receive filtering on VLAN
>>> STAGs */
>>> NETIF_F_HW_L2FW_DOFFLOAD_BIT, /* Allow L2 Forwarding in
>>> Hardware */
>>> NETIF_F_BUSY_POLL_BIT, /* Busy poll */
>>> + NETIF_F_HW_SWITCH_OFFLOAD_BIT, /* HW switch offload */
>>>
>>> /*
>>> * Add your fresh new feature above and remember to update
>>> @@ -124,6 +125,7 @@ enum {
>>> #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX)
>>> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD)
>>> #define NETIF_F_BUSY_POLL __NETIF_F(BUSY_POLL)
>>> +#define NETIF_F_HW_SWITCH_OFFLOAD __NETIF_F(HW_SWITCH_OFFLOAD)
>>
>>
>> Can we use existing flag NETIF_F_HW_L2FW_DOFFLOAD? The comment above
>> on that flag suggests it would be perfect for what we're doing with L2
>> bridging: offloading the L2 forwarding path in HW.
>>
>> NETIF_F_HW_L2FW_DOFFLOAD seems to be orphaned in current net-next, so
>> I'm not sure of it's status, but see if it makes sense to recycle it.
>>
>> I could see a NETIF_F_HW_L3FW_DOFFLOAD for L3 fwd offload, down the road.
>>
>
> Its not orphaned its used by the macvlan device to indicate that the
> port switching should be done in hardware. For example put macvlan in
> VEPA mode and the hardware will deliver the packets directly to the
> netdev and get the pruning right. In VEB mode the hardware will forward
> packets correctly between macvlan netdevs. The implementation on ixgbe
> for example is to give a set of hardware queues per macvlan netdev. I
> still need to add offloaded QOS to make this more interesting.
>
> I could support a l3vlan device as well even on some of the host nics.
>
> The issue with generalizing it is at the moment it is used by ixgbe
> which can't forward between two physical ports. It works on macvlan
> because each macvlan netdev uses the same physical uplink. I think switching
> between different ports is going to require another feature
> bit like its done here.
My bad...I grepped for the _BIT variant and missed the usage in
macvlan, etc. Sorry about that.
>
> .John
>
>
>>
>>> /* Features valid for ethtool to change */
>>> /* = all defined minus driver/device-class-related */
>>> --
>>> 1.7.10.4
>>>
>
>
> --
> John Fastabend Intel Corporation
^ 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