* Re: [PATCH] sh_eth: r8a7790: Handle the RFE (Receive FIFO overflow Error) interrupt
From: Sergei Shtylyov @ 2013-09-12 19:45 UTC (permalink / raw)
To: Laurent Pinchart, Simon Horman; +Cc: netdev, linux-sh, Magnus Damm
In-Reply-To: <4760883.hI1JN8gopV@avalon>
Hello.
On 07/30/2013 06:20 PM, Laurent Pinchart wrote:
>>> The RFE interrupt is enabled for the r8a7790 but isn't handled,
>>> resulting in the interrupts core noticing unhandled interrupts, and
>>> eventually disabling the ethernet IRQ.
>>> Fix it by adding RFE to the bitmask of error interrupts to be handled
>>> for r8a7790.
>> So, Simon hasn't synced his patch to my late bug fix in 3.10... Did this
>> patch help you with your NFS boot issue, Laurent?
> Yes, it fixes the "disabling interrupt, nobody cared" problem. I still have
> intermittent NFS issues, but at least I can now boot.
Looks like the reason for them is the same I had to fix up for the BOCK-W:
the bouncing LINK signal. The PHY used is the same as on BOCK-W, however, its
LED seems to be configured differently: for LINK and ACTIVE LEDs, this is
non-default PHY configuration which AFAIK gets reset to default when the PHY
gets reset. What I saw when I added orintk() for the interrupt enable/mask
tracing was the LINK signal behaving normally at first but after some time ECI
(M-Port in the manuals) interrupts started to behave the way well known from
BOCK-W, i.e. bouncing on and off after each packet; I was also getting endless
RFE (Rx FIFO overflow) interrupts and NFS was unable to mount at all in this
traced mode. The fix was the same as for BOCK-W: to set 'no_ether_link' field
of the platfrom data to 1. After that I've no more seen NFS timeouts and RFE
interrupts. I'm going to continue testing but thought I let everybody know of
my currct findings and the remedy for the NFS issue.
WBR, Sergei
^ permalink raw reply
* Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called
From: Thomas Gleixner @ 2013-09-12 19:37 UTC (permalink / raw)
To: David Miller
Cc: herbert, fan.du, steffen.klassert, dborkman, linux-kernel, netdev
In-Reply-To: <20130912.133252.425268707009916773.davem@davemloft.net>
On Thu, 12 Sep 2013, David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)
>
> > So what about going back to timer_list timers and simply utilize
> > register_pm_notifier(), which will tell you that the system resumed?
>
> The thing to understand is that there are two timeouts for an IPSEC
> rule, a soft and a hard timeout.
>
> There is a gap between these two exactly so that we can negotiate a
> new encapsulation with the IPSEC gateway before communication ceases
> to be possible over the IPSEC protected path.
>
> So the idea is that the soft timeout triggers the re-negotiation,
> and after a hard timeout the IPSEC path is no longer usable and
> all communication will fail.
>
> Simply triggering a re-negoation after every suspend/resume makes
> no sense at all. Spurious re-negotiations are undesirable.
>
> What we want are real timers. We want that rather than a "we
> suspended so just assume all timers expired" event which is not very
> useful for this kind of application.
Your argumentation makes no sense at all. Where is the difference
between the "real timer" plus a clock was set notification and a timer
list timer and a resume notification?
In both cases you need to walk through the timers and reevaluate
them. Just in the clock was set notification case you need to deal
with NTP/settimeofday/PPS and whatever cases which are completely
irrelevant to the life time management.
So what's wrong with:
now = get_seconds();
x->timeout = now + x->soft_timeout;
x->timeout_active = SOFT;
start_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));
In the timer handler:
switch (x->timeout_active) {
case SOFT:
trigger_renegotiation();
hts = x->hard_timeout - x->soft_timeout;
x->timeout += hts;
x->timeout_active = HARD;
start_timer(x->timer, jiffies + sec_to_jiffies(hts));
break;
case HARD:
stop_connection();
break:
}
If the negotiation succeeds:
now = get_seconds();
x->t_timeout = now + x->soft_timeout;
x->timeout_active = SOFT;
mod_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));
Now in the resume notification you walk the active timers and do for
each timer:
now = get_seconds();
switch (x->timeout_active) {
case SOFT:
if (now >= x->timeout) {
hts = x->hard_timeout - x->soft_timeout;
x->timeout += hts;
if (now >= x->timeout) {
del_timer(x->timeout);
stop_connection();
break:
}
trigger_renegotiation();
x->timeout_active = HARD;
mod_timer(x->timer, jiffies + sec_to_jiffies(hts));
break;
}
delta = x->timeout - now;
mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
break;
case HARD:
if (now >= x->timeout) {
del_timer(x->timeout);
stop_connection();
}
delta = x->timeout - now;
mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
break;
}
That's what you have to do with a clock was set notification as
well. And what's worse is that you need to figure out whether the
clock change was due to a suspend/resume cycle or just because
NTP/settimeofday/PPS or whatever decided to fiddle with the wall
time. And you need to do that to avoid the spurious renegotiations
which you are afraid of.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: David Miller @ 2013-09-12 19:18 UTC (permalink / raw)
To: honkiko; +Cc: vyasevic, eric.dumazet, netdev, zhiguohong
In-Reply-To: <1379001428-2727-1-git-send-email-zhiguohong@tencent.com>
From: Hong Zhiguo <honkiko@gmail.com>
Date: Thu, 12 Sep 2013 23:57:08 +0800
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
You did not even compile test this patch.
This is a good way to make people not want to review your work at all,
because if you don't care enough to even compile test your patch why
should other people care enough to look at it?
^ permalink raw reply
* Re: [ovs-dev] [PATCH v2.39 0/7] MPLS actions and matches
From: Ben Pfaff @ 2013-09-12 19:06 UTC (permalink / raw)
To: Simon Horman, Jesse Gross; +Cc: dev, netdev, Isaku Yamahata, Ravi K
In-Reply-To: <1378711207-29890-1-git-send-email-horms@verge.net.au>
I've totally lost track of the status of this patch series. I assume it
needs Jesse's review. Jesse, if I'm wrong about that, let me know and
I'll take a pass at it.
Thanks,
Ben.
^ permalink raw reply
* Re: [PATCH 04/11] ipv6: move route updating for redirect to ndisc layer
From: David Miller @ 2013-09-12 19:03 UTC (permalink / raw)
To: duanj.fnst; +Cc: netdev, hannes
In-Reply-To: <52319B99.6090303@cn.fujitsu.com>
It is completely pointless to use the same exact subject line for
several patches in a patch set.
You absolutely must choose unique subject line text for each patch so
that someone skimming the shortlog can tell what might be unique to
each patch.
^ permalink raw reply
* Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called
From: David Miller @ 2013-09-12 17:32 UTC (permalink / raw)
To: tglx; +Cc: herbert, fan.du, steffen.klassert, dborkman, linux-kernel, netdev
In-Reply-To: <alpine.DEB.2.02.1309121636010.4089@ionos.tec.linutronix.de>
From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)
> So what about going back to timer_list timers and simply utilize
> register_pm_notifier(), which will tell you that the system resumed?
The thing to understand is that there are two timeouts for an IPSEC
rule, a soft and a hard timeout.
There is a gap between these two exactly so that we can negotiate a
new encapsulation with the IPSEC gateway before communication ceases
to be possible over the IPSEC protected path.
So the idea is that the soft timeout triggers the re-negotiation,
and after a hard timeout the IPSEC path is no longer usable and
all communication will fail.
Simply triggering a re-negoation after every suspend/resume makes
no sense at all. Spurious re-negotiations are undesirable.
What we want are real timers. We want that rather than a "we
suspended so just assume all timers expired" event which is not very
useful for this kind of application.
^ permalink raw reply
* Re: [PATCH 20/52] net: fealnx: remove unnecessary pci_set_drvdata()
From: Sergei Shtylyov @ 2013-09-12 17:11 UTC (permalink / raw)
To: Jingoo Han; +Cc: 'David S. Miller', netdev
In-Reply-To: <000e01ceaf4c$90cde130$b269a390$%han@samsung.com>
Hello.
On 09/12/2013 04:11 AM, Jingoo Han wrote:
>>> The driver core clears the driver data to NULL after device_release
>>> or on probe failure. Thus, it is not needed to manually clear the
>>> device driver data to NULL.
>>> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
>>> ---
>>> drivers/net/ethernet/fealnx.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
>>> index c706b7a..99194d1 100644
>>> --- a/drivers/net/ethernet/fealnx.c
>>> +++ b/drivers/net/ethernet/fealnx.c
>>> @@ -699,9 +699,9 @@ static void fealnx_remove_one(struct pci_dev *pdev)
>>> pci_iounmap(pdev, np->mem);
>>> free_netdev(dev);
>>> pci_release_regions(pdev);
>>> - pci_set_drvdata(pdev, NULL);
>>> - } else
>>> + } else {
>>> printk(KERN_ERR "fealnx: remove for unknown device\n");
>>> + }
>> No "drove-by" coding style fixes, please.
> Hi Sergei,
> Sorry, but I just want to know the reason. :-)
> Would you let know the reason not to add coding style fixes?
This change doesn't get covered by the patch subject/changelog. Even if
you're doing it, it should preferably be done in a separate patch and at the
very least documented in the changelog.
> Thank you.
Not at all.
> Best regards,
> Jingoo Han
WBR, Sergei
^ permalink raw reply
* Re: [ethtool 1/3] ethtool: fix ixgbe 82598EB only registers
From: Keller, Jacob E @ 2013-09-12 16:33 UTC (permalink / raw)
To: Ben Hutchings
Cc: Kirsher, Jeffrey T, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com
In-Reply-To: <1379000729.1531.12.camel@bwh-desktop.uk.level5networks.com>
On Thu, 2013-09-12 at 16:45 +0100, Ben Hutchings wrote:
> On Thu, 2013-08-29 at 21:51 +0100, Ben Hutchings wrote:
> > On Tue, 2013-08-27 at 17:08 -0700, Jeff Kirsher wrote:
> > > From: Jacob Keller <jacob.e.keller@intel.com>
> > >
> > > This patch fixes ixgbe_reg_dump to only display registers specific to the
> > > 82598EB part, as these registers display 0xDEADBEEF otherwise, and clutter up
> > > the register dump.
> > >
> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > ---
> > > ixgbe.c | 144 +++++++++++++++++++++++++++++++++-------------------------------
> > > 1 file changed, 75 insertions(+), 69 deletions(-)
> > >
> > > diff --git a/ixgbe.c b/ixgbe.c
> > > index 9b005f2..89cb6be 100644
> > > --- a/ixgbe.c
> > > +++ b/ixgbe.c
> > [...]
> > > - fprintf(stdout,
> > > - "0x02F20: RDPROBE (Rx Probe Mode Status) 0x%08X\n",
> > > - regs_buff[1085]);
> > [...]
> >
> > It looks like this removal really belongs to the next patch, which adds
> > it back with the mac_type < ixgbe_mac_X540 condition.
>
> Well, I've applied the two patches with this bit fixed up.
>
> Ben.
>
Thanks, Ben.
Regards,
Jake
^ permalink raw reply
* [PATCH net-next v3] Don't destroy the netdev until the vif is shut down
From: Paul Durrant @ 2013-09-12 16:19 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: Paul Durrant, David Vrabel, Wei Liu, Ian Campbell
Without this patch, if a frontend cycles through states Closing
and Closed (which Windows frontends need to do) then the netdev
will be destroyed and requires re-invocation of hotplug scripts
to restore state before the frontend can move to Connected. Thus
when udev is not in use the backend gets stuck in InitWait.
With this patch, the netdev is left alone whilst the backend is
still online and is only de-registered and freed just prior to
destroying the vif (which is also nicely symmetrical with the
netdev allocation and registration being done during probe) so
no re-invocation of hotplug scripts is required.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
v2:
- Modify netback_remove() - bug only seemed to manifest with linux guest
v3:
- Move __module_get() and module_put() calls
drivers/net/xen-netback/common.h | 1 +
drivers/net/xen-netback/interface.c | 25 +++++++++----------------
drivers/net/xen-netback/xenbus.c | 17 ++++++++++++-----
3 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a197743..5715318 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -184,6 +184,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
unsigned long rx_ring_ref, unsigned int tx_evtchn,
unsigned int rx_evtchn);
void xenvif_disconnect(struct xenvif *vif);
+void xenvif_free(struct xenvif *vif);
int xenvif_xenbus_init(void);
void xenvif_xenbus_fini(void);
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 625c6f4..6b1096d 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -353,6 +353,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
}
netdev_dbg(dev, "Successfully created xenvif\n");
+
+ __module_get(THIS_MODULE);
+
return vif;
}
@@ -366,8 +369,6 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
if (vif->tx_irq)
return 0;
- __module_get(THIS_MODULE);
-
err = xenvif_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
if (err < 0)
goto err;
@@ -452,12 +453,6 @@ void xenvif_carrier_off(struct xenvif *vif)
void xenvif_disconnect(struct xenvif *vif)
{
- /* Disconnect funtion might get called by generic framework
- * even before vif connects, so we need to check if we really
- * need to do a module_put.
- */
- int need_module_put = 0;
-
if (netif_carrier_ok(vif->dev))
xenvif_carrier_off(vif);
@@ -468,23 +463,21 @@ void xenvif_disconnect(struct xenvif *vif)
unbind_from_irqhandler(vif->tx_irq, vif);
unbind_from_irqhandler(vif->rx_irq, vif);
}
- /* vif->irq is valid, we had a module_get in
- * xenvif_connect.
- */
- need_module_put = 1;
}
if (vif->task)
kthread_stop(vif->task);
+ xenvif_unmap_frontend_rings(vif);
+}
+
+void xenvif_free(struct xenvif *vif)
+{
netif_napi_del(&vif->napi);
unregister_netdev(vif->dev);
- xenvif_unmap_frontend_rings(vif);
-
free_netdev(vif->dev);
- if (need_module_put)
- module_put(THIS_MODULE);
+ module_put(THIS_MODULE);
}
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 1fe48fe3..a53782e 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -42,7 +42,7 @@ static int netback_remove(struct xenbus_device *dev)
if (be->vif) {
kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
- xenvif_disconnect(be->vif);
+ xenvif_free(be->vif);
be->vif = NULL;
}
kfree(be);
@@ -213,9 +213,18 @@ static void disconnect_backend(struct xenbus_device *dev)
{
struct backend_info *be = dev_get_drvdata(&dev->dev);
+ if (be->vif)
+ xenvif_disconnect(be->vif);
+}
+
+static void destroy_backend(struct xenbus_device *dev)
+{
+ struct backend_info *be = dev_get_drvdata(&dev->dev);
+
if (be->vif) {
+ kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
- xenvif_disconnect(be->vif);
+ xenvif_free(be->vif);
be->vif = NULL;
}
}
@@ -246,14 +255,11 @@ static void frontend_changed(struct xenbus_device *dev,
case XenbusStateConnected:
if (dev->state == XenbusStateConnected)
break;
- backend_create_xenvif(be);
if (be->vif)
connect(be);
break;
case XenbusStateClosing:
- if (be->vif)
- kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
disconnect_backend(dev);
xenbus_switch_state(dev, XenbusStateClosing);
break;
@@ -262,6 +268,7 @@ static void frontend_changed(struct xenbus_device *dev,
xenbus_switch_state(dev, XenbusStateClosed);
if (xenbus_dev_is_online(dev))
break;
+ destroy_backend(dev);
/* fall through if not online */
case XenbusStateUnknown:
device_unregister(&dev->dev);
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Eric Dumazet @ 2013-09-12 16:19 UTC (permalink / raw)
To: Hong zhi guo; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
In-Reply-To: <CAA7+ByUy5jL2hpkN+GBDRCXh_GM7qnS58iZvgp1RxZmoAqj+bQ@mail.gmail.com>
On Fri, 2013-09-13 at 00:18 +0800, Hong zhi guo wrote:
> sorry, Eric, maybe I'm using wrong words. Thanks for your review and help.
> So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix.
Please do not top post on netdev.
Thanks
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Paul E. McKenney @ 2013-09-12 16:17 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Nikolay Aleksandrov, Ding Tianhong, Jay Vosburgh, Andy Gospodarek,
David S. Miller, Netdev
In-Reply-To: <20130907150350.GF26163@redhat.com>
On Sat, Sep 07, 2013 at 05:03:50PM +0200, Veaceslav Falico wrote:
> On Sat, Sep 07, 2013 at 04:45:05PM +0200, Nikolay Aleksandrov wrote:
> >
> >On 09/07/2013 04:20 PM, Veaceslav Falico wrote:
> >>On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote:
> ...snip...
> >>diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> >>index f4b1001..37b49d1 100644
> >>--- a/include/linux/rculist.h
> >>+++ b/include/linux/rculist.h
> >>@@ -23,6 +23,7 @@
> >> * way, we must not access it directly
> >> */
> >> #define list_next_rcu(list) (*((struct list_head __rcu
> >>**)(&(list)->next)))
> >>+#define list_prev_rcu(list) (*((struct list_head __rcu
> >>**)(&(list)->prev)))
> >>
> >> /*
> >> * Insert a new entry between two known consecutive entries.
> >>@@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct
> >>list_head *list,
> >> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
> >> })
> >>
> >>+#define list_last_or_null_rcu(ptr, type, member) \
> >>+ ({struct list_head *__ptr = (ptr); \
> >>+ struct list_head __rcu *__last = list_prev_rcu(__ptr); \
> >>+ likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \
> >>+ })
> >>+
> >Hi,
> >Actually I don't think you can dereference ->prev and use the standard
> >list_del_rcu because it guarantees only the ->next ptr will be valid and
> >->prev is set to LIST_POISON2.
> >IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193
> >with the bidir_del and all that.
>
> Yeah, right, my bad - we can rely only on the ->next pointer, indeed,
> missed that part. RCU is hard :).
>
> So it'll be a lot harder to implement bond_last_slave_rcu() in a
> 'straightforward' approach.
>
> I'd rather go in the opposite direction here - i.e. drop the 'reverse'
> traversal completely, and all the use cases for bond_last_slave_rcu(). I've
> got some patches already - http://patchwork.ozlabs.org/patch/272076/ doing
> that, and hopefully will remove the whole 'backword' traversal completely
> in the future.
If it is important, it would be possible to create an RCU-protected
linked list that allowed readers to go in both directions -- mostly just
leave out the poisoning of the ->prev pointers. We do -not- want to
do this for the normal RCU-protected linked lists because the poisoning
does find bugs.
However, you would have to be quite careful with a bi-directional
RCU-protected linked list, because p->next->prev will not necessarily
be equal to p, for all the reasons that you guys listed out earlier in
this thread. So be very careful what you wish for! ;-)
Thanx, Paul
> >But in any case I complete agree with Veaceslav here. Read all the
> >documentation carefully :-)
> >
> >Cheers,
> >Nik
> >
> >> /**
> >> * list_for_each_entry_rcu - iterate over rcu list of given type
> >> * @pos: the type * to use as a loop cursor.
> >>------- END OF PATCH ------
> >>
> >>Anyway, it's up to you.
> >>
> >>Hope that helps.
> >
> --
> 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: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Hong zhi guo @ 2013-09-12 16:18 UTC (permalink / raw)
To: Eric Dumazet; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
In-Reply-To: <1379002315.24408.19.camel@edumazet-glaptop>
sorry, Eric, maybe I'm using wrong words. Thanks for your review and help.
So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix.
On Fri, Sep 13, 2013 at 12:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote:
>> You're right, Vlad.
>> One thing is missing in Eric's fix, NULL dereference is still possible
>> in br_handle_local_finish. Above is the new version of fix.
>
> Hey, it was not a 'fix', but a comment on your patch and bridge
> defensive programming.
>
>
>
--
best regards
Hong Zhiguo
^ permalink raw reply
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Eric Dumazet @ 2013-09-12 16:12 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: vyasevic, davem, netdev, Hong Zhiguo
In-Reply-To: <1379001428-2727-1-git-send-email-zhiguohong@tencent.com>
On Thu, 2013-09-12 at 23:57 +0800, Hong Zhiguo wrote:
> not check IFF_BRIDGE_PORT within br_handle_frame
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
No need to hurry. Take the time to write an extensive changelog, and
test the patch !
Thanks
^ permalink raw reply
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Eric Dumazet @ 2013-09-12 16:11 UTC (permalink / raw)
To: Hong zhi guo; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
In-Reply-To: <CAA7+ByXUnTi1+-yreK+q03Cbqg+FM7DTfQ9qXbyRbZ0N508JBQ@mail.gmail.com>
On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote:
> You're right, Vlad.
> One thing is missing in Eric's fix, NULL dereference is still possible
> in br_handle_local_finish. Above is the new version of fix.
Hey, it was not a 'fix', but a comment on your patch and bridge
defensive programming.
^ permalink raw reply
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Eric Dumazet @ 2013-09-12 16:08 UTC (permalink / raw)
To: vyasevic; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
In-Reply-To: <5231E2C7.1010106@redhat.com>
On Thu, 2013-09-12 at 11:50 -0400, Vlad Yasevich wrote:
> On 09/12/2013 11:24 AM, Eric Dumazet wrote:
> > On Thu, 2013-09-12 at 10:42 -0400, Vlad Yasevich wrote:
> >
> >> Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
> >> redundant as well?
> >
> > Sure but anyway this part is net-next material
> >
> > Lets fix the bug first.
>
> Seems like the race is possible in net as well. If we just include the
> original patch, then br_add_if() also has to be modified to make sure
> the flag is set properly before netdev_rx_handler_register() is called.
>
> If we remove the need to check the flag during bridge input handling,
> then br_add_if() change is not necessary.
Yep, really testing IFF_BRIDGE_PORT in fast path is currently racy.
rcu protection should be enough.
^ permalink raw reply
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Hong zhi guo @ 2013-09-12 16:06 UTC (permalink / raw)
To: vyasevic; +Cc: Eric Dumazet, David Miller, netdev, Hong Zhiguo
In-Reply-To: <1379001428-2727-1-git-send-email-zhiguohong@tencent.com>
You're right, Vlad.
One thing is missing in Eric's fix, NULL dereference is still possible
in br_handle_local_finish. Above is the new version of fix.
On Thu, Sep 12, 2013 at 11:57 PM, Hong Zhiguo <honkiko@gmail.com> wrote:
> not check IFF_BRIDGE_PORT within br_handle_frame
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
> net/bridge/br_input.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a2fd37e..da4714a 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> struct net_bridge *br;
> struct net_bridge_fdb_entry *dst;
> struct net_bridge_mdb_entry *mdst;
> @@ -143,7 +143,7 @@ drop:
> /* note: already called with rcu_read_lock */
> static int br_handle_local_finish(struct sk_buff *skb)
> {
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> u16 vid = 0;
>
> br_vlan_get_tag(skb, &vid);
> @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> if (!skb)
> return RX_HANDLER_CONSUMED;
>
> - p = br_port_get_rcu(skb->dev);
> + p = rcu_dereference(skb->dev->rx_handler_data);
>
> if (unlikely(is_link_local_ether_addr(dest))) {
> /*
> --
> 1.7.0.4
>
--
best regards
Hong Zhiguo
^ permalink raw reply
* Re: [PATCH 14/52] net: cxgb4vf: remove unnecessary pci_set_drvdata()
From: Casey Leedom @ 2013-09-12 15:59 UTC (permalink / raw)
To: Jingoo Han; +Cc: 'David S. Miller', netdev
In-Reply-To: <000001ceaf56$1cdf7d60$569e7820$%han@samsung.com>
Ah, okay. And I presume the same is true of the PCI Remove path. I
wasn't aware of this but if it's the Linux pattern to let the
surrounding support code do this work, then go for it.
As I said, I'm something of a Mad Housewife when it comes to cleaning
things up which were allocated/assigned in a particular routine.
Basically I program like I drive, practising defensive programming ... :-)
Casey
On 09/11/13 18:19, Jingoo Han wrote:
> On Thursday, September 12, 2013 2:24 AM, Casey Leedom wrote:
>> I agree that the redundant pci_set_drvdata(pdev, NULL) in
>> cxgb4vf_pci_probe() under the err_release_regions: label is unneeded,
>> but don't we need to NULL out the PCI Driver Data under the
>> err_free_adapter: label and also in cxgb4vf_pci_remove()? Or is that
>> handled automatically in the PCI infrastructure code which calls the
>> Device Probe and Remove routines? Mostly I was just being an
>> obsessively clean housewife assuming that we'd want to clean up these
>> references ...
>
> No, 'pci_set_drvdata(pdev, NULL) under err_free_adapter label' is not
> necessary.
>
> As you know, pci_set_drvdata(pdev, NULL) calls dev_set_drvdata() as below:
> pci_set_drvdata(pdev, NULL) is dev_set_drvdata(&pdev->dev, NULL).
>
> ./include/linux/pci.h
> 1504:static inline void pci_set_drvdata(struct pci_dev *pdev, void *data)
> 1505{
> 1506 dev_set_drvdata(&pdev->dev, data);
> 1507}
>
>
> However, when the driver goes to err_free_adapter label,
> The following sequence will be done.
> kfree(adapter) -> .... -> return -ENOMEM;
>
> In this case,
> when probe() returns error value such as '-ENOMEM',
> really_probe() of driver core automatically calls 'dev_set_drvdata(dev, NULL)'
> as below:
>
> ./drivers/base/dd.c
> 303-probe_failed:
> 304 devres_release_all(dev);
> 305 driver_sysfs_remove(dev);
> 306 dev->driver = NULL;
> 307 dev_set_drvdata(dev, NULL);
>
> Thus, without 'pci_set_drvdata(pdev, NULL) under err_free_adapter label',
> dev_set_drvdata(dev, NULL) can be called.
>
> I already tested this with other drivers such as e1000e LAN card driver.
>
> Best regards,
> Jingoo Han
>
>
^ permalink raw reply
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
From: Joe Perches @ 2013-09-12 15:59 UTC (permalink / raw)
To: David Laight
Cc: Al Viro, Tetsuo Handa, linux-kernel, kosaki.motohiro, keescook,
fweisbec, dan.carpenter, devel, gregkh, tushar.behera,
lidza.louina, davem, kuznet, jmorris, yoshfuji, kaber, courmisch,
vyasevich, nhorman, netdev, linux-sctp
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B732A@saturn3.aculab.com>
On Thu, 2013-09-12 at 09:06 +0100, David Laight wrote:
> > On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote:
> > > On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> > > > Joe Perches wrote:
> > > > > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > > > > + len = seq_printf(m, "%s%d", con->name, con->index);
> > > >
> > > > Isn't len always 0 or -1 ?
> > >
> > > Right. Well you're no fun...
[]
> > > Suggestions?
>
> Change the return type of seq_printf() to void and require that
> code use access functions/macros to find the length and error
> status. Possibly save the length of the last call separately.
Are there any races if this is done?
---
fs/seq_file.c | 15 +++++++--------
include/linux/seq_file.h | 6 ++++--
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..b6c9eab 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -389,32 +389,31 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
}
EXPORT_SYMBOL(seq_escape);
-int seq_vprintf(struct seq_file *m, const char *f, va_list args)
+void seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
int len;
if (m->count < m->size) {
len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
+ m->last_len = len;
if (m->count + len < m->size) {
m->count += len;
- return 0;
+ m->last_rtn = 0;
+ return;
}
}
seq_set_overflow(m);
- return -1;
+ m->last_rtn = -1;
}
EXPORT_SYMBOL(seq_vprintf);
-int seq_printf(struct seq_file *m, const char *f, ...)
+void seq_printf(struct seq_file *m, const char *f, ...)
{
- int ret;
va_list args;
va_start(args, f);
- ret = seq_vprintf(m, f, args);
+ seq_vprintf(m, f, args);
va_end(args);
-
- return ret;
}
EXPORT_SYMBOL(seq_printf);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..9af05e1 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -20,6 +20,8 @@ struct seq_file {
size_t size;
size_t from;
size_t count;
+ size_t last_len;
+ int last_rtn;
loff_t index;
loff_t read_pos;
u64 version;
@@ -89,8 +91,8 @@ int seq_putc(struct seq_file *m, char c);
int seq_puts(struct seq_file *m, const char *s);
int seq_write(struct seq_file *seq, const void *data, size_t len);
-__printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
-__printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
+__printf(2, 3) void seq_printf(struct seq_file *, const char *, ...);
+__printf(2, 0) void seq_vprintf(struct seq_file *, const char *, va_list args);
int seq_path(struct seq_file *, const struct path *, const char *);
int seq_dentry(struct seq_file *, struct dentry *, const char *);
^ permalink raw reply related
* ethtool 3.11 released
From: Ben Hutchings @ 2013-09-12 15:58 UTC (permalink / raw)
To: netdev
ethtool version 3.11 has been released.
Home page: https://ftp.kernel.org/pub/software/network/ethtool/
Download link:
https://ftp.kernel.org/pub/software/network/ethtool/ethtool-3.11.tar.xz
Release notes:
* Feature: Update Realtek chip list for register dump to match
r8169 driver in Linux 3.11 (-d option)
* Feature: Add ixgbevf support for register dump (-d option)
* Feature: Filter ixgbe register dump according to the specific chip
(-d option)
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH net-next] fix NULL pointer dereference in br_handle_frame
From: Hong Zhiguo @ 2013-09-12 15:57 UTC (permalink / raw)
To: vyasevic; +Cc: eric.dumazet, davem, netdev, Hong Zhiguo
In-Reply-To: <1378988195-2710-1-git-send-email-zhiguohong@tencent.com>
not check IFF_BRIDGE_PORT within br_handle_frame
Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
net/bridge/br_input.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a2fd37e..da4714a 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
int br_handle_frame_finish(struct sk_buff *skb)
{
const unsigned char *dest = eth_hdr(skb)->h_dest;
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
struct net_bridge *br;
struct net_bridge_fdb_entry *dst;
struct net_bridge_mdb_entry *mdst;
@@ -143,7 +143,7 @@ drop:
/* note: already called with rcu_read_lock */
static int br_handle_local_finish(struct sk_buff *skb)
{
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
u16 vid = 0;
br_vlan_get_tag(skb, &vid);
@@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
if (!skb)
return RX_HANDLER_CONSUMED;
- p = br_port_get_rcu(skb->dev);
+ p = rcu_dereference(skb->dev->rx_handler_data);
if (unlikely(is_link_local_ether_addr(dest))) {
/*
--
1.7.0.4
^ permalink raw reply related
* Re: ip l2tp - suspected defect using IPv6 local/remote addresses
From: Ben Hutchings @ 2013-09-12 13:19 UTC (permalink / raw)
To: James Chapman; +Cc: Jeff Loughridge, netdev
In-Reply-To: <CAEwTi7Qo8=X=v51_1y3JMQnbG3xBQYRb9YRePFh6KfYmb_Y0-g@mail.gmail.com>
On Thu, 2013-09-12 at 11:20 +0100, James Chapman wrote:
> On 11 September 2013 19:52, Jeff Loughridge <jeffl@alumni.duke.edu> wrote:
> >
> > Using IPv6 address as L2TPv3 endpoints doesn't seem to work in
> > iproute2 3.11. I see a cosmetic defect in the output of 'ip l2tp show
> > tunnel'. In addition, I can't get tunnels to function with UDP or IP
> > encapsulation.
> >
> > root@debian:~# ip l2tp add tunnel tunnel_id 3000 peer_tunnel_id 4000
> > encap udp local a::1 remote a::2 udp_sport 5000 udp_dport 6000
> > root@debian:~# ip l2tp add session tunnel_id 3000 session_id 1000
> > peer_session_id 2000
> > root@debian:~# ip l2tp show tunnel
> > Tunnel 3000, encap UDP
> > From 127.0.0.1 to 127.0.0.1
> > Peer tunnel 4000
> > UDP source / dest ports: 5000/6000
> > root@debian:~#
>
> You'll need a 3.5 or later kernel for L2TP over IPv6. I see you are
> using 3.2. Are you using a version of iproute2 which is not matched to
> your kernel?
[...]
The iproute2 version number only indicates which kernel features it
supports; it is supposed to be backward-compatible. And it really
should not silently fail like this, although this may well be a bug in
the API that can't be fixed in userland...
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [ethtool 1/3] ethtool: fix ixgbe 82598EB only registers
From: Ben Hutchings @ 2013-09-12 15:45 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: Jacob Keller, netdev, gospo, sassmann
In-Reply-To: <1377809470.5372.16.camel@bwh-desktop.uk.level5networks.com>
On Thu, 2013-08-29 at 21:51 +0100, Ben Hutchings wrote:
> On Tue, 2013-08-27 at 17:08 -0700, Jeff Kirsher wrote:
> > From: Jacob Keller <jacob.e.keller@intel.com>
> >
> > This patch fixes ixgbe_reg_dump to only display registers specific to the
> > 82598EB part, as these registers display 0xDEADBEEF otherwise, and clutter up
> > the register dump.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> > ixgbe.c | 144 +++++++++++++++++++++++++++++++++-------------------------------
> > 1 file changed, 75 insertions(+), 69 deletions(-)
> >
> > diff --git a/ixgbe.c b/ixgbe.c
> > index 9b005f2..89cb6be 100644
> > --- a/ixgbe.c
> > +++ b/ixgbe.c
> [...]
> > - fprintf(stdout,
> > - "0x02F20: RDPROBE (Rx Probe Mode Status) 0x%08X\n",
> > - regs_buff[1085]);
> [...]
>
> It looks like this removal really belongs to the next patch, which adds
> it back with the mac_type < ixgbe_mac_X540 condition.
Well, I've applied the two patches with this bit fixed up.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 20/52] net: fealnx: remove unnecessary pci_set_drvdata()
From: Ben Hutchings @ 2013-09-12 13:16 UTC (permalink / raw)
To: David Miller; +Cc: jg1.han, sergei.shtylyov, netdev
In-Reply-To: <20130912.033606.1689936899104585741.davem@davemloft.net>
On Thu, 2013-09-12 at 03:36 -0400, David Miller wrote:
> From: Jingoo Han <jg1.han@samsung.com>
> Date: Thu, 12 Sep 2013 14:49:56 +0900
>
> > On Thursday, September 12, 2013 2:25 PM, David Miller wrote:
> >> On Thu, 12 Sep 2013 09:11:01 +0900, Jingoo Han wrote:
> >> > Would you let know the reason not to add coding style fixes?
> >>
> >> They should be made seperately so that the individual changes
> >> can be reviewed more easily and without unnecessary unrelated
> >> changes mixed in.
> >
> > OK, I see. :-)
> > Thank you for your answer.
> > Then, I will send V2 patch soon.
>
> Please do not submit such a huge patch series in the future.
>
> Anything more than 16 patches at a time is not reasonable and
> overloads reviewer's capacity to look at your changes.
Given that this is the same trivial change applied to many different
drivers, is it really worthwhile to break it up into a patch per driver?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [ethtool] ethtool: correct ixgbevf copyright date
From: Ben Hutchings @ 2013-09-12 14:17 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: Jacob Keller, netdev, gospo, sassmann
In-Reply-To: <1377925372-13385-1-git-send-email-jeffrey.t.kirsher@intel.com>
On Fri, 2013-08-30 at 22:02 -0700, Jeff Kirsher wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
>
> This patch corrects the copyright date of the ixgbevf code, which was only
> recently added.
Applied, thanks.
Ben.
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> ixgbevf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ixgbevf.c b/ixgbevf.c
> index dae1178..2a3aa6f 100644
> --- a/ixgbevf.c
> +++ b/ixgbevf.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2007 Intel Corporation */
> +/* Copyright (c) 2013 Intel Corporation */
> #include <stdio.h>
> #include "internal.h"
>
--
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 net-next] fix NULL pointer dereference in br_handle_frame
From: Vlad Yasevich @ 2013-09-12 15:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
In-Reply-To: <1378999470.24408.10.camel@edumazet-glaptop>
On 09/12/2013 11:24 AM, Eric Dumazet wrote:
> On Thu, 2013-09-12 at 10:42 -0400, Vlad Yasevich wrote:
>
>> Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
>> redundant as well?
>
> Sure but anyway this part is net-next material
>
> Lets fix the bug first.
Seems like the race is possible in net as well. If we just include the
original patch, then br_add_if() also has to be modified to make sure
the flag is set properly before netdev_rx_handler_register() is called.
If we remove the need to check the flag during bridge input handling,
then br_add_if() change is not necessary.
-vlad
>
> Acked-by: Eric Dumazet <edumazet@google.com>
>
>
> --
> 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
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