Re: [PATCH 2/2] Update ibm,client-architecture call field based on device tree

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2010-01-14 at 14:41 -0600, Joel Schopp wrote:
> In the previous patch the client-architecture field for the number of
> cores supported is set statically as high as is possible.  However, that
> static setting could be too high if the system supports smt, resulting
> in cpus assigned to Linux that are not booted.  This patch reads the
> device tree (before it is unflattened) to determine the amount of smt.
> It then dynamically updates the entires in the array with the proper
> number of cores supported.  Tests show this correctly detecting SMT4 on
> a Power7 and still booting all the supported cores on a large machine.

Same comments about submission format as the previous patch...

> Signed-off-by:Joel Schopp<jschopp@austin.ibm.com> 
> Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
> ===================================================================
> --- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
> +++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
> @@ -141,6 +141,8 @@ typedef u32 cell_t;
>  
>  extern void __start(unsigned long r3, unsigned long r4, unsigned long r5);
>  
> +static int __init prom_smt_way(void);

Do you really need a forward declaration ? We generally avoid those and
prefer instead having the functions in the right order to make it
unnecessary.

>  #ifdef CONFIG_PPC64
>  extern int enter_prom(struct prom_args *args, unsigned long entry);
>  #else
> @@ -811,9 +813,17 @@ static void __init prom_send_capabilitie
>  {
>  	ihandle elfloader, root;
>  	prom_arg_t ret;
> +	u32 *cores;
>  
>  	root = call_prom("open", 1, 1, ADDR("/"));
>  	if (root != 0) {
> +		/*
> +		 * If you add to the struct, please be sure the 100 index
> +		 * didn't change.  The BUILD_BUG_ON is a reminder.
> +		 */
> +		BUILD_BUG_ON(sizeof(ibm_architecture_vec) != 108);

This is indeed a bit fishy... a nicer way may have been to have the
vector in an asm file with labels but that's probably overkill. Just
maybe add a runtime test that checks the value read initially is
NR_CORES and if not, print a big fat warning ?

> +		cores = (u32 *) &ibm_architecture_vec[100];
> +		*cores = (u32) (NR_CPUS/prom_smt_way());

The style is a bit gross though I suppose it will do. Drop the cast on
the second line, it's not useful, and stick some spaces around that
division. Also maybe print out a message saying to what value you
adjusted the max number of supported cores, might be useful on the field
to diagnose issues.

>  		/* try calling the ibm,client-architecture-support method */
>  		prom_printf("Calling ibm,client-architecture-support...");
>  		if (call_prom_ret("call-method", 3, 2, &ret,
> @@ -1031,6 +1041,45 @@ static void __init reserve_mem(u64 base,
>  	RELOC(mem_reserve_cnt) = cnt + 1;
>  }
>  
> +
> +static int __init prom_smt_way(void)
> +{
> +	phandle node;
> +	char type[64];
> +	unsigned int plen;
> +
> +	for (node = 0; prom_next_node(&node); ) {
> +		type[0] = 0;
> +		prom_getprop(node, "device_type", type, sizeof(type));
> +
> +		if (type[0] == 0) {
> +			/*
> +			 * CHRP Longtrail machines have no device_type
> +			 * on the memory node, so check the name instead...
> +			 */
> +			prom_getprop(node, "name", type, sizeof(type));

They also have no SMT :-) Just continue instead. You could also have
instead gone for /cpus and peeked at the first child. Might have lead to
a simpler construct (and faster runtime)

> +		}
> +		if (strcmp(type, RELOC("cpu")))
> +			continue;
> +
> +		/*
> +		 * There is an entry for each smt thread, each entry being
> +		 * 4 bytes long.  All cpus should have the same number of
> +		 * smt threads, so return after finding the first.
> +		 */
> +		plen = prom_getproplen(node, "ibm,ppc-interrupt-server#s");
> +		prom_debug("smt %x\n", (unsigned long) plen);

Might only be a debug message but it should be a tad more verbose about
what it's actually printing.

> +		if (plen >= 4)
> +			return plen / 4;
> +	}
> +	/*
> +	 * If things go wrong and we get here fallback to SMT1
> +	 */
> +	prom_debug("unable to determine smt from device tree, guessing smt1\n");
> +	return 1;
> +
> +}
> +

Cheers,
Ben.

>  /*
>   * Initialize memory allocation mechanism, parse "memory" nodes and
>   * obtain that way the top of memory and RMO to setup out local allocator
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev