Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Linas Vepstas]

On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote:
> 
> On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and
> outsl, so remove the conditional use of insl_ns and outsl_ns.

The rest of this patch might indeed be correct, but the above comment 
bothers me. The "ns" versions of routines are supposed to be
non-byte-swapped versions of the insl/outsl routines (which would
byte-swap on big-endian archs such as powerpc.)

> diff --git a/drivers/net/3c509.c b/drivers/net/3c509.c
> index cbdae54..add6381 100644
> --- a/drivers/net/3c509.c
> +++ b/drivers/net/3c509.c
> @@ -879,11 +879,7 @@ #endif
>  	outw(skb->len, ioaddr + TX_FIFO);
>  	outw(0x00, ioaddr + TX_FIFO);
>  	/* ... and the packet rounded to a doubleword. */
> -#ifdef  __powerpc__
> -	outsl_ns(ioaddr + TX_FIFO, skb->data, (skb->len + 3) >> 2);
> -#else
>  	outsl(ioaddr + TX_FIFO, skb->data, (skb->len + 3) >> 2);
> -#endif

Dohh, a hack like this to work around some possbile byte-swapping
bug should never have been done in the first place :-(

However, I presume someone added the  __powerpc__ define here
because they picked up a 3c509 at a garage sale, stuck it in 
a powerpc, found out it didn't work due to a byte-swapping bug,
and then patched it as above. I'm disturbed that somehow 
outsl_ns() became identical to outsl() at some point, presumably
breaking this patch.

--linas

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Matt Sealey]

Some northbridges and PCI bridges have "clever" byteswapping in 
hardware, maybe this is just an effect of that. In theory depending on 
the host bridge, you should pass in big endian data and have it swap or 
not swap, not pick that way in the driver, UNLESS your driver expects 
bigendian data, in which case on a bigendian platform you can tell it to 
write without swapping. Voila, two functions.

However the existance of these PCI bridges these days? I haven't seen 
one in years, and when I have nobody has ever enabled the magic swappy 
thing as it's unreliable and can't always tell how you present the data.

One wishes that there was a ntoh and hton style macro in standard use 
for PCI access.. hang on though that jsut wouldn't work would it.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Linas Vepstas wrote:
> On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote:
>> On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and
>> outsl, so remove the conditional use of insl_ns and outsl_ns.
> 
> The rest of this patch might indeed be correct, but the above comment 
> bothers me. The "ns" versions of routines are supposed to be
> non-byte-swapped versions of the insl/outsl routines (which would
> byte-swap on big-endian archs such as powerpc.)
> 
>> diff --git a/drivers/net/3c509.c b/drivers/net/3c509.c
>> index cbdae54..add6381 100644
>> --- a/drivers/net/3c509.c
>> +++ b/drivers/net/3c509.c
>> @@ -879,11 +879,7 @@ #endif
>>  	outw(skb->len, ioaddr + TX_FIFO);
>>  	outw(0x00, ioaddr + TX_FIFO);
>>  	/* ... and the packet rounded to a doubleword. */
>> -#ifdef  __powerpc__
>> -	outsl_ns(ioaddr + TX_FIFO, skb->data, (skb->len + 3) >> 2);
>> -#else
>>  	outsl(ioaddr + TX_FIFO, skb->data, (skb->len + 3) >> 2);
>> -#endif
> 
> Dohh, a hack like this to work around some possbile byte-swapping
> bug should never have been done in the first place :-(
> 
> However, I presume someone added the  __powerpc__ define here
> because they picked up a 3c509 at a garage sale, stuck it in 
> a powerpc, found out it didn't work due to a byte-swapping bug,
> and then patched it as above. I'm disturbed that somehow 
> outsl_ns() became identical to outsl() at some point, presumably
> breaking this patch.
> 
> --linas
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Linas Vepstas]

