On 03-01-2013 星期五 07:51, Greg Rose wrote: > On Thu, 28 Feb 2013 15:02:07 +0800 > xunleer <xunleer.li@xxxxxxxxxx> wrote: > >> This patch fix an oops issue caused by NULL pointer. The call stack >> may be like this: >> [<ffffffffa04dae0b>] ixgbevf_down+0x10b/0x300 [ixgbevf] >> [<ffffffffa04dc6bd>] ixgbevf_open+0x27d/0x2b0 [ixgbevf] >> [<ffffffff813866b7>] __dev_open+0xa7/0x100 >> [<ffffffff81386745>] dev_open+0x35/0x60 >> >> When opening the net device, we are trying to request msix irqs, >> if failed, the error procedure released the msix entries and disabled >> the MSI-X interrupts in pci level. However, if the request is not >> succeeded, the ixgbevf_down is called, it synchronizes the irq by >> referring to the msix_entries, it may cause oops as to NULL pointer >> references. >> >> The msix_entries may be allocated in probe procedure and be released >> in remove routine may be the best way. So we removed this error >> processing action. Then we may try to reopen the device, all we must >> reload the driver. > > Li, > > we appreciate the patch you sent and after review have concluded that > it appears to be the right thing to do. However we would like to > modify the description of the patch to improve clarity. How about the > following description? > > ---------- > > When the ixgbevf driver is opened the request to allocate MSIX irq > vectors may fail. In that case the driver will call ixgbevf_down() > which will call ixgbevf_irq_disable() to clear the HW interrupt > registers and calls synchronize_irq() using the msix_entries pointer in > the adapter structure. However, when the function to request the MSIX > irq vectors failed it had already freed the msix_entries which causes > an OOPs from using the NULL pointer in synchronize_irq(). > > The calls to pci_disable_msix() and to free the msix_entries memory > should not occur if device open fails. Instead they should be called > during device driver removal to balance with the call to > pci_enable_msix() and the call to allocate msix_entries memory > during the device probe and driver load. > > ---------- > > If that description works for you then please resubmit the patch and > we will add it to our queue for validation and testing. Or if you would > like to change the description in some other way let me know and we'll > work on it. > > Thanks, > > - Greg Rose > Networking Division > Intel Corp. > >> >> Signed-off-by: Li Xun <xunleer.li@xxxxxxxxxx> >> --- >> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c >> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c index >> c3db6cd..f176208 100644 --- >> a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +++ >> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c @@ -944,9 +944,6 >> @@ free_queue_irqs: free_irq(adapter->msix_entries[vector].vector, >> adapter->q_vector[vector]); >> } >> - pci_disable_msix(adapter->pdev); >> - kfree(adapter->msix_entries); >> - adapter->msix_entries = NULL; >> return err; >> } >> > > > . > Greg, thanks for your advice, that's what exactly what i want to say, i'll resubmit the patch. it's my first time to do this, thanks again. sincerely, Li Xun -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html