Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Fri, Jan 18, 2019 at 01:03:46PM -0800, Matthias Kaehlcke wrote:
> Hi Marcel,
> 
> On Fri, Jan 18, 2019 at 09:57:14AM +0100, Marcel Holtmann wrote:
> > Hi Balakrishna,
> > 
> > >>> On 2019-01-15 06:50, Matthias Kaehlcke wrote:
> > >>> > On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi 
> > >>> > wrote:
> > >>> > > On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> > >>> > > > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi 
> > >>> > > > wrote:
> > >>> > > > > Hi Matthias,
> > >>> > > > >
> > >>> > > > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > >>> > > > > > Hi Balakrishna,
> > >>> > > > > >
> > >>> > > > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna 
> > >>> > > > > > Godavarthi wrote:
> > >>> > > > > > > Hi Matthias,
> > >>> > > > > > >
> > >>> > > > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > >>> > > > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna 
> > >>> > > > > > > > Godavarthi wrote:
> > >>> > > > > > > > > Hi Marcel,
> > >>> > > > > > > > >
> > >>> > > > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > >>> > > > > > > > > > Hi Balakrishna,
> > >>> > > > > > > > > >
> > >>> > > > > > > > > > > > > Latest qualcomm chips are not sending an 
> > >>> > > > > > > > > > > > > command complete event for
> > >>> > > > > > > > > > > > > every firmware packet sent to chip. They only 
> > >>> > > > > > > > > > > > > respond with a vendor
> > >>> > > > > > > > > > > > > specific event for the last firmware packet. 
> > >>> > > > > > > > > > > > > This optimization will
> > >>> > > > > > > > > > > > > decrease the BT ON time. Due to this we are 
> > >>> > > > > > > > > > > > > seeing a timeout error
> > >>> > > > > > > > > > > > > message logs on the console during firmware 
> > >>> > > > > > > > > > > > > download. Now we are
> > >>> > > > > > > > > > > > > injecting a command complete event once we 
> > >>> > > > > > > > > > > > > receive an vendor
> > >>> > > > > > > > > > > > > specific
> > >>> > > > > > > > > > > > > event for the last RAM firmware packet.
> > >>> > > > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > >>> > > > > > > > > > > > > 
> > >>> > > > > > > > > > > > > ---
> > >>> > > > > > > > > > > > > drivers/bluetooth/btqca.c | 39
> > >>> > > > > > > > > > > > > ++-
> > >>> > > > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > >>> > > > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > >>> > > > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
> > >>> > > > > > > > > > > > > b/drivers/bluetooth/btqca.c
> > >>> > > > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > >>> > > > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > >>> > > > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > >>> > > > > > > > > > > > > @@ -144,6 +144,7 @@ static void 
> > >>> > > > > > > > > > > > > qca_tlv_check_data(struct
> > >>> > > > > > > > > > > > > rome_config *config,
> > >>> > > > > > > > > > > > >  * In case VSE is skipped, only 
> > >>> > > > > > > > > > > > > the last segment is acked.
> > >>> > > > > > > > > > > > >  */
> > >>> > > > > > > > > > > > > config->dnld_mode = 
> > >>> > > > > > > > > > > > > tlv_patch->download_mode;
> > >>> > > > > > > > > > > > > +   config->dnld_type = 
> > >>> > > > > > > > > > > > > config->dnld_mode;
> > >>> > > > > > > > > > > > > BT_DBG("Total Length   
> > >>> > > > > > > > > > > > > : %d bytes",
> > >>> > > > > > > > > > > > >
> > >>> > > > > > > > > > > > > le32_to_cpu(tlv_patch->total_size));
> > >>> > > > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
> > >>> > > > > > > > > > > > > qca_tlv_send_segment(struct
> > >>> > > > > > > > > > > > > hci_dev *hdev, int seg_size,
> > >>> > > > > > > > > > > > > return err;
> > >>> > > > > > > > > > > > > }
> > >>> > > > > > > > > > > > > +static int 
> > >>> > > > > > > > > > > > > qca_inject_cmd_complete_event(struct hci_dev 
> > >>> > > > > > > > > > > > > *hdev)
> > >>> > > > > > > > > > > > > +{
> > >>> > > > > > > > > > > > > +   struct hci_event_hdr *hdr;
> > >>> > > > > > > > > > > > > +   struct hci_ev_cmd_complete *evt;
> > >>> > > > > > > > > > > > > +   struct sk_buff *skb;
> > >>> > > > > > > > > > > > > +
> > >>> > > > > > > > > > > > > +   skb = bt_skb_alloc(sizeof(*hdr) + 
> > >>> > > > > > > > > > > > > sizeof(*evt) + 1, GFP_KERNEL);
> > >>> > > > > > > > > > > > > +   if (!skb)
> > >>> > > > > > > > > > > > > +   return -ENOMEM;
> > >>> > > > > > > > > > > > > +
> > >>> > > > > > > > > > > > > +   hdr = skb_put(skb, sizeof(*hdr));
> > >>> > > > > > > > > > > > > +   hdr->evt = HCI_EV_CMD_COMPLETE;
> > >>> > > > > > > > > > > > > +   hdr->plen = sizeof(*evt) + 1;
> > >>> > > > > > > > > > > > > +
> > >>> > > > > > > > > > > > > +   evt = skb_put(skb, 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

Hi Marcel,

On Fri, Jan 18, 2019 at 09:57:14AM +0100, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
> >>> On 2019-01-15 06:50, Matthias Kaehlcke wrote:
> >>> > On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:
> >>> > > On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> >>> > > > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi 
> >>> > > > wrote:
> >>> > > > > Hi Matthias,
> >>> > > > >
> >>> > > > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> >>> > > > > > Hi Balakrishna,
> >>> > > > > >
> >>> > > > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna 
> >>> > > > > > Godavarthi wrote:
> >>> > > > > > > Hi Matthias,
> >>> > > > > > >
> >>> > > > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> >>> > > > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna 
> >>> > > > > > > > Godavarthi wrote:
> >>> > > > > > > > > Hi Marcel,
> >>> > > > > > > > >
> >>> > > > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> >>> > > > > > > > > > Hi Balakrishna,
> >>> > > > > > > > > >
> >>> > > > > > > > > > > > > Latest qualcomm chips are not sending an command 
> >>> > > > > > > > > > > > > complete event for
> >>> > > > > > > > > > > > > every firmware packet sent to chip. They only 
> >>> > > > > > > > > > > > > respond with a vendor
> >>> > > > > > > > > > > > > specific event for the last firmware packet. This 
> >>> > > > > > > > > > > > > optimization will
> >>> > > > > > > > > > > > > decrease the BT ON time. Due to this we are 
> >>> > > > > > > > > > > > > seeing a timeout error
> >>> > > > > > > > > > > > > message logs on the console during firmware 
> >>> > > > > > > > > > > > > download. Now we are
> >>> > > > > > > > > > > > > injecting a command complete event once we 
> >>> > > > > > > > > > > > > receive an vendor
> >>> > > > > > > > > > > > > specific
> >>> > > > > > > > > > > > > event for the last RAM firmware packet.
> >>> > > > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> >>> > > > > > > > > > > > > 
> >>> > > > > > > > > > > > > ---
> >>> > > > > > > > > > > > > drivers/bluetooth/btqca.c | 39
> >>> > > > > > > > > > > > > ++-
> >>> > > > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> >>> > > > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> >>> > > > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
> >>> > > > > > > > > > > > > b/drivers/bluetooth/btqca.c
> >>> > > > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> >>> > > > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> >>> > > > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> >>> > > > > > > > > > > > > @@ -144,6 +144,7 @@ static void 
> >>> > > > > > > > > > > > > qca_tlv_check_data(struct
> >>> > > > > > > > > > > > > rome_config *config,
> >>> > > > > > > > > > > > >* In case VSE is skipped, only the 
> >>> > > > > > > > > > > > > last segment is acked.
> >>> > > > > > > > > > > > >*/
> >>> > > > > > > > > > > > >   config->dnld_mode = 
> >>> > > > > > > > > > > > > tlv_patch->download_mode;
> >>> > > > > > > > > > > > > + config->dnld_type = config->dnld_mode;
> >>> > > > > > > > > > > > >   BT_DBG("Total Length   : %d 
> >>> > > > > > > > > > > > > bytes",
> >>> > > > > > > > > > > > >  
> >>> > > > > > > > > > > > > le32_to_cpu(tlv_patch->total_size));
> >>> > > > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
> >>> > > > > > > > > > > > > qca_tlv_send_segment(struct
> >>> > > > > > > > > > > > > hci_dev *hdev, int seg_size,
> >>> > > > > > > > > > > > >   return err;
> >>> > > > > > > > > > > > > }
> >>> > > > > > > > > > > > > +static int qca_inject_cmd_complete_event(struct 
> >>> > > > > > > > > > > > > hci_dev *hdev)
> >>> > > > > > > > > > > > > +{
> >>> > > > > > > > > > > > > + struct hci_event_hdr *hdr;
> >>> > > > > > > > > > > > > + struct hci_ev_cmd_complete *evt;
> >>> > > > > > > > > > > > > + struct sk_buff *skb;
> >>> > > > > > > > > > > > > +
> >>> > > > > > > > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) 
> >>> > > > > > > > > > > > > + 1, GFP_KERNEL);
> >>> > > > > > > > > > > > > + if (!skb)
> >>> > > > > > > > > > > > > + return -ENOMEM;
> >>> > > > > > > > > > > > > +
> >>> > > > > > > > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> >>> > > > > > > > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> >>> > > > > > > > > > > > > + hdr->plen = sizeof(*evt) + 1;
> >>> > > > > > > > > > > > > +
> >>> > > > > > > > > > > > > + evt = skb_put(skb, sizeof(*evt));
> >>> > > > > > > > > > > > > + evt->ncmd = 1;
> >>> > > > > > > > > > > > > + evt->opcode = HCI_OP_NOP;
> >>> > > > > > > >
> >>> > > > > > > > After looking a bit more at it I realize HCI_OP_NOP is not 
> >>> > > > > > > > a good
> >>> > > > > > > > value in this case:
> >>> > > > > > > >
> >>> > > > > > > > static void hci_cmd_complete_evt(...)
> >>> > > > > > > > {
> >>> > > > > > > >   ...
> >>> > 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Marcel Holtmann]

Hi Balakrishna,

>>> On 2019-01-15 06:50, Matthias Kaehlcke wrote:
>>> > On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:
>>> > > On 2019-01-12 04:42, Matthias Kaehlcke wrote:
>>> > > > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi 
>>> > > > wrote:
>>> > > > > Hi Matthias,
>>> > > > >
>>> > > > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
>>> > > > > > Hi Balakrishna,
>>> > > > > >
>>> > > > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi 
>>> > > > > > wrote:
>>> > > > > > > Hi Matthias,
>>> > > > > > >
>>> > > > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
>>> > > > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna 
>>> > > > > > > > Godavarthi wrote:
>>> > > > > > > > > Hi Marcel,
>>> > > > > > > > >
>>> > > > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
>>> > > > > > > > > > Hi Balakrishna,
>>> > > > > > > > > >
>>> > > > > > > > > > > > > Latest qualcomm chips are not sending an command 
>>> > > > > > > > > > > > > complete event for
>>> > > > > > > > > > > > > every firmware packet sent to chip. They only 
>>> > > > > > > > > > > > > respond with a vendor
>>> > > > > > > > > > > > > specific event for the last firmware packet. This 
>>> > > > > > > > > > > > > optimization will
>>> > > > > > > > > > > > > decrease the BT ON time. Due to this we are seeing 
>>> > > > > > > > > > > > > a timeout error
>>> > > > > > > > > > > > > message logs on the console during firmware 
>>> > > > > > > > > > > > > download. Now we are
>>> > > > > > > > > > > > > injecting a command complete event once we receive 
>>> > > > > > > > > > > > > an vendor
>>> > > > > > > > > > > > > specific
>>> > > > > > > > > > > > > event for the last RAM firmware packet.
>>> > > > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
>>> > > > > > > > > > > > > 
>>> > > > > > > > > > > > > ---
>>> > > > > > > > > > > > > drivers/bluetooth/btqca.c | 39
>>> > > > > > > > > > > > > ++-
>>> > > > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
>>> > > > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
>>> > > > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
>>> > > > > > > > > > > > > b/drivers/bluetooth/btqca.c
>>> > > > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
>>> > > > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
>>> > > > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
>>> > > > > > > > > > > > > @@ -144,6 +144,7 @@ static void 
>>> > > > > > > > > > > > > qca_tlv_check_data(struct
>>> > > > > > > > > > > > > rome_config *config,
>>> > > > > > > > > > > > >  * In case VSE is skipped, only the 
>>> > > > > > > > > > > > > last segment is acked.
>>> > > > > > > > > > > > >  */
>>> > > > > > > > > > > > > config->dnld_mode = 
>>> > > > > > > > > > > > > tlv_patch->download_mode;
>>> > > > > > > > > > > > > +   config->dnld_type = config->dnld_mode;
>>> > > > > > > > > > > > > BT_DBG("Total Length   : %d 
>>> > > > > > > > > > > > > bytes",
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > > > le32_to_cpu(tlv_patch->total_size));
>>> > > > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
>>> > > > > > > > > > > > > qca_tlv_send_segment(struct
>>> > > > > > > > > > > > > hci_dev *hdev, int seg_size,
>>> > > > > > > > > > > > > return err;
>>> > > > > > > > > > > > > }
>>> > > > > > > > > > > > > +static int qca_inject_cmd_complete_event(struct 
>>> > > > > > > > > > > > > hci_dev *hdev)
>>> > > > > > > > > > > > > +{
>>> > > > > > > > > > > > > +   struct hci_event_hdr *hdr;
>>> > > > > > > > > > > > > +   struct hci_ev_cmd_complete *evt;
>>> > > > > > > > > > > > > +   struct sk_buff *skb;
>>> > > > > > > > > > > > > +
>>> > > > > > > > > > > > > +   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) 
>>> > > > > > > > > > > > > + 1, GFP_KERNEL);
>>> > > > > > > > > > > > > +   if (!skb)
>>> > > > > > > > > > > > > +   return -ENOMEM;
>>> > > > > > > > > > > > > +
>>> > > > > > > > > > > > > +   hdr = skb_put(skb, sizeof(*hdr));
>>> > > > > > > > > > > > > +   hdr->evt = HCI_EV_CMD_COMPLETE;
>>> > > > > > > > > > > > > +   hdr->plen = sizeof(*evt) + 1;
>>> > > > > > > > > > > > > +
>>> > > > > > > > > > > > > +   evt = skb_put(skb, sizeof(*evt));
>>> > > > > > > > > > > > > +   evt->ncmd = 1;
>>> > > > > > > > > > > > > +   evt->opcode = HCI_OP_NOP;
>>> > > > > > > >
>>> > > > > > > > After looking a bit more at it I realize HCI_OP_NOP is not a 
>>> > > > > > > > good
>>> > > > > > > > value in this case:
>>> > > > > > > >
>>> > > > > > > > static void hci_cmd_complete_evt(...)
>>> > > > > > > > {
>>> > > > > > > >   ...
>>> > > > > > > >
>>> > > > > > > >   if (*opcode != HCI_OP_NOP)
>>> > > > > > > > cancel_delayed_work(>cmd_timer);
>>> > > > > > > >
>>> > > > > > > >   ...
>>> > > > > > > > }
>>> > > > > > > >
>>> > > > > > > > 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

 Hi Matthias,

On 2019-01-17 01:27, Matthias Kaehlcke wrote:

On Wed, Jan 16, 2019 at 03:06:29PM +0530, Balakrishna Godavarthi wrote:

On 2019-01-15 06:50, Matthias Kaehlcke wrote:
> On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:
> > On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> > > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Matthias,
> > > >
> > > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > > > > Hi Balakrishna,
> > > > >
> > > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi 
wrote:
> > > > > > Hi Matthias,
> > > > > >
> > > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi 
wrote:
> > > > > > > > Hi Marcel,
> > > > > > > >
> > > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > > > > > Hi Balakrishna,
> > > > > > > > >
> > > > > > > > > > > > Latest qualcomm chips are not sending an command 
complete event for
> > > > > > > > > > > > every firmware packet sent to chip. They only respond 
with a vendor
> > > > > > > > > > > > specific event for the last firmware packet. This 
optimization will
> > > > > > > > > > > > decrease the BT ON time. Due to this we are seeing a 
timeout error
> > > > > > > > > > > > message logs on the console during firmware download. 
Now we are
> > > > > > > > > > > > injecting a command complete event once we receive an 
vendor
> > > > > > > > > > > > specific
> > > > > > > > > > > > event for the last RAM firmware packet.
> > > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 

> > > > > > > > > > > > ---
> > > > > > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > > > > > ++-
> > > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
b/drivers/bluetooth/btqca.c
> > > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > > > > > @@ -144,6 +144,7 @@ static void 
qca_tlv_check_data(struct
> > > > > > > > > > > > rome_config *config,
> > > > > > > > > > > >  * In case VSE is skipped, only the last 
segment is acked.
> > > > > > > > > > > >  */
> > > > > > > > > > > > config->dnld_mode = 
tlv_patch->download_mode;
> > > > > > > > > > > > +   config->dnld_type = config->dnld_mode;
> > > > > > > > > > > > BT_DBG("Total Length   : %d bytes",
> > > > > > > > > > > >le32_to_cpu(tlv_patch->total_size));
> > > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
qca_tlv_send_segment(struct
> > > > > > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > > > > > return err;
> > > > > > > > > > > > }
> > > > > > > > > > > > +static int qca_inject_cmd_complete_event(struct 
hci_dev *hdev)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +   struct hci_event_hdr *hdr;
> > > > > > > > > > > > +   struct hci_ev_cmd_complete *evt;
> > > > > > > > > > > > +   struct sk_buff *skb;
> > > > > > > > > > > > +
> > > > > > > > > > > > +   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, 
GFP_KERNEL);
> > > > > > > > > > > > +   if (!skb)
> > > > > > > > > > > > +   return -ENOMEM;
> > > > > > > > > > > > +
> > > > > > > > > > > > +   hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > > > > > +   hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > > > > > +   hdr->plen = sizeof(*evt) + 1;
> > > > > > > > > > > > +
> > > > > > > > > > > > +   evt = skb_put(skb, sizeof(*evt));
> > > > > > > > > > > > +   evt->ncmd = 1;
> > > > > > > > > > > > +   evt->opcode = HCI_OP_NOP;
> > > > > > >
> > > > > > > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > > > > > > value in this case:
> > > > > > >
> > > > > > > static void hci_cmd_complete_evt(...)
> > > > > > > {
> > > > > > >   ...
> > > > > > >
> > > > > > >   if (*opcode != HCI_OP_NOP)
> > > > > > > cancel_delayed_work(>cmd_timer);
> > > > > > >
> > > > > > >   ...
> > > > > > > }
> > > > > > >
> > > > > > > 
https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > > > > > >
> > > > > > > Cancelling the command timeout is precisely what we want. Not 
sure why
> > > > > > > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > > > > > > (but not e.g. when inserting an msleep(1000) after downloading the
> > > > > > > NVM.
> > > > > > >
> > > > > > > I suggest to pass the opcode of the command to be completed.
> > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > > > > > > +
> > > > > > > > > > > > +   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > > > > > > +
> > > > > > > > > > > > +   return hci_recv_frame(hdev, skb);
> 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Wed, Jan 16, 2019 at 03:06:29PM +0530, Balakrishna Godavarthi wrote:
> On 2019-01-15 06:50, Matthias Kaehlcke wrote:
> > On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:
> > > On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> > > > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > > > > > Hi Balakrishna,
> > > > > >
> > > > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi 
> > > > > > wrote:
> > > > > > > Hi Matthias,
> > > > > > >
> > > > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna 
> > > > > > > > Godavarthi wrote:
> > > > > > > > > Hi Marcel,
> > > > > > > > >
> > > > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > > > > > > Hi Balakrishna,
> > > > > > > > > >
> > > > > > > > > > > > > Latest qualcomm chips are not sending an command 
> > > > > > > > > > > > > complete event for
> > > > > > > > > > > > > every firmware packet sent to chip. They only respond 
> > > > > > > > > > > > > with a vendor
> > > > > > > > > > > > > specific event for the last firmware packet. This 
> > > > > > > > > > > > > optimization will
> > > > > > > > > > > > > decrease the BT ON time. Due to this we are seeing a 
> > > > > > > > > > > > > timeout error
> > > > > > > > > > > > > message logs on the console during firmware download. 
> > > > > > > > > > > > > Now we are
> > > > > > > > > > > > > injecting a command complete event once we receive an 
> > > > > > > > > > > > > vendor
> > > > > > > > > > > > > specific
> > > > > > > > > > > > > event for the last RAM firmware packet.
> > > > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > > > > > > ++-
> > > > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
> > > > > > > > > > > > > b/drivers/bluetooth/btqca.c
> > > > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > > > > > > @@ -144,6 +144,7 @@ static void 
> > > > > > > > > > > > > qca_tlv_check_data(struct
> > > > > > > > > > > > > rome_config *config,
> > > > > > > > > > > > >* In case VSE is skipped, only the 
> > > > > > > > > > > > > last segment is acked.
> > > > > > > > > > > > >*/
> > > > > > > > > > > > >   config->dnld_mode = 
> > > > > > > > > > > > > tlv_patch->download_mode;
> > > > > > > > > > > > > + config->dnld_type = config->dnld_mode;
> > > > > > > > > > > > >   BT_DBG("Total Length   : %d 
> > > > > > > > > > > > > bytes",
> > > > > > > > > > > > >  
> > > > > > > > > > > > > le32_to_cpu(tlv_patch->total_size));
> > > > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
> > > > > > > > > > > > > qca_tlv_send_segment(struct
> > > > > > > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > > > > > >   return err;
> > > > > > > > > > > > > }
> > > > > > > > > > > > > +static int qca_inject_cmd_complete_event(struct 
> > > > > > > > > > > > > hci_dev *hdev)
> > > > > > > > > > > > > +{
> > > > > > > > > > > > > + struct hci_event_hdr *hdr;
> > > > > > > > > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > > > > > > > > + struct sk_buff *skb;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) 
> > > > > > > > > > > > > + 1, GFP_KERNEL);
> > > > > > > > > > > > > + if (!skb)
> > > > > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > > > > > > > > + evt->ncmd = 1;
> > > > > > > > > > > > > + evt->opcode = HCI_OP_NOP;
> > > > > > > >
> > > > > > > > After looking a bit more at it I realize HCI_OP_NOP is not a 
> > > > > > > > good
> > > > > > > > value in this case:
> > > > > > > >
> > > > > > > > static void hci_cmd_complete_evt(...)
> > > > > > > > {
> > > > > > > >   ...
> > > > > > > >
> > > > > > > >   if (*opcode != HCI_OP_NOP)
> > > > > > > > cancel_delayed_work(>cmd_timer);
> > > > > > > >
> > > > > > > >   ...
> > > > > > > > }
> > > > > > > >
> > > > > > > > https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > > > > > > >
> > > > > > > > Cancelling the command 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

On 2019-01-15 06:50, Matthias Kaehlcke wrote:

On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:

On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > > Hi Balakrishna,
> > >
> > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Matthias,
> > > >
> > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi 
wrote:
> > > > > > Hi Marcel,
> > > > > >
> > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > > > Hi Balakrishna,
> > > > > > >
> > > > > > > > > > Latest qualcomm chips are not sending an command complete 
event for
> > > > > > > > > > every firmware packet sent to chip. They only respond with 
a vendor
> > > > > > > > > > specific event for the last firmware packet. This 
optimization will
> > > > > > > > > > decrease the BT ON time. Due to this we are seeing a 
timeout error
> > > > > > > > > > message logs on the console during firmware download. Now 
we are
> > > > > > > > > > injecting a command complete event once we receive an vendor
> > > > > > > > > > specific
> > > > > > > > > > event for the last RAM firmware packet.
> > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 

> > > > > > > > > > ---
> > > > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > > > ++-
> > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
b/drivers/bluetooth/btqca.c
> > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > > > > > rome_config *config,
> > > > > > > > > >* In case VSE is skipped, only the last 
segment is acked.
> > > > > > > > > >*/
> > > > > > > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > > > > > > + config->dnld_type = config->dnld_mode;
> > > > > > > > > >   BT_DBG("Total Length   : %d bytes",
> > > > > > > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > > > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > > >   return err;
> > > > > > > > > > }
> > > > > > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev 
*hdev)
> > > > > > > > > > +{
> > > > > > > > > > + struct hci_event_hdr *hdr;
> > > > > > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > > > > > + struct sk_buff *skb;
> > > > > > > > > > +
> > > > > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, 
GFP_KERNEL);
> > > > > > > > > > + if (!skb)
> > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > +
> > > > > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > > > > > +
> > > > > > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > > > > > + evt->ncmd = 1;
> > > > > > > > > > + evt->opcode = HCI_OP_NOP;
> > > > >
> > > > > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > > > > value in this case:
> > > > >
> > > > > static void hci_cmd_complete_evt(...)
> > > > > {
> > > > >   ...
> > > > >
> > > > >   if (*opcode != HCI_OP_NOP)
> > > > > cancel_delayed_work(>cmd_timer);
> > > > >
> > > > >   ...
> > > > > }
> > > > >
> > > > > 
https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > > > >
> > > > > Cancelling the command timeout is precisely what we want. Not sure why
> > > > > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > > > > (but not e.g. when inserting an msleep(1000) after downloading the
> > > > > NVM.
> > > > >
> > > > > I suggest to pass the opcode of the command to be completed.
> > > > >
> > > > > > > > > > +
> > > > > > > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > > > > +
> > > > > > > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > > > > +
> > > > > > > > > > + return hci_recv_frame(hdev, skb);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > > > > > > struct rome_config *config)
> > > > > > > > > > {
> > > > > > > > > > @@ -297,11 +323,22 @@ static int
> > > > > > > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > > > > > > >   ret = qca_tlv_send_segment(hdev, segsize, 
segment,
> > > > > > > > > >   

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Mon, Jan 14, 2019 at 09:21:25PM +0530, Balakrishna Godavarthi wrote:
> On 2019-01-12 04:42, Matthias Kaehlcke wrote:
> > On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > > > Hi Balakrishna,
> > > >
> > > > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi 
> > > > > > wrote:
> > > > > > > Hi Marcel,
> > > > > > >
> > > > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > > > > Hi Balakrishna,
> > > > > > > >
> > > > > > > > > > > Latest qualcomm chips are not sending an command complete 
> > > > > > > > > > > event for
> > > > > > > > > > > every firmware packet sent to chip. They only respond 
> > > > > > > > > > > with a vendor
> > > > > > > > > > > specific event for the last firmware packet. This 
> > > > > > > > > > > optimization will
> > > > > > > > > > > decrease the BT ON time. Due to this we are seeing a 
> > > > > > > > > > > timeout error
> > > > > > > > > > > message logs on the console during firmware download. Now 
> > > > > > > > > > > we are
> > > > > > > > > > > injecting a command complete event once we receive an 
> > > > > > > > > > > vendor
> > > > > > > > > > > specific
> > > > > > > > > > > event for the last RAM firmware packet.
> > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > > > > > 
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > > > > ++-
> > > > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
> > > > > > > > > > > b/drivers/bluetooth/btqca.c
> > > > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > > > > > > rome_config *config,
> > > > > > > > > > >* In case VSE is skipped, only the last 
> > > > > > > > > > > segment is acked.
> > > > > > > > > > >*/
> > > > > > > > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > > > > > > > + config->dnld_type = config->dnld_mode;
> > > > > > > > > > >   BT_DBG("Total Length   : %d bytes",
> > > > > > > > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > > > > > > > @@ -264,6 +265,31 @@ static int 
> > > > > > > > > > > qca_tlv_send_segment(struct
> > > > > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > > > >   return err;
> > > > > > > > > > > }
> > > > > > > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev 
> > > > > > > > > > > *hdev)
> > > > > > > > > > > +{
> > > > > > > > > > > + struct hci_event_hdr *hdr;
> > > > > > > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > > > > > > + struct sk_buff *skb;
> > > > > > > > > > > +
> > > > > > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, 
> > > > > > > > > > > GFP_KERNEL);
> > > > > > > > > > > + if (!skb)
> > > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > > +
> > > > > > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > > > > > > +
> > > > > > > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > > > > > > + evt->ncmd = 1;
> > > > > > > > > > > + evt->opcode = HCI_OP_NOP;
> > > > > >
> > > > > > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > > > > > value in this case:
> > > > > >
> > > > > > static void hci_cmd_complete_evt(...)
> > > > > > {
> > > > > >   ...
> > > > > >
> > > > > >   if (*opcode != HCI_OP_NOP)
> > > > > > cancel_delayed_work(>cmd_timer);
> > > > > >
> > > > > >   ...
> > > > > > }
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > > > > >
> > > > > > Cancelling the command timeout is precisely what we want. Not sure 
> > > > > > why
> > > > > > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > > > > > (but not e.g. when inserting an msleep(1000) after downloading the
> > > > > > NVM.
> > > > > >
> > > > > > I suggest to pass the opcode of the command to be completed.
> > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > > > > > +
> > > > > > > > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > > > > > +
> > > > > > > > > > > + return hci_recv_frame(hdev, skb);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

On 2019-01-12 04:42, Matthias Kaehlcke wrote:

On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:

Hi Matthias,

On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> Hi Balakrishna,
>
> On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:
> > Hi Matthias,
> >
> > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Marcel,
> > > >
> > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > Hi Balakrishna,
> > > > >
> > > > > > > > Latest qualcomm chips are not sending an command complete event 
for
> > > > > > > > every firmware packet sent to chip. They only respond with a 
vendor
> > > > > > > > specific event for the last firmware packet. This optimization 
will
> > > > > > > > decrease the BT ON time. Due to this we are seeing a timeout 
error
> > > > > > > > message logs on the console during firmware download. Now we are
> > > > > > > > injecting a command complete event once we receive an vendor
> > > > > > > > specific
> > > > > > > > event for the last RAM firmware packet.
> > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > > ---
> > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > ++-
> > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
b/drivers/bluetooth/btqca.c
> > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > > > rome_config *config,
> > > > > > > >  * In case VSE is skipped, only the last 
segment is acked.
> > > > > > > >  */
> > > > > > > > config->dnld_mode = tlv_patch->download_mode;
> > > > > > > > +   config->dnld_type = config->dnld_mode;
> > > > > > > > BT_DBG("Total Length   : %d bytes",
> > > > > > > >le32_to_cpu(tlv_patch->total_size));
> > > > > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > return err;
> > > > > > > > }
> > > > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> > > > > > > > +{
> > > > > > > > +   struct hci_event_hdr *hdr;
> > > > > > > > +   struct hci_ev_cmd_complete *evt;
> > > > > > > > +   struct sk_buff *skb;
> > > > > > > > +
> > > > > > > > +   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, 
GFP_KERNEL);
> > > > > > > > +   if (!skb)
> > > > > > > > +   return -ENOMEM;
> > > > > > > > +
> > > > > > > > +   hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > +   hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > +   hdr->plen = sizeof(*evt) + 1;
> > > > > > > > +
> > > > > > > > +   evt = skb_put(skb, sizeof(*evt));
> > > > > > > > +   evt->ncmd = 1;
> > > > > > > > +   evt->opcode = HCI_OP_NOP;
> > >
> > > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > > value in this case:
> > >
> > > static void hci_cmd_complete_evt(...)
> > > {
> > >   ...
> > >
> > >   if (*opcode != HCI_OP_NOP)
> > > cancel_delayed_work(>cmd_timer);
> > >
> > >   ...
> > > }
> > >
> > > 
https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > >
> > > Cancelling the command timeout is precisely what we want. Not sure why
> > > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > > (but not e.g. when inserting an msleep(1000) after downloading the
> > > NVM.
> > >
> > > I suggest to pass the opcode of the command to be completed.
> > >
> > > > > > > > +
> > > > > > > > +   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > > +
> > > > > > > > +   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > > +
> > > > > > > > +   return hci_recv_frame(hdev, skb);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > > > >   struct rome_config *config)
> > > > > > > > {
> > > > > > > > @@ -297,11 +323,22 @@ static int
> > > > > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > > > > > ret = qca_tlv_send_segment(hdev, segsize, 
segment,
> > > > > > > > config->dnld_mode);
> > > > > > > > if (ret)
> > > > > > > > -   break;
> > > > > > > > +   goto out;
> > > > > > > > segment += segsize;
> > > > > > > > }
> > > > > > > > +   /* Latest qualcomm chipsets are not sending a command
> > > > > > > > complete event
> > > > > > > > +* for every fw packet sent. They only respond with a
> > > > > > > > vendor specific
> 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Fri, Jan 11, 2019 at 07:53:43PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-11 02:13, Matthias Kaehlcke wrote:
> > Hi Balakrishna,
> > 
> > On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > > > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Marcel,
> > > > >
> > > > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > > > Hi Balakrishna,
> > > > > >
> > > > > > > > > Latest qualcomm chips are not sending an command complete 
> > > > > > > > > event for
> > > > > > > > > every firmware packet sent to chip. They only respond with a 
> > > > > > > > > vendor
> > > > > > > > > specific event for the last firmware packet. This 
> > > > > > > > > optimization will
> > > > > > > > > decrease the BT ON time. Due to this we are seeing a timeout 
> > > > > > > > > error
> > > > > > > > > message logs on the console during firmware download. Now we 
> > > > > > > > > are
> > > > > > > > > injecting a command complete event once we receive an vendor
> > > > > > > > > specific
> > > > > > > > > event for the last RAM firmware packet.
> > > > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > > > ++-
> > > > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > > > diff --git a/drivers/bluetooth/btqca.c 
> > > > > > > > > b/drivers/bluetooth/btqca.c
> > > > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > > > > rome_config *config,
> > > > > > > > >* In case VSE is skipped, only the last 
> > > > > > > > > segment is acked.
> > > > > > > > >*/
> > > > > > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > > > > > + config->dnld_type = config->dnld_mode;
> > > > > > > > >   BT_DBG("Total Length   : %d bytes",
> > > > > > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > > > > > hci_dev *hdev, int seg_size,
> > > > > > > > >   return err;
> > > > > > > > > }
> > > > > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev 
> > > > > > > > > *hdev)
> > > > > > > > > +{
> > > > > > > > > + struct hci_event_hdr *hdr;
> > > > > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > > > > + struct sk_buff *skb;
> > > > > > > > > +
> > > > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, 
> > > > > > > > > GFP_KERNEL);
> > > > > > > > > + if (!skb)
> > > > > > > > > + return -ENOMEM;
> > > > > > > > > +
> > > > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > > > > +
> > > > > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > > > > + evt->ncmd = 1;
> > > > > > > > > + evt->opcode = HCI_OP_NOP;
> > > >
> > > > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > > > value in this case:
> > > >
> > > > static void hci_cmd_complete_evt(...)
> > > > {
> > > >   ...
> > > >
> > > >   if (*opcode != HCI_OP_NOP)
> > > > cancel_delayed_work(>cmd_timer);
> > > >
> > > >   ...
> > > > }
> > > >
> > > > https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > > >
> > > > Cancelling the command timeout is precisely what we want. Not sure why
> > > > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > > > (but not e.g. when inserting an msleep(1000) after downloading the
> > > > NVM.
> > > >
> > > > I suggest to pass the opcode of the command to be completed.
> > > >
> > > > > > > > > +
> > > > > > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > > > +
> > > > > > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > > > +
> > > > > > > > > + return hci_recv_frame(hdev, skb);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > > > > > struct rome_config *config)
> > > > > > > > > {
> > > > > > > > > @@ -297,11 +323,22 @@ static int
> > > > > > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > > > > > >   ret = qca_tlv_send_segment(hdev, segsize, 
> > > > > > > > > segment,
> > > > > > > > >   config->dnld_mode);
> > > > > > > > >   if (ret)
> > > > > > > > > - break;
> > > > > > > > > + goto 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Hi Matthias,

On 2019-01-11 02:13, Matthias Kaehlcke wrote:

Hi Balakrishna,

On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:

Hi Matthias,

On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:
> > Hi Marcel,
> >
> > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > Hi Balakrishna,
> > >
> > > > > > Latest qualcomm chips are not sending an command complete event for
> > > > > > every firmware packet sent to chip. They only respond with a vendor
> > > > > > specific event for the last firmware packet. This optimization will
> > > > > > decrease the BT ON time. Due to this we are seeing a timeout error
> > > > > > message logs on the console during firmware download. Now we are
> > > > > > injecting a command complete event once we receive an vendor
> > > > > > specific
> > > > > > event for the last RAM firmware packet.
> > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > ---
> > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > ++-
> > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > rome_config *config,
> > > > > >* In case VSE is skipped, only the last segment is acked.
> > > > > >*/
> > > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > > + config->dnld_type = config->dnld_mode;
> > > > > >   BT_DBG("Total Length   : %d bytes",
> > > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > > hci_dev *hdev, int seg_size,
> > > > > >   return err;
> > > > > > }
> > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> > > > > > +{
> > > > > > + struct hci_event_hdr *hdr;
> > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > + struct sk_buff *skb;
> > > > > > +
> > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> > > > > > + if (!skb)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > +
> > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > + evt->ncmd = 1;
> > > > > > + evt->opcode = HCI_OP_NOP;
>
> After looking a bit more at it I realize HCI_OP_NOP is not a good
> value in this case:
>
> static void hci_cmd_complete_evt(...)
> {
>   ...
>
>   if (*opcode != HCI_OP_NOP)
> cancel_delayed_work(>cmd_timer);
>
>   ...
> }
>
> https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
>
> Cancelling the command timeout is precisely what we want. Not sure why
> the patch with HCI_OP_NOP makes the timeouts go away in most cases
> (but not e.g. when inserting an msleep(1000) after downloading the
> NVM.
>
> I suggest to pass the opcode of the command to be completed.
>
> > > > > > +
> > > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > +
> > > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > +
> > > > > > + return hci_recv_frame(hdev, skb);
> > > > > > +}
> > > > > > +
> > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > > struct rome_config *config)
> > > > > > {
> > > > > > @@ -297,11 +323,22 @@ static int
> > > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > > >   ret = qca_tlv_send_segment(hdev, segsize, segment,
> > > > > >   config->dnld_mode);
> > > > > >   if (ret)
> > > > > > - break;
> > > > > > + goto out;
> > > > > >   segment += segsize;
> > > > > >   }
> > > > > > + /* Latest qualcomm chipsets are not sending a command
> > > > > > complete event
> > > > > > +  * for every fw packet sent. They only respond with a
> > > > > > vendor specific
> > > > > > +  * event for the last packet. This optimization in the chip will
> > > > > > +  * decrease the BT in initialization time. Here we will
> > > > > > inject a command
> > > > > > +  * complete event to avoid a command timeout error message.
> > > > > > +  */
> > > > > > + if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> > > > > > + config->dnld_type == ROME_SKIP_EVT_VSE))
> > > > > > + return qca_inject_cmd_complete_event(hdev);
> > > > > > +
> > > > > have you actually considered using __hci_cmd_send in that case. It is
> > > > > allowed for vendor OGF to use that command. I see you actually do use
> > > > > it and now I am failing to understand what this is for.
> > > > [Bala]: thanks for reviewing 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

Hi Balakrishna,

On Thu, Jan 10, 2019 at 08:30:43PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-03 03:45, Matthias Kaehlcke wrote:
> > On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:
> > > Hi Marcel,
> > > 
> > > On 2018-12-30 13:40, Marcel Holtmann wrote:
> > > > Hi Balakrishna,
> > > >
> > > > > > > Latest qualcomm chips are not sending an command complete event 
> > > > > > > for
> > > > > > > every firmware packet sent to chip. They only respond with a 
> > > > > > > vendor
> > > > > > > specific event for the last firmware packet. This optimization 
> > > > > > > will
> > > > > > > decrease the BT ON time. Due to this we are seeing a timeout error
> > > > > > > message logs on the console during firmware download. Now we are
> > > > > > > injecting a command complete event once we receive an vendor
> > > > > > > specific
> > > > > > > event for the last RAM firmware packet.
> > > > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > > > ---
> > > > > > > drivers/bluetooth/btqca.c | 39
> > > > > > > ++-
> > > > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > > > --- a/drivers/bluetooth/btqca.c
> > > > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > > > rome_config *config,
> > > > > > >* In case VSE is skipped, only the last segment is 
> > > > > > > acked.
> > > > > > >*/
> > > > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > > > + config->dnld_type = config->dnld_mode;
> > > > > > >   BT_DBG("Total Length   : %d bytes",
> > > > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > > > hci_dev *hdev, int seg_size,
> > > > > > >   return err;
> > > > > > > }
> > > > > > > +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> > > > > > > +{
> > > > > > > + struct hci_event_hdr *hdr;
> > > > > > > + struct hci_ev_cmd_complete *evt;
> > > > > > > + struct sk_buff *skb;
> > > > > > > +
> > > > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> > > > > > > + if (!skb)
> > > > > > > + return -ENOMEM;
> > > > > > > +
> > > > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > > > +
> > > > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > > > + evt->ncmd = 1;
> > > > > > > + evt->opcode = HCI_OP_NOP;
> > 
> > After looking a bit more at it I realize HCI_OP_NOP is not a good
> > value in this case:
> > 
> > static void hci_cmd_complete_evt(...)
> > {
> >   ...
> > 
> >   if (*opcode != HCI_OP_NOP)
> > cancel_delayed_work(>cmd_timer);
> > 
> >   ...
> > }
> > 
> > https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351
> > 
> > Cancelling the command timeout is precisely what we want. Not sure why
> > the patch with HCI_OP_NOP makes the timeouts go away in most cases
> > (but not e.g. when inserting an msleep(1000) after downloading the
> > NVM.
> > 
> > I suggest to pass the opcode of the command to be completed.
> > 
> > > > > > > +
> > > > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > > > +
> > > > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > > > +
> > > > > > > + return hci_recv_frame(hdev, skb);
> > > > > > > +}
> > > > > > > +
> > > > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > > > struct rome_config *config)
> > > > > > > {
> > > > > > > @@ -297,11 +323,22 @@ static int
> > > > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > > > >   ret = qca_tlv_send_segment(hdev, segsize, segment,
> > > > > > >   config->dnld_mode);
> > > > > > >   if (ret)
> > > > > > > - break;
> > > > > > > + goto out;
> > > > > > >   segment += segsize;
> > > > > > >   }
> > > > > > > + /* Latest qualcomm chipsets are not sending a command
> > > > > > > complete event
> > > > > > > +  * for every fw packet sent. They only respond with a
> > > > > > > vendor specific
> > > > > > > +  * event for the last packet. This optimization in the chip will
> > > > > > > +  * decrease the BT in initialization time. Here we will
> > > > > > > inject a command
> > > > > > > +  * complete event to avoid a command timeout error message.
> > > > > > > +  */
> > > > > > > + if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> > > > > > > + config->dnld_type == ROME_SKIP_EVT_VSE))
> > > > > > > + return qca_inject_cmd_complete_event(hdev);
> > > > > > > +
> > > > > > have you actually 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Hi Matthias,

On 2019-01-03 03:45, Matthias Kaehlcke wrote:

On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:

Hi Marcel,

On 2018-12-30 13:40, Marcel Holtmann wrote:
> Hi Balakrishna,
>
> > > > Latest qualcomm chips are not sending an command complete event for
> > > > every firmware packet sent to chip. They only respond with a vendor
> > > > specific event for the last firmware packet. This optimization will
> > > > decrease the BT ON time. Due to this we are seeing a timeout error
> > > > message logs on the console during firmware download. Now we are
> > > > injecting a command complete event once we receive an vendor
> > > > specific
> > > > event for the last RAM firmware packet.
> > > > Signed-off-by: Balakrishna Godavarthi 
> > > > ---
> > > > drivers/bluetooth/btqca.c | 39
> > > > ++-
> > > > drivers/bluetooth/btqca.h |  3 +++
> > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > > index ec9e03a6b778..0b533f65f652 100644
> > > > --- a/drivers/bluetooth/btqca.c
> > > > +++ b/drivers/bluetooth/btqca.c
> > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > rome_config *config,
> > > >  * In case VSE is skipped, only the last segment is acked.
> > > >  */
> > > > config->dnld_mode = tlv_patch->download_mode;
> > > > +   config->dnld_type = config->dnld_mode;
> > > > BT_DBG("Total Length   : %d bytes",
> > > >le32_to_cpu(tlv_patch->total_size));
> > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > hci_dev *hdev, int seg_size,
> > > > return err;
> > > > }
> > > > +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> > > > +{
> > > > +   struct hci_event_hdr *hdr;
> > > > +   struct hci_ev_cmd_complete *evt;
> > > > +   struct sk_buff *skb;
> > > > +
> > > > +   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> > > > +   if (!skb)
> > > > +   return -ENOMEM;
> > > > +
> > > > +   hdr = skb_put(skb, sizeof(*hdr));
> > > > +   hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > +   hdr->plen = sizeof(*evt) + 1;
> > > > +
> > > > +   evt = skb_put(skb, sizeof(*evt));
> > > > +   evt->ncmd = 1;
> > > > +   evt->opcode = HCI_OP_NOP;


After looking a bit more at it I realize HCI_OP_NOP is not a good
value in this case:

static void hci_cmd_complete_evt(...)
{
  ...

  if (*opcode != HCI_OP_NOP)
cancel_delayed_work(>cmd_timer);

  ...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why
the patch with HCI_OP_NOP makes the timeouts go away in most cases
(but not e.g. when inserting an msleep(1000) after downloading the
NVM.

I suggest to pass the opcode of the command to be completed.


> > > > +
> > > > +   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > +
> > > > +   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > +
> > > > +   return hci_recv_frame(hdev, skb);
> > > > +}
> > > > +
> > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > >   struct rome_config *config)
> > > > {
> > > > @@ -297,11 +323,22 @@ static int
> > > > qca_download_firmware(struct hci_dev *hdev,
> > > > ret = qca_tlv_send_segment(hdev, segsize, segment,
> > > > config->dnld_mode);
> > > > if (ret)
> > > > -   break;
> > > > +   goto out;
> > > > segment += segsize;
> > > > }
> > > > +   /* Latest qualcomm chipsets are not sending a command
> > > > complete event
> > > > +* for every fw packet sent. They only respond with a
> > > > vendor specific
> > > > +* event for the last packet. This optimization in the chip will
> > > > +* decrease the BT in initialization time. Here we will
> > > > inject a command
> > > > +* complete event to avoid a command timeout error message.
> > > > +*/
> > > > +   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> > > > +   config->dnld_type == ROME_SKIP_EVT_VSE))
> > > > +   return qca_inject_cmd_complete_event(hdev);
> > > > +
> > > have you actually considered using __hci_cmd_send in that case. It is
> > > allowed for vendor OGF to use that command. I see you actually do use
> > > it and now I am failing to understand what this is for.
> > [Bala]: thanks for reviewing the change.
> >
> > __hci_cmd_send() can be used only to send the command to the chip.
> > it will not wait for the response for the command sent.
> >
> > as you know that every vendor command sent to chip will respond with
> > vendor specific event and command complete event.
> > but in our case chip will only respond with vendor specific event
> > only. so we are injecting command complete event.
>
> and __hci_cmd_sync_ev is also not working for you? However 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Mon, Dec 31, 2018 at 11:34:46AM +0530, Balakrishna Godavarthi wrote:
> Hi Marcel,
> 
> On 2018-12-30 13:40, Marcel Holtmann wrote:
> > Hi Balakrishna,
> > 
> > > > > Latest qualcomm chips are not sending an command complete event for
> > > > > every firmware packet sent to chip. They only respond with a vendor
> > > > > specific event for the last firmware packet. This optimization will
> > > > > decrease the BT ON time. Due to this we are seeing a timeout error
> > > > > message logs on the console during firmware download. Now we are
> > > > > injecting a command complete event once we receive an vendor
> > > > > specific
> > > > > event for the last RAM firmware packet.
> > > > > Signed-off-by: Balakrishna Godavarthi 
> > > > > ---
> > > > > drivers/bluetooth/btqca.c | 39
> > > > > ++-
> > > > > drivers/bluetooth/btqca.h |  3 +++
> > > > > 2 files changed, 41 insertions(+), 1 deletion(-)
> > > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > > > index ec9e03a6b778..0b533f65f652 100644
> > > > > --- a/drivers/bluetooth/btqca.c
> > > > > +++ b/drivers/bluetooth/btqca.c
> > > > > @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
> > > > > rome_config *config,
> > > > >* In case VSE is skipped, only the last segment is 
> > > > > acked.
> > > > >*/
> > > > >   config->dnld_mode = tlv_patch->download_mode;
> > > > > + config->dnld_type = config->dnld_mode;
> > > > >   BT_DBG("Total Length   : %d bytes",
> > > > >  le32_to_cpu(tlv_patch->total_size));
> > > > > @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
> > > > > hci_dev *hdev, int seg_size,
> > > > >   return err;
> > > > > }
> > > > > +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> > > > > +{
> > > > > + struct hci_event_hdr *hdr;
> > > > > + struct hci_ev_cmd_complete *evt;
> > > > > + struct sk_buff *skb;
> > > > > +
> > > > > + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> > > > > + if (!skb)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + hdr = skb_put(skb, sizeof(*hdr));
> > > > > + hdr->evt = HCI_EV_CMD_COMPLETE;
> > > > > + hdr->plen = sizeof(*evt) + 1;
> > > > > +
> > > > > + evt = skb_put(skb, sizeof(*evt));
> > > > > + evt->ncmd = 1;
> > > > > + evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good
value in this case:

static void hci_cmd_complete_evt(...)
{
  ...

  if (*opcode != HCI_OP_NOP)
cancel_delayed_work(>cmd_timer);

  ...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why
the patch with HCI_OP_NOP makes the timeouts go away in most cases
(but not e.g. when inserting an msleep(1000) after downloading the
NVM.

I suggest to pass the opcode of the command to be completed.

> > > > > +
> > > > > + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> > > > > +
> > > > > + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> > > > > +
> > > > > + return hci_recv_frame(hdev, skb);
> > > > > +}
> > > > > +
> > > > > static int qca_download_firmware(struct hci_dev *hdev,
> > > > > struct rome_config *config)
> > > > > {
> > > > > @@ -297,11 +323,22 @@ static int
> > > > > qca_download_firmware(struct hci_dev *hdev,
> > > > >   ret = qca_tlv_send_segment(hdev, segsize, segment,
> > > > >   config->dnld_mode);
> > > > >   if (ret)
> > > > > - break;
> > > > > + goto out;
> > > > >   segment += segsize;
> > > > >   }
> > > > > + /* Latest qualcomm chipsets are not sending a command
> > > > > complete event
> > > > > +  * for every fw packet sent. They only respond with a
> > > > > vendor specific
> > > > > +  * event for the last packet. This optimization in the chip will
> > > > > +  * decrease the BT in initialization time. Here we will
> > > > > inject a command
> > > > > +  * complete event to avoid a command timeout error message.
> > > > > +  */
> > > > > + if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> > > > > + config->dnld_type == ROME_SKIP_EVT_VSE))
> > > > > + return qca_inject_cmd_complete_event(hdev);
> > > > > +
> > > > have you actually considered using __hci_cmd_send in that case. It is
> > > > allowed for vendor OGF to use that command. I see you actually do use
> > > > it and now I am failing to understand what this is for.
> > > [Bala]: thanks for reviewing the change.
> > > 
> > > __hci_cmd_send() can be used only to send the command to the chip.
> > > it will not wait for the response for the command sent.
> > > 
> > > as you know that every vendor command sent to chip will respond with
> > > vendor specific 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-12-31 11:34, Balakrishna Godavarthi wrote:

Hi Marcel,

On 2018-12-30 13:40, Marcel Holtmann wrote:

Hi Balakrishna,


Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor 
specific

event for the last RAM firmware packet.
Signed-off-by: Balakrishna Godavarthi 
---
drivers/bluetooth/btqca.c | 39 
++-

drivers/bluetooth/btqca.h |  3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct 
rome_config *config,

 * In case VSE is skipped, only the last segment is acked.
 */
config->dnld_mode = tlv_patch->download_mode;
+   config->dnld_type = config->dnld_mode;
BT_DBG("Total Length   : %d bytes",
   le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
*hdev, int seg_size,

return err;
}
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+   struct hci_event_hdr *hdr;
+   struct hci_ev_cmd_complete *evt;
+   struct sk_buff *skb;
+
+   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+   if (!skb)
+   return -ENOMEM;
+
+   hdr = skb_put(skb, sizeof(*hdr));
+   hdr->evt = HCI_EV_CMD_COMPLETE;
+   hdr->plen = sizeof(*evt) + 1;
+
+   evt = skb_put(skb, sizeof(*evt));
+   evt->ncmd = 1;
+   evt->opcode = HCI_OP_NOP;
+
+   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+   return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
  struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int qca_download_firmware(struct 
hci_dev *hdev,

ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
-   break;
+   goto out;
segment += segsize;
}
+	/* Latest qualcomm chipsets are not sending a command complete 
event
+	 * for every fw packet sent. They only respond with a vendor 
specific

+* event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 
command

+* complete event to avoid a command timeout error message.
+*/
+   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+   config->dnld_type == ROME_SKIP_EVT_VSE))
+   return qca_inject_cmd_complete_event(hdev);
+
have you actually considered using __hci_cmd_send in that case. It 
is
allowed for vendor OGF to use that command. I see you actually do 
use

