Prev: ACPI / PM: Do not enable GPEs for system wakeup in advance
Next: per_cpu_ptr_to_phys() failure on UV x86_64
From: Don Zickus on 21 Jun 2010 09:30
On Sat, Jun 19, 2010 at 10:30:56AM +0100, Alan Cox wrote:
> > Typically, one would push a config patch to enable and disable the feature and
> > patch the distribution. However, in some cases this is not feasible in order
>
> If you can push a patch to set the flag you can push a patch to panic or
> reject that combination.

We could, but we didn't think mainline would interested in restricting
what they support based on RHEL's needs. Also we still wanted to retain
the ability to use a piece of hardware even though we do not officially
support it, thus we were trying to avoid the panic and just set a flag
instead.

>
> Devil's advocate time:
>
> Also the fact some distributions chose a binary compatible interface for
> their internal modules was their choice. It is one that has been
> repeatedly rejected by upstream and at kernel summit.
>
> So given we fundamnetally reject your approach why should we carry your
> flag ?

I'm confused, this has nothing to do with KABI. It is just a flag that
something like the installer or a system report could look at to inform
users they are running on a system that may not be supported.

Again, we thought this might be useful for other distros as well who want
to make it easier to filter through bugzillas as close out bugs that has
this flag set.

>
> > In some cases the distribution may want to allow booting of these features but
> > explicitly notify a user that they are not "officially" supported. It is also
>
> We have printk. You can add a module of your own which indicates
> 'support' status too.

>
> > possible that the hardware is fixed via a firmware update at a later date,
> > making it supported again.
>
> IMHO it's not properly named in the first place. You are talking about
> combinations of hardware/firmware and you actually mean 'configuration
> not supported' ?

Mainly hardware platforms that do not necessarily have a config option
wrapped around them.

>
> > This patch introduces the TAINT_HARDWARE_UNSUPPORTED flag for distributions
> > to use.
>
> and why KERN_CRIT when the other printk's don't use that level ?

The thinking was that we wanted to make sure the end user saw the
message, but we can set it to any level really.

>
> A suggestion: instead of all this push a single patch with a comment and
> maybe defines indicating that taint flag bits 28-31 are 'reserved' for
> experimental and out of tree applications.
>
> That way anyone who has a requirement like yours can deal with it and
> nobody has to worry about bit collisions, naming and the like. Nor if you
> suddenely need an extra bit in 3 years time are you going to come unstuck
> on your KABI. That will help other people doing experiments with taint or
> with differing needs to the Red Hat one.

That seems like a reasonable request and basically covers one of the
reasons for pushing this patch.

Thanks for the review.

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 21 Jun 2010 15:30
On Thu, 17 Jun 2010 15:54:09 -0400
Prarit Bhargava <prarit(a)redhat.com> wrote:

>
> This patch is similar to Theordore Ts'o's TAINT_USER patch,
> linux-2.6 commit 34f5a39899f3f3e815da64f48ddb72942d86c366.
>
> Individual distributions may enable "generic" features such as X86 support,
> PPC support, and driver support.
>
> Some of the features that are enabled by these "generic" feature flags may
> not be considered supported by the individual distribution.
>
> For example, a distribution may want to support PPC but not the Power5
> chipset, or the e1000e driver but not a card with a specific DeviceID because
> of known firmware issues.
>
> Typically, one would push a config patch to enable and disable the feature and
> patch the distribution. However, in some cases this is not feasible in order
> to preserve kabi and at the same time maintain parity with the upstream kernel.
> In some cases the distribution may want to allow booting of these features but
> explicitly notify a user that they are not "officially" supported. It is also
> possible that the hardware is fixed via a firmware update at a later date,
> making it supported again.
>
> It would be useful for a distribution to notify the installer and
> bug reporting applications, and notify users that the hardware they are using
> is unsupported during panic, oops, BUG(), and WARN().
>
> This patch introduces the TAINT_HARDWARE_UNSUPPORTED flag for distributions
> to use.
>
> Signed-off-by: Prarit Bhargava <prarit(a)redhat.com>
> Signed-off-by: Don Zickus <dzickus(a)redhat.com>
>
> diff --git a/Documentation/oops-tracing.txt b/Documentation/oops-tracing.txt
> index 6fe9001..e337b0a 100644
> --- a/Documentation/oops-tracing.txt
> +++ b/Documentation/oops-tracing.txt
> @@ -263,6 +263,8 @@ characters, each representing a particular tainted value.
> 12: 'I' if the kernel is working around a severe bug in the platform
> firmware (BIOS or similar).
>
> + 13: 'H' if the hardware is unsupported by the distribution
> +
> The primary reason for the 'Tainted: ' string is to tell kernel
> debuggers if this is a clean kernel or if anything unusual has
> occurred. Tainting is permanent: even if an offending module is
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 8317ec4..f722b0d 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -347,6 +347,7 @@ extern enum system_states {
> #define TAINT_WARN 9
> #define TAINT_CRAP 10
> #define TAINT_FIRMWARE_WORKAROUND 11
> +#define TAINT_HARDWARE_UNSUPPORTED 12
>
> extern void dump_stack(void) __cold;
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 3b16cd9..8d081ff 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -180,6 +180,7 @@ static const struct tnt tnts[] = {
> { TAINT_WARN, 'W', ' ' },
> { TAINT_CRAP, 'C', ' ' },
> { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' },
> + { TAINT_HARDWARE_UNSUPPORTED, 'H', ' ' },
> };
>
> /**
> @@ -197,6 +198,7 @@ static const struct tnt tnts[] = {
> * 'W' - Taint on warning.
> * 'C' - modules from drivers/staging are loaded.
> * 'I' - Working around severe firmware bug.
> + * 'H' - Hardware is unsupported.
> *
> * The string is overwritten by the next call to print_tainted().
> */
> @@ -243,6 +245,9 @@ void add_taint(unsigned flag)
> */
> if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
> printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
> + if (flag == TAINT_HARDWARE_UNSUPPORTED)
> + printk(KERN_CRIT
> + "WARNING: This system's hardware is unsupported.\n");
>
> set_bit(flag, &tainted_mask);
> }

That's pretty user-hostile. What are they to do - throw the entire
computer away because it has the wrong type of mouse?

How about

void add_hardware_unsupported_taint(const char *hardware)
{
printk(KERN_CRIT
"Hardware device %s is unsupported by this kernel's vendor\n",
hardware);
add_taint(TAINT_HARDWARE_UNSUPPORTED);
}

and

/*
* Don't call add_taint(TAINT_HARDWARE_UNSUPPORTED) directly - use
* add_hardware_unsupported_taint()
*/
#define TAINT_HARDWARE_UNSUPPORTED 12

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Don Zickus on 21 Jun 2010 15:50
On Mon, Jun 21, 2010 at 12:21:45PM -0700, Andrew Morton wrote:
> > @@ -243,6 +245,9 @@ void add_taint(unsigned flag)
> > */
> > if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
> > printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
> > + if (flag == TAINT_HARDWARE_UNSUPPORTED)
> > + printk(KERN_CRIT
> > + "WARNING: This system's hardware is unsupported.\n");
> >
> > set_bit(flag, &tainted_mask);
> > }
>
> That's pretty user-hostile. What are they to do - throw the entire
> computer away because it has the wrong type of mouse?
>
> How about
>
> void add_hardware_unsupported_taint(const char *hardware)
> {
> printk(KERN_CRIT
> "Hardware device %s is unsupported by this kernel's vendor\n",
> hardware);
> add_taint(TAINT_HARDWARE_UNSUPPORTED);
> }
>
> and
>
> /*
> * Don't call add_taint(TAINT_HARDWARE_UNSUPPORTED) directly - use
> * add_hardware_unsupported_taint()
> */
> #define TAINT_HARDWARE_UNSUPPORTED 12

