linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] libertas_tf: fix setting the hardware address
@ 2019-02-10 19:52 Lubomir Rintel
  2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Lubomir Rintel @ 2019-02-10 19:52 UTC (permalink / raw)
  To: Kalle Valo; +Cc: David S. Miller, linux-wireless, linux-kernel

Hi,

this short patchset addresses a problem with libertas_tf driver, where
the driver registers the interface without having the MAC address set,
only setting it when the interface is brought up. That is generally too
late, and it confuses NetworkManager.

Some restructuring is needed first, done in first two patches.

Tested on an OLPC XO laptop.

Cheers,
Lubo



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure
  2019-02-10 19:52 [PATCH 0/3] libertas_tf: fix setting the hardware address Lubomir Rintel
@ 2019-02-10 19:52 ` Lubomir Rintel
  2019-02-11 13:47   ` Steve deRosier
  2019-02-19 15:13   ` Kalle Valo
  2019-02-10 19:52 ` [PATCH 2/3] libertas_tf: don't defer firmware loading until start() Lubomir Rintel
  2019-02-10 19:52 ` [PATCH 3/3] libertas_tf: get the MAC address before registering the device Lubomir Rintel
  2 siblings, 2 replies; 8+ messages in thread
From: Lubomir Rintel @ 2019-02-10 19:52 UTC (permalink / raw)
  To: Kalle Valo; +Cc: David S. Miller, linux-wireless, linux-kernel, Lubomir Rintel

We'll need to talk to the firmware to get a hardware address before
device is registered with ieee80211 subsystem at the end of
lbtf_add_card(). Hooking the callbacks after that is too late.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/net/wireless/marvell/libertas_tf/cmd.c  |  2 +-
 .../net/wireless/marvell/libertas_tf/if_usb.c   | 12 +++++++-----
 .../wireless/marvell/libertas_tf/libertas_tf.h  | 17 +++++++++++------
 drivers/net/wireless/marvell/libertas_tf/main.c | 10 ++++++----
 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas_tf/cmd.c b/drivers/net/wireless/marvell/libertas_tf/cmd.c
index 64b147dd2432..130f578daafd 100644
--- a/drivers/net/wireless/marvell/libertas_tf/cmd.c
+++ b/drivers/net/wireless/marvell/libertas_tf/cmd.c
@@ -256,7 +256,7 @@ static void lbtf_submit_command(struct lbtf_private *priv,
 		     command, le16_to_cpu(cmd->seqnum), cmdsize);
 	lbtf_deb_hex(LBTF_DEB_CMD, "DNLD_CMD", (void *) cmdnode->cmdbuf, cmdsize);
 
-	ret = priv->hw_host_to_card(priv, MVMS_CMD, (u8 *) cmd, cmdsize);
+	ret = priv->ops->hw_host_to_card(priv, MVMS_CMD, (u8 *)cmd, cmdsize);
 	spin_unlock_irqrestore(&priv->driver_lock, flags);
 
 	if (ret) {
diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
index 6ede6168bd85..7a5a1a85dcf7 100644
--- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
@@ -131,6 +131,12 @@ static void if_usb_fw_timeo(struct timer_list *t)
 	lbtf_deb_leave(LBTF_DEB_USB);
 }
 
+static const struct lbtf_ops if_usb_ops = {
+	.hw_host_to_card = if_usb_host_to_card,
+	.hw_prog_firmware = if_usb_prog_firmware,
+	.hw_reset_device = if_usb_reset_device,
+};
+
 /**
  *  if_usb_probe - sets the configuration values
  *
@@ -216,15 +222,11 @@ static int if_usb_probe(struct usb_interface *intf,
 		goto dealloc;
 	}
 
-	priv = lbtf_add_card(cardp, &udev->dev);
+	priv = lbtf_add_card(cardp, &udev->dev, &if_usb_ops);
 	if (!priv)
 		goto dealloc;
 
 	cardp->priv = priv;
-
-	priv->hw_host_to_card = if_usb_host_to_card;
-	priv->hw_prog_firmware = if_usb_prog_firmware;
-	priv->hw_reset_device = if_usb_reset_device;
 	cardp->boot2_version = udev->descriptor.bcdDevice;
 
 	usb_get_dev(udev);
diff --git a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
index ad77b92d0b41..11d5ff68bc5e 100644
--- a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
+++ b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
@@ -173,10 +173,19 @@ struct channel_range {
 
 struct if_usb_card;
 
+struct lbtf_ops {
+	/** Hardware access */
+	int (*hw_host_to_card)(struct lbtf_private *priv, u8 type,
+			       u8 *payload, u16 nb);
+	int (*hw_prog_firmware)(struct if_usb_card *cardp);
+	int (*hw_reset_device)(struct if_usb_card *cardp);
+};
+
 /** Private structure for the MV device */
 struct lbtf_private {
 	void *card;
 	struct ieee80211_hw *hw;
+	const struct lbtf_ops *ops;
 
 	/* Command response buffer */
 	u8 cmd_resp_buff[LBS_UPLD_SIZE];
@@ -188,11 +197,6 @@ struct lbtf_private {
 
 	struct work_struct cmd_work;
 	struct work_struct tx_work;
-	/** Hardware access */
-	int (*hw_host_to_card) (struct lbtf_private *priv, u8 type, u8 *payload, u16 nb);
-	int (*hw_prog_firmware) (struct if_usb_card *cardp);
-	int (*hw_reset_device) (struct if_usb_card *cardp);
-
 
 	/** Wlan adapter data structure*/
 	/** STATUS variables */
@@ -486,7 +490,8 @@ void lbtf_cmd_response_rx(struct lbtf_private *priv);
 /* main.c */
 struct chan_freq_power *lbtf_get_region_cfp_table(u8 region,
 	int *cfp_no);
-struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev);
+struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
+				   const struct lbtf_ops *ops);
 int lbtf_remove_card(struct lbtf_private *priv);
 int lbtf_start_card(struct lbtf_private *priv);
 int lbtf_rx(struct lbtf_private *priv, struct sk_buff *skb);
diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
index f93b400db949..8ed3cd158cf5 100644
--- a/drivers/net/wireless/marvell/libertas_tf/main.c
+++ b/drivers/net/wireless/marvell/libertas_tf/main.c
@@ -281,7 +281,7 @@ static void lbtf_tx_work(struct work_struct *work)
 	BUG_ON(priv->tx_skb);
 	spin_lock_irq(&priv->driver_lock);
 	priv->tx_skb = skb;
-	err = priv->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
+	err = priv->ops->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
 	spin_unlock_irq(&priv->driver_lock);
 	if (err) {
 		dev_kfree_skb_any(skb);
@@ -301,7 +301,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
 
 	if (!priv->fw_ready)
 		/* Upload firmware */
-		if (priv->hw_prog_firmware(card))
+		if (priv->ops->hw_prog_firmware(card))
 			goto err_prog_firmware;
 
 	/* poke the firmware */
@@ -322,7 +322,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
 	return 0;
 
 err_prog_firmware:
-	priv->hw_reset_device(card);
+	priv->ops->hw_reset_device(card);
 	lbtf_deb_leave_args(LBTF_DEB_MACOPS, "error programming fw; ret=%d", ret);
 	return ret;
 }
@@ -605,7 +605,8 @@ EXPORT_SYMBOL_GPL(lbtf_rx);
  *
  *  Returns: pointer to struct lbtf_priv.
  */
-struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev)
+struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
+				   const struct lbtf_ops *ops)
 {
 	struct ieee80211_hw *hw;
 	struct lbtf_private *priv = NULL;
@@ -622,6 +623,7 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev)
 
 	priv->hw = hw;
 	priv->card = card;
+	priv->ops = ops;
 	priv->tx_skb = NULL;
 
 	hw->queues = 1;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] libertas_tf: don't defer firmware loading until start()
  2019-02-10 19:52 [PATCH 0/3] libertas_tf: fix setting the hardware address Lubomir Rintel
  2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
@ 2019-02-10 19:52 ` Lubomir Rintel
  2019-02-11 14:10   ` Steve deRosier
  2019-02-10 19:52 ` [PATCH 3/3] libertas_tf: get the MAC address before registering the device Lubomir Rintel
  2 siblings, 1 reply; 8+ messages in thread
From: Lubomir Rintel @ 2019-02-10 19:52 UTC (permalink / raw)
  To: Kalle Valo; +Cc: David S. Miller, linux-wireless, linux-kernel, Lubomir Rintel

In order to be able to get a MAC address before we register the device
with ieee80211 we'll need to load the firmware way earlier.

There seems to be one problem with this: the device seems to start
with radio enabled and starts sending in frames right after the firmware
load finishes. This might be a firmware bug. Disable the radio as soon
as possible.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 .../net/wireless/marvell/libertas_tf/if_usb.c | 24 +++++------
 .../marvell/libertas_tf/libertas_tf.h         |  5 +--
 .../net/wireless/marvell/libertas_tf/main.c   | 41 +++++++++++--------
 3 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
index 7a5a1a85dcf7..a4b9ede70705 100644
--- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
@@ -42,14 +42,14 @@ MODULE_DEVICE_TABLE(usb, if_usb_table);
 
 static void if_usb_receive(struct urb *urb);
 static void if_usb_receive_fwload(struct urb *urb);
-static int if_usb_prog_firmware(struct if_usb_card *cardp);
+static int if_usb_prog_firmware(struct lbtf_private *priv);
 static int if_usb_host_to_card(struct lbtf_private *priv, uint8_t type,
 			       uint8_t *payload, uint16_t nb);
 static int usb_tx_block(struct if_usb_card *cardp, uint8_t *payload,
 			uint16_t nb, u8 data);
 static void if_usb_free(struct if_usb_card *cardp);
 static int if_usb_submit_rx_urb(struct if_usb_card *cardp);
-static int if_usb_reset_device(struct if_usb_card *cardp);
+static int if_usb_reset_device(struct lbtf_private *priv);
 
 /**
  *  if_usb_wrike_bulk_callback -  call back to handle URB status
@@ -222,13 +222,11 @@ static int if_usb_probe(struct usb_interface *intf,
 		goto dealloc;
 	}
 
+	cardp->boot2_version = udev->descriptor.bcdDevice;
 	priv = lbtf_add_card(cardp, &udev->dev, &if_usb_ops);
 	if (!priv)
 		goto dealloc;
 
-	cardp->priv = priv;
-	cardp->boot2_version = udev->descriptor.bcdDevice;
-
 	usb_get_dev(udev);
 	usb_set_intfdata(intf, cardp);
 
@@ -253,7 +251,7 @@ static void if_usb_disconnect(struct usb_interface *intf)
 
 	lbtf_deb_enter(LBTF_DEB_MAIN);
 
-	if_usb_reset_device(cardp);
+	if_usb_reset_device(priv);
 
 	if (priv)
 		lbtf_remove_card(priv);
@@ -336,8 +334,9 @@ static int if_usb_send_fw_pkt(struct if_usb_card *cardp)
 	return 0;
 }
 
-static int if_usb_reset_device(struct if_usb_card *cardp)
+static int if_usb_reset_device(struct lbtf_private *priv)
 {
+	struct if_usb_card *cardp = priv->card;
 	struct cmd_ds_802_11_reset *cmd = cardp->ep_out_buf + 4;
 	int ret;
 
@@ -808,14 +807,17 @@ static int check_fwfile_format(const u8 *data, u32 totlen)
 }
 
 
-static int if_usb_prog_firmware(struct if_usb_card *cardp)
+static int if_usb_prog_firmware(struct lbtf_private *priv)
 {
+	struct if_usb_card *cardp = priv->card;
 	int i = 0;
 	static int reset_count = 10;
 	int ret = 0;
 
 	lbtf_deb_enter(LBTF_DEB_USB);
 
+	cardp->priv = priv;
+
 	kernel_param_lock(THIS_MODULE);
 	ret = request_firmware(&cardp->fw, lbtf_fw_name, &cardp->udev->dev);
 	if (ret < 0) {
@@ -851,7 +853,7 @@ static int if_usb_prog_firmware(struct if_usb_card *cardp)
 
 	if (cardp->bootcmdresp <= 0) {
 		if (--reset_count >= 0) {
-			if_usb_reset_device(cardp);
+			if_usb_reset_device(priv);
 			goto restart;
 		}
 		return -1;
@@ -880,7 +882,7 @@ static int if_usb_prog_firmware(struct if_usb_card *cardp)
 	if (!cardp->fwdnldover) {
 		pr_info("failed to load fw, resetting device!\n");
 		if (--reset_count >= 0) {
-			if_usb_reset_device(cardp);
+			if_usb_reset_device(priv);
 			goto restart;
 		}
 
@@ -889,8 +891,6 @@ static int if_usb_prog_firmware(struct if_usb_card *cardp)
 		goto release_fw;
 	}
 
-	cardp->priv->fw_ready = 1;
-
  release_fw:
 	release_firmware(cardp->fw);
 	cardp->fw = NULL;
diff --git a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
index 11d5ff68bc5e..3ed1fbe28798 100644
--- a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
+++ b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
@@ -177,8 +177,8 @@ struct lbtf_ops {
 	/** Hardware access */
 	int (*hw_host_to_card)(struct lbtf_private *priv, u8 type,
 			       u8 *payload, u16 nb);
-	int (*hw_prog_firmware)(struct if_usb_card *cardp);
-	int (*hw_reset_device)(struct if_usb_card *cardp);
+	int (*hw_prog_firmware)(struct lbtf_private *priv);
+	int (*hw_reset_device)(struct lbtf_private *priv);
 };
 
 /** Private structure for the MV device */
@@ -254,7 +254,6 @@ struct lbtf_private {
 	struct ieee80211_supported_band band;
 	struct lbtf_offset_value offsetvalue;
 
-	u8 fw_ready;
 	u8 surpriseremoved;
 	struct sk_buff_head bc_ps_buf;
 
diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
index 8ed3cd158cf5..b4bd3047eb4e 100644
--- a/drivers/net/wireless/marvell/libertas_tf/main.c
+++ b/drivers/net/wireless/marvell/libertas_tf/main.c
@@ -118,11 +118,6 @@ static void lbtf_cmd_work(struct work_struct *work)
 	priv->cmd_timed_out = 0;
 	spin_unlock_irq(&priv->driver_lock);
 
-	if (!priv->fw_ready) {
-		lbtf_deb_leave_args(LBTF_DEB_CMD, "fw not ready");
-		return;
-	}
-
 	/* Execute the next command */
 	if (!priv->cur_cmd)
 		lbtf_execute_next_command(priv);
@@ -294,36 +289,29 @@ static void lbtf_tx_work(struct work_struct *work)
 static int lbtf_op_start(struct ieee80211_hw *hw)
 {
 	struct lbtf_private *priv = hw->priv;
-	void *card = priv->card;
 	int ret = -1;
 
 	lbtf_deb_enter(LBTF_DEB_MACOPS);
 
-	if (!priv->fw_ready)
-		/* Upload firmware */
-		if (priv->ops->hw_prog_firmware(card))
-			goto err_prog_firmware;
-
 	/* poke the firmware */
 	priv->capability = WLAN_CAPABILITY_SHORT_PREAMBLE;
 	priv->radioon = RADIO_ON;
 	priv->mac_control = CMD_ACT_MAC_RX_ON | CMD_ACT_MAC_TX_ON;
 	ret = lbtf_setup_firmware(priv);
 	if (ret)
-		goto err_prog_firmware;
+		goto err_setup_firmware;
 
 	if ((priv->fwrelease < LBTF_FW_VER_MIN) ||
 	    (priv->fwrelease > LBTF_FW_VER_MAX)) {
 		ret = -1;
-		goto err_prog_firmware;
+		goto err_setup_firmware;
 	}
 
 	lbtf_deb_leave(LBTF_DEB_MACOPS);
 	return 0;
 
-err_prog_firmware:
-	priv->ops->hw_reset_device(card);
-	lbtf_deb_leave_args(LBTF_DEB_MACOPS, "error programming fw; ret=%d", ret);
+err_setup_firmware:
+	lbtf_deb_leave_args(LBTF_DEB_MACOPS, "fw setup error; ret=%d", ret);
 	return ret;
 }
 
@@ -555,6 +543,11 @@ int lbtf_rx(struct lbtf_private *priv, struct sk_buff *skb)
 
 	lbtf_deb_enter(LBTF_DEB_RX);
 
+	if (priv->radioon != RADIO_ON) {
+		lbtf_deb_rx("rx before we turned on the radio");
+		goto done;
+	}
+
 	prxpd = (struct rxpd *) skb->data;
 
 	memset(&stats, 0, sizeof(stats));
@@ -593,13 +586,14 @@ int lbtf_rx(struct lbtf_private *priv, struct sk_buff *skb)
 
 	ieee80211_rx_irqsafe(priv->hw, skb);
 
+done:
 	lbtf_deb_leave(LBTF_DEB_RX);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(lbtf_rx);
 
 /**
- * lbtf_add_card: Add and initialize the card, no fw upload yet.
+ * lbtf_add_card: Add and initialize the card.
  *
  *  @card    A pointer to card
  *
@@ -648,6 +642,19 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
 
 	INIT_WORK(&priv->cmd_work, lbtf_cmd_work);
 	INIT_WORK(&priv->tx_work, lbtf_tx_work);
+
+	if (priv->ops->hw_prog_firmware(priv)) {
+		lbtf_deb_usbd(&udev->dev, "Error programming the firmware\n");
+		priv->ops->hw_reset_device(priv);
+		goto err_init_adapter;
+	}
+
+	/* The firmware seems to start with the radio enabled. Turn it
+	 * off before an actual mac80211 start callback is invoked.
+	 */
+	priv->radioon = RADIO_OFF;
+	lbtf_set_radio_control(priv);
+
 	if (ieee80211_register_hw(hw))
 		goto err_init_adapter;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] libertas_tf: get the MAC address before registering the device
  2019-02-10 19:52 [PATCH 0/3] libertas_tf: fix setting the hardware address Lubomir Rintel
  2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
  2019-02-10 19:52 ` [PATCH 2/3] libertas_tf: don't defer firmware loading until start() Lubomir Rintel
@ 2019-02-10 19:52 ` Lubomir Rintel
  2019-02-11 14:11   ` Steve deRosier
  2 siblings, 1 reply; 8+ messages in thread
From: Lubomir Rintel @ 2019-02-10 19:52 UTC (permalink / raw)
  To: Kalle Valo; +Cc: David S. Miller, linux-wireless, linux-kernel, Lubomir Rintel

The start() callback is too late for this: NetworkManager would already
have seen the hardware, thinking 00:00:00:00:00:00 is its permanent
address.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 .../net/wireless/marvell/libertas_tf/main.c   | 57 ++++---------------
 1 file changed, 11 insertions(+), 46 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
index b4bd3047eb4e..da53da71987e 100644
--- a/drivers/net/wireless/marvell/libertas_tf/main.c
+++ b/drivers/net/wireless/marvell/libertas_tf/main.c
@@ -125,37 +125,6 @@ static void lbtf_cmd_work(struct work_struct *work)
 	lbtf_deb_leave(LBTF_DEB_CMD);
 }
 
-/**
- *  lbtf_setup_firmware: initialize firmware.
- *
- *  @priv    A pointer to struct lbtf_private structure
- *
- *  Returns: 0 on success.
- */
-static int lbtf_setup_firmware(struct lbtf_private *priv)
-{
-	int ret = -1;
-
-	lbtf_deb_enter(LBTF_DEB_FW);
-	/*
-	 * Read priv address from HW
-	 */
-	eth_broadcast_addr(priv->current_addr);
-	ret = lbtf_update_hw_spec(priv);
-	if (ret) {
-		ret = -1;
-		goto done;
-	}
-
-	lbtf_set_mac_control(priv);
-	lbtf_set_radio_control(priv);
-
-	ret = 0;
-done:
-	lbtf_deb_leave_args(LBTF_DEB_FW, "ret: %d", ret);
-	return ret;
-}
-
 /**
  *  This function handles the timeout of command sending.
  *  It will re-send the same command again.
@@ -289,30 +258,17 @@ static void lbtf_tx_work(struct work_struct *work)
 static int lbtf_op_start(struct ieee80211_hw *hw)
 {
 	struct lbtf_private *priv = hw->priv;
-	int ret = -1;
 
 	lbtf_deb_enter(LBTF_DEB_MACOPS);
 
-	/* poke the firmware */
 	priv->capability = WLAN_CAPABILITY_SHORT_PREAMBLE;
 	priv->radioon = RADIO_ON;
 	priv->mac_control = CMD_ACT_MAC_RX_ON | CMD_ACT_MAC_TX_ON;
-	ret = lbtf_setup_firmware(priv);
-	if (ret)
-		goto err_setup_firmware;
-
-	if ((priv->fwrelease < LBTF_FW_VER_MIN) ||
-	    (priv->fwrelease > LBTF_FW_VER_MAX)) {
-		ret = -1;
-		goto err_setup_firmware;
-	}
+	lbtf_set_mac_control(priv);
+	lbtf_set_radio_control(priv);
 
 	lbtf_deb_leave(LBTF_DEB_MACOPS);
 	return 0;
-
-err_setup_firmware:
-	lbtf_deb_leave_args(LBTF_DEB_MACOPS, "fw setup error; ret=%d", ret);
-	return ret;
 }
 
 static void lbtf_op_stop(struct ieee80211_hw *hw)
@@ -649,6 +605,15 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
 		goto err_init_adapter;
 	}
 
+	eth_broadcast_addr(priv->current_addr);
+	if (lbtf_update_hw_spec(priv))
+		goto err_init_adapter;
+
+	if (priv->fwrelease < LBTF_FW_VER_MIN ||
+	    priv->fwrelease > LBTF_FW_VER_MAX) {
+		goto err_init_adapter;
+	}
+
 	/* The firmware seems to start with the radio enabled. Turn it
 	 * off before an actual mac80211 start callback is invoked.
 	 */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure
  2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
@ 2019-02-11 13:47   ` Steve deRosier
  2019-02-19 15:13   ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Steve deRosier @ 2019-02-11 13:47 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: Kalle Valo, David S. Miller, linux-wireless, LKML

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> We'll need to talk to the firmware to get a hardware address before
> device is registered with ieee80211 subsystem at the end of
> lbtf_add_card(). Hooking the callbacks after that is too late.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/net/wireless/marvell/libertas_tf/cmd.c  |  2 +-
>  .../net/wireless/marvell/libertas_tf/if_usb.c   | 12 +++++++-----
>  .../wireless/marvell/libertas_tf/libertas_tf.h  | 17 +++++++++++------
>  drivers/net/wireless/marvell/libertas_tf/main.c | 10 ++++++----
>  4 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/cmd.c b/drivers/net/wireless/marvell/libertas_tf/cmd.c
> index 64b147dd2432..130f578daafd 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/cmd.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/cmd.c
> @@ -256,7 +256,7 @@ static void lbtf_submit_command(struct lbtf_private *priv,
>                      command, le16_to_cpu(cmd->seqnum), cmdsize);
>         lbtf_deb_hex(LBTF_DEB_CMD, "DNLD_CMD", (void *) cmdnode->cmdbuf, cmdsize);
>
> -       ret = priv->hw_host_to_card(priv, MVMS_CMD, (u8 *) cmd, cmdsize);
> +       ret = priv->ops->hw_host_to_card(priv, MVMS_CMD, (u8 *)cmd, cmdsize);
>         spin_unlock_irqrestore(&priv->driver_lock, flags);
>
>         if (ret) {
> diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> index 6ede6168bd85..7a5a1a85dcf7 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> @@ -131,6 +131,12 @@ static void if_usb_fw_timeo(struct timer_list *t)
>         lbtf_deb_leave(LBTF_DEB_USB);
>  }
>
> +static const struct lbtf_ops if_usb_ops = {
> +       .hw_host_to_card = if_usb_host_to_card,
> +       .hw_prog_firmware = if_usb_prog_firmware,
> +       .hw_reset_device = if_usb_reset_device,
> +};
> +
>  /**
>   *  if_usb_probe - sets the configuration values
>   *
> @@ -216,15 +222,11 @@ static int if_usb_probe(struct usb_interface *intf,
>                 goto dealloc;
>         }
>
> -       priv = lbtf_add_card(cardp, &udev->dev);
> +       priv = lbtf_add_card(cardp, &udev->dev, &if_usb_ops);
>         if (!priv)
>                 goto dealloc;
>
>         cardp->priv = priv;
> -
> -       priv->hw_host_to_card = if_usb_host_to_card;
> -       priv->hw_prog_firmware = if_usb_prog_firmware;
> -       priv->hw_reset_device = if_usb_reset_device;
>         cardp->boot2_version = udev->descriptor.bcdDevice;
>
>         usb_get_dev(udev);
> diff --git a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
> index ad77b92d0b41..11d5ff68bc5e 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
> +++ b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
> @@ -173,10 +173,19 @@ struct channel_range {
>
>  struct if_usb_card;
>
> +struct lbtf_ops {
> +       /** Hardware access */
> +       int (*hw_host_to_card)(struct lbtf_private *priv, u8 type,
> +                              u8 *payload, u16 nb);
> +       int (*hw_prog_firmware)(struct if_usb_card *cardp);
> +       int (*hw_reset_device)(struct if_usb_card *cardp);
> +};
> +
>  /** Private structure for the MV device */
>  struct lbtf_private {
>         void *card;
>         struct ieee80211_hw *hw;
> +       const struct lbtf_ops *ops;
>
>         /* Command response buffer */
>         u8 cmd_resp_buff[LBS_UPLD_SIZE];
> @@ -188,11 +197,6 @@ struct lbtf_private {
>
>         struct work_struct cmd_work;
>         struct work_struct tx_work;
> -       /** Hardware access */
> -       int (*hw_host_to_card) (struct lbtf_private *priv, u8 type, u8 *payload, u16 nb);
> -       int (*hw_prog_firmware) (struct if_usb_card *cardp);
> -       int (*hw_reset_device) (struct if_usb_card *cardp);
> -
>
>         /** Wlan adapter data structure*/
>         /** STATUS variables */
> @@ -486,7 +490,8 @@ void lbtf_cmd_response_rx(struct lbtf_private *priv);
>  /* main.c */
>  struct chan_freq_power *lbtf_get_region_cfp_table(u8 region,
>         int *cfp_no);
> -struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev);
> +struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
> +                                  const struct lbtf_ops *ops);
>  int lbtf_remove_card(struct lbtf_private *priv);
>  int lbtf_start_card(struct lbtf_private *priv);
>  int lbtf_rx(struct lbtf_private *priv, struct sk_buff *skb);
> diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
> index f93b400db949..8ed3cd158cf5 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -281,7 +281,7 @@ static void lbtf_tx_work(struct work_struct *work)
>         BUG_ON(priv->tx_skb);
>         spin_lock_irq(&priv->driver_lock);
>         priv->tx_skb = skb;
> -       err = priv->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
> +       err = priv->ops->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
>         spin_unlock_irq(&priv->driver_lock);
>         if (err) {
>                 dev_kfree_skb_any(skb);
> @@ -301,7 +301,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
>
>         if (!priv->fw_ready)
>                 /* Upload firmware */
> -               if (priv->hw_prog_firmware(card))
> +               if (priv->ops->hw_prog_firmware(card))
>                         goto err_prog_firmware;
>
>         /* poke the firmware */
> @@ -322,7 +322,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
>         return 0;
>
>  err_prog_firmware:
> -       priv->hw_reset_device(card);
> +       priv->ops->hw_reset_device(card);
>         lbtf_deb_leave_args(LBTF_DEB_MACOPS, "error programming fw; ret=%d", ret);
>         return ret;
>  }
> @@ -605,7 +605,8 @@ EXPORT_SYMBOL_GPL(lbtf_rx);
>   *
>   *  Returns: pointer to struct lbtf_priv.
>   */
> -struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev)
> +struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
> +                                  const struct lbtf_ops *ops)
>  {
>         struct ieee80211_hw *hw;
>         struct lbtf_private *priv = NULL;
> @@ -622,6 +623,7 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev)
>
>         priv->hw = hw;
>         priv->card = card;
> +       priv->ops = ops;
>         priv->tx_skb = NULL;
>
>         hw->queues = 1;
> --
> 2.20.1
>

