From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Lalancette Subject: Re: [PATCH]: 8139cp: Don't blindly enable interrupts in cp_start_xmit Date: Tue, 16 Jan 2007 09:44:06 -0500 Message-ID: <45ACE4B6.3040809@redhat.com> References: <45ABAE69.4070105@redhat.com> <20070115195635.GA14205@electric-eye.fr.zoreil.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: jgarzik@pobox.com, netdev@vger.kernel.org, Herbert Xu , Ingo Molnar Return-path: Received: from mx1.redhat.com ([66.187.233.31]:47229 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751155AbXAPOoh (ORCPT ); Tue, 16 Jan 2007 09:44:37 -0500 To: Francois Romieu In-Reply-To: <20070115195635.GA14205@electric-eye.fr.zoreil.com> Sender: netdev-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Francois Romieu wrote: > Chris Lalancette : > [...] > >> Similar to this commit: >> >>http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=d15e9c4d9a75702b30e00cdf95c71c88e3f3f51e >> >>It's not safe in cp_start_xmit to blindly call spin_lock_irq and then >>spin_unlock_irq, since it may very well be the case that cp_start_xmit >>was called with interrupts already disabled (I came across this bug in >>the context of netdump in RedHat kernels, but the same issue holds, for >>example, in netconsole). Therefore, replace all instances of spin_lock_irq >>and spin_unlock_irq with spin_lock_irqsave and spin_unlock_irqrestore, >>respectively, in cp_start_xmit(). I tested this against a fully-virtualized >>Xen guest, which happens to use the 8139cp driver to talk to the emulated >>hardware. I don't have a real piece of 8139cp hardware to test on, so >>someone else will have to do that. > > > (message reformated to fit in 80 columns, please fix your mailer) > > As I understand http://lkml.org/lkml/2006/12/12/239, something like the > patch below should had been sent instead. Herbert, ack/nak ? > Francois, Thanks for the comments. While the patch you sent will help, there are still other places that will have problems. For example, in netpoll_send_skb, we call local_irq_save(flags), then call dev->hard_start_xmit(), and then call local_irq_restore(flags). This is a similar situation to what I described above; we will re-enable interrupts in cp_start_xmit(), when netpoll_send_skb doesn't expect that, and will probably run into issues. Is there a problem with changing cp_start_xmit to use the spin_lock_irqsave(), besides the extra instructions it needs? Thanks, Chris Lalancette