stlc45xx: mac80211 driver for N800 and N810

stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Hello all,

Nokia yesterday published stlc45xx, which is a mac80211 driver for
N800 and N810. Webpage here:

http://stlc45xx.garage.maemo.org/

The driver has been implemented with support from ST NXP Wireless, the
chip manufacturer for stlc4550 and stlc4560 found in N800 and N810,
respectively.

After the driver has received some serious cleanup I would like to try
to to get it into wireless-testing tree. But I'll send another email
later on.

-- 
Kalle Valo

Re: stlc45xx: mac80211 driver for N800 and N810

[Luis R. Rodriguez]

On Thu, Sep 18, 2008 at 9:43 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
> Hello all,
>
> Nokia yesterday published stlc45xx, which is a mac80211 driver for
> N800 and N810. Webpage here:
>
> http://stlc45xx.garage.maemo.org/
>
> The driver has been implemented with support from ST NXP Wireless, the
> chip manufacturer for stlc4550 and stlc4560 found in N800 and N810,
> respectively.
>
> After the driver has received some serious cleanup I would like to try
> to to get it into wireless-testing tree. But I'll send another email
> later on.

Cool! I thought n810 basically used a p54 type chipset over SPI?

  Luis

Re: stlc45xx: mac80211 driver for N800 and N810

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Thu, 2008-09-18 at 22:57 -0700, Luis R. Rodriguez wrote:

> > After the driver has received some serious cleanup I would like to try
> > to to get it into wireless-testing tree. But I'll send another email
> > later on.
> 
> Cool! I thought n810 basically used a p54 type chipset over SPI?

Yes, but the firmware (lower MAC) is sufficiently different that it
would be painful to support in p54, afaict.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: stlc45xx: mac80211 driver for N800 and N810

[Christian Lamparter]

On Friday 19 September 2008 10:59:38 Johannes Berg wrote:
> On Thu, 2008-09-18 at 22:57 -0700, Luis R. Rodriguez wrote:
> > > After the driver has received some serious cleanup I would like to try
> > > to to get it into wireless-testing tree. But I'll send another email
> > > later on.
> >
> > Cool! I thought n810 basically used a p54 type chipset over SPI?
>
> Yes, but the firmware (lower MAC) is sufficiently different that it
> would be painful to support in p54, afaict.
>
not that much... only the frequency calibration data is a bit different,
while most other data structures are more or less the same. Really!

Regards,
	Chr

Re: stlc45xx: mac80211 driver for N800 and N810

[Luis R. Rodriguez]

On Fri, Sep 19, 2008 at 7:58 AM, Christian Lamparter <chunkeey@web.de> wrote:
> On Friday 19 September 2008 10:59:38 Johannes Berg wrote:
>> On Thu, 2008-09-18 at 22:57 -0700, Luis R. Rodriguez wrote:
>> > > After the driver has received some serious cleanup I would like to try
>> > > to to get it into wireless-testing tree. But I'll send another email
>> > > later on.
>> >
>> > Cool! I thought n810 basically used a p54 type chipset over SPI?
>>
>> Yes, but the firmware (lower MAC) is sufficiently different that it
>> would be painful to support in p54, afaict.
>>
> not that much... only the frequency calibration data is a bit different,
> while most other data structures are more or less the same. Really!

Well to be fair we had similar question with ath5k and we went with
ath9k because that's how we best could support the hardware as quickly
as possible for users. Eventually we will converge though as it just
makes sense in areas that we can share. So if this driver is targeted
upstream to get users supported I can see how using a new driver
supported by a team can make sense. But eventually convergence should
be considered.

Is this driver targeted for inclusion upstream?

If the driver is targeted upstream and a company is sponsoring its
development we would like a license for the firmware. I mentioned this
on another thread but it must be one which meets distribution
requirements. You can see the WHENCE file on linux-firmware git tree
for examples.

git://git.kernel.org/pub/scm/linux/kernel/git/dwmw2/linux-firmware.git

Otherwise how can I use this driver, test it or enhance it?

  Luis

Re: stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Luis R. Rodriguez <mcgrof@gmail.com> writes:

>>> > Cool! I thought n810 basically used a p54 type chipset over SPI?
>>>
>>> Yes, but the firmware (lower MAC) is sufficiently different that it
>>> would be painful to support in p54, afaict.
>>>
>> not that much... only the frequency calibration data is a bit different,
>> while most other data structures are more or less the same. Really!
>
> Well to be fair we had similar question with ath5k and we went with
> ath9k because that's how we best could support the hardware as quickly
> as possible for users. Eventually we will converge though as it just
> makes sense in areas that we can share. So if this driver is targeted
> upstream to get users supported I can see how using a new driver
> supported by a team can make sense. But eventually convergence should
> be considered.

