* Re: [PATCH RFC 2/2] net: dsa: bcm_sf2: implement HW bridging operations
From: Guenter Roeck @ 2015-02-19 5:59 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, davem, vivien.didelot, jerome.oufella, andrew, cphealy
In-Reply-To: <54E54EF3.9020802@gmail.com>
On Wed, Feb 18, 2015 at 06:48:19PM -0800, Florian Fainelli wrote:
> On 17/02/15 11:26, Florian Fainelli wrote:
> > Update the Broadcom Starfighter 2 switch driver to implement the
> > join/leave/stp_update callbacks required for basic hardware bridging
> > support.
> >
> > There is not much to be done at the driver level but translating the
> > STP state from Linux to their HW values.
> >
> > Joining a bridge means that the joining port and the other port members
> > need to be in the same VLAN membership as the CPU, while leaving the
> > bridge puts the port back into a separate VLAN membership with only the
> > CPU.
>
> I found a couple additional issues while testing:
>
> - manipulating UP/DOWN state of interfaces that are part of a bridge
> would not restore their bridge membership
>
> - removing an interface from a bridge and bringing it back up would
> leave it in blocked state
>
Is this a problem with your implementation for sf2 or a generic problem
with the first patch, such as some missing state transitions ?
For sf2, you might have to set the port state as well as the bridge
association in the port_setup function. That is of course just a
wild guess.
Thanks,
Guenter
^ permalink raw reply
* Re: Port STP state after removing port from bridge
From: roopa @ 2015-02-19 5:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: Scott Feldman, Jiří Pírko, netdev,
Stephen Hemminger
In-Reply-To: <CAGVrzcbRO1f2Bt-fxSV0=DTnGdXTB-KeE=Kma2+0qgGkqc5n8w@mail.gmail.com>
On 2/18/15, 9:00 PM, Florian Fainelli wrote:
> 2015-02-18 20:54 GMT-08:00 roopa <roopa@cumulusnetworks.com>:
>> On 2/18/15, 8:39 PM, Florian Fainelli wrote:
>>> Hi,
>>>
>>> It just occured to me that the following sequence:
>>>
>>> brctl addbr br0
>>> brctl addif br0 port0
>>> ... STP happens
>>> brctl delif br0 port0
>>>
>>> will leave port0 in STP disabled state, because the bridge code will
>>> set the STP state to DISABLED, and only a down/up sequence can bring
>>> it back to FORWARDING.
>>>
>>> Is this something that we should somehow fix? As an user it seems a
>>> little convoluted having to do a down/up sequence to restore things. I
>>> believe however that it is valid for the bridge layer to mark a port
>>> as DISABLED when removing it. This is typically not noticed or even
>>> remotely a problem with software bridges because we cannot enforce an
>>> actual STP state at the HW level.
>>>
>> Just curious, Are you only talking about hw state being left it DISABLED
>> state in the switchdev context ?.
> Right, this is in the context of DSA using NET_SWITCHDEV.
>
>> If yes, then cant the switch driver who is already listening to port leave
>> msgs, clear the disabled state on the port. ?
> I guess that is definitively possible, I am not seeing that being done
> for rocker which I used as a model here, maybe that needs fixing there
> as well?
o ok, yes, the switch drivers can do it. In this case rocker can/should.
Our driver does do the cleanup.
But if all drivers end up having this logic, it would maybe make sense
to consider it doing it in the bridge driver delete port code.
Thanks,
Roopa
^ permalink raw reply
* Re: [PATCH next v5 2/6] bonding: implement bond_poll_controller()
From: Mahesh Bandewar @ 2015-02-19 5:06 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Nikolay Aleksandrov, Jay Vosburgh, Andy Gospodarek,
Veaceslav Falico, David Miller, netdev, Eric Dumazet
In-Reply-To: <CANP3RGe7a9hwDB5iUapNvxVS_TD0kkmA3rWJnku=j5tUZ9v6rQ@mail.gmail.com>
On Wed, Feb 18, 2015 at 6:26 PM, Maciej Żenczykowski <maze@google.com> wrote:
>>>> Hi Mahesh,
>>>> I should've explained more in my review, you cannot sleep in
>>>> bond_poll_controller() so you cannot acquire rtnl like that. I was thinking
>>>> more about using rcu and switching to the _rcu version of
>>>> bond_for_each_slave instead.
>>>>
>>> That makes sense. The path that triggered this netpoll() could have
>>> been holding the rtnl itself and this would be a problem. I think
>>> using the _rcu variant of the slave iterator is a good idea, my bad!
>>>
>> ... however we cannot use the _rcu variant either since there is the
>> netpoll mutex (ni->dev_lock)!
>> The fact that we are here itself means that something bad had happened
>> and trying to take additional lock(s) would complicate the situation
>> further.
>
> I think you might be incorrectly assuming that we only get here on
> kernel crashes,
> upstream (netconsole) [may] violate[s] this assumption.
>
> (background: internally we don't consider netconsole production
> worthy, but we do
> 'abuse' the netpoll support framework to generate a minimal network
> dump on kernel crash,
> in which case if we crash or otherwise violate assumptions while
> crashing, we're not any worse off...)
>
I think irrespective of how we use it, there are few contradictory things here -
(a) If it can't sleep in netpoll() (which I agree with) then that
netpoll mutex does not make sense. But if it can then rtnl isn't a
wrong thing.
(b) If the netpoll mutex is right then slaves can not be iterated with
rcu protection.
(c) If we takeout the netpoll-mutex, then everything seems right but
can we really do that?
I'll take this patch out of this series for the time being and let
other patches proceed while I think about this patch some more.
--mahesh..
> - Maciej
^ permalink raw reply
* Re: Port STP state after removing port from bridge
From: Florian Fainelli @ 2015-02-19 5:00 UTC (permalink / raw)
To: roopa; +Cc: Scott Feldman, Jiří Pírko, netdev,
Stephen Hemminger
In-Reply-To: <54E56C6A.1060108@cumulusnetworks.com>
2015-02-18 20:54 GMT-08:00 roopa <roopa@cumulusnetworks.com>:
> On 2/18/15, 8:39 PM, Florian Fainelli wrote:
>>
>> Hi,
>>
>> It just occured to me that the following sequence:
>>
>> brctl addbr br0
>> brctl addif br0 port0
>> ... STP happens
>> brctl delif br0 port0
>>
>> will leave port0 in STP disabled state, because the bridge code will
>> set the STP state to DISABLED, and only a down/up sequence can bring
>> it back to FORWARDING.
>>
>> Is this something that we should somehow fix? As an user it seems a
>> little convoluted having to do a down/up sequence to restore things. I
>> believe however that it is valid for the bridge layer to mark a port
>> as DISABLED when removing it. This is typically not noticed or even
>> remotely a problem with software bridges because we cannot enforce an
>> actual STP state at the HW level.
>>
> Just curious, Are you only talking about hw state being left it DISABLED
> state in the switchdev context ?.
Right, this is in the context of DSA using NET_SWITCHDEV.
> If yes, then cant the switch driver who is already listening to port leave
> msgs, clear the disabled state on the port. ?
I guess that is definitively possible, I am not seeing that being done
for rocker which I used as a model here, maybe that needs fixing there
as well?
Thanks!
--
Florian
^ permalink raw reply
* Re: Port STP state after removing port from bridge
From: roopa @ 2015-02-19 4:54 UTC (permalink / raw)
To: Florian Fainelli
Cc: Scott Feldman, Jiří Pírko, netdev,
Stephen Hemminger
In-Reply-To: <CAGVrzcZMV3vSd3hEsXqk6BtyU-Ts9=oJvb79DTZp25TnnGm5CQ@mail.gmail.com>
On 2/18/15, 8:39 PM, Florian Fainelli wrote:
> Hi,
>
> It just occured to me that the following sequence:
>
> brctl addbr br0
> brctl addif br0 port0
> ... STP happens
> brctl delif br0 port0
>
> will leave port0 in STP disabled state, because the bridge code will
> set the STP state to DISABLED, and only a down/up sequence can bring
> it back to FORWARDING.
>
> Is this something that we should somehow fix? As an user it seems a
> little convoluted having to do a down/up sequence to restore things. I
> believe however that it is valid for the bridge layer to mark a port
> as DISABLED when removing it. This is typically not noticed or even
> remotely a problem with software bridges because we cannot enforce an
> actual STP state at the HW level.
>
Just curious, Are you only talking about hw state being left it DISABLED
state in the switchdev context ?.
If yes, then cant the switch driver who is already listening to port
leave msgs, clear the disabled state on the port. ?
^ permalink raw reply
* RE: Questions about the sh_eth driver and hardware
From: yoshihiro shimoda @ 2015-02-19 4:41 UTC (permalink / raw)
To: Ben Hutchings, Nobuhiro Iwamatsu, MITSUHIRO KIMURA,
HISASHI NAKAMURA(Retired), Yoshihiro Kaneko
Cc: netdev@vger.kernel.org, ct-linux-kernel
In-Reply-To: <1421861078.1222.271.camel@xylophone.i.decadent.org.uk>
Hi Ben,
This is Shimoda of Renesas. I'm a software engineer.
I asked hardware team about your questions. So, I wrote some comments below.
> 1. When freeing packet buffers, we currently try to stop the DMA engines
> by clearing EDRRR and EDTRR but we *don't* wait after that. This seems
> unsafe because in general register writes are not serialised with DMA.
>
> The R8A7790 (R-Car H2) manual specifically says that the R bit of EDTRR
> (aka CXR2) cannot be cleared by writing to it. I think that we could
> stop TX DMA by clearing the active flags of all the descriptors and then
> polling the R bit until it clears. What do you think?
>
> As for RX DMA, I think we should wait some time after clearing the R bit
> that we can be sure is long enough to transfer one packet. Do you know
> how long that could be?
They said that a software should wait for "set amount of time" or RX interruption.
So, I think that your patch (commit id = 740c7f31c in net-next.git) is enough.
> 2. In case of a Receive Descriptor Empty error (RDE), we currently read
> the RDFAR register to find the next descriptor the DMA engine will use.
> But this register is not documented for the R8A7790 and the driver does
> not define an offset for it on R-Car chips. The manual doesn't say how
> to set the address of the next descriptor to use. Maybe we should
> assume that R-Car chips will never skip descriptors after RDE?
They said that software should find the next descriptor without RDFAR register somehow
after RDE. However, I checked this driver handling on the R-Car M2, and the following
code didn't run actually. So, we should assume the R-Car chips will never skip descriptors
after RDE.
< sh_eth.c: sh_eth_rx() >
if (!(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
/* fix the values for the next receiving if RDE is set */
if (intr_status & EESR_RDE) {
/*vvvvvvv The code below didn't run on R-Car Gen2 vvvvvvv*/
u32 count = (sh_eth_read(ndev, RDFAR) -
sh_eth_read(ndev, RDLAR)) >> 4;
mdp->cur_rx = count;
mdp->dirty_rx = count;
/*^^^^^^^ The code above didn't run on R-Car Gen2 ^^^^^^^*/
}
sh_eth_write(ndev, EDRRR_R, EDRRR);
}
< Test method >
Host PC $ sudo ping -f -l 128 <the board's ip address>
Best regards,
Yoshihiro Shimoda
^ permalink raw reply
* Port STP state after removing port from bridge
From: Florian Fainelli @ 2015-02-19 4:39 UTC (permalink / raw)
To: Scott Feldman, Jiří Pírko, netdev,
Stephen Hemminger
Hi,
It just occured to me that the following sequence:
brctl addbr br0
brctl addif br0 port0
... STP happens
brctl delif br0 port0
will leave port0 in STP disabled state, because the bridge code will
set the STP state to DISABLED, and only a down/up sequence can bring
it back to FORWARDING.
Is this something that we should somehow fix? As an user it seems a
little convoluted having to do a down/up sequence to restore things. I
believe however that it is valid for the bridge layer to mark a port
as DISABLED when removing it. This is typically not noticed or even
remotely a problem with software bridges because we cannot enforce an
actual STP state at the HW level.
Let me know your thoughts.
--
Florian
^ permalink raw reply
* Re: [PATCH RFC 2/2] net: dsa: bcm_sf2: implement HW bridging operations
From: Florian Fainelli @ 2015-02-19 2:48 UTC (permalink / raw)
To: netdev; +Cc: davem, vivien.didelot, jerome.oufella, linux, andrew, cphealy
In-Reply-To: <1424201196-4901-3-git-send-email-f.fainelli@gmail.com>
On 17/02/15 11:26, Florian Fainelli wrote:
> Update the Broadcom Starfighter 2 switch driver to implement the
> join/leave/stp_update callbacks required for basic hardware bridging
> support.
>
> There is not much to be done at the driver level but translating the
> STP state from Linux to their HW values.
>
> Joining a bridge means that the joining port and the other port members
> need to be in the same VLAN membership as the CPU, while leaving the
> bridge puts the port back into a separate VLAN membership with only the
> CPU.
I found a couple additional issues while testing:
- manipulating UP/DOWN state of interfaces that are part of a bridge
would not restore their bridge membership
- removing an interface from a bridge and bringing it back up would
leave it in blocked state
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/dsa/bcm_sf2.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 4daffb284931..006c86a4df54 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -23,6 +23,7 @@
> #include <linux/of_address.h>
> #include <net/dsa.h>
> #include <linux/ethtool.h>
> +#include <linux/if_bridge.h>
>
> #include "bcm_sf2.h"
> #include "bcm_sf2_regs.h"
> @@ -400,6 +401,95 @@ static int bcm_sf2_sw_set_eee(struct dsa_switch *ds, int port,
> return 0;
> }
>
> +static int bcm_sf2_sw_br_join(struct dsa_switch *ds, int port,
> + u32 br_port_mask)
> +{
> + struct bcm_sf2_priv *priv = ds_to_priv(ds);
> + unsigned int i;
> + u32 reg, p_ctl;
> +
> + p_ctl = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(port));
> +
> + for (i = 0; i < priv->hw_params.num_ports; i++) {
> + if (!((1 << i) & br_port_mask))
> + continue;
> +
> + reg = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(i));
> + reg |= 1 << port;
> + core_writel(priv, reg, CORE_PORT_VLAN_CTL_PORT(i));
> +
> + p_ctl |= 1 << i;
> + }
> +
> + core_writel(priv, p_ctl, CORE_PORT_VLAN_CTL_PORT(port));
> +
> + return 0;
> +}
> +
> +static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port,
> + u32 br_port_mask)
> +{
> + struct bcm_sf2_priv *priv = ds_to_priv(ds);
> + unsigned int i;
> + u32 reg, p_ctl;
> +
> + p_ctl = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(port));
> +
> + for (i = 0; i < priv->hw_params.num_ports; i++) {
> + /* Don't touch the remaining ports */
> + if (!((1 << i) & br_port_mask))
> + continue;
> +
> + reg = core_readl(priv, CORE_PORT_VLAN_CTL_PORT(i));
> + reg &= ~(1 << port);
> + core_writel(priv, reg, CORE_PORT_VLAN_CTL_PORT(i));
> +
> + /* Prevent self removal to preserve isolation */
> + if (port != i)
> + p_ctl &= ~(1 << i);
> + }
> +
> + core_writel(priv, p_ctl, CORE_PORT_VLAN_CTL_PORT(port));
> +
> + return 0;
> +}
> +
> +static int bcm_sf2_sw_br_stp_update(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + struct bcm_sf2_priv *priv = ds_to_priv(ds);
> + u32 reg;
> + u8 hw_state;
> +
> + switch (state) {
> + case BR_STATE_DISABLED:
> + hw_state = G_MISTP_DIS_STATE;
> + break;
> + case BR_STATE_LISTENING:
> + hw_state = G_MISTP_LISTEN_STATE;
> + break;
> + case BR_STATE_LEARNING:
> + hw_state = G_MISTP_LEARN_STATE;
> + break;
> + case BR_STATE_FORWARDING:
> + hw_state = G_MISTP_FWD_STATE;
> + break;
> + case BR_STATE_BLOCKING:
> + hw_state = G_MISTP_BLOCK_STATE;
> + break;
> + default:
> + pr_err("%s: invalid STP state: %d\n", __func__, state);
> + return -EINVAL;
> + }
> +
> + reg = core_readl(priv, CORE_G_PCTL_PORT(port));
> + reg &= ~(G_MISTP_STATE_MASK << G_MISTP_STATE_SHIFT);
> + reg |= hw_state;
> + core_writel(priv, reg, CORE_G_PCTL_PORT(port));
> +
> + return 0;
> +}
> +
> static irqreturn_t bcm_sf2_switch_0_isr(int irq, void *dev_id)
> {
> struct bcm_sf2_priv *priv = dev_id;
> @@ -916,6 +1006,9 @@ static struct dsa_switch_driver bcm_sf2_switch_driver = {
> .port_disable = bcm_sf2_port_disable,
> .get_eee = bcm_sf2_sw_get_eee,
> .set_eee = bcm_sf2_sw_set_eee,
> + .port_join_bridge = bcm_sf2_sw_br_join,
> + .port_leave_bridge = bcm_sf2_sw_br_leave,
> + .port_stp_update = bcm_sf2_sw_br_stp_update,
> };
>
> static int __init bcm_sf2_init(void)
>
--
Florian
^ permalink raw reply
* Re: [PATCH next v5 2/6] bonding: implement bond_poll_controller()
From: Maciej Żenczykowski @ 2015-02-19 2:26 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: Nikolay Aleksandrov, Jay Vosburgh, Andy Gospodarek,
Veaceslav Falico, David Miller, netdev, Eric Dumazet
In-Reply-To: <CAF2d9ji7VqqB95nD=VezL6fAaGJKaHCJRYf+b1VsCCLFy4r7NA@mail.gmail.com>
>>> Hi Mahesh,
>>> I should've explained more in my review, you cannot sleep in
>>> bond_poll_controller() so you cannot acquire rtnl like that. I was thinking
>>> more about using rcu and switching to the _rcu version of
>>> bond_for_each_slave instead.
>>>
>> That makes sense. The path that triggered this netpoll() could have
>> been holding the rtnl itself and this would be a problem. I think
>> using the _rcu variant of the slave iterator is a good idea, my bad!
>>
> ... however we cannot use the _rcu variant either since there is the
> netpoll mutex (ni->dev_lock)!
> The fact that we are here itself means that something bad had happened
> and trying to take additional lock(s) would complicate the situation
> further.
I think you might be incorrectly assuming that we only get here on
kernel crashes,
upstream (netconsole) [may] violate[s] this assumption.
(background: internally we don't consider netconsole production
worthy, but we do
'abuse' the netpoll support framework to generate a minimal network
dump on kernel crash,
in which case if we crash or otherwise violate assumptions while
crashing, we're not any worse off...)
- Maciej
^ permalink raw reply
* Re: [PATCH next v5 2/6] bonding: implement bond_poll_controller()
From: Mahesh Bandewar @ 2015-02-19 1:30 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico, David Miller,
Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <CAF2d9jjUg1_SzoCNvTsGq5+UNuWnoreCB8dqk28TLgeT1tsO8w@mail.gmail.com>
On Wed, Feb 18, 2015 at 5:19 PM, Mahesh Bandewar <maheshb@google.com> wrote:
> On Wed, Feb 18, 2015 at 4:10 PM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
>> On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
>>> This patches implements the poll_controller support for all
>>> bonding driver. If the slaves have poll_controller net_op defined,
>>> this implementation calls them. This is mode agnostic implementation
>>> and iterates through all slaves (based on mode) and calls respective
>>> handler.
>>>
>>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>>> ---
>>> v1:
>>> Initial version
>>> v2:
>>> Eliminate bool variable.
>>> v3:
>>> Rebase
>>> v4:
>>> Removed 3AD port_operational check
>>> v5:
>>> Added rtnl protection for bond_for_each_slave()
>>>
>>> drivers/net/bonding/bond_main.c | 33 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 33 insertions(+)
>>>
>>
>> Hi Mahesh,
>> I should've explained more in my review, you cannot sleep in
>> bond_poll_controller() so you cannot acquire rtnl like that. I was thinking
>> more about using rcu and switching to the _rcu version of
>> bond_for_each_slave instead.
>>
> That makes sense. The path that triggered this netpoll() could have
> been holding the rtnl itself and this would be a problem. I think
> using the _rcu variant of the slave iterator is a good idea, my bad!
>
... however we cannot use the _rcu variant either since there is the
netpoll mutex (ni->dev_lock)!
The fact that we are here itself means that something bad had happened
and trying to take additional lock(s) would complicate the situation
further.
>> Cheers,
>> Nik
>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index b979c265fc51..63e6c0dbe7b3 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -928,6 +928,39 @@ static inline void slave_disable_netpoll(struct slave *slave)
>>>
>>> static void bond_poll_controller(struct net_device *bond_dev)
>>> {
>>> + struct bonding *bond = netdev_priv(bond_dev);
>>> + struct slave *slave = NULL;
>>> + struct list_head *iter;
>>> + struct ad_info ad_info;
>>> + struct netpoll_info *ni;
>>> + const struct net_device_ops *ops;
>>> +
>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD)
>>> + if (bond_3ad_get_active_agg_info(bond, &ad_info))
>>> + return;
>>> +
>>> + rtnl_lock();
>>> + bond_for_each_slave(bond, slave, iter) {
>>> + ops = slave->dev->netdev_ops;
>>> + if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
>>> + continue;
>>> +
>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>> + struct aggregator *agg =
>>> + SLAVE_AD_INFO(slave)->port.aggregator;
>>> +
>>> + if (agg &&
>>> + agg->aggregator_identifier != ad_info.aggregator_id)
>>> + continue;
>>> + }
>>> +
>>> + ni = rcu_dereference_bh(slave->dev->npinfo);
>>> + if (down_trylock(&ni->dev_lock))
>>> + continue;
>>> + ops->ndo_poll_controller(slave->dev);
>>> + up(&ni->dev_lock);
>>> + }
>>> + rtnl_unlock();
>>> }
>>>
>>> static void bond_netpoll_cleanup(struct net_device *bond_dev)
>>>
>>
^ permalink raw reply
* Re: [PATCH next v5 2/6] bonding: implement bond_poll_controller()
From: Mahesh Bandewar @ 2015-02-19 1:19 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico, David Miller,
Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <54E529E6.609@redhat.com>
On Wed, Feb 18, 2015 at 4:10 PM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
>> This patches implements the poll_controller support for all
>> bonding driver. If the slaves have poll_controller net_op defined,
>> this implementation calls them. This is mode agnostic implementation
>> and iterates through all slaves (based on mode) and calls respective
>> handler.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>> v1:
>> Initial version
>> v2:
>> Eliminate bool variable.
>> v3:
>> Rebase
>> v4:
>> Removed 3AD port_operational check
>> v5:
>> Added rtnl protection for bond_for_each_slave()
>>
>> drivers/net/bonding/bond_main.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>
> Hi Mahesh,
> I should've explained more in my review, you cannot sleep in
> bond_poll_controller() so you cannot acquire rtnl like that. I was thinking
> more about using rcu and switching to the _rcu version of
> bond_for_each_slave instead.
>
That makes sense. The path that triggered this netpoll() could have
been holding the rtnl itself and this would be a problem. I think
using the _rcu variant of the slave iterator is a good idea, my bad!
> Cheers,
> Nik
>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b979c265fc51..63e6c0dbe7b3 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -928,6 +928,39 @@ static inline void slave_disable_netpoll(struct slave *slave)
>>
>> static void bond_poll_controller(struct net_device *bond_dev)
>> {
>> + struct bonding *bond = netdev_priv(bond_dev);
>> + struct slave *slave = NULL;
>> + struct list_head *iter;
>> + struct ad_info ad_info;
>> + struct netpoll_info *ni;
>> + const struct net_device_ops *ops;
>> +
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> + if (bond_3ad_get_active_agg_info(bond, &ad_info))
>> + return;
>> +
>> + rtnl_lock();
>> + bond_for_each_slave(bond, slave, iter) {
>> + ops = slave->dev->netdev_ops;
>> + if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
>> + continue;
>> +
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> + struct aggregator *agg =
>> + SLAVE_AD_INFO(slave)->port.aggregator;
>> +
>> + if (agg &&
>> + agg->aggregator_identifier != ad_info.aggregator_id)
>> + continue;
>> + }
>> +
>> + ni = rcu_dereference_bh(slave->dev->npinfo);
>> + if (down_trylock(&ni->dev_lock))
>> + continue;
>> + ops->ndo_poll_controller(slave->dev);
>> + up(&ni->dev_lock);
>> + }
>> + rtnl_unlock();
>> }
>>
>> static void bond_netpoll_cleanup(struct net_device *bond_dev)
>>
>
^ permalink raw reply
* Re: [PATCH] net: smc91x: improve neponset hack
From: Russell King - ARM Linux @ 2015-02-19 0:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: netdev, davem, linux-arm-kernel, Dmitry Eremin-Solenikov,
linux-kernel, Linus Walleij
In-Reply-To: <5763365.j5Nvv4LgM3@wuerfel>
On Wed, Feb 18, 2015 at 08:47:30PM +0100, Arnd Bergmann wrote:
> The smc91x driver tries to support multiple platforms at compile
> time, but they are mutually exclusive at runtime, and not clearly
> defined.
I'd prefer to rework this to fix that properly. From what I remember,
the whole SA11x0 stuff in this driver was a mess.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH next v5 6/6] bonding: Implement user key part of port_key in an AD system.
From: Nikolay Aleksandrov @ 2015-02-19 0:21 UTC (permalink / raw)
To: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller
Cc: Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <1424298689-18473-1-git-send-email-maheshb@google.com>
On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
> The port key has three components - user-key, speed-part, and duplex-part.
> The LSBit is for the duplex-part, next 5 bits are for the speed while the
> remaining 10 bits are the user defined key bits. Get these 10 bits
> from the user-space (through the SysFs interface) and use it to form the
> admin port-key. Allowed range for the user-key is 0 - 1023 (10 bits). If
> it is not provided then use zero for the user-key-bits (default).
>
> It can set using following example code -
>
> # modprobe bonding mode=4
> # usr_port_key=$(( RANDOM & 0x3FF ))
> # echo $usr_port_key > /sys/class/net/bond0/bonding/ad_user_port_key
> # echo +eth1 > /sys/class/net/bond0/bonding/slaves
> ...
> # ip link set bond0 up
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1:
> Initial version
> v2:
> Renamed ad_actor_user_port_key ad_user_port_key
> v3-v4:
> Rebase
> v5:
> Cosmetic changes
>
> Documentation/networking/bonding.txt | 63 ++++++++++++++++++++++++++++++++++++
> drivers/net/bonding/bond_3ad.c | 14 ++++----
> drivers/net/bonding/bond_main.c | 10 ++++++
> drivers/net/bonding/bond_options.c | 26 +++++++++++++++
> drivers/net/bonding/bond_sysfs.c | 15 +++++++++
> include/net/bond_options.h | 1 +
> include/net/bonding.h | 1 +
> 7 files changed, 123 insertions(+), 7 deletions(-)
>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
^ permalink raw reply
* Re: Why do we use RX queue mapping for TX?
From: Eric Dumazet @ 2015-02-19 0:16 UTC (permalink / raw)
To: Cong Wang; +Cc: David S. Miller, netdev
In-Reply-To: <CAHA+R7PbLRw-oA4v9ivZ0s-++ALx99zddJLDqDoemyBTc2XZ1g@mail.gmail.com>
On Wed, 2015-02-18 at 15:57 -0800, Cong Wang wrote:
> I am aware of it. I even would guess (means not digging the history)
> skb_edit was invented for bonding queue mapping, since I don't see it
> even works as a general TC action on the physical interface. We pick
> the tx queue prior to getting the Qdisc, therefore too late to set
> skb->queue_mapping to specify a hardware TX queue. This is
> another story I planned to bring it up to David.
skbedit can be done on ingress I presume.
^ permalink raw reply
* Re: [PATCH next v5 5/6] bonding: Allow userspace to set actors' macaddr in an AD-system.
From: Nikolay Aleksandrov @ 2015-02-19 0:15 UTC (permalink / raw)
To: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller
Cc: Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <1424298688-18434-1-git-send-email-maheshb@google.com>
On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
> In an AD system, the communication between actor and partner is the
> business between these two entities. In the current setup anyone on the
> same L2 can "guess" the LACPDU contents and then possibly send the
> spoofed LACPDUs and trick the partner causing connectivity issues for
> the AD system. This patch allows to use a random mac-address obscuring
> it's identity making it harder for someone in the L2 is do the same thing.
>
> This patch allows user-space to choose the mac-address for the AD-system.
> This mac-address can not be NULL or a Multicast. If the mac-address is set
> from user-space; kernel will honor it and will not overwrite it. In the
> absence (value from user space); the logic will default to using the
> masters' mac as the mac-address for the AD-system.
>
> It can be set using example code below -
>
> # modprobe bonding mode=4
> # sys_mac_addr=$(printf '%02x:%02x:%02x:%02x:%02x:%02x' \
> $(( (RANDOM & 0xFE) | 0x02 )) \
> $(( RANDOM & 0xFF )) \
> $(( RANDOM & 0xFF )) \
> $(( RANDOM & 0xFF )) \
> $(( RANDOM & 0xFF )) \
> $(( RANDOM & 0xFF )))
> # echo $sys_mac_addr > /sys/class/net/bond0/bonding/ad_actor_system
> # echo +eth1 > /sys/class/net/bond0/bonding/slaves
> ...
> # ip link set bond0 up
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1:
> Initial version
> v2:
> Renamed ad_actor_system_mac_address to ad_actor_system
> v3:
> Fixed commit message.
> v4:
> Rebase
> v5:
> Cosmetic changes
>
> Documentation/networking/bonding.txt | 12 ++++++++++++
> drivers/net/bonding/bond_3ad.c | 7 ++++++-
> drivers/net/bonding/bond_main.c | 1 +
> drivers/net/bonding/bond_options.c | 29 +++++++++++++++++++++++++++++
> drivers/net/bonding/bond_procfs.c | 6 ++++++
> drivers/net/bonding/bond_sysfs.c | 15 +++++++++++++++
> include/net/bond_options.h | 1 +
> include/net/bonding.h | 1 +
> 8 files changed, 71 insertions(+), 1 deletion(-)
>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
^ permalink raw reply
* Re: [PATCH next v5 4/6] bonding: Allow userspace to set actors' system_priority in AD system
From: Nikolay Aleksandrov @ 2015-02-19 0:13 UTC (permalink / raw)
To: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller
Cc: Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <1424298685-18394-1-git-send-email-maheshb@google.com>
On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
> This patch allows user to randomize the system-priority in an ad-system.
> The allowed range is 1 - 0xFFFF while default value is 0xFFFF. If user
> does not specify this value, the system defaults to 0xFFFF, which is
> what it was before this patch.
>
> Following example code could set the value -
> # modprobe bonding mode=4
> # sys_prio=$(( 1 + RANDOM + RANDOM ))
> # echo $sys_prio > /sys/class/net/bond0/bonding/ad_actor_sys_prio
> # echo +eth1 > /sys/class/net/bond0/bonding/slaves
> ...
> # ip link set bond0 up
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1:
> Initial version
> v2:
> Rename ad_actor_system_priority to ad_actor_sys_prio
> v3-v4:
> Rebase
> v5:
> Cosmetic changes
>
> Documentation/networking/bonding.txt | 9 +++++++++
> drivers/net/bonding/bond_3ad.c | 5 ++++-
> drivers/net/bonding/bond_main.c | 14 ++++++++++++++
> drivers/net/bonding/bond_options.c | 29 ++++++++++++++++++++++++++++-
> drivers/net/bonding/bond_procfs.c | 2 ++
> drivers/net/bonding/bond_sysfs.c | 15 +++++++++++++++
> include/net/bond_options.h | 1 +
> include/net/bonding.h | 1 +
> 8 files changed, 74 insertions(+), 2 deletions(-)
>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
^ permalink raw reply
* Re: [PATCH next v5 3/6] bonding: Implement port churn-machine (AD standard 43.4.17).
From: Nikolay Aleksandrov @ 2015-02-19 0:12 UTC (permalink / raw)
To: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller
Cc: Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <1424298684-18267-1-git-send-email-maheshb@google.com>
On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
> The Churn Detection machines detect the situation where a port is operable,
> but the Actor and Partner have not attached the link to an Aggregator and
> brought the link into operation within a bound time period. Under normal
> operation of the LACP, agreement between Actor and Partner should be reached
> very rapidly. Continued failure to reach agreement can be symptomatic of
> device failure.
>
> Actor-churn-detection state-machine
> ===================================
>
> BEGIN=True + PortEnable=False
> |
> v
> +------------------------+ ActorPort.Sync=True +------------------+
> | ACTOR_CHURN_MONITOR | ---------------------> | NO_ACTOR_CHURN |
> |========================| |==================|
> | ActorChurn=False | ActorPort.Sync=False | ActorChurn=False |
> | ActorChurn.Timer=Start | <--------------------- | |
> +------------------------+ +------------------+
> | ^
> | |
> ActorChurn.Timer=Expired |
> | ActorPort.Sync=True
> | |
> | +-----------------+ |
> | | ACTOR_CHURN | |
> | |=================| |
> +--------------> | ActorChurn=True | ------------+
> | |
> +-----------------+
>
> Similar for the Partner-churn-detection.
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1:
> Initial version
> v2-v4:
> Rebase
> v5:
> Cosmetic changes
>
> drivers/net/bonding/bond_3ad.c | 56 +++++++++++++++++++++++++++++++++++++--
> drivers/net/bonding/bond_procfs.c | 41 +++++++++++++++++++++++++---
> include/net/bond_3ad.h | 29 ++++++++++++++++++++
> 3 files changed, 120 insertions(+), 6 deletions(-)
>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
^ permalink raw reply
* Re: [PATCH next v5 2/6] bonding: implement bond_poll_controller()
From: Nikolay Aleksandrov @ 2015-02-19 0:10 UTC (permalink / raw)
To: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller
Cc: Maciej Zenczykowski, netdev, Eric Dumazet
In-Reply-To: <1424298682-18227-1-git-send-email-maheshb@google.com>
On 02/18/2015 11:31 PM, Mahesh Bandewar wrote:
> This patches implements the poll_controller support for all
> bonding driver. If the slaves have poll_controller net_op defined,
> this implementation calls them. This is mode agnostic implementation
> and iterates through all slaves (based on mode) and calls respective
> handler.
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1:
> Initial version
> v2:
> Eliminate bool variable.
> v3:
> Rebase
> v4:
> Removed 3AD port_operational check
> v5:
> Added rtnl protection for bond_for_each_slave()
>
> drivers/net/bonding/bond_main.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
Hi Mahesh,
I should've explained more in my review, you cannot sleep in
bond_poll_controller() so you cannot acquire rtnl like that. I was thinking
more about using rcu and switching to the _rcu version of
bond_for_each_slave instead.
Cheers,
Nik
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b979c265fc51..63e6c0dbe7b3 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -928,6 +928,39 @@ static inline void slave_disable_netpoll(struct slave *slave)
>
> static void bond_poll_controller(struct net_device *bond_dev)
> {
> + struct bonding *bond = netdev_priv(bond_dev);
> + struct slave *slave = NULL;
> + struct list_head *iter;
> + struct ad_info ad_info;
> + struct netpoll_info *ni;
> + const struct net_device_ops *ops;
> +
> + if (BOND_MODE(bond) == BOND_MODE_8023AD)
> + if (bond_3ad_get_active_agg_info(bond, &ad_info))
> + return;
> +
> + rtnl_lock();
> + bond_for_each_slave(bond, slave, iter) {
> + ops = slave->dev->netdev_ops;
> + if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
> + continue;
> +
> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> + struct aggregator *agg =
> + SLAVE_AD_INFO(slave)->port.aggregator;
> +
> + if (agg &&
> + agg->aggregator_identifier != ad_info.aggregator_id)
> + continue;
> + }
> +
> + ni = rcu_dereference_bh(slave->dev->npinfo);
> + if (down_trylock(&ni->dev_lock))
> + continue;
> + ops->ndo_poll_controller(slave->dev);
> + up(&ni->dev_lock);
> + }
> + rtnl_unlock();
> }
>
> static void bond_netpoll_cleanup(struct net_device *bond_dev)
>
^ permalink raw reply
* Re: [PATCH] net: Remove state argument from skb_find_text()
From: Pablo Neira Ayuso @ 2015-02-19 0:05 UTC (permalink / raw)
To: Bojan Prtvar; +Cc: netdev, tgraf, netfilter-devel, netfilter, dan.carpenter
In-Reply-To: <1424301408-3659-1-git-send-email-prtvar.b@gmail.com>
On Thu, Feb 19, 2015 at 12:16:48AM +0100, Bojan Prtvar wrote:
> Although it is clear that textsearch state is intentionally passed
> to skb_find_text() as uninitialized argument, it was never used by
> the callers. Therefore, we can simplify skb_find_text() by making it
> local variable.
This infrastructure was conceived to allow keeping the state between
chunk of data, I don't find a good reason to restrict this.
^ permalink raw reply
* Re: Why do we use RX queue mapping for TX?
From: Cong Wang @ 2015-02-18 23:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev
In-Reply-To: <1424301248.4942.99.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Feb 18, 2015 at 3:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2015-02-18 at 14:11 -0800, Cong Wang wrote:
>> On Wed, Feb 18, 2015 at 1:18 PM, Cong Wang <cwang@twopensource.com> wrote:
>> >
>> > 2) This breaks the queue mapping specified by an skbedit action,
>> > since for TX queues the index starts with 0 while for RX it starts with 1
>> > (for some reason I don't see yet). There is at least a mismatch.
>>
>> So queue 0 is reserved for TX, at least for bonding queue mapping.
>
> It seems you missed that bonding ndo_select_queue() is rather special.
>
>
I am aware of it. I even would guess (means not digging the history)
skb_edit was invented for bonding queue mapping, since I don't see it
even works as a general TC action on the physical interface. We pick
the tx queue prior to getting the Qdisc, therefore too late to set
skb->queue_mapping to specify a hardware TX queue. This is
another story I planned to bring it up to David.
However, this still doesn't seem to be a reason to break people who
don't use bonding at all? At least we just want to map skb's to different
hardware TX queues by setting skb->queue_mapping (before
dev_queue_xmit() of course) and sysfs reports the queues starting with
index 0. This is why I complain. :)
^ permalink raw reply
* Re: [PATCH net] netfilter: xt_socket: fix a stack corruption bug
From: Pablo Neira Ayuso @ 2015-02-18 23:45 UTC (permalink / raw)
To: Eric Dumazet
Cc: Balazs Scheidler, KOVACS Krisztian, Patrick McHardy,
Jozsef Kadlecsik, netfilter-devel, netdev
In-Reply-To: <1424055825.4942.50.camel@edumazet-glaptop2.roam.corp.google.com>
On Sun, Feb 15, 2015 at 07:03:45PM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> As soon as extract_icmp6_fields() returns, its local storage (automatic
> variables) is deallocated and can be overwritten.
>
> Lets add an additional parameter to make sure storage is valid long
> enough.
>
> While we are at it, adds some const qualifiers.
Applied, thanks Eric.
^ permalink raw reply
* [PATCH net] cxgb4: Fix incorrect 'c' suffix to %pI4, use %pISc instead
From: Anish Bhatt @ 2015-02-18 23:29 UTC (permalink / raw)
To: netdev; +Cc: davem, hariprasad, dan.carpenter, Anish Bhatt
Issue caught by 0-day kernel test infrastructure. Code changed to use sockaddr
members so that %pISc can be used instead.
Fixes: b5a02f503caa ('cxgb4 : Update ipv6 address handling api')
Signed-off-by: Anish Bhatt <anish@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c | 57 ++++++++++++++-------------
drivers/net/ethernet/chelsio/cxgb4/clip_tbl.h | 6 ++-
2 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c b/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c
index 9062a84..c308429 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c
@@ -35,10 +35,10 @@ static inline unsigned int ipv6_clip_hash(struct clip_tbl *d, const u32 *key)
}
static unsigned int clip_addr_hash(struct clip_tbl *ctbl, const u32 *addr,
- int addr_len)
+ u8 v6)
{
- return addr_len == 4 ? ipv4_clip_hash(ctbl, addr) :
- ipv6_clip_hash(ctbl, addr);
+ return v6 ? ipv6_clip_hash(ctbl, addr) :
+ ipv4_clip_hash(ctbl, addr);
}
static int clip6_get_mbox(const struct net_device *dev,
@@ -78,23 +78,22 @@ int cxgb4_clip_get(const struct net_device *dev, const u32 *lip, u8 v6)
struct clip_entry *ce, *cte;
u32 *addr = (u32 *)lip;
int hash;
- int addr_len;
- int ret = 0;
+ int ret = -1;
if (!ctbl)
return 0;
- if (v6)
- addr_len = 16;
- else
- addr_len = 4;
-
- hash = clip_addr_hash(ctbl, addr, addr_len);
+ hash = clip_addr_hash(ctbl, addr, v6);
read_lock_bh(&ctbl->lock);
list_for_each_entry(cte, &ctbl->hash_list[hash], list) {
- if (addr_len == cte->addr_len &&
- memcmp(lip, cte->addr, cte->addr_len) == 0) {
+ if (cte->addr6.sin6_family == AF_INET6 && v6)
+ ret = memcmp(lip, cte->addr6.sin6_addr.s6_addr,
+ sizeof(struct in6_addr));
+ else if (cte->addr.sin_family == AF_INET && !v6)
+ ret = memcmp(lip, (char *)(&cte->addr.sin_addr),
+ sizeof(struct in_addr));
+ if (!ret) {
ce = cte;
read_unlock_bh(&ctbl->lock);
goto found;
@@ -111,15 +110,20 @@ int cxgb4_clip_get(const struct net_device *dev, const u32 *lip, u8 v6)
spin_lock_init(&ce->lock);
atomic_set(&ce->refcnt, 0);
atomic_dec(&ctbl->nfree);
- ce->addr_len = addr_len;
- memcpy(ce->addr, lip, addr_len);
list_add_tail(&ce->list, &ctbl->hash_list[hash]);
if (v6) {
+ ce->addr6.sin6_family = AF_INET6;
+ memcpy(ce->addr6.sin6_addr.s6_addr,
+ lip, sizeof(struct in6_addr));
ret = clip6_get_mbox(dev, (const struct in6_addr *)lip);
if (ret) {
write_unlock_bh(&ctbl->lock);
return ret;
}
+ } else {
+ ce->addr.sin_family = AF_INET;
+ memcpy((char *)(&ce->addr.sin_addr), lip,
+ sizeof(struct in_addr));
}
} else {
write_unlock_bh(&ctbl->lock);
@@ -140,19 +144,19 @@ void cxgb4_clip_release(const struct net_device *dev, const u32 *lip, u8 v6)
struct clip_entry *ce, *cte;
u32 *addr = (u32 *)lip;
int hash;
- int addr_len;
-
- if (v6)
- addr_len = 16;
- else
- addr_len = 4;
+ int ret = -1;
- hash = clip_addr_hash(ctbl, addr, addr_len);
+ hash = clip_addr_hash(ctbl, addr, v6);
read_lock_bh(&ctbl->lock);
list_for_each_entry(cte, &ctbl->hash_list[hash], list) {
- if (addr_len == cte->addr_len &&
- memcmp(lip, cte->addr, cte->addr_len) == 0) {
+ if (cte->addr6.sin6_family == AF_INET6 && v6)
+ ret = memcmp(lip, cte->addr6.sin6_addr.s6_addr,
+ sizeof(struct in6_addr));
+ else if (cte->addr.sin_family == AF_INET && !v6)
+ ret = memcmp(lip, (char *)(&cte->addr.sin_addr),
+ sizeof(struct in_addr));
+ if (!ret) {
ce = cte;
read_unlock_bh(&ctbl->lock);
goto found;
@@ -249,10 +253,7 @@ int clip_tbl_show(struct seq_file *seq, void *v)
for (i = 0 ; i < ctbl->clipt_size; ++i) {
list_for_each_entry(ce, &ctbl->hash_list[i], list) {
ip[0] = '\0';
- if (ce->addr_len == 16)
- sprintf(ip, "%pI6c", ce->addr);
- else
- sprintf(ip, "%pI4c", ce->addr);
+ sprintf(ip, "%pISc", &ce->addr);
seq_printf(seq, "%-25s %u\n", ip,
atomic_read(&ce->refcnt));
}
diff --git a/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.h b/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.h
index 2eaba01..35eb43c 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/clip_tbl.h
@@ -14,8 +14,10 @@ struct clip_entry {
spinlock_t lock; /* Hold while modifying clip reference */
atomic_t refcnt;
struct list_head list;
- u32 addr[4];
- int addr_len;
+ union {
+ struct sockaddr_in addr;
+ struct sockaddr_in6 addr6;
+ };
};
struct clip_tbl {
--
2.3.0
^ permalink raw reply related
* [PATCH] net: Remove state argument from skb_find_text()
From: Bojan Prtvar @ 2015-02-18 23:16 UTC (permalink / raw)
To: netdev; +Cc: tgraf, netfilter-devel, netfilter, dan.carpenter, Bojan Prtvar
Although it is clear that textsearch state is intentionally passed to skb_find_text() as uninitialized argument, it was never used by the callers. Therefore, we can simplify skb_find_text() by making it local variable.
Signed-off-by: Bojan Prtvar <prtvar.b@gmail.com>
---
include/linux/skbuff.h | 3 +--
net/core/skbuff.c | 9 ++++-----
net/netfilter/nf_conntrack_amanda.c | 10 +++-------
net/netfilter/xt_string.c | 4 +---
net/sched/em_text.c | 3 +--
5 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 08074a8..6cc1b7d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -776,8 +776,7 @@ unsigned int skb_seq_read(unsigned int consumed, const u8 **data,
void skb_abort_seq_read(struct skb_seq_state *st);
unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
- unsigned int to, struct ts_config *config,
- struct ts_state *state);
+ unsigned int to, struct ts_config *config);
/*
* Packet hash types specify the type of hash in skb_set_hash.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1b62343..c0e64f7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2747,7 +2747,6 @@ static void skb_ts_finish(struct ts_config *conf, struct ts_state *state)
* @from: search offset
* @to: search limit
* @config: textsearch configuration
- * @state: uninitialized textsearch state variable
*
* Finds a pattern in the skb data according to the specified
* textsearch configuration. Use textsearch_next() to retrieve
@@ -2755,17 +2754,17 @@ static void skb_ts_finish(struct ts_config *conf, struct ts_state *state)
* to the first occurrence or UINT_MAX if no match was found.
*/
unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
- unsigned int to, struct ts_config *config,
- struct ts_state *state)
+ unsigned int to, struct ts_config *config)
{
+ struct ts_state state;
unsigned int ret;
config->get_next_block = skb_ts_get_next_block;
config->finish = skb_ts_finish;
- skb_prepare_seq_read(skb, from, to, TS_SKB_CB(state));
+ skb_prepare_seq_read(skb, from, to, TS_SKB_CB(&state));
- ret = textsearch_find(config, state);
+ ret = textsearch_find(config, &state);
return (ret <= to - from ? ret : UINT_MAX);
}
EXPORT_SYMBOL(skb_find_text);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index b8b95f4..57a26cc 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -88,7 +88,6 @@ static int amanda_help(struct sk_buff *skb,
struct nf_conn *ct,
enum ip_conntrack_info ctinfo)
{
- struct ts_state ts;
struct nf_conntrack_expect *exp;
struct nf_conntrack_tuple *tuple;
unsigned int dataoff, start, stop, off, i;
@@ -113,23 +112,20 @@ static int amanda_help(struct sk_buff *skb,
return NF_ACCEPT;
}
- memset(&ts, 0, sizeof(ts));
start = skb_find_text(skb, dataoff, skb->len,
- search[SEARCH_CONNECT].ts, &ts);
+ search[SEARCH_CONNECT].ts);
if (start == UINT_MAX)
goto out;
start += dataoff + search[SEARCH_CONNECT].len;
- memset(&ts, 0, sizeof(ts));
stop = skb_find_text(skb, start, skb->len,
- search[SEARCH_NEWLINE].ts, &ts);
+ search[SEARCH_NEWLINE].ts);
if (stop == UINT_MAX)
goto out;
stop += start;
for (i = SEARCH_DATA; i <= SEARCH_INDEX; i++) {
- memset(&ts, 0, sizeof(ts));
- off = skb_find_text(skb, start, stop, search[i].ts, &ts);
+ off = skb_find_text(skb, start, stop, search[i].ts);
if (off == UINT_MAX)
continue;
off += start + search[i].len;
diff --git a/net/netfilter/xt_string.c b/net/netfilter/xt_string.c
index d3c48b1..0bc3460 100644
--- a/net/netfilter/xt_string.c
+++ b/net/netfilter/xt_string.c
@@ -26,14 +26,12 @@ static bool
string_mt(const struct sk_buff *skb, struct xt_action_param *par)
{
const struct xt_string_info *conf = par->matchinfo;
- struct ts_state state;
bool invert;
- memset(&state, 0, sizeof(struct ts_state));
invert = conf->u.v1.flags & XT_STRING_FLAG_INVERT;
return (skb_find_text((struct sk_buff *)skb, conf->from_offset,
- conf->to_offset, conf->config, &state)
+ conf->to_offset, conf->config)
!= UINT_MAX) ^ invert;
}
diff --git a/net/sched/em_text.c b/net/sched/em_text.c
index 15d353d..21bc53f 100644
--- a/net/sched/em_text.c
+++ b/net/sched/em_text.c
@@ -34,7 +34,6 @@ static int em_text_match(struct sk_buff *skb, struct tcf_ematch *m,
{
struct text_match *tm = EM_TEXT_PRIV(m);
int from, to;
- struct ts_state state;
from = tcf_get_base_ptr(skb, tm->from_layer) - skb->data;
from += tm->from_offset;
@@ -42,7 +41,7 @@ static int em_text_match(struct sk_buff *skb, struct tcf_ematch *m,
to = tcf_get_base_ptr(skb, tm->to_layer) - skb->data;
to += tm->to_offset;
- return skb_find_text(skb, from, to, tm->config, &state) != UINT_MAX;
+ return skb_find_text(skb, from, to, tm->config) != UINT_MAX;
}
static int em_text_change(struct tcf_proto *tp, void *data, int len,
--
1.7.9.5
^ permalink raw reply related
* [PATCH] ipvlan: Fix text that talks about ip util support
From: Mahesh Bandewar @ 2015-02-18 23:15 UTC (permalink / raw)
To: netdev, Stephen Hemminger; +Cc: Mahesh Bandewar
ipvlan was added into 3.19 release and iproute2 added support
for the same in iproute2-3.19 package.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/net/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 84673ebcf428..df51d6025a90 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -157,7 +157,7 @@ config IPVLAN
making it transparent to the connected L2 switch.
Ipvlan devices can be added using the "ip" command from the
- iproute2 package starting with the iproute2-X.Y.ZZ release:
+ iproute2 package starting with the iproute2-3.19 release:
"ip link add link <main-dev> [ NAME ] type ipvlan"
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related
* Re: Why do we use RX queue mapping for TX?
From: Eric Dumazet @ 2015-02-18 23:14 UTC (permalink / raw)
To: Cong Wang; +Cc: David S. Miller, netdev
In-Reply-To: <CAHA+R7OVHgwQBM200UZDKiw5eypuzhWj=B4E3MRkOK3yViZDCQ@mail.gmail.com>
On Wed, 2015-02-18 at 14:11 -0800, Cong Wang wrote:
> On Wed, Feb 18, 2015 at 1:18 PM, Cong Wang <cwang@twopensource.com> wrote:
> >
> > 2) This breaks the queue mapping specified by an skbedit action,
> > since for TX queues the index starts with 0 while for RX it starts with 1
> > (for some reason I don't see yet). There is at least a mismatch.
>
> So queue 0 is reserved for TX, at least for bonding queue mapping.
It seems you missed that bonding ndo_select_queue() is rather special.
^ 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