i386 allmodconfig isn't that hard, guys.
drivers/net/wireless/mac80211/zd1211rw/zd_mac.c:600: warning: 'fill_rt_header' defined but not used
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c: In function 'iwl_hw_tx_queue_free_tfd':
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:964: warning: left shift count >= width of type
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c: In function 'iwl_hw_tx_queue_attach_buffer_to_tfd':
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:2041: warning: left shift count >= width of type
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:2041: warning: left shift count >= width of type
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:2047: warning: left shift count >= width of type
drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:2050: warning: left shift count >= width of type
With some trepidation I looked in just that header.
> #define iwl_get_bits(src, pos, len) \
> ({ \
> u32 __tmp = le32_to_cpu(src); \
> __tmp >>= pos; \
> __tmp &= (1UL << len) - 1; \
> __tmp; \
> })
Can be a inlined C function. Should be commented.
> #define iwl_set_bits(dst, pos, len, val) \
> ({ \
> u32 __tmp = le32_to_cpu(*dst); \
> __tmp &= ~((1UL << (pos+len)) - (1 << pos)); \
> __tmp |= (val & ((1UL << len) - 1)) << pos; \
> *dst = cpu_to_le32(__tmp); \
> })
Ditto. Whitespace broken.
> #define _IWL_SET_BITS(s, d, o, l, v) \
> iwl_set_bits(&s.d, o, l, v)
>
> #define IWL_SET_BITS(s, sym, v) \
> _IWL_SET_BITS((s), IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, IWL_ ## sym ## _LEN, (v))
>
> #define _IWL_GET_BITS(s, v, o, l) \
> iwl_get_bits(s.v, o, l)
>
> #define IWL_GET_BITS(s, sym) \
> _IWL_GET_BITS((s), IWL_ ## sym ## _SYM, IWL_ ## sym ## _POS, IWL_ ## sym ## _LEN)
Shudder.
> /*
> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
> */
> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
This is identical to ARRAY_SIZE.
And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
off and create some private thing and leave everyone else twisting in the
wind.
> /* Debug and printf string expansion helpers for printing bitfields */
> #define BIT_FMT8 "%c%c%c%c-%c%c%c%c"
> #define BIT_FMT16 BIT_FMT8 ":" BIT_FMT8
> #define BIT_FMT32 BIT_FMT16 " " BIT_FMT16
>
> #define BITC(x,y) (((x>>y)&1)?'1':'0')
> #define BIT_ARG8(x) \
> BITC(x,7),BITC(x,6),BITC(x,5),BITC(x,4),\
> BITC(x,3),BITC(x,2),BITC(x,1),BITC(x,0)
>
> #define BIT_ARG16(x) \
> BITC(x,15),BITC(x,14),BITC(x,13),BITC(x,12),\
> BITC(x,11),BITC(x,10),BITC(x,9),BITC(x,8),\
> BIT_ARG8(x)
>
> #define BIT_ARG32(x) \
> BITC(x,31),BITC(x,30),BITC(x,29),BITC(x,28),\
> BITC(x,27),BITC(x,26),BITC(x,25),BITC(x,24),\
> BITC(x,23),BITC(x,22),BITC(x,21),BITC(x,20),\
> BITC(x,19),BITC(x,18),BITC(x,17),BITC(x,16),\
> BIT_ARG16(x)
None of the above is appropriate to a driver-private header.
> #define KELVIN_TO_CELSIUS(x) ((x)-273)
Nor is that.
> #define IEEE80211_CHAN_W_RADAR_DETECT 0x00000010
>
> static inline struct ieee80211_conf *ieee80211_get_hw_conf(struct ieee80211_hw
> *hw)
> {
> return &hw->conf;
> }
>
> static inline const struct ieee80211_hw_mode *iwl_get_hw_mode(struct iwl_priv
> *priv, int mode)
> {
> int i;
>
> for (i = 0; i < 3; i++)
> if (priv->modes[i].mode == mode)
> return &priv->modes[i];
>
> return NULL;
> }
Far too large to inline, has five callsites.
> #define WLAN_FC_GET_TYPE(fc) (((fc) & IEEE80211_FCTL_FTYPE))
> #define WLAN_FC_GET_STYPE(fc) (((fc) & IEEE80211_FCTL_STYPE))
> #define WLAN_GET_SEQ_FRAG(seq) ((seq) & 0x000f)
> #define WLAN_GET_SEQ_SEQ(seq) ((seq) >> 4)
These don't need to be macros
> #define QOS_CONTROL_LEN 2
>
> static inline u16 *ieee80211_get_qos_ctrl(struct ieee80211_hdr *hdr)
> {
> int hdr_len = ieee80211_get_hdrlen(hdr->frame_control);
> if (hdr->frame_control & IEEE80211_STYPE_QOS_DATA)
> return (u16 *) ((u8 *) hdr + (hdr_len) - QOS_CONTROL_LEN);
> return NULL;
> }
Two callsites, too large to inline.
> #define IEEE80211_STYPE_BACK_REQ 0x0080
> #define IEEE80211_STYPE_BACK 0x0090
>
> #define ieee80211_is_back_request(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_CTL) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_BACK_REQ))
>
> #define ieee80211_is_probe_response(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> ( WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_PROBE_RESP ))
>
> #define ieee80211_is_probe_request(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> ( WLAN_FC_GET_STYPE(fc) ==IEEE80211_STYPE_PROBE_REQ ))
>
> #define ieee80211_is_beacon(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> ( WLAN_FC_GET_STYPE(fc) ==IEEE80211_STYPE_BEACON ))
>
> #define ieee80211_is_atim(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> ( WLAN_FC_GET_STYPE(fc) ==IEEE80211_STYPE_ATIM ))
>
> #define ieee80211_is_management(fc) \
> (WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT)
>
> #define ieee80211_is_control(fc) \
> (WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_CTL)
>
> #define ieee80211_is_data(fc) \
> (WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_DATA)
>
> #define ieee80211_is_assoc_request(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_ASSOC_REQ))
>
> #define ieee80211_is_assoc_response(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_ASSOC_RESP))
>
> #define ieee80211_is_auth(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_ASSOC_REQ))
>
> #define ieee80211_is_deauth(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_ASSOC_REQ))
>
> #define ieee80211_is_disassoc(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_ASSOC_REQ))
>
> #define ieee80211_is_reassoc_request(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_REASSOC_REQ))
>
> #define ieee80211_is_reassoc_response(fc) \
> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_REASSOC_RESP))
None of the above should be macros.
> static inline int iwl_is_empty_essid(const char *essid, int essid_len)
> {
> /* Single white space is for Linksys APs */
> if (essid_len == 1 && essid[0] == ' ')
> return 1;
>
> /* Otherwise, if the entire essid is 0, we assume it is hidden */
> while (essid_len) {
> essid_len--;
> if (essid[essid_len] != '\0')
> return 0;
> }
>
> return 1;
> }
The above large function gets inlined into iwl_escape_essid()
> static inline int iwl_check_bits(unsigned long field, unsigned long mask)
> {
> return ((field & mask) == mask) ? 1 : 0;
> }
>
> static inline const char *iwl_escape_essid(const char *essid, u8 essid_len)
> {
> static char escaped[IW_ESSID_MAX_SIZE * 2 + 1];
> const char *s = essid;
> char *d = escaped;
>
> if (iwl_is_empty_essid(essid, essid_len)) {
> memcpy(escaped, "<hidden>", sizeof("<hidden>"));
> return escaped;
> }
>
> essid_len = min(essid_len, (u8) IW_ESSID_MAX_SIZE);
> while (essid_len--) {
> if (*s == '\0') {
> *d++ = '\\';
> *d++ = '0';
> s++;
> } else {
> *d++ = *s++;
> }
> }
> *d = '\0';
> return escaped;
> }
This now-enormous function has three callsites. Madness!
> static inline unsigned long elapsed_jiffies(unsigned long start,
> unsigned long end)
> {
> if (end > start)
> return end - start;
>
> return end + (MAX_JIFFY_OFFSET - start);
> }
Whatever this uncommented function is doing, it is not appropriate to a
per-driver header file.
> #include <linux/ctype.h>
This should go at the top of the file.
> static inline int snprint_line(char *buf, size_t count,
> const u8 * data, u32 len, u32 ofs)
> {
> int out, i, j, l;
> char c;
>
> out = snprintf(buf, count, "%08X", ofs);
>
> for (l = 0, i = 0; i < 2; i++) {
> out += snprintf(buf + out, count - out, " ");
> for (j = 0; j < 8 && l < len; j++, l++)
> out +=
> snprintf(buf + out, count - out, "%02X ",
> data[(i * 8 + j)]);
> for (; j < 8; j++)
> out += snprintf(buf + out, count - out, " ");
> }
> out += snprintf(buf + out, count - out, " ");
> for (l = 0, i = 0; i < 2; i++) {
> out += snprintf(buf + out, count - out, " ");
> for (j = 0; j < 8 && l < len; j++, l++) {
> c = data[(i * 8 + j)];
> if (!isascii(c) || !isprint(c))
> c = '.';
>
> out += snprintf(buf + out, count - out, "%c", c);
> }
>
> for (; j < 8; j++)
> out += snprintf(buf + out, count - out, " ");
> }
>
> return out;
> }
We're kidding. Three callsites!
Dump the whole thing, use lib/hexdump.c.
> #ifdef CONFIG_IWLWIFI_DEBUG
> static inline void printk_buf(int level, const void *p, u32 len)
> {
> const u8 *data = p;
> char line[81];
> u32 ofs = 0;
> if (!(iwl_debug_level & level))
> return;
>
> while (len) {
> snprint_line(line, sizeof(line), &data[ofs],
> min(len, 16U), ofs);
> printk(KERN_DEBUG "%s\n", line);
> ofs += 16;
> len -= min(len, 16U);
> }
> }
This is even huger and it has six callsites.
> #else
> #define printk_buf(level, p, len) do {} while (0)
> #endif
>
> #endif /* __iwl_helpers_h__ */
And that's one file out of a 3MB diff.
Please, don't anybody dare think about thinking about letting this anywhere
near mainline until it has had a thorough review. Or at least, a little bit
of review.
<goes back to fixing all the new compile errors and warnings>
Andrew Morton wrote:
> i386 allmodconfig isn't that hard, guys.
>
...
> drivers/net/wireless/mac80211/iwlwifi/iwl-4965.c:2050: warning: left shift count >= width of type
My mistake. I ran that and even fixed the warning at one point... anyway, I've committed a patch to our tree to fix the code and also added comments to the iwl_{get,set}_bits code.
>> /*
>> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
>> */
>> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
> This is identical to ARRAY_SIZE.
>
> And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
> off and create some private thing and leave everyone else twisting in the
> wind.
The code was resolving the sparse warnings. GLOBAL_ARRAY_SIZE removes the part of the ARRAY_SIZE definition that causes sparse to complain ('+ __must_be_array(arr)'). I don't know enough about how sparse works to fix sparse, or to know if its a problem with __must_be_array. The code itself was fine -- using an array with ARRAY_SIZE.
>> /* Debug and printf string expansion helpers for printing bitfields */
>> #define BIT_FMT8 "%c%c%c%c-%c%c%c%c"
>> #define BIT_FMT16 BIT_FMT8 ":" BIT_FMT8
>> #define BIT_FMT32 BIT_FMT16 " " BIT_FMT16
...
>> #define BIT_ARG32(x) \
>> BITC(x,31),BITC(x,30),BITC(x,29),BITC(x,28),\
>> BITC(x,27),BITC(x,26),BITC(x,25),BITC(x,24),\
>> BITC(x,23),BITC(x,22),BITC(x,21),BITC(x,20),\
>> BITC(x,19),BITC(x,18),BITC(x,17),BITC(x,16),\
>> BIT_ARG16(x)
>
> None of the above is appropriate to a driver-private header.
Where would be the appropriate place? We use it in with iwlwifi; I don't know if others need it or want to use it. Do you know of other projects using something similar?
>> #define KELVIN_TO_CELSIUS(x) ((x)-273)
>
> Nor is that.
Where would the appropriate place be?
>> static inline const struct ieee80211_hw_mode *iwl_get_hw_mode(struct iwl_priv
>> *priv, int mode)
>> {
>> int i;
>>
>> for (i = 0; i < 3; i++)
>> if (priv->modes[i].mode == mode)
>> return &priv->modes[i];
>>
>> return NULL;
>> }
>
> Far too large to inline, has five callsites.
Currently CodingStyle states to use inline where you might have otherwise used a macro, and then later if the function is not overly complex (citing "3 lines" as a guideline). Is this too long because it has a for loop in it? Or a loop and a branch?
Removing static inline from the functions declared in header files means they need to be moved to .c files, declared as extern, and pollute the namespace. In prior drivers we had been beaten up about doing that...
>> #define WLAN_FC_GET_TYPE(fc) (((fc) & IEEE80211_FCTL_FTYPE))
>> #define WLAN_FC_GET_STYPE(fc) (((fc) & IEEE80211_FCTL_STYPE))
>> #define WLAN_GET_SEQ_FRAG(seq) ((seq) & 0x000f)
>> #define WLAN_GET_SEQ_SEQ(seq) ((seq) >> 4)
>
> These don't need to be macros
What would you like these to be?
These currently exist as macros in ieee80211.h and there are other examples in the kernel of similar macros. If a goal is to remove *all macros* then that should be stated in CodingStyle, preferably in a way that helps developers understand how they are supposed to write their code.
>> #define QOS_CONTROL_LEN 2
>>
>> static inline u16 *ieee80211_get_qos_ctrl(struct ieee80211_hdr *hdr)
>> {
>> int hdr_len = ieee80211_get_hdrlen(hdr->frame_control);
>> if (hdr->frame_control & IEEE80211_STYPE_QOS_DATA)
>> return (u16 *) ((u8 *) hdr + (hdr_len) - QOS_CONTROL_LEN);
>> return NULL;
>> }
>
> Two callsites, too large to inline.
If something is used more than once then it is unsuitable for an inline? Again maybe updating CodingStyle would be helpful?
Change:
"Generally, inline functions are preferable to macros resembling functions."
To:
"Macros resembling functions and inline functions should *NOT* be used."
IIRC, ieee80211_get_qos_ctrl used to be a macros, and was then changed to an inline per CodingStyle. Now we don't want inlines, and instead want pure functions (and consequently polluted namespace--or do we want to add to CodingStyle that all functions should be implemented in a single file and marked static?)
"A reasonable rule of thumb is to not put inline at functions that have more
than 3 lines of code in them. "
An assignment and a branch return isn't overly complex; it does call another function... is that the problem?
If callsites is a key indicator of when to and not to use inline, please add that to CodingStyle. We look at other drivers and headers, we see what they do, and we replicate. We read CodingStyle and use it as a guideline.
...
>> #define ieee80211_is_reassoc_response(fc) \
>> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
>> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_REASSOC_RESP))
>
> None of the above should be macros.
So inline with these (they have lots of callsites...) or ?
>> static inline unsigned long elapsed_jiffies(unsigned long start,
>> unsigned long end)
>> {
>> if (end > start)
>> return end - start;
>>
>> return end + (MAX_JIFFY_OFFSET - start);
>> }
>
> Whatever this uncommented function is doing, it is not appropriate to a
> per-driver header file.
Where should it go? jiffies.h?
Is there already a kernel function for doing this? (I couldn't find one.)
> Dump the whole thing, use lib/hexdump.c.
Agreed; iwlwifi should be using lib/hexcump.c
James
John W. Linville wrote:
> On Tue, Jun 05, 2007 at 02:06:14AM -0700, Andrew Morton wrote:
>
>> Please, don't anybody dare think about thinking about letting this anywhere
>> near mainline until it has had a thorough review. Or at least, a little bit
>> of review.
>
> Don't worry -- I assure you that everyone is aware of the issues.
>
> John
Yes, we certainly don't want a driver to be "near mainline" that does things that the rest of the kernel and other drivers are doing. We should force them to stay out-of-tree until any and everything is resolved. Heaven forbid that the code should be merged, contributed, and improved upon as a community.
You'd think these guys were trying to enable folks with a driver so they can use the hardware they bought or something. Its almost laughable. What idiots.
James
On Tue, Jun 05, 2007 at 01:12:03PM -0700, James Ketrenos wrote:
> Yes, we certainly don't want a driver to be "near mainline" that does
> things that the rest of the kernel and other drivers are doing. We should
> force them to stay out-of-tree until any and everything is resolved.
> Heaven forbid that the code should be merged, contributed, and improved
> upon as a community.
>
> You'd think these guys were trying to enable folks with a driver so they
> can use the hardware they bought or something. Its almost laughable. What
> idiots.
Maybe you should make sure your companies publishes specs or useable drivers
earlier, and you should stop writing crap code instead of these hypocritical
comments. We're a big project and we need to be up to some standards before
we can put code in that we'll have to maintain then.
>
> James
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
Christoph Hellwig wrote:
> On Tue, Jun 05, 2007 at 01:12:03PM -0700, James Ketrenos wrote:
>> Yes, we certainly don't want a driver to be "near mainline" that does
>> things that the rest of the kernel and other drivers are doing. We should
>> force them to stay out-of-tree until any and everything is resolved.
>> Heaven forbid that the code should be merged, contributed, and improved
>> upon as a community.
>>
>> You'd think these guys were trying to enable folks with a driver so they
>> can use the hardware they bought or something. Its almost laughable. What
>> idiots.
>
> Maybe you should make sure your companies publishes specs
I'm flattered that you think I am high enough at Intel to do that. If it was in my power, accurate and complete specifications would be available for all of Intel's hardware--for everyone. I doubt you'll find a single Linux developer at Intel who is not fighting to try and reach that goal, including myself.
The reality is, however, (and this may shock you) I am not a vice president or head of a major division at Intel. Its true. Intel doesn't do what I want just because I snap my fingers and say it should be so.
Instead, we try and do what we can -- enable people with a driver (and a community to contribute into) so that the hardware they purchased can operate to its fullest potential on Linux. We realize within the community that some areas are difficult for people outside of Intel to initially contribute to given the lack of specs. However, there are vast quantities of features and functionality that can be enabled and improved given nothing more than the driver sources.
Anyway, its obvious from your emails that our efforts are not sufficient and you seem to resent that we're even trying. I apologize.
If you'd like to submit patches to help us make the perfect driver, we're very willing to review and merge those in.
James
On Wed, 06 Jun 2007 13:51:41 -0700 James Ketrenos <[email protected]> wrote:
>
> >> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
> >> */
> >> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >
> > This is identical to ARRAY_SIZE.
> >
> > And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
> > off and create some private thing and leave everyone else twisting in the
> > wind.
>
> The code was resolving the sparse warnings. GLOBAL_ARRAY_SIZE removes the part of the ARRAY_SIZE definition that causes sparse to complain ('+ __must_be_array(arr)'). I don't know enough about how sparse works to fix sparse, or to know if its a problem with __must_be_array. The code itself was fine -- using an array with ARRAY_SIZE.
(These 340-column emails are rather hard to reply to)
Your GLOBAL_ARRAY_SIZE() is, afaict, identical to ARRAY_SIZE().
If ARRAY_SIZE() is spitting some sparse warning then please report it and
we'll take a look into it.
>
> >> /* Debug and printf string expansion helpers for printing bitfields */
> >> #define BIT_FMT8 "%c%c%c%c-%c%c%c%c"
> >> #define BIT_FMT16 BIT_FMT8 ":" BIT_FMT8
> >> #define BIT_FMT32 BIT_FMT16 " " BIT_FMT16
> ...
> >> #define BIT_ARG32(x) \
> >> BITC(x,31),BITC(x,30),BITC(x,29),BITC(x,28),\
> >> BITC(x,27),BITC(x,26),BITC(x,25),BITC(x,24),\
> >> BITC(x,23),BITC(x,22),BITC(x,21),BITC(x,20),\
> >> BITC(x,19),BITC(x,18),BITC(x,17),BITC(x,16),\
> >> BIT_ARG16(x)
> >
> > None of the above is appropriate to a driver-private header.
>
> Where would be the appropriate place?
Dunno. include/linux/bitfield-helpers.h?
> We use it in with iwlwifi; I don't know if others need it or want to use it. Do you know of other projects using something similar?
I haven't looked.
> >> #define KELVIN_TO_CELSIUS(x) ((x)-273)
> >
> > Nor is that.
>
> Where would the appropriate place be?
include/linux/temperature.h? acpi could use it, and there are other things
we could put into temperature.h
> >> static inline const struct ieee80211_hw_mode *iwl_get_hw_mode(struct iwl_priv
> >> *priv, int mode)
> >> {
> >> int i;
> >>
> >> for (i = 0; i < 3; i++)
> >> if (priv->modes[i].mode == mode)
> >> return &priv->modes[i];
> >>
> >> return NULL;
> >> }
> >
> > Far too large to inline, has five callsites.
>
> Currently CodingStyle states to use inline where you might have otherwise used a macro, and then later if the function is not overly complex (citing "3 lines" as a guideline). Is this too long because it has a for loop in it? Or a loop and a branch?
Anything more than 10-20 instructions turns out to be too large.
> Removing static inline from the functions declared in header files means they need to be moved to .c files, declared as extern, and pollute the namespace. In prior drivers we had been beaten up about doing that...
You were mis-beaten-up. Choose an appropriate namespace (iwl_* sounds OK),
stick to it and you'll be fine.
> >> #define WLAN_FC_GET_TYPE(fc) (((fc) & IEEE80211_FCTL_FTYPE))
> >> #define WLAN_FC_GET_STYPE(fc) (((fc) & IEEE80211_FCTL_STYPE))
> >> #define WLAN_GET_SEQ_FRAG(seq) ((seq) & 0x000f)
> >> #define WLAN_GET_SEQ_SEQ(seq) ((seq) >> 4)
> >
> > These don't need to be macros
>
> What would you like these to be?
/*
* comment goes here
*/
static inline suitable_return_type wlan_fc_get_type(whatever_type_fc_has fc)
{
return fc & IEEE80211_FCTL_FTYPE;
}
> These currently exist as macros in ieee80211.h and there are other examples in the kernel of similar macros. If a goal is to remove *all macros* then that should be stated in CodingStyle, preferably in a way that helps developers understand how they are supposed to write their code.
These things come up again and again in lkml code-review. Could be that
CodingStyle doesn't cover everything. Common sense and taste applies too.
> >> #define QOS_CONTROL_LEN 2
> >>
> >> static inline u16 *ieee80211_get_qos_ctrl(struct ieee80211_hdr *hdr)
> >> {
> >> int hdr_len = ieee80211_get_hdrlen(hdr->frame_control);
> >> if (hdr->frame_control & IEEE80211_STYPE_QOS_DATA)
> >> return (u16 *) ((u8 *) hdr + (hdr_len) - QOS_CONTROL_LEN);
> >> return NULL;
> >> }
> >
> > Two callsites, too large to inline.
>
> If something is used more than once then it is unsuitable for an inline? Again maybe updating CodingStyle would be helpful?
>
> Change:
> "Generally, inline functions are preferable to macros resembling functions."
>
> To:
> "Macros resembling functions and inline functions should *NOT* be used."
>
> IIRC, ieee80211_get_qos_ctrl used to be a macros, and was then changed to an inline per CodingStyle. Now we don't want inlines, and instead want pure functions (and consequently polluted namespace--or do we want to add to CodingStyle that all functions should be implemented in a single file and marked static?)
inlines are better than macros because they are more readable, because they
have typechecking and because for some reasons programmers are more likely
to comment inlines than they are to comment macros.
The general guideline for inlines is "don't do it if it makes .text
larger". Because larger is generally slower. Plus it's larger. There are
exceptions to this, but they're special-cases.
> ...
> >> #define ieee80211_is_reassoc_response(fc) \
> >> ((WLAN_FC_GET_TYPE(fc) == IEEE80211_FTYPE_MGMT) && \
> >> (WLAN_FC_GET_STYPE(fc) == IEEE80211_STYPE_REASSOC_RESP))
> >
> > None of the above should be macros.
>
> So inline with these (they have lots of callsites...) or ?
I wouldn't expect significant speed and space savings from uninlining these
- they look appropriate for inlining.
> >> static inline unsigned long elapsed_jiffies(unsigned long start,
> >> unsigned long end)
> >> {
> >> if (end > start)
> >> return end - start;
> >>
> >> return end + (MAX_JIFFY_OFFSET - start);
> >> }
> >
> > Whatever this uncommented function is doing, it is not appropriate to a
> > per-driver header file.
>
> Where should it go? jiffies.h?
Sounds reasonable.
> Is there already a kernel function for doing this? (I couldn't find one.)
I don't know what the above does, but it _looks_ general-purpose to me.
Unless you think that no other subsystem is likely to need this then yeah,
it should be proposed as a kernel-wide thing, and added (with
documentation) to the appropriate place in core kernel.
> Agreed; iwlwifi should be using lib/hexcump.c
The hexdump.c interfaces are about to change, so I wouldn't do anything
with this until after 2.6.23-rc1.
On Wed, 06 Jun 2007 15:33:46 -0700 James Ketrenos <[email protected]> wrote:
> Andrew Morton wrote:
> > On Wed, 06 Jun 2007 13:51:41 -0700 James Ketrenos <[email protected]> wrote:
> >
> >>>> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
> >>>> */
> >>>> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >>> This is identical to ARRAY_SIZE.
> >>>
> >>> And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
> >>> off and create some private thing and leave everyone else twisting in the
> >>> wind.
> >> The code was resolving the sparse warnings.
> >
> ...
> > Your GLOBAL_ARRAY_SIZE() is, afaict, identical to ARRAY_SIZE().
>
> >From include/linux/kernel.h
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
o crap, sorry, I was looking at one of the other definitions of ARRAY_SIZE :(
> >From drivers/net/wireless/mac80211/iwlwifi/iwl-helpers.h
>
> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
> The '+ __must_be_array(arr)' part of ARRAY_SIZE was causing sparse to complain
> with:
>
> drivers/net/wireless/mac80211/iwlwifi/base.c:4646:22: error: cannot size expression
> ...
OK, that's a problem in sparse, I guess.
There _should_ be some #ifdeffable thing which is being passed to cpp when
we run sparse (but I'm not sure what it is). If there is such a thing then
we could disable the __must_be_array() thing during sparse runs.
But longer-term, sparse should be taught about __must_be_array().
> When I had run my builds, I had restricted the sparse checks to just iwlwifi
> (vs. checking the rest of the kernel).
>
> I just ran it C=2 CF=-Wall against the rest of the kernel and do see other code
> with the same problem, eg:
>
> sound/core/memalloc.c:521:14: error: cannot size expression
> ...
>
> I had erroneously thought it was just a problem with iwlwifi...
>
On Tue, Jun 05, 2007 at 02:06:14AM -0700, Andrew Morton wrote:
> Please, don't anybody dare think about thinking about letting this anywhere
> near mainline until it has had a thorough review. Or at least, a little bit
> of review.
Don't worry -- I assure you that everyone is aware of the issues.
John
--
John W. Linville
[email protected]
On Wed, Jun 06, 2007 at 06:04:21PM -0700, Andrew Morton wrote:
> There _should_ be some #ifdeffable thing which is being passed to cpp when
> we run sparse (but I'm not sure what it is).
#ifdef __CHECKER__
(See include/linux/compiler.h, this is how we implement __user & friends)
Dave
--
http://www.codemonkey.org.uk
On Tue, 05 Jun 2007 13:12:03 -0700
James Ketrenos <[email protected]> wrote:
> John W. Linville wrote:
> > On Tue, Jun 05, 2007 at 02:06:14AM -0700, Andrew Morton wrote:
> >
> >> Please, don't anybody dare think about thinking about letting this anywhere
> >> near mainline until it has had a thorough review. Or at least, a little bit
> >> of review.
> >
> > Don't worry -- I assure you that everyone is aware of the issues.
> >
> > John
>
> Yes, we certainly don't want a driver to be "near mainline" that does things that the rest of the kernel and other drivers are doing. We should force them to stay out-of-tree until any and everything is resolved. Heaven forbid that the code should be merged, contributed, and improved upon as a community.
That isn't the only decision criterion.
Overall the c files look reasonable to me: a few little things like large
on-stack arrays built at runtim which I think could be assembled at
compile-time, various unneeded casts, a bit of space-vs-tab confusion, but
nothing serious leaps out.
So perhaps that header file was unrepresentative. It is seriously
duplicative and bloaty though.
This:
akpm:/usr/src/25> perl scripts/checkpatch.pl patches/git-wireless.patch | wc -l
9941
should be an endless source of fun.
On Wed, 6 Jun 2007 16:35:34 -0700 Andrew Morton wrote:
> On Wed, 06 Jun 2007 13:51:41 -0700 James Ketrenos <[email protected]> wrote:
>
> >
> > >> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
> > >> */
> > >> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > >
> > > This is identical to ARRAY_SIZE.
> > >
> > > And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
> > > off and create some private thing and leave everyone else twisting in the
> > > wind.
> >
> > The code was resolving the sparse warnings. GLOBAL_ARRAY_SIZE removes the part of the ARRAY_SIZE definition that causes sparse to complain ('+ __must_be_array(arr)'). I don't know enough about how sparse works to fix sparse, or to know if its a problem with __must_be_array. The code itself was fine -- using an array with ARRAY_SIZE.
>
> (These 340-column emails are rather hard to reply to)
>
> Your GLOBAL_ARRAY_SIZE() is, afaict, identical to ARRAY_SIZE().
>
> If ARRAY_SIZE() is spitting some sparse warning then please report it and
> we'll take a look into it.
Looks like a sparse problem. See
http://marc.info/?l=linux-sparse&m=118105261224378&w=2
> > Agreed; iwlwifi should be using lib/hexcump.c
>
> The hexdump.c interfaces are about to change, so I wouldn't do anything
> with this until after 2.6.23-rc1.
James, hexdump.c changes are already in 2.6.22-rc4-mm1 just in case
you want to see them.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
Andrew Morton wrote:
> On Wed, 06 Jun 2007 13:51:41 -0700 James Ketrenos <[email protected]> wrote:
>
>>>> * make C=2 CF=-Wall will complain if you use ARRAY_SIZE on global data
>>>> */
>>>> #define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>> This is identical to ARRAY_SIZE.
>>>
>>> And if there's some problem with ARRAY_SIZE then fix ARRAY_SIZE! Don't go
>>> off and create some private thing and leave everyone else twisting in the
>>> wind.
>> The code was resolving the sparse warnings.
>
...
> Your GLOBAL_ARRAY_SIZE() is, afaict, identical to ARRAY_SIZE().
>From include/linux/kernel.h
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>From drivers/net/wireless/mac80211/iwlwifi/iwl-helpers.h
#define GLOBAL_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
The '+ __must_be_array(arr)' part of ARRAY_SIZE was causing sparse to complain
with:
drivers/net/wireless/mac80211/iwlwifi/base.c:4646:22: error: cannot size expression
...
When I had run my builds, I had restricted the sparse checks to just iwlwifi
(vs. checking the rest of the kernel).
I just ran it C=2 CF=-Wall against the rest of the kernel and do see other code
with the same problem, eg:
sound/core/memalloc.c:521:14: error: cannot size expression
...
I had erroneously thought it was just a problem with iwlwifi...
Thanks,
James