On 15 November 2016 at 18:07, Erik Stromdahl <erik.stromdahl@xxxxxxxxx> wrote:
> On 11/15/2016 11:13 AM, Michal Kazior wrote:
>> On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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ł
