| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200709152017.02646.flamingice@sourmilk.net>
Date: Sat, 15 Sep 2007 20:16:58 -0400
From: Michael Wu <flamingice@...rmilk.net>
To: Jeff Garzik <jeff@...zik.org>
Cc: "John W. Linville" <linville@...driver.com>,
netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
David Miller <davem@...emloft.net>
Subject: Re: Please pull 'adm8211' branch of wireless-2.6
On Saturday 15 September 2007 17:32, Jeff Garzik wrote:
> Review summary: many minor issues, only one major one: irq handler loop
>
CCing me would help.
> John W. Linville wrote:
> > +static unsigned int tx_ring_size __read_mostly = 16;
> > +static unsigned int rx_ring_size __read_mostly = 16;
> > +
> > +module_param(tx_ring_size, uint, 0);
> > +module_param(rx_ring_size, uint, 0);
>
> should be in ethtool (or another per-interface runtime config tool), not
> a module parameter.
>
ethtool is not accessible to mac80211 drivers. I'll just make it a constant
because tuning the ring size doesn't help much.
> > + } else if ((flags & IFF_ALLMULTI) || (mc_count > -1)) {
>
> mc_count > -1 ?
>
> what kind of bogus placeholder is that?
>
Hm, right, I disabled the multicast filter config for some reason by doing
s/32/-1/ and I forgot to reenable it. Will fix.
> > + if (flags & IFF_PROMISC)
> > + dev->flags |= IEEE80211_HW_RX_INCLUDES_FCS;
> > + else
> > + dev->flags &= ~IEEE80211_HW_RX_INCLUDES_FCS;
>
> why does promisc dictate inclusion of FCS?
>
Because that's the way the hardware works.
> not this driver's fault, but, ieee80211_wake_queue() warrants revisiting
> now that MQ stuff is in
>
> when queue==0 is hardcoded, you can just use netif_wake/stop_queue() AFAICS
>
mac80211 have no access to any struct net_device.
> the '%' operator is expensive. mask combined with power-of-2 is better
>
Sure.
> > +static irqreturn_t adm8211_interrupt(int irq, void *dev_id)
> > +{
> > +#define ADM8211_INT(x) \
> > +do { \
> > + if (unlikely(stsr & ADM8211_STSR_ ## x)) \
> > + printk(KERN_DEBUG "%s: " #x "\n", wiphy_name(dev->wiphy)); \
> > +} while (0)
> > +
> > + struct ieee80211_hw *dev = dev_id;
> > + struct adm8211_priv *priv = dev->priv;
> > + unsigned int count = 0;
> > + u32 stsr;
> > +
> > + do {
> > + stsr = ADM8211_CSR_READ(STSR);
> > + ADM8211_CSR_WRITE(STSR, stsr);
> > + if (stsr == 0xffffffff)
> > + return IRQ_HANDLED;
> > +
> > + if (!(stsr & (ADM8211_STSR_NISS | ADM8211_STSR_AISS)))
> > + break;
> > +
> > + if (stsr & ADM8211_STSR_RCI)
> > + adm8211_interrupt_rci(dev);
> > + if (stsr & ADM8211_STSR_TCI)
> > + adm8211_interrupt_tci(dev);
> > +
> > + /*ADM8211_INT(LinkOn);*/
> > + /*ADM8211_INT(LinkOff);*/
> > +
> > + ADM8211_INT(PCF);
> > + ADM8211_INT(BCNTC);
> > + ADM8211_INT(GPINT);
> > + ADM8211_INT(ATIMTC);
> > + ADM8211_INT(TSFTF);
> > + ADM8211_INT(TSCZ);
> > + ADM8211_INT(SQL);
> > + ADM8211_INT(WEPTD);
> > + ADM8211_INT(ATIME);
> > + /*ADM8211_INT(TBTT);*/
> > + ADM8211_INT(TEIS);
> > + ADM8211_INT(FBE);
> > + ADM8211_INT(REIS);
> > + ADM8211_INT(GPTT);
> > + ADM8211_INT(RPS);
> > + ADM8211_INT(RDU);
> > + ADM8211_INT(TUF);
> > + /*ADM8211_INT(TRT);*/
> > + /*ADM8211_INT(TLT);*/
> > + /*ADM8211_INT(TDU);*/
> > + ADM8211_INT(TPS);
>
> lame. WAY too many branches, even if marked with unlikely()
>
> this should be surrounded by a single "if stsr &
> BITS_WE_SHOULD_NEVER_SEE" test
>
I'm just gonna delete these. This was only mildly interesting when the driver
was younger.
> > + } while (count++ < 20);
>
> NAK -- talk about back to the past.
>
> It's bogus to loop in the interrupt handler, then loop again in both RX
> and TX paths. You are getting close to reinventing the wheel here.
>
> Use NAPI rather than doing such a loop.
>
This is old interrupt handler code from Jouni's original driver. I never
bothered to replace it with the simpler designs used in newer drivers I've
worked on.
Also - mac80211 drivers have no access to NAPI.
> > +#define
> > WRITE_SYN(name,v_mask,v_shift,a_mask,a_shift,bits,prewrite,postwrite)\
> > +static void adm8211_rf_write_syn_ ## name (struct ieee80211_hw *dev,
> > \ + u16 addr, u32 value) { \
> > + struct adm8211_priv *priv = dev->priv; \
> > + unsigned int i; \
> > + u32 reg, bitbuf; \
> > + \
> > + value &= v_mask; \
> > + addr &= a_mask; \
> > + bitbuf = (value << v_shift) | (addr << a_shift); \
> > + \
> > + ADM8211_CSR_WRITE(SYNRF, ADM8211_SYNRF_IF_SELECT_1); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + ADM8211_CSR_WRITE(SYNRF, ADM8211_SYNRF_IF_SELECT_0); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + \
> > + if (prewrite) { \
> > + ADM8211_CSR_WRITE(SYNRF, ADM8211_SYNRF_WRITE_SYNDATA_0); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + } \
> > + \
> > + for (i = 0; i <= bits; i++) { \
> > + if (bitbuf & (1 << (bits - i))) \
> > + reg = ADM8211_SYNRF_WRITE_SYNDATA_1; \
> > + else \
> > + reg = ADM8211_SYNRF_WRITE_SYNDATA_0; \
> > + \
> > + ADM8211_CSR_WRITE(SYNRF, reg); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + \
> > + ADM8211_CSR_WRITE(SYNRF, reg | ADM8211_SYNRF_WRITE_CLOCK_1); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + ADM8211_CSR_WRITE(SYNRF, reg | ADM8211_SYNRF_WRITE_CLOCK_0); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + } \
> > + \
> > + if (postwrite == 1) { \
> > + ADM8211_CSR_WRITE(SYNRF, reg | ADM8211_SYNRF_IF_SELECT_0); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + } \
> > + if (postwrite == 2) { \
> > + ADM8211_CSR_WRITE(SYNRF, reg | ADM8211_SYNRF_IF_SELECT_1); \
> > + ADM8211_CSR_READ(SYNRF); \
> > + } \
> > + \
> > + ADM8211_CSR_WRITE(SYNRF, 0); \
> > + ADM8211_CSR_READ(SYNRF); \
> > +}
> > +
> > +WRITE_SYN(max2820, 0x00FFF, 0, 0x0F, 12, 15, 1, 1)
> > +WRITE_SYN(al2210l, 0xFFFFF, 4, 0x0F, 0, 23, 1, 1)
> > +WRITE_SYN(rfmd2958, 0x3FFFF, 0, 0x1F, 18, 23, 0, 1)
> > +WRITE_SYN(rfmd2948, 0x0FFFF, 4, 0x0F, 0, 21, 0, 2)
>
> code bloat from hell. just pass these arguments (or a pointer to an
> entry in a table that contains this info)
>
Yeah sure. I wrote that code a long time ago,, would never do something like
that now. :p
> > +static void adm8211_hw_init(struct ieee80211_hw *dev)
> > +{
> > + struct adm8211_priv *priv = dev->priv;
> > + u32 reg;
> > + u8 cline;
> > +
> > + reg = le32_to_cpu(ADM8211_CSR_READ(PAR));
> > + reg |= ADM8211_PAR_MRLE | ADM8211_PAR_MRME;
> > + reg &= ~(ADM8211_PAR_BAR | ADM8211_PAR_CAL);
>
> what do BAR and CAL bits do?
>
Need to dig up the specs to find out.
> > + if (!pci_set_mwi(priv->pdev)) {
> > + reg |= 0x1 << 24;
> > + pci_read_config_byte(priv->pdev, PCI_CACHE_LINE_SIZE, &cline);
> > +
> > + switch (cline) {
> > + case 0x8: reg |= (0x1 << 14);
> > + break;
> > + case 0x16: reg |= (0x2 << 14);
> > + break;
> > + case 0x32: reg |= (0x3 << 14);
> > + break;
> > + default: reg |= (0x0 << 14);
> > + break;
> > + }
> > + }
> > +
> > + ADM8211_CSR_WRITE(PAR, reg);
>
> this should be set regardless of MWI usage AFAICS
>
Sure.
> > + rx_info->skb = dev_alloc_skb(RX_PKT_SIZE);
> > + if (rx_info->skb == NULL)
> > + break;
>
> it seems highly bogus to set RX_PKT_SIZE for all RX descriptors, then
> bail if allocation starts failing. the state of the ring becomes out of
> sync with reality.
>
Yeah, probably. Much of the tx/rx ring code is somewhat old and questionable.
(but tested and working!)
> > + if (priv->cur_tx - priv->dirty_tx == priv->tx_ring_size - 2)
>
> extra parens would be nice for readability
>
Ok.
> > + priv->tx_buffers[entry].skb = skb;
> > + priv->tx_buffers[entry].mapping = mapping;
> > + memcpy(&priv->tx_buffers[entry].tx_control, control, sizeof(*control));
> > + priv->tx_buffers[entry].hdrlen = hdrlen;
> > + priv->tx_ring[entry].buffer1 = cpu_to_le32(mapping);
> > +
> > + if (entry == priv->tx_ring_size - 1)
> > + flag |= TDES1_CONTROL_TER;
> > + priv->tx_ring[entry].length = cpu_to_le32(flag | skb->len);
> > +
> > + /* Set TX rate (SIGNAL field in PLCP PPDU format) */
> > + flag = TDES0_CONTROL_OWN | (plcp_signal << 20) | 8 /* ? */;
> > + priv->tx_ring[entry].status = cpu_to_le32(flag);
> > +
> > + priv->cur_tx++;
> > +
> > + spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > + /* Trigger transmit poll */
> > + ADM8211_CSR_WRITE(TDR, 0);
> > +}
> > +
> > +/* Put adm8211_tx_hdr on skb and transmit */
> > +static int adm8211_tx(struct ieee80211_hw *dev, struct sk_buff *skb,
> > + struct ieee80211_tx_control *control)
> > +{
> > + struct adm8211_tx_hdr *txhdr;
> > + u16 fc;
> > + size_t payload_len, hdrlen;
> > + int plcp, dur, len, plcp_signal, short_preamble;
> > + struct ieee80211_hdr *hdr;
> > +
> > + if (control->tx_rate < 0) {
> > + short_preamble = 1;
> > + plcp_signal = -control->tx_rate;
> > + } else {
> > + short_preamble = 0;
> > + plcp_signal = control->tx_rate;
> > + }
> > +
> > + hdr = (struct ieee80211_hdr *)skb->data;
> > + fc = le16_to_cpu(hdr->frame_control) & ~IEEE80211_FCTL_PROTECTED;
> > + hdrlen = ieee80211_get_hdrlen(fc);
> > + memcpy(skb->cb, skb->data, hdrlen);
>
> to confirm: I thought skb->cb was owned by the skb creator? In this
> the driver is definitely _not_ the skb creator
>
Once the skb is passed to the driver, the driver owns cb. cb is owned by the
layer which has the skb.
> > +static int adm8211_alloc_rings(struct ieee80211_hw *dev)
> > +{
> > + struct adm8211_priv *priv = dev->priv;
> > + unsigned int ring_size;
> > +
> > + priv->rx_buffers = kmalloc(sizeof(*priv->rx_buffers) *
> > priv->rx_ring_size + + sizeof(*priv->tx_buffers) *
> > priv->tx_ring_size, GFP_KERNEL); + if (!priv->rx_buffers)
> > + return -ENOMEM;
> > +
> > + priv->tx_buffers = (void *)priv->rx_buffers +
> > + sizeof(*priv->rx_buffers) * priv->rx_ring_size;
> > +
> > + /* Allocate TX/RX descriptors */
> > + ring_size = sizeof(struct adm8211_desc) * priv->rx_ring_size +
> > + sizeof(struct adm8211_desc) * priv->tx_ring_size;
> > + priv->rx_ring = pci_alloc_consistent(priv->pdev, ring_size,
> > + &priv->rx_ring_dma);
> > +
> > + if (!priv->rx_ring) {
> > + kfree(priv->rx_buffers);
> > + priv->rx_buffers = NULL;
> > + priv->tx_buffers = NULL;
> > + return -ENOMEM;
> > + }
> > +
> > + priv->tx_ring = (struct adm8211_desc *)(priv->rx_ring +
> > + priv->rx_ring_size);
> > + priv->tx_ring_dma = priv->rx_ring_dma +
> > + sizeof(struct adm8211_desc) * priv->rx_ring_size;
> > +
>
> why not alloc ring contents (skbs) here too?
>
I really don't like this function. It allocates some ring structures at pci
initialization when it can really be deferred to open time. So, instead of
allocating the ring contents here, the ring should be allocated at the same
place where the ring contents are allocated. Doing that reduces the number of
things that can fail at pci init time.
> > +static int __devinit adm8211_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + struct ieee80211_hw *dev;
> > + struct adm8211_priv *priv;
> > + unsigned long mem_addr, mem_len;
> > + unsigned int io_addr, io_len;
> > + int err;
> > + u32 reg;
> > + u8 perm_addr[ETH_ALEN];
> > +
> > +#ifndef MODULE
> > + static unsigned int cardidx;
> > + if (!cardidx++)
> > + printk(version);
> > +#endif
>
> this is silly leftover logic from ancient days.
>
> unconditionally printk() the version here, and remove from module init
>
Sure.
> > + err = pci_enable_device(pdev);
> > + if (err) {
> > + printk(KERN_ERR "%s (adm8211): Cannot enable new PCI device\n",
> > + pci_name(pdev));
>
> no need for printk, PCI layer already does this
>
Ok.
> > + err = pci_request_regions(pdev, "adm8211");
> > + if (err) {
> > + printk(KERN_ERR "%s (adm8211): Cannot obtain PCI resources\n",
> > + pci_name(pdev));
>
> ditto
>
Ok.
> > + return err; /* someone else grabbed it? don't disable it */
> > + }
> > +
> > + if (pci_set_dma_mask(pdev, DMA_32BIT_MASK) ||
> > + pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK)) {
> > + printk(KERN_ERR "%s (adm8211): No suitable DMA available\n",
> > + pci_name(pdev));
> > + goto err_free_reg;
> > + }
> > +
> > + pci_set_master(pdev);
> > +
> > + dev = ieee80211_alloc_hw(sizeof(*priv), &adm8211_ops);
> > + if (!dev) {
> > + printk(KERN_ERR "%s (adm8211): ieee80211 alloc failed\n",
> > + pci_name(pdev));
> > + err = -ENOMEM;
> > + goto err_free_reg;
> > + }
> > + priv = dev->priv;
> > + priv->pdev = pdev;
> > +
> > + spin_lock_init(&priv->lock);
> > +
> > + SET_IEEE80211_DEV(dev, &pdev->dev);
> > +
> > + pci_set_drvdata(pdev, dev);
> > +
> > + priv->map = pci_iomap(pdev, 1, mem_len);
> > + if (!priv->map)
> > + priv->map = pci_iomap(pdev, 0, io_len);
>
> is this paranoia?
>
> just code 100% MMIO only, and ditch the iomap per-register-access overhead
>
Will do.
> > + pci_read_config_byte(pdev, PCI_CLASS_REVISION, &priv->revid);
>
> this is in struct pci_dev now
>
Convenient.
> > +/* CSR (Host Control and Status Registers) */
> > +struct adm8211_csr {
>[snip]
> > +} __attribute__ ((packed));
>
> attributed(packed) is unneccesary here, and its use results in
> sub-optimal code
>
How? Doesn't this just turn into a bunch of offsets?
> enums are preferred. they do not disappear at the cpp stage, and confer
> type information.
>
I'd rather not.
> > +#define ADM8211_IDLE_RX() \
> > +do { \
> > + if (priv->nar & ADM8211_NAR_SR) { \
> > + ADM8211_CSR_WRITE(NAR, priv->nar & ~ADM8211_NAR_SR); \
> > + ADM8211_CSR_READ(NAR); \
> > + mdelay(20); \
> > + } \
> > +} while (0)
>
> should be msleep() AFAICS
>
Nope, this can be called in atomic context. Admittedly, 20 ms delay is
ridiculously long.. I'll find a better way to handle this.
> use of <tab> to separate type and name greatly enhances readability.
> look at many other net drivers, to see the positive effects
>
Sure, will do.
Thanks,
-Michael Wu
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists