On Sun, Jun 5, 2022 at 9:34 AM Linus Torvalds
<[email protected]> wrote:
>
> That code shouldn't use DECLARE_BITMAP() at all, it should just use
>
> struct bdaddr_list_with_flags {
> ..
> unsigned long flags;
> };
>
> and then use '&br_params->flags' when it needs the actual atomic
> 'set_bit()' things and friends,
Actually, I'm not convinced those things should be atomic at all.
*Most* of the accesses to those connection flags seem to be with
hci_dev_lock() held, and the ones that aren't can't possibly depend on
atomicity since those things are currently copied around with random
other "copy bitmaps" functions.
So I think the bluetooth code would actually be much better off with
something like
/* Bitmask of connection flags */
enum hci_conn_flags {
HCI_CONN_FLAG_REMOTE_WAKEUP = 1,
HCI_CONN_FLAG_DEVICE_PRIVACY = 2,
};
typedef u8 hci_conn_flags_t;
instead of playing games with DECLARE_BITMAP() and then using a mix of
atomic set_bit/clear_bit() and random non-atomic "copy bitmap values
around".
This attached patch builds cleanly for me, doing the above. But see a
few comments about those atomicity issues...
And no, it doesn't really need that new "hci_conn_flags_t", and it
could just use "u32" (which is what "current_flags" and
"supported_flags" use in the code), but those structures that contain
those connection flags do seem to have various other byte fields and
it would appear to pack better using just that simple one-byte type.
It *literally* just uses two bits in it, after all, and as mentioned,
the whole atomicity situation right now is very very dubious, so it
seems to make sense to do something like this.
Reactions?
Linus