it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip. it 
will not wait for the response for the command sent.


as you know that every vendor command sent to chip will respond with 
vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event 
only. so we are injecting command complete event.


and __hci_cmd_sync_ev is also not working for you? However since you
are not waiting for the vendor event anyway and just injecting
cmd_complete, I wonder what’s the difference in just using
__hci_cmd_send and not bothering to wait or inject at all. I am
failing to see where this injection makes a difference.

For me it is a big difference if we are injecting one event like in
the case of Intel compared to injecting one for every command. It will
show a wrong picture in btmon and that is a bad idea.

Regards

Marcel


[Bala]: here is the use case, when ever we download the fw packets
i.e. RAM image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command
complete event.

the above is taking more time to setup the BT device. then we came up
with solution where we enable flags in fw file (i.e. RAM image header)
whether to wait for event to be received or sent the total packets and
wait for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for
event for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent,
we are only receiving an vendor 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-12-30 13:40, Marcel Holtmann wrote:

Hi Balakrishna,


Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor 
specific

event for the last RAM firmware packet.
Signed-off-by: Balakrishna Godavarthi 
---
drivers/bluetooth/btqca.c | 39 
++-

drivers/bluetooth/btqca.h |  3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct 
rome_config *config,

 * In case VSE is skipped, only the last segment is acked.
 */
