On Mon, 2014-09-08 at 10:40 +0300, Luiz Augusto von Dentz wrote:
> Hi Tanu,
>
> On Fri, Sep 5, 2014 at 4:03 PM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Fri, 2014-08-22 at 17:04 +0300, Luiz Augusto von Dentz wrote:
> >> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
> >>
> >> ---
> >> src/modules/bluetooth/backend-ofono.c | 66 ++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 65 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
> >> index 79765bb..5fb99de 100644
> >> --- a/src/modules/bluetooth/backend-ofono.c
> >> +++ b/src/modules/bluetooth/backend-ofono.c
> >> @@ -23,9 +23,13 @@
> >> #include <config.h>
> >> #endif
> >>
> >> +#include <errno.h>
> >> +#include <poll.h>
> >> +
> >> #include <pulsecore/core-util.h>
> >> #include <pulsecore/dbus-shared.h>
> >> #include <pulsecore/shared.h>
> >> +#include <pulsecore/core-error.h>
> >>
> >> #include "bluez5-util.h"
> >>
> >> @@ -118,8 +122,68 @@ static void hf_audio_card_free(void *data) {
> >> pa_xfree(card);
> >> }
> >>
> >> +static int socket_accept(int sock)
> >> +{
> >> + char c;
> >> + struct pollfd pfd;
> >> +
> >> + if (sock < 0)
> >> + return -ENOTCONN;
> >> +
> >> + memset(&pfd, 0, sizeof(pfd));
> >> + pfd.fd = sock;
> >> + pfd.events = POLLOUT;
> >> +
> >> + if (poll(&pfd, 1, 0) < 0)
> >> + return -errno;
> >> +
> >> + /* If socket already writable then it is not in defer setup state */
> >
> > I think this "defer setup" thing needs to be explained somewhere. What
> > is it, why is it needed?
>
> This can probably be added to NewConnection documentation, Defer Setup
> is used to authorize connection, the connection setup is interrupted
> while in the this state waiting for userspace to authorize which is
> done by read.
Ok, do you think the documentation will appear soon?
> >> + if ((pfd.revents & POLLOUT))
> >> + return 0;
> >> +
> >> + /* Enable socket by reading 1 byte */
> >> + if (read(sock, &c, 1) < 0)
> >> + return -errno;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
> >> - return -1;
> >> + pa_bluetooth_backend *backend = t->userdata;
> >> + hf_audio_card *card;
> >> + int err;
> >> +
> >> + pa_assert(backend);
> >> + pa_assert_se(card = pa_hashmap_get(backend->cards, t->path));
> >
> > I think it would be good to set the transport userdata to point to the
> > hf_audio_card, and get the backend from the hf_audio_card object.
>
> Either way it is fine with me, we just need to be consistent I guess.
Well, my thinking is that if the an object (transport) is permanently
associated with another object (card), there should be a way to get from
the first object to the second object by following one or more pointers,
rather than having to search for it from some container.
> >> +
> >> + if (!optional) {
> >> + DBusMessage *m;
> >> +
> >> + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
> >> + pa_assert_se(dbus_connection_send(pa_dbus_connection_get(backend->connection), m, NULL));
> >> +
> >> + return -1;
> >
> > I'd like to have a comment here explaining what !optional means and why
> > it's a good idea to send a Connect message and then return -1. Is this
> > some kind of an "asynchronous acquire" thing, i.e. the acquire operation
> > will complete once oFono calls NewConnection()? If Connect() returns an
> > error, do we need to care about that?
>
> I guess it should be named overwrite or something like that, the idea
> is that this would force SCO to connect but since the operation is
> asynchronous we have to return -1 because Connect the method does not
> return the fd immediately.
What if Connect() fails, do we need to react to that somehow?
> >> + }
> >> +
> >> + /* The correct block size should take into account the SCO MTU from
> >> + * the Bluetooth adapter and (for adapters in the USB bus) the MxPS
> >> + * value from the Isoc USB endpoint in use by btusb and should be
> >> + * made available to userspace by the Bluetooth kernel subsystem.
> >> + * Meanwhile the empiric value 48 will be used. */
> >
> > Do you know what downsides the hardcoding has? Those could be documented
> > here. Also, is the kernel bug tracked somewhere?
>
> Actually we have been debating whether kernel could actually do some
> sort of flow control, currently the socket MTU is doubled so if we
> read via socket options it would be 96.
Nice to know, but I don't think this really answers my questions :) Are
there some situations where the hardcoded 48 doesn't work or works
somehow suboptimally?
--
Tanu