On Tue, Sep 19, 2006 at 08:52:18PM +0200, Matt Sealey wrote:
[...]
> 
> Linas Vepstas wrote:
> >On Tue, Sep 19, 2006 at 02:54:33PM +1000, Stephen Rothwell wrote:
> >>On powerpc and ppc, insl_ns and insl are identical as are outsl_ns and
> >>outsl, so remove the conditional use of insl_ns and outsl_ns.
> >
> >The rest of this patch might indeed be correct, but the above comment 
> >bothers me. The "ns" versions of routines are supposed to be
> >non-byte-swapped versions of the insl/outsl routines (which would

Never mind. Silly me don't know my history. 

asm-powerpc/io.h clearly states

 * The *_ns versions below don't do byte-swapping.
 * Neither do the standard versions now, these are just here
 * for older code.

Seems that the byteswapping machanism was changed a while ago, 
and no longer handled in this way any more.

--linas

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Benjamin Herrenschmidt]

On Tue, 2006-09-19 at 20:52 +0200, Matt Sealey wrote:
> Some northbridges and PCI bridges have "clever" byteswapping in 
> hardware, maybe this is just an effect of that. In theory depending on 
> the host bridge, you should pass in big endian data and have it swap or 
> not swap, not pick that way in the driver, UNLESS your driver expects 
> bigendian data, in which case on a bigendian platform you can tell it to 
> write without swapping. Voila, two functions.

It's generally considered pretty bad for a northbridge to try to muck
around with byte order. There are fairly well defined rules to plug a
little endian bus (PCI, ISA, ...) on a big endian machine.

The trick that some people didn't get a while ago is that while
accessors like inw/inl shall return a byteswapped data, string
operations like in insw/insl who are copying from a fifo basically to
memory (and opposite write versions) shall -not- byteswap since the data
isn't interpreted. it's a byte stream. It doesn't have any endian
semantic associated to it until it's actually read back from memory in
which case the appropriate endian swap (if any) has to be used depending
on the endianness and size of a given field read/written.

Since some people didn't get it, in the early days, some BE
architectures like ppc had versions of insw/insl that did byteswap,
which was wrong. The bits in this driver are remains from that era.

Note that to aggravate the problem, it still happens that HW engineers
try to be "smart" when hooking a 16 bits or 32 bits FIFO to a BE machine
and byteswap it in hardware. This is of course totally bogus but did
happen with IDE controllers typically (I think atari or amiga has one of
these, the Tivo is like that too, and a bunch of embedded  things). The
net result is that you have to pump the data fifo using a byteswapping
accessor and you cannot use DMA unless you DMA controller can re-swap
the other way around.... But lots of HW people still don't get it :) 

> However the existance of these PCI bridges these days? I haven't seen 
> one in years, and when I have nobody has ever enabled the magic swappy 
> thing as it's unreliable and can't always tell how you present the data.
> 
> One wishes that there was a ntoh and hton style macro in standard use 
> for PCI access.. hang on though that jsut wouldn't work would it.

Nah. We have the basic rule that readl/writel are little endian. PowerPC
additionally provides arch specific low level in_{be,le}32 type
accessors with explicit endianness. Or you can also use
cpu_to_le32/le32_to_cpu kind of macros to convert between native and
explicit endianness.

Ben.

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Segher Boessenkool]

> Nah. We have the basic rule that readl/writel are little endian.  
> PowerPC
> additionally provides arch specific low level in_{be,le}32 type
> accessors with explicit endianness. Or you can also use
> cpu_to_le32/le32_to_cpu kind of macros to convert between native and
> explicit endianness.

Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
only?  I sure hope not.

It would make a lot more sense if readX()/writeX() used the endianness
of the bus they are performed on.  PowerPC byteswaps are cheap -- for
16- and 32-bit accesses.  They're quite bad for 64-bit though; it would
be a pity to end up doing two of those for a 64-bit big-endian I/O  
access
(one on the access itself, one to convert the data back to CPU order).

This would happily solve the problem of the various variations of
byte-swapping bus bridges, too ("natural" swap, 32-bit swap, 64-bit  
swap,
perhaps others that I thankfully have never seen or cannot remember).

Now you can say, use readl_be() or something similar, but that's a)  
ugly,
b) error-prone, c) exponential interface explosion, d) ugly.


Segher

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Benjamin Herrenschmidt]