config->dnld_mode = tlv_patch->download_mode;
+   config->dnld_type = config->dnld_mode;
BT_DBG("Total Length   : %d bytes",
   le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
*hdev, int seg_size,

return err;
}
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+   struct hci_event_hdr *hdr;
+   struct hci_ev_cmd_complete *evt;
+   struct sk_buff *skb;
+
+   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+   if (!skb)
+   return -ENOMEM;
+
+   hdr = skb_put(skb, sizeof(*hdr));
+   hdr->evt = HCI_EV_CMD_COMPLETE;
+   hdr->plen = sizeof(*evt) + 1;
+
+   evt = skb_put(skb, sizeof(*evt));
+   evt->ncmd = 1;
+   evt->opcode = HCI_OP_NOP;
+
+   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+   return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
  struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int qca_download_firmware(struct 
hci_dev *hdev,

ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
-   break;
+   goto out;
segment += segsize;
}
+	/* Latest qualcomm chipsets are not sending a command complete 
event
+	 * for every fw packet sent. They only respond with a vendor 
specific

+* event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 
command

+* complete event to avoid a command timeout error message.
+*/
+   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+   config->dnld_type == ROME_SKIP_EVT_VSE))
+   return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is
allowed for vendor OGF to use that command. I see you actually do use
it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip. it 
will not wait for the response for the command sent.


