ZyDAS ZD1211 USB-WLAN driver

I tried to submit this patch yesterday, but it doesn't appear to have been delivered. The patch is probably a bit on the large side, so I'll try again over http:

http://dev.gentoo.org/~dsd/kernel/zd1211rw.patch

Any comments appreciated.

[PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

There are 60+ USB wifi adapters available on the market based on the ZyDAS ZD1211 chip.

Unlike the predecessor (ZD1201), ZD1211 does not have a hardware MAC, so most data operations are coordinated by the device driver. The ZD1211 chip sits alongside an RF transceiver which is also controlled by the driver. Our driver currently supports 2 RF types, we know of one other available in a few marketed products which we will be supporting soon.

Our driver also supports the newer revision of ZD1211, called ZD1211B. The initialization and RF operations are slightly different for the new revision, but the main difference is 802.11e support. Our driver does not support the QoS features yet, but we think we know how to use them.

This driver is based on ZyDAS's own GPL driver available from www.zydas.com.tw. ZyDAS engineers have been responsive and supportive of our efforts, so thumbs up to them. Additionally, the firmware is redistributable and they have provided device specs.

This driver has been written primarily by Ulrich Kunitz and myself. Graham Gower, Greg KH, Remco and Bryan Rittmeyer have also contributed. The developers of ieee80211 and softmac have made our lives so much easier- thanks!

We maintain a small info-page: http://zd1211.ath.cx/wiki/DriverRewrite

If there is enough time for review, we would like to aim for inclusion in 2.6.18. The driver works nicely as a STA, and can connect to both open and encrypted networks (we are using software-based encryption for now). We will work towards supporting more advanced features in the future (ad-hoc, master mode, 802.11a, ...).

Am Samstag, 3. Juni 2006 13:20 schrieb Daniel Drake:

+static int read_mac_addr(struct zd_chip *chip, u8 *mac_addr) +{
+ static const zd_addr_t addr[2] = { CR_MAC_ADDR_P1, CR_MAC_ADDR_P2 };
+ return _read_mac_addr(chip, mac_addr, (const zd_addr_t *)addr);
+}

Why on the stack?

+static int zd1211_hw_reset_phy(struct zd_chip *chip) +{
+ static const struct zd_ioreq16 ioreqs[] = {

+		{ CR0,   0x0a }, { CR1,   0x06 }, { CR2,   0x26 },
+		{ CR3,   0x38 }, { CR4,   0x80 }, { CR9,   0xa0 },
+		{ CR10,  0x81 }, { CR11,  0x00 }, { CR12,  0x7f },
+		{ CR13,  0x8c }, { CR14,  0x80 }, { CR15,  0x3d },
+		{ CR16,  0x20 }, { CR17,  0x1e }, { CR18,  0x0a },
+		{ CR19,  0x48 }, { CR20,  0x0c }, { CR21,  0x0c },
+		{ CR22,  0x23 }, { CR23,  0x90 }, { CR24,  0x14 },
+		{ CR25,  0x40 }, { CR26,  0x10 }, { CR27,  0x19 },
+		{ CR28,  0x7f }, { CR29,  0x80 }, { CR30,  0x4b },
+		{ CR31,  0x60 }, { CR32,  0x43 }, { CR33,  0x08 },
+		{ CR34,  0x06 }, { CR35,  0x0a }, { CR36,  0x00 },
+		{ CR37,  0x00 }, { CR38,  0x38 }, { CR39,  0x0c },
+		{ CR40,  0x84 }, { CR41,  0x2a }, { CR42,  0x80 },
+		{ CR43,  0x10 }, { CR44,  0x12 }, { CR46,  0xff },
+		{ CR47,  0x1E }, { CR48,  0x26 }, { CR49,  0x5b },
+		{ CR64,  0xd0 }, { CR65,  0x04 }, { CR66,  0x58 },
+		{ CR67,  0xc9 }, { CR68,  0x88 }, { CR69,  0x41 },
+		{ CR70,  0x23 }, { CR71,  0x10 }, { CR72,  0xff },
+		{ CR73,  0x32 }, { CR74,  0x30 }, { CR75,  0x65 },
+		{ CR76,  0x41 }, { CR77,  0x1b }, { CR78,  0x30 },
+		{ CR79,  0x68 }, { CR80,  0x64 }, { CR81,  0x64 },
+		{ CR82,  0x00 }, { CR83,  0x00 }, { CR84,  0x00 },
+		{ CR85,  0x02 }, { CR86,  0x00 }, { CR87,  0x00 },
+		{ CR88,  0xff }, { CR89,  0xfc }, { CR90,  0x00 },
+		{ CR91,  0x00 }, { CR92,  0x00 }, { CR93,  0x08 },
+		{ CR94,  0x00 }, { CR95,  0x00 }, { CR96,  0xff },
+		{ CR97,  0xe7 }, { CR98,  0x00 }, { CR99,  0x00 },

+ { CR100, 0x00 }, { CR101, 0xae }, { CR102, 0x02 },
+ { CR103, 0x00 }, { CR104, 0x03 }, { CR105, 0x65 },
+ { CR106, 0x04 }, { CR107, 0x00 }, { CR108, 0x0a },
+ { CR109, 0xaa }, { CR110, 0xaa }, { CR111, 0x25 },
+ { CR112, 0x25 }, { CR113, 0x00 }, { CR119, 0x1e },
+ { CR125, 0x90 }, { CR126, 0x00 }, { CR127, 0x00 },
+ { },

+		{ CR5,   0x00 }, { CR6,   0x00 }, { CR7,   0x00 },
+		{ CR8,   0x00 }, { CR9,   0x20 }, { CR12,  0xf0 },
+		{ CR20,  0x0e }, { CR21,  0x0e }, { CR27,  0x10 },
+		{ CR44,  0x33 }, { CR47,  0x1E }, { CR83,  0x24 },
+		{ CR84,  0x04 }, { CR85,  0x00 }, { CR86,  0x0C },
+		{ CR87,  0x12 }, { CR88,  0x0C }, { CR89,  0x00 },
+		{ CR90,  0x10 }, { CR91,  0x08 }, { CR93,  0x00 },
+		{ CR94,  0x01 }, { CR95,  0x00 }, { CR96,  0x50 },
+		{ CR97,  0x37 }, { CR98,  0x35 }, { CR101, 0x13 },

+ { CR102, 0x27 }, { CR103, 0x27 }, { CR104, 0x18 },
+ { CR105, 0x12 }, { CR109, 0x27 }, { CR110, 0x27 },
+ { CR111, 0x27 }, { CR112, 0x27 }, { CR113, 0x27 },
+ { CR114, 0x27 }, { CR115, 0x26 }, { CR116, 0x24 },
+ { CR117, 0xfc }, { CR118, 0xfa }, { CR120, 0x4f },
+ { CR123, 0x27 }, { CR125, 0xaa }, { CR127, 0x03 },
+ { CR128, 0x14 }, { CR129, 0x12 }, { CR130, 0x10 },
+ { CR131, 0x0C }, { CR136, 0xdf }, { CR137, 0x40 },
+ { CR138, 0xa0 }, { CR139, 0xb0 }, { CR140, 0x99 },
+ { CR141, 0x82 }, { CR142, 0x54 }, { CR143, 0x1c },
+ { CR144, 0x6c }, { CR147, 0x07 }, { CR148, 0x4c },
+ { CR149, 0x50 }, { CR150, 0x0e }, { CR151, 0x18 },
+ { CR160, 0xfe }, { CR161, 0xee }, { CR162, 0xaa },
+ { CR163, 0xfa }, { CR164, 0xfa }, { CR165, 0xea },
+ { CR166, 0xbe }, { CR167, 0xbe }, { CR168, 0x6a },
+ { CR169, 0xba }, { CR170, 0xba }, { CR171, 0xba },
+ /* Note: CR204 must lead the CR203 */
+ { CR204, 0x7d },
+ { },
+ { CR203, 0x30 },
+ };

This is too much to allocate on the stack.

+static void disconnect(struct usb_interface *intf) +{
+ struct net_device *netdev = zd_intf_to_netdev(intf);
+ struct zd_mac *mac = zd_netdev_mac(netdev);
+ struct zd_usb *usb = &mac->chip.usb;
+
+ dev_dbg_f(zd_usb_dev(usb), "\n");
+
+ zd_netdev_disconnect(netdev);
+
+ /* Just in case something has gone wrong! */
+ zd_usb_disable_rx(usb);
+ zd_usb_disable_int(usb);
+
+ /* If the disconnect has been caused by a removal of the
+ * driver module, the reset allows reloading of the driver. If the
+ * reset will not be executed here, the upload of the firmware in the
+ * probe function caused by the reloading of the driver will fail.
+ */
+ usb_reset_device(interface_to_usbdev(intf));
+
+ /* If somebody still waits on this lock now, this is an error. */
+ zd_netdev_free(netdev);
+ dev_dbg(&intf->dev, "disconnected\n");
+}

This is racy. It allows io to disconnected devices. You must take the lock and set a flag that you test after you've taken the lock elsewhere.

Regards
Oliver

Technically it's not on the stack because it is static const (it goes in rodata), but I don't think that this invalidates your point. What's the alternative? kmalloc and kfree every time?

(Just seems a little over the top for such a small array)

Again static const, it's definately in rodata, checked with objdump. Do we need to change this?

Will fix, thanks.

Daniel

Am Samstag, 3. Juni 2006 21:35 schrieb Daniel Drake:

In this case rodata will work. However, if you ever switch to direct DMA it will fail. I really did overlook the const keyword.

[..]

You're welcome

Regards
Oliver

Hello,

I must confess I don't know much about the ZyDas driver and the rewrite dirver, but folliowing this post I looked a bit at the code (of both zd1211 and the rewrite version) and I have a little question; this may be
seen as a (little) off topic but I will be happy if somebody will raise this coin.

I had noticed that the zd1211 driver does call request_irq() in zd1205_open(),
file zd1205.c; grepping for request_irq() in the rewrite driver yields no results.
(I looked at the rewrite version from a week ago but in this point it is probably the
same).
Why is this so ? I assume that the softmac layer does not call request_irq() on behalf of the driver because this is not supposed to be like it, as I understand its functionality. Can anybody briefly calrify this point ?

Regards,
John

Am Sonntag, 4. Juni 2006 18:29 schrieb John Que:

A USB driver never will request an irq. Interrupt handling is done in the core usb layer. Individual drivers have no business there.

Regards
Oliver

Hi,

While I did not yet review fully the code , I saw some definitions
which are not in use. (more than 150 such definitions, in one file) I think it may be better to remove this #define statements to make the code more readable. In case there will be a need in the future, the needed #define statements can be returned.
The definitions which are not in use are from zd_chip.h.

It is mentioned there that these definitions are taken from the ZYDAS driver and not all of them are relevant for the rewrite driver; however, it seems to me that
removing them may be thought of.

Anyhow you did a great job and I hope I will find time to really delve into the code.

The definitions I am talking about are: (see zd_chip.h)

CR_RF_IF_CLK
CR_RF_IF_DATA
CR_PE1_PE2
CR_PE2_DLY
CR_LE1
CR_LE2
CR_RADIO_PD
CR_RF2948_PD
CR_ENABLE_PS_MANUAL_AGC
CR_SA2400_SER_AP
CR_I2C_WRITE
CR_SA2400_SER_RP
CR_RADIO_PE
CR_RST_BUS_MASTER
CR_HSTSCHG
CR_PHY_ON
CR_RX_DELAY
CR_GPIO_1
CR_GPIO_2
CR_EncryBufMux
CR_MAC_PS_STATE
INT_TX_COMPLETE
INT_RX_COMPLETE
INT_RETRY_FAIL
INT_WAKEUP
INT_DTIM_NOTIFY
INT_CFG_NEXT_BCN
INT_BUS_ABORT
INT_TX_FIFO_READY
INT_UART
INT_TX_COMPLETE_EN
INT_RX_COMPLETE_EN
INT_RETRY_FAIL_EN
INT_WAKEUP_EN
INT_DTIM_NOTIFY_EN
INT_CFG_NEXT_BCN_EN
INT_BUS_ABORT_EN
INT_TX_FIFO_READY_EN
INT_UART_EN
CR_TSF_LOW_PART
CR_TSF_HIGH_PART
CR_UART_RBR_THR_DLL
CR_UART_DLM_IER
CR_UART_IIR_FCR
CR_UART_LCR
CR_UART_MCR
CR_UART_LSR
CR_UART_MSR
CR_UART_ECR
CR_UART_STATUS
CR_PCI_TX_ADDR_P1
CR_PCI_TX_AddR_P2
CR_PCI_RX_AddR_P1
CR_PCI_RX_AddR_P2
CR_BSSID_P1
CR_BSSID_P2
CR_RX_TIMEOUT
CR_RATE_9M
CR_RATE_18M
CR_RATE_36M
CR_RATE_48M
CR_RATE_54M
CR_RX_OFFSET
CR_PHY_DELAY
CR_BCN_FIFO
CR_DEVICE_STATE
CR_UNDERRUN_CNT
CR_BCN_FIFO_SEMAPHORE
CR_IFS_VALUE
CR_RX_TIME_OUT
CR_TOTAL_RX_FRM
CR_CRC32_CNT
CR_CRC16_CNT
CR_DECRYPTION_ERR_UNI
CR_RX_FIFO_OVERRUN
CR_DECRYPTION_ERR_MUL
CR_NAV_CNT
CR_NAV_CCA
CR_RETRY_CNT
CR_READ_TCB_ADDR
CR_READ_RFD_ADDR
CR_TOTAL_TX_FRM
CR_CAM_MODE
CR_CAM_ROLL_TB_LOW
CR_CAM_ROLL_TB_HIGH
CR_CAM_ADDRESS
CR_CAM_DATA
CR_ROMDIR
CR_DECRY_ERR_FLG_LOW
CR_DECRY_ERR_FLG_HIGH
CR_WEPKEY0
CR_WEPKEY1
CR_WEPKEY2
CR_WEPKEY3
CR_WEPKEY4
CR_WEPKEY5
CR_WEPKEY6
CR_WEPKEY7
CR_WEPKEY8
CR_WEPKEY9
CR_WEPKEY10
CR_WEPKEY11
CR_WEPKEY12
CR_WEPKEY13
CR_WEPKEY14
CR_WEPKEY15
CR_TKIP_MODE
CR_EEPROM_PROTECT0
CR_EEPROM_PROTECT1
CR_DBG_FIFO_RD
CR_DBG_SELECT
CR_FIFO_Length
CR_RSSI_MGC
CR_PON
CR_RX_ON
CR_TX_ON
CR_CHIP_EN
CR_LO_SW
CR_TXRX_SW
CR_S_MD
CR_USB_DEBUG_PORT
STA_RX_FILTER
E2P_PWR_CAL_VALUE2
E2P_PWR_CAL_VALUE3
E2P_PWR_CAL_VALUE4
E2P_PWR_INT_VALUE2
E2P_PWR_INT_VALUE3
E2P_PWR_INT_VALUE4
E2P_ALLOWED_CHANNEL
E2P_DEVICE_VER
E2P_36M_CAL_VALUE2
E2P_36M_CAL_VALUE3
E2P_36M_CAL_VALUE4
E2P_11A_INT_VALUE1
E2P_11A_INT_VALUE2
E2P_11A_INT_VALUE3
E2P_11A_INT_VALUE4
E2P_48M_CAL_VALUE2
E2P_48M_CAL_VALUE3
E2P_48M_CAL_VALUE4
E2P_48M_INT_VALUE1
E2P_48M_INT_VALUE2
E2P_48M_INT_VALUE3
E2P_48M_INT_VALUE4
E2P_54M_CAL_VALUE2
E2P_54M_CAL_VALUE3
E2P_54M_CAL_VALUE4
E2P_54M_INT_VALUE1
E2P_54M_INT_VALUE2
E2P_54M_INT_VALUE3
E2P_54M_INT_VALUE4
FW_SOFT_RESET
FW_FLASH_CHK

Regards,
Rami Rosen

They are mostly relevant, not all come from the vendor driver directly, some will be used later, many are useful for documenting the bits and bobs that we *do* know about the CR registers (not a lot).

I don't think there are any readability issues while all defines are kept out of the way from the code.

Daniel

Hello Oliver,

I am sorry, but I think I don't understand ,

but in the zd1211 driver (not the rewrite version) I found this call to request an irq:
request_irq(dev->irq, &zd1205_intr, SA_SHIRQ, dev->name, dev)

Also when looking in the linux tree, I see some request_irq() calls in USB drivers, for exmaple in some drivers under usb/gadgaets, and more.

Can you please elaborate a bit ? What do you mean by saying "A USB driver never will request an irq"?

-- John

Am Sonntag, 4. Juni 2006 20:22 schrieb John Que:

The architecture is as follows:

device driver <---> USB core <---> host driver <=== USB ===> gadget driver <---> function driver

Only those drivers that deal with the hardware directly request interrupts. A device driver submits URBs and deals with callbacks.

Regards
Oliver

Look at it in context:

#ifndef HOST_IF_USB	
	if ((rc = request_irq(dev->irq, &zd1205_intr, SA_SHIRQ, dev->name, 
dev)) != 0) {
		printk(KERN_ERR "zd1205: failed to request_irq\n");	

The ZD1211 vendor driver appears to be an adapted version of another ZyDAS driver which supports a PCI wifi chip, presumably there are some similarities. There are many such headaches when trying to read through this driver...

request_irq never happens because HOST_IF_USB gets defined.

Daniel

On some platforms rodata will work; but it's not guaranteed on any. See Documentation/DMA-mapping.txt and read the little section at the top explaining what types of memory can be DMA'd from ... neither stack, nor BSS, nor data, nor rodata, nor text are OK.

- Dave

Not so fast, because I don't know what to fix.

Actually there are two cases, when disconnect is called. The first is, when the device is removed. The second is, when the kernel module is removed from the kernel. It should also be recognized that disconnect() must always succeed, so we have to ignore IO errors anyhow.

In the first case we don't need the IO and in the second case we have to do it, because otherwise, the device will not come up, if we load the driver again.

Testing shows, that doing the IO doesn't create a lot of issues, there are some errors reported, but that's it and the second case just works fine. So there doesn't appear a problem and even if want to be "clean", I would like to know, how to distinguish both cases without trying to do IO with the device.

In the meanwhile I removed the misleading comment about locking. I would like also like to know, that beside the fact that device IO fails, which races can happen?

Uli Kunitz

Am Samstag, 10. Juni 2006 13:23 schrieb Ulrich Kunitz:

There are three cases. It can also be triggered by sysfs and usbfs.

You can ignore IO errors in disconnect(). They are harmless.

The problem is not that you may do IO _during_ disconnect. You may do IO _after_ disconnect, even if usbfs might already have claimed the device. Disconnect() must either terminate ongoing IO or wait for it to end. And it must make sure that there'll be no IO afterwards.

Regards
Oliver

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

I am working on it. A couple more mails to Herbert Xu should clarify it, then I'll post a patch. On first glance I was quite worried that there are a lot of potential races, but now I think there will only be a few places.

Daniel

It's been a bit of an adventure, but I'm now fairly confident that only a single-line change is required to fix this: stop the TX queue early on in the netdev->stop function.

When disconnect() happens, we call unregister_netdev(). This will call netdev->stop if the interface was up, and netdev->stop ensures that no more packets are queued for transmission, and that the host does not receive any more packets or interrupts from the device.

unregister_netdev() also does some synchronization and rtnl stuff, and after it has returned, it is guaranteed that no ioctls or other functions (hard_start_xmit/set_mac_address) are in progress, and it is guaranteed that no more of those will happen.

In disconnect() we also kill all urbs that we know about. All of our URB completions handle the cancelled case without generating further I/O.

Additionally, the return values of all of our usb I/O function calls are checked and propagated up, so most code will simply stop when the device gets unplugged.

At this point, it's now safe to say that nobody is using (or will use) the various structures, so disconnect() frees the whole lot and returns.

Do you agree, or did you have certain other race paths in mind which I might have missed?

Thanks,
Daniel