Reviewed-by: Steve deRosier <derosier@cal-sierra.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] libertas_tf: don't defer firmware loading until start()
  2019-02-10 19:52 ` [PATCH 2/3] libertas_tf: don't defer firmware loading until start() Lubomir Rintel
@ 2019-02-11 14:10   ` Steve deRosier
  0 siblings, 0 replies; 8+ messages in thread
From: Steve deRosier @ 2019-02-11 14:10 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: Kalle Valo, David S. Miller, linux-wireless, LKML

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> In order to be able to get a MAC address before we register the device
> with ieee80211 we'll need to load the firmware way earlier.
>
> There seems to be one problem with this: the device seems to start
> with radio enabled and starts sending in frames right after the firmware
> load finishes. This might be a firmware bug. Disable the radio as soon
> as possible.
>

I've got a quibble with calling the behavior a bug. As far as I
remember, it's behaving as designed/specified and was quite
appropriate back when this driver originally went upstream. This is a
10 year old fw and driver and needs have changed - which doesn't mean
it was wrong to do originally.

Now, I'm not saying there's no bugs in the fw. Nor that it wouldn't be
nice to change the behavior to accommodate this change. If I still had
access to the firmware source I even might do so.

...
> @@ -648,6 +642,19 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
>
>         INIT_WORK(&priv->cmd_work, lbtf_cmd_work);
>         INIT_WORK(&priv->tx_work, lbtf_tx_work);
> +
> +       if (priv->ops->hw_prog_firmware(priv)) {
> +               lbtf_deb_usbd(&udev->dev, "Error programming the firmware\n");
> +               priv->ops->hw_reset_device(priv);
> +               goto err_init_adapter;
> +       }
> +
> +       /* The firmware seems to start with the radio enabled. Turn it
> +        * off before an actual mac80211 start callback is invoked.
> +        */
> +       priv->radioon = RADIO_OFF;

