Re: [PATCH V2 5/6] rtlwifi: btcoexist: Add routines for RTL8812AE kernel socket communications

Re: [PATCH V2 5/6] rtlwifi: btcoexist: Add routines for RTL8812AE kernel socket communications

[Kalle Valo]

Hi,

I'm adding bluetooth list to the discussion. Full patch is available
here:

https://patchwork.kernel.org/patch/5712591/

Larry Finger <Larry.Finger@lwfinger.net> writes:

> From: Troy Tan <troy_tan@realsil.com.cn>
>
> This patch adds the routines used to communicate between the RTL8812AE (wifi)
> device and the RTL8761AU (bluetooth) device that are part of the same chip.
> Unlike other similar dual-function devices, this chip does not contain special
> hardware that lets the firmware pass coexistence info from one part to the
> other. As a result, this driver implements such communication as a kernel
> socket.
>
> Signed-off-by: Troy Tan <troy_tan@realsil.com.cn>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> V2 - Add comments explaining the routine that sends a message via the
> socket.

The commit log is not still explaining that much about the actual
functionality, so I investigated on my own:

> +static u8 rtl_btcoex_create_kernel_socket(struct rtl_priv *rtlpriv,
> +					  u8 is_invite)
> +{
> +	struct bt_coex_info *pcoex_info = &rtlpriv->coex_info;
> +	s8 kernel_socket_err;
> +
> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
> +		  "%s CONNECT_PORT %d\n", __func__, CONNECT_PORT);
> +
> +	if (!pcoex_info) {
> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL, "coex_info: NULL\n");
> +		return _FAIL;
> +	}
> +
> +	kernel_socket_err = sock_create(PF_INET, SOCK_DGRAM, 0,
> +					&pcoex_info->udpsock);
> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
> +		  "binding socket, err = %d\n", kernel_socket_err);
> +
> +	if (kernel_socket_err < 0) {
> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
> +			  "Error during creation of socket error:%d\n",
> +			  kernel_socket_err);
> +		return _FAIL;
> +	}
> +	memset(&pcoex_info->sin, 0, sizeof(pcoex_info->sin));
> +	pcoex_info->sin.sin_family = AF_INET;
> +	pcoex_info->sin.sin_port = htons(CONNECT_PORT);
> +	pcoex_info->sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +
> +	memset(&pcoex_info->bt_addr, 0, sizeof(pcoex_info->bt_addr));
> +	pcoex_info->bt_addr.sin_family = AF_INET;
> +	pcoex_info->bt_addr.sin_port = htons(CONNECT_PORT_BT);
> +	pcoex_info->bt_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +
> +	pcoex_info->sk_store = NULL;
> +
> +	kernel_socket_err =
> +	   pcoex_info->udpsock->ops->bind(pcoex_info->udpsock,
> +					  (struct sockaddr *)&pcoex_info->sin,
> +					  sizeof(pcoex_info->sin));
> +	if (kernel_socket_err == 0) {
> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
> +			  "binding socket success\n");
> +		pcoex_info->udpsock->sk->sk_data_ready =
> +			rtl_btcoex_recvmsg_int;
> +		pcoex_info->sock_open |=  KERNEL_SOCKET_OK;
> +		pcoex_info->bt_attend = false;
> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
> +			  "WIFI sending attend_req\n");
> +		rtl_btcoex_sendmsgbysocket(rtlpriv, attend_req,
> +					   sizeof(attend_req), true);
> +		return _SUCCESS;

So the wireless driver communicates with the bluetooth driver (which is
not in upstream) via a localhost UDP connection?

> +#define CONNECT_PORT 30000
> +#define CONNECT_PORT_BT 30001

And these are the UDP ports used.

> +struct hci_link_info {
> +	u16		connect_handle;
> +	u8		incoming_traffic_mode;
> +	u8		outgoing_traffic_mode;
> +	u8		bt_profile;
> +	u8		bt_corespec;
> +	s8		bt_RSSI;
> +	u8		traffic_profile;
> +	u8		link_role;
> +};
> +
> +#define	MAX_BT_ACL_LINK_NUM		8
> +
> +struct hci_ext_config {
> +	struct hci_link_info	acl_link[MAX_BT_ACL_LINK_NUM];
> +	u8	bt_operation_code;
> +	u16	current_connect_handle;
> +	u8	current_incoming_traffic_mode;
> +	u8	current_outgoing_traffic_mode;
> +
> +	u8	number_of_acl;
> +	u8	number_of_sco;
> +	u8	current_bt_status;
> +	u16	hci_ext_ver;
> +	bool	enable_wifi_scan_notify;
> +};

