Re: [RFC PATCH 2/3] ath10k: add diag_read() to hif ops

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 11 September 2014 22:31, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> diag_read() is used for reading from firmware memory via the diagnose window.
>> First user will be cal_data debugfs file.
> [...]
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index 154451ab3e3c..d3c51079034e 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -941,6 +941,13 @@ err:
>>         return err;
>>  }
>>
>> +static int ath10k_pci_hif_diag_read(struct ath10k *ar, u32 address, void *buf,
>> +                                   size_t buf_len)
>> +{
>> +       /* FIXME: locking */
>> +       return ath10k_pci_diag_read_mem(ar, address, buf, buf_len);
>
> Hmm.. This can potentially collide with firmware crash as it uses CE
> diag window as well.

Yeah, I was worried something like that could happen.

> I'm thinking entire bodies of diag_read_mem()/diag_writemem() should
> be protected by ar_pci->ce_lock. It shouldn't be hard. As for
> diag_read_mem():
>
>  * s/ath10k_ce_rx_post_buf/__ath10k_ce_rx_post_buf/
>  * s/ath10k_ce_send/ath10k_ce_send_nolock/
>  * s/ath10k_ce_completed_send_next/ath10k_ce_completed_send_next_nolock/
>  * s/ath10k_ce_completed_recv_next/ath10k_ce_completed_recv_next_nolock/
>
> Note: Most _nolock stuff is static in ce.c so it needs to be changed
> to be accessible in pci.c.

Sounds like a good idea, I'll take a closer look and see how it works
out. On the downside this means that there will be one more function
call in hot path (I assume the compiler is able to inline the _nolock
static functions).

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k