as you know that every vendor command sent to chip will respond with 
vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event 
only. so we are injecting command complete event.


and __hci_cmd_sync_ev is also not working for you? However since you
are not waiting for the vendor event anyway and just injecting
cmd_complete, I wonder what’s the difference in just using
__hci_cmd_send and not bothering to wait or inject at all. I am
failing to see where this injection makes a difference.

For me it is a big difference if we are injecting one event like in
the case of Intel compared to injecting one for every command. It will
show a wrong picture in btmon and that is a bad idea.

Regards

Marcel


[Bala]: here is the use case, when ever we download the fw packets i.e. 
RAM image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command 
complete event.


the above is taking more time to setup the BT device. then we came up 
with solution where we enable flags in fw file (i.e. RAM image header)
whether to wait for event to be received or sent the total packets and 
wait for the events for the last packet.


So currently we are handling both the cases in the code. i.e wait for 
event for all packet or wait for an event for the last packet.


but in the second case i.e. wait for event for the last packet sent, we 
are only receiving an vendor specific event from chip which holds the 
status of fw 

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Marcel Holtmann]

Hi Balakrishna,

>>> Latest qualcomm chips are not sending an command complete event for
>>> every firmware packet sent to chip. They only respond with a vendor
>>> specific event for the last firmware packet. This optimization will
>>> decrease the BT ON time. Due to this we are seeing a timeout error
>>> message logs on the console during firmware download. Now we are
>>> injecting a command complete event once we receive an vendor specific
>>> event for the last RAM firmware packet.
>>> Signed-off-by: Balakrishna Godavarthi 
>>> ---
>>> drivers/bluetooth/btqca.c | 39 ++-
>>> drivers/bluetooth/btqca.h |  3 +++
>>> 2 files changed, 41 insertions(+), 1 deletion(-)
>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>> index ec9e03a6b778..0b533f65f652 100644
>>> --- a/drivers/bluetooth/btqca.c
>>> +++ b/drivers/bluetooth/btqca.c
>>> @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config 
>>> *config,
>>>  * In case VSE is skipped, only the last segment is acked.
>>>  */
>>> config->dnld_mode = tlv_patch->download_mode;
>>> +   config->dnld_type = config->dnld_mode;
>>> BT_DBG("Total Length   : %d bytes",
>>>le32_to_cpu(tlv_patch->total_size));
>>> @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, 
>>> int seg_size,
>>> return err;
>>> }
>>> +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
>>> +{
>>> +   struct hci_event_hdr *hdr;
>>> +   struct hci_ev_cmd_complete *evt;
>>> +   struct sk_buff *skb;
>>> +
>>> +   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
>>> +   if (!skb)
>>> +   return -ENOMEM;
>>> +
>>> +   hdr = skb_put(skb, sizeof(*hdr));
>>> +   hdr->evt = HCI_EV_CMD_COMPLETE;
>>> +   hdr->plen = sizeof(*evt) + 1;
>>> +
>>> +   evt = skb_put(skb, sizeof(*evt));
>>> +   evt->ncmd = 1;
>>> +   evt->opcode = HCI_OP_NOP;
>>> +
>>> +   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
>>> +
>>> +   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
>>> +
>>> +   return hci_recv_frame(hdev, skb);
>>> +}
>>> +
>>> static int qca_download_firmware(struct hci_dev *hdev,
>>>   struct rome_config *config)
>>> {
>>> @@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>> ret = qca_tlv_send_segment(hdev, segsize, segment,
>>> config->dnld_mode);
>>> if (ret)
>>> -   break;
>>> +   goto out;
>>> segment += segsize;
>>> }
>>> +   /* Latest qualcomm chipsets are not sending a command complete event
>>> +* for every fw packet sent. They only respond with a vendor specific
>>> +* event for the last packet. This optimization in the chip will
>>> +* decrease the BT in initialization time. Here we will inject a command
>>> +* complete event to avoid a command timeout error message.
>>> +*/
>>> +   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
>>> +   config->dnld_type == ROME_SKIP_EVT_VSE))
>>> +   return qca_inject_cmd_complete_event(hdev);
>>> +
>> have you actually considered using __hci_cmd_send in that case. It is
>> allowed for vendor OGF to use that command. I see you actually do use
>> it and now I am failing to understand what this is for.
> [Bala]: thanks for reviewing the change.
> 
> __hci_cmd_send() can be used only to send the command to the chip. it will 
> not wait for the response for the command sent.
> 
> as you know that every vendor command sent to chip will respond with vendor 
> specific event and command complete event.
> but in our case chip will only respond with vendor specific event only. so we 
> are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not 
waiting for the vendor event anyway and just injecting cmd_complete, I wonder 
what’s the difference in just using __hci_cmd_send and not bothering to wait or 
inject at all. I am failing to see where this injection makes a difference.

