All of lore.kernel.org
 help / color / mirror / Atom feed
From: linas@austin.ibm.com (Linas Vepstas)
To: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: akpm@osdl.org, ppc-dev <linuxppc-dev@ozlabs.org>,
	jgarzik@pobox.com, netdev@vger.kernel.org
Subject: Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
Date: Tue, 19 Sep 2006 13:42:43 -0500	[thread overview]
Message-ID: <20060919184243.GL29167@austin.ibm.com> (raw)
In-Reply-To: <20060919145433.8fc7d478.sfr@canb.auug.org.au>

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

WARNING: multiple messages have this Message-ID (diff)
From: linas@austin.ibm.com (Linas Vepstas)
To: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: ppc-dev <linuxppc-dev@ozlabs.org>,
	netdev@vger.kernel.org, akpm@osdl.org, jgarzik@pobox.com
Subject: Re: Fw: [PATCH] Remove powerpc specific parts of 3c509 driver
Date: Tue, 19 Sep 2006 13:42:43 -0500	[thread overview]
Message-ID: <20060919184243.GL29167@austin.ibm.com> (raw)
In-Reply-To: <20060919145433.8fc7d478.sfr@canb.auug.org.au>

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


  reply	other threads:[~2006-09-19 18:42 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-19  4:54 Fw: [PATCH] Remove powerpc specific parts of 3c509 driver Stephen Rothwell
2006-09-19 18:42 ` Linas Vepstas [this message]
2006-09-19 18:42   ` Linas Vepstas
2006-09-19 18:52   ` Matt Sealey
2006-09-19 18:52     ` Matt Sealey
2006-09-19 19:44     ` Linas Vepstas
2006-09-19 19:44       ` Linas Vepstas
2006-09-19 23:24     ` Benjamin Herrenschmidt
2006-09-19 23:24       ` Benjamin Herrenschmidt
2006-09-20  0:21       ` Segher Boessenkool
2006-09-20  0:25         ` Benjamin Herrenschmidt
2006-09-20  0:58           ` Segher Boessenkool
2006-09-20  0:58             ` Segher Boessenkool
2006-09-20  1:17             ` Linas Vepstas
2006-09-20  1:17               ` Linas Vepstas
2006-09-20  1:41               ` Segher Boessenkool
2006-09-20  0:57         ` Jeff Garzik
2006-09-20  1:08           ` Segher Boessenkool
2006-09-20  1:08             ` Segher Boessenkool
2006-09-20  1:18             ` Jeff Garzik
2006-09-20  1:18               ` Jeff Garzik
2006-09-20 20:06               ` Segher Boessenkool
2006-09-20 20:06                 ` Segher Boessenkool
2006-09-20 21:29                 ` Jeff Garzik
2006-09-20 21:29                   ` Jeff Garzik
2006-09-19 23:18   ` Fw: " Benjamin Herrenschmidt
2006-09-19 23:27   ` Paul Mackerras
2006-09-19 23:27     ` Paul Mackerras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060919184243.GL29167@austin.ibm.com \
    --to=linas@austin.ibm.com \
    --cc=akpm@osdl.org \
    --cc=jgarzik@pobox.com \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=netdev@vger.kernel.org \
    --cc=sfr@canb.auug.org.au \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.