linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 1/3] Bluetooth: Use queue in the device list
@ 2011-11-16 15:30 Emeltchenko Andrei
  2011-11-16 15:30 ` [RFC 2/3] Bluetooth: remove old code Emeltchenko Andrei
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Emeltchenko Andrei @ 2011-11-16 15:30 UTC (permalink / raw)
  To: linux-bluetooth

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Use queue instead of stack discipline for device list. When processing
dev_list with list_for_each* devices will be prosessed in order they
were added (Usually BR/EDR first and AMP later).

Also output from hciconfig looks nicer :-)

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cf18f6d..aee76fd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1452,7 +1452,7 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add(&hdev->list, head);
+	list_add_tail(&hdev->list, head);
 
 	atomic_set(&hdev->refcnt, 1);
 	spin_lock_init(&hdev->lock);
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [RFC 3/3] Bluetooth: Add AMP initialization
@ 2011-11-21 16:23 james.steele
  2011-11-21 16:53 ` Marcel Holtmann
  0 siblings, 1 reply; 20+ messages in thread
From: james.steele @ 2011-11-21 16:23 UTC (permalink / raw)
  To: marcel, Andrei.Emeltchenko.news; +Cc: pkrystad, linux-bluetooth

[-- Attachment #1: Type: text/plain, Size: 7013 bytes --]

Hi Marcel, Andrei,

>> > > > However since we are talking about controller init. Can someone please
>> > > > first propose a list of how we init AMP vs BR/EDR/LE controllers instead
>> > > > of just dumping code here. I think it needs some clear strategy on how
>> > > > to do it.
>> > >
>> > > AMP controller is initialized following way in Code Aurora code:
>> > >
>> > > Common part:
>> > >
>> > > <------8<--------------------------------------------------
>> > > |  /* Mandatory initialization */
>> > > |
>> > > |  /* Reset */
>> > > |  if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
>> > > |          set_bit(HCI_RESET, &hdev->flags);
>> > > |          hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
>> > > |  }
>> >
>> > this check for QUIRK_NO_RESET is pointless. That is just there for old
>> > Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
>> > controllers are fine and just issue OP_RESET.
>> >
>> > > |  /* Read Local Version */
>> > > |  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
>> > > |
>> > > |  /* Read HCI Flow Control Mode */
>> > > |  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
>> > > |
>> > > |  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
>> > > |  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
>> > > |
>> > > |  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
>> > > |  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
>> > > <------8<------------------------------------------------------
>> >
>> > This is not a generic sequence. We will not be reading flow control or
>> > data block size commands. It will heavily fail on older controllers.
>>
>> What about following code (basically BR/EDR initialization is the same as
>> now and only amp_init is new). I use now virtual AMP device so any init is
>> good, I need Read Version and Read AMP info commands.
>>
>> static void bredr_init(struct hci_dev *hdev)
>> {
>>       struct hci_cp_delete_stored_link_key cp;
>>       __le16 param;
>>       __u8 flt_type;
>>
>>       /* Mandatory initialization */
>>       /* Reset */
>>       if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
>>               set_bit(HCI_RESET, &hdev->flags);
>>               hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
>>       }
>>
>>       /* Read Local Supported Features */
>>       hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
>>
>>       /* Read Local Version */
>>       hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
>>
>>       /* Read Buffer Size (ACL mtu, max pkt, etc.) */
>>       hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
>>
>>       /* Read BD Address */
>>       hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
>>
>>       /* Read Class of Device */
>>       hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
>>
>>       /* Read Local Name */
>>       hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
>>
>>       /* Read Voice Setting */
>>       hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
>>
>>       /* Optional initialization */
>>       /* Clear Event Filters */
>>       flt_type = HCI_FLT_CLEAR_ALL;
>>       hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
>>
>>       /* Connection accept timeout ~20 secs */
>>       param = cpu_to_le16(0x7d00);
>>       hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
>>
>>       bacpy(&cp.bdaddr, BDADDR_ANY);
>>       cp.delete_all = 1;
>>       hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
>> }
>>
>> static void amp_init(struct hci_dev *hdev)
>> {
>>       /* Mandatory initialization */
>>       /* Reset */
>>       hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
>>
>>       /* Read Local Version */
>>       hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
>>
>>       /* Read Buffer Size (ACL mtu, max pkt, etc.) */
>>       hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
>>
>>       /* Read AMP Info */
>>       hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
>> }
>
> actually I think the READ_AMP_INFO is wrong here. It should be only
> executed based on an AMP_Get_Info_Request during a connection setup
> procedure.