For me it is a big difference if we are injecting one event like in the case of 
Intel compared to injecting one for every command. It will show a wrong picture 
in btmon and that is a bad idea.

Regards

Marcel

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-12-29 12:48, Marcel Holtmann wrote:

Hi Balakrishna,


Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by: Balakrishna Godavarthi 
---
drivers/bluetooth/btqca.c | 39 ++-
drivers/bluetooth/btqca.h |  3 +++
2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config 
*config,

 * In case VSE is skipped, only the last segment is acked.
 */
config->dnld_mode = tlv_patch->download_mode;
+   config->dnld_type = config->dnld_mode;

BT_DBG("Total Length   : %d bytes",
   le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
*hdev, int seg_size,

return err;
}

+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+   struct hci_event_hdr *hdr;
+   struct hci_ev_cmd_complete *evt;
+   struct sk_buff *skb;
+
+   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+   if (!skb)
+   return -ENOMEM;
+
+   hdr = skb_put(skb, sizeof(*hdr));
+   hdr->evt = HCI_EV_CMD_COMPLETE;
+   hdr->plen = sizeof(*evt) + 1;
+
+   evt = skb_put(skb, sizeof(*evt));
+   evt->ncmd = 1;
+   evt->opcode = HCI_OP_NOP;
+
+   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+   return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
  struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev 
