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:
signature.asc
Description: This is a digitally signed message part
