[PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

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, ...).

Signed-off-by: Daniel Drake <dsd@gentoo.org>

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Oliver Neukum]

Am Samstag, 3. Juni 2006 13:20 schrieb Daniel Drake:
> 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.

+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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

Oliver Neukum wrote:
> +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?

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)

> +static int zd1211_hw_reset_phy(struct zd_chip *chip)
> +{
> +	static const struct zd_ioreq16 ioreqs[] = {
> 
> This is too much to allocate on the stack.

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

> +static void disconnect(struct usb_interface *intf)
> 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.

Will fix, thanks.

Daniel

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Oliver Neukum]

Am Samstag, 3. Juni 2006 21:35 schrieb Daniel Drake:
> Oliver Neukum wrote:
> > +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?
> 
> 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?

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

[..]
> > +static void disconnect(struct usb_interface *intf)
> > 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.
> 
> Will fix, thanks.

You're welcome

	Regards
		Oliver

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[John Que]

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


On 6/4/06, Oliver Neukum <oliver@neukum.org> wrote:
> Am Samstag, 3. Juni 2006 21:35 schrieb Daniel Drake:
> > Oliver Neukum wrote:
> > > +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?
> >
> > 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?
>
> In this case rodata will work. However, if you ever switch to direct DMA
> it will fail. I really did overlook the const keyword.
>
> [..]
> > > +static void disconnect(struct usb_interface *intf)
> > > 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.
> >
> > Will fix, thanks.
>
> You're welcome
>
>         Regards
>                 Oliver
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Oliver Neukum]

Am Sonntag, 4. Juni 2006 18:29 schrieb John Que:
>   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 ?

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Rami Rosen]

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



On 6/4/06, Oliver Neukum <oliver@neukum.org> wrote:
> Am Sonntag, 4. Juni 2006 18:29 schrieb John Que:
> > 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 ?
>
> 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
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

Rami Rosen wrote:
> 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.

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[John Que]

Hello Oliver,

    I am sorry, but I think I don't understand ,
You said:
>A USB driver never will request an irq. Interrupt handling is done in
> the core usb layer. Individual drivers have no business there.

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

On 6/4/06, Oliver Neukum <oliver@neukum.org> wrote:
> Am Sonntag, 4. Juni 2006 18:29 schrieb John Que:
> > 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 ?
>
> 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
>

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Oliver Neukum]

Am Sonntag, 4. Juni 2006 20:22 schrieb John Que:
> Hello Oliver,
> 
>     I am sorry, but I think I don't understand ,
> You said:
> >A USB driver never will request an irq. Interrupt handling is done in
> > the core usb layer. Individual drivers have no business there.
> 
> 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"?

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

John Que wrote:
>  I had noticed that the zd1211 driver does call request_irq() in 
> zd1205_open(),
> file zd1205.c; 

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[David Brownell]

On Saturday 03 June 2006 3:25 pm, Oliver Neukum wrote:
> Am Samstag, 3. Juni 2006 21:35 schrieb Daniel Drake:
> > Oliver Neukum wrote:
> > > +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?
> > 
> > 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?
> 
> In this case rodata will work. However, if you ever switch to direct DMA
> it will fail. I really did overlook the const keyword.

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Ulrich Kunitz]

On 06-06-04 00:25 Oliver Neukum wrote:

> > > +static void disconnect(struct usb_interface *intf)
> > > 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.
> > 
> > Will fix, thanks.
> 
> You're welcome

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

Re: [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Oliver Neukum]

Am Samstag, 10. Juni 2006 13:23 schrieb Ulrich Kunitz:
> On 06-06-04 00:25 Oliver Neukum wrote:
> 
> > > > +static void disconnect(struct usb_interface *intf)
> > > > 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.
> > > 
> > > Will fix, thanks.
> > 
> > You're welcome
> 
> Not so fast, because I don't know what to fix.
> 
> Actually there are two cases, when disconnect is called. The first

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

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

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

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

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


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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

Ulrich Kunitz wrote:
> Not so fast, because I don't know what to fix.

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

Re: [linux-usb-devel] [PATCH RFC] ZyDAS ZD1211 USB-WLAN driver

[Daniel Drake]

Oliver Neukum wrote:
> +static void disconnect(struct usb_interface *intf)
> +{
> 
> 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.

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