Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs.

From: Balakrishna Godavarthi <bgodavar@codeaurora.org>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-bluetooth@vger.kernel.org, hemantg@codeaurora.org,
	linux-arm-msm@vger.kernel.org,
	Harish Bandi <c-hbandi@codeaurora.org>
Subject: Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs.
Date: Tue, 16 Oct 2018 19:32:32 +0530	[thread overview]
Message-ID: <eca6c8832017269881709e51eadcf543@codeaurora.org> (raw)
In-Reply-To: <2206464E-FA01-4A5A-8BC4-5CF70A0B9765@holtmann.org>

Hi Marcel,

On 2018-10-03 22:26, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>> This patch will prevent error messages splashing on console.
>> 
>> [   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL 
>> packet for unknown connection handle 3804
>> [   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL 
>> packet for unknown connection handle 3804
>> [   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL 
>> packet for unknown connection handle 3804
>> [   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL 
>> packet for unknown connection handle 3804
>> 
>> QCA wcn3990 will send the debug logs in the form of ACL packets.
>> While decoding packet in qca_recv(), marking the received debug log
>> packet as diagnostic packet.
>> 
>> Signed-off-by: Harish Bandi <c-hbandi@codeaurora.org>
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> v2:
>>  * Addressed reviewer comments.
>> v1:
>>  * initial patch
>> ---
>> drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index d98ed0442201..63ac3c6b4149 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -63,6 +63,10 @@
>> /* susclk rate */
>> #define SUSCLK_RATE_32KHZ	32768
>> 
>> +/* Controller debug log header */
>> +#define QCA_DEBUG_HDR_MSB	0xDC
>> +#define QCA_DEBUG_HDR_LSB	0x2E
>> +
> 
> since this is actually the ACL handle, why not call it 
> QCA_DEBUG_HANDLE.
> 

[Bala]: will update.

>> /* HCI_IBS transmit side sleep protocol states */
>> enum tx_ibs_states {
>> 	HCI_IBS_TX_ASLEEP,
>> @@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, 
>> struct sk_buff *skb)
>> 	return 0;
>> }
>> 
>> +static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff 
>> *skb)
>> +{
>> +	/* We receive debug logs from chip as an ACL packets.
>> +	 * Instead of sending the data to ACL to decode the
>> +	 * received data, we are pushing them to the above layers
>> +	 * as a diagnostic packet.
>> +	 */
>> +	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
>> +	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
> 
> Skip the individual () since they are not needed. Also the skb->len
> check is not needed since the H4_RECV_ACL already ensures the proper
> length of the header.
> 
> And just use get_unaligned_le16(skb->data) here (or be16 if that is
> the byte order).
> 
[Bala] : will update condition with _le16()

>> +		return hci_recv_diag(hdev, skb);
> 
> Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or
> would it be better to be stripped off. Mainly asking are they any
> other magic handles that we might want to feed through the diag
> channel?
> 

[Bala]: yes we need header in the stack, to  differentiate between 
actual diagnostic packet and debug packet.

>> +
>> +	return hci_recv_frame(hdev, skb);
>> +}
>> +
> 
> Regards
> 
> Marcel

-- 
Regards
Balakrishna.