All of lore.kernel.org
 help / color / mirror / Atom feed
From: Larry Finger <Larry.Finger@lwfinger.net>
To: Pavel Roskin <proski@gnu.org>
Cc: John W Linville <linville@tuxdriver.com>,
	Michael Buesch <mb@bu3sch.de>,
	zajec5@gmail.com, b43-dev@lists.infradead.org,
	linux-wireless@vger.kernel.org
Subject: [PATCH] ssb: Convert to use crc8 code in kernel library
Date: Fri, 14 Oct 2011 11:30:10 -0500	[thread overview]
Message-ID: <4E986392.7020008@lwfinger.net> (raw)
In-Reply-To: <20111014111103.5658d4aa@mj>

On 10/14/2011 10:11 AM, Pavel Roskin wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger<Larry.Finger@lwfinger.net>  wrote:
>
>> +static inline void htol16_buf(u16 *buf, unsigned int size)
>> +{
>> +	size /= 2;
>> +	while (size--)
>> +		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
>>   }
>
> I'm not not sure compilers would optimize it out on little-endian
> systems.  Perhaps you want a define that uses this code on
> big-endian systems and does nothing on little endian systems.
>
> Also, it would be nice to have a compile-time check that size is even.
> Or maybe size should be the number of 16-bit words, but then it would be
> better to call the argument "count" or something like that.

The patch was dropped. Even so, as this routine is found in brcmsmac, your 
comments warrant further discussion.

I am pretty sure that the compiler would optimize out the entire htol16_buf 
routine. After substitution for cpu_to_le16() on a little-endian system, the 
statement in the while loop becomes '*(buf + size) = *(buf + size)', which is 
certainly optimized away, as will the now empty while loop. The entire routine 
is reduced to 'size /= 2'. As this will have no effect on the external world, it 
will also be dropped leaving an empty htol16_buf(). I don't think any "#ifdef 
__BIG_ENDIAN ... #endif" statements are needed.

Your suggestion that the argument be renamed is good, but there is no need to 
check for an even number as the data in question come from 16-bit reads of the
SPROM on the b43 device. That number of 16-bit quantities was multiplied by 2 to 
get the byte count before calling this routine. Of course, the routine should 
have been passed the number of 16-bit words, not the byte count. My second 
version would have done this.

Larry

WARNING: multiple messages have this Message-ID (diff)
From: Larry Finger <Larry.Finger@lwfinger.net>
To: Pavel Roskin <proski@gnu.org>
Cc: John W Linville <linville@tuxdriver.com>,
	Michael Buesch <mb@bu3sch.de>,
	zajec5@gmail.com, b43-dev@lists.infradead.org,
	linux-wireless@vger.kernel.org
Subject: Re: [PATCH] ssb: Convert to use crc8 code in kernel library
Date: Fri, 14 Oct 2011 11:30:10 -0500	[thread overview]
Message-ID: <4E986392.7020008@lwfinger.net> (raw)
In-Reply-To: <20111014111103.5658d4aa@mj>

On 10/14/2011 10:11 AM, Pavel Roskin wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger<Larry.Finger@lwfinger.net>  wrote:
>
>> +static inline void htol16_buf(u16 *buf, unsigned int size)
>> +{
>> +	size /= 2;
>> +	while (size--)
>> +		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
>>   }
>
> I'm not not sure compilers would optimize it out on little-endian
> systems.  Perhaps you want a define that uses this code on
> big-endian systems and does nothing on little endian systems.
>
> Also, it would be nice to have a compile-time check that size is even.
> Or maybe size should be the number of 16-bit words, but then it would be
> better to call the argument "count" or something like that.

The patch was dropped. Even so, as this routine is found in brcmsmac, your 
comments warrant further discussion.

I am pretty sure that the compiler would optimize out the entire htol16_buf 
routine. After substitution for cpu_to_le16() on a little-endian system, the 
statement in the while loop becomes '*(buf + size) = *(buf + size)', which is 
certainly optimized away, as will the now empty while loop. The entire routine 
is reduced to 'size /= 2'. As this will have no effect on the external world, it 
will also be dropped leaving an empty htol16_buf(). I don't think any "#ifdef 
__BIG_ENDIAN ... #endif" statements are needed.

Your suggestion that the argument be renamed is good, but there is no need to 
check for an even number as the data in question come from 16-bit reads of the
SPROM on the b43 device. That number of 16-bit quantities was multiplied by 2 to 
get the byte count before calling this routine. Of course, the routine should 
have been passed the number of 16-bit words, not the byte count. My second 
version would have done this.

Larry

  parent reply	other threads:[~2011-10-14 16:30 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-08 22:28 [PATCH] ssb: Convert to use crc8 code in kernel library Larry Finger
2011-10-08 22:38 ` Joe Perches
2011-10-08 23:00   ` Larry Finger
2011-10-08 23:00     ` Larry Finger
2011-10-08 23:11     ` Joe Perches
2011-10-08 22:51 ` Michael Büsch
2011-10-08 22:51   ` Michael Büsch
2011-10-09  8:48   ` Rafał Miłecki
2011-10-09  8:48     ` Rafał Miłecki
2011-10-09 10:33     ` Arend van Spriel
2011-10-09 14:35     ` Larry Finger
2011-10-09 14:35       ` Larry Finger
2011-10-09  9:50   ` Arend van Spriel
2011-10-14 15:11 ` Pavel Roskin
2011-10-14 15:30   ` Arend van Spriel
2011-10-14 16:30   ` Larry Finger [this message]
2011-10-14 16:30     ` Larry Finger
2011-10-14 16:47     ` Michael Büsch
2011-10-14 16:47       ` Michael Büsch
2011-10-15  8:27     ` Arend van Spriel
2011-10-15 13:29       ` Larry Finger
2011-10-15 13:29         ` Larry Finger
2011-10-15 13:53         ` Michael Büsch
2011-10-15 13:53           ` Michael Büsch
2011-10-15 14:18           ` Larry Finger
2011-10-15 14:18             ` Larry Finger

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=4E986392.7020008@lwfinger.net \
    --to=larry.finger@lwfinger.net \
    --cc=b43-dev@lists.infradead.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linville@tuxdriver.com \
    --cc=mb@bu3sch.de \
    --cc=proski@gnu.org \
    --cc=zajec5@gmail.com \
    /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.