Internally here that is how we planned on using the flag. We weren't sure
if upstream would accept that piece or not, so we started with the flag
for now to at least reserve the bit.

But I can see your point about the other printk being a little
user-hostile if someone forgot to call the wrapper function.

I can respin with the wrapper function (Prarit is on vacation) or go with
Alan's idea of just reserving the upper few bits.

Thoughts?

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 21 Jun 2010 16:10
On Mon, 21 Jun 2010 15:45:59 -0400
Don Zickus <dzickus(a)redhat.com> wrote:

> On Mon, Jun 21, 2010 at 12:21:45PM -0700, Andrew Morton wrote:
> > > @@ -243,6 +245,9 @@ void add_taint(unsigned flag)
> > > */
> > > if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
> > > printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
> > > + if (flag == TAINT_HARDWARE_UNSUPPORTED)
> > > + printk(KERN_CRIT
> > > + "WARNING: This system's hardware is unsupported.\n");
> > >
> > > set_bit(flag, &tainted_mask);
> > > }
> >
> > That's pretty user-hostile. What are they to do - throw the entire
> > computer away because it has the wrong type of mouse?
> >
> > How about
> >
> > void add_hardware_unsupported_taint(const char *hardware)
> > {
> > printk(KERN_CRIT
> > "Hardware device %s is unsupported by this kernel's vendor\n",
> > hardware);
> > add_taint(TAINT_HARDWARE_UNSUPPORTED);
> > }
> >
> > and
> >
> > /*
> > * Don't call add_taint(TAINT_HARDWARE_UNSUPPORTED) directly - use
> > * add_hardware_unsupported_taint()
> > */
> > #define TAINT_HARDWARE_UNSUPPORTED 12
>
> Internally here that is how we planned on using the flag. We weren't sure
> if upstream would accept that piece or not, so we started with the flag
> for now to at least reserve the bit.
>
> But I can see your point about the other printk being a little
> user-hostile if someone forgot to call the wrapper function.
>
> I can respin with the wrapper function (Prarit is on vacation)

Not telling the user _which_ hardware is causing the problem is daft.

> or go with
> Alan's idea of just reserving the upper few bits.

um. Given that the vendor needs to patch the individual drivers to
indicate ther unsupported status, why not just patch in the taint
changes as well?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Don Zickus on 21 Jun 2010 16:50
On Mon, Jun 21, 2010 at 01:00:08PM -0700, Andrew Morton wrote:
> > Internally here that is how we planned on using the flag. We weren't sure
> > if upstream would accept that piece or not, so we started with the flag
> > for now to at least reserve the bit.
> >
> > But I can see your point about the other printk being a little
> > user-hostile if someone forgot to call the wrapper function.
> >
> > I can respin with the wrapper function (Prarit is on vacation)
>
> Not telling the user _which_ hardware is causing the problem is daft.
>
> > or go with
> > Alan's idea of just reserving the upper few bits.
>
> um. Given that the vendor needs to patch the individual drivers to
> indicate ther unsupported status, why not just patch in the taint
> changes as well?

Well we wanted to secure a bit flag first, so we don't bite ourselves later and
have to change the userspace tools.

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: ACPI / PM: Do not enable GPEs for system wakeup in advance
Next: per_cpu_ptr_to_phys() failure on UV x86_64