Re: [113/197] x86, cacheinfo: Calculate L3 indices

[Jiri Kosina]

On Thu, 6 May 2010, Borislav Petkov wrote:

> > The point is though, that we really should be checking for return value of 
> > node_to_k8_nb_misc() as it can legitimately return NULL, can't it? (and 
> > other places, such as show_cache_disable() and store_cache_disable(), are 
> > checking for this being NULL properly).
> 
> Well, I don't like sprinkling NULL ptr checks all over the place - this
> makes the code unreadable and means that there's something wrong with
> its design in the first place.
> 
> Therefore, it is better IMO to verify whether the K8_NB thing is
> initialized properly and exit early if not. And this is what the two
> patches I'm suggesting try to accomplish:
> 
> 1. Call into K8_NB on AMD unconditionally so that we have those
> initialized (we'll be needing them more in the future, I guess)
> 
> 2. Check in the L3 cache code whether K8_NB has initted properly and
> bail out if not.
> 
> With this, no need for NULL ptr checks anywhere and you got
> better/smaller/more readable code with the proper assumptions in place.

Well, we have

	#ifdef CONFIG_K8_NB
	static inline struct pci_dev *node_to_k8_nb_misc(int node)
	{
		return (node < num_k8_northbridges) ? k8_northbridges[node] : NULL;
	}
	#else
	static inline struct pci_dev *node_to_k8_nb_misc(int node)
	{
		return NULL;
	}
	#endif

So it legitimately returns NULL in two cases:

	1) if someone passes too large node
	2) if CONFIG_K8_NB is unset

1) Either we assume that node will always be in the range (i.e. 
amd_get_nb_id() will never go crazy return anything bogus), and then we 
could just drop the test completely. Or we want to check for such the 
possibility, and then node_to_k8_nb_misc() is going to return NULL in such 
cases, and so we want to check for it.

2) is now moot, as all three in-tree callers are now under proper ifdefs 
(CONFIG_CPU_SUP_AMD, which depends on CONFIG_K8_NB after your patch has 
been applied). So I believe we could remove it, right?

Either way, current state seems inconsistent. So either we should add the 
return value check to amd_calc_l3_indices() as well, or remove all the 
NULL magic altogether, i.e. the (untested) patch below.

What do you think?


From: Jiri Kosina <jkosina@xxxxxxx>
Subject: x86, cacheinfo: remove unnecessary NULL pointer checks

As K8_NB thing is now always initialized on AMD CPUs, and we don't have 
any caller that would be outside CONFIG_CPU_SUP_AMD/CONFIG_K8_NB, it is 
safe to assume that kb_northbridges has been always initialized).

Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>

 arch/x86/include/asm/k8.h             |   10 +---------
 arch/x86/kernel/cpu/intel_cacheinfo.c |    6 ------
 2 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/k8.h b/arch/x86/include/asm/k8.h
index f70e600..0cff8ba 100644
--- a/arch/x86/include/asm/k8.h
+++ b/arch/x86/include/asm/k8.h
@@ -15,17 +15,9 @@ extern int k8_get_nodes(struct bootnode *nodes);
 extern int k8_numa_init(unsigned long start_pfn, unsigned long end_pfn);
 extern int k8_scan_nodes(void);
 
-#ifdef CONFIG_K8_NB
 static inline struct pci_dev *node_to_k8_nb_misc(int node)
 {
-	return (node < num_k8_northbridges) ? k8_northbridges[node] : NULL;
+	return k8_northbridges[node];
 }
-#else
-static inline struct pci_dev *node_to_k8_nb_misc(int node)
-{
-	return NULL;
-}
-#endif
-
 
 #endif /* _ASM_X86_K8_H */
diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index b3eeb66..6c5f3cd 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -355,9 +355,6 @@ static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
 	if (!this_leaf->can_disable)
 		return -EINVAL;
 
-	if (!dev)
-		return -EINVAL;
-
 	pci_read_config_dword(dev, 0x1BC + index * 4, &reg);
 	return sprintf(buf, "0x%08x\n", reg);
 }
@@ -388,9 +385,6 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!dev)
-		return -EINVAL;
-
 	if (strict_strtoul(buf, 10, &val) < 0)
 		return -EINVAL;
 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/