| Message ID | 1479141222-8493-4-git-send-email-erik.stromdahl@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: > This patch changes the order in which the driver waits for the > target to become ready and the service connect of the HTC > control service. > > The HTC control service is connected before the driver starts > waiting for the HTC ready control message. > > The reason for this is that the HTC ready control message is > transmitted on EP 0 and that sdio/mbox based systems will ignore > messages received on unconnected endpoints. > > Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> > --- > drivers/net/wireless/ath/ath10k/htc.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c > index e3f7bf4..7257366 100644 > --- a/drivers/net/wireless/ath/ath10k/htc.c > +++ b/drivers/net/wireless/ath/ath10k/htc.c > @@ -606,6 +606,22 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc) > u16 credit_count; > u16 credit_size; > > + /* setup our pseudo HTC control endpoint connection */ > + memset(&conn_req, 0, sizeof(conn_req)); > + memset(&conn_resp, 0, sizeof(conn_resp)); > + conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete; > + conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete; > + conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS; > + conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL; > + > + /* connect fake service */ > + status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp); > + if (status) { > + ath10k_err(ar, "could not connect to htc service (%d)\n", > + status); > + return status; > + } > + How is this supposed to work? ath10k_htc_connect_service() requires htc->target_credit_size to compute tx_credits_per_max_message. Or am I missing something? Applying this patch alone results in: [ 6.680101] divide error: 0000 [#1] SMP [ 6.681342] Modules linked in: ath10k_pci(O) ath10k_core(O) ath mac80211 cfg80211 [ 6.684876] CPU: 3 PID: 823 Comm: kworker/u8:2 Tainted: G W O 4.9.0-rc4-wt-ath+ #79 [ 6.688051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 [ 6.691644] Workqueue: ath10k_wq ath10k_core_register_work [ath10k_core] [ 6.694309] task: ffff88000a190000 task.stack: ffffc900006d4000 [ 6.695458] RIP: 0010:[<ffffffffa01ae46b>] [<ffffffffa01ae46b>] ath10k_htc_connect_service+0x21b/0x420 [ath10k_core] Michał
On 11/15/2016 11:13 AM, Michal Kazior wrote: > On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: >> This patch changes the order in which the driver waits for the >> target to become ready and the service connect of the HTC >> control service. >> >> The HTC control service is connected before the driver starts >> waiting for the HTC ready control message. >> >> The reason for this is that the HTC ready control message is >> transmitted on EP 0 and that sdio/mbox based systems will ignore >> messages received on unconnected endpoints. >> >> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> >> --- >> drivers/net/wireless/ath/ath10k/htc.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c >> index e3f7bf4..7257366 100644 >> --- a/drivers/net/wireless/ath/ath10k/htc.c >> +++ b/drivers/net/wireless/ath/ath10k/htc.c >> @@ -606,6 +606,22 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc) >> u16 credit_count; >> u16 credit_size; >> >> + /* setup our pseudo HTC control endpoint connection */ >> + memset(&conn_req, 0, sizeof(conn_req)); >> + memset(&conn_resp, 0, sizeof(conn_resp)); >> + conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete; >> + conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete; >> + conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS; >> + conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL; >> + >> + /* connect fake service */ >> + status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp); >> + if (status) { >> + ath10k_err(ar, "could not connect to htc service (%d)\n", >> + status); >> + return status; >> + } >> + > > How is this supposed to work? ath10k_htc_connect_service() requires > htc->target_credit_size to compute tx_credits_per_max_message. Or am I > missing something? Applying this patch alone results in: > > [ 6.680101] divide error: 0000 [#1] SMP > [ 6.681342] Modules linked in: ath10k_pci(O) ath10k_core(O) ath > mac80211 cfg80211 > [ 6.684876] CPU: 3 PID: 823 Comm: kworker/u8:2 Tainted: G W > O 4.9.0-rc4-wt-ath+ #79 > [ 6.688051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 > [ 6.691644] Workqueue: ath10k_wq ath10k_core_register_work [ath10k_core] > [ 6.694309] task: ffff88000a190000 task.stack: ffffc900006d4000 > [ 6.695458] RIP: 0010:[<ffffffffa01ae46b>] [<ffffffffa01ae46b>] > ath10k_htc_connect_service+0x21b/0x420 [ath10k_core] > > > Michał > You're right. I have totally missed this. What is strange is that my compiler (ARM linaro) seems to optimize the code in a way that removes the tx_credits_per_max_message value. If I add a printk in ath10k_htc_connect_service (printing the value) I get a similar oops. I think it has to do with the fact the this value isn't really used at all. grepping the code reveals that tx_credits_per_max_message is only used inside ath10k_htc_connect_service (only written, never read). Removing it doesn't seem to break anything, so perhaps it should be removed? Or is there something I have missed? /Erik
On 15 November 2016 at 18:07, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: > On 11/15/2016 11:13 AM, Michal Kazior wrote: >> On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> wrote: >>> This patch changes the order in which the driver waits for the >>> target to become ready and the service connect of the HTC >>> control service. >>> >>> The HTC control service is connected before the driver starts >>> waiting for the HTC ready control message. >>> >>> The reason for this is that the HTC ready control message is >>> transmitted on EP 0 and that sdio/mbox based systems will ignore >>> messages received on unconnected endpoints. >>> >>> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> >>> --- >>> drivers/net/wireless/ath/ath10k/htc.c | 32 ++++++++++++++++---------------- >>> 1 file changed, 16 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c >>> index e3f7bf4..7257366 100644 >>> --- a/drivers/net/wireless/ath/ath10k/htc.c >>> +++ b/drivers/net/wireless/ath/ath10k/htc.c >>> @@ -606,6 +606,22 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc) >>> u16 credit_count; >>> u16 credit_size; >>> >>> + /* setup our pseudo HTC control endpoint connection */ >>> + memset(&conn_req, 0, sizeof(conn_req)); >>> + memset(&conn_resp, 0, sizeof(conn_resp)); >>> + conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete; >>> + conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete; >>> + conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS; >>> + conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL; >>> + >>> + /* connect fake service */ >>> + status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp); >>> + if (status) { >>> + ath10k_err(ar, "could not connect to htc service (%d)\n", >>> + status); >>> + return status; >>> + } >>> + >> >> How is this supposed to work? ath10k_htc_connect_service() requires >> htc->target_credit_size to compute tx_credits_per_max_message. Or am I >> missing something? Applying this patch alone results in: >> >> [ 6.680101] divide error: 0000 [#1] SMP >> [ 6.681342] Modules linked in: ath10k_pci(O) ath10k_core(O) ath >> mac80211 cfg80211 >> [ 6.684876] CPU: 3 PID: 823 Comm: kworker/u8:2 Tainted: G W >> O 4.9.0-rc4-wt-ath+ #79 >> [ 6.688051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), >> BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 >> [ 6.691644] Workqueue: ath10k_wq ath10k_core_register_work [ath10k_core] >> [ 6.694309] task: ffff88000a190000 task.stack: ffffc900006d4000 >> [ 6.695458] RIP: 0010:[<ffffffffa01ae46b>] [<ffffffffa01ae46b>] >> ath10k_htc_connect_service+0x21b/0x420 [ath10k_core] >> >> >> Michał >> > > You're right. I have totally missed this. What is strange is that my > compiler (ARM linaro) seems to optimize the code in a way that removes > the tx_credits_per_max_message value. > > If I add a printk in ath10k_htc_connect_service (printing the value) I > get a similar oops. > > I think it has to do with the fact the this value isn't really used at > all. grepping the code reveals that tx_credits_per_max_message is only > used inside ath10k_htc_connect_service (only written, never read). > > Removing it doesn't seem to break anything, so perhaps it should be removed? I think it's safe to remove now. This is legacy inherited from the internal driver. Otherwise this looks okay except commit log which is a bit unclear. I'm still not sure *why* you need to reorder this. FWIW this can be a general clean up thing that ends up moving the if (eid == EP0) {code} to control endpoint handler in patch 4. Michał
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c index e3f7bf4..7257366 100644 --- a/drivers/net/wireless/ath/ath10k/htc.c +++ b/drivers/net/wireless/ath/ath10k/htc.c @@ -606,6 +606,22 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc) u16 credit_count; u16 credit_size; + /* setup our pseudo HTC control endpoint connection */ + memset(&conn_req, 0, sizeof(conn_req)); + memset(&conn_resp, 0, sizeof(conn_resp)); + conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete; + conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete; + conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS; + conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL; + + /* connect fake service */ + status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp); + if (status) { + ath10k_err(ar, "could not connect to htc service (%d)\n", + status); + return status; + } + time_left = wait_for_completion_timeout(&htc->ctl_resp, ATH10K_HTC_WAIT_TIMEOUT_HZ); if (!time_left) { @@ -665,22 +681,6 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc) ath10k_htc_setup_target_buffer_assignments(htc); - /* setup our pseudo HTC control endpoint connection */ - memset(&conn_req, 0, sizeof(conn_req)); - memset(&conn_resp, 0, sizeof(conn_resp)); - conn_req.ep_ops.ep_tx_complete = ath10k_htc_control_tx_complete; - conn_req.ep_ops.ep_rx_complete = ath10k_htc_control_rx_complete; - conn_req.max_send_queue_depth = ATH10K_NUM_CONTROL_TX_BUFFERS; - conn_req.service_id = ATH10K_HTC_SVC_ID_RSVD_CTRL; - - /* connect fake service */ - status = ath10k_htc_connect_service(htc, &conn_req, &conn_resp); - if (status) { - ath10k_err(ar, "could not connect to htc service (%d)\n", - status); - return status; - } - return 0; }
This patch changes the order in which the driver waits for the target to become ready and the service connect of the HTC control service. The HTC control service is connected before the driver starts waiting for the HTC ready control message. The reason for this is that the HTC ready control message is transmitted on EP 0 and that sdio/mbox based systems will ignore messages received on unconnected endpoints. Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com> --- drivers/net/wireless/ath/ath10k/htc.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)