[PATCH 1/2] Bluetooth: Fix request completion for command status events

[PATCH 1/2] Bluetooth: Fix request completion for command status events

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

If a HCI command triggered by hci_request() fails at the command status
phase we need to properly inform the request tracking code of the
completion of the request.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dfe6fbc..236f895 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2055,6 +2055,9 @@ static inline void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	opcode = __le16_to_cpu(ev->opcode);
 
+	if (ev->status != 0)
+		hci_req_complete(hdev, opcode, ev->status);
+
 	switch (opcode) {
 	case HCI_OP_INQUIRY:
 		hci_cs_inquiry(hdev, ev->status);
-- 
1.7.7.3

[PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
even though (according to the specification) they should support it.
Since this HCI command is part of the controller init sequence
controllers broken in this manner would be unusable. Since not having
the list of supported commands is not critical for these controllers it
is better to just ignore failures in this case (additionally, this is
also how older kernels used to behave).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9218888..1719257 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -106,6 +106,13 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
 	if (test_bit(HCI_INIT, &hdev->flags) && hdev->init_last_cmd != cmd)
 		return;
 
+	/* Some 1.2 controllers report failure for HCI_Read_Local_Commands
+	 * even though they should support it. Since it's not a critical
+	 * issue not to have a proper result from them just ignore a
+	 * failure status */
+	if (cmd == HCI_OP_READ_LOCAL_COMMANDS && result != 0)
+		result = 0;
+
 	if (hdev->req_status == HCI_REQ_PEND) {
 		hdev->req_result = result;
 		hdev->req_status = HCI_REQ_DONE;
-- 
1.7.7.3

Re: [PATCH 1/2] Bluetooth: Fix request completion for command status events

[Marcel Holtmann]

Hi Johan,

> If a HCI command triggered by hci_request() fails at the command status
> phase we need to properly inform the request tracking code of the
> completion of the request.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_event.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index dfe6fbc..236f895 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2055,6 +2055,9 @@ static inline void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	opcode = __le16_to_cpu(ev->opcode);
>  
> +	if (ev->status != 0)
> +		hci_req_complete(hdev, opcode, ev->status);
> +

we actually would write it "if (!ev->status)" in the kernel code.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Marcel Holtmann]

Hi Johan,

> Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> even though (according to the specification) they should support it.
> Since this HCI command is part of the controller init sequence
> controllers broken in this manner would be unusable. Since not having
> the list of supported commands is not critical for these controllers it
> is better to just ignore failures in this case (additionally, this is
> also how older kernels used to behave).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 9218888..1719257 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -106,6 +106,13 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
>  	if (test_bit(HCI_INIT, &hdev->flags) && hdev->init_last_cmd != cmd)
>  		return;
>  
> +	/* Some 1.2 controllers report failure for HCI_Read_Local_Commands
> +	 * even though they should support it. Since it's not a critical
> +	 * issue not to have a proper result from them just ignore a
> +	 * failure status */
> +	if (cmd == HCI_OP_READ_LOCAL_COMMANDS && result != 0)
> +		result = 0;
> +

this is pretty ugly handling. Can't we come up with something better?

Regards

Marcel

[PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
even though (according to the specification) they should support it.
Since this HCI command is part of the controller init sequence
controllers broken in this manner would be unusable. Since not having
the list of supported commands is not critical for these controllers it
is better to just ignore failures in this case (additionally, this is
also how older kernels used to behave).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index af24b3d..e950865 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1400,6 +1400,18 @@ static void hci_cs_le_start_enc(struct hci_dev *hdev, u8 status)
 	BT_DBG("%s status 0x%x", hdev->name, status);
 }
 
+static void hci_cs_read_local_commands(struct hci_dev *hdev, u8 status)
+{
+	if (!status)
+		return;
+
+	/* Some 1.2 controllers report failure for HCI_Read_Local_Commands
+	 * even though they should support it. Since it's not a critical
+	 * issue not to have a proper result from them just ignore a
+	 * failure status (i.e. consider it a success) */
+	hci_req_complete(hdev, HCI_OP_READ_LOCAL_COMMANDS, 0);
+}
+
 static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -2113,6 +2125,10 @@ static inline void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_le_start_enc(hdev, ev->status);
 		break;
 
+	case HCI_OP_READ_LOCAL_COMMANDS:
+		hci_cs_read_local_commands(hdev, ev->status);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%x", hdev->name, opcode);
 		if (ev->status)
-- 
1.7.7.3

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Johan Hedberg]

Hi Marcel,

On Mon, Nov 21, 2011, Marcel Holtmann wrote:
> > Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> > even though (according to the specification) they should support it.
> > Since this HCI command is part of the controller init sequence
> > controllers broken in this manner would be unusable. Since not having
> > the list of supported commands is not critical for these controllers it
> > is better to just ignore failures in this case (additionally, this is
> > also how older kernels used to behave).
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> >  net/bluetooth/hci_core.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 9218888..1719257 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -106,6 +106,13 @@ void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
> >  	if (test_bit(HCI_INIT, &hdev->flags) && hdev->init_last_cmd != cmd)
> >  		return;
> >  
> > +	/* Some 1.2 controllers report failure for HCI_Read_Local_Commands
> > +	 * even though they should support it. Since it's not a critical
> > +	 * issue not to have a proper result from them just ignore a
> > +	 * failure status */
> > +	if (cmd == HCI_OP_READ_LOCAL_COMMANDS && result != 0)
> > +		result = 0;
> > +
> 
> this is pretty ugly handling. Can't we come up with something better?

I just sent a second attempt at both patches. They accomplish the same
thing but since we now do the handling inside the cmd_status switch
statement the code becomes less hackish (at least in my opinion).

Johan

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Gustavo Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2011-11-21 20:01:24 +0200]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> even though (according to the specification) they should support it.
> Since this HCI command is part of the controller init sequence
> controllers broken in this manner would be unusable. Since not having
> the list of supported commands is not critical for these controllers it
> is better to just ignore failures in this case (additionally, this is
> also how older kernels used to behave).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_event.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)

