Main griefs:
a) home-grown lock debugger (what can it do what lockdep can't?)
b) lack of endian annotations.
c) driver fallbacks to USA regulatory domain if it can't find valid one
from the list. My gut feeling is that this can bring an unattentive Linux
user to police on bad day.
lesser griefs are scattered over the rest of email.
-
+/* Locking: */
+/* very talkative */
+/* #define PARANOID_LOCKING 1 */
+/* normal (use when bug-free) */
+#define DO_LOCKING 1
+/* else locking is disabled! */
lock debugging.
+#define CLEAR_BIT(val, mask) ((val) = ~(mask))
+#define SET_BIT(val, mask) ((val) |= (mask))
silly macros and even misused in code like
CLEAR_BIT(feat.feature_options, cpu_to_le32(feature_options));
Please, open code.
+/* These functions *must* be inline or they will break horribly on SPARC, due
+ * to its weird semantics for save/restore flags */
+
+#if defined(PARANOID_LOCKING) /* Lock debugging */
+
+void acx_lock_debug(acx_device_t *adev, const char* where);
+void acx_unlock_debug(acx_device_t *adev, const char* where);
+void acx_down_debug(acx_device_t *adev, const char* where);
+void acx_up_debug(acx_device_t *adev, const char* where);
+void acx_lock_unhold(void);
+void acx_sem_unhold(void);
+
+static inline void
+acx_lock_helper(acx_device_t *adev, unsigned long *fp, const char* where)
+{
+ acx_lock_debug(adev, where);
+ spin_lock_irqsave(adev-lock, *fp);
+}
+static inline void
+acx_unlock_helper(acx_device_t *adev, unsigned long *fp, const char* where)
+{
+ acx_unlock_debug(adev, where);
+ spin_unlock_irqrestore(adev-lock, *fp);
+}
+static inline void
+acx_down_helper(acx_device_t *adev, const char* where)
+{
+ acx_down_debug(adev, where);
+}
+static inline void
+acx_up_helper(acx_device_t *adev, const char* where)
+{
+ acx_up_debug(adev, where);
+}
+#define acx_lock(adev, flags) acx_lock_helper(adev, (flags), __FILE__ :
STRING(__LINE__))
+#define acx_unlock(adev, flags)acx_unlock_helper(adev, (flags),
__FILE__ : STRING(__LINE__))
+#define acx_sem_lock(adev) acx_down_helper(adev, __FILE__ :
STRING(__LINE__))
+#define acx_sem_unlock(adev) acx_up_helper(adev, __FILE__ :
STRING(__LINE__))
+
+#elif defined(DO_LOCKING)
+
+#define acx_lock(adev, flags) spin_lock_irqsave(adev-lock, flags)
+#define acx_unlock(adev, flags)spin_unlock_irqrestore(adev-lock,
flags)
+#define acx_sem_lock(adev) down(adev-sem)
+#define acx_sem_unlock(adev) up(adev-sem)
+#define acx_lock_unhold() ((void)0)
+#define acx_sem_unhold() ((void)0)
+
+#else /* no locking! :( */
+
+#define acx_lock(adev, flags) ((void)0)
+#define acx_unlock(adev, flags)((void)0)
+#define acx_sem_lock(adev) ((void)0)
+#define acx_sem_unlock(adev) ((void)0)
+#define acx_lock_unhold() ((void)0)
+#define acx_sem_unhold() ((void)0)
+
+#endif
+enum {
+ L_LOCK = (ACX_DEBUG1)*0x0001, /* locking debug log */
lock debugging
+#define ACX_PACKED __WLAN_ATTRIB_PACK__
Just add __packed in kernel.h I guess.
+#define VEC_SIZE(a) (sizeof(a)/sizeof(a[0]))
That would be already existing ARRAY_SIZE()
+/***
+** Constants
+*/
+#define OK 0
+#define NOT_OK 1
That's not OK.
+#if !defined(CONFIG_ACX_PCI) !defined(CONFIG_ACX_USB)
+#error Driver must include PCI and/or USB support. You selected neither.
+#endif
Can it be done some via Kconfig magic? I don't know.
+/* An opaque typesafe helper type
+ *
+ * Some hardware fields are actually pointers,
+ * but they have to remain u32, since using ptr instead
+ * (8 bytes on 64bit systems!) would disrupt the fixed descriptor
+ * format the acx firmware expects in the non-user area.
+ * Since we cannot cram an 8 byte ptr into 4 bytes, we need to
+ * enforce that pointed to data remains in low memory
+ * (address value needs to fit in 4 bytes) on 64bit systems.
+ *
+ * This is easy to get wrong, thus we are using a small struct
+ * and special macros to access it. Macros will check for
+ * attempts to overflow an acx_ptr with value 0x.
+ *
+ * Attempts to use acx_ptr without macros result in compile-time errors */
+
+typedef struct {
+ u32 v;
+} ACX_PACKED acx_ptr;
+
+#if ACX_DEBUG
+#define CHECK32(n) BUG_ON(sizeof(n)4 (long)(n)0xff00)
+#else
+#define CHECK32(n) ((void)0)
+#endif
+
+/* acx_ptr - integer conversion */
+#define cpu2acx(n) ({ CHECK32(n); ((acx_ptr){ .v = cpu_to_le32(n) }); })
+#define acx2cpu(a) (le32_to_cpu(a.v))
+
+/* acx_ptr - pointer conversion */
+#define ptr2acx(p) ({ CHECK32(p); ((acx_ptr){ .v = cpu_to_le32((u32)(long)(p))
}); })
+#define acx2ptr(a) ((void*)le32_to_cpu(a.v))
Duh!
+struct acx_device {
+ /* most frequent accesses first (dereferencing and cache line!) */
+
+ /*** Locking ***/
+ struct semaphore
