cpumask move patch - RFC

[R Sharada]

Hello,

On Thu, Aug 05, 2004 at 06:14:24PM -0500, Nathan Lynch wrote:
>
> On Tue, 2004-08-03 at 08:15, R Sharada wrote:
> > Hello,
> > 	This is part of an attempt to clean up some of the kernel data
> > structure initialisations from prom.c and move to later boot code.
>
> +void cpumask_setup()
> +{
> +       unsigned long ind;
> +       struct device_node *np = NULL;
> +       int cpuid = 0;
> +       unsigned int *reg = NULL;
> +       char *statusp = NULL;
> +       int *propsize = NULL;
>
> The reg, statusp, and propsize initializations seem unnecessary.

Don't we need to initialize the pointers for cleaniness sake? That was the only
idea behind setting them to NULL.

>
> +                       cpuid++;
> +               }
> +               of_node_put(np);
> +               return;
>
> Most of these of_node_put's are superfluous unless there's a chance you
> have explicitly broken out of the loop.

One question here though. The of_node_put() calls in the patch are actually out
of the while loop. I see that of_find_node_by_type() actually increments the
node->users via the of_node_get() call, and decrements for the parent node;
hence for the last node, we would still need to decrement the refcount, by
calling of_node_put explicitly outside of the while loop, is it not?
Or did I miss something?

>
> +               propsize = (int *)get_property(np, "ibm,ppc-interrupt-server#s", NULL);
> +               if (*propsize < 0) {
> +                       /* no property.  old hardware has no SMT */
> +                       cpu_threads = 1;
> +               } else {
> +                       /* We have a threaded processor */
> +                       cpu_threads = *propsize / sizeof(u32);
> +                       if (cpu_threads > 2)
> +                       cpu_threads = 1; /* ToDo: panic? */
>
> This is incorrect -- get_property does not return the size of the
> property; it stores the size in the third argument.  The return value of
> get_property is a pointer to the kernel's copy of the property itself.

Thanks and yes, that was my mistake. I will change this to read the property
size correctly from the get_property() call.

>
> While I agree in theory with removing all the cpumask initializations
> from prom_hold_cpus, I don't think simply transplanting the mess is the
> way to do it.  Wouldn't it be nice to have one loop which works on pmac
> and pSeries, SMP and UP, without all those #ifdef's?
>

I agree that what you suggest is the cleaner way to go instead of a plain copy.
Well, I could remove the #ifdef SMP in the code. However, as regards merging
pmac and pseries, the code for pmac does not seem to really check for the
cpu status, etc. Is it not needed on pmac? I am not too aware of pmac and need
to see the devicetree for pmac and understand if it is different from the
pseries tree.

> Nathan
>
>

I am working on the changes and will post a revised patch soon. I might need
to look a little more to merge the pseries and pmac stuff together.

Thanks and Regards,
Sharada

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/