Bryan Rittmeyer wrote:
Hello again. I've been reviewing the zd1211rw code with the thought of
working on ad-hoc mode.

While reading I found the amusing locking in zd_mac_clear().

I think the point is to make sure that nobody is holding the lock before we finish clearing up. The code is obviously inaccurate because if that was even possible, someone could and would take it immediately after the spin_unlock.

There is actually even more redundancy in the clear path, see the attached patch. When the clearing happens, the network interface is down and our interrupts are disabled, so yes, no need for locking. Also, the whole structure gets freed immediately after clear, so there is no point in zeroing everything again.

The only risk is that our future housekeeping thread is still running, but it goes without saying that we must make sure that it is fully stopped when the interface is brought down when we come to implement it.

Should probably get Ulrich's opinion on this before committing, the confusing interleaving of layers is his thing ;)

Daniel
diff --git a/zd_chip.c b/zd_chip.c
index 65def95..91c3e90 100644
--- a/zd_chip.c
+++ b/zd_chip.c
@@ -42,12 +42,8 @@ void zd_chip_init(struct zd_chip *chip,
 
 void zd_chip_clear(struct zd_chip *chip)
 {
-       mutex_lock(&chip->mutex);
        zd_usb_clear(&chip->usb);
-       zd_rf_clear(&chip->rf);
-       mutex_unlock(&chip->mutex);
        mutex_destroy(&chip->mutex);
-       memset(chip, 0, sizeof(*chip));
 }
 
 static int scnprint_mac_oui(const u8 *addr, char *buffer, size_t size)
diff --git a/zd_mac.c b/zd_mac.c
index 114b23d..4bb5a14 100644
--- a/zd_mac.c
+++ b/zd_mac.c
@@ -124,11 +124,7 @@ out:
 
 void zd_mac_clear(struct zd_mac *mac)
 {
-       /* Aquire the lock. */
-       spin_lock(&mac->lock);
-       spin_unlock(&mac->lock);
        zd_chip_clear(&mac->chip);
-       memset(mac, 0, sizeof(*mac));
 }
 
 static int reset_mode(struct zd_mac *mac)
diff --git a/zd_rf.c b/zd_rf.c
index 4198062..45bd775 100644
--- a/zd_rf.c
+++ b/zd_rf.c
@@ -54,11 +54,6 @@ void zd_rf_init(struct zd_rf *rf)
        memset(rf, 0, sizeof(*rf));
 }
 
-void zd_rf_clear(struct zd_rf *rf)
-{
-       memset(rf, 0, sizeof(*rf));
-}
-
 int zd_rf_init_hw(struct zd_rf *rf, u8 type)
 {
        int r, t;
diff --git a/zd_rf.h b/zd_rf.h
index 0276e1e..0da2cf8 100644
--- a/zd_rf.h
+++ b/zd_rf.h
@@ -64,7 +64,6 @@ struct zd_rf {
 
 const char *zd_rf_name(u8 type);
 void zd_rf_init(struct zd_rf *rf);
-void zd_rf_clear(struct zd_rf *rf);
 int zd_rf_init_hw(struct zd_rf *rf, u8 type);
 int zd_rf_reset_hw(struct zd_rf *rf);
 
diff --git a/zd_usb.c b/zd_usb.c
index b773ec1..bfa3208 100644
--- a/zd_usb.c
+++ b/zd_usb.c
@@ -865,7 +865,6 @@ void zd_usb_clear(struct zd_usb *usb)
 {
        usb_set_intfdata(usb->intf, NULL);
        usb_put_intf(usb->intf);
-       memset(usb, 0, sizeof(*usb));
        /* FIXME: usb_interrupt, usb_tx, usb_rx? */
 }
 
_______________________________________________
Zd1211-devs mailing list - http://zd1211.ath.cx/
Unsubscribe: https://lists.sourceforge.net/lists/listinfo/zd1211-devs

Reply via email to