[...]

> +enum HCI_STATUS {
> +	/* Success */
> +	HCI_STATUS_SUCCESS				= 0x00,
> +	/* Unknown HCI Command */
> +	HCI_STATUS_UNKNOW_HCI_CMD			= 0x01,
> +	/* Unknown Connection Identifier */
> +	HCI_STATUS_UNKNOW_CONNECT_ID			= 0X02,
> +	/* Hardware Failure */
> +	HCI_STATUS_HW_FAIL				= 0X03,
> +	/* Page Timeout */
> +	HCI_STATUS_PAGE_TIMEOUT				= 0X04,
> +	/* Authentication Failure */
> +	HCI_STATUS_AUTH_FAIL				= 0X05,
> +	/* PIN or Key Missing */
> +	HCI_STATUS_PIN_OR_KEY_MISSING			= 0X06,
> +	/* Memory Capacity Exceeded */
> +	HCI_STATUS_MEM_CAP_EXCEED			= 0X07,
> +	/* Connection Timeout */
> +	HCI_STATUS_CONNECT_TIMEOUT			= 0X08,
> +	/* Connection Limit Exceeded */

And here's part of the information exchanged between the drivers.

This is something which needs to be properly reviewed both in wireless
and bluetooth lists, a wireless driver cannot just invent these on their
own. And using UDP sockets for this is in my opinion just horrible.

I know there's a general need for something similar like this, but it
needs to properly discussed and designed.

Comments?

-- 
Kalle Valo

Re: [PATCH V2 5/6] rtlwifi: btcoexist: Add routines for RTL8812AE kernel socket communications

[Marcel Holtmann]

Hi Kalle,

> I'm adding bluetooth list to the discussion. Full patch is available
> here:
> 
> https://patchwork.kernel.org/patch/5712591/
> 
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
>> From: Troy Tan <troy_tan@realsil.com.cn>
>> 
>> This patch adds the routines used to communicate between the RTL8812AE (wifi)
>> device and the RTL8761AU (bluetooth) device that are part of the same chip.
>> Unlike other similar dual-function devices, this chip does not contain special
>> hardware that lets the firmware pass coexistence info from one part to the
>> other. As a result, this driver implements such communication as a kernel
>> socket.
>> 
>> Signed-off-by: Troy Tan <troy_tan@realsil.com.cn>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> ---
>> V2 - Add comments explaining the routine that sends a message via the
>> socket.
> 
> The commit log is not still explaining that much about the actual
> functionality, so I investigated on my own:
> 
>> +static u8 rtl_btcoex_create_kernel_socket(struct rtl_priv *rtlpriv,
>> +					  u8 is_invite)
>> +{
>> +	struct bt_coex_info *pcoex_info = &rtlpriv->coex_info;
>> +	s8 kernel_socket_err;
>> +
>> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>> +		  "%s CONNECT_PORT %d\n", __func__, CONNECT_PORT);
>> +
>> +	if (!pcoex_info) {
>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL, "coex_info: NULL\n");
>> +		return _FAIL;
>> +	}
>> +
>> +	kernel_socket_err = sock_create(PF_INET, SOCK_DGRAM, 0,
>> +					&pcoex_info->udpsock);
>> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>> +		  "binding socket, err = %d\n", kernel_socket_err);
>> +
>> +	if (kernel_socket_err < 0) {
>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>> +			  "Error during creation of socket error:%d\n",
>> +			  kernel_socket_err);
>> +		return _FAIL;
>> +	}
>> +	memset(&pcoex_info->sin, 0, sizeof(pcoex_info->sin));
>> +	pcoex_info->sin.sin_family = AF_INET;
>> +	pcoex_info->sin.sin_port = htons(CONNECT_PORT);
>> +	pcoex_info->sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
>> +
>> +	memset(&pcoex_info->bt_addr, 0, sizeof(pcoex_info->bt_addr));
>> +	pcoex_info->bt_addr.sin_family = AF_INET;
>> +	pcoex_info->bt_addr.sin_port = htons(CONNECT_PORT_BT);
>> +	pcoex_info->bt_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
>> +
>> +	pcoex_info->sk_store = NULL;
>> +
>> +	kernel_socket_err =
>> +	   pcoex_info->udpsock->ops->bind(pcoex_info->udpsock,
>> +					  (struct sockaddr *)&pcoex_info->sin,
>> +					  sizeof(pcoex_info->sin));
>> +	if (kernel_socket_err == 0) {
>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>> +			  "binding socket success\n");
>> +		pcoex_info->udpsock->sk->sk_data_ready =
>> +			rtl_btcoex_recvmsg_int;
>> +		pcoex_info->sock_open |=  KERNEL_SOCKET_OK;
>> +		pcoex_info->bt_attend = false;
>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>> +			  "WIFI sending attend_req\n");
>> +		rtl_btcoex_sendmsgbysocket(rtlpriv, attend_req,
>> +					   sizeof(attend_req), true);
>> +		return _SUCCESS;
> 
> So the wireless driver communicates with the bluetooth driver (which is
> not in upstream) via a localhost UDP connection?

