On Tue, 2011-04-19 at 13:31 -0700, Greg KH wrote:
> 2.6.32-longterm review patch. If anyone has any objections, please let us know.
>
> ------------------
>
> From: Dmitry Torokhov <[email protected]>
>
> commit 5a6c2f3ff039154872ce597952f8b8900ea0d732 upstream.
>
> Macro arguments used in expressions need to be enclosed in parenthesis
> to avoid unpleasant surprises.

Do you know of any specific uses of these macros where the missing
parentheses caused 'unpleasant surprises'?

Ben.

> This should be queued for kernels back to 2.6.31
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Sarah Sharp <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> ---
> drivers/usb/host/xhci.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -232,7 +232,7 @@ struct xhci_op_regs {
> * notification type that matches a bit set in this bit field.
> */
> #define DEV_NOTE_MASK (0xffff)
> -#define ENABLE_DEV_NOTE(x) (1 << x)
> +#define ENABLE_DEV_NOTE(x) (1 << (x))
> /* Most of the device notification types should only be used for debug.
> * SW does need to pay attention to function wake notifications.
> */
> @@ -579,11 +579,11 @@ struct xhci_ep_ctx {
> #define EP_STATE_STOPPED 3
> #define EP_STATE_ERROR 4
> /* Mult - Max number of burtst within an interval, in EP companion desc. */
> -#define EP_MULT(p) ((p & 0x3) << 8)
> +#define EP_MULT(p) (((p) & 0x3) << 8)
> /* bits 10:14 are Max Primary Streams */
> /* bit 15 is Linear Stream Array */
> /* Interval - period between requests to an endpoint - 125u increments. */
> -#define EP_INTERVAL(p) ((p & 0xff) << 16)
> +#define EP_INTERVAL(p) (((p) & 0xff) << 16)
> #define EP_INTERVAL_TO_UFRAMES(p) (1 << (((p) >> 16) & 0xff))
>
> /* ep_info2 bitmasks */
>
>
> _______________________________________________
> Stable-review mailing list
> [email protected]
> http://linux.kernel.org/mailman/listinfo/stable-review
>

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2011-04-20 02:17:50

On Tue, 2011-04-19 at 20:11 -0700, Greg KH wrote:
> On Wed, Apr 20, 2011 at 03:17:42AM +0100, Ben Hutchings wrote:
> > On Tue, 2011-04-19 at 19:01 -0700, Greg KH wrote:
> > > On Wed, Apr 20, 2011 at 02:40:53AM +0100, Ben Hutchings wrote:
> > > > On Tue, 2011-04-19 at 13:30 -0700, Greg KH wrote:
> > > > > 2.6.32-longterm review patch. If anyone has any objections, please let us know.
> > > > >
> > > > > ------------------
> > > > >
> > > > > From: Hans Rosenfeld <[email protected]>
> > > > >
> > > > > commit 9d8888c2a214aece2494a49e699a097c2ba9498b upstream.
> > > > >
> > > > > Remove check_c1e_idle() and use the new AMD errata checking framework
> > > > > instead.
> > > >
> > > > Clean-up patches are generally not candidates for longterm updates.
> > >
> > > This was added because a follow-on patch required it.
> >
> > Ah yes, 'x86, AMD: Set ARAT feature on AMD processors' is using the same
> > condition.
> >
> > Of course, that could have been backported by referring to the function
> > that this removes, rather than pulling in a load of other changes with
> > consequent risk of regressions.
>
> I prefer to take original patches for stable, it makes it easier in the
> end.

It makes what easier, when? What I see here is a bug fix that is much
larger than necessary, with a consequent risk of regression that seems
way out of proportion to the benefit. (What actually *is* the benefit
of these AMD changes?) And we have had several serious regressions in
the 2.6.32.y series recently, so I really don't think we are getting the
trade-off right.

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2011-04-20 05:05:19

by Greg KH

[permalink] [raw]

Subject: Re: [Stable-review] [12/28] x86, cpu: Clean up AMD erratum 400 workaround

On Wed, Apr 20, 2011 at 05:48:30AM +0100, Ben Hutchings wrote:
> On Tue, 2011-04-19 at 20:11 -0700, Greg KH wrote:
> > On Wed, Apr 20, 2011 at 03:17:42AM +0100, Ben Hutchings wrote:
> > > On Tue, 2011-04-19 at 19:01 -0700, Greg KH wrote:
> > > > On Wed, Apr 20, 2011 at 02:40:53AM +0100, Ben Hutchings wrote:
> > > > > On Tue, 2011-04-19 at 13:30 -0700, Greg KH wrote:
> > > > > > 2.6.32-longterm review patch. If anyone has any objections, please let us know.
> > > > > >
> > > > > > ------------------
> > > > > >
> > > > > > From: Hans Rosenfeld <[email protected]>
> > > > > >
> > > > > > commit 9d8888c2a214aece2494a49e699a097c2ba9498b upstream.
> > > > > >
> > > > > > Remove check_c1e_idle() and use the new AMD errata checking framework
> > > > > > instead.
> > > > >
> > > > > Clean-up patches are generally not candidates for longterm updates.
> > > >
> > > > This was added because a follow-on patch required it.
> > >
> > > Ah yes, 'x86, AMD: Set ARAT feature on AMD processors' is using the same
> > > condition.
> > >
> > > Of course, that could have been backported by referring to the function
> > > that this removes, rather than pulling in a load of other changes with
> > > consequent risk of regressions.
> >
> > I prefer to take original patches for stable, it makes it easier in the
> > end.
>
> It makes what easier, when? What I see here is a bug fix that is much
> larger than necessary, with a consequent risk of regression that seems
> way out of proportion to the benefit. (What actually *is* the benefit
> of these AMD changes?) And we have had several serious regressions in
> the 2.6.32.y series recently, so I really don't think we are getting the
> trade-off right.

We got a few new quirks added for AMD hardware platforms that fix
problems. It took 3 patches to get there, yes, but now, as time goes
on, adding new ones is even easier as the .32 code matches the .39 code
in this area due to these patches being added.

Now if you find problems in these, great, let me know and I will work to
resolve them.

As for regressions, what are you referring to?

thanks,

greg k-h

2011-04-20 05:51:25

by Willy Tarreau

[permalink] [raw]

Subject: Re: [Stable-review] [12/28] x86, cpu: Clean up AMD erratum 400 workaround

On Tue, Apr 19, 2011 at 10:06:17PM -0700, Greg KH wrote:
> On Wed, Apr 20, 2011 at 05:48:30AM +0100, Ben Hutchings wrote:
> > On Tue, 2011-04-19 at 20:11 -0700, Greg KH wrote:
> > > On Wed, Apr 20, 2011 at 03:17:42AM +0100, Ben Hutchings wrote:
> > > > On Tue, 2011-04-19 at 19:01 -0700, Greg KH wrote:
> > > > > On Wed, Apr 20, 2011 at 02:40:53AM +0100, Ben Hutchings wrote:
> > > > > > On Tue, 2011-04-19 at 13:30 -0700, Greg KH wrote:
> > > > > > > 2.6.32-longterm review patch. If anyone has any objections, please let us know.
> > > > > > >
> > > > > > > ------------------
> > > > > > >
> > > > > > > From: Hans Rosenfeld <[email protected]>
> > > > > > >
> > > > > > > commit 9d8888c2a214aece2494a49e699a097c2ba9498b upstream.
> > > > > > >
> > > > > > > Remove check_c1e_idle() and use the new AMD errata checking framework
> > > > > > > instead.
> > > > > >
> > > > > > Clean-up patches are generally not candidates for longterm updates.
> > > > >
> > > > > This was added because a follow-on patch required it.
> > > >
> > > > Ah yes, 'x86, AMD: Set ARAT feature on AMD processors' is using the same
> > > > condition.
> > > >
> > > > Of course, that could have been backported by referring to the function
> > > > that this removes, rather than pulling in a load of other changes with
> > > > consequent risk of regressions.
> > >
> > > I prefer to take original patches for stable, it makes it easier in the
> > > end.
> >
> > It makes what easier, when? What I see here is a bug fix that is much
> > larger than necessary, with a consequent risk of regression that seems
> > way out of proportion to the benefit. (What actually *is* the benefit
> > of these AMD changes?) And we have had several serious regressions in
> > the 2.6.32.y series recently, so I really don't think we are getting the
> > trade-off right.
>
> We got a few new quirks added for AMD hardware platforms that fix
> problems. It took 3 patches to get there, yes, but now, as time goes
> on, adding new ones is even easier as the .32 code matches the .39 code
> in this area due to these patches being added.

I totally agree with Greg here. I'd say that sometimes it's even unpleasant
to backport apparently useless fixes, but it's needed to get in sync with
mainline. I've had quite a number of backporting issues in 2.4 because some
fixes were backported so much differently that at one point it was not even
possible to know whether a later fix was needed or not, or how to apply it.

Here if we start remodeling patches to make them smaller before backporting
them, we'll get different code and all subsequent fixes will have to be
adapted to this distinct branch. By doing this, we even risk regressions
because at one point we can introduce stable-specific bugs.

Willy

2011-04-20 05:53:07

by Willy Tarreau

[permalink] [raw]

Subject: Re: [Stable-review] [24/28] USB: xhci - fix unsafe macro definitions

On Wed, Apr 20, 2011 at 03:02:04AM +0100, Ben Hutchings wrote:
> On Tue, 2011-04-19 at 13:31 -0700, Greg KH wrote:
> > 2.6.32-longterm review patch. If anyone has any objections, please let us know.
> >
> > ------------------
> >
> > From: Dmitry Torokhov <[email protected]>
> >
> > commit 5a6c2f3ff039154872ce597952f8b8900ea0d732 upstream.
> >
> > Macro arguments used in expressions need to be enclosed in parenthesis
> > to avoid unpleasant surprises.
>
> Do you know of any specific uses of these macros where the missing
> parentheses caused 'unpleasant surprises'?

In my opinion, this type of fix should be backported even if the current
code does not appear to be at risk, otherwise a later fix in the kernel
could cause a serious regression when backported to -stable. For instance,
if we later have to backport this patch (cut'n'pasted) :

--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1033,7 +1033,7 @@ static inline unsigned int xhci_get_endpoint_interval(struct usb_device *udev,
default:
BUG();
}
- return EP_INTERVAL(interval);
+ return EP_INTERVAL(interval + 1);
}

How can you guess that while works in mainline, it breaks -stable ?

As a user, I'd rather have known valid code in -stable and -longterm at the
risk of an occasional *temporary* regression than longterm unexplainable
regressions due to diverging code causing unexpected issues with backported
fixes.

Regards,
Willy

2011-04-20 12:34:59

On Tue, 2011-04-19 at 22:06 -0700, Greg KH wrote:
> On Wed, Apr 20, 2011 at 05:48:30AM +0100, Ben Hutchings wrote:
> > On Tue, 2011-04-19 at 20:11 -0700, Greg KH wrote:
> > > On Wed, Apr 20, 2011 at 03:17:42AM +0100, Ben Hutchings wrote:
> > > > On Tue, 2011-04-19 at 19:01 -0700, Greg KH wrote:
> > > > > On Wed, Apr 20, 2011 at 02:40:53AM +0100, Ben Hutchings wrote:
> > > > > > On Tue, 2011-04-19 at 13:30 -0700, Greg KH wrote:
> > > > > > > 2.6.32-longterm review patch. If anyone has any objections, please let us know.
> > > > > > >
> > > > > > > ------------------
> > > > > > >
> > > > > > > From: Hans Rosenfeld <[email protected]>
> > > > > > >
> > > > > > > commit 9d8888c2a214aece2494a49e699a097c2ba9498b upstream.
> > > > > > >
> > > > > > > Remove check_c1e_idle() and use the new AMD errata checking framework
> > > > > > > instead.
> > > > > >
> > > > > > Clean-up patches are generally not candidates for longterm updates.
> > > > >
> > > > > This was added because a follow-on patch required it.
> > > >
> > > > Ah yes, 'x86, AMD: Set ARAT feature on AMD processors' is using the same
> > > > condition.
> > > >
> > > > Of course, that could have been backported by referring to the function
> > > > that this removes, rather than pulling in a load of other changes with
> > > > consequent risk of regressions.
> > >
> > > I prefer to take original patches for stable, it makes it easier in the
> > > end.
> >
> > It makes what easier, when? What I see here is a bug fix that is much
> > larger than necessary, with a consequent risk of regression that seems
> > way out of proportion to the benefit. (What actually *is* the benefit
> > of these AMD changes?) And we have had several serious regressions in
> > the 2.6.32.y series recently, so I really don't think we are getting the
> > trade-off right.
>
> We got a few new quirks added for AMD hardware platforms that fix
> problems.

Maybe, but I still haven't seen anyone explain what those problems are!

> It took 3 patches to get there, yes, but now, as time goes
> on, adding new ones is even easier as the .32 code matches the .39 code
> in this area due to these patches being added.
>
> Now if you find problems in these, great, let me know and I will work to
> resolve them.
>
> As for regressions, what are you referring to?

These are the regressions I'm aware of in the last few updates:

2.6.32.36: 'x86: Cleanup highmap after brk is concluded' broke
hibernation on x86-64 and some x86-32 machines (reverted in .37)
2.6.32.36: 'signal: Prevent rt_sigqueueinfo and rt_tgsigqueueinfo from
spoofing the signal code' broke glibc aio (fixed in .37)
2.6.32.34: powerpc kdump/kexec changes didn't compile on UP or 32-bit
(fixed in .37)
2.6.32.30: 'x86 quirk: Fix polarity for IRQ0 pin2 override on SB800
systems' broke the revision check for other quirks (fixed in .34)
2.6.32.29: 'USB: Prevent buggy hubs from crashing the USB stack' broke
MUSB on BeagleBoards (still unfixed?)

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2011-05-05 23:01:41

by Greg KH

[permalink] [raw]

Subject: Re: [Stable-review] [12/28] x86, cpu: Clean up AMD erratum 400 workaround

On Thu, Apr 21, 2011 at 04:29:15AM +0100, Ben Hutchings wrote:
> > We got a few new quirks added for AMD hardware platforms that fix
> > problems.
>
> Maybe, but I still haven't seen anyone explain what those problems are!

I'll let the AMD people answer this.

> > It took 3 patches to get there, yes, but now, as time goes
> > on, adding new ones is even easier as the .32 code matches the .39 code
> > in this area due to these patches being added.
> >
> > Now if you find problems in these, great, let me know and I will work to
> > resolve them.
> >
> > As for regressions, what are you referring to?
>
> These are the regressions I'm aware of in the last few updates:
>
> 2.6.32.36: 'x86: Cleanup highmap after brk is concluded' broke
> hibernation on x86-64 and some x86-32 machines (reverted in .37)
> 2.6.32.36: 'signal: Prevent rt_sigqueueinfo and rt_tgsigqueueinfo from
> spoofing the signal code' broke glibc aio (fixed in .37)
> 2.6.32.34: powerpc kdump/kexec changes didn't compile on UP or 32-bit
> (fixed in .37)
> 2.6.32.30: 'x86 quirk: Fix polarity for IRQ0 pin2 override on SB800
> systems' broke the revision check for other quirks (fixed in .34)

Bugs are always a part of life, the fact that we fix them is what is
important.

> 2.6.32.29: 'USB: Prevent buggy hubs from crashing the USB stack' broke
> MUSB on BeagleBoards (still unfixed?)

This woried me, as I don't recall anyone notifying me of it. Then I saw
your revert, and I noticed that all you really need to resolve this is
commit id ec95d35a6bd0047f05fe8a21e6c52f8bb418da55 to be added to the
tree. I'll go queue that one up for the next .32-stable tree.

So all should be resolved now, right?

thanks,

greg k-h

2011-05-06 07:41:27

by Borislav Petkov

[permalink] [raw]

Subject: Re: [Stable-review] [12/28] x86, cpu: Clean up AMD erratum 400 workaround

Hey Ben,

On Thu, May 05, 2011 at 03:53:02PM -0700, Greg KH wrote:
> On Thu, Apr 21, 2011 at 04:29:15AM +0100, Ben Hutchings wrote:
> > > We got a few new quirks added for AMD hardware platforms that fix
> > > problems.
> >
> > Maybe, but I still haven't seen anyone explain what those problems are!
>
> I'll let the AMD people answer this.

it's hard to follow back in this thread to what exactly you'd like to
have answered. Can you please repeat your questions concerning the AMD
backports? Is it, per chance, about Erratum 400 CPU f/m/s ranges that
got changed?

Thanks.

--
Regards/Gruss,
Boris.

2011-05-10 03:05:57

by Ben Hutchings

[permalink] [raw]

Subject: Re: [Stable-review] [12/28] x86, cpu: Clean up AMD erratum 400 workaround

On Thu, 2011-05-05 at 15:53 -0700, Greg KH wrote:
> On Thu, Apr 21, 2011 at 04:29:15AM +0100, Ben Hutchings wrote:
[...]
> > 2.6.32.29: 'USB: Prevent buggy hubs from crashing the USB stack' broke
> > MUSB on BeagleBoards (still unfixed?)
>
> This woried me, as I don't recall anyone notifying me of it. Then I saw
> your revert, and I noticed that all you really need to resolve this is
> commit id ec95d35a6bd0047f05fe8a21e6c52f8bb418da55 to be added to the
> tree. I'll go queue that one up for the next .32-stable tree.
>
> So all should be resolved now, right?

Yes, thanks.

Ben.

--
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2011-05-10 12:59:11

On Tue, May 10, 2011 at 08:13:05AM -0700, Greg KH wrote:
> On Tue, May 10, 2011 at 04:02:19PM +0200, Borislav Petkov wrote:
> > On Tue, May 10, 2011 at 08:58:59AM -0400, Ben Hutchings wrote:
> > > I read that:
> > >
> > > "
> > > Support for Always Running APIC timer (ARAT) was introduced in
> > > commit db954b5898dd3ef3ef93f4144158ea8f97deb058. This feature
> > > allows us to avoid switching timers from LAPIC to something else
> > > (e.g. HPET) and go into timer broadcasts when entering deep
> > > C-states.
> > >
> > > AMD processors don't provide a CPUID bit for that feature but
> > > they also keep APIC timers running in deep C-states (except for
> > > cases when the processor is affected by erratum 400). Therefore
> > > we should set ARAT feature bit on AMD CPUs.
> > > "
> > >
> > > This implies that the HPET was previously used during deep C-states, and
> > > that all this erratum checking is about deciding whether the CPU has
> > > ARAT. So what bug is being fixed by using ARAT instead of the HPET?
> >
> > That's a good question, actually. The original upstream commit
> > b87cf80af3ba4b4c008b4face3c68d604e1715c6 wasn't tagged for stable and
> > Boris O. didn't send it to stable either, as I'm being told.
> >
> > Maybe Greg has an idea?
>
> I added it because it was needed to get another errata patch to apply
> properly, and I saw no reason not to apply other "quirks" like this to
> older kernels, right?
>
> Or should I not have done that?

Nah, I wouldn't say that. While this patch doesn't directly fix a
regression, it does away with the erratum 400 workaround where we switch
to HPET so it is more or less a regression fix for current and future
machines which aren't affected by that erratum. Besides reading the
LAPIC is much cheaper so we should be getting even a slight performance
bump with it.

So no, I don't see a problem with the patch being in stable.

Thanks.

--
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632