*hdev,

ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
-   break;
+   goto out;

segment += segsize;
}

+   /* Latest qualcomm chipsets are not sending a command complete event
+	 * for every fw packet sent. They only respond with a vendor 
specific

+* event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 
command

+* complete event to avoid a command timeout error message.
+*/
+   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+   config->dnld_type == ROME_SKIP_EVT_VSE))
+   return qca_inject_cmd_complete_event(hdev);
+


have you actually considered using __hci_cmd_send in that case. It is
allowed for vendor OGF to use that command. I see you actually do use
it and now I am failing to understand what this is for.


[Bala]: thanks for reviewing the change.

 __hci_cmd_send() can be used only to send the command to the chip. it 
will not wait for the response for the command sent.


as you know that every vendor command sent to chip will respond with 
vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. 
so we are injecting command complete event.



Regards

Marcel


--
Regards
Balakrishna.

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Marcel Holtmann]

Hi Balakrishna,

> Latest qualcomm chips are not sending an command complete event for
> every firmware packet sent to chip. They only respond with a vendor
> specific event for the last firmware packet. This optimization will
> decrease the BT ON time. Due to this we are seeing a timeout error
> message logs on the console during firmware download. Now we are
> injecting a command complete event once we receive an vendor specific
> event for the last RAM firmware packet.
> 
> Signed-off-by: Balakrishna Godavarthi 
> ---
> drivers/bluetooth/btqca.c | 39 ++-
> drivers/bluetooth/btqca.h |  3 +++
> 2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index ec9e03a6b778..0b533f65f652 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
>* In case VSE is skipped, only the last segment is acked.
>*/
>   config->dnld_mode = tlv_patch->download_mode;
> + config->dnld_type = config->dnld_mode;
> 
>   BT_DBG("Total Length   : %d bytes",
>  le32_to_cpu(tlv_patch->total_size));
> @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, 
> int seg_size,
>   return err;
> }
> 
> +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> +{
> + struct hci_event_hdr *hdr;
> + struct hci_ev_cmd_complete *evt;
> + struct sk_buff *skb;
> +
> + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = skb_put(skb, sizeof(*hdr));
> + hdr->evt = HCI_EV_CMD_COMPLETE;
> + hdr->plen = sizeof(*evt) + 1;
> +
> + evt = skb_put(skb, sizeof(*evt));
> + evt->ncmd = 1;
> + evt->opcode = HCI_OP_NOP;
> +
> + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> +
> + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> +
> + return hci_recv_frame(hdev, skb);
> +}
> +
> static int qca_download_firmware(struct hci_dev *hdev,
> struct rome_config *config)
> {
> @@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
>   ret = qca_tlv_send_segment(hdev, segsize, segment,
>   config->dnld_mode);
>   if (ret)
> - break;
> + goto out;
> 
>   segment += segsize;
>   }
> 
> + /* Latest qualcomm chipsets are not sending a command complete event
> +  * for every fw packet sent. They only respond with a vendor specific
> +  * event for the last packet. This optimization in the chip will
> +  * decrease the BT in initialization time. Here we will inject a command
> +  * complete event to avoid a command timeout error message.
> +  */
> + if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> + config->dnld_type == ROME_SKIP_EVT_VSE))
> + return qca_inject_cmd_complete_event(hdev);
> +