I think the first order of business should be to get the Bluetooth driver upstream. Until that has happened this is all kinda pointless discussion.

>> +#define CONNECT_PORT 30000
>> +#define CONNECT_PORT_BT 30001
> 
> And these are the UDP ports used.
> 
>> +struct hci_link_info {
>> +	u16		connect_handle;
>> +	u8		incoming_traffic_mode;
>> +	u8		outgoing_traffic_mode;
>> +	u8		bt_profile;
>> +	u8		bt_corespec;
>> +	s8		bt_RSSI;
>> +	u8		traffic_profile;
>> +	u8		link_role;
>> +};
>> +
>> +#define	MAX_BT_ACL_LINK_NUM		8
>> +
>> +struct hci_ext_config {
>> +	struct hci_link_info	acl_link[MAX_BT_ACL_LINK_NUM];
>> +	u8	bt_operation_code;
>> +	u16	current_connect_handle;
>> +	u8	current_incoming_traffic_mode;
>> +	u8	current_outgoing_traffic_mode;
>> +
>> +	u8	number_of_acl;
>> +	u8	number_of_sco;
>> +	u8	current_bt_status;
>> +	u16	hci_ext_ver;
>> +	bool	enable_wifi_scan_notify;
>> +};
> 
> [...]
> 
>> +enum HCI_STATUS {
>> +	/* Success */
>> +	HCI_STATUS_SUCCESS				= 0x00,
>> +	/* Unknown HCI Command */
>> +	HCI_STATUS_UNKNOW_HCI_CMD			= 0x01,
>> +	/* Unknown Connection Identifier */
>> +	HCI_STATUS_UNKNOW_CONNECT_ID			= 0X02,
>> +	/* Hardware Failure */
>> +	HCI_STATUS_HW_FAIL				= 0X03,
>> +	/* Page Timeout */
>> +	HCI_STATUS_PAGE_TIMEOUT				= 0X04,
>> +	/* Authentication Failure */
>> +	HCI_STATUS_AUTH_FAIL				= 0X05,
>> +	/* PIN or Key Missing */
>> +	HCI_STATUS_PIN_OR_KEY_MISSING			= 0X06,
>> +	/* Memory Capacity Exceeded */
>> +	HCI_STATUS_MEM_CAP_EXCEED			= 0X07,
>> +	/* Connection Timeout */
>> +	HCI_STATUS_CONNECT_TIMEOUT			= 0X08,
>> +	/* Connection Limit Exceeded */
> 
> And here's part of the information exchanged between the drivers.
> 
> This is something which needs to be properly reviewed both in wireless
> and bluetooth lists, a wireless driver cannot just invent these on their
> own. And using UDP sockets for this is in my opinion just horrible.
> 
> I know there's a general need for something similar like this, but it
> needs to properly discussed and designed.

This is just insane. Clear NAK.

Two kernel modules will not use UDP ports over the loopback interface to communicate with each other.

Regards

Marcel

Re: [PATCH V2 5/6] rtlwifi: btcoexist: Add routines for RTL8812AE kernel socket communications

[Larry Finger]

On 01/30/2015 03:57 AM, Marcel Holtmann wrote:

Marcel,

> Hi Kalle,
>
>> I'm adding bluetooth list to the discussion. Full patch is available
>> here:
>>
>> https://patchwork.kernel.org/patch/5712591/
>>
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> From: Troy Tan <troy_tan@realsil.com.cn>
>>>
>>> This patch adds the routines used to communicate between the RTL8812AE (wifi)
>>> device and the RTL8761AU (bluetooth) device that are part of the same chip.
>>> Unlike other similar dual-function devices, this chip does not contain special
>>> hardware that lets the firmware pass coexistence info from one part to the
>>> other. As a result, this driver implements such communication as a kernel
>>> socket.
>>>
>>> Signed-off-by: Troy Tan <troy_tan@realsil.com.cn>
>>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>>> ---
>>> V2 - Add comments explaining the routine that sends a message via the
>>> socket.
>>
>> The commit log is not still explaining that much about the actual
>> functionality, so I investigated on my own:
>>
>>> +static u8 rtl_btcoex_create_kernel_socket(struct rtl_priv *rtlpriv,
>>> +					  u8 is_invite)
>>> +{
>>> +	struct bt_coex_info *pcoex_info = &rtlpriv->coex_info;
>>> +	s8 kernel_socket_err;
>>> +
>>> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>>> +		  "%s CONNECT_PORT %d\n", __func__, CONNECT_PORT);
>>> +
>>> +	if (!pcoex_info) {
>>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL, "coex_info: NULL\n");
>>> +		return _FAIL;
>>> +	}
>>> +
>>> +	kernel_socket_err = sock_create(PF_INET, SOCK_DGRAM, 0,
>>> +					&pcoex_info->udpsock);
>>> +	BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>>> +		  "binding socket, err = %d\n", kernel_socket_err);
>>> +
>>> +	if (kernel_socket_err < 0) {
>>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>>> +			  "Error during creation of socket error:%d\n",
>>> +			  kernel_socket_err);
>>> +		return _FAIL;
>>> +	}
>>> +	memset(&pcoex_info->sin, 0, sizeof(pcoex_info->sin));
>>> +	pcoex_info->sin.sin_family = AF_INET;
>>> +	pcoex_info->sin.sin_port = htons(CONNECT_PORT);
>>> +	pcoex_info->sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
>>> +
>>> +	memset(&pcoex_info->bt_addr, 0, sizeof(pcoex_info->bt_addr));
>>> +	pcoex_info->bt_addr.sin_family = AF_INET;
>>> +	pcoex_info->bt_addr.sin_port = htons(CONNECT_PORT_BT);
>>> +	pcoex_info->bt_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
>>> +
>>> +	pcoex_info->sk_store = NULL;
>>> +
>>> +	kernel_socket_err =
>>> +	   pcoex_info->udpsock->ops->bind(pcoex_info->udpsock,
>>> +					  (struct sockaddr *)&pcoex_info->sin,
>>> +					  sizeof(pcoex_info->sin));
>>> +	if (kernel_socket_err == 0) {
>>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>>> +			  "binding socket success\n");
>>> +		pcoex_info->udpsock->sk->sk_data_ready =
>>> +			rtl_btcoex_recvmsg_int;
>>> +		pcoex_info->sock_open |=  KERNEL_SOCKET_OK;
>>> +		pcoex_info->bt_attend = false;
>>> +		BTC_PRINT(BTC_MSG_SOCKET, SOCKET_CRITICAL,
>>> +			  "WIFI sending attend_req\n");
>>> +		rtl_btcoex_sendmsgbysocket(rtlpriv, attend_req,
>>> +					   sizeof(attend_req), true);
>>> +		return _SUCCESS;
>>
>> So the wireless driver communicates with the bluetooth driver (which is
>> not in upstream) via a localhost UDP connection?
>
> I think the first order of business should be to get the Bluetooth driver upstream. Until that has happened this is all kinda pointless discussion.

