Re: What is the motivation for conn->power_save

[Marcel Holtmann <marcel@holtmann.org>]

Hi Nick,

> >> >> >> If I understand correctly, conn->power_save prevents the host stack
> >> >> >> from requesting active mode if it was not the host stack that
> >> >> >> requested sniff mode.
> >> >> >>
> >> >> >> I don't understand the motivation for this. If we have ACL data to
> >> >> >> send, then it seems like a good idea for the host stack to explicitly
> >> >> >> request active mode, regardless of the reason that we entered sniff
> >> >> >> mode.
> >> >> >>
> >> >> >> We want to enter active mode more aggressively when setting up SCO
> >> >> >> connections, to avoid a 5 second delay with certain sniff modes. But
> >> >> >> the conn->power_save code is getting in the way and doesn't appear to
> >> >> >> be useful in the first place.
> >> >> >
> >> >> > we have discussed this a few times. And if you lock through the code
> >> >> > history then you see that initially we just took devices out of sniff
> >> >> > mode if we had to send data. However with HID devices this falls flat on
> >> >> > its face. They need to stay in control of sniff mode if they initiated
> >> >> > it. Some of them crash and others just drain the battery. With sniff
> >> >> > mode you can send small amounts of data even while in sniff and for HID
> >> >> > that is sometimes used. So the remote side better not interfere.
> >> >> >
> >> >> > What we really need is a socket option where we can control this on a
> >> >> > per socket basis if we take devices out of sniff mode. And one extra
> >> >> > case might be when we try to establish a SCO channel, because then it is
> >> >> > clearly not an HID device. However even A2DP has this sort of problems
> >> >> > sometimes where the stream setup takes time.
> >> >>
> >> >> Makes sense. Thanks for the explanation.
> >> >
> >> > this means you will be working on a patch for this :)
> >> >
> >> >> > Not sure if we have to make SCO setup special. The only reason would be
> >> >> > if there is a case where we don't get an AT command before SCO needs to
> >> >> > be established.
> >> >>
> >> >> If you are in-call, and transfer audio from handset to BT headset,
> >> >> then there is SCO setup without any AT command.
> >> >
> >> > Fair enough.
> >> >
> >> >> I think for the SCO setup case we would always want to enter active
> >> >> mode. I could modify enter_active_mode() to take a parameter like 'int
> >> >> force' that would force us to enter active mode regardless of the
> >> >> state of power_save, and use this when setting up SCO. What do you
> >> >> think?
> >> >
> >> > Actually when you leave sniff mode, then all bets for the power_save
> >> > value are off again. So you better set power_save and just call
> >> > enter_active_mode. Something like this:
> >> >
> >> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >> > index a975098..e4591e0 100644
> >> > --- a/net/bluetooth/hci_conn.c
> >> > +++ b/net/bluetooth/hci_conn.c
> >> > @@ -376,6 +376,9 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type,
> >> >
> >> >        if (acl->state == BT_CONNECTED &&
> >> >                        (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
> >> > +               acl->power_save = 1;
> >> > +               hci_conn_enter_active_mode(acl);
> >> > +
> >> >                if (lmp_esco_capable(hdev))
> >> >                        hci_setup_sync(sco, acl->handle);
> >> >                else
> >> >
> >> > Alternatively we could create hci_conn_force_active_mode() or just
> >> > implement a proper per socket sniff/active policy.
> >> >
> >>
> >> Patch submitted to Android tree as:
> >>
> >> http://android.git.kernel.org/?p=kernel/common.git;a=commit;h=201ac2f225a31dffcb05f1db4d609c467c9c694c
> >
> > can you please send a version to the mailing list so I can easily apply
> > it and also have it here for reference in the archives.
> 
> Patch attached.

patch has been applied. Please keep the commit message to 72 chars so
that git log doesn't exceed on an 80 chars terminal. Also your hcidump
seems to line wrap. I had to fix that ;)

Regards

Marcel