I don't have the p54 hardware (for regression testing) or the time to
add stlc45xx support to p54, and most probably there might be even
some legal implications which might prevent me of doing the work. So
somebody else would have to do the converge between p54 and stlc45xx.

> Is this driver targeted for inclusion upstream?

Yes, that's my goal but it's not quite ready yet. Now that the driver
is published I will be doing cleanup, starting from Johannes'
comments. I also understand that there might problems with the
upstream inclusion, biggest one being the proprietary user space
calibration tool.

I'm very interested in mac80211 and would like to see it suitable for
embedded devices like N800/N810. The just announced stlc45xx driver
would be a perfect tool for that kind of development and having the
driver upstream would make development a lot easier. 

Features that I am interested in, just from the top of my head, are
proper roaming support, power save mode and beacon filtering. In my
opinion the biggest areas needing work are everything related to
mobility or the power consumption.

> If the driver is targeted upstream and a company is sponsoring its
> development we would like a license for the firmware. I mentioned this
> on another thread but it must be one which meets distribution
> requirements. You can see the WHENCE file on linux-firmware git tree
> for examples.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dwmw2/linux-firmware.git
>
> Otherwise how can I use this driver, test it or enhance it?

Currently sltc45xx is usable only on N800/N810 and the rootfs in those
devices contains the firmware. But, like I already said in
stlc45xx-devel, I'm working on this. Just don't expect a quick
resolution, working with lawyers is slow.

-- 
Kalle Valo

Re: stlc45xx: mac80211 driver for N800 and N810

[Chr]

On Monday 22 September 2008 10:06:05 Kalle Valo wrote:
> Luis R. Rodriguez <mcgrof@gmail.com> writes:
> I don't have the p54 hardware (for regression testing) or the time to
> add stlc45xx support to p54, and most probably there might be even
> some legal implications which might prevent me of doing the work. So
> somebody else would have to do the converge between p54 and stlc45xx.
>
Hmm, don't worry... you won't break p54 hardware with these chances,
we did this before and I still could connect to my APs.
Even better, Johannes wrote most of the code for "p54spi.c" already.
I guess, we can reuse and fix his code, rather than reinventing the wheel once again.
I'll take a look what can be done when I'm back next week.

OT: Do you know if there's some way to get a card with a stlc45xx for or in a normal PC?

> > Is this driver targeted for inclusion upstream?
>
> I'm very interested in mac80211 and would like to see it suitable for
> embedded devices like N800/N810. The just announced stlc45xx driver
> would be a perfect tool for that kind of development and having the
> driver upstream would make development a lot easier.
>
> Features that I am interested in, just from the top of my head, are
> proper roaming support, power save mode and beacon filtering. In my
> opinion the biggest areas needing work are everything related to
> mobility or the power consumption.
Yes, this is definitely stuff that should go right into mac80211 stack and nowhere else.
Therefore FULL ACK.

Regards,
	Chr

Re: stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Chr <chunkeey@web.de> writes:

> On Monday 22 September 2008 10:06:05 Kalle Valo wrote:
>> Luis R. Rodriguez <mcgrof@gmail.com> writes:
>> I don't have the p54 hardware (for regression testing) or the time to
>> add stlc45xx support to p54, and most probably there might be even
>> some legal implications which might prevent me of doing the work. So
>> somebody else would have to do the converge between p54 and stlc45xx.
>
> Hmm, don't worry... you won't break p54 hardware with these chances,

Heh, you don't know how good I'm breaking things ;)

> we did this before and I still could connect to my APs. Even better,
> Johannes wrote most of the code for "p54spi.c" already. I guess, we
> can reuse and fix his code, rather than reinventing the wheel once
> again. I'll take a look what can be done when I'm back next week.

Thanks.

> OT: Do you know if there's some way to get a card with a stlc45xx
> for or in a normal PC?

No idea. If you find a way, please tell me. stlc45xx supports both SPI
and SDIO buses, but N800/N810 use only SPI. So you would need some
kind of SPI bridge and solder the chip somehow with all the antennas
etc. Not an easy task.

-- 
Kalle Valo

Re: stlc45xx: mac80211 driver for N800 and N810

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

On Fri, 2008-09-19 at 07:43 +0300, Kalle Valo wrote:
> Hello all,
> 
> Nokia yesterday published stlc45xx, which is a mac80211 driver for
> N800 and N810. Webpage here:
> 
> http://stlc45xx.garage.maemo.org/

