Skip to contentSkip to conversation

stlc45xx: mac80211 driver for N800 and N810

DISCUSSION Kalle Valo 4 participants 15 messages Sep 19, 2008 – Dec 5, 2008

lore
Share & tools
lore

Kalle Valo

author
#0 lore raw

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.

Signature · 1 line
Kalle Valo
#1 lore raw
Quoted 7 lines · Kalle Valo

Hello all,

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

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

Quoted 7 lines

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

#2 lore raw
Quoted 5 lines · Luis R. Rodriguez
Quoted 3 lines

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

#3 lore raw
Quoted 10 lines · On Friday 19 September 2008 10:59:38 Johannes Berg wrote:
Quoted 5 lines · Luis R. Rodriguez
Quoted 3 lines

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

#4 lore raw
Quoted 6 lines · Christian Lamparter
Quoted 5 lines · On Friday 19 September 2008 10:59:38 Johannes Berg wrote:
Quoted 4 lines · Luis R. Rodriguez
Quoted 3 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

author
#5 lore raw
Quoted 8 lines · Luis R. Rodriguez
Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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!

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 9 lines

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.

Signature · 1 line
Kalle Valo
#6 lore raw
Quoted 6 lines · On Monday 22 September 2008 10:06:05 Kalle Valo wrote:

Luis R. Rodriguez <[email protected]> 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?

Quoted 11 lines
Quoted 1 line

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

author
#7 lore raw
Quoted 8 lines · Chr
Quoted 5 lines · On Monday 22 September 2008 10:06:05 Kalle Valo wrote:

Luis R. Rodriguez <[email protected]> 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 ;)

Quoted 4 lines

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.

Quoted 2 lines

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.

Signature · 1 line
Kalle Valo
#9 lore raw
In reply to Kalle Valo
Quoted 6 lines · Kalle Valo

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

Quoted 16 lines

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?

Quoted 1 line
		stlc45xx_error("invalid cal_rssi lenght: %d", count);

typo. I love reviewing in a program with spell checking ;)

Quoted 1 line
	stlc->cal_rssi_ready = 1;

that variable is unneeded

Quoted 1 line
	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?

Quoted 1 line
	stlc->cal_channels_ready = 1;

another pointless variable

Quoted 2 lines
	ssize_t len;

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

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

Oh, yes, as far as I know.

Quoted 1 line
	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.

Quoted 8 lines

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.

Quoted 7 lines

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?

Quoted 1 line

static void stlc45xx_dump_registers(struct stlc45xx *stlc)

Do we need this? It looks unused.

Quoted 11 lines
	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.

Quoted 12 lines

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 :)

Quoted 7 lines
	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.

Quoted 6 lines
	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);
Quoted 8 lines
		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 */
Quoted 10 lines
	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)

Quoted 13 lines
{
	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

Quoted 7 lines
	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.

Quoted 6 lines

/* 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.

Quoted 7 lines

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

Quoted 6 lines

/* 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.

Quoted 4 lines

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.

Quoted 9 lines

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 [...]

Quoted 8 lines
	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;
Quoted 7 lines
	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...

Quoted 1 line
	__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.

Quoted 5 lines
	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

Quoted 13 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

/* 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.

Quoted 4 lines

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 :)

Quoted 2 lines
	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

author
#10 lore raw
Quoted 9 lines · Johannes Berg
Quoted 6 lines · Kalle Valo

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.

Quoted 3 lines

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.

Signature · 1 line
Kalle Valo
#11 lore raw

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

author
#12 lore raw
Quoted 2 lines · Luis R. Rodriguez

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.

Quoted 2 lines

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

I think so too.

Quoted 1 line

Anyway, cool stuff.

Thanks.

Signature · 1 line
Kalle Valo
#13 lore raw
In reply to Kalle Valo
Quoted 8 lines · Kalle Valo
Quoted 6 lines · Johannes Berg
Quoted 5 lines · Kalle Valo

Hello all,

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

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

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

author
#14 lore raw
In reply to Johannes Berg
Quoted 9 lines · Johannes Berg
Quoted 6 lines · Kalle Valo

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.

Quoted 7 lines
Quoted 4 lines
	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.

Quoted 4 lines
Quoted 2 lines
		stlc45xx_error("invalid cal_rssi lenght: %d", count);

typo. I love reviewing in a program with spell checking ;)

Fixed.

Quoted 3 lines
Quoted 1 line
	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.

Quoted 4 lines
Quoted 1 line
	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.

Quoted 1 line
Quoted 1 line
	stlc->cal_channels_ready = 1;

Same comment as with cal_rssi_ready

Quoted 6 lines

another pointless variable

Quoted 1 line
	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.

Quoted 4 lines
Quoted 2 lines
	/* FIXME: what's the maximum length of buf? page size? */
	len = 500;

Oh, yes, as far as I know.

Ok, I used PAGE_SIZE.

Quoted 4 lines
Quoted 1 line
	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 :)

Quoted 10 lines
Quoted 8 lines

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.

Quoted 10 lines
Quoted 7 lines

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.

Quoted 3 lines
Quoted 1 line

static void stlc45xx_dump_registers(struct stlc45xx *stlc)

Do we need this? It looks unused.

Removed.

Quoted 8 lines
Quoted 8 lines
	list_for_each_entry(txbuffer, &stlc->txbuffer, buffer_list) {
		if (pos + len < txbuffer->start) {
			found = 1;
			break;
		}
                pos = ALIGN(txbuffer->end + 1, 4);
	}
Quoted 6 lines
Quoted 3 lines
	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.

Quoted 3 lines

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.

Quoted 6 lines
Quoted 6 lines

static int stlc45xx_txbuffer_add(struct stlc45xx *stlc,
struct txbuffer *txbuffer)

{
	struct txbuffer *r, *prev = NULL;
	int ret = -1;
Quoted 8 lines
Quoted 6 lines
	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.

Quoted 10 lines
Quoted 7 lines
	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.

Quoted 15 lines
Quoted 6 lines
	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);
Quoted 8 lines
		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;
	}
Quoted 1 line

This looks complicated and buggy, how about this instead:

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

Quoted 5 lines
	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;
		}
Quoted 15 lines
		/* 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;
	}

[...]

Quoted 5 lines
Quoted 3 lines
        if (pos < 0) {
		return NULL;
        }

useless braces

Fixed.

Quoted 8 lines
Quoted 7 lines
	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;
Quoted 4 lines

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.

Quoted 8 lines
Quoted 6 lines

/* 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.

Quoted 9 lines
Quoted 7 lines

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.

Quoted 9 lines
Quoted 6 lines

/* 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.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 11 lines
Quoted 9 lines

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.

Quoted 10 lines
Quoted 10 lines
	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;
Quoted 8 lines
Quoted 5 lines
	/*
	 * 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.

Quoted 5 lines
Quoted 1 line
	__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.

Quoted 7 lines
Quoted 5 lines
	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.

Quoted 14 lines
Quoted 13 lines

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;
	}
Quoted 2 lines

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.

Quoted 9 lines
Quoted 5 lines

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.

Quoted 11 lines
Quoted 8 lines

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.

Quoted 5 lines
Quoted 2 lines

/* 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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 4 lines
Quoted 2 lines
	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.

Signature · 1 line
Kalle Valo