* Re: [Patch net-next] vxlan: revert "vxlan: Bypass encapsulation if the destination is local"
From: Sergei Shtylyov @ 2013-04-10 14:29 UTC (permalink / raw)
To: Sridhar Samudrala; +Cc: Cong Wang, netdev, David S. Miller
In-Reply-To: <1365530913.29336.50.camel@oc1677441337.ibm.com>
Hello.
On 09-04-2013 22:08, Sridhar Samudrala wrote:
>> From: Cong Wang <amwang@redhat.com>
>> This reverts commit 9dcc71e1fdbb7aa10d92a3d35e8a201adc84abd0.
>> It apparently breaks my vxlan tests between different namespaces.
> I haven't tried vxlan with network namespaces.
> This patch effects the following 2 code paths
> - when source and destination endpoints are on the same bridge and
> route short-circuiting is enabled. I guess you are not hitting
> this path as this is possible only if you specify 'rsc' flag when
> creating vxlan device.
> - when source and destination endpoints belonging to different vni's
> are on 2 different bridges on the same host. encap bypass is done
> in this scenario by checking if rt_flags has RTCF_LOCAL set. I think
> you must be hitting this path and the following patch should fix
> it by only doing bypass if the source and dest devices belong to
> the same net. Can you try it and see if it fixes your tests?
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 9a64715..d53d8cb 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1012,12 +1012,15 @@ static netdev_tx_t vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
> goto tx_error;
> }
>
> - /* Bypass encapsulation if the destination is local */
> - if (rt->rt_flags & RTCF_LOCAL) {
> + /* Bypass encapsulation if the destination is local and in the same
> + network namespace.
> + */
Note that the preferred multi-line comment style in the networking code is:
/* bla
* bla
*/
WBR, Sergei
^ permalink raw reply
* Re: be2net: GRO for non-inet protocols
From: Erik Hugne @ 2013-04-10 13:53 UTC (permalink / raw)
To: Bandi,Sarveshwar
Cc: Eric Dumazet, Perla, Sathya, Seetharaman, Subramanian,
Khaparde, Ajit, netdev@vger.kernel.org
In-Reply-To: <9982e479-7a8c-4520-999b-9fb59dc62689@CMEXHTCAS2.ad.emulex.com>
On Wed, Apr 10, 2013 at 01:46:53PM +0000, Bandi,Sarveshwar wrote:
> Can you check if packets are garbled when using netif_receive_skb?
> Or it happens only when napi_gro_receive is used?
Yes, all received packets are OK when netif_receive_skb is used.
It's only when napi_gro_receive is used (from the context of be_rx_compl_process,
as per Eric's patch) that i get the corrupted packets.
//E
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Oliver Neukum @ 2013-04-10 13:58 UTC (permalink / raw)
To: Dan Williams
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <1365602083.28888.40.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
On Wednesday 10 April 2013 08:54:43 Dan Williams wrote:
> > The refcounting is very good. Just don't mess around with "force"
>
> That's easy to do if the helpers aren't used for suspend/resume, which
> is what I had previously in my v2 patches until Ming suggested that I
> use the helpers there. I can go back to that approach if you'd like, it
> is a bit less complicated at the expense of sprinkling the interrupt urb
> submit/kill code around more widely.
If you introduce a third helper like "forcibly_stop_interrupt" or something,
I'll be perfectly happy. Just don't use flags which completely alter behavior.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Dan Williams @ 2013-04-10 13:54 UTC (permalink / raw)
To: Oliver Neukum
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <1542762.9EJvNHlBNo-7ztolUikljGernLeA6q8OA@public.gmane.org>
On Wed, 2013-04-10 at 15:29 +0200, Oliver Neukum wrote:
> On Wednesday 10 April 2013 08:18:57 Dan Williams wrote:
> > On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> > > On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > > > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > >
> > > > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > > > + bool force)
> > > > > > +{
> > > > > > + int ret = 0;
> > > > > > + bool submit = false;
> > > > > > +
> > > > > > + if (!dev->interrupt)
> > > > > > + return 0;
> > > > > > +
> > > > > > + mutex_lock(&dev->interrupt_mutex);
> > > > > > +
> > > > > > + if (force) {
> > > > >
> > > > > That design means that interrupt_count isn't accurate if force is used.
> > > > > That is extremely ugly.
> > > >
> > > > True; the problem here is that the URB isn't always submitted when
> > > > suspend is used. For example, in a normal driver that doesn't need the
> > > > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > > > Then if the system suspends, we can't decrement interrupt_count because
> > > > it's zero.
> > >
> > > We don't need to. You ought to understand interrupt_count as
> > > valid only while the device is not suspended.
> >
> > Ok, so at suspend we just drop the count to zero, force-kill the URB,
>
> No, at suspend() ignore interrupt_count. Just kill.
Isn't that what the code already does? The suspend handler sets force
to true, which always kills the URB at suspend time.
> > and then on resume it's not re-submitted again? That seems odd, since
>
> On resume() evaluate interrupt_count.
Because suspend/resume doesn't touch interrupt_count (due to the problem
that interrupt_count may be 0 at suspend time if the URB is not yet
submitted), we need a flag to know whether or not to increment the
count, and that's what force is there to do.
> > the usbnet driver handles submit/resubmit internally if the interface is
> > IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
> > track whether they submitted the urb or not, and then clear that on
> > suspend? Having separate behavior for when the sub-driver starts the
> > URB and when usbnet does seems inconsistent and error-prone.
> >
> > What approach would you suggest here?
>
> Religiously use interrupt_count. With one exception.
> The start/stop helpers are good. Just don't use them at suspend().
So open-code the killing at suspend()? That's what I had in a previous
patch, but Ming suggested I use the helpers instead to make things
cleaner. So I did. Should I revert to the old behavior?
> [..]
> > See my questions above. Then we'd have to have the sub-drivers
> > implement suspend/resume hooks so they'd be able to resubmit the
> > interrupt URB on resume, and the whole point of this patch was to handle
> > all that in usbnet. The sub-drivers don't know what the core driver's
> > suspend/resume count is, because dev->suspend_count isn't exposed to
> > subdrivers, and thus they don't know whether the device is actually
> > suspended or not.
> >
> > The core problem is this... the sub-driver submits the URB before
> > IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
> > Let's say later the sub-driver doesn't need its private interrupt URB
> > submission anymore, but it can't kill the URB because usbnet has
> > submitted it too. Hence the refcounting.
>
> The refcounting is very good. Just don't mess around with "force"
That's easy to do if the helpers aren't used for suspend/resume, which
is what I had previously in my v2 patches until Ming suggested that I
use the helpers there. I can go back to that approach if you'd like, it
is a bit less complicated at the expense of sprinkling the interrupt urb
submit/kill code around more widely.
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: be2net: GRO for non-inet protocols
From: Bandi,Sarveshwar @ 2013-04-10 13:46 UTC (permalink / raw)
To: Erik Hugne
Cc: Eric Dumazet, Perla, Sathya, Seetharaman, Subramanian,
Khaparde, Ajit, netdev@vger.kernel.org
In-Reply-To: <20130409163108.GA6818@eerihug-hybrid.ki.sw.ericsson.se>
Erik,
Can you check if packets are garbled when using netif_receive_skb? Or it happens only when napi_gro_receive is used?
Thanks,
Sarvesh
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Erik Hugne
Sent: Tuesday, April 09, 2013 10:01 PM
To: Bandi,Sarveshwar
Cc: Eric Dumazet; Perla, Sathya; Seetharaman, Subramanian; Khaparde, Ajit; netdev@vger.kernel.org
Subject: Re: be2net: GRO for non-inet protocols
On Tue, Apr 09, 2013 at 02:31:38PM +0000, Bandi,Sarveshwar wrote:
> Apart from this I can't see anything in the driver that can cause corruption.
Still, that's what i'm getting..
Now I've removed almost all GRO logic, and it's flushing the queue for all packets.
It looks something like this:
https://gist.github.com/Hugne/5347164
Here's the output when i start up TIPC on node A (node B is already running, sending periodic ndisc requests (msg_user=13)
[ 1656.912898] tipc: packet length=42 data_len=0 mac_len=14 [ 1656.912912] machdr: ff ff ff ff ff ff 10 60 4b b4 29 f0 88 ca 1656.912916] network header: 5b 50 00 28 00 00 a1 de 01 00 10 00 01 00 10 0a 00 00 12 67 00 03 00 01 10 60 4b b4 29 f0 00 00 [P.(...............g.....`K.)...
[ 1656.912920] network header: 00 00 00 00 00 00 00 00 00 00 08 0a 00 86 96 0d 00 05 27 da 4b b4 45 69 00 26 88 a8 ..................'.K.Ei.&..
[ 1656.912922] tipc: msg_user=13 msg_type=0
>>># Proper ndisc message from node B
[ 1657.523303] tipc: packet length=56 data_len=56 mac_len=14 [ 1657.523316] machdr: 10 60 4b b4 45 68 10 60 4b b4 29 f0 88 ca [ 1657.523320] network header: 45 00 00 34 33 a3 40 00 39 06 e8 f3 82 64 5e 8f 0a 33 3a 07 ec e5 00 16 4c 95 9a 58 47 21 b1 ed E..43.@.9....d^..3:.....L..XG!..
[ 1657.523324] network header: 80 10 01 4b c4 51 00 00 01 01 08 0a 00 86 96 51 00 05 28 1e 4b b4 45 69 00 26 88 a8 ...K.Q.........Q..(.K.Ei.&..
[ 1657.523326] tipc: msg_user=2 msg_type=1
>>># This actually looks like an IPv4 packet.. but the ethertype is TIPC..
>>># The crude "check for tipc header" passes becase it only looks at
>>>the first 3 bits #(No, this did not come from the wire)
[ 1657.904889] tipc: Garbage packet received [ 1657.904903] tipc: packet length=56 data_len=56 mac_len=14 [ 1657.904907] machdr: 10 60 4b b4 45 68 10 60 4b b4 29 f0 88 ca [ 1657.904911] network header: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 62 a2 48 b9 8e 05 ..........................b.H...
[ 1657.904914] network header: 80 18 b8 23 61 1c 00 ea ff ff 0e 08 00 00 38 00 00 00 10 60 4b b4 45 69 00 26 88 a8 ...#a.........8....`K.Ei.&..
[ 1657.904917] tipc: msg_user=0 msg_type=0
>>># I have no idea what this is.. but it's not a TIPC packet..
>>># And it did not come from the wire either
//E
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next 4/5] netback: set transport header before passing it to kernel
From: Ian Campbell @ 2013-04-10 13:33 UTC (permalink / raw)
To: Jason Wang
Cc: davem@davemloft.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, mst@redhat.com, Eric Dumazet
In-Reply-To: <1364278799-37285-5-git-send-email-jasowang@redhat.com>
On Tue, 2013-03-26 at 06:19 +0000, Jason Wang wrote:
> Currently, for the packets receives from netback, before doing header check,
> kernel just reset the transport header in netif_receive_skb() which pretends non
> l4 header. This is suboptimal for precise packet length estimation (introduced
> in 1def9238: net_sched: more precise pkt_len computation) which needs correct l4
> header for gso packets.
>
> The patch just reuse the header probed by netback for partial checksum packets
> and tries to use skb_flow_dissect() for other cases, if both fail, just pretend
> no l4 header.
>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/xen-netback/netback.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index aa28550..fc8faa7 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -39,6 +39,7 @@
> #include <linux/udp.h>
>
> #include <net/tcp.h>
> +#include <net/flow_keys.h>
>
> #include <xen/xen.h>
> #include <xen/events.h>
> @@ -1184,6 +1185,7 @@ static int checksum_setup(struct xenvif *vif, struct sk_buff *skb)
> if (th >= skb_tail_pointer(skb))
> goto out;
>
> + skb_set_transport_header(skb, 4 * iph->ihl);
> skb->csum_start = th - skb->head;
Should the use of th here (and perhaps above) be replaced with
skb_transport_header() too?
> switch (iph->protocol) {
> case IPPROTO_TCP:
> @@ -1495,6 +1497,7 @@ static void xen_netbk_tx_submit(struct xen_netbk *netbk)
>
> skb->dev = vif->dev;
> skb->protocol = eth_type_trans(skb, skb->dev);
> + skb_reset_network_header(skb);
>
> if (checksum_setup(vif, skb)) {
> netdev_dbg(vif->dev,
> @@ -1503,6 +1506,15 @@ static void xen_netbk_tx_submit(struct xen_netbk *netbk)
> continue;
> }
>
> + if (!skb_transport_header_was_set(skb)) {
> + struct flow_keys keys;
> +
> + if (skb_flow_dissect(skb, &keys))
> + skb_set_transport_header(skb, keys.thoff);
> + else
> + skb_reset_transport_header(skb);
> + }
> +
> vif->dev->stats.rx_bytes += skb->len;
> vif->dev->stats.rx_packets++;
>
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Oliver Neukum @ 2013-04-10 13:31 UTC (permalink / raw)
To: Bjørn Mork
Cc: Dan Williams, Ming Lei, Elina Pasheva, Network Development,
linux-usb, Rory Filer, Phil Sutter
In-Reply-To: <87li8q5xgi.fsf@nemi.mork.no>
On Wednesday 10 April 2013 15:25:49 Bjørn Mork wrote:
> Dan Williams <dcbw@redhat.com> writes:
>
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > + /* Only drivers that implement a status hook should call this */
> > + BUG_ON(dev->interrupt == NULL);
> >
> I still don't think there is any reason to BUG out. See for example
> http://article.gmane.org/gmane.linux.kernel/52102
On second consideration, yes, WARN_ON() would do the job.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Oliver Neukum @ 2013-04-10 13:29 UTC (permalink / raw)
To: Dan Williams
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <1365599937.28888.33.camel@dcbw.foobar.com>
On Wednesday 10 April 2013 08:18:57 Dan Williams wrote:
> On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> > On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> >
> > > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > > + bool force)
> > > > > +{
> > > > > + int ret = 0;
> > > > > + bool submit = false;
> > > > > +
> > > > > + if (!dev->interrupt)
> > > > > + return 0;
> > > > > +
> > > > > + mutex_lock(&dev->interrupt_mutex);
> > > > > +
> > > > > + if (force) {
> > > >
> > > > That design means that interrupt_count isn't accurate if force is used.
> > > > That is extremely ugly.
> > >
> > > True; the problem here is that the URB isn't always submitted when
> > > suspend is used. For example, in a normal driver that doesn't need the
> > > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > > Then if the system suspends, we can't decrement interrupt_count because
> > > it's zero.
> >
> > We don't need to. You ought to understand interrupt_count as
> > valid only while the device is not suspended.
>
> Ok, so at suspend we just drop the count to zero, force-kill the URB,
No, at suspend() ignore interrupt_count. Just kill.
> and then on resume it's not re-submitted again? That seems odd, since
On resume() evaluate interrupt_count.
> the usbnet driver handles submit/resubmit internally if the interface is
> IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
> track whether they submitted the urb or not, and then clear that on
> suspend? Having separate behavior for when the sub-driver starts the
> URB and when usbnet does seems inconsistent and error-prone.
>
> What approach would you suggest here?
Religiously use interrupt_count. With one exception.
The start/stop helpers are good. Just don't use them at suspend().
[..]
> See my questions above. Then we'd have to have the sub-drivers
> implement suspend/resume hooks so they'd be able to resubmit the
> interrupt URB on resume, and the whole point of this patch was to handle
> all that in usbnet. The sub-drivers don't know what the core driver's
> suspend/resume count is, because dev->suspend_count isn't exposed to
> subdrivers, and thus they don't know whether the device is actually
> suspended or not.
>
> The core problem is this... the sub-driver submits the URB before
> IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
> Let's say later the sub-driver doesn't need its private interrupt URB
> submission anymore, but it can't kill the URB because usbnet has
> submitted it too. Hence the refcounting.
The refcounting is very good. Just don't mess around with "force"
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Bjørn Mork @ 2013-04-10 13:25 UTC (permalink / raw)
To: Dan Williams
Cc: Ming Lei, Oliver Neukum, Elina Pasheva, Network Development,
linux-usb, Rory Filer, Phil Sutter
In-Reply-To: <1365548547.23372.2.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> +{
> + /* Only drivers that implement a status hook should call this */
> + BUG_ON(dev->interrupt == NULL);
>
I still don't think there is any reason to BUG out. See for example
http://article.gmane.org/gmane.linux.kernel/52102
> +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> +{
> + if (dev->interrupt) {
> + mutex_lock(&dev->interrupt_mutex);
> + if (!force)
> + BUG_ON(dev->interrupt_count == 0);
Same here. You can deal with this just fine. Warn once, and go on
ignoring the problem. Why kill the machine because of some minor driver
issue?
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Dan Williams @ 2013-04-10 13:18 UTC (permalink / raw)
To: Oliver Neukum
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <17293717.QHaggPmODU@linux-5eaq.site>
On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
>
> > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > + bool force)
> > > > +{
> > > > + int ret = 0;
> > > > + bool submit = false;
> > > > +
> > > > + if (!dev->interrupt)
> > > > + return 0;
> > > > +
> > > > + mutex_lock(&dev->interrupt_mutex);
> > > > +
> > > > + if (force) {
> > >
> > > That design means that interrupt_count isn't accurate if force is used.
> > > That is extremely ugly.
> >
> > True; the problem here is that the URB isn't always submitted when
> > suspend is used. For example, in a normal driver that doesn't need the
> > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > Then if the system suspends, we can't decrement interrupt_count because
> > it's zero.
>
> We don't need to. You ought to understand interrupt_count as
> valid only while the device is not suspended.
Ok, so at suspend we just drop the count to zero, force-kill the URB,
and then on resume it's not re-submitted again? That seems odd, since
the usbnet driver handles submit/resubmit internally if the interface is
IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
track whether they submitted the urb or not, and then clear that on
suspend? Having separate behavior for when the sub-driver starts the
URB and when usbnet does seems inconsistent and error-prone.
What approach would you suggest here?
> > Besides, if the system is suspended, no driver can call
> > usbnet_interrupt_start() or usbnet_interrupt_stop(), correct? Suspend
> > is a special condition here and nothing that starts/stops the urbs will
> > ever run while the system is suspended.
>
> Unfortunately there's also runtime power management.
Hmm, right.
> > > > + /* Only submit now if the URB was previously submitted */
> > > > + if (dev->interrupt_count)
> > > > + submit = true;
> > > > + } else if (++dev->interrupt_count == 1)
> > > > + submit = true;
> > > > +
> > > > + if (submit)
> > > > + ret = usb_submit_urb(dev->interrupt, mem_flags);
> > > > +
> > > > + dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > > > + dev->interrupt_count);
> > > > + mutex_unlock(&dev->interrupt_mutex);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > > > +{
> > > > + /* Only drivers that implement a status hook should call this */
> > > > + BUG_ON(dev->interrupt == NULL);
> > > > +
> > > > + if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > > > + return -EINVAL;
> > >
> > > This looks like a race condition.
> >
> > True, I'll have to fix this. But it looks like EVENT_DEV_ASLEEP is
> > protected by *either* rxq.lock (rx_submit) or txq.lock
> > (usbnet_start_xmit, usbnet_suspend, usbnet_resume). That doesn't seem
> > right, actually... shouldn't it be protected all by one lock, not two
> > different ones?
>
> Yes.
>
> > > > + return __usbnet_status_start(dev, mem_flags, false);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > > > +
> > > > +/* Kill the interrupt URB if all submitters want it killed */
> > > > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > > > +{
> > > > + if (dev->interrupt) {
> > > > + mutex_lock(&dev->interrupt_mutex);
> > > > + if (!force)
> > > > + BUG_ON(dev->interrupt_count == 0);
>
> BTW: please unify this in case somebody compiles out BUG_ON
Can do.
> > > > +
> > > > + if (force || --dev->interrupt_count == 0)
> > > > + usb_kill_urb(dev->interrupt);
> > >
> > > Why so complicated? If it may be on, kill it unconditionally.
> >
> > This function isn't only called from suspend. It's also called if the
> > sub-driver doesn't need the interrupt urb open anymore, because earlier
> > you indicated that we didn't want to unconditionally keep the URB open
> > if something didn't need it, because it's wasteful of resources.
> >
> > So for example, sierra_net will call usbnet_status_start() at driver
> > init time, and then it could call usbnet_status_stop() when it has
> > received the RESTART indication about 2 seconds after driver init, all
> > before the interface is IFF_UP and before usbnet would ever have
> > submitted the URB. However, if during that 2 seconds, somethign *does*
> > set the interface IFF_UP, you don't want sierra_net causing the urb to
> > be killed right underneath usbnet. Hence the refcounting scheme here.
> >
> > force is used only for suspend/resume specifically to ensure that the
> > URB is unconditionally killed at suspend time.
>
> It is likely to be more elegant to drop force and have an unconditional kill
> in suspend.
See my questions above. Then we'd have to have the sub-drivers
implement suspend/resume hooks so they'd be able to resubmit the
interrupt URB on resume, and the whole point of this patch was to handle
all that in usbnet. The sub-drivers don't know what the core driver's
suspend/resume count is, because dev->suspend_count isn't exposed to
subdrivers, and thus they don't know whether the device is actually
suspended or not.
The core problem is this... the sub-driver submits the URB before
IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
Let's say later the sub-driver doesn't need its private interrupt URB
submission anymore, but it can't kill the URB because usbnet has
submitted it too. Hence the refcounting.
Dan
^ permalink raw reply
* RE: [PATCH] lib, net: make isodigit public and use it
From: David Laight @ 2013-04-10 13:05 UTC (permalink / raw)
To: Denis Kirjanov, Andy Shevchenko
Cc: Jason Baron, netdev, Trond Myklebust, Andrew Morton
In-Reply-To: <CAOJe8K0H2CbM+05wpM+Gzi4aztq6R4QQ+JwS20uNJz5ck-eSYg@mail.gmail.com>
> what about #define isoctal(c) (((c) & ~7) ?
I presume you meant (((c) & ~7) == '0')
Actually the compiler will convert:
c >= '0' && c <= '7';
into
(unsigned)(c - '0') <= '7' - '0';
so it makes no significant difference.
Whether adding it to ctype.h is a good idea is a different
problem for the C standards people.
David
^ permalink raw reply
* Re: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Oliver Neukum @ 2013-04-10 13:06 UTC (permalink / raw)
To: Dan Williams
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <1365598151.28888.25.camel@dcbw.foobar.com>
On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > + bool force)
> > > +{
> > > + int ret = 0;
> > > + bool submit = false;
> > > +
> > > + if (!dev->interrupt)
> > > + return 0;
> > > +
> > > + mutex_lock(&dev->interrupt_mutex);
> > > +
> > > + if (force) {
> >
> > That design means that interrupt_count isn't accurate if force is used.
> > That is extremely ugly.
>
> True; the problem here is that the URB isn't always submitted when
> suspend is used. For example, in a normal driver that doesn't need the
> URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> Then if the system suspends, we can't decrement interrupt_count because
> it's zero.
We don't need to. You ought to understand interrupt_count as
valid only while the device is not suspended.
> Besides, if the system is suspended, no driver can call
> usbnet_interrupt_start() or usbnet_interrupt_stop(), correct? Suspend
> is a special condition here and nothing that starts/stops the urbs will
> ever run while the system is suspended.
Unfortunately there's also runtime power management.
> > > + /* Only submit now if the URB was previously submitted */
> > > + if (dev->interrupt_count)
> > > + submit = true;
> > > + } else if (++dev->interrupt_count == 1)
> > > + submit = true;
> > > +
> > > + if (submit)
> > > + ret = usb_submit_urb(dev->interrupt, mem_flags);
> > > +
> > > + dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > > + dev->interrupt_count);
> > > + mutex_unlock(&dev->interrupt_mutex);
> > > + return ret;
> > > +}
> > > +
> > > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > > +{
> > > + /* Only drivers that implement a status hook should call this */
> > > + BUG_ON(dev->interrupt == NULL);
> > > +
> > > + if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > > + return -EINVAL;
> >
> > This looks like a race condition.
>
> True, I'll have to fix this. But it looks like EVENT_DEV_ASLEEP is
> protected by *either* rxq.lock (rx_submit) or txq.lock
> (usbnet_start_xmit, usbnet_suspend, usbnet_resume). That doesn't seem
> right, actually... shouldn't it be protected all by one lock, not two
> different ones?
Yes.
> > > + return __usbnet_status_start(dev, mem_flags, false);
> > > +}
> > > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > > +
> > > +/* Kill the interrupt URB if all submitters want it killed */
> > > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > > +{
> > > + if (dev->interrupt) {
> > > + mutex_lock(&dev->interrupt_mutex);
> > > + if (!force)
> > > + BUG_ON(dev->interrupt_count == 0);
BTW: please unify this in case somebody compiles out BUG_ON
> > > +
> > > + if (force || --dev->interrupt_count == 0)
> > > + usb_kill_urb(dev->interrupt);
> >
> > Why so complicated? If it may be on, kill it unconditionally.
>
> This function isn't only called from suspend. It's also called if the
> sub-driver doesn't need the interrupt urb open anymore, because earlier
> you indicated that we didn't want to unconditionally keep the URB open
> if something didn't need it, because it's wasteful of resources.
>
> So for example, sierra_net will call usbnet_status_start() at driver
> init time, and then it could call usbnet_status_stop() when it has
> received the RESTART indication about 2 seconds after driver init, all
> before the interface is IFF_UP and before usbnet would ever have
> submitted the URB. However, if during that 2 seconds, somethign *does*
> set the interface IFF_UP, you don't want sierra_net causing the urb to
> be killed right underneath usbnet. Hence the refcounting scheme here.
>
> force is used only for suspend/resume specifically to ensure that the
> URB is unconditionally killed at suspend time.
It is likely to be more elegant to drop force and have an unconditional kill
in suspend.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] lib, net: make isodigit public and use it
From: Andy Shevchenko @ 2013-04-10 13:03 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: Jason Baron, netdev, Trond Myklebust, Andrew Morton
In-Reply-To: <CAOJe8K0H2CbM+05wpM+Gzi4aztq6R4QQ+JwS20uNJz5ck-eSYg@mail.gmail.com>
On Wed, 2013-04-10 at 16:43 +0400, Denis Kirjanov wrote:
> what about #define isoctal(c) (((c) & ~7) ?
Few reasons why not:
- the name isodigit is used already
- above name is more logical to existing isdigit & isxdigit
- macro is usually worse than inline function
- your implementation is not strict to octal digits
>
> On 4/10/13, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > There are at least two users of isodigit. Let's make it a public function
> > of
> > ctype.h.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > include/linux/ctype.h | 6 ++++++
> > lib/dynamic_debug.c | 1 -
> > net/sunrpc/cache.c | 1 -
> > 3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/ctype.h b/include/linux/ctype.h
> > index 8acfe31..653589e 100644
> > --- a/include/linux/ctype.h
> > +++ b/include/linux/ctype.h
> > @@ -61,4 +61,10 @@ static inline char _tolower(const char c)
> > return c | 0x20;
> > }
> >
> > +/* Fast check for octal digit */
> > +static inline int isodigit(const char c)
> > +{
> > + return c >= '0' && c <= '7';
> > +}
> > +
> > #endif
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 5276b99..4603245 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -281,7 +281,6 @@ static inline int parse_lineno(const char *str, unsigned
> > int *val)
> > * allow the user to express a query which matches a format
> > * containing embedded spaces.
> > */
> > -#define isodigit(c) ((c) >= '0' && (c) <= '7')
> > static char *unescape(char *str)
> > {
> > char *in = str;
> > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > index 05ea9ac..80fe5c8 100644
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -1210,7 +1210,6 @@ EXPORT_SYMBOL_GPL(sunrpc_cache_pipe_upcall);
> > * key and content are both parsed by cache
> > */
> >
> > -#define isodigit(c) (isdigit(c) && c <= '7')
> > int qword_get(char **bpp, char *dest, int bufsize)
> > {
> > /* return bytes copied, or -1 on error */
> > --
> > 1.8.2.rc0.22.gb3600c3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH net-next 04/11] tg3: Add a warning during link settings change if mgmt enabled
From: Ben Hutchings @ 2013-04-10 12:56 UTC (permalink / raw)
To: Nithin Nayak Sujir; +Cc: davem, netdev, mchan
In-Reply-To: <1365533291-5672-5-git-send-email-nsujir@broadcom.com>
On Tue, 2013-04-09 at 11:48 -0700, Nithin Nayak Sujir wrote:
> When the user executes certain ethtool commands such as -s, -A, -G, -L,
> -r a phy reset or autonegotiate is performed which results in management
> traffic being interrupted.
>
> Add a warning in these cases.
It seems entirely obvious that PHY reconfiguration will break the link
and stop all traffic until the link is re-established; this is nothing
specific to this driver. Is it more disruptive to management traffic on
these controllers? Unless it is, I think this is not a helpful warning.
Ben.
> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
> drivers/net/ethernet/broadcom/tg3.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 3f0d43f..a744998 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -2531,6 +2531,13 @@ static void tg3_carrier_off(struct tg3 *tp)
> tp->link_up = false;
> }
>
> +static void tg3_warn_mgmt_link_flap(struct tg3 *tp)
> +{
> + if (tg3_flag(tp, ENABLE_ASF))
> + netdev_warn(tp->dev,
> + "Management side-band traffic will be interrupted during phy settings change\n");
> +}
> +
> /* This will reset the tigon3 PHY if there is no valid
> * link unless the FORCE argument is non-zero.
> */
> @@ -11565,6 +11572,8 @@ static int tg3_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> tp->link_config.duplex = cmd->duplex;
> }
>
> + tg3_warn_mgmt_link_flap(tp);
> +
> if (netif_running(dev))
> tg3_setup_phy(tp, 1);
>
> @@ -11643,6 +11652,8 @@ static int tg3_nway_reset(struct net_device *dev)
> if (tp->phy_flags & TG3_PHYFLG_PHY_SERDES)
> return -EINVAL;
>
> + tg3_warn_mgmt_link_flap(tp);
> +
> if (tg3_flag(tp, USE_PHYLIB)) {
> if (!(tp->phy_flags & TG3_PHYFLG_IS_CONNECTED))
> return -EAGAIN;
> @@ -11755,6 +11766,9 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
> struct tg3 *tp = netdev_priv(dev);
> int err = 0;
>
> + if (tp->link_config.autoneg == AUTONEG_ENABLE)
> + tg3_warn_mgmt_link_flap(tp);
> +
> if (tg3_flag(tp, USE_PHYLIB)) {
> u32 newadv;
> struct phy_device *phydev;
--
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: [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active
From: Dan Williams @ 2013-04-10 12:49 UTC (permalink / raw)
To: Oliver Neukum
Cc: Ming Lei, Elina Pasheva, Network Development, linux-usb,
Rory Filer, Phil Sutter
In-Reply-To: <2328167.HJRXSHNhKT-7ztolUikljGernLeA6q8OA@public.gmane.org>
On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > Some drivers (sierra_net) need the status interrupt URB
> > active even when the device is closed, because they receive
> > custom indications from firmware. Add functions to refcount
> > the status interrupt URB submit/kill operation so that
> > sub-drivers and the generic driver don't fight over whether
> > the status interrupt URB is active or not.
> >
> > A sub-driver can call usbnet_status_start() at any time, but
> > the URB is only submitted the first time the function is
> > called. Likewise, when the sub-driver is done with the URB,
> > it calls usbnet_status_stop() but the URB is only killed when
> > all users have stopped it. The URB is still killed and
> > re-submitted for suspend/resume, as before, with the same
> > refcount it had at suspend.
> >
> > Signed-off-by: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > drivers/net/usb/usbnet.c | 79 ++++++++++++++++++++++++++++++++++++++++++----
> > include/linux/usb/usbnet.h | 5 +++
> > 2 files changed, 77 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index 51f3192..e8b363a 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -42,7 +42,7 @@
> > #include <linux/workqueue.h>
> > #include <linux/mii.h>
> > #include <linux/usb.h>
> > -#include <linux/usb/usbnet.h>
> > +#include "usbnet.h"
> > #include <linux/slab.h>
> > #include <linux/kernel.h>
> > #include <linux/pm_runtime.h>
> > @@ -252,6 +252,70 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)
> > return 0;
> > }
> >
> > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > + bool force)
> > +{
> > + int ret = 0;
> > + bool submit = false;
> > +
> > + if (!dev->interrupt)
> > + return 0;
> > +
> > + mutex_lock(&dev->interrupt_mutex);
> > +
> > + if (force) {
>
> That design means that interrupt_count isn't accurate if force is used.
> That is extremely ugly.
True; the problem here is that the URB isn't always submitted when
suspend is used. For example, in a normal driver that doesn't need the
URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
Then if the system suspends, we can't decrement interrupt_count because
it's zero.
Besides, if the system is suspended, no driver can call
usbnet_interrupt_start() or usbnet_interrupt_stop(), correct? Suspend
is a special condition here and nothing that starts/stops the urbs will
ever run while the system is suspended.
> > + /* Only submit now if the URB was previously submitted */
> > + if (dev->interrupt_count)
> > + submit = true;
> > + } else if (++dev->interrupt_count == 1)
> > + submit = true;
> > +
> > + if (submit)
> > + ret = usb_submit_urb(dev->interrupt, mem_flags);
> > +
> > + dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > + dev->interrupt_count);
> > + mutex_unlock(&dev->interrupt_mutex);
> > + return ret;
> > +}
> > +
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > + /* Only drivers that implement a status hook should call this */
> > + BUG_ON(dev->interrupt == NULL);
> > +
> > + if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > + return -EINVAL;
>
> This looks like a race condition.
True, I'll have to fix this. But it looks like EVENT_DEV_ASLEEP is
protected by *either* rxq.lock (rx_submit) or txq.lock
(usbnet_start_xmit, usbnet_suspend, usbnet_resume). That doesn't seem
right, actually... shouldn't it be protected all by one lock, not two
different ones?
> > + return __usbnet_status_start(dev, mem_flags, false);
> > +}
> > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > +
> > +/* Kill the interrupt URB if all submitters want it killed */
> > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > +{
> > + if (dev->interrupt) {
> > + mutex_lock(&dev->interrupt_mutex);
> > + if (!force)
> > + BUG_ON(dev->interrupt_count == 0);
> > +
> > + if (force || --dev->interrupt_count == 0)
> > + usb_kill_urb(dev->interrupt);
>
> Why so complicated? If it may be on, kill it unconditionally.
This function isn't only called from suspend. It's also called if the
sub-driver doesn't need the interrupt urb open anymore, because earlier
you indicated that we didn't want to unconditionally keep the URB open
if something didn't need it, because it's wasteful of resources.
So for example, sierra_net will call usbnet_status_start() at driver
init time, and then it could call usbnet_status_stop() when it has
received the RESTART indication about 2 seconds after driver init, all
before the interface is IFF_UP and before usbnet would ever have
submitted the URB. However, if during that 2 seconds, somethign *does*
set the interface IFF_UP, you don't want sierra_net causing the urb to
be killed right underneath usbnet. Hence the refcounting scheme here.
force is used only for suspend/resume specifically to ensure that the
URB is unconditionally killed at suspend time.
Dan
> > +
> > + dev_dbg(&dev->udev->dev,
> > + "decremented interrupt URB count to %d\n",
> > + dev->interrupt_count);
> > + mutex_unlock(&dev->interrupt_mutex);
> > + }
> > +}
> > +
>
> Regards
> Oliver
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] lib, net: make isodigit public and use it
From: Denis Kirjanov @ 2013-04-10 12:43 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jason Baron, netdev, Trond Myklebust, Andrew Morton
In-Reply-To: <1365585826-14824-1-git-send-email-andriy.shevchenko@linux.intel.com>
what about #define isoctal(c) (((c) & ~7) ?
On 4/10/13, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> There are at least two users of isodigit. Let's make it a public function
> of
> ctype.h.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> include/linux/ctype.h | 6 ++++++
> lib/dynamic_debug.c | 1 -
> net/sunrpc/cache.c | 1 -
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ctype.h b/include/linux/ctype.h
> index 8acfe31..653589e 100644
> --- a/include/linux/ctype.h
> +++ b/include/linux/ctype.h
> @@ -61,4 +61,10 @@ static inline char _tolower(const char c)
> return c | 0x20;
> }
>
> +/* Fast check for octal digit */
> +static inline int isodigit(const char c)
> +{
> + return c >= '0' && c <= '7';
> +}
> +
> #endif
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 5276b99..4603245 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -281,7 +281,6 @@ static inline int parse_lineno(const char *str, unsigned
> int *val)
> * allow the user to express a query which matches a format
> * containing embedded spaces.
> */
> -#define isodigit(c) ((c) >= '0' && (c) <= '7')
> static char *unescape(char *str)
> {
> char *in = str;
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 05ea9ac..80fe5c8 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1210,7 +1210,6 @@ EXPORT_SYMBOL_GPL(sunrpc_cache_pipe_upcall);
> * key and content are both parsed by cache
> */
>
> -#define isodigit(c) (isdigit(c) && c <= '7')
> int qword_get(char **bpp, char *dest, int bufsize)
> {
> /* return bytes copied, or -1 on error */
> --
> 1.8.2.rc0.22.gb3600c3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH net] bnx2x: Prevent null pointer dereference in AFEX mode
From: Yuval Mintz @ 2013-04-10 10:34 UTC (permalink / raw)
To: davem, netdev; +Cc: Yuval Mintz, Ariel Elior, Eilon Greenstein
The cnic module is responsible for initializing various bnx2x structs
via callbacks provided by the bnx2x module.
One such struct is the queue object for the FCoE queue.
If a device is working in AFEX mode and its configuration allows FCoE yet
the cnic module is not loaded, it's very likely a null pointer dereference
will occur, as the bnx2x will erroneously access the FCoE's queue object.
Prevent said access until cnic properly registers itself.
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
Hi Dave,
This small patch prevents a possible null pointer dereference in bnx2x.
Please consider applying it to `net'.
Thanks,
Yuval
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index fdfe33b..25df400 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -4959,7 +4959,7 @@ static void bnx2x_after_function_update(struct bnx2x *bp)
q);
}
- if (!NO_FCOE(bp)) {
+ if (!NO_FCOE(bp) && CNIC_ENABLED(bp)) {
fp = &bp->fp[FCOE_IDX(bp)];
queue_params.q_obj = &bnx2x_sp_obj(bp, fp).q_obj;
@@ -13450,6 +13450,7 @@ static int bnx2x_unregister_cnic(struct net_device *dev)
RCU_INIT_POINTER(bp->cnic_ops, NULL);
mutex_unlock(&bp->cnic_mutex);
synchronize_rcu();
+ bp->cnic_enabled = false;
kfree(bp->cnic_kwq);
bp->cnic_kwq = NULL;
--
1.8.1.227.g44fe835
^ permalink raw reply related
* Re: [RFC PATCH ipsec] xfrm: use the right dev to fill xdst
From: Daniel Baluta @ 2013-04-10 11:39 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, nicolas.dichtel, herbert, netdev
In-Reply-To: <20130410112900.GC21448@secunet.com>
On 04/10/2013 02:29 PM, Steffen Klassert wrote:
> On Tue, Apr 09, 2013 at 09:18:33PM +0300, Daniel Baluta wrote:
>> On Tue, Apr 9, 2013 at 8:33 PM, David Miller<davem@davemloft.net> wrote:
>>> From: Daniel Baluta<daniel.baluta@gmail.com>
>>> Date: Tue, 9 Apr 2013 20:31:30 +0300
>>>
>>>> As I mentioned earlier in this thread we are using some custom
>>>> kernel modules that create the interfaces.
>>>>
>>>> It's likely that these interfaces, for memory saving purposes, to
>>>> skip attaching ipv6 device information.
>>> So this is not an upstream bug.
>> Correct.
>>
>> With conditions mentioned by Steffen, in upstream, each net_device
>> has an in6_device assigned so we won't hit the problem.
>>
> We have an in6_device assigned in almost all of the cases, but I doubt
> it is always the right one.
>
> Let's say we want to tunnel ipv6 over ipv4. We do an ipv6 route lookup
> that returns a route with output device, say eth0. With that route,
> we do an IPsec route lookup and we get an ipv4 tunnel route with output
> device eth1.
>
> When we create the xfrm_dst in xfrm_bundle_create(), we copy the
> informations from the original ipv6 route, because we pass the new
> allocated IPsec route back to the ipv6 layer. But the device is taken
> from the ipv4 tunnel route (eth1 instead of eth0), so we pass a
> dst_entry with a ipv4 device assigned to the ipv6 layer.
>
> For that reason, xfrm6_fill_dst() complains about a NULL in6_device,
> when the mtu of the ipv4 device (passed to xfrm6_fill_dst()) is
> below IPV6_MIN_MTU.
>
> I think there is to fix something, but this needs some more research
> before we can do anything about that.
>
I will try to further look into this. If you have any scripts
that can ease IPSec tunnels setup please do share.
thanks,
daniel.
^ permalink raw reply
* Re: [RFC PATCH ipsec] xfrm: use the right dev to fill xdst
From: Steffen Klassert @ 2013-04-10 11:29 UTC (permalink / raw)
To: Daniel Baluta; +Cc: David Miller, nicolas.dichtel, herbert, netdev
In-Reply-To: <CAEnQRZBLLUviOrgQxzA=X9Q6YpSq9vf8kBtgPxeTiitS+P-oFw@mail.gmail.com>
On Tue, Apr 09, 2013 at 09:18:33PM +0300, Daniel Baluta wrote:
> On Tue, Apr 9, 2013 at 8:33 PM, David Miller <davem@davemloft.net> wrote:
> > From: Daniel Baluta <daniel.baluta@gmail.com>
> > Date: Tue, 9 Apr 2013 20:31:30 +0300
> >
> >> As I mentioned earlier in this thread we are using some custom
> >> kernel modules that create the interfaces.
> >>
> >> It's likely that these interfaces, for memory saving purposes, to
> >> skip attaching ipv6 device information.
> >
> > So this is not an upstream bug.
>
> Correct.
>
> With conditions mentioned by Steffen, in upstream, each net_device
> has an in6_device assigned so we won't hit the problem.
>
We have an in6_device assigned in almost all of the cases, but I doubt
it is always the right one.
Let's say we want to tunnel ipv6 over ipv4. We do an ipv6 route lookup
that returns a route with output device, say eth0. With that route,
we do an IPsec route lookup and we get an ipv4 tunnel route with output
device eth1.
When we create the xfrm_dst in xfrm_bundle_create(), we copy the
informations from the original ipv6 route, because we pass the new
allocated IPsec route back to the ipv6 layer. But the device is taken
from the ipv4 tunnel route (eth1 instead of eth0), so we pass a
dst_entry with a ipv4 device assigned to the ipv6 layer.
For that reason, xfrm6_fill_dst() complains about a NULL in6_device,
when the mtu of the ipv4 device (passed to xfrm6_fill_dst()) is
below IPV6_MIN_MTU.
I think there is to fix something, but this needs some more research
before we can do anything about that.
^ permalink raw reply
* [PATCH net-next] ptp: dynamic allocation of PHC char devices
From: Jiri Benc @ 2013-04-10 10:47 UTC (permalink / raw)
To: netdev; +Cc: Richard Cochran
As network adapters supporting PTP are becoming more common, machines with
many NICs suddenly have many PHCs, too. The current limit of eight /dev/ptp*
char devices (and thus, 8 network interfaces with PHC) is insufficient. Let
the ptp driver allocate the char devices dynamically.
The character devices are allocated in chunks. idr is used for tracking
minor numbers allocation; the mapping from index to pointer is not used, as
it is not needed for posix clocks.
Tested with 28 PHCs, removing and re-adding some of them.
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
drivers/ptp/ptp_clock.c | 81 ++++++++++++++++++++++++++++++++++-------------
1 files changed, 59 insertions(+), 22 deletions(-)
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 79f4bce..9d07641 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -17,7 +17,7 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
-#include <linux/bitops.h>
+#include <linux/idr.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/init.h>
@@ -32,7 +32,7 @@
#include "ptp_private.h"
#define PTP_MAX_ALARMS 4
-#define PTP_MAX_CLOCKS 8
+#define PTP_CLOCKS_CHUNK 8
#define PTP_PPS_DEFAULTS (PPS_CAPTUREASSERT | PPS_OFFSETASSERT)
#define PTP_PPS_EVENT PPS_CAPTUREASSERT
#define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
@@ -40,9 +40,10 @@
/* private globals */
static dev_t ptp_devt;
+static int ptp_devt_count;
static struct class *ptp_class;
-static DECLARE_BITMAP(ptp_clocks_map, PTP_MAX_CLOCKS);
+static struct idr ptp_clocks_map;
static DEFINE_MUTEX(ptp_clocks_mutex); /* protects 'ptp_clocks_map' */
/* time stamp event queue operations */
@@ -166,17 +167,51 @@ static struct posix_clock_operations ptp_clock_ops = {
.read = ptp_read,
};
-static void delete_ptp_clock(struct posix_clock *pc)
+static int ptp_clock_alloc_index(struct ptp_clock *ptp)
{
- struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int index, err, major = MAJOR(ptp_devt);
- mutex_destroy(&ptp->tsevq_mux);
+ mutex_lock(&ptp_clocks_mutex);
+ index = idr_alloc(&ptp_clocks_map, ptp, 0, 0, GFP_KERNEL);
+ if (index < 0)
+ goto no_idr;
+ if (index >= ptp_devt_count) {
+ err = -EBUSY;
+ if (ptp_devt_count + PTP_CLOCKS_CHUNK - 1 > MINORMASK)
+ goto no_devt;
+ err = register_chrdev_region(MKDEV(major, ptp_devt_count),
+ PTP_CLOCKS_CHUNK, "ptp");
+ if (err < 0)
+ goto no_devt;
+ ptp_devt_count += PTP_CLOCKS_CHUNK;
+ }
+ err = -EBUSY;
+ if (WARN_ON(index >= ptp_devt_count))
+ goto no_devt;
+ mutex_unlock(&ptp_clocks_mutex);
+ return index;
- /* Remove the clock from the bit map. */
+no_devt:
+ idr_remove(&ptp_clocks_map, index);
+ index = err;
+no_idr:
+ mutex_unlock(&ptp_clocks_mutex);
+ return index;
+}
+
+static void ptp_clock_free_index(int index)
+{
mutex_lock(&ptp_clocks_mutex);
- clear_bit(ptp->index, ptp_clocks_map);
+ idr_remove(&ptp_clocks_map, index);
mutex_unlock(&ptp_clocks_mutex);
+}
+
+static void delete_ptp_clock(struct posix_clock *pc)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ mutex_destroy(&ptp->tsevq_mux);
+ ptp_clock_free_index(ptp->index);
kfree(ptp);
}
@@ -191,21 +226,18 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
if (info->n_alarm > PTP_MAX_ALARMS)
return ERR_PTR(-EINVAL);
- /* Find a free clock slot and reserve it. */
- err = -EBUSY;
- mutex_lock(&ptp_clocks_mutex);
- index = find_first_zero_bit(ptp_clocks_map, PTP_MAX_CLOCKS);
- if (index < PTP_MAX_CLOCKS)
- set_bit(index, ptp_clocks_map);
- else
- goto no_slot;
-
/* Initialize a clock structure. */
err = -ENOMEM;
ptp = kzalloc(sizeof(struct ptp_clock), GFP_KERNEL);
if (ptp == NULL)
goto no_memory;
+ index = ptp_clock_alloc_index(ptp);
+ if (index < 0) {
+ err = index;
+ goto no_slot;
+ }
+
ptp->clock.ops = ptp_clock_ops;
ptp->clock.release = delete_ptp_clock;
ptp->info = info;
@@ -260,11 +292,10 @@ no_sysfs:
device_destroy(ptp_class, ptp->devid);
no_device:
mutex_destroy(&ptp->tsevq_mux);
+ ptp_clock_free_index(index);
+no_slot:
kfree(ptp);
no_memory:
- clear_bit(index, ptp_clocks_map);
-no_slot:
- mutex_unlock(&ptp_clocks_mutex);
return ERR_PTR(err);
}
EXPORT_SYMBOL(ptp_clock_register);
@@ -322,8 +353,12 @@ EXPORT_SYMBOL(ptp_clock_index);
static void __exit ptp_exit(void)
{
+ int i, major = MAJOR(ptp_devt);
+
class_destroy(ptp_class);
- unregister_chrdev_region(ptp_devt, PTP_MAX_CLOCKS);
+ for (i = 0; i < ptp_devt_count; i += PTP_CLOCKS_CHUNK)
+ unregister_chrdev_region(MKDEV(major, i), PTP_CLOCKS_CHUNK);
+ idr_destroy(&ptp_clocks_map);
}
static int __init ptp_init(void)
@@ -336,11 +371,13 @@ static int __init ptp_init(void)
return PTR_ERR(ptp_class);
}
- err = alloc_chrdev_region(&ptp_devt, 0, PTP_MAX_CLOCKS, "ptp");
+ idr_init(&ptp_clocks_map);
+ err = alloc_chrdev_region(&ptp_devt, 0, PTP_CLOCKS_CHUNK, "ptp");
if (err < 0) {
pr_err("ptp: failed to allocate device region\n");
goto no_region;
}
+ ptp_devt_count = PTP_CLOCKS_CHUNK;
ptp_class->dev_attrs = ptp_dev_attrs;
pr_info("PTP clock support registered\n");
--
1.7.6.5
^ permalink raw reply related
* [PATCH] lib, net: make isodigit public and use it
From: Andy Shevchenko @ 2013-04-10 9:23 UTC (permalink / raw)
To: Jason Baron, netdev, Trond Myklebust, Andrew Morton; +Cc: Andy Shevchenko
There are at least two users of isodigit. Let's make it a public function of
ctype.h.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/linux/ctype.h | 6 ++++++
lib/dynamic_debug.c | 1 -
net/sunrpc/cache.c | 1 -
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/linux/ctype.h b/include/linux/ctype.h
index 8acfe31..653589e 100644
--- a/include/linux/ctype.h
+++ b/include/linux/ctype.h
@@ -61,4 +61,10 @@ static inline char _tolower(const char c)
return c | 0x20;
}
+/* Fast check for octal digit */
+static inline int isodigit(const char c)
+{
+ return c >= '0' && c <= '7';
+}
+
#endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5276b99..4603245 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -281,7 +281,6 @@ static inline int parse_lineno(const char *str, unsigned int *val)
* allow the user to express a query which matches a format
* containing embedded spaces.
*/
-#define isodigit(c) ((c) >= '0' && (c) <= '7')
static char *unescape(char *str)
{
char *in = str;
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 05ea9ac..80fe5c8 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1210,7 +1210,6 @@ EXPORT_SYMBOL_GPL(sunrpc_cache_pipe_upcall);
* key and content are both parsed by cache
*/
-#define isodigit(c) (isdigit(c) && c <= '7')
int qword_get(char **bpp, char *dest, int bufsize)
{
/* return bytes copied, or -1 on error */
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related
* [net] e100: Add dma mapping error check
From: Jeff Kirsher @ 2013-04-10 9:19 UTC (permalink / raw)
To: davem
Cc: Neil Horman, netdev, gospo, sassmann, Josh Boyer, e1000-devel,
Jeff Kirsher
From: Neil Horman <nhorman@tuxdriver.com>
e100 uses pci_map_single, but fails to check for a dma mapping error after its
use, resulting in a stack trace:
[ 46.656594] ------------[ cut here ]------------
[ 46.657004] WARNING: at lib/dma-debug.c:933 check_unmap+0x47b/0x950()
[ 46.657004] Hardware name: To Be Filled By O.E.M.
[ 46.657004] e100 0000:00:0e.0: DMA-API: device driver failed to check map
error[device address=0x000000007a4540fa] [size=90 bytes] [mapped as single]
[ 46.657004] Modules linked in:
[ 46.657004] w83627hf hwmon_vid snd_via82xx ppdev snd_ac97_codec ac97_bus
snd_seq snd_pcm snd_mpu401 snd_mpu401_uart ns558 snd_rawmidi gameport parport_pc
e100 snd_seq_device parport snd_page_alloc snd_timer snd soundcore skge shpchp
k8temp mii edac_core i2c_viapro edac_mce_amd nfsd auth_rpcgss nfs_acl lockd
sunrpc binfmt_misc uinput ata_generic pata_acpi radeon i2c_algo_bit
drm_kms_helper ttm firewire_ohci drm firewire_core pata_via sata_via i2c_core
sata_promise crc_itu_t
[ 46.657004] Pid: 792, comm: ip Not tainted 3.8.0-0.rc6.git0.1.fc19.x86_64 #1
[ 46.657004] Call Trace:
[ 46.657004] <IRQ> [<ffffffff81065ed0>] warn_slowpath_common+0x70/0xa0
[ 46.657004] [<ffffffff81065f4c>] warn_slowpath_fmt+0x4c/0x50
[ 46.657004] [<ffffffff81364cfb>] check_unmap+0x47b/0x950
[ 46.657004] [<ffffffff8136522f>] debug_dma_unmap_page+0x5f/0x70
[ 46.657004] [<ffffffffa030f0f0>] ? e100_tx_clean+0x30/0x210 [e100]
[ 46.657004] [<ffffffffa030f1a8>] e100_tx_clean+0xe8/0x210 [e100]
[ 46.657004] [<ffffffffa030fc6f>] e100_poll+0x56f/0x6c0 [e100]
[ 46.657004] [<ffffffff8159dce1>] ? net_rx_action+0xa1/0x370
[ 46.657004] [<ffffffff8159ddb2>] net_rx_action+0x172/0x370
[ 46.657004] [<ffffffff810703bf>] __do_softirq+0xef/0x3d0
[ 46.657004] [<ffffffff816e4ebc>] call_softirq+0x1c/0x30
[ 46.657004] [<ffffffff8101c485>] do_softirq+0x85/0xc0
[ 46.657004] [<ffffffff81070885>] irq_exit+0xd5/0xe0
[ 46.657004] [<ffffffff816e5756>] do_IRQ+0x56/0xc0
[ 46.657004] [<ffffffff816dacb2>] common_interrupt+0x72/0x72
[ 46.657004] <EOI> [<ffffffff816da1eb>] ?
_raw_spin_unlock_irqrestore+0x3b/0x70
[ 46.657004] [<ffffffff816d124d>] __slab_free+0x58/0x38b
[ 46.657004] [<ffffffff81214424>] ? fsnotify_clear_marks_by_inode+0x34/0x120
[ 46.657004] [<ffffffff811b0417>] ? kmem_cache_free+0x97/0x320
[ 46.657004] [<ffffffff8157fc14>] ? sock_destroy_inode+0x34/0x40
[ 46.657004] [<ffffffff8157fc14>] ? sock_destroy_inode+0x34/0x40
[ 46.657004] [<ffffffff811b0692>] kmem_cache_free+0x312/0x320
[ 46.657004] [<ffffffff8157fc14>] sock_destroy_inode+0x34/0x40
[ 46.657004] [<ffffffff811e8c28>] destroy_inode+0x38/0x60
[ 46.657004] [<ffffffff811e8d5e>] evict+0x10e/0x1a0
[ 46.657004] [<ffffffff811e9605>] iput+0xf5/0x180
[ 46.657004] [<ffffffff811e4338>] dput+0x248/0x310
[ 46.657004] [<ffffffff811ce0e1>] __fput+0x171/0x240
[ 46.657004] [<ffffffff811ce26e>] ____fput+0xe/0x10
[ 46.657004] [<ffffffff8108d54c>] task_work_run+0xac/0xe0
[ 46.657004] [<ffffffff8106c6ed>] do_exit+0x26d/0xc30
[ 46.657004] [<ffffffff8109eccc>] ? finish_task_switch+0x7c/0x120
[ 46.657004] [<ffffffff816dad58>] ? retint_swapgs+0x13/0x1b
[ 46.657004] [<ffffffff8106d139>] do_group_exit+0x49/0xc0
[ 46.657004] [<ffffffff8106d1c4>] sys_exit_group+0x14/0x20
[ 46.657004] [<ffffffff816e3b19>] system_call_fastpath+0x16/0x1b
[ 46.657004] ---[ end trace 4468c44e2156e7d1 ]---
[ 46.657004] Mapped at:
[ 46.657004] [<ffffffff813663d1>] debug_dma_map_page+0x91/0x140
[ 46.657004] [<ffffffffa030e8eb>] e100_xmit_prepare+0x12b/0x1c0 [e100]
[ 46.657004] [<ffffffffa030c924>] e100_exec_cb+0x84/0x140 [e100]
[ 46.657004] [<ffffffffa030e56a>] e100_xmit_frame+0x3a/0x190 [e100]
[ 46.657004] [<ffffffff8159ee89>] dev_hard_start_xmit+0x259/0x6c0
Easy fix, modify the cb paramter to e100_exec_cb to return an error, and do the
dma_mapping_error check in the obvious place
This was reported previously here:
http://article.gmane.org/gmane.linux.network/257893
But nobody stepped up and fixed it.
CC: Josh Boyer <jwboyer@redhat.com>
CC: e1000-devel@lists.sourceforge.net
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Michal Jaegermann <michal@harddata.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e100.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index ec800b0..d2bea3f 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -870,7 +870,7 @@ err_unlock:
}
static int e100_exec_cb(struct nic *nic, struct sk_buff *skb,
- void (*cb_prepare)(struct nic *, struct cb *, struct sk_buff *))
+ int (*cb_prepare)(struct nic *, struct cb *, struct sk_buff *))
{
struct cb *cb;
unsigned long flags;
@@ -888,10 +888,13 @@ static int e100_exec_cb(struct nic *nic, struct sk_buff *skb,
nic->cbs_avail--;
cb->skb = skb;
+ err = cb_prepare(nic, cb, skb);
+ if (err)
+ goto err_unlock;
+
if (unlikely(!nic->cbs_avail))
err = -ENOSPC;
- cb_prepare(nic, cb, skb);
/* Order is important otherwise we'll be in a race with h/w:
* set S-bit in current first, then clear S-bit in previous. */
@@ -1091,7 +1094,7 @@ static void e100_get_defaults(struct nic *nic)
nic->mii.mdio_write = mdio_write;
}
-static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
+static int e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
{
struct config *config = &cb->u.config;
u8 *c = (u8 *)config;
@@ -1181,6 +1184,7 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
netif_printk(nic, hw, KERN_DEBUG, nic->netdev,
"[16-23]=%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X\n",
c[16], c[17], c[18], c[19], c[20], c[21], c[22], c[23]);
+ return 0;
}
/*************************************************************************
@@ -1331,7 +1335,7 @@ static const struct firmware *e100_request_firmware(struct nic *nic)
return fw;
}
-static void e100_setup_ucode(struct nic *nic, struct cb *cb,
+static int e100_setup_ucode(struct nic *nic, struct cb *cb,
struct sk_buff *skb)
{
const struct firmware *fw = (void *)skb;
@@ -1358,6 +1362,7 @@ static void e100_setup_ucode(struct nic *nic, struct cb *cb,
cb->u.ucode[min_size] |= cpu_to_le32((BUNDLESMALL) ? 0xFFFF : 0xFF80);
cb->command = cpu_to_le16(cb_ucode | cb_el);
+ return 0;
}
static inline int e100_load_ucode_wait(struct nic *nic)
@@ -1400,18 +1405,20 @@ static inline int e100_load_ucode_wait(struct nic *nic)
return err;
}
-static void e100_setup_iaaddr(struct nic *nic, struct cb *cb,
+static int e100_setup_iaaddr(struct nic *nic, struct cb *cb,
struct sk_buff *skb)
{
cb->command = cpu_to_le16(cb_iaaddr);
memcpy(cb->u.iaaddr, nic->netdev->dev_addr, ETH_ALEN);
+ return 0;
}
-static void e100_dump(struct nic *nic, struct cb *cb, struct sk_buff *skb)
+static int e100_dump(struct nic *nic, struct cb *cb, struct sk_buff *skb)
{
cb->command = cpu_to_le16(cb_dump);
cb->u.dump_buffer_addr = cpu_to_le32(nic->dma_addr +
offsetof(struct mem, dump_buf));
+ return 0;
}
static int e100_phy_check_without_mii(struct nic *nic)
@@ -1581,7 +1588,7 @@ static int e100_hw_init(struct nic *nic)
return 0;
}
-static void e100_multi(struct nic *nic, struct cb *cb, struct sk_buff *skb)
+static int e100_multi(struct nic *nic, struct cb *cb, struct sk_buff *skb)
{
struct net_device *netdev = nic->netdev;
struct netdev_hw_addr *ha;
@@ -1596,6 +1603,7 @@ static void e100_multi(struct nic *nic, struct cb *cb, struct sk_buff *skb)
memcpy(&cb->u.multi.addr[i++ * ETH_ALEN], &ha->addr,
ETH_ALEN);
}
+ return 0;
}
static void e100_set_multicast_list(struct net_device *netdev)
@@ -1756,11 +1764,18 @@ static void e100_watchdog(unsigned long data)
round_jiffies(jiffies + E100_WATCHDOG_PERIOD));
}
-static void e100_xmit_prepare(struct nic *nic, struct cb *cb,
+static int e100_xmit_prepare(struct nic *nic, struct cb *cb,
struct sk_buff *skb)
{
+ dma_addr_t dma_addr;
cb->command = nic->tx_command;
+ dma_addr = pci_map_single(nic->pdev,
+ skb->data, skb->len, PCI_DMA_TODEVICE);
+ /* If we can't map the skb, have the upper layer try later */
+ if (pci_dma_mapping_error(nic->pdev, dma_addr))
+ return -ENOMEM;
+
/*
* Use the last 4 bytes of the SKB payload packet as the CRC, used for
* testing, ie sending frames with bad CRC.
@@ -1777,11 +1792,10 @@ static void e100_xmit_prepare(struct nic *nic, struct cb *cb,
cb->u.tcb.tcb_byte_count = 0;
cb->u.tcb.threshold = nic->tx_threshold;
cb->u.tcb.tbd_count = 1;
- cb->u.tcb.tbd.buf_addr = cpu_to_le32(pci_map_single(nic->pdev,
- skb->data, skb->len, PCI_DMA_TODEVICE));
- /* check for mapping failure? */
+ cb->u.tcb.tbd.buf_addr = cpu_to_le32(dma_addr);
cb->u.tcb.tbd.size = cpu_to_le16(skb->len);
skb_tx_timestamp(skb);
+ return 0;
}
static netdev_tx_t e100_xmit_frame(struct sk_buff *skb,
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH 1/4] Add packet recirculation
From: Simon Horman @ 2013-04-10 9:16 UTC (permalink / raw)
To: Jesse Gross
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, netdev, Ravi K,
Isaku Yamahata
In-Reply-To: <CAEP_g=-baZGttBowXKGdgKafxp9fkUrQ=44y562ZzfnOL-XaQg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Apr 09, 2013 at 08:44:02AM -0700, Jesse Gross wrote:
> On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote:
> >> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> >> > diff --git a/datapath/actions.c b/datapath/actions.c
> >> > index e9634fe..7b0f022 100644
> >> > --- a/datapath/actions.c
> >> > +++ b/datapath/actions.c
> >> > @@ -617,6 +617,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >> > case OVS_ACTION_ATTR_SAMPLE:
> >> > err = sample(dp, skb, a);
> >> > break;
> >> > +
> >> > + case OVS_ACTION_ATTR_RECIRCULATE:
> >> > + return 1;
> >>
> >> I think that if we've had a previous output action with the port
> >> stored in prev_port then this will cause the packet to not actually be
> >> output.
> >
> > I'm not so sure.
> >
> > I see something like this occurring:
> >
> > 1. Iteration of for loop for output action
> >
> > switch (nla_type(a)) {
> > case OVS_ACTION_ATTR_OUTPUT:
> > prev_port = nla_get_u32(a);
> > break;
> > ...
> > }
> >
> > 2. Iteration of of for loop for next action, lets say its is recirculate
> >
> > i. Output packet
> >
> > if (prev_port != -1) {
> > do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
> > prev_port = -1;
> > }
> >
> > ii. Return due to recirculate
> > switch (nla_type(a)) {
> > ...
> > case OVS_ACTION_ATTR_RECIRCULATE:
> > return 1;
> > }
> >
> >
> > Am I missing something?
>
> Sorry, you're right.
>
> >> > @@ -901,6 +913,9 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
> >> > skip_copy = true;
> >> > break;
> >> >
> >> > + case OVS_ACTION_ATTR_RECIRCULATE:
> >> > + break;
> >>
> >> I think we might want to jump out the loop here to better model how
> >> the actions are actually executed.
> >
> > Sure, perhaps something like this?
> >
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index ab39dd7..721a52c 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -914,7 +914,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
> > break;
> >
> > case OVS_ACTION_ATTR_RECIRCULATE:
> > - break;
> > + goto out;
> >
> > default:
> > return -EINVAL;
> > @@ -926,6 +926,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr,
> > }
> > }
> >
> > +out:
> > if (rem > 0)
> > return -EINVAL;
>
> Since this function is now both validating and copying I think this
> will result in the recirculate action not being copied.
Thanks, I'll look into that.
> >> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> > index 47830c1..5129da1 100644
> >> > --- a/ofproto/ofproto-dpif.c
> >> > +++ b/ofproto/ofproto-dpif.c
> >>
> >> I'm still working on more detailed comments for this. However, I'm
> >> concerned about whether the behavior for revalidation and stats is
> >> correct.
> >
> > I am a little concerned about that too.
> > Perhaps Ben could look over it?
>
> To rephrase, there are problems in both of those areas. Validation in
> particular I don't think handles resubmitted facets and I believe that
> stats on rules will be the sum of all resubmitted passes.
Some questions:
By resubmitted do you mean recirculated?
What is the stats behaviour that you would like?
With regards to validation, I assume the area of concern
is around facet_revalidate(). I will look into that.
> Both of these will likely significantly affect the data structures, so
> please look into this before we go further.
Sure. I was not planning to push (much) further until this series
is reviewed properly.
> In general, I'd also like
> to see patches that are standalone without needing follow on patches
> to fix known problems (for example, the recirculation ID patches or
> MPLS GSO) unless there is a good reason.
Thanks, I understand. I'll try and structure my patches accordingly.
^ permalink raw reply
* Re: [BUG] Fatal exception in interrupt - nf_nat_cleanup_conntrack during IPv6 tests
From: Florian Westphal @ 2013-04-10 9:01 UTC (permalink / raw)
To: CAI Qian; +Cc: netdev, stable, LKML, netfilter-devel
In-Reply-To: <1089062823.1991307.1365563878922.JavaMail.root@redhat.com>
CAI Qian <caiqian@redhat.com> wrote:
[ CC'd nf-devel ]
> Just hit this very often during IPv6 tests in both the latest stable
> and mainline kernel.
>
> [ 3597.206166] Modules linked in:
[..]
> nf_nat_ipv4(F-)
[..]
> [ 3597.804861] RIP: 0010:[<ffffffffa03227f2>] [<ffffffffa03227f2>] nf_nat_cleanup_conntrack+0x42/0x70 [nf_nat]
> [ 3597.855207] RSP: 0018:ffff880202c63d40 EFLAGS: 00010246
> [ 3597.881350] RAX: 0000000000000000 RBX: ffff8801ac7bec28 RCX: ffff8801d0eedbe0
> [ 3597.917226] RDX: dead000000200200 RSI: 0000000000000011 RDI: ffffffffa03265b8
[..]
> [ 3598.421036] <IRQ>
> [ 3598.430467] [<ffffffffa0305bb4>] __nf_ct_ext_destroy+0x44/0x60 [nf_conntrack]
> [ 3598.499191] [<ffffffffa02fd3fe>] nf_conntrack_free+0x2e/0x70 [nf_conntrack]
> [ 3598.534121] [<ffffffffa02febed>] destroy_conntrack+0xbd/0x110 [nf_conntrack]
> [ 3598.569981] [<ffffffff81532187>] nf_conntrack_destroy+0x17/0x20
> [ 3598.599579] [<ffffffffa02fe77c>] death_by_timeout+0xdc/0x1b0 [nf_conntrack]
[..]
> [ 3599.241868] Code: 83 ec 08 0f b6 58 11 84 db 74 43 48 01 c3 48 83 7b 20 00 74 39 48 c7 c7 b8 65 32 a0 e8 98 fc 2e e1 48 8b 03 48 8b 53 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 ba 00 02 20 00 00 00 ad de 48 c7
> [ 3599.337037] RIP [<ffffffffa03227f2>] nf_nat_cleanup_conntrack+0x42/0x70 [nf_nat]
Looks like we tried to remove bysource hash twice (rdx is
LIST_POISON_2).
I wonder if this would explain it:
static void nf_nat_l4proto_clean(u8 l3proto, u8 l4proto)
{
[..]
/* Step 1 - remove from bysource hash */
clean.hash = true;
for_each_net(net)
nf_ct_iterate_cleanup(net, nf_nat_proto_clean, &clean);
A nfct->timer fires and a conntrack is free'd before step 2 memsets the
nat extension. In that case, we would try to delete nat->bysource
again?
^ permalink raw reply
* [PATCH iproute2] vxlan: Allow setting unicast address to the group address
From: Atzm Watanabe @ 2013-04-10 8:52 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger
This patch allows setting unicast address to the VXLAN group address.
It allows that VXLAN can be used as peer-to-peer tunnel without
multicast.
Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
---
ip/iplink_vxlan.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 1025326..cfe324c 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -66,9 +66,6 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
} else if (!matches(*argv, "group")) {
NEXT_ARG();
gaddr = get_addr32(*argv);
-
- if (!IN_MULTICAST(ntohl(gaddr)))
- invarg("invald group address", *argv);
} else if (!matches(*argv, "local")) {
NEXT_ARG();
if (strcmp(*argv, "any"))
--
1.8.1.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox