Looks broken to me: x86, cyrix: debug

Looks broken to me: x86, cyrix: debug

[Alan Cox]

On Sat, 11 Oct 2008 20:03:32 GMT
Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=026e2c05ef58ef413e2d52696f125d5ea1aa8bce
> Commit:     026e2c05ef58ef413e2d52696f125d5ea1aa8bce
> Parent:     93ded9b8fd42abe2c3607097963d8de6ad9117eb
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Tue Jul 22 11:58:14 2008 +0200
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Tue Jul 22 12:24:00 2008 +0200
> 
>     x86, cyrix: debug

Looks broken to me:

>  	/* Load/Store Serialize to mem access disable (=reorder it) */
> -	setCx86(CX86_PCR0, getCx86(CX86_PCR0) & ~0x80);
> +	setCx86_old(CX86_PCR0, getCx86_old(CX86_PCR0) & ~0x80);

Three problems:

1.

> +#define getCx86_old(reg) ({ outb((reg), 0x22); inb(0x23); })

Doesn't expand to any kind of returned value.

 +#define setCx86_old(reg, data) do { \
> +	outb((reg), 0x22); \
> +	outb((data), 0x23); \
> +} while (0)

2. 
And even if it did it would seem to expand to

	outb reg, 0x22
	outb reg, 0x22
	inb 0x23
	outb something, 0x23

which doesn't in fact work as the outb/inb or outb/outb to 0x22/0x23 must
be paired.

3.

What exactly does fiddling with stuff like the memory access ordering
(which is rather important!) have to do with a single oneliner "cyrix:
debug" title and no changelog.


Alan

Re: Looks broken to me: x86, cyrix: debug

[Maciej W. Rozycki]

On Sat, 11 Oct 2008, Alan Cox wrote:

> > +#define getCx86_old(reg) ({ outb((reg), 0x22); inb(0x23); })
> 
> Doesn't expand to any kind of returned value.

 Well, actually it returns the value of the inb() expression -- cf the 
semantics of compound statements.  The rest looks suspicious indeed.

  Maciej

Re: Looks broken to me: x86, cyrix: debug

[Ingo Molnar]

* Maciej W. Rozycki <macro@linux-mips.org> wrote:

> On Sat, 11 Oct 2008, Alan Cox wrote:
> 
> > > +#define getCx86_old(reg) ({ outb((reg), 0x22); inb(0x23); })
> > 
> > Doesn't expand to any kind of returned value.
> 
>  Well, actually it returns the value of the inb() expression -- cf the 
> semantics of compound statements.  The rest looks suspicious indeed.

indeed it's broken - will sort it out. It's all about breakage that has 
been in this code since v2.6.20, see c6744955d0. Will likely revert the 
whole thing.

	Ingo

Re: Looks broken to me: x86, cyrix: debug

[Willy Tarreau]

On Sat, Oct 11, 2008 at 10:58:58PM +0200, Ingo Molnar wrote:
> 
> * Maciej W. Rozycki <macro@linux-mips.org> wrote:
> 
> > On Sat, 11 Oct 2008, Alan Cox wrote:
> > 
> > > > +#define getCx86_old(reg) ({ outb((reg), 0x22); inb(0x23); })
> > > 
> > > Doesn't expand to any kind of returned value.
> > 
> >  Well, actually it returns the value of the inb() expression -- cf the 
> > semantics of compound statements.  The rest looks suspicious indeed.
> 
> indeed it's broken - will sort it out. It's all about breakage that has 
> been in this code since v2.6.20, see c6744955d0. Will likely revert the 
> whole thing.

In fact the whole commit looks buggy to me as it simply reintroduces the
old bug at several places. The correct way of accessing the registers is
precisely through the inline functions and never through a macro due to
the ordering problem.

Willy

Re: Looks broken to me: x86, cyrix: debug

[Maciej W. Rozycki]

On Sun, 12 Oct 2008, Willy Tarreau wrote:

> In fact the whole commit looks buggy to me as it simply reintroduces the
> old bug at several places. The correct way of accessing the registers is
> precisely through the inline functions and never through a macro due to
> the ordering problem.

 Well, with some GCC extensions macros can be written such that ordering 
is kept, but inline functions tend to be simpler and often more readable.

  Maciej