[2/2] ksz884x: fix Endian

From: Li RongQing <roy.qing.li@gmail.com>

ETH_P_IP is host Endian, skb->protocol is big Endian, when
compare them, we should change skb->protocol from big endian
to host endian, ntohs, not htons.

CC: Tristram Ha <Tristram.Ha@micrel.com>
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 drivers/net/ethernet/micrel/ksz884x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Thu, 2012-07-05 at 10:06 +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> ETH_P_IP is host Endian, skb->protocol is big Endian, when
> compare them, we should change skb->protocol from big endian
> to host endian, ntohs, not htons.
> 
> CC: Tristram Ha <Tristram.Ha@micrel.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  drivers/net/ethernet/micrel/ksz884x.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/micrel/ksz884x.c b/drivers/net/ethernet/micrel/ksz884x.c
> index eaf9ff0..d9727f7 100644
> --- a/drivers/net/ethernet/micrel/ksz884x.c
> +++ b/drivers/net/ethernet/micrel/ksz884x.c
> @@ -4882,7 +4882,7 @@ static netdev_tx_t netdev_tx(struct sk_buff *skb, struct net_device *dev)
>  	if (left) {
>  		if (left < num ||
>  				((CHECKSUM_PARTIAL == skb->ip_summed) &&
> -				(ETH_P_IPV6 == htons(skb->protocol)))) {
> +				(ETH_P_IPV6 == ntohs(skb->protocol)))) {

This should really be changed to the idiomatic 'skb->protocol ==
htons(ETH_P_IPV6)'.  For the current code, the compiler will probably
generate a run-time byte-swap for little-endian systems.

Ben.

>  			struct sk_buff *org_skb = skb;
>  
>  			skb = netdev_alloc_skb(dev, org_skb->len);

2012/7/7, Ben Hutchings <bhutchings@solarflare.com>:
> On Thu, 2012-07-05 at 10:06 +0800, roy.qing.li@gmail.com wrote:
>> From: Li RongQing <roy.qing.li@gmail.com>
>>
>> ETH_P_IP is host Endian, skb->protocol is big Endian, when
>> compare them, we should change skb->protocol from big endian
>> to host endian, ntohs, not htons.
>>
>> CC: Tristram Ha <Tristram.Ha@micrel.com>
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>> ---
>>  drivers/net/ethernet/micrel/ksz884x.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/micrel/ksz884x.c
>> b/drivers/net/ethernet/micrel/ksz884x.c
>> index eaf9ff0..d9727f7 100644
>> --- a/drivers/net/ethernet/micrel/ksz884x.c
>> +++ b/drivers/net/ethernet/micrel/ksz884x.c
>> @@ -4882,7 +4882,7 @@ static netdev_tx_t netdev_tx(struct sk_buff *skb,
>> struct net_device *dev)
>>  	if (left) {
>>  		if (left < num ||
>>  				((CHECKSUM_PARTIAL == skb->ip_summed) &&
>> -				(ETH_P_IPV6 == htons(skb->protocol)))) {
>> +				(ETH_P_IPV6 == ntohs(skb->protocol)))) {
>
> This should really be changed to the idiomatic 'skb->protocol ==
> htons(ETH_P_IPV6)'.  For the current code, the compiler will probably
> generate a run-time byte-swap for little-endian systems.
>
> Ben.
>
>>  			struct sk_buff *org_skb = skb;
>>
>>  			skb = netdev_alloc_skb(dev, org_skb->len);
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Please ignore the first reply.

OK, I will change it as Ben's suggestion.

Thanks

-Roy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2012-07-09 at 13:26 +0800, RongQing Li wrote:
> 2012/7/7, Ben Hutchings <bhutchings@solarflare.com>:
> > On Thu, 2012-07-05 at 10:06 +0800, roy.qing.li@gmail.com wrote:
> >> ETH_P_IP is host Endian, skb->protocol is big Endian, when
> >> compare them, we should change skb->protocol from big endian
> >> to host endian, ntohs, not htons.
[]
> >> diff --git a/drivers/net/ethernet/micrel/ksz884x.c
[]
> >> @@ -4882,7 +4882,7 @@ static netdev_tx_t netdev_tx(struct sk_buff *skb,
> >> struct net_device *dev)
> >>  	if (left) {
> >>  		if (left < num ||
> >>  				((CHECKSUM_PARTIAL == skb->ip_summed) &&
> >> -				(ETH_P_IPV6 == htons(skb->protocol)))) {
> >> +				(ETH_P_IPV6 == ntohs(skb->protocol)))) {
> >
> > This should really be changed to the idiomatic 'skb->protocol ==
> > htons(ETH_P_IPV6)'.  For the current code, the compiler will probably
> > generate a run-time byte-swap for little-endian systems.

True.  Perhaps this would be better written as:

	if (left) {
		if (left < num ||
		    (ip->ip_summed == CHECKSUM_PARTIAL &&
		     skb->protocol == htons(ETH_P_IPV6))) {

			etc...


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html