re: Bluetooth: hciuart: Add support QCA chipset for UART

re: Bluetooth: hciuart: Add support QCA chipset for UART

[Dan Carpenter]

Hello Ben Young Tae Kim,

The patch 0ff252c1976d: "Bluetooth: hciuart: Add support QCA chipset
for UART" from Aug 10, 2015, leads to the following static checker
warning:

	drivers/bluetooth/hci_qca.c:485 qca_debugfs_init()
	warn: passing casted pointer '&qca->tx_vote' to 'debugfs_create_bool()' 1 vs 32.

drivers/bluetooth/hci_qca.c
   482                             &qca->ibs_recv_wakes);
   483          debugfs_create_u64("ibs_recv_wake_acks", mode, ibs_dir,
   484                             &qca->ibs_recv_wacks);
   485          debugfs_create_bool("tx_vote", mode, ibs_dir, (u32 *)&qca->tx_vote);

debugfs_create_bool() is stupid, because the name says bool but it
writes a u32.  I am not sure why this is.  Maybe because the sizeof
_Bool is not defined in C?  Anyway, this will not work and will also
corrupt memory.

   486          debugfs_create_u64("tx_votes_on", mode, ibs_dir, &qca->tx_votes_on);
   487          debugfs_create_u64("tx_votes_off", mode, ibs_dir, &qca->tx_votes_off);
   488          debugfs_create_bool("rx_vote", mode, ibs_dir, (u32 *)&qca->rx_vote);
                               ^^^^
Same.

   489          debugfs_create_u64("rx_votes_on", mode, ibs_dir, &qca->rx_votes_on);
   490          debugfs_create_u64("rx_votes_off", mode, ibs_dir, &qca->rx_votes_off);



regards,
dan carpenter

Re: Bluetooth: hciuart: Add support QCA chipset for UART

[Ben Young Tae Kim]

Hi Dan,

On 08/13/15 13:59, Dan Carpenter wrote:
> Hello Ben Young Tae Kim,
>
> The patch 0ff252c1976d: "Bluetooth: hciuart: Add support QCA chipset
> for UART" from Aug 10, 2015, leads to the following static checker
> warning:
>
> 	drivers/bluetooth/hci_qca.c:485 qca_debugfs_init()
> 	warn: passing casted pointer '&qca->tx_vote' to 'debugfs_create_bool()' 1 vs 32.
>
> drivers/bluetooth/hci_qca.c
>     482                             &qca->ibs_recv_wakes);
>     483          debugfs_create_u64("ibs_recv_wake_acks", mode, ibs_dir,
>     484                             &qca->ibs_recv_wacks);
>     485          debugfs_create_bool("tx_vote", mode, ibs_dir, (u32 *)&qca->tx_vote);
>
> debugfs_create_bool() is stupid, because the name says bool but it
> writes a u32.  I am not sure why this is.  Maybe because the sizeof
> _Bool is not defined in C?  Anyway, this will not work and will also
> corrupt memory.

I agree with you. The reason why I do the cast as u32 type pointer is 
debugfs_create_bool() function is required to put value as u32 *. But it 
will cause memory corruption to read that because it is actual 1 byte. 
Thanks for your report. I'll submit the patch to address it soon.

>
>     486          debugfs_create_u64("tx_votes_on", mode, ibs_dir, &qca->tx_votes_on);
>     487          debugfs_create_u64("tx_votes_off", mode, ibs_dir, &qca->tx_votes_off);
>     488          debugfs_create_bool("rx_vote", mode, ibs_dir, (u32 *)&qca->rx_vote);
>                                 ^^^^
> Same.
>
>     489          debugfs_create_u64("rx_votes_on", mode, ibs_dir, &qca->rx_votes_on);
>     490          debugfs_create_u64("rx_votes_off", mode, ibs_dir, &qca->rx_votes_off);
>
>
>
> regards,
> dan carpenter

Thanks
-- Ben Kim

Re: Bluetooth: hciuart: Add support QCA chipset for UART

[Ben Young Tae Kim]

Hi Dan,

On 08/13/15 14:51, Ben Young Tae Kim wrote:
> Hi Dan,
>
> On 08/13/15 13:59, Dan Carpenter wrote:
>> Hello Ben Young Tae Kim,
>>
>> The patch 0ff252c1976d: "Bluetooth: hciuart: Add support QCA chipset
>> for UART" from Aug 10, 2015, leads to the following static checker
>> warning:
>>

BTW, which tool are you using to detect static warning? Can you 
recommend me some tool so that I could prevent same error to the 
upcoming patches?

Thanks
-- Ben Kim

Re: Bluetooth: hciuart: Add support QCA chipset for UART

[Dan Carpenter]

On Thu, Aug 13, 2015 at 02:59:30PM -0700, Ben Young Tae Kim wrote:
> BTW, which tool are you using to detect static warning? Can you
> recommend me some tool so that I could prevent same error to the
> upcoming patches?
> 

This test is a Smatch test but it has too many false positives to
publish.  Many of them can probably be silenced...  I will see what I
can do.

regards,
dan carpenter