From: Govind Singh <govinds@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: niklas.cassel@linaro.org, bjorn.andersson@linaro.org,
david.brown@linaro.org, andy.gross@linaro.org, robh@kernel.org,
devicetree@vger.kernel.org, ath10k@lists.infradead.org,
linux-wireless@vger.kernel.org
Subject: Re: [PATCH v4 6/6] ath10k: Add QMI message handshake for wcn3990 client
Date: Tue, 14 Aug 2018 16:45:31 +0530 [thread overview]
Message-ID: <7658c6c9d8981424cf60a01b3bd5e47d@codeaurora.org> (raw)
In-Reply-To: <20180813200633.GA38014@ban.mtv.corp.google.com>
Thanks Brian for the review.
On 2018-08-14 01:36, Brian Norris wrote:
> Hi Govind,
>
> On Mon, Jul 23, 2018 at 06:04:28PM +0530, Govind Singh wrote:
>> Add WCN3990 QMI client handshakes for Q6 integrated WLAN connectivity
>> subsystem. This layer is responsible for communicating qmi control
>> messages to wifi fw QMI service using QMI messaging protocol.
>>
>> Qualcomm MSM Interface(QMI) is a messaging format used to communicate
>> between components running between remote processors with underlying
>> transport layer based on integrated chipset(shared memory) or
>> discrete chipset(PCI/USB/SDIO/UART).
>>
>> Signed-off-by: Govind Singh <govinds@codeaurora.org>
>> ---
>> drivers/net/wireless/ath/ath10k/Kconfig | 1 +
>> drivers/net/wireless/ath/ath10k/Makefile | 4 +-
>> drivers/net/wireless/ath/ath10k/core.c | 6 +-
>> drivers/net/wireless/ath/ath10k/core.h | 2 +
>> drivers/net/wireless/ath/ath10k/qmi.c | 1019
>> ++++++++++++++++++++++
>> drivers/net/wireless/ath/ath10k/qmi.h | 129 +++
>> drivers/net/wireless/ath/ath10k/snoc.c | 262 +++++-
>> drivers/net/wireless/ath/ath10k/snoc.h | 4 +
>> 8 files changed, 1417 insertions(+), 10 deletions(-)
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h
>>
>
>> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c
>> b/drivers/net/wireless/ath/ath10k/qmi.c
>> new file mode 100644
>> index 000000000000..d91499a7354c
>> --- /dev/null
>> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
>> @@ -0,0 +1,1019 @@
>
> ...
>
>> +int ath10k_qmi_deinit(struct ath10k *ar)
>> +{
>> + struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>> + struct ath10k_qmi *qmi = ar_snoc->qmi;
>> +
>> + cancel_work_sync(&qmi->event_work);
>> + destroy_workqueue(qmi->event_wq);
>
> I believe this is incorrect ordering. You should not be destroying the
> work queue that handles QMI event responses before you release the QMI
> handle. As it stands, this means you segfault during 'rmmod'.
>
Yes, correct. I will correct this in v5 as this is still not merged in
ath10k-next.
>> + qmi_handle_release(&qmi->qmi_hdl);
>
> I think this should be:
>
> qmi_handle_release(&qmi->qmi_hdl);
> cancel_work_sync(&qmi->event_work);
> destroy_workqueue(qmi->event_wq);
>
>> + qmi = NULL;
>
> This assignment is pointless, as 'qmi' is a local variable. Maybe you
> meant to clear the struct member, as a precaution? So this would be:
>
> ar_snoc->qmi = NULL;
>
Yes, i will correct this in v5.
> Brian
>
>> +
>> + return 0;
>> +}
Thanks,
Govind
next prev parent reply other threads:[~2018-08-14 14:02 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-23 12:34 [PATCH v4 0/6] *** Add support for wifi QMI client handshakes *** Govind Singh
2018-07-23 12:34 ` [PATCH v4 1/6] ath10k: Add qmi service helpers for wcn3990 qmi client Govind Singh
2018-07-23 12:34 ` [PATCH v4 2/6] ath10k: Add support to create boardname for non-bmi target Govind Singh
2018-07-23 12:34 ` [PATCH v4 3/6] dt: bindings: add bindings for msa memory region Govind Singh
2018-07-24 23:28 ` Rob Herring
2018-07-23 12:34 ` [PATCH v4 4/6] firmware: qcom: scm: Add WLAN VMID for Qualcomm SCM interface Govind Singh
2018-07-23 12:34 ` [PATCH v4 5/6] ath10k: Add debug mask for QMI layer Govind Singh
2018-07-23 12:34 ` [PATCH v4 6/6] ath10k: Add QMI message handshake for wcn3990 client Govind Singh
2018-08-13 20:06 ` Brian Norris
2018-08-14 11:15 ` Govind Singh [this message]
2018-07-24 20:23 ` [PATCH v4 0/6] *** Add support for wifi QMI client handshakes *** Niklas Cassel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7658c6c9d8981424cf60a01b3bd5e47d@codeaurora.org \
--to=govinds@codeaurora.org \
--cc=andy.gross@linaro.org \
--cc=ath10k@lists.infradead.org \
--cc=bjorn.andersson@linaro.org \
--cc=briannorris@chromium.org \
--cc=david.brown@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=niklas.cassel@linaro.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).