I agree with this; however, the last time I tried to submit a BT driver for 
Realtek, I was told that this driver should use some (as yet included) feature. 
I have watched the driver development, and if that feature was ever included, it 
was in a form that I did not recognize. I'm sorry that this is vague, but this 
happened a long time ago.

--snip--

>> I know there's a general need for something similar like this, but it
>> needs to properly discussed and designed.
>
> This is just insane. Clear NAK.
>
> Two kernel modules will not use UDP ports over the loopback interface to communicate with each other.

I will work on combining the latest BT drivers from Realtek with btusb to see if 
I can achieve a patch that will both work with the Realtek hardware, and get 
approval from the reviewers.

What would be an approved method of communicating between two kernel modules? Is 
there some example in the kernel that I could study?

Larry

Re: [PATCH V2 5/6] rtlwifi: btcoexist: Add routines for RTL8812AE kernel socket communications

[Marcel Holtmann]

Hi Larry,

>>> I'm adding bluetooth list to the discussion. Full patch is available
>>> here:
>>> 
>>> https://patchwork.kernel.org/patch/5712591/

<snip>

>>> So the wireless driver communicates with the bluetooth driver (which is
>>> not in upstream) via a localhost UDP connection?
>> 
>> I think the first order of business should be to get the Bluetooth driver upstream. Until that has happened this is all kinda pointless discussion.
> 
> I agree with this; however, the last time I tried to submit a BT driver for Realtek, I was told that this driver should use some (as yet included) feature. I have watched the driver development, and if that feature was ever included, it was in a form that I did not recognize. I'm sorry that this is vague, but this happened a long time ago.