Maybe move this up a few lines to before you program the fw? Seems
appropriate to initialize it first. While I expect there's no chance
of a race as the
mac80211 start callback hasn't been invoked yet, superficially it
looks like there could be.


> +       lbtf_set_radio_control(priv);
> +
>         if (ieee80211_register_hw(hw))
>                 goto err_init_adapter;
>
> --
> 2.20.1
>

Reviewed-by: Steve deRosier <derosier@cal-sierra.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] libertas_tf: get the MAC address before registering the device
  2019-02-10 19:52 ` [PATCH 3/3] libertas_tf: get the MAC address before registering the device Lubomir Rintel
@ 2019-02-11 14:11   ` Steve deRosier
  0 siblings, 0 replies; 8+ messages in thread
From: Steve deRosier @ 2019-02-11 14:11 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: Kalle Valo, David S. Miller, linux-wireless, LKML

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The start() callback is too late for this: NetworkManager would already
> have seen the hardware, thinking 00:00:00:00:00:00 is its permanent
> address.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  .../net/wireless/marvell/libertas_tf/main.c   | 57 ++++---------------
>  1 file changed, 11 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
> index b4bd3047eb4e..da53da71987e 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -125,37 +125,6 @@ static void lbtf_cmd_work(struct work_struct *work)
>         lbtf_deb_leave(LBTF_DEB_CMD);
>  }
>
> -/**
> - *  lbtf_setup_firmware: initialize firmware.
> - *
> - *  @priv    A pointer to struct lbtf_private structure
> - *
> - *  Returns: 0 on success.
> - */
> -static int lbtf_setup_firmware(struct lbtf_private *priv)
> -{
> -       int ret = -1;
> -
> -       lbtf_deb_enter(LBTF_DEB_FW);
> -       /*
> -        * Read priv address from HW
> -        */
> -       eth_broadcast_addr(priv->current_addr);
> -       ret = lbtf_update_hw_spec(priv);
> -       if (ret) {
> -               ret = -1;
> -               goto done;
> -       }
> -
> -       lbtf_set_mac_control(priv);
> -       lbtf_set_radio_control(priv);
> -
> -       ret = 0;
> -done:
> -       lbtf_deb_leave_args(LBTF_DEB_FW, "ret: %d", ret);
> -       return ret;
> -}
> -
>  /**
>   *  This function handles the timeout of command sending.
>   *  It will re-send the same command again.
> @@ -289,30 +258,17 @@ static void lbtf_tx_work(struct work_struct *work)
>  static int lbtf_op_start(struct ieee80211_hw *hw)
>  {
>         struct lbtf_private *priv = hw->priv;
> -       int ret = -1;
>
>         lbtf_deb_enter(LBTF_DEB_MACOPS);
>
> -       /* poke the firmware */
>         priv->capability = WLAN_CAPABILITY_SHORT_PREAMBLE;
>         priv->radioon = RADIO_ON;
>         priv->mac_control = CMD_ACT_MAC_RX_ON | CMD_ACT_MAC_TX_ON;
> -       ret = lbtf_setup_firmware(priv);
> -       if (ret)
> -               goto err_setup_firmware;
> -
> -       if ((priv->fwrelease < LBTF_FW_VER_MIN) ||
> -           (priv->fwrelease > LBTF_FW_VER_MAX)) {
> -               ret = -1;
> -               goto err_setup_firmware;
> -       }
> +       lbtf_set_mac_control(priv);
> +       lbtf_set_radio_control(priv);
>
>         lbtf_deb_leave(LBTF_DEB_MACOPS);
>         return 0;
> -
> -err_setup_firmware:
> -       lbtf_deb_leave_args(LBTF_DEB_MACOPS, "fw setup error; ret=%d", ret);
> -       return ret;
>  }
>
>  static void lbtf_op_stop(struct ieee80211_hw *hw)
> @@ -649,6 +605,15 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
>                 goto err_init_adapter;
>         }
>
> +       eth_broadcast_addr(priv->current_addr);
> +       if (lbtf_update_hw_spec(priv))
> +               goto err_init_adapter;
> +
> +       if (priv->fwrelease < LBTF_FW_VER_MIN ||
> +           priv->fwrelease > LBTF_FW_VER_MAX) {
> +               goto err_init_adapter;
> +       }
> +
>         /* The firmware seems to start with the radio enabled. Turn it
>          * off before an actual mac80211 start callback is invoked.
>          */
> --
> 2.20.1
>

