[2/4] ar5523: Add driver header file

Commit Message

Pontus Fuchs Oct. 11, 2012, 8:01 p.m. UTC

Header file containing structures and defines used by the ar5523
driver.

Signed-off-by: Pontus Fuchs <[e-mail]>
---
 drivers/staging/ar5523/ar5523.h |  164 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 drivers/staging/ar5523/ar5523.h

Comments

Kalle Valo Oct. 13, 2012, 5:41 a.m. UTC | #1

Pontus Fuchs <[e-mail]> writes:

> +#define ar5523_err(ar, format, arg...) \
> +do { \
> +	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
> +		dev_err(&(ar)->dev->dev, format, ## arg); \
> +	} \
> +} while (0)

This looks suspicious and might hide something important. It's better to
fix the real cause than to workaround it in the error log printer. Do
you know why this was added?

Pontus Fuchs Oct. 13, 2012, 10:14 a.m. UTC | #2

On 2012-10-13 07:41, Kalle Valo wrote:
> Pontus Fuchs <[e-mail]> writes:
>
>> +#define ar5523_err(ar, format, arg...) \
>> +do { \
>> +	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
>> +		dev_err(&(ar)->dev->dev, format, ## arg); \
>> +	} \
>> +} while (0)
> This looks suspicious and might hide something important. It's better to
> fix the real cause than to workaround it in the error log printer. Do
> you know why this was added?
Yes. The purpose is to hide all the USB errors that happens when you
hot-unplug the dongle. There can be quite a few URBs in flight and
all of them will fail with -ENODEV or -ESHUTDOWN. Instead of checking
for these errors in all the usb submissions and callbacks I added this
flag. Do you still a problem with this approach?

Cheers,

Pontus

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig Oct. 13, 2012, 3:45 p.m. UTC | #3

On Sat, Oct 13, 2012 at 12:14:29PM +0200, Pontus Fuchs wrote:
> On 2012-10-13 07:41, Kalle Valo wrote:
>> Pontus Fuchs <[e-mail]> writes:
>>
>>> +#define ar5523_err(ar, format, arg...) \
>>> +do { \
>>> +	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
>>> +		dev_err(&(ar)->dev->dev, format, ## arg); \
>>> +	} \
>>> +} while (0)
>> This looks suspicious and might hide something important. It's better to
>> fix the real cause than to workaround it in the error log printer. Do
>> you know why this was added?
> Yes. The purpose is to hide all the USB errors that happens when you
> hot-unplug the dongle. There can be quite a few URBs in flight and
> all of them will fail with -ENODEV or -ESHUTDOWN. Instead of checking
> for these errors in all the usb submissions and callbacks I added this
> flag. Do you still a problem with this approach?

I think it's fine, but it needs a comment explaining it with basically
the same information you just wrote above.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Valo Oct. 21, 2012, 7:58 p.m. UTC | #4

Pontus Fuchs <[e-mail]> writes:

> On 2012-10-13 07:41, Kalle Valo wrote:
>> Pontus Fuchs <[e-mail]> writes:
>>
>>> +#define ar5523_err(ar, format, arg...) \
>>> +do { \
>>> +	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
>>> +		dev_err(&(ar)->dev->dev, format, ## arg); \
>>> +	} \
>>> +} while (0)
>> This looks suspicious and might hide something important. It's better to
>> fix the real cause than to workaround it in the error log printer. Do
>> you know why this was added?
>
> Yes. The purpose is to hide all the USB errors that happens when you
> hot-unplug the dongle. There can be quite a few URBs in flight and
> all of them will fail with -ENODEV or -ESHUTDOWN. Instead of checking
> for these errors in all the usb submissions and callbacks I added this
> flag. Do you still a problem with this approach?

IMHO it's bad style to hide that inside a logging macro and might hide
some other bugs. It's better to check the state explicitly in the
callback.

But again this isn't anything important, just something I noticed while
reviewing the code. Do what you think is best.

Patch

diff --git a/drivers/staging/ar5523/ar5523.h b/drivers/staging/ar5523/ar5523.h
new file mode 100644
index 0000000..119f9ed
--- /dev/null
+++ b/drivers/staging/ar5523/ar5523.h
@@ -0,0 +1,164 @@ 
+/*
+ * Copyright (c) 2012 Pontus Fuchs <[e-mail]>
+ *
+ *  This file is free software: you may copy, redistribute and/or modify it
+ *  under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License, or (at your
+ *  option) any later version.
+ *
+ *  This file is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * This file incorporates work covered by the following copyright and
+ * permission notice:
+ *
+ * Copyright (c) 2006
+ *	Damien Bergamini <[e-mail]>
+ * Copyright (c) 2006 Sam Leffler, Errno Consulting
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#define AR5523_FLAG_PRE_FIRMWARE	(1 << 0)
+#define AR5523_FLAG_ABG			(1 << 1)
+
+#define AR5523_FIRMWARE_FILE	"ar5523.bin"
+
+#define AR5523_CMD_TX_PIPE	0x01
+#define	AR5523_DATA_TX_PIPE	0x02
+#define	AR5523_CMD_RX_PIPE	0x81
+#define	AR5523_DATA_RX_PIPE	0x82
+
+#define ar5523_cmd_tx_pipe(dev) \
+	usb_sndbulkpipe((dev), AR5523_CMD_TX_PIPE)
+#define ar5523_data_tx_pipe(dev) \
+	usb_sndbulkpipe((dev), AR5523_DATA_TX_PIPE)
+#define ar5523_cmd_rx_pipe(dev) \
+	usb_rcvbulkpipe((dev), AR5523_CMD_RX_PIPE)
+#define ar5523_data_rx_pipe(dev) \
+	usb_rcvbulkpipe((dev), AR5523_DATA_RX_PIPE)
+
+#define	AR5523_DATA_TIMEOUT	10000
+#define	AR5523_CMD_TIMEOUT	1000
+
+#define AR5523_TX_DATA_COUNT		4
+#define AR5523_TX_DATA_RESTART_COUNT	1
+#define AR5523_RX_DATA_COUNT		16
+#define AR5523_RX_DATA_REFILL_COUNT	8
+
+#define AR5523_CMD_ID	1
+#define AR5523_DATA_ID	2
+
+#define AR5523_TX_WD_TIMEOUT	(HZ * 2)
+#define AR5523_FLUSH_TIMEOUT	(HZ * 3)
+
+enum AR5523_flags {
+	AR5523_HW_UP,
+	AR5523_USB_DISCONNECTED,
+	AR5523_CONNECTED
+};
+
+struct ar5523_tx_cmd {
+	struct ar5523		*ar;
+	struct urb		*urb_tx;
+	void			*buf_tx;
+	void			*odata;
+	int			olen;
+	int			flags;
+	int			res;
+	struct completion	done;
+};
+
+/* This struct is placed in tx_info->driver_data. It must not be larger
+ *  than IEEE80211_TX_INFO_DRIVER_DATA_SIZE.
+ */
+struct ar5523_tx_data {
+	struct list_head	list;
+	struct ar5523		*ar;
+	struct sk_buff		*skb;
+	struct urb		*urb;
+};
+
+struct ar5523_rx_data {
+	struct	list_head	list;
+	struct ar5523		*ar;
+	struct urb		*urb;
+	struct sk_buff		*skb;
+};
+
+struct ar5523 {
+	struct usb_device	*dev;
+	struct ieee80211_hw	*hw;
+
+	unsigned long		flags;
+	struct mutex		mutex;
+	struct workqueue_struct *wq;
+
+	struct ar5523_tx_cmd	tx_cmd;
+
+	struct delayed_work	stat_work;
+
+	struct timer_list	tx_wd_timer;
+	struct work_struct	tx_wd_work;
+	struct work_struct	tx_work;
+	struct list_head	tx_queue_pending;
+	struct list_head	tx_queue_submitted;
+	spinlock_t		tx_data_list_lock;
+	wait_queue_head_t	tx_flush_waitq;
+
+	/* Queued + Submitted TX frames */
+	atomic_t		tx_nr_total;
+
+	/* Submitted TX frames */
+	atomic_t		tx_nr_pending;
+
+	void			*rx_cmd_buf;
+	struct urb		*rx_cmd_urb;
+
+	struct ar5523_rx_data	rx_data[AR5523_RX_DATA_COUNT];
+	spinlock_t		rx_data_list_lock;
+	struct list_head	rx_data_free;
+	struct list_head	rx_data_used;
+	atomic_t		rx_data_free_cnt;
+
+	struct work_struct	rx_refill_work;
+
+	int			rxbufsz;
+	u8			serial[16];
+
+	struct ieee80211_channel channels[14];
+	struct ieee80211_rate	rates[12];
+	struct ieee80211_supported_band band;
+	struct ieee80211_vif	*vif;
+};
+
+/* flags for sending firmware commands */
+#define AR5523_CMD_FLAG_READ	(1 << 1)
+#define AR5523_CMD_FLAG_MAGIC	(1 << 2)
+
+#define ar5523_dbg(ar, format, arg...) \
+	dev_dbg(&(ar)->dev->dev, format, ## arg)
+#define ar5523_err(ar, format, arg...) \
+do { \
+	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
+		dev_err(&(ar)->dev->dev, format, ## arg); \
+	} \
+} while (0)
+#define ar5523_info(ar, format, arg...)	\
+	dev_info(&(ar)->dev->dev, format, ## arg)
+