have you actually considered using __hci_cmd_send in that case. It is allowed 
for vendor OGF to use that command. I see you actually do use it and now I am 
failing to understand what this is for.

Regards

Marcel

Re: [PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Matthias Kaehlcke]

On Fri, Dec 28, 2018 at 05:18:19PM +0530, Balakrishna Godavarthi wrote:
> Latest qualcomm chips are not sending an command complete event for
> every firmware packet sent to chip. They only respond with a vendor
> specific event for the last firmware packet. This optimization will
> decrease the BT ON time. Due to this we are seeing a timeout error
> message logs on the console during firmware download. Now we are
> injecting a command complete event once we receive an vendor specific
> event for the last RAM firmware packet.
> 
> Signed-off-by: Balakrishna Godavarthi 
> ---
>  drivers/bluetooth/btqca.c | 39 ++-
>  drivers/bluetooth/btqca.h |  3 +++
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index ec9e03a6b778..0b533f65f652 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
>* In case VSE is skipped, only the last segment is acked.
>*/
>   config->dnld_mode = tlv_patch->download_mode;
> + config->dnld_type = config->dnld_mode;
>  
>   BT_DBG("Total Length   : %d bytes",
>  le32_to_cpu(tlv_patch->total_size));
> @@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, 
> int seg_size,
>   return err;
>  }
>  
> +static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
> +{
> + struct hci_event_hdr *hdr;
> + struct hci_ev_cmd_complete *evt;
> + struct sk_buff *skb;
> +
> + skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = skb_put(skb, sizeof(*hdr));
> + hdr->evt = HCI_EV_CMD_COMPLETE;
> + hdr->plen = sizeof(*evt) + 1;
> +
> + evt = skb_put(skb, sizeof(*evt));
> + evt->ncmd = 1;
> + evt->opcode = HCI_OP_NOP;
> +
> + skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
> +
> + hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
> +
> + return hci_recv_frame(hdev, skb);
> +}
> +
>  static int qca_download_firmware(struct hci_dev *hdev,
> struct rome_config *config)
>  {
> @@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
>   ret = qca_tlv_send_segment(hdev, segsize, segment,
>   config->dnld_mode);
>   if (ret)
> - break;
> + goto out;
>  
>   segment += segsize;
>   }
>  
> + /* Latest qualcomm chipsets are not sending a command complete event
> +  * for every fw packet sent. They only respond with a vendor specific
> +  * event for the last packet. This optimization in the chip will
> +  * decrease the BT in initialization time. Here we will inject a command
> +  * complete event to avoid a command timeout error message.
> +  */
> + if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
> + config->dnld_type == ROME_SKIP_EVT_VSE))
> + return qca_inject_cmd_complete_event(hdev);
> +
> +out:
>   release_firmware(fw);
>  
>   return ret;
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 0c01f375fe83..5c8fc54133e3 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -40,6 +40,8 @@
>  #define QCA_WCN3990_POWERON_PULSE0xFC
>  #define QCA_WCN3990_POWEROFF_PULSE   0xC0
>  
> +#define QCA_HCI_CC_SUCCESS   0x00
> +
>  enum qca_bardrate {
>   QCA_BAUDRATE_115200 = 0,
>   QCA_BAUDRATE_57600,
> @@ -81,6 +83,7 @@ struct rome_config {
>   char fwname[64];
>   uint8_t user_baud_rate;
>   enum rome_tlv_dnld_mode dnld_mode;
> + enum rome_tlv_dnld_mode dnld_type;

The ->dnld_type vs ->dnld_mode is a bit confusing, I don't have a
better suggestion though.

>  };
>  
>  struct edl_event_hdr {

I'm not an expert in dealing with HCI SKBs, as far as I can tell the
above looks sane to me and I confirmed that it fixes the timeout when
downloading firmware.

Tested-by: Matthias Kaehlcke 
Reviewed-by: Matthias Kaehlcke 

[PATCH v7 4/4] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by: Balakrishna Godavarthi 
---
 drivers/bluetooth/btqca.c | 39 ++-
 drivers/bluetooth/btqca.h |  3 +++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
 * In case VSE is skipped, only the last segment is acked.
 */
config->dnld_mode = tlv_patch->download_mode;
+   config->dnld_type = config->dnld_mode;
 
BT_DBG("Total Length   : %d bytes",
   le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, int 
seg_size,
return err;
 }
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+   struct hci_event_hdr *hdr;
+   struct hci_ev_cmd_complete *evt;
+   struct sk_buff *skb;
+
+   skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+   if (!skb)
+   return -ENOMEM;
+
+   hdr = skb_put(skb, sizeof(*hdr));
+   hdr->evt = HCI_EV_CMD_COMPLETE;
+   hdr->plen = sizeof(*evt) + 1;
+
+   evt = skb_put(skb, sizeof(*evt));
+   evt->ncmd = 1;
+   evt->opcode = HCI_OP_NOP;
+
+   skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+   hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+   return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
  struct rome_config *config)
 {
@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
-   break;
+   goto out;
 
segment += segsize;
}
 
+   /* Latest qualcomm chipsets are not sending a command complete event
+* for every fw packet sent. They only respond with a vendor specific
+* event for the last packet. This optimization in the chip will
+* decrease the BT in initialization time. Here we will inject a command
+* complete event to avoid a command timeout error message.
+*/
+   if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+   config->dnld_type == ROME_SKIP_EVT_VSE))
+   return qca_inject_cmd_complete_event(hdev);
+
+out:
release_firmware(fw);
 
return ret;
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 0c01f375fe83..5c8fc54133e3 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -40,6 +40,8 @@
 #define QCA_WCN3990_POWERON_PULSE  0xFC
 #define QCA_WCN3990_POWEROFF_PULSE 0xC0
 
+#define QCA_HCI_CC_SUCCESS 0x00
+
 enum qca_bardrate {
QCA_BAUDRATE_115200 = 0,
QCA_BAUDRATE_57600,
@@ -81,6 +83,7 @@ struct rome_config {
char fwname[64];
uint8_t user_baud_rate;
enum rome_tlv_dnld_mode dnld_mode;
+   enum rome_tlv_dnld_mode dnld_type;
 };
 
 struct edl_event_hdr {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project