Hi Alain,
>>> This patch implements the read default system parameters and the set
>>> default system parameters mgmt commands.
>>>
>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
>>> Signed-off-by: Alain Michaud <alainm@xxxxxxxxxxxx>
>>> ---
>>>
>>> net/bluetooth/mgmt.c | 429 +++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 429 insertions(+)
>>
>> I have the feeling that we want to create a net/bluetooth/mgmt_config.c to move all the configuration parameters parsing and handling into a separate. I have the feeling that otherwise mgmt.c will grow quickly.
> ok, I'll send out a v2 that split it out in mgmt_config.c/h
>
>>
>>>
>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> index 9e8a3cccc6ca..98a4193c8e66 100644
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -111,6 +111,8 @@ static const u16 mgmt_commands[] = {
>>> MGMT_OP_READ_SECURITY_INFO,
>>> MGMT_OP_READ_EXP_FEATURES_INFO,
>>> MGMT_OP_SET_EXP_FEATURE,
>>> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
>>> + MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS,
>>> };
>>
>> Please also add the OP_READ_ command to mgmt_untrusted_commands array.
> ACK.
>>
>>> static const u16 mgmt_events[] = {
>>> @@ -3849,6 +3851,431 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
>>> MGMT_STATUS_NOT_SUPPORTED);
>>> }
>>>
>>> +static int read_default_system_parameters(struct sock *sk, struct hci_dev *hdev,
>>> + void *data, u16 data_len)
>>> +{
>>> + struct {
>>> + struct mgmt_system_parameter_tlv entry;
>>> + union {
>>> + /* This is a simplification for now since all values
>>> + * are 16 bits. In the future, this code may need
>>> + * refactoring to account for variable length values
>>> + * and properly calculate the required buffer size.
>>> + */
>>> + u16 value;
>>> + };
>>> + } __packed params[] = {
>>> + /* Please see mgmt-api.txt for documentation of these values */
>>> + {
>>> + { 0x0000, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_type) }
>>> + },
>>> + {
>>> + { 0x0001, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_int) }
>>> + },
>>> + {
>>> + { 0x0002, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_scan_window) }
>>> + },
>>> + {
>>> + { 0x0003, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_type) }
>>> + },
>>> + {
>>> + { 0x0004, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_int) }
>>> + },
>>> + {
>>> + { 0x0005, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_inq_scan_window) }
>>> + },
>>> + {
>>> + { 0x0006, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_br_lsto) }
>>> + },
>>> + {
>>> + { 0x0007, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_page_timeout) }
>>> + },
>>> + {
>>> + { 0x0008, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->sniff_min_interval) }
>>> + },
>>> + {
>>> + { 0x0009, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->sniff_max_interval) }
>>> + },
>>> + {
>>> + { 0x000a, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_adv_min_interval) }
>>> + },
>>> + {
>>> + { 0x000b, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_adv_max_interval) }
>>> + },
>>> + {
>>> + { 0x000c, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->def_multi_adv_rotation_duration) }
>>> + },
>>> + {
>>> + { 0x000d, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_interval) }
>>> + },
>>> + {
>>> + { 0x000e, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window) }
>>> + },
>>> + {
>>> + { 0x000f, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_suspend) }
>>> + },
>>> + {
>>> + { 0x0010, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_suspend) }
>>> + },
>>> + {
>>> + { 0x0011, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_discovery) }
>>> + },
>>> + {
>>> + { 0x0012, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_discovery) }
>>> + },
>>> + {
>>> + { 0x0013, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_adv_monitor) }
>>> + },
>>> + {
>>> + { 0x0014, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_adv_monitor) }
>>> + },
>>> + {
>>> + { 0x0015, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_int_connect) }
>>> + },
>>> + {
>>> + { 0x0016, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_scan_window_connect) }
>>> + },
>>> + {
>>> + { 0x0017, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_min_interval) }
>>> + },
>>> + {
>>> + { 0x0018, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_max_interval) }
>>> + },
>>> + {
>>> + { 0x0019, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_conn_latency) }
>>> + },
>>> + {
>>> + { 0x001a, sizeof(__u16) },
>>> + { cpu_to_le16(hdev->le_supv_timeout) }
>>> + },
>>
>> This is creative and I like it. However I would take it one step further. Define a macro similar USB_DEVICE etc. so you get a simpler table.
>>
>> { HDEV_PARAM(0x0018, le_conn_max_interval) },
>> { HDEV_PARAM(0x0019, le_conn_latency) },
>> { }
>>
>>
> Ok, that makes sense, I'll think of something for v2.
>
>>> + };
>>> + struct mgmt_rp_read_default_system_parameters *rp = (void *)params;
>>> +
>>> + bt_dev_dbg(hdev, "sock %p", sk);
>>> +
>>> + return mgmt_cmd_complete(sk, hdev->id,
>>> + MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS,
>>> + 0, rp, sizeof(params));
>>
>> Please introduce HCI_MGMT_SYSTEM_CONFIG_EVENTS and send them out as described in mgmt-api.txt
> Looks like you added this after my original proposal to mgmt-api.txt.
> I'd like to propose this is also implemented in a separate patch.
feel free to add it as a separate patch. See the Experimental Feature commands + event on how this could be done.
Regards
Marcel