Cool, thanks for posting.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: stlc45xx: mac80211 driver for N800 and N810

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 9972 bytes --]

On Fri, 2008-09-19 at 07:43 +0300, Kalle Valo wrote:
> Hello all,
> 
> Nokia yesterday published stlc45xx, which is a mac80211 driver for
> N800 and N810. Webpage here:
> 
> http://stlc45xx.garage.maemo.org/

quick look at the code

> static ssize_t stlc45xx_sysfs_show_cal_rssi(struct device *dev,
> 					    struct device_attribute *attr,
> 					    char *buf)
> {
> 	struct stlc45xx *stlc = dev_get_drvdata(dev);
> 	ssize_t len;
> 	
> 	stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
> 
> 	/* FIXME: what's the maximum length of buf? page size?*/
> 	len = 500;
> 
> 	if (mutex_lock_interruptible(&stlc->mutex) < 0) {
> 		len = 0;
> 		goto out;
> 	}

The interruptible seems fairly useless, I don't see the mutex being held
for very long periods of time anywhere?

> 		stlc45xx_error("invalid cal_rssi lenght: %d", count);

typo. I love reviewing in a program with spell checking ;)
	
> 	stlc->cal_rssi_ready = 1;

that variable is unneeded

> 	if (count != CHANNEL_CAL_ARRAY_LEN) {

I'd suspect that is not a constant, in the US you have 11 channels? Or
are they in there but disabled?
	
> 	stlc->cal_channels_ready = 1;

another pointless variable

> 	ssize_t len;
> 	

trailing whitespace in a number of places, run sed 's/\s*$//' or
something like that :)

> 	/* FIXME: what's the maximum length of buf? page size? */
> 	len = 500;

Oh, yes, as far as I know.

> 	len = snprintf(buf, len, "%i\n", stlc->psm);

but really, there's little use for snprintf for something that can at
most get like 13 characters.