Both patches have been applied, thanks.

	Gustavo

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Marcel Holtmann]

Hi Gustavo,

> > Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> > even though (according to the specification) they should support it.
> > Since this HCI command is part of the controller init sequence
> > controllers broken in this manner would be unusable. Since not having
> > the list of supported commands is not critical for these controllers it
> > is better to just ignore failures in this case (additionally, this is
> > also how older kernels used to behave).
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> >  net/bluetooth/hci_event.c |   16 ++++++++++++++++
> >  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> Both patches have been applied, thanks.

I would have preferred you didn't. Since now I have to get this all
fixed up as well.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2011-12-01 14:09:33 +0100]:

> Hi Gustavo,
> 
> > > Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> > > even though (according to the specification) they should support it.
> > > Since this HCI command is part of the controller init sequence
> > > controllers broken in this manner would be unusable. Since not having
> > > the list of supported commands is not critical for these controllers it
> > > is better to just ignore failures in this case (additionally, this is
> > > also how older kernels used to behave).
> > > 
> > > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > > ---
> > >  net/bluetooth/hci_event.c |   16 ++++++++++++++++
> > >  1 files changed, 16 insertions(+), 0 deletions(-)
> > 
> > Both patches have been applied, thanks.
> 
> I would have preferred you didn't. Since now I have to get this all
> fixed up as well.

Is this for both of them?

	Gustavo

Re: [PATCH 2/2] Bluetooth: Ignore HCI_Read_Local_Commands failures

[Marcel Holtmann]

Hi Gustavo,

> > > > Some 1.2 controllers will fail the HCI_Read_Local_Commands HCI command
> > > > even though (according to the specification) they should support it.
> > > > Since this HCI command is part of the controller init sequence
> > > > controllers broken in this manner would be unusable. Since not having
> > > > the list of supported commands is not critical for these controllers it
> > > > is better to just ignore failures in this case (additionally, this is
> > > > also how older kernels used to behave).
> > > > 
> > > > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > > > ---
> > > >  net/bluetooth/hci_event.c |   16 ++++++++++++++++
> > > >  1 files changed, 16 insertions(+), 0 deletions(-)
> > > 
> > > Both patches have been applied, thanks.
> > 
> > I would have preferred you didn't. Since now I have to get this all
> > fixed up as well.
> 
> Is this for both of them?

yes, we need to stop hacking around here and once and for all solve this
properly.

Regards

Marcel