* Re: [PATCH] net: add new QCA alx ethernet driver
From: Joe Perches @ 2012-08-09 15:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ren, Cloud, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, qca-linux-team, nic-devel,
Huang, Xiong, hao-ran.liu@canonical.com, Rodriguez, Luis
In-Reply-To: <20120809145454.GA21195@home.goodmis.org>
On Thu, 2012-08-09 at 10:54 -0400, Steven Rostedt wrote:
> On Thu, Aug 09, 2012 at 06:49:25AM +0000, Ren, Cloud wrote:
> > Luis has ever asked joe for adding as a Signed-off-by.
[]
> I'm curious, did Luis ask on list or off?
Off vger lists with some qca list or exploder added.
> The Signed-off tag has some legal bindings (all other tags are for
> credit/info only). It is that person saying that the changes they made
> to a patch are theirs and are giving the right to distribute it.
I believe I posted all patches I signed-off to vger lists.
> The sign-off is a simple line at the end of the explanation for the
> patch, which certifies that you wrote it or otherwise have the right to
> pass it on as an open-source patch.
>
> Joe, If you actually modified part of the code in the patch, you do have
> the right to add a signed-off-by tag. If you just made suggestions or
> reviewed, then a 'Suggested-by' or 'Reviewed-by' tag would be more
> appropriate.
Yes, I sent signed-off patches to their original RFC submission
(back last year?). To me, this single large patch is like a
snapshot of a git tree and not all contributors to that git
tree should be noted as signers of the entire thing.
^ permalink raw reply
* Re: [PATCH] net: add new QCA alx ethernet driver
From: Steven Rostedt @ 2012-08-09 15:28 UTC (permalink / raw)
To: Joe Perches
Cc: Ren, Cloud, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, qca-linux-team, nic-devel,
Huang, Xiong, hao-ran.liu@canonical.com, Rodriguez, Luis
In-Reply-To: <1344525758.3165.8.camel@joe2Laptop>
On Thu, 2012-08-09 at 08:22 -0700, Joe Perches wrote:
> Yes, I sent signed-off patches to their original RFC submission
> (back last year?). To me, this single large patch is like a
> snapshot of a git tree and not all contributors to that git
> tree should be noted as signers of the entire thing.
Ouch, that looks like we are losing all history.
One large patch should not be submitted. If it was developed in steps,
then it is best to show those steps when they are incorporated into the
Linux repository.
Sure, the patches may be reworked and folded together where appropriate,
but when I read "this single large patch is like a snapshot of a git
tree", that right there is a red flag.
-- Steve
^ permalink raw reply
* Re: [net-next RFC V5 3/5] virtio: intorduce an API to set affinity for a virtqueue
From: Avi Kivity @ 2012-08-09 15:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: krkumar2, habanero, kvm, mst, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <5023D3A1.8040102@redhat.com>
On 08/09/2012 06:13 PM, Paolo Bonzini wrote:
> Il 05/07/2012 12:29, Jason Wang ha scritto:
>> Sometimes, virtio device need to configure irq affiniry hint to maximize the
>> performance. Instead of just exposing the irq of a virtqueue, this patch
>> introduce an API to set the affinity for a virtqueue.
>>
>> The api is best-effort, the affinity hint may not be set as expected due to
>> platform support, irq sharing or irq type. Currently, only pci method were
>> implemented and we set the affinity according to:
>>
>> - if device uses INTX, we just ignore the request
>> - if device has per vq vector, we force the affinity hint
>> - if the virtqueues share MSI, make the affinity OR over all affinities
>> requested
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> It looks like both I and Jason will need these patches during the 3.7
> merge window, and from different trees (net-next vs. scsi). How do we
> synchronize?
Get one of them to promise not to rebase, merge it, and base your
patches on top of the merge.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock
From: Eric Dumazet @ 2012-08-09 15:36 UTC (permalink / raw)
To: Paul Moore
Cc: David Miller, Casey Schaufler, Eric Paris, John Stultz,
Serge E. Hallyn, lkml, James Morris, selinux, john.johansen, LSM,
netdev
In-Reply-To: <5799181.tjlnF0gIh2@sifl>
On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
> Is is possible to do the call to security_sk_alloc() in the ip_init() function
> or does the per-cpu nature of the socket make this a pain?
>
Its a pain, if we want NUMA affinity.
Here, each cpu should get memory from its closest node.
^ permalink raw reply
* Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock
From: Paul Moore @ 2012-08-09 15:59 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Casey Schaufler, Eric Paris, John Stultz,
Serge E. Hallyn, lkml, James Morris, selinux, john.johansen, LSM,
netdev
In-Reply-To: <1344526608.28967.1092.camel@edumazet-glaptop>
On Thu, Aug 9, 2012 at 11:36 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
>
>> Is is possible to do the call to security_sk_alloc() in the ip_init() function
>> or does the per-cpu nature of the socket make this a pain?
>>
>
> Its a pain, if we want NUMA affinity.
>
> Here, each cpu should get memory from its closest node.
Okay, makes sense.
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC net-next 2/4] gianfar: Clear ievent from interrupt handler for [RT]x int
From: Claudiu Manoil @ 2012-08-09 16:04 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: netdev, David S. Miller
In-Reply-To: <20120808161156.GB11043@windriver.com>
On 8/8/2012 7:11 PM, Paul Gortmaker wrote:
> [[RFC net-next 2/4] gianfar: Clear ievent from interrupt handler for [RT]x int] On 08/08/2012 (Wed 15:26) Claudiu Manoil wrote:
>
>> It's the interrupt handler's job to clear ievent for the Tx/Rx paths, as soon
>> as the corresponding interrupt sources have been masked.
>
> What wasn't clear to me was whether we'd ever have an instance of
> gfar_poll run without RTX_MASK being cleared (in less normal conditions,
> like netconsole, KGDBoE etc), since the gfar_schedule_cleanup is only
> called from rx/tx IRQ threads, and neither of those are used by
> gfar_poll, it seems.
Hi Paul,
As I see it, netconsole has the ndo_poll_controller hook, which points
to gfar_netpoll() -> gfar_interrupt() -> gfar_receive|transmit() ->
gfar_schedule_cleanup(), so it passes through schedule_cleanup.
My understanding is that gfar_poll() is scheduled for execution
only by "__napi_schedule(&gfargrp->napi);" from the Rx/Tx interrupt
handler (gfar_receive/transmit()->gfar_schedule_cleanup()), and that
it will be executed sometimes after the interrupt handler returns,
which means RTX_MASK has been cleared (and the interrupt sources are
already masked).
I think that we might have an issue if we don't clear IEVENT right away
in the interrupt handler, as this register might cause additional hw
interrupts to the PIC upon returning from the interrupt handler.
^ permalink raw reply
* Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock
From: Eric Paris @ 2012-08-09 16:05 UTC (permalink / raw)
To: Eric Dumazet
Cc: Paul Moore, David Miller, Casey Schaufler, John Stultz,
Serge E. Hallyn, lkml, James Morris, selinux, john.johansen, LSM,
netdev
In-Reply-To: <1344526608.28967.1092.camel@edumazet-glaptop>
On Thu, Aug 9, 2012 at 11:36 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
>
>> Is is possible to do the call to security_sk_alloc() in the ip_init() function
>> or does the per-cpu nature of the socket make this a pain?
>>
>
> Its a pain, if we want NUMA affinity.
>
> Here, each cpu should get memory from its closest node.
I really really don't like it. I won't say NAK, but it is the first
and only place in the kernel where I believe we allocate an object and
don't allocate the security blob until some random later point in
time. If it is such a performance issue to have the security blob in
the same numa node, isn't adding a number of branches and putting this
function call on every output at least as bad? Aren't we discouraged
from GFP_ATOMIC? In __init we can use GFP_KERNEL.
This still doesn't fix these sockets entirely. We now have the
security blob allocated, but it was never set to something useful.
Paul, are you looking into this? This is a bandaide, not a fix....
-Eric
^ permalink raw reply
* Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock
From: Paul Moore @ 2012-08-09 16:09 UTC (permalink / raw)
To: Eric Paris
Cc: Eric Dumazet, David Miller, Casey Schaufler, John Stultz,
Serge E. Hallyn, lkml, James Morris, selinux, john.johansen, LSM,
netdev
In-Reply-To: <CACLa4ptTfMzvhYk7_DaUJd-9u406FXf2CUHjn1mQrPDa4fFW4w@mail.gmail.com>
On Thu, Aug 9, 2012 at 12:05 PM, Eric Paris <eparis@parisplace.org> wrote:
> Paul, are you looking into this? This is a bandaide, not a fix....
Yep, I mentioned this a few times in the other thread. The problem is
there is not going to be an easy fix for the labeling so I'd rather we
see this patch, or something like it, go in now to resolve the kernel
panic, and fix the labeling later.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC net-next 3/4] gianfar: Separate out the Rx and Tx coalescing functions
From: Claudiu Manoil @ 2012-08-09 16:24 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: netdev, David S. Miller
In-Reply-To: <20120808154454.GA11043@windriver.com>
On 8/8/2012 6:44 PM, Paul Gortmaker wrote:
> [[RFC net-next 3/4] gianfar: Separate out the Rx and Tx coalescing functions] On 08/08/2012 (Wed 15:26) Claudiu Manoil wrote:
>
>> Split the coalescing programming support by Rx and Tx h/w queues, in order to
>> introduce a separate NAPI for the Tx confirmation path (next patch). This way,
>> the Rx processing path will handle the coalescing settings for the Rx queues
>> only, resp. the Tx confirmation processing path will handle the Tx queues.
>>
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
>> ---
>> drivers/net/ethernet/freescale/gianfar.c | 36 +++++++++++++++++++++++------
>> 1 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
>> index ddd350a..919acb3 100644
>> --- a/drivers/net/ethernet/freescale/gianfar.c
>> +++ b/drivers/net/ethernet/freescale/gianfar.c
>> @@ -1794,8 +1794,8 @@ void gfar_start(struct net_device *dev)
>> dev->trans_start = jiffies; /* prevent tx timeout */
>> }
>>
>> -void gfar_configure_coalescing(struct gfar_private *priv,
>> - unsigned long tx_mask, unsigned long rx_mask)
>> +static inline void gfar_configure_tx_coalescing(struct gfar_private *priv,
>
> I believe the preference is to not specify inline when all the chunks in
> play are present in the one C file -- i.e. let gcc figure it out. Same
> for the Rx instance below.
>
> P.
> --
I agree with you.
thanks
^ permalink raw reply
* [PATCH net-next] af_packet: relax BUG statement in tpacket_destruct_skb
From: Daniel Borkmann @ 2012-08-09 16:39 UTC (permalink / raw)
To: davem; +Cc: netdev
Here's a quote of the comment about the BUG macro from asm-generic/bug.h:
Don't use BUG() or BUG_ON() unless there's really no way out; one
example might be detecting data structure corruption in the middle
of an operation that can't be backed out of. If the (sub)system
can somehow continue operating, perhaps with reduced functionality,
it's probably not BUG-worthy.
If you're tempted to BUG(), think again: is completely giving up
really the *only* solution? There are usually better options, where
users don't need to reboot ASAP and can mostly shut down cleanly.
In our case, the status flag of a ring buffer slot is managed from both sides,
the kernel space and the user space. This means that even though the kernel
side might work as expected, the user space screws up and changes this flag
right between the send(2) is triggered when the flag is changed to
TP_STATUS_SENDING and a given skb is destructed after some time. Then, this
will hit the BUG macro. Instead, we relax this condition with a WARN_ON_ONCE
macro, so that the user is aware of this situation. I've tested it and the
system still behaves /stable/, so in accordance with the above comment, we
should rather relax this behavior with a warning.
Signed-off-by: Daniel Borkmann <daniel.borkmann@tik.ee.ethz.ch>
---
net/packet/af_packet.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ceaca7c..4def36f 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1936,7 +1936,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
if (likely(po->tx_ring.pg_vec)) {
ph = skb_shinfo(skb)->destructor_arg;
- BUG_ON(__packet_get_status(po, ph) != TP_STATUS_SENDING);
+ WARN_ON_ONCE(__packet_get_status(po, ph) != TP_STATUS_SENDING);
BUG_ON(atomic_read(&po->tx_ring.pending) == 0);
atomic_dec(&po->tx_ring.pending);
__packet_set_status(po, ph, TP_STATUS_AVAILABLE);
^ permalink raw reply related
* Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock
From: Eric Dumazet @ 2012-08-09 17:46 UTC (permalink / raw)
To: Eric Paris
Cc: Paul Moore, David Miller, Casey Schaufler, John Stultz,
Serge E. Hallyn, lkml, James Morris, selinux, john.johansen, LSM,
netdev
In-Reply-To: <CACLa4ptTfMzvhYk7_DaUJd-9u406FXf2CUHjn1mQrPDa4fFW4w@mail.gmail.com>
On Thu, 2012-08-09 at 12:05 -0400, Eric Paris wrote:
> On Thu, Aug 9, 2012 at 11:36 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
> >
> >> Is is possible to do the call to security_sk_alloc() in the ip_init() function
> >> or does the per-cpu nature of the socket make this a pain?
> >>
> >
> > Its a pain, if we want NUMA affinity.
> >
> > Here, each cpu should get memory from its closest node.
>
> I really really don't like it. I won't say NAK, but it is the first
> and only place in the kernel where I believe we allocate an object and
> don't allocate the security blob until some random later point in
> time.
...
> If it is such a performance issue to have the security blob in
> the same numa node, isn't adding a number of branches and putting this
> function call on every output at least as bad? Aren't we discouraged
> from GFP_ATOMIC? In __init we can use GFP_KERNEL.
What a big deal. Its done _once_ time per cpu, and this is so small blob
of memory you'll have to show us one single failure out of one million
boots.
If the security_sk_alloc() fails, we dont care. We are about sending a
RESET or ACK packet. They can be lost by the network, or even skb
allocation can fail. Nobody ever noticed and complained.
Every time we accept() a new socket (and call security_sk_alloc()), its
done under soft irq, thus GFP_ATOMIC, and you didn't complain yet, while
a socket needs about 2 Kbytes of memory...
>
> This still doesn't fix these sockets entirely. We now have the
> security blob allocated, but it was never set to something useful.
> Paul, are you looking into this? This is a bandaide, not a fix....
>
Please do so, on a followup patch, dont pretend I must fix all this
stuff.
^ permalink raw reply
* [PATCH net-next,1/1] hyperv: Add comments for the extended buffer after RNDIS message
From: Haiyang Zhang @ 2012-08-09 18:04 UTC (permalink / raw)
To: davem, netdev; +Cc: haiyangz, kys, olaf, linux-kernel, devel
Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/rndis_filter.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 1e88a10..06f8601 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -46,8 +46,14 @@ struct rndis_request {
/* Simplify allocation by having a netvsc packet inline */
struct hv_netvsc_packet pkt;
struct hv_page_buffer buf;
- /* FIXME: We assumed a fixed size request here. */
+
struct rndis_message request_msg;
+ /*
+ * The buffer for the extended info after the RNDIS message. It's
+ * referenced based on the data offset in the RNDIS message. Its size
+ * is enough for current needs, and should be sufficient for the near
+ * future.
+ */
u8 ext[100];
};
--
1.7.4.1
^ permalink raw reply related
* [net-next] bonding: don't allow the master to become its slave
From: Flavio Leitner @ 2012-08-09 18:30 UTC (permalink / raw)
To: netdev; +Cc: Jay Vosburgh, Andy Gospodarek, Leonardo Chiquitto, Flavio Leitner
It doesn't make any sense to allow the master to become
its slave. That creates a loop of events causing a crash.
Reported-by: Leonardo Chiquitto <lchiquitto@suse.com>
Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
drivers/net/bonding/bond_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..5407b44 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
int link_reporting;
int res = 0;
+ if (bond_dev == slave_dev) {
+ pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name);
+ return -EINVAL;
+ }
+
if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
slave_ops->ndo_do_ioctl == NULL) {
pr_warning("%s: Warning: no link monitoring support for %s\n",
--
1.7.11.2
^ permalink raw reply related
* Re: [PATCH] net: add new QCA alx ethernet driver
From: Ben Hutchings @ 2012-08-09 18:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: Joe Perches, Ren, Cloud, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, qca-linux-team, nic-devel,
Huang, Xiong, hao-ran.liu@canonical.com, Rodriguez, Luis
In-Reply-To: <1344526098.6935.44.camel@gandalf.stny.rr.com>
On Thu, 2012-08-09 at 11:28 -0400, Steven Rostedt wrote:
> On Thu, 2012-08-09 at 08:22 -0700, Joe Perches wrote:
>
> > Yes, I sent signed-off patches to their original RFC submission
> > (back last year?). To me, this single large patch is like a
> > snapshot of a git tree and not all contributors to that git
> > tree should be noted as signers of the entire thing.
>
> Ouch, that looks like we are losing all history.
>
> One large patch should not be submitted. If it was developed in steps,
> then it is best to show those steps when they are incorporated into the
> Linux repository.
>
> Sure, the patches may be reworked and folded together where appropriate,
> but when I read "this single large patch is like a snapshot of a git
> tree", that right there is a red flag.
I don't think this has ever been a requirement for new drivers.
The alterations to the description of atl1c ought to be broken out as a
separate patch, though.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [net-next] bonding: don't allow the master to become its slave
From: Ben Hutchings @ 2012-08-09 19:03 UTC (permalink / raw)
To: Flavio Leitner
Cc: netdev, Jay Vosburgh, Andy Gospodarek, Leonardo Chiquitto,
Jiri Pirko
In-Reply-To: <1344537049-11473-1-git-send-email-fbl@redhat.com>
On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
> It doesn't make any sense to allow the master to become
> its slave. That creates a loop of events causing a crash.
What if there are other intermediate devices, e.g. the slave is a VLAN
sub-device of the bond? And doesn't team also have this problem?
I think a more general check for such loops might be required.
Ben.
> Reported-by: Leonardo Chiquitto <lchiquitto@suse.com>
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6fae5f3..5407b44 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> int link_reporting;
> int res = 0;
>
> + if (bond_dev == slave_dev) {
> + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name);
> + return -EINVAL;
> + }
> +
> if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
> slave_ops->ndo_do_ioctl == NULL) {
> pr_warning("%s: Warning: no link monitoring support for %s\n",
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [net-next] bonding: don't allow the master to become its slave
From: Leonardo Chiquitto @ 2012-08-09 18:45 UTC (permalink / raw)
To: Flavio Leitner, netdev; +Cc: Andy Gospodarek, Jay Vosburgh
In-Reply-To: <1344537049-11473-1-git-send-email-fbl@redhat.com>
>>> On 8/9/2012 at 03:30 PM, Flavio Leitner <fbl@redhat.com> wrote:
> It doesn't make any sense to allow the master to become
> its slave. That creates a loop of events causing a crash.
>
> Reported-by: Leonardo Chiquitto <lchiquitto@suse.com>
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6fae5f3..5407b44 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct
> net_device *slave_dev)
> int link_reporting;
> int res = 0;
>
> + if (bond_dev == slave_dev) {
> + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name);
> + return -EINVAL;
> + }
> +
> if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
> slave_ops->ndo_do_ioctl == NULL) {
> pr_warning("%s: Warning: no link monitoring support for %s\n",
I've tested it here and confirm it prevents the crash.
Tested-by: Leonardo Chiquitto <lchiquitto@suse.com>
Thanks,
Leonardo
^ permalink raw reply
* Re: [PATCH] net: add new QCA alx ethernet driver
From: Luis R. Rodriguez @ 2012-08-09 19:13 UTC (permalink / raw)
To: Steven Rostedt
Cc: Joe Perches, Ren, Cloud, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, qca-linux-team, nic-devel,
Huang, Xiong, hao-ran.liu@canonical.com
In-Reply-To: <1344526098.6935.44.camel@gandalf.stny.rr.com>
On Thu, Aug 9, 2012 at 8:28 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2012-08-09 at 08:22 -0700, Joe Perches wrote:
>
>> Yes, I sent signed-off patches to their original RFC submission
>> (back last year?).
And that is why your SOB was kept.
>> To me, this single large patch is like a
>> snapshot of a git tree and not all contributors to that git
>> tree should be noted as signers of the entire thing.
I am not sure we really ever addressed this technical difference and
from what I have seen we always run into this question on new driver
submissions.
Entity x goes on and posts driver joojoo-1, and we some folks in the
community may merge such patch onto their tree and edit the driver and
send some of their own changes and improvements to the developers /
mailing list. Typically if the patches sent had a SOB tag the right
thing to due given the legal implications of the Developer's
Certificate of Origin is to add those SOBs to the new driver
submission if company x goes on and posts joojoo-2.. but...
> Ouch, that looks like we are losing all history.
Agreed! It'd be best instead to have such changes as part of a linear
history somewhere so maybe what would be right prior to inclusion
upstream is for such patches to be merged on an external github tree
or wherever to be able to keep record of the submissions done during
development. Then upon submission to the Linux kernel again as
joojoo-2 instead of joojoo-1 company x would simply provide their own
SOB, and the record of the previous SOBs would be kept on the forked
git tree.
Whether or not it is required to have the SOB tog of previous patch
submitters is not clear but to me its not necessary if you have a
record somewhere that they were submitted and merged in some other
tree. The mailing list alone may serve as a good place holder for this
information as well so a separate tree may not be required.
So -- are we OK to *not* include SOBs of evolutions sent to developers
of joojoo-1 when company x submits joojoo-2 so long as those patches
were sent publicly or a record is kept somewhere ?
> One large patch should not be submitted. If it was developed in steps,
> then it is best to show those steps when they are incorporated into the
> Linux repository.
Sure, but at times some changes are asked after a submission of
joojoo-1, and at times this may be ongoing until maybe even joojoo-10,
before we are happy to merge it upstream.
Luis
^ permalink raw reply
* [iproute2][PATCH 1/2] tc: add ipset ematch
From: Florian Westphal @ 2012-08-09 19:18 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
example usage:
tc filter add dev $dev parent $id: basic match not ipset'(foobar src)' ..
also updates iproute2/ematch_map, else tc complains:
Error: Unable to find ematch "ipset" in /etc/iproute2/ematch_map
Please assign a unique ID to the ematch kind the suggested entry is:
8 ipset
when trying to use this ematch.
(text ematch (5) only exists in kernel, a vlan ematch (6) exists neither in
kernel nor userspace, but kernel headers define TCF_EM_VLAN == 6).
---
configure | 34 ++++++
etc/iproute2/ematch_map | 1 +
tc/Makefile | 4 +
tc/em_ipset.c | 265 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 304 insertions(+), 0 deletions(-)
create mode 100644 tc/em_ipset.c
diff --git a/configure b/configure
index 0f4444f..a1916de 100755
--- a/configure
+++ b/configure
@@ -183,6 +183,37 @@ fi
rm -f $TMPDIR/setnstest.c $TMPDIR/setnstest
}
+check_ipset()
+{
+cat >$TMPDIR/ipsettest.c <<EOF
+#include <linux/netfilter/ipset/ip_set.h>
+#ifndef IP_SET_INVALID
+#define IPSET_DIM_MAX 3
+typedef unsigned short ip_set_id_t;
+#endif
+#include <linux/netfilter/xt_set.h>
+
+struct xt_set_info info;
+#if IPSET_PROTOCOL == 6
+int main(void)
+{
+ return IPSET_MAXNAMELEN;
+}
+#else
+#error unknown ipset version
+#endif
+EOF
+
+if gcc -I$INCLUDE -o $TMPDIR/ipsettest $TMPDIR/ipsettest.c >/dev/null 2>&1
+then
+ echo "TC_CONFIG_IPSET:=y" >>Config
+ echo "yes"
+else
+ echo "no"
+fi
+rm -f $TMPDIR/ipsettest.c $TMPDIR/ipsettest
+}
+
echo "# Generated config based on" $INCLUDE >Config
echo "TC schedulers"
@@ -196,6 +227,9 @@ check_xt_old
check_xt_old_internal_h
check_ipt
+echo -n " IPSET "
+check_ipset
+
echo -n "iptables modules directory: "
check_ipt_lib_dir
diff --git a/etc/iproute2/ematch_map b/etc/iproute2/ematch_map
index 7c6a281..69b007d 100644
--- a/etc/iproute2/ematch_map
+++ b/etc/iproute2/ematch_map
@@ -3,3 +3,4 @@
2 nbyte
3 u32
4 meta
+8 ipset
diff --git a/tc/Makefile b/tc/Makefile
index 64d93ad..dfbfac5 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -50,6 +50,10 @@ TCMODULES += q_mqprio.o
TCMODULES += q_codel.o
TCMODULES += q_fq_codel.o
+ifeq ($(TC_CONFIG_IPSET), y)
+ TCMODULES += em_ipset.o
+endif
+
TCSO :=
ifeq ($(TC_CONFIG_ATM),y)
TCSO += q_atm.so
diff --git a/tc/em_ipset.c b/tc/em_ipset.c
new file mode 100644
index 0000000..a2d0d15
--- /dev/null
+++ b/tc/em_ipset.c
@@ -0,0 +1,265 @@
+/*
+ * em_ipset.c IPset Ematch
+ *
+ * (C) 2012 Florian Westphal <fw@strlen.de>
+ *
+ * Parts taken from iptables libxt_set.h:
+ * Copyright (C) 2000-2002 Joakim Axelsson <gozem@linux.nu>
+ * Patrick Schaaf <bof@bof.de>
+ * Martin Josefsson <gandalf@wlug.westbo.se>
+ * Copyright (C) 2003-2010 Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <errno.h>
+#include <netdb.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#include <getopt.h>
+
+#include <xtables.h>
+#include <linux/netfilter/ipset/ip_set.h>
+
+#ifndef IPSET_INVALID_ID
+typedef __u16 ip_set_id_t;
+
+enum ip_set_dim {
+ IPSET_DIM_ZERO = 0,
+ IPSET_DIM_ONE,
+ IPSET_DIM_TWO,
+ IPSET_DIM_THREE,
+ IPSET_DIM_MAX = 6,
+};
+#endif /* IPSET_INVALID_ID */
+
+#include <linux/netfilter/xt_set.h>
+#include "m_ematch.h"
+
+#ifndef IPSET_INVALID_ID
+#define IPSET_INVALID_ID 65535
+#define SO_IP_SET 83
+
+union ip_set_name_index {
+ char name[IPSET_MAXNAMELEN];
+ __u16 index;
+};
+
+#define IP_SET_OP_GET_BYNAME 0x00000006 /* Get set index by name */
+struct ip_set_req_get_set {
+ unsigned op;
+ unsigned version;
+ union ip_set_name_index set;
+};
+
+#define IP_SET_OP_GET_BYINDEX 0x00000007 /* Get set name by index */
+/* Uses ip_set_req_get_set */
+
+#define IP_SET_OP_VERSION 0x00000100 /* Ask kernel version */
+struct ip_set_req_version {
+ unsigned op;
+ unsigned version;
+};
+#endif /* IPSET_INVALID_ID */
+
+extern struct ematch_util ipset_ematch_util;
+
+static int get_version(unsigned *version)
+{
+ int res, sockfd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW);
+ struct ip_set_req_version req_version;
+ socklen_t size = sizeof(req_version);
+
+ if (sockfd < 0) {
+ fputs("Can't open socket to ipset.\n", stderr);
+ return -1;
+ }
+
+ req_version.op = IP_SET_OP_VERSION;
+ res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req_version, &size);
+ if (res != 0) {
+ perror("xt_set getsockopt");
+ return -1;
+ }
+
+ *version = req_version.version;
+ return sockfd;
+}
+
+static int do_getsockopt(struct ip_set_req_get_set *req)
+{
+ int sockfd, res;
+ socklen_t size = sizeof(struct ip_set_req_get_set);
+ sockfd = get_version(&req->version);
+ if (sockfd < 0)
+ return -1;
+ res = getsockopt(sockfd, SOL_IP, SO_IP_SET, req, &size);
+ if (res != 0)
+ perror("Problem when communicating with ipset");
+ close(sockfd);
+ if (res != 0)
+ return -1;
+
+ if (size != sizeof(struct ip_set_req_get_set)) {
+ fprintf(stderr,
+ "Incorrect return size from kernel during ipset lookup, "
+ "(want %zu, got %zu)\n",
+ sizeof(struct ip_set_req_get_set), (size_t)size);
+ return -1;
+ }
+
+ return res;
+}
+
+static int
+get_set_byid(char *setname, unsigned int idx)
+{
+ struct ip_set_req_get_set req;
+ int res;
+
+ req.op = IP_SET_OP_GET_BYINDEX;
+ req.set.index = idx;
+ res = do_getsockopt(&req);
+ if (res != 0)
+ return -1;
+ if (req.set.name[0] == '\0') {
+ fprintf(stderr,
+ "Set with index %i in kernel doesn't exist.\n", idx);
+ return -1;
+ }
+
+ strncpy(setname, req.set.name, IPSET_MAXNAMELEN);
+ return 0;
+}
+
+static int
+get_set_byname(const char *setname, struct xt_set_info *info)
+{
+ struct ip_set_req_get_set req;
+ int res;
+
+ req.op = IP_SET_OP_GET_BYNAME;
+ strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
+ req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
+ res = do_getsockopt(&req);
+ if (res != 0)
+ return -1;
+ if (req.set.index == IPSET_INVALID_ID)
+ return -1;
+ info->index = req.set.index;
+ return 0;
+}
+
+static int
+parse_dirs(const char *opt_arg, struct xt_set_info *info)
+{
+ char *saved = strdup(opt_arg);
+ char *ptr, *tmp = saved;
+
+ if (!tmp) {
+ perror("strdup");
+ return -1;
+ }
+
+ while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
+ info->dim++;
+ ptr = strsep(&tmp, ",");
+ if (strncmp(ptr, "src", 3) == 0)
+ info->flags |= (1 << info->dim);
+ else if (strncmp(ptr, "dst", 3) != 0) {
+ fputs("You must specify (the comma separated list of) 'src' or 'dst'\n", stderr);
+ free(saved);
+ return -1;
+ }
+ }
+
+ if (tmp)
+ fprintf(stderr, "Can't be more src/dst options than %u", IPSET_DIM_MAX);
+ free(saved);
+ return tmp ? -1 : 0;
+}
+
+static void ipset_print_usage(FILE *fd)
+{
+ fprintf(fd,
+ "Usage: ipset(SETNAME FLAGS)\n" \
+ "where: SETNAME:= string\n" \
+ " FLAGS := { FLAG[,FLAGS] }\n" \
+ " FLAG := { src | dst }\n" \
+ "\n" \
+ "Example: 'ipset(bulk src,dst)'\n");
+}
+
+static int ipset_parse_eopt(struct nlmsghdr *n, struct tcf_ematch_hdr *hdr,
+ struct bstr *args)
+{
+ struct xt_set_info set_info;
+ int ret;
+
+ memset(&set_info, 0, sizeof(set_info));
+
+#define PARSE_ERR(CARG, FMT, ARGS...) \
+ em_parse_error(EINVAL, args, CARG, &ipset_ematch_util, FMT ,##ARGS)
+
+ if (args == NULL)
+ return PARSE_ERR(args, "ipset: missing set name");
+
+ if (args->len >= IPSET_MAXNAMELEN)
+ return PARSE_ERR(args, "ipset: set name too long (max %u)", IPSET_MAXNAMELEN - 1);
+ ret = get_set_byname(args->data, &set_info);
+ if (ret < 0)
+ return PARSE_ERR(args, "ipset: unknown set name '%s'", args->data);
+
+ if (args->next == NULL)
+ return PARSE_ERR(args, "ipset: missing set flags");
+
+ args = bstr_next(args);
+ if (parse_dirs(args->data, &set_info))
+ return PARSE_ERR(args, "ipset: error parsing set flags");
+
+ if (args->next) {
+ args = bstr_next(args);
+ return PARSE_ERR(args, "ipset: unknown parameter");
+ }
+
+ addraw_l(n, MAX_MSG, hdr, sizeof(*hdr));
+ addraw_l(n, MAX_MSG, &set_info, sizeof(set_info));
+
+#undef PARSE_ERR
+ return 0;
+}
+
+static int ipset_print_eopt(FILE *fd, struct tcf_ematch_hdr *hdr, void *data,
+ int data_len)
+{
+ int i;
+ char setname[IPSET_MAXNAMELEN];
+ const struct xt_set_info *set_info = data;
+
+ if (data_len != sizeof(*set_info)) {
+ fprintf(stderr, "xt_set_info struct size mismatch\n");
+ return -1;
+ }
+
+ if (get_set_byid(setname, set_info->index))
+ return -1;
+ fputs(setname, fd);
+ for (i = 1; i <= set_info->dim; i++) {
+ fprintf(fd, "%s%s", i == 1 ? " " : ",", set_info->flags & (1 << i) ? "src" : "dst");
+ }
+
+ return 0;
+}
+
+struct ematch_util ipset_ematch_util = {
+ .kind = "ipset",
+ .kind_num = TCF_EM_IPSET,
+ .parse_eopt = ipset_parse_eopt,
+ .print_eopt = ipset_print_eopt,
+ .print_usage = ipset_print_usage
+};
--
1.7.3.4
^ permalink raw reply related
* [iproute2][PATCH 2/2] add ematch man page
From: Florian Westphal @ 2012-08-09 19:18 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <1344539931-2653-1-git-send-email-fw@strlen.de>
---
could need more work, but a terse one is better than none.
man/man8/tc-ematch.8 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
man/man8/tc.8 | 1 +
2 files changed, 153 insertions(+), 0 deletions(-)
create mode 100644 man/man8/tc-ematch.8
diff --git a/man/man8/tc-ematch.8 b/man/man8/tc-ematch.8
new file mode 100644
index 0000000..53ae161
--- /dev/null
+++ b/man/man8/tc-ematch.8
@@ -0,0 +1,152 @@
+.TH filter ematch "6 August 2012" iproute2 Linux
+.
+.SH NAME
+ematch \- extended matches for use with "basic" or "flow" filters
+.
+.SH SYNOPSIS
+.sp
+.ad l
+.in +8
+.ti -8
+.B "tc filter add .. basic match"
+.RI EXPR
+.B .. flowid ..
+.sp
+
+.ti -8
+.IR EXPR " := " TERM " [ { "
+.B and | or
+}
+.IR EXPR
+]
+
+.ti -8
+.IR TERM " := [ " not " ] { " MATCH " | '(' " EXPR " ')' } "
+
+.ti -8
+.IR MATCH " := " module " '(' " ARGS " ')' "
+
+.ti -8
+.IR ARGS " := " ARG1 " " ARG2 " ..
+
+.SH MATCHES
+
+.SS cmp
+Simple comparison ematch: arithmetic compare of packet data to a given value.
+.ti
+.IR cmp "( " ALIGN " at " OFFSET " [ " ATTRS " ] { " eq " | " lt " | " gt " } " VALUE " )
+
+.ti
+.IR ALIGN " := { " u8 " | " u16 " | " u32 " } "
+
+.ti
+.IR ATTRS " := [ layer " LAYER " ] [ mask " MASK " ] [ " trans " ] "
+
+.ti
+.IR ALIGN " := { " u8 " | " u16 " | " u32 } "
+
+.ti
+.IR LAYER " := { " link " | " network " | " transport " | " 0..%d " }
+
+.SS meta
+Metadata ematch
+.ti
+.IR meta "( " OBJECT " { " eq " | " lt " |" gt " } " OBJECT " )
+
+.ti
+.IR OBJECT " := { " META_ID " | " VALUE " }
+
+.ti
+.IR META_ID " := id " [ shift " SHIFT " ] [ mask " MASK " ]
+
+.TP
+meta attributes:
+
+\fBrandom\fP 32 bit random value
+
+\fBloadavg_1\fP Load average in last 5 minutes
+
+\fBnf_mark\fP Netfilter mark
+
+\fBvlan\fP Vlan tag
+
+\fBsk_rcvbuf\fP Receive buffer size
+
+\fBsk_snd_queue\fP Send queue length
+
+.PP
+A full list of meta attributes can be obtained via
+
+# tc filter add dev eth1 basic match 'meta(list)'
+
+.SS nbyte
+match packet data byte sequence
+.ti
+.IR nbyte "( " NEEDLE " at " OFFSET " [ layer " LAYER " ] )
+
+.ti
+.IR NEEDLE " := { " string " | " c-escape-sequence " } "
+
+.ti
+.IR OFFSET " := " int
+
+.ti
+.IR LAYER " := { " link " | " network " | " transport " | " 0..%d " }
+
+.SS u32
+u32 ematch
+.ti
+.IR u32 "( " ALIGN VALUE MASK " at " [ nexthdr+ ] " OFFSET " )
+
+.ti
+.IR ALIGN " := " { " u8 " | " u16 " | " u32 " }
+
+.SS ipset
+test packet agains ipset membership
+.ti
+.IR ipset "( " SETNAME FLAGS )
+
+.ti
+.IR SETNAME " := " string
+
+.ti
+.IR FLAGS " := " { " FLAG " [, " FLAGS "] }
+
+The flag options are the same as those used by the iptables "set" match.
+
+When using the ipset ematch with the "ip_set_hash:net,iface" set type,
+the interface can be queried using "src,dst (source ip address, outgoing interface) or
+"src,src" (source ip address, incoming interface) syntax.
+
+.SH CAVEATS
+
+The ematch syntax uses '(' and ')' to group expressions. All braces need to be
+escaped properly to prevent shell commandline from interpreting these directly.
+
+When using the ipset ematch with the "ifb" device, the outgoing device will be the
+ifb device itself, e.g. "ifb0".
+The original interface (i.e. the device the packet arrived on) is treated as the incoming interface.
+
+.SH EXAMPLE & USAGE
+
+# tc filter add .. basic match ...
+
+# 'cmp(u16 at 3 layer 2 mask 0xff00 gt 20)'
+
+# 'meta(nfmark gt 24)' and 'meta(tcindex mask 0xf0 eq 0xf0)'
+
+# 'nbyte("ababa" at 12 layer 1)'
+
+# 'u32(u16 0x1122 0xffff at nexthdr+4)'
+
+Check if packet source ip address is member of set named \fBbulk\fP:
+
+# 'ipset(bulk src)'
+
+Check if packet source ip and the interface the packet arrived on is member of "hash:net,iface" set named \fBinteractive\fP:
+
+# 'ipset(interactive src,src)'
+
+.SH "AUTHOR"
+
+The extended match infrastructure was added by Thomas Graf.
diff --git a/man/man8/tc.8 b/man/man8/tc.8
index 95571a3..a285c49 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -374,6 +374,7 @@ was written by Alexey N. Kuznetsov and added in Linux 2.2.
.BR tc-choke (8),
.BR tc-codel (8),
.BR tc-drr (8),
+.BR tc-ematch (8),
.BR tc-fq_codel (8),
.BR tc-hfsc (7),
.BR tc-hfsc (8),
--
1.7.3.4
^ permalink raw reply related
* Re: [net-next] bonding: don't allow the master to become its slave
From: Jay Vosburgh @ 2012-08-09 19:23 UTC (permalink / raw)
To: Ben Hutchings
Cc: Flavio Leitner, netdev, Andy Gospodarek, Leonardo Chiquitto,
Jiri Pirko
In-Reply-To: <1344539003.2593.7.camel@bwh-desktop.uk.solarflarecom.com>
Ben Hutchings <bhutchings@solarflare.com> wrote:
>On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
>> It doesn't make any sense to allow the master to become
>> its slave. That creates a loop of events causing a crash.
>
>What if there are other intermediate devices, e.g. the slave is a VLAN
>sub-device of the bond? And doesn't team also have this problem?
>
>I think a more general check for such loops might be required.
I thought we had disallowed any nesting of bonds at all, but I
checked the netdev archives, and it appears we discussed it (and agreed
it didn't work), but it kind of petered out.
http://patchwork.ozlabs.org/patch/79705/
In any event, I think a patch like the following would get all
cases (double enslavement or enslavement of any bonding master) in one
place:
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..d14651c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1505,18 +1505,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
int link_reporting;
int res = 0;
+ if (slave_dev->priv_flags & IFF_BONDING) {
+ pr_debug("Error, Device was already enslaved\n");
+ return -EBUSY;
+ }
+
if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
slave_ops->ndo_do_ioctl == NULL) {
pr_warning("%s: Warning: no link monitoring support for %s\n",
bond_dev->name, slave_dev->name);
}
- /* already enslaved */
- if (slave_dev->flags & IFF_SLAVE) {
- pr_debug("Error, Device was already enslaved\n");
- return -EBUSY;
- }
-
/* vlan challenged mutual exclusion */
/* no need to lock since we're protected by rtnl_lock */
if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {
This is basically the same logic that Jiri Bohac originally
proposed in the discussion I mention above, although this patch moves
the test further up and combines the master and slave tests into one.
Comments? I haven't tested this at all, but I think the logic
is correct. I don't think having two separate tests to get special
"master" and "slave" error cases is worthwhile.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply related
* Re: [PATCH] net: add new QCA alx ethernet driver
From: Luis R. Rodriguez @ 2012-08-09 19:31 UTC (permalink / raw)
To: Steven Rostedt
Cc: Joe Perches, Ren, Cloud, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, qca-linux-team, nic-devel,
Huang, Xiong, hao-ran.liu@canonical.com
In-Reply-To: <CAB=NE6X4JQmQgtgcy9is0hn0nNypTKJN-d2RHFR7vvg=vO_eHQ@mail.gmail.com>
On Thu, Aug 9, 2012 at 12:13 PM, Luis R. Rodriguez
<rodrigue@qca.qualcomm.com> wrote:
> So -- are we OK to *not* include SOBs of evolutions sent to developers
> of joojoo-1 when company x submits joojoo-2 so long as those patches
> were sent publicly or a record is kept somewhere ?
And lets be fair, some folks *may* want their SOB passed along to the
second joojoo-2, its hard to please everyone but so long as we can
decide on a method -- then great, we don't have to come back to this
again and then we can add this as documentation. I'm inclined to
prefer to not have the small patch submitters SOB tag if their patch
to joojoo-1 was public and in it they did have a SOB tag, given that
in practice *a few* secondary patch submitters tend to get surprised
when their SOB is added to a secondary patch submission if they only
contributed a few lines. To be clear the SOB does not have anything to
do with how many lines you contributed, its meaning is here:
http://gerrit.googlecode.com/svn-history/r1526/documentation/2.1.2/user-signedoffby.html
So even if those secondary patch submitters *think* it has to do with
volume of code, perhaps its best in practice to not include them so
long as we have a record of the original small patch submission
publicly and with a SOB tag.
Luis
^ permalink raw reply
* Re: [PATCH 07/14] bridge: add some comments for NETDEV_RELEASE
From: Stephen Hemminger @ 2012-08-09 19:44 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David Miller, Stephen Hemminger
If you are going to add an explanation, then I would prefer a more complete one. Something like:
"Since more than one interface can be attached to a bridge, there still maybe an alternate path for netconsole to use; therefore there is no reason for a NETDEV_RELEASE event."
But my opinion it really isn't necessary to document what isn't done in the code, only what is done. The purpose of comments is to explain the wider impacts of the code.
^ permalink raw reply
* Re: [net-next] bonding: don't allow the master to become its slave
From: Flavio Leitner @ 2012-08-09 19:39 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, Jay Vosburgh, Andy Gospodarek, Leonardo Chiquitto,
Jiri Pirko
In-Reply-To: <1344539003.2593.7.camel@bwh-desktop.uk.solarflarecom.com>
On Thu, 9 Aug 2012 20:03:23 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
> > It doesn't make any sense to allow the master to become
> > its slave. That creates a loop of events causing a crash.
>
> What if there are other intermediate devices, e.g. the slave is a VLAN
> sub-device of the bond? And doesn't team also have this problem?
>
> I think a more general check for such loops might be required.
Maybe patching netdev_set_master() to fail in the loop case is
the way to go. That would work for bonding, team and bridge.
What you think?
fbl
^ permalink raw reply
* Re: [net-next] bonding: don't allow the master to become its slave
From: Jiri Pirko @ 2012-08-09 19:54 UTC (permalink / raw)
To: Ben Hutchings
Cc: Flavio Leitner, netdev, Jay Vosburgh, Andy Gospodarek,
Leonardo Chiquitto
In-Reply-To: <1344539003.2593.7.camel@bwh-desktop.uk.solarflarecom.com>
Thu, Aug 09, 2012 at 09:03:23PM CEST, bhutchings@solarflare.com wrote:
>On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
>> It doesn't make any sense to allow the master to become
>> its slave. That creates a loop of events causing a crash.
>
>What if there are other intermediate devices, e.g. the slave is a VLAN
>sub-device of the bond? And doesn't team also have this problem?
Yes, it does.
>
>I think a more general check for such loops might be required.
I agree.
>
>Ben.
>
>> Reported-by: Leonardo Chiquitto <lchiquitto@suse.com>
>> Signed-off-by: Flavio Leitner <fbl@redhat.com>
>> ---
>> drivers/net/bonding/bond_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 6fae5f3..5407b44 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1505,6 +1505,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> int link_reporting;
>> int res = 0;
>>
>> + if (bond_dev == slave_dev) {
>> + pr_err("%s: Error: cannot enslave itself.\n", bond_dev->name);
>> + return -EINVAL;
>> + }
>> +
>> if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
>> slave_ops->ndo_do_ioctl == NULL) {
>> pr_warning("%s: Warning: no link monitoring support for %s\n",
>
>--
>Ben Hutchings, Staff Engineer, Solarflare
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: [net-next] bonding: don't allow the master to become its slave
From: Jiri Pirko @ 2012-08-09 19:55 UTC (permalink / raw)
To: Flavio Leitner
Cc: Ben Hutchings, netdev, Jay Vosburgh, Andy Gospodarek,
Leonardo Chiquitto
In-Reply-To: <20120809163906.6dc0b6d4@obelix.rh>
Thu, Aug 09, 2012 at 09:39:06PM CEST, fbl@redhat.com wrote:
>On Thu, 9 Aug 2012 20:03:23 +0100
>Ben Hutchings <bhutchings@solarflare.com> wrote:
>
>> On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
>> > It doesn't make any sense to allow the master to become
>> > its slave. That creates a loop of events causing a crash.
>>
>> What if there are other intermediate devices, e.g. the slave is a VLAN
>> sub-device of the bond? And doesn't team also have this problem?
>>
>> I think a more general check for such loops might be required.
>
>Maybe patching netdev_set_master() to fail in the loop case is
>the way to go. That would work for bonding, team and bridge.
>
>What you think?
How about other devices who do not use "->master" like vlan, macvlan?
>
>fbl
^ 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