* [PATCH 2/4] net/irda: Hold port lock while bumping blocked_open
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362499747-4262-1-git-send-email-peter@hurleysoftware.com>
Although tty_lock() already protects concurrent update to
blocked_open, that fails to meet the separation-of-concerns between
tty_port and tty.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
net/irda/ircomm/ircomm_tty.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 1721dc7..d282bbe 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -317,8 +317,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count--;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open++;
+ spin_unlock_irqrestore(&port->lock, flags);
while (1) {
if (tty->termios.c_cflag & CBAUD)
@@ -362,8 +362,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
spin_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count++;
- spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open--;
+ spin_unlock_irqrestore(&port->lock, flags);
IRDA_DEBUG(1, "%s(%d):block_til_ready after blocking on %s open_count=%d\n",
__FILE__, __LINE__, tty->driver->name, port->count);
--
1.8.1.2
^ permalink raw reply related
* [PATCH 1/4] net/irda: Fix port open counts
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362499747-4262-1-git-send-email-peter@hurleysoftware.com>
Saving the port count bump is unsafe. If the tty is hung up while
this open was blocking, the port count is zeroed.
Explicitly check if the tty was hung up while blocking, and correct
the port count if not.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
net/irda/ircomm/ircomm_tty.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 9a5fd3c..1721dc7 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -280,7 +280,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
struct tty_port *port = &self->port;
DECLARE_WAITQUEUE(wait, current);
int retval;
- int do_clocal = 0, extra_count = 0;
+ int do_clocal = 0;
unsigned long flags;
IRDA_DEBUG(2, "%s()\n", __func__ );
@@ -315,10 +315,8 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
__FILE__, __LINE__, tty->driver->name, port->count);
spin_lock_irqsave(&port->lock, flags);
- if (!tty_hung_up_p(filp)) {
- extra_count = 1;
+ if (!tty_hung_up_p(filp))
port->count--;
- }
spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open++;
@@ -361,12 +359,10 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
__set_current_state(TASK_RUNNING);
remove_wait_queue(&port->open_wait, &wait);
- if (extra_count) {
- /* ++ is not atomic, so this should be protected - Jean II */
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ if (!tty_hung_up_p(filp))
port->count++;
- spin_unlock_irqrestore(&port->lock, flags);
- }
+ spin_unlock_irqrestore(&port->lock, flags);
port->blocked_open--;
IRDA_DEBUG(1, "%s(%d):block_til_ready after blocking on %s open_count=%d\n",
--
1.8.1.2
^ permalink raw reply related
* [PATCH 0/4] other ircomm_tty fixes (was Re: [PATCH] ircomm: release tty before sleeping potentially indefintely)
From: Peter Hurley @ 2013-03-05 16:09 UTC (permalink / raw)
To: David Miller, Samuel Ortiz
Cc: Sasha Levin, Greg Kroah-Hartman, netdev, Jiri Slaby, linux-kernel,
Peter Hurley
In-Reply-To: <1362371063.3221.185.camel@thor.lan>
On Sun, 2013-03-03 at 23:24 -0500, Peter Hurley wrote:
> On Sun, 2013-03-03 at 21:36 -0500, David Miller wrote:
> > From: Peter Hurley <peter@hurleysoftware.com>
> > Date: Sun, 03 Mar 2013 20:06:18 -0500
> >
> > > But regardless, this function __cannot__ sleep holding the tty_lock().
> >
> > So drop it across the schedule(), but recheck the termios after
> > regrabbing it.
>
> I'll have to do some research on that.
Still working on this...
> In the meantime, while reviewing that code, I noticed there's a handful
> of serious bugs in that one function that I'll send a patchset for.
As promised.
Peter Hurley (4):
net/irda: Fix port open counts
net/irda: Hold port lock while bumping blocked_open
net/irda: Use barrier to set task state
net/irda: Raise dtr in non-blocking open
net/irda/ircomm/ircomm_tty.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
--
1.8.1.2
^ permalink raw reply
* Re: [PATCH] lglock: add read-preference local-global rwlock
From: Lai Jiangshan @ 2013-03-05 15:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Michel Lespinasse, Srivatsa S. Bhat, Lai Jiangshan, linux-doc,
peterz, fweisbec, linux-kernel, namhyung, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, vincent.guittot, tglx, linux-arm-kernel, netdev,
sbw, tj, akpm, linuxppc-dev
In-Reply-To: <20130302170656.GB29769@redhat.com>
On 03/03/13 01:06, Oleg Nesterov wrote:
> On 03/02, Michel Lespinasse wrote:
>>
>> My version would be slower if it needs to take the
>> slow path in a reentrant way, but I'm not sure it matters either :)
>
> I'd say, this doesn't matter at all, simply because this can only happen
> if we race with the active writer.
>
It can also happen when interrupted. (still very rarely)
arch_spin_trylock()
------->interrupted,
__this_cpu_read() returns 0.
arch_spin_trylock() fails
slowpath, any nested will be slowpath too.
...
..._read_unlock()
<-------interrupt
__this_cpu_inc()
....
I saw get_online_cpu_atomic() is called very frequent.
And the above thing happens in one CPU rarely, but how often it
happens in the whole system if we have 4096 CPUs?
(I worries to much. I tend to remove FALLBACK_BASE now, we should
add it only after we proved we needed it, this part is not proved)
Thanks,
Lai
^ permalink raw reply
* Re: [Xen-devel] [PATCH 3/8] netback: get/put module along with vif connect/disconnect
From: Konrad Rzeszutek Wilk @ 2013-03-05 15:53 UTC (permalink / raw)
To: David Vrabel
Cc: Wei Liu, netdev@vger.kernel.org, annie.li@oracle.com,
Ian Campbell, xen-devel@lists.xen.org
In-Reply-To: <5135FC2E.9050705@citrix.com>
On Tue, Mar 05, 2013 at 02:07:42PM +0000, David Vrabel wrote:
> On 05/03/13 13:30, Wei Liu wrote:
> > On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote:
> >> On 15/02/13 16:00, Wei Liu wrote:
> >>> If there is vif running and user unloads netback, guest's network interface
> >>> just mysteriously stops working. So we need to prevent unloading netback
> >>> module if there is vif running.
> >>
> >> It's not mysterious -- it is cleanly disconnected, and will reconnect
> >> when the module is reinserted.
> >>
> >
> > From a guest's POV, it just stops without any sign. This should be
> > prevented IMHO.
>
> This is a bug in the frontend or a bug in the backend failing to
> disconnect correctly.
>
> I posted a series of "xen-foofront: handle backend CLOSED without
> CLOSING" patches that may help here. (I didn't get applied to netfront
> for some reason.)
Hm, could you resent it please and make sure that the networking
maintainer is on the To list?
>
> Disabling module unload doesn't prevent this from happening away. You
> can always manually unbind the backend device from the xen-netback
> driver which has the same effect as unloading the module.
>
> > Netback / netfront lose all states when netback is unloaded. And
> > netfront doesn't support reconfiguration at the moment. My guess is that
> > this is the reason why netback doesn't even have unload function at
> > first.
>
> If netfront cannot handle reconnect then that's a bug in the frontend or
> a bug in the backend xenbus code not setting up the reconnect correctly.
>
> >> Being able to unload modules while they are in use is standard so I
> >> don't think this should be applied.
> >
> > I don't think this is true from a module dependency point of view - just
> > try to unload any in use module, rmmod / modprobe will give you a fatal
> > error.
>
> Try it with any other network interface driver and it will unload just fine.
>
> David
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply
* Re: [Xen-devel] [PATCH 1/8] netback: don't bind kthread to cpu
From: Konrad Rzeszutek Wilk @ 2013-03-05 15:52 UTC (permalink / raw)
To: David Vrabel
Cc: Wei Liu, netdev@vger.kernel.org, annie.li@oracle.com,
Ian Campbell, xen-devel@lists.xen.org
In-Reply-To: <51360452.5070200@citrix.com>
On Tue, Mar 05, 2013 at 02:42:26PM +0000, David Vrabel wrote:
> On 05/03/13 13:56, Konrad Rzeszutek Wilk wrote:
> > On Tue, Mar 05, 2013 at 01:30:10PM +0000, Wei Liu wrote:
> >> On Mon, 2013-03-04 at 20:51 +0000, Konrad Rzeszutek Wilk wrote:
> >>> On Fri, Feb 15, 2013 at 04:00:02PM +0000, Wei Liu wrote:
> >>>> The initialization process makes an assumption that the online cpus are
> >>>> numbered from 0 to xen_netbk_group_nr-1, which is not always true.
> >>>
> >>> And xen_netbk_group_nr is num_online_cpus()?
> >>>
> >>
> >> Yes.
> >>
> >>> So under what conditions does this change? Is this when the CPU hotplug
> >>> is involved and the CPUs go offline? In which case should there be a
> >>
> >> Yes, the hotplug path.
> >>
> >>> CPU hotplug notifier to re-bind the workers are appropiate?
> >
> > ?
> > Can't that option be explored?
>
> I'm not sure binding netback threads to particular VCPUs is useful
> without also binding the events to the corresponding VCPUs.
>
> I would hope that the scheduler would tend to the correct behavior if
> threads aren't bound, anyway.
That is fine too. If that is what we want we just need to make the git
commit message be more descriptive of why we don't want to bind
to VCPUs.
>
> David
^ permalink raw reply
* Re: [PATCH V2] lglock: add read-preference local-global rwlock
From: Lai Jiangshan @ 2013-03-05 15:41 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Michel Lespinasse, Oleg Nesterov, Lai Jiangshan, linux-doc,
peterz, fweisbec, linux-kernel, namhyung, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, vincent.guittot, tglx, linux-arm-kernel, netdev,
sbw, tj, akpm, linuxppc-dev
In-Reply-To: <513232B6.9060905@linux.vnet.ibm.com>
On 03/03/13 01:11, Srivatsa S. Bhat wrote:
> On 03/02/2013 06:44 PM, Lai Jiangshan wrote:
>> From 345a7a75c314ff567be48983e0892bc69c4452e7 Mon Sep 17 00:00:00 2001
>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Date: Sat, 2 Mar 2013 20:33:14 +0800
>> Subject: [PATCH] lglock: add read-preference local-global rwlock
>>
>> Current lglock is not read-preference, so it can't be used on some cases
>> which read-preference rwlock can do. Example, get_cpu_online_atomic().
>>
> [...]
>> diff --git a/kernel/lglock.c b/kernel/lglock.c
>> index 6535a66..52e9b2c 100644
>> --- a/kernel/lglock.c
>> +++ b/kernel/lglock.c
>> @@ -87,3 +87,71 @@ void lg_global_unlock(struct lglock *lg)
>> preempt_enable();
>> }
>> EXPORT_SYMBOL(lg_global_unlock);
>> +
>> +#define FALLBACK_BASE (1UL << 30)
>> +
>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw)
>> +{
>> + struct lglock *lg = &lgrw->lglock;
>> +
>> + preempt_disable();
>> + if (likely(!__this_cpu_read(*lgrw->reader_refcnt))) {
>> + rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
>> + if (unlikely(!arch_spin_trylock(this_cpu_ptr(lg->lock)))) {
>> + read_lock(&lgrw->fallback_rwlock);
>> + __this_cpu_write(*lgrw->reader_refcnt, FALLBACK_BASE);
>> + return;
>> + }
>> + }
>> +
>> + __this_cpu_inc(*lgrw->reader_refcnt);
>> +}
>> +EXPORT_SYMBOL(lg_rwlock_local_read_lock);
>> +
>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
>> +{
>> + switch (__this_cpu_read(*lgrw->reader_refcnt)) {
>> + case 1:
>> + __this_cpu_write(*lgrw->reader_refcnt, 0);
>> + lg_local_unlock(&lgrw->lglock);
>> + return;
>
> This should be a break, instead of a return, right?
> Otherwise, there will be a preempt imbalance...
"lockdep" and "preempt" are handled in lg_local_unlock(&lgrw->lglock);
Thanks,
Lai
>
>> + case FALLBACK_BASE:
>> + __this_cpu_write(*lgrw->reader_refcnt, 0);
>> + read_unlock(&lgrw->fallback_rwlock);
>> + rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
>> + break;
>> + default:
>> + __this_cpu_dec(*lgrw->reader_refcnt);
>> + break;
>> + }
>> +
>> + preempt_enable();
>> +}
>
>
> Regards,
> Srivatsa S. Bhat
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 15:29 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <871ubtq47a.fsf@nemi.mork.no>
On Tue, Mar 5, 2013 at 11:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>> On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
>>
>>> Now, after inspecting wdm_suspend() which hides behind
>>> info->subdriver->suspend(), I see that this is a completely theoretical
>>> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>>>
>>> Which means that your change really is a noop(). I still don't like it,
>>> because it gives the impression that errors from info->subdriver->suspend()
>>> isn't dealt with.
>>
>> Yes, it needn't dealt with in system sleep, so it has the document benefit.
>
> The document that the return code is ignored by forcing it to 0 and
> always return success.
>
> But am still not convinced this is correct in any way. AFAICS there is
> no documentation stating that the drivers should care about what the USB
We have source code, :-)
> core use the return value for. Drivers are allowed to fail and the core
> will ignore and clean up if necessary.
>
> I still find any partial failures horrendous. If we are not going to
> fail on errors, then we'll return success and pretend to be suspended.
>
>> No, USB core does not handle it, and will always ignore the return
>> failure from driver's suspend, see usb_suspend_both() for non-autosuspend
>> case.
>
> Right you are. Somehow I read the inverse. Sorry about that.
>
> In any case, it ends up here and everythin will be OK, which I assume is
> why it can ignore the errors:
>
> /* If the suspend succeeded then prevent any more URB submissions
> * and flush any outstanding URBs.
> */
> } else {
> udev->can_submit = 0;
> for (i = 0; i < 16; ++i) {
> usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
> usb_hcd_flush_endpoint(udev, udev->ep_in[i]);
> }
> }
Yes, USB core will flush any outstanding URBs, but the driver still need
to deal with suspend failure carefully, for example, suppose usb_resume()
is called in suspend failure path, and the submitted URBs are killed
by USB core later. But after the device is wakeup, and the resume() will
do nothing since the suspend count is leaked. So it is what the patches
are fixing, and it is better to not depend on the default flushing URBs of
USB core.
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH V2] lglock: add read-preference local-global rwlock
From: Lai Jiangshan @ 2013-03-05 15:27 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Michel Lespinasse, Srivatsa S. Bhat, Lai Jiangshan, linux-doc,
peterz, fweisbec, linux-kernel, namhyung, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, vincent.guittot, tglx, linux-arm-kernel, netdev,
sbw, tj, akpm, linuxppc-dev
In-Reply-To: <20130302172003.GC29769@redhat.com>
On 03/03/13 01:20, Oleg Nesterov wrote:
> On 03/02, Lai Jiangshan wrote:
>>
>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
>> +{
>> + switch (__this_cpu_read(*lgrw->reader_refcnt)) {
>> + case 1:
>> + __this_cpu_write(*lgrw->reader_refcnt, 0);
>> + lg_local_unlock(&lgrw->lglock);
>> + return;
>> + case FALLBACK_BASE:
>> + __this_cpu_write(*lgrw->reader_refcnt, 0);
>> + read_unlock(&lgrw->fallback_rwlock);
>> + rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
>
> I guess "case 1:" should do rwlock_release() too.
Already do it in "lg_local_unlock(&lgrw->lglock);" before it returns.
(I like reuse old code)
>
> Otherwise, at first glance looks correct...
>
> However, I still think that FALLBACK_BASE only adds the unnecessary
> complications. But even if I am right this is subjective of course, please
> feel free to ignore.
OK, I kill FALLBACK_BASE in later patch.
>
> And btw, I am not sure about lg->lock_dep_map, perhaps we should use
> fallback_rwlock->dep_map ?
Use either one is OK.
>
> We need rwlock_acquire_read() even in the fast-path, and this acquire_read
> should be paired with rwlock_acquire() in _write_lock(), but it does
> spin_acquire(lg->lock_dep_map). Yes, currently this is the same (afaics)
> but perhaps fallback_rwlock->dep_map would be more clean.
>
I can't tell which one is better. I try to use fallback_rwlock->dep_map later.
> Oleg.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* hitting lockdep warning as of too early VF probe with 3.9-rc1
From: Or Gerlitz @ 2013-03-05 15:21 UTC (permalink / raw)
To: Ming Lei, Greg Kroah-Hartman, David Miller, Roland Dreier
Cc: netdev, Yan Burman, Jack Morgenstein
Hi Ming, Greg, Roland, Dave, all..
With 3.9-rc1, we are hitting the below lockdep with probing of virtual
functions over the mlx4 driver, where it seems that the probing of the
VF starts before the PF initialization is done.
Yan Burman from our team bisected that to be introduced by commit
190888ac01d059e38ffe77a2291d44cafa9016fb
"driver core: fix possible missing of device probe".
Basically what happens is that the VF probe fails, and once the PF
probing/initialization is done, the VF
is probed again and this time it succeeds.
Anything here which people see to be possibly wrong with the mlx4_core
(drivers/net/ethernet/mellanox/mlx4) driver interaction with the PCI
subsystem?
Or.
mlx4_core: Initializing 0000:04:00.0
mlx4_core 0000:04:00.0: Enabling SR-IOV with 1 VFs
pci 0000:04:00.1: [15b3:1004] type 00 class 0x028000
=============================================
[ INFO: possible recursive locking detected ]
3.9.0-rc1 #96 Not tainted
---------------------------------------------
kworker/0:1/734 is trying to acquire lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81066cb0>] flush_work+0x0/0x250
but task is already holding lock:
((&wfc.work)){+.+.+.}, at: [<ffffffff81064352>]
process_one_work+0x162/0x4c0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock((&wfc.work));
lock((&wfc.work));
*** DEADLOCK ***
May be due to missing lock nesting notation
3 locks held by kworker/0:1/734:
#0: (events){.+.+.+}, at: [<ffffffff81064352>]
process_one_work+0x162/0x4c0
#1: ((&wfc.work)){+.+.+.}, at: [<ffffffff81064352>]
process_one_work+0x162/0x4c0
#2: (&__lockdep_no_validate__){......}, at: [<ffffffff812db225>]
device_attach+0x25/0xb0
stack backtrace:
Pid: 734, comm: kworker/0:1 Not tainted 3.9.0-rc1 #96
Call Trace:
[<ffffffff810948ec>] validate_chain+0xdcc/0x11f0
[<ffffffff81095150>] __lock_acquire+0x440/0xc70
[<ffffffff81095150>] ? __lock_acquire+0x440/0xc70
[<ffffffff810959da>] lock_acquire+0x5a/0x70
[<ffffffff81066cb0>] ? wq_worker_waking_up+0x60/0x60
[<ffffffff81066cf5>] flush_work+0x45/0x250
[<ffffffff81066cb0>] ? wq_worker_waking_up+0x60/0x60
[<ffffffff810922be>] ? mark_held_locks+0x9e/0x130
[<ffffffff81066a96>] ? queue_work_on+0x46/0x90
[<ffffffff810925dd>] ? trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff8109267d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff81066f74>] work_on_cpu+0x74/0x90
[<ffffffff81063820>] ? keventd_up+0x20/0x20
[<ffffffff8121fd30>] ? pci_pm_prepare+0x60/0x60
[<ffffffff811f9293>] ? cpumask_next_and+0x23/0x40
[<ffffffff81220a1a>] pci_device_probe+0xba/0x110
[<ffffffff812dadca>] ? driver_sysfs_add+0x7a/0xb0
[<ffffffff812daf1f>] driver_probe_device+0x8f/0x230
[<ffffffff812db170>] ? __driver_attach+0xb0/0xb0
[<ffffffff812db1bb>] __device_attach+0x4b/0x60
[<ffffffff812d9314>] bus_for_each_drv+0x64/0x90
[<ffffffff812db298>] device_attach+0x98/0xb0
[<ffffffff81218474>] pci_bus_add_device+0x24/0x50
[<ffffffff81232e80>] virtfn_add+0x240/0x3e0
[<ffffffff8146ce3d>] ? _raw_spin_unlock_irqrestore+0x3d/0x80
[<ffffffff812333be>] pci_enable_sriov+0x23e/0x500
[<ffffffffa011fa1a>] __mlx4_init_one+0x5da/0xce0 [mlx4_core]
[<ffffffffa012016d>] mlx4_init_one+0x2d/0x60 [mlx4_core]
[<ffffffff8121fd79>] local_pci_probe+0x49/0x80
[<ffffffff81063833>] work_for_cpu_fn+0x13/0x20
[<ffffffff810643b8>] process_one_work+0x1c8/0x4c0
[<ffffffff81064352>] ? process_one_work+0x162/0x4c0
[<ffffffff81064cfb>] worker_thread+0x30b/0x430
[<ffffffff810649f0>] ? manage_workers+0x340/0x340
[<ffffffff8106cea6>] kthread+0xd6/0xe0
[<ffffffff8106cdd0>] ? __init_kthread_worker+0x70/0x70
[<ffffffff8146daac>] ret_from_fork+0x7c/0xb0
[<ffffffff8106cdd0>] ? __init_kthread_worker+0x70/0x70
mlx4_core: Initializing 0000:04:00.1
mlx4_core 0000:04:00.1: enabling device (0000 -> 0002)
mlx4_core 0000:04:00.1: Detected virtual function - running in slave mode
mlx4_core 0000:04:00.1: Sending reset
mlx4_core 0000:04:00.1: Got slave FLRed from Communication channel (ret:0x1)
mlx4_core 0000:04:00.1: slave is currently in themiddle of FLR.
retrying...(try num:1)
mlx4_core 0000:04:00.1: Communication channel is not idle.my toggle is 1
(cmd:0x0)
mlx4_core 0000:04:00.1: slave is currently in themiddle of FLR.
retrying...(try num:2)
[... repeated the same ...]
mlx4_core 0000:04:00.1: slave is currently in themiddle of FLR.
retrying...(try num:10)
mlx4_core 0000:04:00.1: Communication channel is not idle.my toggle is 1
(cmd:0x0)
mlx4_core 0000:04:00.1: slave driver version is not supported by the master
mlx4_core 0000:04:00.1: Communication channel is not idle.my toggle is 1
(cmd:0x0)
mlx4_core 0000:04:00.1: Failed to initialize slave
mlx4_core: probe of 0000:04:00.1 failed with error -5
mlx4_core 0000:04:00.0: Running in master mode
mlx4_core 0000:04:00.0: FW version 2.11.500 (cmd intf rev 3), max
commands 16
mlx4_core 0000:04:00.0: Catastrophic error buffer at 0x1f020, size 0x10,
BAR 0
mlx4_core 0000:04:00.0: Communication vector bar:2 offset:0x800
[... probing of PF continues ...]
mlx4_core 0000:04:00.0: Started init_resource_tracker: 80 slaves
mlx4_core 0000:04:00.0: irq 83 for MSI/MSI-X
mlx4_core 0000:04:00.0: irq 84 for MSI/MSI-X
mlx4_core 0000:04:00.0: irq 85 for MSI/MSI-X
mlx4_core 0000:04:00.0: irq 86 for MSI/MSI-X
mlx4_core 0000:04:00.0: NOP command IRQ test passed
[... probing of PF ends ...]
[... probing of VF done again ...]
mlx4_core: Initializing 0000:04:00.1
mlx4_core 0000:04:00.1: enabling device (0000 -> 0002)
mlx4_core 0000:04:00.1: Detected virtual function - running in slave mode
mlx4_core 0000:04:00.1: Sending reset
mlx4_core 0000:04:00.0: Received reset from slave:1
mlx4_core 0000:04:00.1: Sending vhcr0
[... probing of VF succeeds ...]
mlx4_core 0000:04:00.1: NOP command IRQ test passed
^ permalink raw reply
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Bjørn Mork @ 2013-03-05 15:03 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVOg9cVNYsRuAVkexwy4hvD_iJm_vARRksBAeiWppWnoXA@mail.gmail.com>
Ming Lei <ming.lei@canonical.com> writes:
> On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
>
>> Now, after inspecting wdm_suspend() which hides behind
>> info->subdriver->suspend(), I see that this is a completely theoretical
>> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>>
>> Which means that your change really is a noop(). I still don't like it,
>> because it gives the impression that errors from info->subdriver->suspend()
>> isn't dealt with.
>
> Yes, it needn't dealt with in system sleep, so it has the document benefit.
The document that the return code is ignored by forcing it to 0 and
always return success.
But am still not convinced this is correct in any way. AFAICS there is
no documentation stating that the drivers should care about what the USB
core use the return value for. Drivers are allowed to fail and the core
will ignore and clean up if necessary.
I still find any partial failures horrendous. If we are not going to
fail on errors, then we'll return success and pretend to be suspended.
> No, USB core does not handle it, and will always ignore the return
> failure from driver's suspend, see usb_suspend_both() for non-autosuspend
> case.
Right you are. Somehow I read the inverse. Sorry about that.
In any case, it ends up here and everythin will be OK, which I assume is
why it can ignore the errors:
/* If the suspend succeeded then prevent any more URB submissions
* and flush any outstanding URBs.
*/
} else {
udev->can_submit = 0;
for (i = 0; i < 16; ++i) {
usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
usb_hcd_flush_endpoint(udev, udev->ep_in[i]);
}
}
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-input" 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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 14:50 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <87d2veot6t.fsf@nemi.mork.no>
On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
>>> Ming Lei <ming.lei@canonical.com> writes:
>>>
>>>> If suspend callback fails in system sleep context, usb core will
>>>> ignore the failure and let system sleep go ahead further, so
>>>> this patch doesn't recover device under this situation.
>>>>
>>>> Cc: Bjørn Mork <bjorn@mork.no>
>>>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>>>> ---
>>>> drivers/net/usb/cdc_mbim.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
>>>> index 248d2dc..da83546 100644
>>>> --- a/drivers/net/usb/cdc_mbim.c
>>>> +++ b/drivers/net/usb/cdc_mbim.c
>>>> @@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
>>>>
>>>> if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>>>> ret = info->subdriver->suspend(intf, message);
>>>> - if (ret < 0)
>>>> + if (ret < 0 && PMSG_IS_AUTO(msg))
>>>> usbnet_resume(intf);
>>>>
>>>> error:
>>>
>>> NAK. We if the device cannot suspend, then it cannot do suspend halfways
>>
>> Sorry, what do you mean that the device cannot suspend?
>
>
> Now, after inspecting wdm_suspend() which hides behind
> info->subdriver->suspend(), I see that this is a completely theoretical
> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>
> Which means that your change really is a noop(). I still don't like it,
> because it gives the impression that errors from info->subdriver->suspend()
> isn't dealt with.
Yes, it needn't dealt with in system sleep, so it has the document benefit.
>
>> If you mean
>> usb_suspend_device(), its failure is still ignored, and generally it is OK
>> since the suspend action is driven by upstream port.
>
>
> I mean that we do two operations here: first we suspend usbnet, then we
> suspend wdm:
>
> ret = usbnet_suspend(intf, message);
> if (ret < 0)
> goto error;
> if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
> ret = info->subdriver->suspend(intf, message);
> if (ret < 0)
> usbnet_resume(intf);
> error:
> return ret;
>
>
> The case you are trying to modify is when the second fails after the
> first succeeded. You know suspend has already failed at this point. It
> is the implication of the error. Your only task is to decide what to do
> about that failure. You seem to think that you can fix it. You
> cannot. It already has failed. If you are going to fix that, then you
> have to go back to where it failed.
>
> So your next step if going down this route will naturally be looking
> into how you can prevent info->subdriver->suspend() from ever failing.
> Which will be easy as it won't. But assuming for this argument that it
> can fail, which I guess can be true for some of the serial driver
> callback errors etc you also fixed this way. I am pretty sure that when
> you look into those to make sure they never can fail, you will find
> another function you have to ensure never can fail.
>
> Forget it. suspend can and will fail. Deal with it in the upper
> layers. In fact, the USB core already does. If you think it doesn't
> then that's where you fix it.
No, USB core does not handle it, and will always ignore the return
failure from driver's suspend, see usb_suspend_both() for non-autosuspend
case.
>
>>> either. Whether the caller will ignore the error or not, is irrelevant.
>>
>> Anyway, usbnet_resume() can't be called to submit new URBs in
>> the failure path of system suspend, so the patch should be correct.
>
> I fail to see how this is any different from a composite device with
> e.g. a usbnet function and a serial function where suspending the serial
> function fails after successfully suspending the usbnet function.
> usb_suspend_both() will then resume the usbnet function and return the
> error.
No, usb_suspend_both() always ignores the suspend failure from drivers
and does not resume a non-root-hub device during system sleep.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" 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: BUG: IPv4: Attempt to release TCP socket in state 1
From: Eric Dumazet @ 2013-03-05 14:46 UTC (permalink / raw)
To: dormando; +Cc: Cong Wang, linux-kernel, netdev
In-Reply-To: <alpine.DEB.2.02.1303042138330.7811@localhost6.localdomain6>
On Mon, 2013-03-04 at 21:44 -0800, dormando wrote:
> No 3rd party modules. There's a tiny patch for controlling initcwnd from
> userspace and another one for the extra_free_kbytes tunable that I brought
> up in another thread. We've had the initcwnd patch in for a long time
> without trouble. The extra_free_kbytes tunable isn't even being used yet,
> so all that's doing is adding a 0 somewhere.
>
> Only two iptables rules loaded: global NOTRACK rules for PREROUTING/OUTPUT
> in raw.
>
> Kernel's as close to pristine as I can make it. We had the 10g patch in
> but I've dropped it.
> --
Hmm, I spent time on this bug report but found nothing.
Please post as much information as you can on your setup.
I see you use macvlan, bridge, so maybe there is a configuration issue
(and a kernel bug of course)
^ permalink raw reply
* Re: [Xen-devel] [PATCH 3/8] netback: get/put module along with vif connect/disconnect
From: Wei Liu @ 2013-03-05 14:44 UTC (permalink / raw)
To: David Vrabel
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, konrad.wilk@oracle.com, annie.li@oracle.com
In-Reply-To: <5135FC2E.9050705@citrix.com>
On Tue, 2013-03-05 at 14:07 +0000, David Vrabel wrote:
> On 05/03/13 13:30, Wei Liu wrote:
> > On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote:
> >> On 15/02/13 16:00, Wei Liu wrote:
> >>> If there is vif running and user unloads netback, guest's network interface
> >>> just mysteriously stops working. So we need to prevent unloading netback
> >>> module if there is vif running.
> >>
> >> It's not mysterious -- it is cleanly disconnected, and will reconnect
> >> when the module is reinserted.
> >>
> >
> > From a guest's POV, it just stops without any sign. This should be
> > prevented IMHO.
>
> This is a bug in the frontend or a bug in the backend failing to
> disconnect correctly.
>
> I posted a series of "xen-foofront: handle backend CLOSED without
> CLOSING" patches that may help here. (I didn't get applied to netfront
> for some reason.)
>
Any links? And the reason why it was not applied?
> Disabling module unload doesn't prevent this from happening away. You
> can always manually unbind the backend device from the xen-netback
> driver which has the same effect as unloading the module.
>
Yes, but that's not a normal use case.
> > Netback / netfront lose all states when netback is unloaded. And
> > netfront doesn't support reconfiguration at the moment. My guess is that
> > this is the reason why netback doesn't even have unload function at
> > first.
>
> If netfront cannot handle reconnect then that's a bug in the frontend or
> a bug in the backend xenbus code not setting up the reconnect correctly.
>
AFAICT, various frontends (hvc, fb, blk etc.) don't respond to Closing
Closed Reconfigur{ing,ed} XenbusState. Is it the "bug" you're referring
to?
> >> Being able to unload modules while they are in use is standard so I
> >> don't think this should be applied.
> >
> > I don't think this is true from a module dependency point of view - just
> > try to unload any in use module, rmmod / modprobe will give you a fatal
> > error.
>
> Try it with any other network interface driver and it will unload just fine.
>
I don't think we are talking about the same thing here...
If you unload a network module in Dom0, that's fine, because you lose
your interface in Dom0 as well. But for a DomU, frontend don't know
about this.
Wei.
> David
^ permalink raw reply
* Re: [Xen-devel] [PATCH 1/8] netback: don't bind kthread to cpu
From: David Vrabel @ 2013-03-05 14:42 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Wei Liu, netdev@vger.kernel.org, annie.li@oracle.com,
Ian Campbell, xen-devel@lists.xen.org
In-Reply-To: <20130305135649.GA2589@phenom.dumpdata.com>
On 05/03/13 13:56, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 05, 2013 at 01:30:10PM +0000, Wei Liu wrote:
>> On Mon, 2013-03-04 at 20:51 +0000, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Feb 15, 2013 at 04:00:02PM +0000, Wei Liu wrote:
>>>> The initialization process makes an assumption that the online cpus are
>>>> numbered from 0 to xen_netbk_group_nr-1, which is not always true.
>>>
>>> And xen_netbk_group_nr is num_online_cpus()?
>>>
>>
>> Yes.
>>
>>> So under what conditions does this change? Is this when the CPU hotplug
>>> is involved and the CPUs go offline? In which case should there be a
>>
>> Yes, the hotplug path.
>>
>>> CPU hotplug notifier to re-bind the workers are appropiate?
>
> ?
> Can't that option be explored?
I'm not sure binding netback threads to particular VCPUs is useful
without also binding the events to the corresponding VCPUs.
I would hope that the scheduler would tend to the correct behavior if
threads aren't bound, anyway.
David
^ permalink raw reply
* Re: tipc: MTU discovery
From: Erik Hugne @ 2013-03-05 14:18 UTC (permalink / raw)
To: Sebastian Pöhn; +Cc: netdev, jon.maloy, allan.stephens
In-Reply-To: <CAGUzgdKJ4pPdhx7VkGCGS8CynXgOrxhTu8dZ+s77C+WKrCcCeA@mail.gmail.com>
On Tue, Mar 05, 2013 at 01:43:29PM +0100, Sebastian Pöhn wrote:
> State Messages larger than 1500 which as used for the MTU negotiation
> do not appear in the TIPC stack. But I am able to seen them entering
> the correct device.
> Because of that no reply with the correct max_packet field set can be
> send and the negotiation will always end up at 1.5k.
>
> So my questions are:
> # Is TIPC meant to detect and use the MTU larger than 1.5k?
Yes, it should probe and detect MTU's up to ~66k
> # Why are the packets not passed to the TIPC stack?
TIPC just registers itself as a handler for ETH_P_TIPC through dev_add_pack.
If link mtu probes >1.5k are not passed to TIPC, it sounds to me that the NIC driver
is to blame. What NIC type and kernel version are you running?
Do you see any packet drops in ethtool statistics?
//E
^ permalink raw reply
* Re: [Xen-devel] [PATCH 3/8] netback: get/put module along with vif connect/disconnect
From: David Vrabel @ 2013-03-05 14:07 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, Ian Campbell,
konrad.wilk@oracle.com, annie.li@oracle.com
In-Reply-To: <1362490213.4748.24.camel@zion.uk.xensource.com>
On 05/03/13 13:30, Wei Liu wrote:
> On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote:
>> On 15/02/13 16:00, Wei Liu wrote:
>>> If there is vif running and user unloads netback, guest's network interface
>>> just mysteriously stops working. So we need to prevent unloading netback
>>> module if there is vif running.
>>
>> It's not mysterious -- it is cleanly disconnected, and will reconnect
>> when the module is reinserted.
>>
>
> From a guest's POV, it just stops without any sign. This should be
> prevented IMHO.
This is a bug in the frontend or a bug in the backend failing to
disconnect correctly.
I posted a series of "xen-foofront: handle backend CLOSED without
CLOSING" patches that may help here. (I didn't get applied to netfront
for some reason.)
Disabling module unload doesn't prevent this from happening away. You
can always manually unbind the backend device from the xen-netback
driver which has the same effect as unloading the module.
> Netback / netfront lose all states when netback is unloaded. And
> netfront doesn't support reconfiguration at the moment. My guess is that
> this is the reason why netback doesn't even have unload function at
> first.
If netfront cannot handle reconnect then that's a bug in the frontend or
a bug in the backend xenbus code not setting up the reconnect correctly.
>> Being able to unload modules while they are in use is standard so I
>> don't think this should be applied.
>
> I don't think this is true from a module dependency point of view - just
> try to unload any in use module, rmmod / modprobe will give you a fatal
> error.
Try it with any other network interface driver and it will unload just fine.
David
^ permalink raw reply
* Re: [Xen-devel] [PATCH 1/8] netback: don't bind kthread to cpu
From: Wei Liu @ 2013-03-05 14:04 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: wei.liu2, netdev@vger.kernel.org, annie.li@oracle.com,
Ian Campbell, xen-devel@lists.xen.org
In-Reply-To: <20130305135649.GA2589@phenom.dumpdata.com>
On Tue, 2013-03-05 at 13:56 +0000, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 05, 2013 at 01:30:10PM +0000, Wei Liu wrote:
> > On Mon, 2013-03-04 at 20:51 +0000, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Feb 15, 2013 at 04:00:02PM +0000, Wei Liu wrote:
> > > > The initialization process makes an assumption that the online cpus are
> > > > numbered from 0 to xen_netbk_group_nr-1, which is not always true.
> > >
> > > And xen_netbk_group_nr is num_online_cpus()?
> > >
> >
> > Yes.
> >
> > > So under what conditions does this change? Is this when the CPU hotplug
> > > is involved and the CPUs go offline? In which case should there be a
> >
> > Yes, the hotplug path.
> >
> > > CPU hotplug notifier to re-bind the workers are appropiate?
>
> ?
> Can't that option be explored?
Thinking about it. Should be doable.
Wei.
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 14:03 UTC (permalink / raw)
To: Oliver Neukum
Cc: Bjørn Mork, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Alan Stern, netdev, linux-usb, linux-input
In-Reply-To: <2867446.m8zodPstBd@linux-5eaq.site>
On Tue, Mar 5, 2013 at 9:28 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Tuesday 05 March 2013 21:08:09 Ming Lei wrote:
>> On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
>> > In other words, if we don't handle errors, there must be no errors,
>> > otherwise it doesn't matter what we do in the error case. We'd leave
>> > the problem to generic layers.
>>
>> Generic layers can't handle the driver's specific failure.
>
> We depend on stopping the HC forcing all devices into suspend.
> We know this is problematic. For example some disk enclosures
> need to flush cache. Fortunately for us this is done in the SCSI
> layer.
I mean only the driver can know its specific suspend failure
in its suspend callback, so it is reasonable to deal with it in its
resume() callback, suppose the policy to ignore suspend failure
isn't changed.
>
>> If driver records its suspend failure state in suspend(), resume()
>> should and can deal with it without much difficulty.
>
> Yes, but why bother? Either we can safely suspend in any state or
> we must not ignore errors.
In fact, for many drivers, its suspend() in system sleep just return 0,
for example, cdc_wcm/cdc_acm/hub, etc, so these drivers simply
ignore any failures from suspend() during system sleep.
For other drivers, if suspend() don't return success in system sleep,
it shouldn't try to recover the device, and its resume has to handle
the failure. Or do you have better approach? Suppose we keep
ignoring the suspend failure.
At least, I think URBs can't be submitted to device in its system
suspend failure path, which is generally one part of suspend
recovery for many usb drivers. Doing such recovery may confuse
resume() very much, and it is really wrong.
That is the inconsistency of handling system suspend between
usb drivers and usb core.
>
>> > Furthermore there is a small chance that although the device tree
>> > is walked, teh system suspend fails for another later reason that
>> > is not ignored. In that case the drivers need to do error recovery,
>> > albeit in resume().
>>
>> Yes, resume() need to handle the USB system suspend failure
>> either in normal resume or error recovery, both are basically same.
>
> In theory yes, in practice usually power is cut.
reset_resume() or rebind() can handle the power cut case, for example,
the suspend failure state can be ignored in reset_resume(), or rebind()
just forget previous suspend failure. Correct me if it is wrong.
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [Xen-devel] [PATCH 1/8] netback: don't bind kthread to cpu
From: Konrad Rzeszutek Wilk @ 2013-03-05 13:56 UTC (permalink / raw)
To: Wei Liu
Cc: netdev@vger.kernel.org, annie.li@oracle.com, Ian Campbell,
xen-devel@lists.xen.org
In-Reply-To: <1362490210.4748.23.camel@zion.uk.xensource.com>
On Tue, Mar 05, 2013 at 01:30:10PM +0000, Wei Liu wrote:
> On Mon, 2013-03-04 at 20:51 +0000, Konrad Rzeszutek Wilk wrote:
> > On Fri, Feb 15, 2013 at 04:00:02PM +0000, Wei Liu wrote:
> > > The initialization process makes an assumption that the online cpus are
> > > numbered from 0 to xen_netbk_group_nr-1, which is not always true.
> >
> > And xen_netbk_group_nr is num_online_cpus()?
> >
>
> Yes.
>
> > So under what conditions does this change? Is this when the CPU hotplug
> > is involved and the CPUs go offline? In which case should there be a
>
> Yes, the hotplug path.
>
> > CPU hotplug notifier to re-bind the workers are appropiate?
?
Can't that option be explored?
> >
> > >
> > > As we only need a pool of worker threads, simply don't bind them to specific
> > > cpus.
> >
> > OK. Is there another method of doing this? Are there patches to make the thread
> > try to be vCPU->guest affinite?
> >
>
> No, not at the moment.
>
>
> Wei.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Bjørn Mork @ 2013-03-05 13:46 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVMRpf0REYxfi1wtzUpXn2PTCeS6qNfYQP12tW8ca3Sp=g@mail.gmail.com>
Ming Lei <ming.lei@canonical.com> writes:
> On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>>
>>> If suspend callback fails in system sleep context, usb core will
>>> ignore the failure and let system sleep go ahead further, so
>>> this patch doesn't recover device under this situation.
>>>
>>> Cc: Bjørn Mork <bjorn@mork.no>
>>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>>> ---
>>> drivers/net/usb/cdc_mbim.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
>>> index 248d2dc..da83546 100644
>>> --- a/drivers/net/usb/cdc_mbim.c
>>> +++ b/drivers/net/usb/cdc_mbim.c
>>> @@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
>>>
>>> if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>>> ret = info->subdriver->suspend(intf, message);
>>> - if (ret < 0)
>>> + if (ret < 0 && PMSG_IS_AUTO(msg))
>>> usbnet_resume(intf);
>>>
>>> error:
>>
>> NAK. We if the device cannot suspend, then it cannot do suspend halfways
>
> Sorry, what do you mean that the device cannot suspend?
Now, after inspecting wdm_suspend() which hides behind
info->subdriver->suspend(), I see that this is a completely theoretical
discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
Which means that your change really is a noop(). I still don't like it,
because it gives the impression that errors from info->subdriver->suspend()
isn't dealt with.
> If you mean
> usb_suspend_device(), its failure is still ignored, and generally it is OK
> since the suspend action is driven by upstream port.
I mean that we do two operations here: first we suspend usbnet, then we
suspend wdm:
ret = usbnet_suspend(intf, message);
if (ret < 0)
goto error;
if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
ret = info->subdriver->suspend(intf, message);
if (ret < 0)
usbnet_resume(intf);
error:
return ret;
The case you are trying to modify is when the second fails after the
first succeeded. You know suspend has already failed at this point. It
is the implication of the error. Your only task is to decide what to do
about that failure. You seem to think that you can fix it. You
cannot. It already has failed. If you are going to fix that, then you
have to go back to where it failed.
So your next step if going down this route will naturally be looking
into how you can prevent info->subdriver->suspend() from ever failing.
Which will be easy as it won't. But assuming for this argument that it
can fail, which I guess can be true for some of the serial driver
callback errors etc you also fixed this way. I am pretty sure that when
you look into those to make sure they never can fail, you will find
another function you have to ensure never can fail.
Forget it. suspend can and will fail. Deal with it in the upper
layers. In fact, the USB core already does. If you think it doesn't
then that's where you fix it.
>> either. Whether the caller will ignore the error or not, is irrelevant.
>
> Anyway, usbnet_resume() can't be called to submit new URBs in
> the failure path of system suspend, so the patch should be correct.
I fail to see how this is any different from a composite device with
e.g. a usbnet function and a serial function where suspending the serial
function fails after successfully suspending the usbnet function.
usb_suspend_both() will then resume the usbnet function and return the
error.
Bjørn
^ permalink raw reply
* Re: [PATCH 1/8] netback: don't bind kthread to cpu
From: Wei Liu @ 2013-03-05 13:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, annie.li@oracle.com
In-Reply-To: <20130304205154.GB16762@phenom.dumpdata.com>
On Mon, 2013-03-04 at 20:51 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 15, 2013 at 04:00:02PM +0000, Wei Liu wrote:
> > The initialization process makes an assumption that the online cpus are
> > numbered from 0 to xen_netbk_group_nr-1, which is not always true.
>
> And xen_netbk_group_nr is num_online_cpus()?
>
Yes.
> So under what conditions does this change? Is this when the CPU hotplug
> is involved and the CPUs go offline? In which case should there be a
Yes, the hotplug path.
> CPU hotplug notifier to re-bind the workers are appropiate?
>
> >
> > As we only need a pool of worker threads, simply don't bind them to specific
> > cpus.
>
> OK. Is there another method of doing this? Are there patches to make the thread
> try to be vCPU->guest affinite?
>
No, not at the moment.
Wei.
^ permalink raw reply
* Re: [Xen-devel] [PATCH 3/8] netback: get/put module along with vif connect/disconnect
From: Wei Liu @ 2013-03-05 13:30 UTC (permalink / raw)
To: David Vrabel
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, konrad.wilk@oracle.com, annie.li@oracle.com
In-Reply-To: <5135C2C8.8030708@citrix.com>
On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote:
> On 15/02/13 16:00, Wei Liu wrote:
> > If there is vif running and user unloads netback, guest's network interface
> > just mysteriously stops working. So we need to prevent unloading netback
> > module if there is vif running.
>
> It's not mysterious -- it is cleanly disconnected, and will reconnect
> when the module is reinserted.
>
>From a guest's POV, it just stops without any sign. This should be
prevented IMHO.
Netback / netfront lose all states when netback is unloaded. And
netfront doesn't support reconfiguration at the moment. My guess is that
this is the reason why netback doesn't even have unload function at
first.
> Being able to unload modules while they are in use is standard so I
> don't think this should be applied.
>
I don't think this is true from a module dependency point of view - just
try to unload any in use module, rmmod / modprobe will give you a fatal
error.
The situation for netback is a bit different. The module dependency is
not within the same kernel, we can only do this by explicitly get/put
this module.
Wei.
^ permalink raw reply
* Re: [Xen-devel] [PATCH 2/8] netback: add module unload function
From: Wei Liu @ 2013-03-05 13:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, annie.li@oracle.com
In-Reply-To: <20130304205530.GC16762@phenom.dumpdata.com>
On Mon, 2013-03-04 at 20:55 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 15, 2013 at 04:00:03PM +0000, Wei Liu wrote:
> > Enable users to unload netback module. Users should make sure there is not vif
> > runnig.
>
> 'sure there are no vif's running.'
>
> Any way of making this VIF part be automatic? Meaning that netback
> can figure out if there are VIFs running and if so don't unload
> all of the parts and just mention that you are leaking memory.
>
> This looks quite dangerous - meaning if there are guests running and
> we for fun do 'rmmod xen_netback' it looks like we could crash dom0?
>
Dom0 will not crash. But as you suggested in a latter email, I should
move the get/put module patch before this one.
The rationale behind this patch is that if there's anything wrong
discovered inside netback, we can just migrate all VMs to a new host,
unload old netback, load new netback then migrate all VMs back. This
should be useful for both production and testing.
Wei.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > drivers/net/xen-netback/common.h | 1 +
> > drivers/net/xen-netback/netback.c | 18 ++++++++++++++++++
> > drivers/net/xen-netback/xenbus.c | 5 +++++
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> > index 9d7f172..35d8772 100644
> > --- a/drivers/net/xen-netback/common.h
> > +++ b/drivers/net/xen-netback/common.h
> > @@ -120,6 +120,7 @@ void xenvif_get(struct xenvif *vif);
> > void xenvif_put(struct xenvif *vif);
> >
> > int xenvif_xenbus_init(void);
> > +void xenvif_xenbus_exit(void);
> >
> > int xenvif_schedulable(struct xenvif *vif);
> >
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index db8d45a..de59098 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1761,5 +1761,23 @@ failed_init:
> >
> > module_init(netback_init);
> >
> > +static void __exit netback_exit(void)
> > +{
> > + int group, i;
> > + xenvif_xenbus_exit();
>
> You should check the return code of this function.
>
> > + for (group = 0; group < xen_netbk_group_nr; group++) {
> > + struct xen_netbk *netbk = &xen_netbk[group];
> > + for (i = 0; i < MAX_PENDING_REQS; i++) {
> > + if (netbk->mmap_pages[i])
> > + __free_page(netbk->mmap_pages[i]);
> > + }
> > + del_timer_sync(&netbk->net_timer);
> > + kthread_stop(netbk->task);
> > + }
> > + vfree(xen_netbk);
> > +}
> > +
> > +module_exit(netback_exit);
> > +
> > MODULE_LICENSE("Dual BSD/GPL");
> > MODULE_ALIAS("xen-backend:vif");
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> > index 410018c..65d14f2 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -485,3 +485,8 @@ int xenvif_xenbus_init(void)
> > {
> > return xenbus_register_backend(&netback_driver);
> > }
> > +
> > +void xenvif_xenbus_exit(void)
> > +{
> > + return xenbus_unregister_driver(&netback_driver);
> > +}
> > --
> > 1.7.10.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> >
^ permalink raw reply
* Re: [PATCH 2/8] netback: add module unload function
From: Wei Liu @ 2013-03-05 13:30 UTC (permalink / raw)
To: Stephen Hemminger
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, konrad.wilk@oracle.com, annie.li@oracle.com
In-Reply-To: <20130304135831.4d52eddc@nehalam.linuxnetplumber.net>
On Mon, 2013-03-04 at 21:58 +0000, Stephen Hemminger wrote:
> On Fri, 15 Feb 2013 16:00:03 +0000
> Wei Liu <wei.liu2@citrix.com> wrote:
>
> > Enable users to unload netback module. Users should make sure there is not vif
> > runnig.
>
>
> Isn't it likely that some admin might be trying to cleanup or do shutdown and there
> is an ordering problem. What happens if vif's are still running?
>
> Why depend on users? You should allow it anytime and do refcounting and auto-destroy the vif's for them.
Sure, we should not depend on users. I will move my vif get/put module
patch before this one.
Wei.
^ 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