headers
Krzysztof Helt | 1 Jun 2008 01:01

Re: [PATCH] convert sticore.c to PCI ROM API (bugzilla #9425)

On Sun, 1 Jun 2008 00:22:23 +0200
Helge Deller <deller <at> gmx.de> wrote:

> Hello Krzysztof,
> 
> On Saturday 31 May 2008, Krzysztof Helt wrote:
> > From: Krzysztof Helt <krzysztof.h1 <at> wp.pl>
> > 
> > Convert console/sticore.c file to use PCI ROM API.
> > 
> > Signed-off-by: Krzysztof Helt <krzysztof.h1 <at> wp.pl>
> > 
> > ---
> > I don't have cross-compiler nor the hardware so PARISC guys
> > please test it.
> > [...]
> 
> Thanks for the patch.
> Below is the fixed version for parisc.
> Compile- and run-tested by me.
> Please apply.
> 

Thank you for testing. I have only one question. Why does the address
parameter to the sti_try_rom_generic() is unsigned long and not (char __iomem *)?
It would simplify the code (two type casts removed). Now, I see that I have made
an error in the patch that no rom_base variable has been defined in 
the sti_try_rom_generic(). My intention was to use the address parameter directly.
Is this ok (change the parameter type) on parisc linux?
(Continue reading)

Permalink | Reply | 0 comments |
headers
Grant Grundler | 1 Jun 2008 03:37
Favicon

Re: [bug] gcc-4.3 miscompiling causing networking to bugger up

On Sat, May 31, 2008 at 04:58:58AM -0600, Matthew Wilcox wrote:
...
> So if GCC has something in a register, it might not bother to write it
> back to ram before this asm if we don't have the memory clobber.

Willy, thanks for explaining...I had to reread this a few times to
"get it". This would also explain why we saw it on ip_output where
the ipv4 header was being written.

> Fantastic work, Kyle.  Thanks for spending so much time on this.

Agreed. I owe you a bar of chocolate.

> > diff --git a/include/asm-parisc/checksum.h b/include/asm-parisc/checksum.h
> > index cc3ec1b..1916ebe 100644
> > --- a/include/asm-parisc/checksum.h
> > +++ b/include/asm-parisc/checksum.h
> >  <at>  <at>  -65,7 +65,7  <at>  <at>  static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
> >  "2:\n"
> >  	: "=r" (sum), "=r" (iph), "=r" (ihl)
> >  	: "1" (iph), "2" (ihl)
> > -	: "r19", "r20", "r21" );
> > +	: "r19", "r20", "r21", "memory" );
> >  
> >  	return (__force __sum16)sum;
> >  }

Does csum_ipv6_magic() also need the same treatment?

Kyle suspects csum_ipv6_magic() might be ok because it's passing
(Continue reading)

Permalink | Reply | 0 comments |
headers
Krzysztof Helt | 1 Jun 2008 09:03

