On Sun, Jun 06, 2010 at 10:50:19PM -0400, James Bottomley wrote:
On Sun, 2010-06-06 at 16:10 -0700, mark gross wrote:
On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
[alsa-devel says it's a moderated list, so feel free to drop before replying]
Since every caller has to squirrel away the returned pointer anyway, they might as well supply the memory area. This fixes a bug in a few of the call sites where the returned pointer was dereferenced without checking it for NULL (which gets returned if the kzalloc failed).
I'd like to hear how sound and netdev feels about this: it will add about two more pointers worth of data to struct netdev and struct snd_pcm_substream .. but I think it's worth it. If you're OK, I'll add your acks and send through the pm tree.
This also looks to me like an android independent clean up (even though it renders the request_add atomically callable). I also added include guards to include/linux/pm_qos_params.h
James
drivers/net/e1000e/netdev.c | 17 ++++------ drivers/net/igbvf/netdev.c | 9 ++--- drivers/net/wireless/ipw2x00/ipw2100.c | 12 +++--- include/linux/netdevice.h | 2 +- include/linux/pm_qos_params.h | 12 +++++-- include/sound/pcm.h | 2 +- kernel/pm_qos_params.c | 55 ++++++++++++++++--------------- sound/core/pcm_native.c | 12 ++----- 8 files changed, 60 insertions(+), 61 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index 24507f3..47ea62f 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -2901,10 +2901,10 @@ static void e1000_configure_rx(struct e1000_adapter *adapter) * dropped transactions. */ pm_qos_update_request(
adapter->netdev->pm_qos_req, 55);
&adapter->netdev->pm_qos_req, 55);
adapter->netdev->pm_qos_req,
&adapter->netdev->pm_qos_req, PM_QOS_DEFAULT_VALUE);@@ -3196,9 +3196,9 @@ int e1000e_up(struct e1000_adapter *adapter)
/* DMA latency requirement to workaround early-receive/jumbo issue */ if (adapter->flags & FLAG_HAS_ERT)
adapter->netdev->pm_qos_req =pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,PM_QOS_DEFAULT_VALUE);
pm_qos_add_request(&adapter->netdev->pm_qos_req,PM_QOS_CPU_DMA_LATENCY,PM_QOS_DEFAULT_VALUE);/* hardware has been reset, we need to reload some things */ e1000_configure(adapter);
@@ -3263,11 +3263,8 @@ void e1000e_down(struct e1000_adapter *adapter) e1000_clean_tx_ring(adapter); e1000_clean_rx_ring(adapter);
- if (adapter->flags & FLAG_HAS_ERT) {
pm_qos_remove_request(adapter->netdev->pm_qos_req);adapter->netdev->pm_qos_req = NULL;- }
if (adapter->flags & FLAG_HAS_ERT)
pm_qos_remove_request(&adapter->netdev->pm_qos_req);/*
- TODO: for power management, we could drop the link and
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c index 5e2b2a8..5da569f 100644 --- a/drivers/net/igbvf/netdev.c +++ b/drivers/net/igbvf/netdev.c @@ -48,7 +48,7 @@ #define DRV_VERSION "1.0.0-k0" char igbvf_driver_name[] = "igbvf"; const char igbvf_driver_version[] = DRV_VERSION; -struct pm_qos_request_list *igbvf_driver_pm_qos_req; +struct pm_qos_request_list igbvf_driver_pm_qos_req; static const char igbvf_driver_string[] = "Intel(R) Virtual Function Network Driver"; static const char igbvf_copyright[] = "Copyright (c) 2009 Intel Corporation."; @@ -2902,8 +2902,8 @@ static int __init igbvf_init_module(void) printk(KERN_INFO "%s\n", igbvf_copyright);
ret = pci_register_driver(&igbvf_driver);
- igbvf_driver_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
PM_QOS_DEFAULT_VALUE);
- pm_qos_add_request(igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
should be:
- pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
Yes, thanks ... must be not compiling this driver for some reason in the test case.
I think we need to do better with the file gard or fix e100e/netdev.c to avoid panicking when un-loading the e1000e driver.
I'm on the fence whether to address the e1000e brain damage, or put in protection from whatever is causing the crash. (which funny enough I can't get a serial console to that crashing system to share it with you. but I'm pretty sure an un-initialized netdev->pm_qos_request is getting passed in (with class==0) resulting in the de-referencing of NULL in the update_target function.
I'll be other abusers are doing something similar we'll need to guard again. Would you like me to send an updated patch with some of this guarding in place?
--mgross