[PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[Ben Dooks (Codethink)]

It looks like in hci_init4_req() the request is being
initialised from cpu-endian data but the packet is specified
to be little-endian. This causes an warning from sparse due
to __le16 to u16 conversion.

Fix this by using cpu_to_le16() on the two fields in the packet.

net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment (different base types)
net/bluetooth/hci_core.c:845:27:    expected restricted __le16 [usertype] tx_len
net/bluetooth/hci_core.c:845:27:    got unsigned short [usertype] le_max_tx_len
net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment (different base types)
net/bluetooth/hci_core.c:846:28:    expected restricted __le16 [usertype] tx_time
net/bluetooth/hci_core.c:846:28:    got unsigned short [usertype] le_max_tx_time

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-bluetooth@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 net/bluetooth/hci_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 04bc79359a17..b2559d4bed81 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
 	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
 		struct hci_cp_le_write_def_data_len cp;
 
-		cp.tx_len = hdev->le_max_tx_len;
-		cp.tx_time = hdev->le_max_tx_time;
+		cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
+		cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);
 		hci_req_add(req, HCI_OP_LE_WRITE_DEF_DATA_LEN, sizeof(cp), &cp);
 	}
 
-- 
2.23.0

Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[Simon Horman]

On Wed, Oct 16, 2019 at 12:39:43PM +0100, Ben Dooks (Codethink) wrote:
> It looks like in hci_init4_req() the request is being
> initialised from cpu-endian data but the packet is specified
> to be little-endian. This causes an warning from sparse due
> to __le16 to u16 conversion.
> 
> Fix this by using cpu_to_le16() on the two fields in the packet.
> 
> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment (different base types)
> net/bluetooth/hci_core.c:845:27:    expected restricted __le16 [usertype] tx_len
> net/bluetooth/hci_core.c:845:27:    got unsigned short [usertype] le_max_tx_len
> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment (different base types)
> net/bluetooth/hci_core.c:846:28:    expected restricted __le16 [usertype] tx_time
> net/bluetooth/hci_core.c:846:28:    got unsigned short [usertype] le_max_tx_time
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-bluetooth@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  net/bluetooth/hci_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 04bc79359a17..b2559d4bed81 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
>  	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
>  		struct hci_cp_le_write_def_data_len cp;
>  
> -		cp.tx_len = hdev->le_max_tx_len;
> -		cp.tx_time = hdev->le_max_tx_time;
> +		cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
> +		cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);

I would suggest that the naming of the le_ fields of struct hci_dev
implies that the values stored in those fields should be little endian
(but those that are more than bone byte wide are not).

In any case, the question arises as to if this has ever worked on big
endian machines.

>  		hci_req_add(req, HCI_OP_LE_WRITE_DEF_DATA_LEN, sizeof(cp), &cp);
>  	}
>  
> -- 
> 2.23.0
> 

Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[Marcel Holtmann]

Hi Ben,

> It looks like in hci_init4_req() the request is being
> initialised from cpu-endian data but the packet is specified
> to be little-endian. This causes an warning from sparse due
> to __le16 to u16 conversion.
> 
> Fix this by using cpu_to_le16() on the two fields in the packet.
> 
> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment (different base types)
> net/bluetooth/hci_core.c:845:27:    expected restricted __le16 [usertype] tx_len
> net/bluetooth/hci_core.c:845:27:    got unsigned short [usertype] le_max_tx_len
> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment (different base types)
> net/bluetooth/hci_core.c:846:28:    expected restricted __le16 [usertype] tx_time
> net/bluetooth/hci_core.c:846:28:    got unsigned short [usertype] le_max_tx_time
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-bluetooth@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> net/bluetooth/hci_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[Marcel Holtmann]

Hi Simon,

>> It looks like in hci_init4_req() the request is being
>> initialised from cpu-endian data but the packet is specified
>> to be little-endian. This causes an warning from sparse due
>> to __le16 to u16 conversion.
>> 
>> Fix this by using cpu_to_le16() on the two fields in the packet.
>> 
>> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment (different base types)
>> net/bluetooth/hci_core.c:845:27:    expected restricted __le16 [usertype] tx_len
>> net/bluetooth/hci_core.c:845:27:    got unsigned short [usertype] le_max_tx_len
>> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment (different base types)
>> net/bluetooth/hci_core.c:846:28:    expected restricted __le16 [usertype] tx_time
>> net/bluetooth/hci_core.c:846:28:    got unsigned short [usertype] le_max_tx_time
>> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> Cc: Marcel Holtmann <marcel@holtmann.org>
>> Cc: Johan Hedberg <johan.hedberg@gmail.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: linux-bluetooth@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> net/bluetooth/hci_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 04bc79359a17..b2559d4bed81 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
>> 	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
>> 		struct hci_cp_le_write_def_data_len cp;
>> 
>> -		cp.tx_len = hdev->le_max_tx_len;
>> -		cp.tx_time = hdev->le_max_tx_time;
>> +		cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
>> +		cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);
> 
> I would suggest that the naming of the le_ fields of struct hci_dev
> implies that the values stored in those fields should be little endian
> (but those that are more than bone byte wide are not).

the le_ stands for Low Energy and not for Little Endian.

Regards

Marcel

Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req

[Simon Horman]

On Wed, Oct 16, 2019 at 08:42:48PM +0200, Marcel Holtmann wrote:
> Hi Simon,
> 
> >> It looks like in hci_init4_req() the request is being
> >> initialised from cpu-endian data but the packet is specified
> >> to be little-endian. This causes an warning from sparse due
> >> to __le16 to u16 conversion.
> >> 
> >> Fix this by using cpu_to_le16() on the two fields in the packet.
> >> 
> >> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment (different base types)
> >> net/bluetooth/hci_core.c:845:27:    expected restricted __le16 [usertype] tx_len
> >> net/bluetooth/hci_core.c:845:27:    got unsigned short [usertype] le_max_tx_len
> >> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment (different base types)
> >> net/bluetooth/hci_core.c:846:28:    expected restricted __le16 [usertype] tx_time
> >> net/bluetooth/hci_core.c:846:28:    got unsigned short [usertype] le_max_tx_time
> >> 
> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >> ---
> >> Cc: Marcel Holtmann <marcel@holtmann.org>
> >> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: linux-bluetooth@vger.kernel.org
> >> Cc: netdev@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >> net/bluetooth/hci_core.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 04bc79359a17..b2559d4bed81 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req, unsigned long opt)
> >> 	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
> >> 		struct hci_cp_le_write_def_data_len cp;
> >> 
> >> -		cp.tx_len = hdev->le_max_tx_len;
> >> -		cp.tx_time = hdev->le_max_tx_time;
> >> +		cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
> >> +		cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);
> > 
> > I would suggest that the naming of the le_ fields of struct hci_dev
> > implies that the values stored in those fields should be little endian
> > (but those that are more than bone byte wide are not).
> 
> the le_ stands for Low Energy and not for Little Endian.

Thanks, in that case I have no objections.