Re: [PATCH 02/12] Bluetooth: Add basic start/complete HCI transaction functions

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Szymon,

On Fri, Feb 15, 2013, Szymon Janc wrote:
> On Friday 15 of February 2013 10:22:36 Johan Hedberg wrote:
> > > > +int hci_start_transaction(struct hci_dev *hdev)
> > > > +{
> > > > +       struct hci_transaction *transaction;
> > > > +       int err;
> > > > +
> > > > +       hci_transaction_lock(hdev);
> > > > +
> > > > +       /* We can't start a new transaction if there's another one in
> > > > +        * progress of being built.
> > > > +        */
> > > > +       if (hdev->build_transaction) {
> > > > +               err = -EBUSY;
> > > > +               goto unlock;
> > > > +       }
> > > > +
> > > > +       transaction = kmalloc(sizeof(*transaction), GFP_ATOMIC);
> > > 
> > > I've failed to see why we need GFP_ATOMIC here. As this code is not
> > > running in any atomic section, we can allocate memory using
> > > GFP_KERNEL.
> > 
> > Since one of the intentions of this API is to create an async version of
> > hci_request() I think it's better to keep GFP_ATOMIC here. One situation
> > where you couldn't for sure use hci_request() is if you're in an atomic
> > section and then a HCI request would be the only other alternative.
> 
> You lock mutex in hci_start_transaction so it is non-atomic anyway..

This has me a bit confused since hci_dev_lock() also uses the same. Does
this mean that there's no code at all in the Bluetooth subsystem anymore
that can run in atomic context? (since most things require locking at
least the hdev struct). I remember seeing some time back (after the
whole workqueue conversion) bugs arising from incorrect GFP_ATOMIC ->
GFP_KERNEL conversions (none of which luckily made it to Linus' tree), so at
least some parts seemed to still be atomic. Do a grep for GFP_ATOMIC in
net/bluetooth/ and you'll find plenty of them.

Johan