On Wed, 2006-09-20 at 02:21 +0200, Segher Boessenkool wrote:
> > Nah. We have the basic rule that readl/writel are little endian.  
> > PowerPC
> > additionally provides arch specific low level in_{be,le}32 type
> > accessors with explicit endianness. Or you can also use
> > cpu_to_le32/le32_to_cpu kind of macros to convert between native and
> > explicit endianness.
> 
> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
> only?  I sure hope not.

It's defined for PCI and possibly ISA memory. You can use it for other
things if you whish to, but "other things" are arch specific in any
case.

> It would make a lot more sense if readX()/writeX() used the endianness
> of the bus they are performed on.

No way ! Again, it's evil if such a simple thing start doing different
things depending on random external factors.

Different bus -> different accessor.

We defined on PowerPC that readl was fine for anything that comes out of
ioremap and is little endian, but that's also why you have the explicit
{in,out}_{le,be}{16,32}. That's what you should use in fact with non-PCI
busses unless you know you are LE.

> PowerPC byteswaps are cheap -- for 16- and 32-bit accesses.  They're quite bad for 64-bit though; it would
> be a pity to end up doing two of those for a 64-bit big-endian I/O  
> access
> (one on the access itself, one to convert the data back to CPU order).
> 
> This would happily solve the problem of the various variations of
> byte-swapping bus bridges, too ("natural" swap, 32-bit swap, 64-bit  
> swap,
> perhaps others that I thankfully have never seen or cannot remember).
> 
> Now you can say, use readl_be() or something similar, but that's a)  
> ugly,
> b) error-prone, c) exponential interface explosion, d) ugly.

I'd rather has an interface explosion than having black endian magic
happening inside of the accessors.

Ben.

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Segher Boessenkool]

>> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
>> only?  I sure hope not.
>
> It's defined for PCI and possibly ISA memory. You can use it for other
> things if you whish to, but "other things" are arch specific in any
> case.

Huh?  You're saying that only PCI and ISA are standardised busses?

>> It would make a lot more sense if readX()/writeX() used the  
>> endianness
>> of the bus they are performed on.
>
> No way ! Again, it's evil if such a simple thing start doing different
> things depending on random external factors.

That's your opinion, yes.

I'm saying it's *not* doing different things: in both cases it just does
the correct-endian access.  Also it doesn't depend on "random external
factors" -- they're not random factors, and not external either: it only
depends on the bus the access is done on.

> Different bus -> different accessor.

Then please rename readX()/writeX() to pci_readX()/pci_writeX().

>> Now you can say, use readl_be() or something similar, but that's a)
>> ugly,
>> b) error-prone, c) exponential interface explosion, d) ugly.
>
> I'd rather has an interface explosion than having black endian magic
> happening inside of the accessors.

Any comments on a), b) and d) as well?


Segher

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Linas Vepstas]

Hi,

I am alarmed and embarassed that sloppy comments on my part has turned
onto a long conversation.

On Wed, Sep 20, 2006 at 02:58:39AM +0200, Segher Boessenkool wrote:
> >> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
> >> only?  I sure hope not.
> >
> > It's defined for PCI and possibly ISA memory. You can use it for other
> > things if you whish to, but "other things" are arch specific in any
> > case.
> 
> Huh?  You're saying that only PCI and ISA are standardised busses?

Well, I'm having trouble thinking of other busses that have as strong 
a sense of the "address-data" style I/O as PCI. Busses like scsi and 
ide are primarily "command-data" or "data-data" in style.  Only the
address-data style busses need readl/writel-style routines.

I can't prove, but suspect that the "adress-data" style of access is 
why PCI is wired up "close to" the CPU.  What other bsses are there 
that are direct-attached to the CPU? I can't think of much ...

The sbus on sparc ... hypertransport from AMD ... but hypertransport is 
more or less invisible to the kernel.  ... some recent attempts to 
supplant the system bus with infiniband, but I get the impression that 
this will be strangely engineered, and semi-invisible to the kernel as
well. The actual infiniband protocols are ipv6-like+rdma and so fall
into a "data-data" programming style.

