Messages in this thread | | | Date | Wed, 19 Aug 2009 16:51:57 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 3/10] x86: introduce a set of platform feature flags |
| |
Jacob,
On Thu, 16 Jul 2009, Pan, Jacob jun wrote:
> >From 697d29ca621fffa63bc0bac022861f582d1905a6 Mon Sep 17 00:00:00 2001 > From: Jacob Pan <jacob.jun.pan@intel.com> > Date: Thu, 11 Jun 2009 09:37:26 -0700 > Subject: [PATCH] x86: introduce a set of platform feature flags > > This patch introduces a set of x86 pc platform feature flags. the intention is > to clean up setup code based on the availability of patform features.
Hmm, if it would be used to cleanup the setup code, fine. But as I can see in the later patches it just adds lots of conditionals based on that feature flags.
> +#ifndef _ASM_X86_PLATFORM_FEATURE_H > +#define _ASM_X86_PLATFORM_FEATURE_H > + > +#ifndef __ASSEMBLY__ > +#include <linux/bitops.h> > +#endif > +#include <asm/required-features.h>
Why does this header require asm/required-features.h ?
> diff --git a/arch/x86/kernel/mkx86pcflags.pl b/arch/x86/kernel/mkx86pcflags.pl > new file mode 100644 > index 0000000..19c13aa > --- /dev/null > +++ b/arch/x86/kernel/mkx86pcflags.pl > @@ -0,0 +1,32 @@ > +#!/usr/bin/perl > +# > +# Generate the x86_platform_available_feature[] array from arch/x86/include/asm/platform_feature.h
What's the purpose of this ? Creating an array of strings ?
> +static ssize_t > +sysfs_show_available_platform_feature(struct sys_device *dev, > + struct sysdev_attribute *attr, char *buf) > +{ > + ssize_t count = 0; > + int i; > + > + for (i = 0; i < 32*N_PLATFORM_CAPINTS; i++) { > + if (x86_platform_available_feature[i] != NULL) { > + count += snprintf(buf + count, > + max((ssize_t)PAGE_SIZE - count, > + (ssize_t)0), "%s ", > + x86_platform_available_feature[i]); > + } > + }
Is this really worth an extra sysfs machinery ? IMO it's sufficient to printk the hex value of the set feature bits once during boot.
> +/* > + * Initialize a set of default feature flags based on subarch IDs > + * Currently, only MRST platform has non-X86 PC standard feature set. > + */ > +void platform_feature_init_default(void) > +{ > + int subarch_id = boot_params.hdr.hardware_subarch; > + > + if ((subarch_id >= 0) && (subarch_id < N_X86_SUBARCHS)) { > + if (subarch_id == X86_SUBARCH_MRST) { > + setup_mrst_default_feature(); > + return; > + } > + } else { > + printk(KERN_INFO "Use default X86 platform feature set\n"); > + } > + /* platforms such as Moorestown and paravirt should be filtered out */ > + if (platform_has(X86_PLATFORM_FEATURE_BIOS)) > + x86_quirks->reserve_ebda_region = reserve_ebda_region;
I don't like that at all. We do not want tons of places where we init parts of the setup depending on different platforms. platform setup has to be done in a platform file and there we override quirks or platform functions. Not on some random place.
Thanks,
tglx
| |