* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
From: Eric Dumazet @ 2012-07-02 9:07 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
In-Reply-To: <1341199140-17135-1-git-send-email-roy.qing.li@gmail.com>
On Mon, 2012-07-02 at 11:18 +0800, roy.qing.li@gmail.com wrote:
> From: RongQing.Li <roy.qing.li@gmail.com>
>
> opt always equals np->opts, so it is meaningless to define opt, and
> check if opt does not equal np->opts and then try to free opt.
>
> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
> ---
> net/ipv6/tcp_ipv6.c | 16 +++-------------
> 1 files changed, 3 insertions(+), 13 deletions(-)
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH] iwlegacy: print how long queue was actually stuck
From: Stanislaw Gruszka @ 2012-07-02 9:06 UTC (permalink / raw)
To: Paul Bolle; +Cc: John W. Linville, linux-wireless, netdev, linux-kernel
In-Reply-To: <1341062406.1911.76.camel@x61.thuisdomein>
[-- Attachment #1: Type: text/plain, Size: 3071 bytes --]
On Sat, Jun 30, 2012 at 03:20:06PM +0200, Paul Bolle wrote:
> On Wed, 2012-06-27 at 10:36 +0200, Paul Bolle wrote:
> > Every now and then, after resuming from suspend, the iwlegacy driver
> > prints
> > iwl4965 0000:03:00.0: Queue 2 stuck for 2000 ms.
> > iwl4965 0000:03:00.0: On demand firmware reload
> >
> > I have no idea what causes these errors. But the code currently uses
> > wd_timeout in the first error. wd_timeout will generally be set at
> > IL_DEF_WD_TIMEOUT (ie, 2000). Perhaps printing for how long the queue
> > was actually stuck can clarify the cause of these errors.
>
> 0) It's not just after resume! I just found the following lines through
> dmesg (note that it's a period that all messages in dmesg were wlan
> related, for some reason):
>
> [115840.219977] wlan0: authenticate with [...]
> [115840.228865] wlan0: send auth to [...] (try 1/3)
> [115840.429054] wlan0: send auth to [...] (try 2/3)
> [115840.630026] wlan0: send auth to [...] (try 3/3)
> [115840.831051] wlan0: authentication with [...] timed out
> [115848.022336] wlan0: authenticate with [...]
> [115848.022495] wlan0: direct probe to [...] (try 1/3)
> [115848.223052] wlan0: direct probe to [...] (try 2/3)
> [115848.424052] wlan0: direct probe to [...] (try 3/3)
> [115848.625048] wlan0: authentication with [...] timed out
> [115855.702461] wlan0: authenticate with [...]
> [115855.702623] wlan0: direct probe to [...] (try 1/3)
> [115855.903053] wlan0: direct probe to [...] (try 2/3)
> [115856.104061] wlan0: direct probe to [...] (try 3/3)
> [115856.305050] wlan0: authentication with [...] timed out
> [115863.464067] wlan0: authenticate with [...]
> [115863.464221] wlan0: direct probe to [...] (try 1/3)
> [115863.665054] wlan0: direct probe to [...] (try 2/3)
> [115863.866058] wlan0: direct probe to [...] (try 3/3)
> [115864.067051] wlan0: authentication with [...] timed out
> [115871.267216] wlan0: authenticate with [...]
> [115871.267376] wlan0: send auth to [...] (try 1/3)
> [115871.269191] wlan0: authenticated
> [115871.279459] wlan0: associate with [...] (try 1/3)
> [115871.281715] wlan0: RX AssocResp from [...] (capab=0x411 status=0 aid=2)
> [115871.281723] wlan0: associated
> [115871.457043] iwl4965 0000:03:00.0: Queue 2 stuck for 33487 ms.
> [115871.457048] iwl4965 0000:03:00.0: On demand firmware reload
For some reason we are not able to authenticate, what itself is a
problem. Maybe this is wrong key offset problem and that will
be fixed by Emmanuel patch.
Regarding "Queue 2 stuck", there is another fix in iwlwifi that
did not make to iwlegacy, which perhaps could help. If not here
then maybe on suspend.
commit 342bbf3fee2fa9a18147e74b2e3c4229a4564912
Author: Johannes Berg <johannes.berg@intel.com>
Date: Sun Mar 4 08:50:46 2012 -0800
iwlwifi: always monitor for stuck queues
I'm attaching iwlegacy version of it.
> 2) It's always "Queue 2" that's stuck. What does that queue do?
It's TX queue, probably one used for default traffic i.e. for Best
Effort category (others are Video, Voice and Background).
Stanislaw
[-- Attachment #2: iwlegacy-allways-monitor-for-stuck-queues.patch --]
[-- Type: text/plain, Size: 822 bytes --]
diff --git a/drivers/net/wireless/iwlegacy/common.c b/drivers/net/wireless/iwlegacy/common.c
index cbf2dc1..5d4807c 100644
--- a/drivers/net/wireless/iwlegacy/common.c
+++ b/drivers/net/wireless/iwlegacy/common.c
@@ -4767,14 +4767,12 @@ il_bg_watchdog(unsigned long data)
return;
/* monitor and check for other stuck queues */
- if (il_is_any_associated(il)) {
- for (cnt = 0; cnt < il->hw_params.max_txq_num; cnt++) {
- /* skip as we already checked the command queue */
- if (cnt == il->cmd_queue)
- continue;
- if (il_check_stuck_queue(il, cnt))
- return;
- }
+ for (cnt = 0; cnt < il->hw_params.max_txq_num; cnt++) {
+ /* skip as we already checked the command queue */
+ if (cnt == il->cmd_queue)
+ continue;
+ if (il_check_stuck_queue(il, cnt))
+ return;
}
mod_timer(&il->watchdog,
^ permalink raw reply related
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
From: Eric Dumazet @ 2012-07-02 8:54 UTC (permalink / raw)
To: David Miller; +Cc: roy.qing.li, netdev
In-Reply-To: <1341216816.5269.32.camel@edumazet-glaptop>
On Mon, 2012-07-02 at 10:13 +0200, Eric Dumazet wrote:
> Note that the old one is the np->opt of the listener, not the child.
>
> I dont understand how np->opt could change under us, since we have
> the listener socket locked.
>
> If np->opt can change under us, we are doomed and need to add refcounts.
Hmm, it seems net/ipv6/udp.c uses np->opt outside of the
lock_sock()/release_sock(sk) boundary.
^ permalink raw reply
* AW: AW: RFC: replace packets already in queue
From: Erdt, Ralph @ 2012-07-02 8:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Rick Jones, netdev@vger.kernel.org
In-Reply-To: <1341214310.5269.27.camel@edumazet-glaptop>
> > Even if the wireless queue is a problem (because of our setup, this
> is
> > not a problem), the network stack queue (*) is the biggest queue, and
> > a good point to optimize.
>
> Hmm, I am not convinced you have no queues on wireless.
>
> Please describe how you managed this.
>
> In fact this is the biggest problem with wireless : mac82011 framework
> aggressively pull packets from Linux packet qdisc in order to perform
> packet aggregation.
I did not talking about W-LAN (802.11). I'm talking about an property technology which is able to send over KILOMETERs (WLAN < 100m) but with VERY low bandwidth: 9600 bit (no Mega, Giga or Kilo!) (W-LAN: slowest: 1Mbit).
The devices is loosely connected to our boxes: No linux driver but a program which create an virtual network device. This just sends one packet to the devices and then waits for the acknowledgement that the packet was sent. THEN the next packet will be send. There is no further queue, because the wireless is so lame, that there is no need for that!
(BTW: the qdisc and the connector are distinct problems/programs. There is no dependency.)
> Most packets don't stay in qdisc but are sitting in wireless driver,
> unless you really flood it. If it happens, you already are in trouble.
We ARE in trouble... :-/
> So code your qdisc thing, but I am not sure you'll get much
> improvement.
It's implemented already (it's just a little source file), and it worked in the simulation. But I cannot test them on real devices ATM.
My question is: Should I do the work to create and release a kernel patch and make it perfect over the time, or is this just our internal code, which I can leave at the current state? I know our module won't be used widely (too special), but I'm sure, there are a few people out there, which would be thankful for this.
^ permalink raw reply
* Re: [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API
From: David Miller @ 2012-07-02 8:36 UTC (permalink / raw)
To: ogerlitz; +Cc: roland, yevgenyp, oren, netdev, hadarh
In-Reply-To: <4FF15BA2.5000104@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Mon, 2 Jul 2012 11:28:18 +0300
> On 7/2/2012 11:14 AM, Roland Dreier wrote:
>> What was wrong with Dave's initial suggestion of ethtool? All the
>> other steering stuff is configured that way, why not this hash?
>
> Roland, as I wrote earlier on this thread --> [...] pointed out in the
> change-log, note that this policy is **global** to the HCA, that is
> effects all the Ethernet (and down the road, also when adding support
> for IPoIB flow-steering, under a config of card with one IB port and
> one Eth port) net-devices that relate to that mlx4 device instance.
>
> In a system with (say) one card and two Ethernet ports, where for each
> port there's corresponding ethN interface, **both** mlx4_en
> net-devices use the same instance of struct mlx4_device, which means
> that if we let the user to set through ethtool the flow steering hash
> of ethN1 this will evetually change also the hash used for packets
> going to ethN2, or in other words, in mlx4 language this param belong
> to the mlx4_core module. In that respect, I was thinking on using
> sysfs as we do for the port link type and IB mtu, hope this makes
> things clearer, SB the relevant code, now with the global module param
> which can change to using per mlx4_core device sysfs.
I frankly don't care what your special unique situation is.
You cannot create chipset specific interfaces like module parms
and randomly named sysfs files as an interface to configure your
hardware.
Other chipsets will want the same thing or something similar.
So you must create a real generic interface that other chipsets
in similar situations can hook into as well.
^ permalink raw reply
* Re: [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API
From: David Miller @ 2012-07-02 8:34 UTC (permalink / raw)
To: ogerlitz; +Cc: roland, yevgenyp, oren, netdev, hadarh
In-Reply-To: <4FF153F0.8080707@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Mon, 2 Jul 2012 10:55:28 +0300
> On 7/2/2012 12:42 AM, David Miller wrote:
>> [...] Module parameters stink because every driver is going to provide
>> the knob differently, with a different name, and different
>> semantics. This creates a terrible user experience, and I will not
>> allow it.
>
> OK, so if looking on what we are left with on the table, seems that
> sysfs entry on the mlx4_core
> level (as we do for the port link type {IB, Eth} or IB port MTU) could
> be fine here, Roland, agree?
No way.
You have to create a real interface, that other vendors with similar
chips can consistently use.
^ permalink raw reply
* Re: [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API
From: Or Gerlitz @ 2012-07-02 8:28 UTC (permalink / raw)
To: Roland Dreier; +Cc: David Miller, yevgenyp, oren, netdev, hadarh
In-Reply-To: <CAG4TOxNyxP8FpzNdMZR3WMvFCQtEx_r3oLh6s7sZOcBnqN=6tA@mail.gmail.com>
On 7/2/2012 11:14 AM, Roland Dreier wrote:
> What was wrong with Dave's initial suggestion of ethtool? All the
> other steering stuff is configured that way, why not this hash?
Roland, as I wrote earlier on this thread --> [...] pointed out in the
change-log, note that this policy is **global** to the HCA, that is
effects all the Ethernet (and down the road, also when adding support
for IPoIB flow-steering, under a config of card with one IB port and one
Eth port) net-devices that relate to that mlx4 device instance.
In a system with (say) one card and two Ethernet ports, where for each
port there's corresponding ethN interface, **both** mlx4_en net-devices
use the same instance of struct mlx4_device, which means that if we let
the user to set through ethtool the flow steering hash of ethN1 this
will evetually change also the hash used for packets going to ethN2, or
in other words, in mlx4 language this param belong to the mlx4_core
module. In that respect, I was thinking on using sysfs as we do for the
port link type and IB mtu, hope this makes things clearer, SB the
relevant code, now with the global module param which can change to
using per mlx4_core device sysfs.
Or.
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -1231,6 +1236,21 @@ static int mlx4_init_hca(struct mlx4_dev *dev)
> goto err_stop_fw;
> }
>
> + priv->fs_hash_mode = mlx4_flow_steering_hash;
> +
> + switch (priv->fs_hash_mode) {
> + case MLX4_FS_L2_HASH:
> + init_hca.fs_hash_enable_bits = 0;
> + break;
> +
> + case MLX4_FS_L2_L3_L4_HASH:
> + /* Enable flow steering with
> + udp unicast and tcp unicast*/
> + init_hca.fs_hash_enable_bits =
> + MLX4_FS_UDP_UC_EN | MLX4_FS_TCP_UC_EN;
> + break;
> + }
> +
> profile = default_profile;
> if (dev->caps.steering_mode ==
> MLX4_STEERING_MODE_DEVICE_MANAGED)
^ permalink raw reply
* Re: [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API
From: Roland Dreier @ 2012-07-02 8:14 UTC (permalink / raw)
To: Or Gerlitz; +Cc: David Miller, yevgenyp, oren, netdev, hadarh
In-Reply-To: <4FF153F0.8080707@mellanox.com>
On Mon, Jul 2, 2012 at 12:55 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> On 7/2/2012 12:42 AM, David Miller wrote:
>> [...] Module parameters stink because every driver is going to provide the
>> knob differently, with a different name, and different semantics. This
>> creates a terrible user experience, and I will not allow it.
>
> OK, so if looking on what we are left with on the table, seems that sysfs
> entry on the mlx4_core
> level (as we do for the port link type {IB, Eth} or IB port MTU) could be
> fine here, Roland, agree?
What was wrong with Dave's initial suggestion of ethtool? All the
other steering stuff is configured that way, why not this hash?
- R.
^ permalink raw reply
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
From: Eric Dumazet @ 2012-07-02 8:13 UTC (permalink / raw)
To: David Miller; +Cc: roy.qing.li, netdev
In-Reply-To: <20120701.202610.12425223200611171.davem@davemloft.net>
On Sun, 2012-07-01 at 20:26 -0700, David Miller wrote:
> From: roy.qing.li@gmail.com
> Date: Mon, 2 Jul 2012 11:18:59 +0800
>
> > - if (opt) {
> > - newnp->opt = ipv6_dup_options(newsk, opt);
> > - if (opt != np->opt)
> > - sock_kfree_s(sk, opt, opt->tot_len);
>
> This is bogus, if we copy the options into a new copy in
> ipv6_dup_options() we have to free the old one or else we
> leak it.
Note that the old one is the np->opt of the listener, not the child.
I dont understand how np->opt could change under us, since we have
the listener socket locked.
If np->opt can change under us, we are doomed and need to add refcounts.
^ permalink raw reply
* Re: [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API
From: Or Gerlitz @ 2012-07-02 7:55 UTC (permalink / raw)
To: David Miller, roland; +Cc: yevgenyp, oren, netdev, hadarh
In-Reply-To: <20120701.144252.792146486861614931.davem@davemloft.net>
On 7/2/2012 12:42 AM, David Miller wrote:
> [...] Module parameters stink because every driver is going to provide
> the knob differently, with a different name, and different semantics.
> This creates a terrible user experience, and I will not allow it.
OK, so if looking on what we are left with on the table, seems that
sysfs entry on the mlx4_core
level (as we do for the port link type {IB, Eth} or IB port MTU) could
be fine here, Roland, agree?
Or.
We're talking on the /sys/bus/pci/devices entry for the card, e.g for a
card sitting
in 06:00.0 this new entry will be under /sys/bus/pci/devices/0000:06:00.0/
^ permalink raw reply
* Re: AW: RFC: replace packets already in queue
From: Eric Dumazet @ 2012-07-02 7:31 UTC (permalink / raw)
To: Erdt, Ralph; +Cc: Rick Jones, netdev@vger.kernel.org
In-Reply-To: <FB112703C4930F4ABEBB5B763F964911393795D6@MAILSERV2A.lorien.fkie.fgan.de>
On Mon, 2012-07-02 at 07:02 +0000, Erdt, Ralph wrote:
> Even if the wireless queue is a problem (because of our setup, this is
> not a problem), the network stack queue (*) is the biggest queue, and
> a good point to optimize.
Hmm, I am not convinced you have no queues on wireless.
Please describe how you managed this.
In fact this is the biggest problem with wireless : mac82011 framework
aggressively pull packets from Linux packet qdisc in order to perform
packet aggregation.
Most packets don't stay in qdisc but are sitting in wireless driver,
unless you really flood it. If it happens, you already are in trouble.
So code your qdisc thing, but I am not sure you'll get much improvement.
You would need to implement it in wireless code instead.
^ permalink raw reply
* RE: [PATCH net-next 2/2] r8169: support RTL8168G
From: hayeswang @ 2012-07-02 7:27 UTC (permalink / raw)
To: 'Francois Romieu'; +Cc: netdev, linux-kernel
In-Reply-To: <20120629135111.GB8560@electric-eye.fr.zoreil.com>
[...]
> > +static void r8168_phy_ocp_write(void __iomem *ioaddr, u32
> reg, u32 data)
> > +{
> > + int i;
> > +
> > + if (reg & 0xffff0001)
> > + BUG();
>
> The patch adds a lot of BUG(). BUG is terrible from a system
> or end user
> viewpoint.
>
> Were they only a devel helper or are they still supposed to be of use
> in the future ? If the latter applies, why ?
>
I think if the reg is invalid, there must be something wrong. The code should
have bug, and I should notice develper or someone using the code. I would
replace them with showing messages.
[...]
> > +static void rtl_ocp_write_fw(struct rtl8169_private *tp,
> struct rtl_fw *rtl_fw)
> > +{
> > + struct rtl_fw_phy_action *pa = &rtl_fw->phy_action;
> > + void __iomem *ioaddr = tp->mmio_addr;
> > + u32 predata, count;
> > + u32 base_addr;
> > + size_t index;
> > +
> > + predata = count = 0;
> > + base_addr = 0xa400;
> > +
> > + for (index = 0; index < pa->size; ) {
> > + u32 action = le32_to_cpu(pa->code[index]);
> > + u32 data = action & 0x0000ffff;
> > + u32 regno = (action & 0x0fff0000) >> 16;
> > +
> > + if (!action)
> > + break;
> > +
> > + switch(action & 0xf0000000) {
> > + case PHY_READ:
> > + predata = r8168_phy_ocp_read(ioaddr,
> > + base_addr + (regno -16) * 2);
> > + count++;
> > + index++;
> > + break;
> [duplicated code removed]
> > + case PHY_WRITE:
> > + if (regno == 0x1f)
> > + base_addr = data << 4;
> > + else
> > + r8168_phy_ocp_write(ioaddr,
> > + base_addr +
> (regno - 0x10) * 2,
> > + data);
> > + index++;
> > + break;
> [duplicated code removed]
> > + case PHY_WRITE_PREVIOUS:
> > + r8168_phy_ocp_write(ioaddr, base_addr +
> (regno -16) * 2,
> > + predata);
> > + index++;
> > + break;
>
> I can't believe that the hardware people have designed something which
> needs a different firmware write method, especially as it
> copies at lot
> of code.
>
> How did you come to the conclusion that it was not possible
> to hide this
> stuff behind r8168g_mdio_{read / write} ?
>
> I would not mind replacing the
> PHY_{READ/WRITE/WRITE_PREVIOUS} case with
> chipset specific {READ/WRITE/WRITE_PREVIOUS} methods as long as the
> semantic looks the same but going through a different
> (*write_fw) does not
> trivially seem to be the best abstraction.
>
The difficulty is how to deal with the base_addr. Although it should not happen,
the base_addr may be modify if two threads access phy at the same time. I would
replace the method by saving the base_addr with a global variable. Then, the
r8168g_mdio functions could deal with both of the firmware and phy settings.
[...]
> > +static void __devinit rtl_hw_initialize(struct rtl8169_private *tp)
> > +{
> > + switch (tp->mac_version) {
> > + case RTL_GIGA_MAC_VER_40:
> > + case RTL_GIGA_MAC_VER_41:
> > + rtl_hw_init_8168g(tp);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
>
> Why doesn't it belong to hw_start ?
>
According to the initialization process from our hardware, there are something
needed to do before reset. Maybe this considers the rebooting from the other OS
or hwardware behavior. I don't sure if it is safe, to let them belong to
hw_start.
> Is it completely unneeded if the device requires a rtl8169_hw_reset,
> resumes or such ?
>
By the information from our hardware, these are the initial settings and only
need be done once.
Best Regards,
Hayes
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Eric Dumazet @ 2012-07-02 7:13 UTC (permalink / raw)
To: Alexander Smirnov; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <1341212942.5269.10.camel@edumazet-glaptop>
On Mon, 2012-07-02 at 09:09 +0200, Eric Dumazet wrote:
> mething...
>
> - initialize frame->lock
or better, remove lock from struct lowpan_fragment
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Eric Dumazet @ 2012-07-02 7:09 UTC (permalink / raw)
To: Alexander Smirnov; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <CAJmB2rDB6McXg=O_z-M3xFa0OrB5hkTWT=ZHM975GDTPvuGjtQ@mail.gmail.com>
On Mon, 2012-07-02 at 10:53 +0400, Alexander Smirnov wrote:
> Dear Eric,
>
> >> > static DEFINE_SPINLOCK(flist_lock);
> >>
> >>
> >> and of course commit 768f7c7c121e80f4 (6lowpan: add missing
> >> spin_lock_init() ) must be reverted.
> >
> > You should validate this code with LOCKDEP
> >
> > lowpan_dellink() does a spin_lock(&flist_lock);
> > while same lock can be taken by lowpan_fragment_timer_expired() from
> > timer irq, -> deadlock.
> >
> > del_timer() probably needs a del_timer_sync() too
> >
>
> Thanks a lot for the hints!
While you are changing this code, please add in
lowpan_alloc_new_frame() :
- use netdev_alloc_skb_ip_align() instead of alloc_skb() to get some
extra headroom in case we need to forward this frame in a tunnel or
something...
- initialize frame->lock
^ permalink raw reply
* AW: RFC: replace packets already in queue
From: Erdt, Ralph @ 2012-07-02 7:02 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Rick Jones, netdev@vger.kernel.org
In-Reply-To: <1340960817.15719.6.camel@edumazet-glaptop>
Hello Eric Dumazet,
sorry for the late answer, but sometimes it's not easy here...
> Problem is : with wireless, chances are high that the old packet is not
> waiting in qdisc, but in wireless queues.
Even if the wireless queue is a problem (because of our setup, this is not a problem), the network stack queue (*) is the biggest queue, and a good point to optimize. And its impossible doing things 100%. We are happy to get 99%. This is a great improvement compared to the previous situation.
(* When I'm getting the picture in http://lartc.org/howto/lartc.qdisc.terminology.html correct, the qdisc is the outgoing queue and the last queue of the kernel. After this queue, there are only the hardware queues.)
> Anyway, adding a maxdelay to codel / fq_codel is really easy : This
> would drop packet if its sejourn time is above a given limit.
Just dropping when to old is not our goal. In fact we are avoiding blind dropping! With our replace we make sure that a packet of one class will go over the air when this packet is in line. But the information was replaced during the stay in the queue, so that the newest information got sent.
> If you want I can cook this patch.
Thank for you offer/support. But as described above, this is not a solution for us (we discussed this again in our group).
Greetings
Ralph Erdt
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Alexander Smirnov @ 2012-07-02 6:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <1341211751.5269.6.camel@edumazet-glaptop>
Dear Eric,
>> > static DEFINE_SPINLOCK(flist_lock);
>>
>>
>> and of course commit 768f7c7c121e80f4 (6lowpan: add missing
>> spin_lock_init() ) must be reverted.
>
> You should validate this code with LOCKDEP
>
> lowpan_dellink() does a spin_lock(&flist_lock);
> while same lock can be taken by lowpan_fragment_timer_expired() from
> timer irq, -> deadlock.
>
> del_timer() probably needs a del_timer_sync() too
>
Thanks a lot for the hints!
Alex
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Eric Dumazet @ 2012-07-02 6:49 UTC (permalink / raw)
To: Alexander Smirnov; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <1341211476.5269.2.camel@edumazet-glaptop>
On Mon, 2012-07-02 at 08:44 +0200, Eric Dumazet wrote:
> On Mon, 2012-07-02 at 08:37 +0200, Eric Dumazet wrote:
> > On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote:
> > > Make symbols static to avoid the following warning shown up
> > > by sparse:
> > >
> > > warning: symbol ... was not declared. Should it be static?
> > >
> > > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
> > > ---
> > > net/ieee802154/6lowpan.c | 2 +-
> > > net/mac802154/mac_cmd.c | 2 +-
> > > net/mac802154/mib.c | 2 +-
> > > 3 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> > > index cd5007f..17ad28f 100644
> > > --- a/net/ieee802154/6lowpan.c
> > > +++ b/net/ieee802154/6lowpan.c
> > > @@ -124,7 +124,7 @@ struct lowpan_fragment {
> > >
> > > static unsigned short fragment_tag;
> > > static LIST_HEAD(lowpan_fragments);
> > > -spinlock_t flist_lock;
> > > +static spinlock_t flist_lock;
> > >
> >
> > static DEFINE_SPINLOCK(flist_lock);
>
>
> and of course commit 768f7c7c121e80f4 (6lowpan: add missing
> spin_lock_init() ) must be reverted.
You should validate this code with LOCKDEP
lowpan_dellink() does a spin_lock(&flist_lock);
while same lock can be taken by lowpan_fragment_timer_expired() from
timer irq, -> deadlock.
del_timer() probably needs a del_timer_sync() too
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Eric Dumazet @ 2012-07-02 6:44 UTC (permalink / raw)
To: Alexander Smirnov; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <1341211072.5269.0.camel@edumazet-glaptop>
On Mon, 2012-07-02 at 08:37 +0200, Eric Dumazet wrote:
> On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote:
> > Make symbols static to avoid the following warning shown up
> > by sparse:
> >
> > warning: symbol ... was not declared. Should it be static?
> >
> > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
> > ---
> > net/ieee802154/6lowpan.c | 2 +-
> > net/mac802154/mac_cmd.c | 2 +-
> > net/mac802154/mib.c | 2 +-
> > 3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> > index cd5007f..17ad28f 100644
> > --- a/net/ieee802154/6lowpan.c
> > +++ b/net/ieee802154/6lowpan.c
> > @@ -124,7 +124,7 @@ struct lowpan_fragment {
> >
> > static unsigned short fragment_tag;
> > static LIST_HEAD(lowpan_fragments);
> > -spinlock_t flist_lock;
> > +static spinlock_t flist_lock;
> >
>
> static DEFINE_SPINLOCK(flist_lock);
and of course commit 768f7c7c121e80f4 (6lowpan: add missing
spin_lock_init() ) must be reverted.
^ permalink raw reply
* Re: [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Eric Dumazet @ 2012-07-02 6:37 UTC (permalink / raw)
To: Alexander Smirnov; +Cc: davem, netdev, dbaryshkov
In-Reply-To: <1341209912-6030-2-git-send-email-alex.bluesman.smirnov@gmail.com>
On Mon, 2012-07-02 at 10:18 +0400, Alexander Smirnov wrote:
> Make symbols static to avoid the following warning shown up
> by sparse:
>
> warning: symbol ... was not declared. Should it be static?
>
> Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
> ---
> net/ieee802154/6lowpan.c | 2 +-
> net/mac802154/mac_cmd.c | 2 +-
> net/mac802154/mib.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> index cd5007f..17ad28f 100644
> --- a/net/ieee802154/6lowpan.c
> +++ b/net/ieee802154/6lowpan.c
> @@ -124,7 +124,7 @@ struct lowpan_fragment {
>
> static unsigned short fragment_tag;
> static LIST_HEAD(lowpan_fragments);
> -spinlock_t flist_lock;
> +static spinlock_t flist_lock;
>
static DEFINE_SPINLOCK(flist_lock);
^ permalink raw reply
* [PATCH net-next 2/2] drivers/ieee802154/at231rf230: remove unused return status
From: Alexander Smirnov @ 2012-07-02 6:18 UTC (permalink / raw)
To: davem; +Cc: netdev, dbaryshkov, Alexander Smirnov
In-Reply-To: <1341209912-6030-1-git-send-email-alex.bluesman.smirnov@gmail.com>
Remove excessive variable used for the return status.
Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
---
drivers/ieee802154/at86rf230.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/ieee802154/at86rf230.c b/drivers/ieee802154/at86rf230.c
index 4d033d4..902e38b 100644
--- a/drivers/ieee802154/at86rf230.c
+++ b/drivers/ieee802154/at86rf230.c
@@ -585,7 +585,6 @@ err:
static int at86rf230_rx(struct at86rf230_local *lp)
{
u8 len = 128, lqi = 0;
- int rc;
struct sk_buff *skb;
skb = alloc_skb(len, GFP_KERNEL);
@@ -607,7 +606,7 @@ static int at86rf230_rx(struct at86rf230_local *lp)
ieee802154_rx_irqsafe(lp->dev, skb, lqi);
- dev_dbg(&lp->spi->dev, "READ_FBUF: %d %d %x\n", rc, len, lqi);
+ dev_dbg(&lp->spi->dev, "READ_FBUF: %d %x\n", len, lqi);
return 0;
err:
--
1.7.2.3
^ permalink raw reply related
* [PATCH net-next 1/2] ieee802154: sparse warnings: make symbols static
From: Alexander Smirnov @ 2012-07-02 6:18 UTC (permalink / raw)
To: davem; +Cc: netdev, dbaryshkov, Alexander Smirnov
In-Reply-To: <1341209912-6030-1-git-send-email-alex.bluesman.smirnov@gmail.com>
Make symbols static to avoid the following warning shown up
by sparse:
warning: symbol ... was not declared. Should it be static?
Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
---
net/ieee802154/6lowpan.c | 2 +-
net/mac802154/mac_cmd.c | 2 +-
net/mac802154/mib.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index cd5007f..17ad28f 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -124,7 +124,7 @@ struct lowpan_fragment {
static unsigned short fragment_tag;
static LIST_HEAD(lowpan_fragments);
-spinlock_t flist_lock;
+static spinlock_t flist_lock;
static inline struct
lowpan_dev_info *lowpan_dev_info(const struct net_device *dev)
diff --git a/net/mac802154/mac_cmd.c b/net/mac802154/mac_cmd.c
index 7f5403e..e6659fa 100644
--- a/net/mac802154/mac_cmd.c
+++ b/net/mac802154/mac_cmd.c
@@ -55,7 +55,7 @@ static int mac802154_mlme_start_req(struct net_device *dev,
return 0;
}
-struct wpan_phy *mac802154_get_phy(const struct net_device *dev)
+static struct wpan_phy *mac802154_get_phy(const struct net_device *dev)
{
struct mac802154_sub_if_data *priv = netdev_priv(dev);
diff --git a/net/mac802154/mib.c b/net/mac802154/mib.c
index 380829d..9cfa5f3 100644
--- a/net/mac802154/mib.c
+++ b/net/mac802154/mib.c
@@ -39,7 +39,7 @@ struct hw_addr_filt_notify_work {
unsigned long changed;
};
-struct mac802154_priv *mac802154_slave_get_priv(struct net_device *dev)
+static struct mac802154_priv *mac802154_slave_get_priv(struct net_device *dev)
{
struct mac802154_sub_if_data *priv = netdev_priv(dev);
--
1.7.2.3
^ permalink raw reply related
* [PATCH net-next 0/0] fix sparse warnings
From: Alexander Smirnov @ 2012-07-02 6:18 UTC (permalink / raw)
To: davem; +Cc: netdev, dbaryshkov
Hello David,
I've received a notification from Fengguang Wu that there are new sparse
warnings are shown up after my patch sets.
Completely forgot about 'sparse', aghhr...
These patches fixes the following sparse warnings:
- drivers/ieee802154/at86rf230.c:610:2: warning:
'rc' may be used uninitialized in this function [-Wuninitialized]
- net/ieee802154/6lowpan.c:127:12: sparse:
symbol 'flist_lock' was not declared. Should it be static?
- net/mac802154/mac_cmd.c:58:17: warning:
symbol 'mac802154_get_phy' was not declared. Should it be static?
- net/mac802154/mib.c:42:23: warning:
symbol 'mac802154_slave_get_priv' was not declared. Should it be static?
With best regards,
Alexander Smirnov
Alexander Smirnov (2):
ieee802154: sparse warnings: make symbols static
drivers/ieee802154/at231rf230: remove unused return status
drivers/ieee802154/at86rf230.c | 3 +--
net/ieee802154/6lowpan.c | 2 +-
net/mac802154/mac_cmd.c | 2 +-
net/mac802154/mib.c | 2 +-
4 files changed, 4 insertions(+), 5 deletions(-)
--
1.7.2.3
^ permalink raw reply
* [PATCH net-next] 6lowpan: revert 'reuse eth_mac_addr()'
From: Alexander Smirnov @ 2012-07-02 5:58 UTC (permalink / raw)
To: davem; +Cc: netdev, danny.kukawka, dbaryshkov, Alexander Smirnov
This reverts the commit cdf49c283e2e105da86ca575ad35b453f5ff24ea which
replaces lowpan '.ndo_set_mac_address' method by ethernet's one.
Accorind to the IEEE 802.15.4 standard, device has 8-byte length address,
so this hook loses the last 2 bytes which may rise a compatibility problems
with other IEEE 802.15.4 standard implementations.
Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com>
---
net/ieee802154/6lowpan.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index ad0c226..049d8cd 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -55,7 +55,6 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/netdevice.h>
-#include <linux/etherdevice.h>
#include <net/af_ieee802154.h>
#include <net/ieee802154.h>
#include <net/ieee802154_netdev.h>
@@ -936,6 +935,19 @@ drop:
return -EINVAL;
}
+static int lowpan_set_address(struct net_device *dev, void *p)
+{
+ struct sockaddr *sa = p;
+
+ if (netif_running(dev))
+ return -EBUSY;
+
+ /* TODO: validate addr */
+ memcpy(dev->dev_addr, sa->sa_data, dev->addr_len);
+
+ return 0;
+}
+
static int lowpan_get_mac_header_length(struct sk_buff *skb)
{
/*
@@ -1078,7 +1090,7 @@ static struct header_ops lowpan_header_ops = {
static const struct net_device_ops lowpan_netdev_ops = {
.ndo_start_xmit = lowpan_xmit,
- .ndo_set_mac_address = eth_mac_addr,
+ .ndo_set_mac_address = lowpan_set_address,
};
static struct ieee802154_mlme_ops lowpan_mlme = {
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
From: David Miller @ 2012-07-02 5:37 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
In-Reply-To: <CAJFZqHxyR0PErHV8cqLLe6eqL0fCYS9gjPRc9zpthpevsU7wkA@mail.gmail.com>
From: RongQing Li <roy.qing.li@gmail.com>
Date: Mon, 2 Jul 2012 13:23:09 +0800
> 2012/7/2 David Miller <davem@davemloft.net>:
>> From: roy.qing.li@gmail.com
>> Date: Mon, 2 Jul 2012 11:18:59 +0800
>>
>>> - if (opt) {
>>> - newnp->opt = ipv6_dup_options(newsk, opt);
>>> - if (opt != np->opt)
>>> - sock_kfree_s(sk, opt, opt->tot_len);
>>
>> This is bogus, if we copy the options into a new copy in
>> ipv6_dup_options() we have to free the old one or else we
>> leak it.
>
> Do you mean I should free newnp->opt firstly ?
>
> If I understand it right, I think we do not need to free it. the
> process is below:
>
> newsk = tcp_v4_syn_recv_sock(sk, skb, req, dst);
> ..
> newnp = inet6_sk(newsk);
> ..
> memcpy(newnp, np, sizeof(struct ipv6_pinfo));
> ..
> newnp->opt = NULL;
>
> So newnp->opt is not a effective memory.
ipv6_dup_options() allocates new memory for the options and this call
statement assigns that new pointer to np->opt.
If you do not free the old (before ipv6_dup_options()) np->opt memory
here, it is lost forever.
^ permalink raw reply
* Re: [PATCH net-next 1/2] ipv6: remove unnecessary codes in tcp_ipv6.c
From: RongQing Li @ 2012-07-02 5:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20120701.202610.12425223200611171.davem@davemloft.net>
2012/7/2 David Miller <davem@davemloft.net>:
> From: roy.qing.li@gmail.com
> Date: Mon, 2 Jul 2012 11:18:59 +0800
>
>> - if (opt) {
>> - newnp->opt = ipv6_dup_options(newsk, opt);
>> - if (opt != np->opt)
>> - sock_kfree_s(sk, opt, opt->tot_len);
>
> This is bogus, if we copy the options into a new copy in
> ipv6_dup_options() we have to free the old one or else we
> leak it.
Do you mean I should free newnp->opt firstly ?
If I understand it right, I think we do not need to free it. the
process is below:
newsk = tcp_v4_syn_recv_sock(sk, skb, req, dst);
..
newnp = inet6_sk(newsk);
..
memcpy(newnp, np, sizeof(struct ipv6_pinfo));
..
newnp->opt = NULL;
So newnp->opt is not a effective memory.
Thanks.
-Roy
^ 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