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

[Balakrishna Godavarthi]

Hi Balakrishna,

On 2018-10-18 13:28, 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@xxxxxxxxxxxxxx>
> > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > > > > <bgodavar@xxxxxxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > > 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.
> > > > > > > please explain that a bit more. There is just one debug handle or 
> > > > > > > do
> > > > > > > you have more special handles?
> > > > > > [Bala]: Qualcomm bluetooth chip sends the debug data of the chip 
> > > > > > with ACL header along with fixed handler i.e. 0x2edc..
> > > > > >      this handler is dedicated for the debug logs sent by qcomm bt 
> > > > > > chip and it is not used for any other connection handlers.
> > > > > >      so here we are checking this handler and sending to stack as 
> > > > > > an
> > > > > >      a diagnostic packet.. so during the log capture we will read 
> > > > > > this diagnostic packet and handler,to decode the debug packet.
> > > > > >      that is the  reason why we don't remove handler during 
> > > > > > condition check,  in future, if qualcomm chip sends diagnostic 
> > > > > > packet,
> > > > > >      then this will create an problem to upper layers to decide 
> > > > > > whether it is an diagnostic packet or the debug packet.
> > > > > >     this is how we do it.
> > > > > >    if( diagnostic packet && handler == 0x2edc)
> > > > > >       treat them as an ddebug packet
> > > > > >    else
> > > > > >        treat as diagnostic packet.
> > > > > let me interpret this answer. Besides debug handle 0x2edc, there 
> > > > > are
> > > > > other handles that are not connection handles. And in future 
> > > > > updates
> > > > > you want to send these handles also via hci_recv_diag channel.
> > > > No, 0x2edc is an dedicated handle for debug logs, i don't really 
> > > > think they we will have an new handle coming up.
> > > > this is only the handle we will send via hci_recv_diag channel to 
> > > > the stack.
> > > > what i mean in my previous answer was,we don't to remove debug 
> > > > connection handle from the skb, because in the stack
> > > > we look for a diagnostic packet and debug connection handler to 
> > > > decode the message.
> > > if 0x2edc is the only magic connection handle, then lets strip the 
> > > SKB
> > > of the ACL packet header and send only the payload to via
> > > hci_recv_diag. All the information is present and you would be just
> > > sending the ACL packet header for no reason. The diagnostic packets
> > > get a separate channel on the monitor interface.
> > [Bala]: even we the felt the same to remove ACL packet header before 
> > sending to stack via an diagnostic channel,
> >
> >        here is the case, let us assume that an in future qaulcomm bt 
> > chip want to send diagnosis data as a diagnostic packet.
> >        if we remove ACL header for ACL debug packet, how can the stack 
> > differentiate between debug packet and diagnosis packet.
>
> fair enough. Can we also do the same for the other Qualcomm / Atheros
> devices on USB and SMD bus? And what about implementing hdev->set_diag
> to enable / disable sending of debug logs. And of course the addition
> to btmon to decode the diagnostic channel.
[Bala]:
       Does thing change need to be done for other Qualcomm/Atheros 
device on USB and smd:  For the legacy Qualcomm/Atheros Chips 
(Rome/AR30x2/AR30x3), they donât have this ACL data debug log feature.
       hdev->set_diag implementation to enable or disable the debug logs 
from controller: will study further and get back to you on this
       Qualcomm specific file in Bluez stack to decode the bypassed 
debug packets in btmon: soon we will be posting the parsing file.


> Regards
>
> Marcel

--
Regards
Balakrishna.