if the Bluetooth side is running over USB, then it should be driven from the existing btusb.ko module with a vendor specific ->setup() callback. However nothing materialized that I could merge it.

>>> I know there's a general need for something similar like this, but it
>>> needs to properly discussed and designed.
>> 
>> This is just insane. Clear NAK.
>> 
>> Two kernel modules will not use UDP ports over the loopback interface to communicate with each other.
> 
> I will work on combining the latest BT drivers from Realtek with btusb to see if I can achieve a patch that will both work with the Realtek hardware, and get approval from the reviewers.
> 
> What would be an approved method of communicating between two kernel modules? Is there some example in the kernel that I could study?

We need a btcoex subsystem that both WiFi and Bluetooth can register to and communicate with.

Regards

Marcel

btcoex subsystem

[Kalle Valo]

Marcel Holtmann <marcel@holtmann.org> writes:

>> I will work on combining the latest BT drivers from Realtek with
>> btusb to see if I can achieve a patch that will both work with the
>> Realtek hardware, and get approval from the reviewers.
>> 
>> What would be an approved method of communicating between two kernel
>> modules? Is there some example in the kernel that I could study?
>
> We need a btcoex subsystem that both WiFi and Bluetooth can register
> to and communicate with.

The need for this seems to periodically come up, I remember needing
something like that back in the Maemo wl1251 days and also ath6kl needed
this. Are there any ideas for this subsystem? How would this btcoex
subsystem work? What kind of information would it provide?

Is this something we should discuss at Ottawa?

-- 
Kalle Valo

Re: btcoex subsystem

[Marcel Holtmann]

Hi Kalle,

>>> I will work on combining the latest BT drivers from Realtek with
>>> btusb to see if I can achieve a patch that will both work with the
>>> Realtek hardware, and get approval from the reviewers.
>>> 
>>> What would be an approved method of communicating between two kernel
>>> modules? Is there some example in the kernel that I could study?
>> 
>> We need a btcoex subsystem that both WiFi and Bluetooth can register
>> to and communicate with.
> 
> The need for this seems to periodically come up, I remember needing
> something like that back in the Maemo wl1251 days and also ath6kl needed
> this. Are there any ideas for this subsystem? How would this btcoex
> subsystem work? What kind of information would it provide?
> 
> Is this something we should discuss at Ottawa?

sadly something came up and I am not making it to Ottawa. However we should be talking about this at some point since Bluetooth has MWS for its coexistence which could be a good starting point. Something we have to look into for the Bluetooth subsystem at some point.

Regards

Marcel