Reviewed-by: Steve deRosier <derosier@cal-sierra.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure
  2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
  2019-02-11 13:47   ` Steve deRosier
@ 2019-02-19 15:13   ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2019-02-19 15:13 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: David S. Miller, linux-wireless, linux-kernel, Lubomir Rintel

Lubomir Rintel <lkundrak@v3.sk> wrote:

> We'll need to talk to the firmware to get a hardware address before
> device is registered with ieee80211 subsystem at the end of
> lbtf_add_card(). Hooking the callbacks after that is too late.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Steve deRosier <derosier@cal-sierra.com>

Failed to compile:

drivers/net/wireless/marvell/libertas_tf/main.c: In function 'lbtf_add_card':
drivers/net/wireless/marvell/libertas_tf/main.c:603:309: error: 'udev' undeclared (first use in this function); did you mean 'cdev'?
   lbtf_deb_usbd(&udev->dev, "Error programming the firmware\n");
                                                                                                                                                                                                                                                                                                                     ^   
                                                                                                                                                                                                                                                                                                                     cdev
drivers/net/wireless/marvell/libertas_tf/main.c:603:309: note: each undeclared identifier is reported only once for each function it appears in
make[5]: *** [drivers/net/wireless/marvell/libertas_tf/main.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [drivers/net/wireless/marvell/libertas_tf] Error 2
make[3]: *** [drivers/net/wireless/marvell] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [drivers/net/wireless] Error 2
make[1]: *** [drivers/net] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

3 patches set to Changes Requested.

10804885 [1/3] libertas_tf: move hardware callbacks to a separate structure
10804881 [2/3] libertas_tf: don't defer firmware loading until start()
10804883 [3/3] libertas_tf: get the MAC address before registering the device

-- 
https://patchwork.kernel.org/patch/10804885/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-02-19 15:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-10 19:52 [PATCH 0/3] libertas_tf: fix setting the hardware address Lubomir Rintel
2019-02-10 19:52 ` [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure Lubomir Rintel
2019-02-11 13:47   ` Steve deRosier
2019-02-19 15:13   ` Kalle Valo
2019-02-10 19:52 ` [PATCH 2/3] libertas_tf: don't defer firmware loading until start() Lubomir Rintel
2019-02-11 14:10   ` Steve deRosier
2019-02-10 19:52 ` [PATCH 3/3] libertas_tf: get the MAC address before registering the device Lubomir Rintel
2019-02-11 14:11   ` Steve deRosier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).