> > Different bus -> different accessor.
> 
> Then please rename readX()/writeX() to pci_readX()/pci_writeX().

Well, I don't get the impression that there will be othre busses for
which this is an issue the way it is on pci.

--linas

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Segher Boessenkool]

> Well, I'm having trouble thinking of other busses that have as strong
> a sense of the "address-data" style I/O as PCI. Busses like scsi and
> ide are primarily "command-data" or "data-data" in style.  Only the
> address-data style busses need readl/writel-style routines.

SBUS, JBUS, VMEbus, NuBus, RapidIO, eBus, various SoC busses, to name
a few.  There are many, and most are big-endian.


Segher

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Jeff Garzik]

Segher Boessenkool wrote:
> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
> only?

Yes.

For other buses, use foo_writel(), etc.

	Jeff

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Segher Boessenkool]

>> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
>> only?
>
> Yes.
>
> For other buses, use foo_writel(), etc.

Can this please be documented then?  Never heard this before...


Segher

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Jeff Garzik]

Segher Boessenkool wrote:
>>> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
>>> only?
>>
>> Yes.
>>
>> For other buses, use foo_writel(), etc.
> 
> Can this please be documented then?  Never heard this before...

You have come late to the party.  This has been the case for many, many 
years.

And there is no point in a massive rename to pci_writel(), either.

	Jeff

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Segher Boessenkool]

>>>> Sure, PCI busses are little-endian.  But is readX()/writeX() for  
>>>> PCI
>>>> only?
>>>
>>> Yes.
>>>
>>> For other buses, use foo_writel(), etc.
>> Can this please be documented then?  Never heard this before...
>
> You have come late to the party.

WHat do you mean here?  Could you please explain?

> This has been the case for many, many years.

No, it was never documented AFAICS.

> And there is no point in a massive rename to pci_writel(), either.

That would be really inconvenient, sure.  It's also inconvenient
that all the nice short names are PCI-only.


Segher

Re: [PATCH] Remove powerpc specific parts of 3c509 driver

[Jeff Garzik]

Segher Boessenkool wrote:
>>>>> Sure, PCI busses are little-endian.  But is readX()/writeX() for PCI
>>>>> only?
>>>>
>>>> Yes.
>>>>
>>>> For other buses, use foo_writel(), etc.
>>> Can this please be documented then?  Never heard this before...
>>
>> You have come late to the party.
> 
> WHat do you mean here?  Could you please explain?
> 
>> This has been the case for many, many years.
> 
> No, it was never documented AFAICS.

A de facto standard does not need to be documented, to be a de facto 
standard.

A lot of Linux "standards" are often based on emails from Linus buried 
halfway down a thread.  A decision gets made, and people follow.


>> And there is no point in a massive rename to pci_writel(), either.
> 
> That would be really inconvenient, sure.  It's also inconvenient
> that all the nice short names are PCI-only.

Only to you, a decided minority of developers.

	Jeff

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Benjamin Herrenschmidt]

> However, I presume someone added the  __powerpc__ define here
> because they picked up a 3c509 at a garage sale, stuck it in 
> a powerpc, found out it didn't work due to a byte-swapping bug,
> and then patched it as above. I'm disturbed that somehow 
> outsl_ns() became identical to outsl() at some point, presumably
> breaking this patch.

The problem is that somebody had the "bright" idea to implement ppc
outsl as byteswapping a loooong time ago. This was totally bogus and got
fixed, but it looks like this driver holds a remain of that period.

Ben.

Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver

[Paul Mackerras]

Linas Vepstas writes:

> The rest of this patch might indeed be correct, but the above comment 
> bothers me. The "ns" versions of routines are supposed to be
> non-byte-swapped versions of the insl/outsl routines (which would
> byte-swap on big-endian archs such as powerpc.)

If it were true that in/outsw and in/outsl were actually used to
transfer arrays of 16-bit data items or 32-bit data items to/from an
I/O device, I would agree with you, but they aren't.  They are
universally used to transfer arrays of bytes, with the optimization of
doing so 2 or 4 bytes at a time.  That is why in/outsw and in/outsl
don't (and shouldn't) do byte swapping.

Paul.