> static ssize_t stlc45xx_sysfs_store_psm(struct device *dev,
> 					struct device_attribute *attr, 
> 					const char *buf, size_t count)
> {
> 	struct stlc45xx *stlc = dev_get_drvdata(dev);
> 	int val, ret;
> 
> 	ret = sscanf(buf, "%d", &val);

I think you may want strict_strtoul.

> static u16 stlc45xx_read16(struct stlc45xx *stlc, unsigned long addr)

> static u32 stlc45xx_read32(struct stlc45xx *stlc, unsigned long addr)

> static void stlc45xx_write16(struct stlc45xx *stlc, unsigned long addr, u16 val)

> static void stlc45xx_write32(struct stlc45xx *stlc, unsigned long addr, u32 val)

I'd almost think those should be declared inline, although the compiler
might?

> static void stlc45xx_dump_registers(struct stlc45xx *stlc)

Do we need this? It looks unused.

> 	list_for_each_entry(txbuffer, &stlc->txbuffer, buffer_list) {
> 		if (pos + len < txbuffer->start) {
> 			found = 1;
> 			break;
> 		}
>                 pos = ALIGN(txbuffer->end + 1, 4);
> 	}
> 
> 	if (!found && (pos + len > FIRMWARE_TXBUFFER_END))
> 		/* not enough room */
> 		pos = -1;

Afaict the found variable can be removed since the txbuffer->start will
be < FIRMWARE_TXBUFFER_END of course.

This code is actually rather subtle, comments would be good since the
list must always contain the entries in the order that they're in in the
firmware buffer too.

> static int stlc45xx_txbuffer_add(struct stlc45xx *stlc,
> 				 struct txbuffer *txbuffer) 
> {
> 	struct txbuffer *r, *prev = NULL;
> 	int ret = -1;
> 	
> 	stlc45xx_debug(DEBUG_FUNC, "%s()", __func__);
> 
> 	if (list_empty(&stlc->txbuffer)) {
> 		list_add(&txbuffer->buffer_list, &stlc->txbuffer);
> 		ret = 0;
> 		goto out;

you can just return since there is no cleanup :)

> 	r = list_first_entry(&stlc->txbuffer, struct txbuffer, buffer_list);
> 	
> 	if (txbuffer->start < r->start) {
> 		list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
> 		ret = 0;
> 		goto out;
> 	}

list_add_tail? This seems like it should be list_add() since it's before
the first item.

> 	prev = NULL;
> 	list_for_each_entry(r, &stlc->txbuffer, buffer_list) {
> 		WARN_ON_ONCE(txbuffer->start >= r->start
> 			     && txbuffer->start <= r->end);
> 		WARN_ON_ONCE(txbuffer->end >= r->start
> 			     && txbuffer->end <= r->end);
> 		if (prev && prev->end < txbuffer->start &&
> 		    txbuffer->start < r->start) {                <---****** txbuffer->end??
> 			list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
> 			ret = 0;
> 			goto out;
> 		}
> 		prev = r;
> 	}

This looks complicated and buggy, how about this instead:

	prev = NULL;
	list_for_each_entry(r, &stlc->txbuffer, buffer_list) {
		/* skip first entry, we checked for that above */
		if (!prev)
			continue;

		/* double-check overlaps */
		WARN_ON_ONCE(txbuffer->start >= r->start &&
			     txbuffer->start <= r->end);
		WARN_ON_ONCE(txbuffer->end >= r->start &&
			     txbuffer->end <= r->end);

		if (prev->end < txbuffer->start &&
		    txbuffer->end < r->start) {
			/* insert at this spot */
			list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
			return 0;
		}
		prev = r;
	}

	/* not found */
> 	list_add_tail(&txbuffer->buffer_list, &stlc->txbuffer);
> 	ret = 0;
> 
> out:
> 	return ret;


> /* caller must hold tx_lock */
> static struct txbuffer *stlc45xx_txbuffer_alloc(struct stlc45xx *stlc,
> 						 size_t frame_len)
> {
> 	struct txbuffer *entry = NULL;
> 	size_t len;
> 	int pos;
> 
> 	stlc45xx_debug(DEBUG_FUNC, "%s()", __func__);
> 	
> 	len = FIRMWARE_TXBUFFER_HEADER + frame_len + FIRMWARE_TXBUFFER_TRAILER;
>         pos = stlc45xx_txbuffer_find(stlc, len);
> 
>         if (pos < 0) {
> 		return NULL;
>         }

useless braces

> 	WARN_ON_ONCE(pos + len > FIRMWARE_TXBUFFER_END);
> 	WARN_ON_ONCE(pos < FIRMWARE_TXBUFFER_START);
> 
> 	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
> 	entry->start = pos;
> 	entry->frame_start = pos + FIRMWARE_TXBUFFER_HEADER;
> 	entry->end = entry->start + len;

I think this can be
	entry->end = entry->start + len - 1
since you treat it as such in all the other code afaict. Might pack the
buffers a bit better and require less padding.

> /* caller must hold tx_lock */
> static void stlc45xx_check_txsent(struct stlc45xx *stlc) 
> {
> 	struct txbuffer *entry, *n;
> 
> 	/* FIXME: notify mac80211? */

Yeah, you'd probably want to.

> static void stlc45xx_power_on(struct stlc45xx *stlc)
> {
> 	omap_set_gpio_dataout(stlc->config->power_gpio, 1);
> 	enable_irq(OMAP_GPIO_IRQ(stlc->config->irq_gpio));
> 
> 	/*
> 	 * need to wait a while before device can be accessed, the lenght

another typo

> /* caller must hold tx_lock */
> static void stlc45xx_flush_queues(struct stlc45xx *stlc) 
> {
> 	struct txbuffer *entry;
> 
> 	/* FIXME: notify mac80211? */

given that reset shouldn't happen and on stop mac80211 doesn't care, I
wouldn't bother.

> static void stlc45xx_work_reset(struct work_struct *work)
> {
> 	struct stlc45xx *stlc = container_of(work, struct stlc45xx,
> 					     work_reset);

If reset _does_ happen you could use the mac80211 notification callback
to tell it to associate again.

> static int stlc45xx_rx_txack(struct stlc45xx *stlc, struct sk_buff *skb)
> {


> 	stlc45xx_check_txsent(stlc);
> 	if (list_empty(&stlc->tx_sent))
> 		/* there are pending frames, we can stop the tx timeout
> 		 * timer */
> 		cancel_delayed_work(&stlc->work_tx_timeout);

there are _no_ pending frames [...]

> 	memset(&status, 0, sizeof(status));
> 
> 	status.freq = data->frequency;
> 	status.signal = data->rcpi / 2 - 110;
> 
> 	/* let's assume that maximum rcpi value is 140 (= 35 dBm) */
> 	status.qual = data->rcpi * 100 / 140;
> 
> 	status.band = IEEE80211_BAND_2GHZ;
> 
> 	/*
> 	 * FIXME: this gives warning from __ieee80211_rx()
> 	 *
> 	 * status.rate_idx = data->rate;
> 	 */

That's strange, you should print out the index and see why it warns, it
shouldn't afaict. Unless the docs are wrong...

> 	__ieee80211_rx(stlc->hw, skb, &status);

Use ieee80211_rx() without the underscores, the __ is just a hack to
make the symbol clash go away... Jiri never should have let it escape
the header file.

> 	setup->antenna = 2;
> 	setup->rx_align = 0;
> 	setup->rx_buffer = FIRMWARE_RXBUFFER_START;
> 	setup->rx_mtu = FIRMWARE_MTU;
> 	setup->frontend = 5;

There are #defines in the header file for some of these

> static int stlc45xx_op_add_interface(struct ieee80211_hw *hw,
> 				     struct ieee80211_if_init_conf *conf)
> {
> 	struct stlc45xx *stlc = hw->priv;
> 
> 	stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
> 	
> 	switch (conf->type) {
> 	case IEEE80211_IF_TYPE_STA:
> 		break;
> 	default:
> 		return -EOPNOTSUPP;
> 	}

You need to keep track whether you're already up or not, right now
you're allowing multiple STA interfaces.
	
> static void stlc45xx_op_remove_interface(struct ieee80211_hw *hw,
> 					 struct ieee80211_if_init_conf *conf)
> {
> 	stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
> }

That's why you need _remove_interface in any case and it isn't
optional :) Also you really should clear the MAC address or go into the
no-ack mode so pure monitoring works.

> static void stlc45xx_op_configure_filter(struct ieee80211_hw *hw,
> 				      unsigned int changed_flags,
> 				      unsigned int *total_flags,
> 				      int mc_count,
> 				      struct dev_addr_list *mc_list) 
> {
> 	*total_flags = 0;
> }

If I understand the documentation correctly you can actually support a
few things here.

> /* can't be const, mac80211 writes to this */
> static struct ieee80211_supported_band stlc45xx_band_2ghz = {

I don't think that's true for this one? Not that it matters. const is a
pure compiler thing anyway.

> static int stlc45xx_register_mac80211(struct stlc45xx *stlc)
> {
> 	/* FIXME: SET_IEEE80211_PERM_ADDR() requires default_mac_addr
> 	   to be non-const for some strange reason */

Bug I guess :)

> 	stlc = hw->priv;
> 	memset(stlc, 0, sizeof(*stlc));

should already be cleared, but you can do it again of course :)


Overall looks pretty good, I'm concerned that the binary helper is
_required_, we had this with ipw3945 and it wasn't accepted. Any way to
make it not required?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2008-09-19 at 07:43 +0300, Kalle Valo wrote:
>> Hello all,
>> 
>> Nokia yesterday published stlc45xx, which is a mac80211 driver for
>> N800 and N810. Webpage here:
>> 
>> http://stlc45xx.garage.maemo.org/
>
> quick look at the code

Thanks a lot! I didn't send any code for review yet because, as you
noticed, the code is in early stages still. I will go through your
comments next week and fix them.

> Overall looks pretty good, I'm concerned that the binary helper is
> _required_, we had this with ipw3945 and it wasn't accepted. Any way to
> make it not required?

I guess I'll have to find a way. Maybe have very convervative default
values in the driver which the binary tool can update? That way the
driver would work without stlc45xx-cal (with the cost of worse
performance) and still our requirements would be fulfilled.

-- 
Kalle Valo

Re: stlc45xx: mac80211 driver for N800 and N810

[Luis R. Rodriguez]

Hey just saw this device supports reporting signal strength in RCPI,
we'll need to add that flag now, or I guess we can keep the dbm as the
software can do the direct mapping. Anyway, cool stuff.

  Luis

Re: stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Luis R. Rodriguez <mcgrof@gmail.com> writes:

> Hey just saw this device supports reporting signal strength in RCPI,
> we'll need to add that flag now,

Nah, I don't think it's worth the trouble.

> or I guess we can keep the dbm as the software can do the direct
> mapping.

I think so too.

> Anyway, cool stuff.

Thanks.

-- 
Kalle Valo

Re: stlc45xx: mac80211 driver for N800 and N810

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Sat, 2008-09-20 at 11:16 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Fri, 2008-09-19 at 07:43 +0300, Kalle Valo wrote:
> >> Hello all,
> >> 
> >> Nokia yesterday published stlc45xx, which is a mac80211 driver for
> >> N800 and N810. Webpage here:
> >> 
> >> http://stlc45xx.garage.maemo.org/
> >
> > quick look at the code
> 
> Thanks a lot! I didn't send any code for review yet because, as you
> noticed, the code is in early stages still.

Right, just figured I was looking anyway...

Anyway, something else occurred to me, you should be handling the
sequence number TX flag from mac80211 because otherwise the firmware
will probably overwrite the per-TID sequence numbers. According to the
API documentation, there is a "disable firmware sequence number" flag
which then perfectly corresponds to !IEEE80211_TXF_ASSIGN_SEQ.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: stlc45xx: mac80211 driver for N800 and N810

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2008-09-19 at 07:43 +0300, Kalle Valo wrote:
>> Hello all,
>> 
>> Nokia yesterday published stlc45xx, which is a mac80211 driver for
>> N800 and N810. Webpage here:
>> 
>> http://stlc45xx.garage.maemo.org/
>
> quick look at the code

And a VERY late reply, sorry for taking so long.

>> 	if (mutex_lock_interruptible(&stlc->mutex) < 0) {
>> 		len = 0;
>> 		goto out;
>> 	}
>
> The interruptible seems fairly useless, I don't see the mutex being held
> for very long periods of time anywhere?

I added basically for bailing out deadlocks, so that I wouldn't have
to reboot the device during deadlock. But I removed the interruptible
versions now.

>
>> 		stlc45xx_error("invalid cal_rssi lenght: %d", count);
>
> typo. I love reviewing in a program with spell checking ;)

Fixed.

>> 	stlc->cal_rssi_ready = 1;
>
> that variable is unneeded

Actually it's still used:

	if (!stlc->cal_rssi_ready) {
		stlc45xx_error("rssi calibration data missing");
		ret = -ENOENT;
		goto out_unlock;
	}

But I'll remove it in upcoming patches.

>> 	if (count != CHANNEL_CAL_ARRAY_LEN) {
>
> I'd suspect that is not a constant, in the US you have 11 channels? Or
> are they in there but disabled?

It's a constant.

>> 	stlc->cal_channels_ready = 1;

Same comment as with cal_rssi_ready

> another pointless variable
>
>> 	ssize_t len;
>
> trailing whitespace in a number of places, run sed 's/\s*$//' or
> something like that :)

I now run checkpatch and all whitespace problems should be fixed.

>> 	/* FIXME: what's the maximum length of buf? page size? */
>> 	len = 500;
>
> Oh, yes, as far as I know.

Ok, I used PAGE_SIZE.

>> 	len = snprintf(buf, len, "%i\n", stlc->psm);
>
> but really, there's little use for snprintf for something that can at
> most get like 13 characters.

Think it more like academic interest :)

>> static ssize_t stlc45xx_sysfs_store_psm(struct device *dev,
>> 					struct device_attribute *attr, 
>> 					const char *buf, size_t count)
>> {
>> 	struct stlc45xx *stlc = dev_get_drvdata(dev);
>> 	int val, ret;
>> 
>> 	ret = sscanf(buf, "%d", &val);
>
> I think you may want strict_strtoul.

I removed entire function and used CONF_PS instead.

>> static u16 stlc45xx_read16(struct stlc45xx *stlc, unsigned long addr)
>
>> static u32 stlc45xx_read32(struct stlc45xx *stlc, unsigned long addr)
>
>> static void stlc45xx_write16(struct stlc45xx *stlc, unsigned long addr, u16 val)
>
>> static void stlc45xx_write32(struct stlc45xx *stlc, unsigned long addr, u32 val)
>
> I'd almost think those should be declared inline, although the compiler
> might?

Compiler should do it automatically.

>> static void stlc45xx_dump_registers(struct stlc45xx *stlc)
>
> Do we need this? It looks unused.

Removed.

>> 	list_for_each_entry(txbuffer, &stlc->txbuffer, buffer_list) {
>> 		if (pos + len < txbuffer->start) {
>> 			found = 1;
>> 			break;
>> 		}
>>                 pos = ALIGN(txbuffer->end + 1, 4);
>> 	}
>> 
>> 	if (!found && (pos + len > FIRMWARE_TXBUFFER_END))
>> 		/* not enough room */
>> 		pos = -1;
>
> Afaict the found variable can be removed since the txbuffer->start will
> be < FIRMWARE_TXBUFFER_END of course.

You are right (as usual), removed the variable.

> This code is actually rather subtle, comments would be good since the
> list must always contain the entries in the order that they're in in the
> firmware buffer too.

I added some comments, but I'll try to add more later.

>> static int stlc45xx_txbuffer_add(struct stlc45xx *stlc,
>> 				 struct txbuffer *txbuffer) 
>> {
>> 	struct txbuffer *r, *prev = NULL;
>> 	int ret = -1;
>> 	
>> 	stlc45xx_debug(DEBUG_FUNC, "%s()", __func__);
>> 
>> 	if (list_empty(&stlc->txbuffer)) {
>> 		list_add(&txbuffer->buffer_list, &stlc->txbuffer);
>> 		ret = 0;
>> 		goto out;
>
> you can just return since there is no cleanup :)

Changed.

>> 	r = list_first_entry(&stlc->txbuffer, struct txbuffer, buffer_list);
>> 	
>> 	if (txbuffer->start < r->start) {
>> 		list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
>> 		ret = 0;
>> 		goto out;
>> 	}
>
> list_add_tail? This seems like it should be list_add() since it's before
> the first item.

Definitely, fixed.

>> 	prev = NULL;
>> 	list_for_each_entry(r, &stlc->txbuffer, buffer_list) {
>> 		WARN_ON_ONCE(txbuffer->start >= r->start
>> 			     && txbuffer->start <= r->end);
>> 		WARN_ON_ONCE(txbuffer->end >= r->start
>> 			     && txbuffer->end <= r->end);
>> 		if (prev && prev->end < txbuffer->start &&
>> 		    txbuffer->start < r->start) {                <---****** txbuffer->end??
>> 			list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
>> 			ret = 0;
>> 			goto out;
>> 		}
>> 		prev = r;
>> 	}
>
> This looks complicated and buggy, how about this instead:

Yeah, I'm not very proud of that buffer allocation code.

> 	prev = NULL;
> 	list_for_each_entry(r, &stlc->txbuffer, buffer_list) {
> 		/* skip first entry, we checked for that above */
> 		if (!prev)
> 			continue;

Expect that we have to update prev, I did it like this:

		/* skip first entry, we checked for that above */
		if (!prev) {
			prev = r;
			continue;
		}

> 		/* double-check overlaps */
> 		WARN_ON_ONCE(txbuffer->start >= r->start &&
> 			     txbuffer->start <= r->end);
> 		WARN_ON_ONCE(txbuffer->end >= r->start &&
> 			     txbuffer->end <= r->end);
>
> 		if (prev->end < txbuffer->start &&
> 		    txbuffer->end < r->start) {
> 			/* insert at this spot */
> 			list_add_tail(&txbuffer->buffer_list, &r->buffer_list);
> 			return 0;
> 		}
> 		prev = r;
> 	}
>

[...]

>>         if (pos < 0) {
>> 		return NULL;
>>         }
>
> useless braces

Fixed.

>> 	WARN_ON_ONCE(pos + len > FIRMWARE_TXBUFFER_END);
>> 	WARN_ON_ONCE(pos < FIRMWARE_TXBUFFER_START);
>> 
>> 	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
>> 	entry->start = pos;
>> 	entry->frame_start = pos + FIRMWARE_TXBUFFER_HEADER;
>> 	entry->end = entry->start + len;
>
> I think this can be
> 	entry->end = entry->start + len - 1
> since you treat it as such in all the other code afaict. Might pack the
> buffers a bit better and require less padding.

I think you are right, though didn't check that throughly. Seems to
work with iperf still, I'm satisfied.

>> /* caller must hold tx_lock */
>> static void stlc45xx_check_txsent(struct stlc45xx *stlc) 
>> {
>> 	struct txbuffer *entry, *n;
>> 
>> 	/* FIXME: notify mac80211? */
>
> Yeah, you'd probably want to.

Fixed.

>> static void stlc45xx_power_on(struct stlc45xx *stlc)
>> {
>> 	omap_set_gpio_dataout(stlc->config->power_gpio, 1);
>> 	enable_irq(OMAP_GPIO_IRQ(stlc->config->irq_gpio));
>> 
>> 	/*
>> 	 * need to wait a while before device can be accessed, the lenght
>
> another typo

Fixed.

>> /* caller must hold tx_lock */
>> static void stlc45xx_flush_queues(struct stlc45xx *stlc) 
>> {
>> 	struct txbuffer *entry;
>> 
>> 	/* FIXME: notify mac80211? */
>
> given that reset shouldn't happen and on stop mac80211 doesn't care, I
> wouldn't bother.

Ok, fixme removed.

>> static void stlc45xx_work_reset(struct work_struct *work)
>> {
>> 	struct stlc45xx *stlc = container_of(work, struct stlc45xx,
>> 					     work_reset);
>
> If reset _does_ happen you could use the mac80211 notification callback
> to tell it to associate again.

Actually that's not needed, in practise the firmware reset happens so
fast that the AP doesn't notice anything. We just need to give exactly
same settings back to the firmware after reset, and then we can
continue as before the reset.

>> static int stlc45xx_rx_txack(struct stlc45xx *stlc, struct sk_buff *skb)
>> {
>
>
>> 	stlc45xx_check_txsent(stlc);
>> 	if (list_empty(&stlc->tx_sent))
>> 		/* there are pending frames, we can stop the tx timeout
>> 		 * timer */
>> 		cancel_delayed_work(&stlc->work_tx_timeout);
>
> there are _no_ pending frames [...]

Oops, fixed.

>> 	memset(&status, 0, sizeof(status));
>> 
>> 	status.freq = data->frequency;
>> 	status.signal = data->rcpi / 2 - 110;
>> 
>> 	/* let's assume that maximum rcpi value is 140 (= 35 dBm) */
>> 	status.qual = data->rcpi * 100 / 140;
>> 
>> 	status.band = IEEE80211_BAND_2GHZ;
>> 
>> 	/*
>> 	 * FIXME: this gives warning from __ieee80211_rx()
>> 	 *
>> 	 * status.rate_idx = data->rate;
>> 	 */
>
> That's strange, you should print out the index and see why it warns, it
> shouldn't afaict. Unless the docs are wrong...

I'll check it later.

>> 	__ieee80211_rx(stlc->hw, skb, &status);
>
> Use ieee80211_rx() without the underscores, the __ is just a hack to
> make the symbol clash go away... Jiri never should have let it escape
> the header file.

Fixed.

>> 	setup->antenna = 2;
>> 	setup->rx_align = 0;
>> 	setup->rx_buffer = FIRMWARE_RXBUFFER_START;
>> 	setup->rx_mtu = FIRMWARE_MTU;
>> 	setup->frontend = 5;
>
> There are #defines in the header file for some of these

Added an entry to TODO file.

>> static int stlc45xx_op_add_interface(struct ieee80211_hw *hw,
>> 				     struct ieee80211_if_init_conf *conf)
>> {
>> 	struct stlc45xx *stlc = hw->priv;
>> 
>> 	stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
>> 	
>> 	switch (conf->type) {
>> 	case IEEE80211_IF_TYPE_STA:
>> 		break;
>> 	default:
>> 		return -EOPNOTSUPP;
>> 	}
>
> You need to keep track whether you're already up or not, right now
> you're allowing multiple STA interfaces.

Again added added to TODO file, will fix it later.

>> static void stlc45xx_op_remove_interface(struct ieee80211_hw *hw,
>> 					 struct ieee80211_if_init_conf *conf)
>> {
>> 	stlc45xx_debug(DEBUG_FUNC, "%s", __func__);
>> }
>
> That's why you need _remove_interface in any case and it isn't
> optional :) Also you really should clear the MAC address or go into the
> no-ack mode so pure monitoring works.

Ack.

>> static void stlc45xx_op_configure_filter(struct ieee80211_hw *hw,
>> 				      unsigned int changed_flags,
>> 				      unsigned int *total_flags,
>> 				      int mc_count,
>> 				      struct dev_addr_list *mc_list) 
>> {
>> 	*total_flags = 0;
>> }
>
> If I understand the documentation correctly you can actually support a
> few things here.

Sure does, never just found the time to implement them.

>> /* can't be const, mac80211 writes to this */
>> static struct ieee80211_supported_band stlc45xx_band_2ghz = {
>
> I don't think that's true for this one? Not that it matters. const is a
> pure compiler thing anyway.

Haven't checked for a long time.

>> static int stlc45xx_register_mac80211(struct stlc45xx *stlc)
>> {
>> 	/* FIXME: SET_IEEE80211_PERM_ADDR() requires default_mac_addr
>> 	   to be non-const for some strange reason */
>
> Bug I guess :)

Ok, I'll try to remember to send a patch for this.

>> 	stlc = hw->priv;
>> 	memset(stlc, 0, sizeof(*stlc));
>
> should already be cleared, but you can do it again of course :)

Removed memset().

Thanks a lot for the comments, they were very valuable. Your comments
are also a very good example to show people in our company why open
source really works.

I pushed all the changes to the stlc45xx git repository now:

http://gitorious.org/projects/stlc45xx

gitorious.org doesn't seem to understand about branches and the commit
log looks confusing. Do a git checkout if you want to look at the
changes in more detail.

-- 
Kalle Valo