by James Ketrenos

[permalink] [raw]

On Friday 18 May 2007 16:05, Christoph Hellwig wrote:
> On Wed, May 16, 2007 at 02:45:32PM -0700, James Ketrenos wrote:
> > drivers/net/wireless/mac80211/Kconfig | 1 +
> > drivers/net/wireless/mac80211/Makefile | 1 +
> > drivers/net/wireless/mac80211/iwlwifi/Kconfig | 96 +
> > drivers/net/wireless/mac80211/iwlwifi/Makefile | 27 +
> > drivers/net/wireless/mac80211/iwlwifi/base.c | 8146
>
> You're not reading linux-wireless a lot, are you? The last
> driver submission got a pretty clear answer that there should not
> be a drivers/net/wireless/mac80211/ subdirectory.
>
This is just for wireless-dev. When it's being pushed for wireless-2.6 and
upstream it should go away then.

-Michael Wu

Attachments:

(No filename) (746.00 B)
(No filename) (189.00 B)
Download all attachments

2007-05-18 22:05:51

by James Ketrenos

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers

Jeff Garzik wrote:
> James Ketrenos wrote:
>> Randy Dunlap wrote:
>>> 15. Limit source lines to <= 80 columns (this patch contains
>>> over 200 lines that are > 80 columns).
>>
>> Fixed in the .c files. Still have 155 instances in the .h files (most
>> due to mid-line tabs)
>
> See my email on this subject. This is NOT a hard and fast rule. If you
> have to do something stupid to the formatting to get it to fit, then you
> are moving in the wrong direction.

The bulk of the problems in the .h files are caused by Lindent injecting tabs instead of spaces between the C language item and the comment describing it, blowing the comments over 80.

That said, some of the define names are horrendously long... over time we can get those cleaned up w/ some sed scripts.

> As an aside, sometimes long lines are an indication that a function
> needs to be broken up into smaller sub-functions.

Nod.

James

2007-05-18 23:05:51

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
> Alright; I'll add replacing the bitfields with bitwise access to the todo
> list.
>
> Are you OK with us marking the driver as not working on big endian for now
> and getting this one fixed over time?

Given that Intel still hasn't fixed the older ipw2X00 drivers for big
endian despite clong-time onstant prodding I'm dead set against this.
It just won't happen if you don't do it now.

2007-05-21 16:27:50

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

On Mon, May 21, 2007 at 07:56:10AM -0700, James Ketrenos wrote:
> Christoph Hellwig wrote:
> >On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
> >>Alright; I'll add replacing the bitfields with bitwise access to the todo
> >>list.
> >>
> >>Are you OK with us marking the driver as not working on big endian for
> >>now and getting this one fixed over time?
> >
> >Given that Intel still hasn't fixed the older ipw2X00 drivers for big
> >endian despite clong-time onstant prodding I'm dead set against this.
>
> Really? Long time constant prodding?
>
> I'll admit I'm not CC:'d on every email regarding the ipw2100 and ipw2200,
> so if I missed a long-time and constant thread, please forward it my way.
> Searching Google for 'ipw endian' (and several other searches) found a
> reference to a fix for an endian issue when it was raised for the ipw2200.

Just from my outbox I have a thread in August 2006 where I tell Zhu Yi
to fix the endianess problems instead of masking them, and even offer
testing on big endian when Intel sends me hardware. Despite that this
got completely ignored and resubmitted in december, where I replied again.
And probably quite a few more I haven't caught.

2007-05-17 01:52:50

by Michael Wu

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers

On Wednesday 16 May 2007 17:45, James Ketrenos wrote:
> This patch adds the iwlwifi project directory and sources needed to
> build the mac80211 based wireless drivers for the Intel PRO/Wireless
> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN adapters.
>
Looks much better than when I last checked. A few comments:

- The enter/leave stuff should be removed.
- Many of the in_interrupt() checks are unnecessary. Jiri recently documented
which callbacks need to be atomic - the ones that don't need to be atomic
definitely won't be called in an interrupt.
- Get rid of the stub functions for callbacks, like set_multicast_list and
reset. mac80211 checks if those callbacks are defined before calling them.
- CONFIG_IWLWIFI_QOS doesn't appear to be defined.
- Read and set the MAC address before calling ieee80211_register_hw.
- flush_scheduled_work in d_stop can lead to deadlocks since RTNL is held. It
looks like everything is running on a dedicated workqueue so that doesn't
really even do anything. priv->workqueue should be flushed instead if needed.
- mac80211 (very recently) sets up a workqueue for the driver to use so you
don't need to create your own anymore.
- param_auto_create looks very suspicious. mac80211 already creates an adhoc
station if it can't join one.. what's this for?
- The suspend and resume callback setting in struct pci_driver iwl_driver
needs to be put on two lines.
- Why are probe requests being dropped in adhoc mode? (assuming hardware
handles it.. but the message output for that doesn't sound like it)
- The bitwise ORs shouldn't be getting a line to itself in iwl3945_rx_init and
iwl3945_tx_reset.
- Mixed case variable name in iwl_hw_txq_ctx_stop: queueId.
- CONFIG_IWLWIFI_MONITOR and CONFIG_IEEE80211_RADIOTAP doesn't look right.
- RX_FLAG_RADIOTAP should be set in the ieee80211_rx_status flag when a
radiotap header is added to a frame. (otherwise, mac80211 will add its own
radiotap header)
- If you don't want to add a radiotap header all the time, you should check
the IEEE80211_CONF_RADIOTAP flag to see if a radiotap header is desired by
the stack. (not whether or not we're configured as IEEE80211_IF_TYPE_MNTR)
- Splitting rx_start->phy_flags over two lines in iwl4965_rx_reply_rx doesn't
look good.
- The IEEE80211_FTYPE_MGMT case in the switch statement inside
iwl4965_rx_reply_rx doesn't need that extra set of braces. Also, the &&
and || don't need their own lines.
- param_channel looks suspicious too.
- ieee80211chan2mhz is provided by ieee80211_radiotap.h.

Haven't seen anything severe enough to be a blocker for wireless-dev, however.

-Michael Wu

Attachments:

(No filename) (2.58 kB)
(No filename) (189.00 B)
Download all attachments

2007-05-23 01:07:42

On Wednesday 16 May 2007 22:56, Jeff Garzik wrote:
> This is the maintainer's personal preference. I put them in all my
> drivers, and they look perfectly fine here.
>
I consider it generally ugly and to be avoided. These seem to be placed only
in the mac80211 callbacks so it's more of a tool to figure out when mac80211
uses callbacks.. doesn't really belong here IMHO.

But.. if James likes them, fine. I haven't seen anything better using the gcc
mcount stuff Christoph Hellwig was talking about so this is the only option.

-Michael Wu

Attachments:

(No filename) (545.00 B)
(No filename) (189.00 B)
Download all attachments

2007-05-17 15:19:36

by Stephen Hemminger

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

On Wed, 16 May 2007 20:35:05 -0700
Randy Dunlap <[email protected]> wrote:

>
> 5. This function:
>
> +static inline u8 iwl4965_get_dma_hi_address(dma_addr_t addr)
> +{
> +#ifdef _X86_64_TYPES_H
> + return addr >> 32;
> +#else
> +#ifdef _I386_TYPES_H
> + return 0;
> +#else
> +#error "unsupported architecture"
> +#endif
> +#endif
> +}
>
> a. For i386, dma_addr_t can still be 64 bits. See asm-i386/types.h:
>
> #ifdef CONFIG_HIGHMEM64G
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
>
> b. Should use CONFIG_64BIT instead of checking _X64_64_TYPES_H or
> _I386_TYPES_H.
>
> c. can just shift addr 32 bits right (in 2 shifts) in all cases:
>
> return (addr >> 16) >> 16;
>

I did this in sky2 driver:

/* Return high part of DMA address (could be 32 or 64 bit) */
static inline u32 high32(dma_addr_t a)
{
return sizeof(a) > sizeof(u32) ? (a >> 16) >> 16 : 0;
}

--
Stephen Hemminger <[email protected]>

2007-05-17 02:56:32

by Jeff Garzik

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers

2007-05-23 01:06:43

[permalink] [raw]

Subject: Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

Christoph Hellwig wrote:
> On Fri, May 18, 2007 at 02:31:54PM -0700, James Ketrenos wrote:
>> Alright; I'll add replacing the bitfields with bitwise access to the todo
>> list.
>>
>> Are you OK with us marking the driver as not working on big endian for now
>> and getting this one fixed over time?
>
> Given that Intel still hasn't fixed the older ipw2X00 drivers for big
> endian despite clong-time onstant prodding I'm dead set against this.

Really? Long time constant prodding?

I'll admit I'm not CC:'d on every email regarding the ipw2100 and ipw2200, so if I missed a long-time and constant thread, please forward it my way.

Searching Google for 'ipw endian' (and several other searches) found a reference to a fix for an endian issue when it was raised for the ipw2200. For the ipw2100 I see a post back in 2004 by one person asking if anyone had tried the ipw2100 on a big endian system.

> It just won't happen if you don't do it now.

Well, if you have evidence to this then great. If not, please don't spread FUD.

Historical footnote #1 -- the ipw2200 driver was added to the kernel w/out big endian support:
commit: 43f66a6ce8da299344cf1bc2ac2311889cc88555

Historical footnote #2 -- we fixed big endian problems with the driver in August 2005
commit: a613bffd3aac89bb0a8c9b7afa72af9b0ae30f0a (and a few other commits as users reported issues)

For the ipw2100, aside from one post in 2004 referencing it, I don't see anything.

James

2007-05-21 21:44:15

by Guy Cohen

[permalink] [raw]

Subject: RE: [PATCH] Add iwlwifi wireless drivers [part 2/2]

From: Randy Dunlap [mailto:[email protected]]
>Jeff Garzik wrote:
>> Cohen, Guy wrote:
>>> Support for big-endian platforms (as well as AP mode) for iwlwifi is
>>> currently *planned* to be added by Intel in the near future for
several
>>> purposes.
>>
>>> However, the vast majority of iwlwifi users at this point run it
over
>>> little-endian platforms. _Delaying_ the inclusion of iwlwifi driver
will
>>> not assist iwlwifi-over-big-endian-platform users in any way, but
the
>>> impact on the rest of the users is clear.
>>
>>
>> All Linux drivers that use the proper APIs are transparently portable
to
>> either little endian or big endian, 32-bit or 64-bit machines.
>>
>> If your driver is not portable, then it is not using the proper APIs,
>> and needs updating.
>>
>> Next time, write the driver using the correct APIs and you will not
run
>> into these sort of problems...
>
>Ack. Basically same idea for coding style issues also.
>Do it right the first time, don't have to redo it.
>

You two are right. Any other reply from my side will be dumb... We will
learn the lesson...

But at this point (we can't change the past nor the present), we would
like to focus on more urgent issues with iwlwifi in the immediate term
and delay a bit the big-endian support. Is this acceptable?

>--
>~Randy
>*** Remember to use Documentation/SubmitChecklist when testing your
code
>***