You can read the AMP info over HCI upon receiving a request by A2MP, but there are reasons to issue the READ_AMP_INFO command in advance.  For example, you need the Controller_Type for responding to the AMP_Discover_Request.  Also you may (tenuously) want the know the Max_PDU_Size in advance for selecting SDU sizes (w.r.t. segmentation).

Certainly READ_AMP_INFO is a command intended to be issued during controller initialization, the majority of the values should not change during the lifetime of the controller, and only once that command has been issued will the Controller commence sending AMP_Status_Change_Events to communicate changes in status.  At a minimum READ_AMP_INFO must be issued for each AMP_Discover_Request (and AMP_Get_Info_Request) received; but if AMP discovery is slower, then transitioning a channel to an AMP link will be slower.

>
> And even the READ_BUFFER_SIZE is the wrong command since by default the
> AMP controller is in block based flow control mode. So you would need to
> read the flow control mode first actually.

Correct, although reading the flow control mode on an AMP controller after a HCI_Reset *should* always return "data block mode" (as that is the default).  The READ_BUFFER_SIZE should only be used if HCI_Write_Flow_Control_Mode has been issued first (and successfully enabled packet based flow control).  Instead I believe we should issue the HCI_Read_Data_Block_Size command, where the correct information is then provided for the host stack (with its "new" data block flow control model for AMP controllers).  I don't think there is any requirement forcing an AMP controller to implement packet based flow control, so if there is no data block flow control model in the host yet then initialization has to be prepared to fail upon trying to configure packet based flow control mode.

Also for AMP Controller initialization, the HCI_Set_Event_Mask_Page_2 command should be issued to ensure the AMP related events are provided by the controller (by default they are turned off).  A suitable HCI_Set_Event_Mask command may also be needed if using enhanced flush on the AMP controller.  Event filters also apply to AMP controllers IIRC, so if following the BR/EDR example you may want to clear them too (although I imagine such initialization could be added when required).

Regards,
James


________________________________
Subject to local law, communications with Accenture and its affiliates including telephone calls and emails (including content), may be monitored by our systems for the purposes of security and the assessment of internal compliance with Accenture policy.
______________________________________________________________________________________

www.accenture.com

[-- Attachment #2: Type: text/html, Size: 12987 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2011-11-23 14:51 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-16 15:30 [RFC 1/3] Bluetooth: Use queue in the device list Emeltchenko Andrei
2011-11-16 15:30 ` [RFC 2/3] Bluetooth: remove old code Emeltchenko Andrei
2011-11-16 20:44   ` Gustavo Padovan
2011-11-16 21:47     ` Marcel Holtmann
2011-11-16 15:30 ` [RFC 3/3] Bluetooth: Add AMP initialization Emeltchenko Andrei
2011-11-16 21:49   ` Marcel Holtmann
2011-11-17 13:20     ` Emeltchenko Andrei
2011-11-17 15:45       ` Marcel Holtmann
2011-11-17 18:17         ` Peter Krystad
2011-11-18  5:14           ` Marcel Holtmann
2011-11-18 13:54             ` 'Emeltchenko Andrei'
2011-11-18 16:23               ` Marcel Holtmann
2011-11-21 10:13                 ` 'Emeltchenko Andrei'
2011-11-21 13:00                   ` Marcel Holtmann
2011-11-16 21:48 ` [RFC 1/3] Bluetooth: Use queue in the device list Marcel Holtmann
2011-11-21 16:46 ` Gustavo Padovan
  -- strict thread matches above, loose matches on Subject: below --
2011-11-21 16:23 [RFC 3/3] Bluetooth: Add AMP initialization james.steele
2011-11-21 16:53 ` Marcel Holtmann
2011-11-23 10:04   ` Andrei Emeltchenko
2011-11-23 14:51     ` Marcel Holtmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).