Re: [PATCH] convert sticore.c to PCI ROM API (bugzilla #9425)

On Sun, 1 Jun 2008 01:01:55 +0200
Krzysztof Helt <krzysztof.h1 <at> poczta.fm> wrote:

> On Sun, 1 Jun 2008 00:22:23 +0200
> Helge Deller <deller <at> gmx.de> wrote:
> 
>> Thanks for the patch.
>> Below is the fixed version for parisc.
>> Compile- and run-tested by me.
>> Please apply.
> 
> Thank you for testing. I have only one question. Why does the address
> parameter to the sti_try_rom_generic() is unsigned long and not (char __iomem *)?
> It would simplify the code (two type casts removed). Now, I see that I have made
> an error in the patch that no rom_base variable has been defined in 
> the sti_try_rom_generic(). My intention was to use the address parameter directly.
> Is this ok (change the parameter type) on parisc linux?
> 

It was late night when I answered. I see it now - one uses gsc_readl function
on the address so one needs the address represented as unsigned long (or casted).

Your patch is 100% correct (copied without changes below).

Regards,
Krzysztof

------------------------

Convert console/sticore.c file to use PCI ROM API.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Kyle McMartin | 3 Jun 2008 07:42
Picon

parisc: add barriers to mmio accessors

Prevents GCC from reordering mmio accesses with regular memory
accesses.

They're in assembly since I felt like maybe using the strongly
ordered completer on the instructions, so they would be more obvious
in disassembly. (Yes, I know there's no weakly ordered PA
implementation...)

diff --git a/include/asm-parisc/io.h b/include/asm-parisc/io.h
index 55ddb18..e5ce0a9 100644
--- a/include/asm-parisc/io.h
+++ b/include/asm-parisc/io.h
 <at>  <at>  -148,36 +148,82  <at>  <at>  extern void iounmap(const volatile void __iomem *addr);

 static inline unsigned char __raw_readb(const volatile void __iomem *addr)
 {
-	return (*(volatile unsigned char __force *) (addr));
+	unsigned char c;
+	asm volatile(
+	"	ldb	0(%0), %1\n"
+	: : "r"(addr), "r"(c) : "memory");
+	return c;
 }
+
 static inline unsigned short __raw_readw(const volatile void __iomem *addr)
 {
-	return *(volatile unsigned short __force *) addr;
+	unsigned short s;
+	asm volatile(
+	"	lds	0(%0), %1\n"
(Continue reading)

Permalink | Reply | 4 comments |
headers
Carlos O'Donell | 3 Jun 2008 14:22
Gravatar

Re: parisc: add barriers to mmio accessors

On Tue, Jun 3, 2008 at 1:42 AM, Kyle McMartin <kyle <at> mcmartin.ca> wrote:
> Prevents GCC from reordering mmio accesses with regular memory
> accesses.
>
> They're in assembly since I felt like maybe using the strongly
> ordered completer on the instructions, so they would be more obvious
> in disassembly. (Yes, I know there's no weakly ordered PA
> implementation...)
> +#ifdef CONFIG_64BIT
> +       asm volatile(
> +       "       ldq     0(%0), %1\n"
> +       : : "r"(addr), "r"(q) : "memory");
> +#else
> +       unsigned int q_lo, q_hi;
> +       q_hi = __raw_readl(addr);
> +       q_lo = __raw_readl(addr+4);
> +       q = (unsigned long long)(q_hi << 32) | (q_lo);
> +#endif
> +
> +       return q;

Is there any reason the 32-bit version is uses plain C?

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
(Continue reading)

Permalink | Reply | 1 comment |
headers
Carlos O'Donell | 3 Jun 2008 14:23
Gravatar

Re: parisc: add barriers to mmio accessors

On Tue, Jun 3, 2008 at 8:22 AM, Carlos O'Donell <carlos <at> systemhalted.org> wrote:
>> +#ifdef CONFIG_64BIT
>> +       asm volatile(
>> +       "       ldq     0(%0), %1\n"
>> +       : : "r"(addr), "r"(q) : "memory");
>> +#else
>> +       unsigned int q_lo, q_hi;
>> +       q_hi = __raw_readl(addr);
>> +       q_lo = __raw_readl(addr+4);
>> +       q = (unsigned long long)(q_hi << 32) | (q_lo);
>> +#endif
>> +
>> +       return q;
>
> Is there any reason the 32-bit version is uses plain C?

I see, you optimized this for the 64-bit case, that makes sense. I
should drink more coffee.

Cheers,
Carlos.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Permalink | Reply | 0 comments |
headers
Matthew Wilcox | 3 Jun 2008 14:42

Re: parisc: add barriers to mmio accessors

On Tue, Jun 03, 2008 at 01:42:12AM -0400, Kyle McMartin wrote:
> +#ifdef CONFIG_64BIT
> +	asm volatile(
> +	"	ldq	0(%0), %1\n"
> +	: : "r"(addr), "r"(q) : "memory");
> +#else
> +	unsigned int q_lo, q_hi;
> +	q_hi = __raw_readl(addr);
> +	q_lo = __raw_readl(addr+4);
> +	q = (unsigned long long)(q_hi << 32) | (q_lo);
> +#endif

Are you sure this is correct?  Seems to me it should be:

	q = ((unsigned long long)q_hi << 32) | q_lo;

I would have thought GCC would complain about a shift exceeding the
width of the type.

> +static inline void __raw_writeq(unsigned long long q, volatile void __iomem *addr)
>  {
> -	*(volatile unsigned long long __force *) addr = b;
> +#ifdef CONFIG_64BIT
> +	asm volatile(
> +	"	stq	%1, 0(%0)\n"
> +	: : "r"(addr), "r"(q) : "memory");
> +#else
> +	unsigned int q_hi = (q >> 32) & ~0UL;
> +	unsigned int q_lo = (q) & ~0UL;
> +	__raw_writel(q_hi, addr);
(Continue reading)

Permalink | Reply | 1 comment |
headers
Kyle McMartin | 3 Jun 2008 16:29
Picon

Re: parisc: add barriers to mmio accessors

On Tue, Jun 03, 2008 at 06:42:29AM -0600, Matthew Wilcox wrote:
> Are you sure this is correct?  Seems to me it should be:
> 
> 	q = ((unsigned long long)q_hi << 32) | q_lo;
> 
> I would have thought GCC would complain about a shift exceeding the
> width of the type.
> 

I probably fat fingered the braces.

> A third thing is that you're doing this to the __raw_ variants which
> don't have to be serialised.  How about we keep the current definitions of
> __raw_readX/__raw_writeX, define the regular readX/writeX to be the inline
> assembler you've just posted, and add new defines of __readX/__writeX
> as byteswapping versions of __raw_readX/__raw_writeX?
> 

I was just going to define them all in terms of this... since, well,
performance is mostly irrelevant.

> [For those who aren't on linux-arch, there's just been a long thread
> about the semantics of the different accessors and the above reflects my
> understanding of that thread.]
> 
> Should we add memory clobbers to gsc_readX/gsc_writeX?
> 

Yeah, I think so (although, I really hope GCC won't reorder serializing
instructions like rsm/ssm. ;-)
(Continue reading)

Permalink | Reply | 0 comments |
headers
Krzysztof Helt | 5 Jun 2008 23:13

[RESEND] [PATCH] convert sticore.c to PCI ROM API

From: Krzysztof Helt <krzysztof.h1 <at> wp.pl>

Convert console/sticore.c file to use PCI ROM API.

Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425

Signed-off-by: Krzysztof Helt <krzysztof.h1 <at> wp.pl>
Signed-off-by: Helge Deller <deller <at> gmx.de>

---
This patch was fixed and tested by Helge Deller on his
PARISC machine.

diff --git a/drivers/video/console/sticore.c b/drivers/video/console/sticore.c
index e9ab657..df30499 100644
--- a/drivers/video/console/sticore.c
+++ b/drivers/video/console/sticore.c
 <at>  <at>  -780,11 +780,13  <at>  <at>  out_err:
 }

 static struct sti_struct * __devinit
-sti_try_rom_generic(unsigned long address, unsigned long hpa, struct pci_dev *pd)
+sti_try_rom_generic(unsigned long address, unsigned long hpa,
+		    struct pci_dev *pd)
 {
+	char __iomem *rom_base = (char __iomem *) address;
 	struct sti_struct *sti;
 	int ok;
-	u32 sig;
+	__le32 sig;
(Continue reading)

Permalink | Reply | 0 comments |
headers
Andrew Morton | 6 Jun 2008 00:11

Re: [RESEND] [PATCH] convert sticore.c to PCI ROM API

On Thu, 5 Jun 2008 23:13:27 +0200
Krzysztof Helt <krzysztof.h1 <at> poczta.fm> wrote:

> From: Krzysztof Helt <krzysztof.h1 <at> wp.pl>
> 
> Convert console/sticore.c file to use PCI ROM API.
> 
> Addresses http://bugzilla.kernel.org/show_bug.cgi?id=9425

I'm unable to understand that bug report.  Is this just a cleanup
or does it fix some runtime error, or...?

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
Permalink | Reply | 0 comments |

Gmane