2012-05-29 16:19:26

by Gustavo Padovan

Subject: [PATCH] Bluetooth: Create empty l2cap ops function

From: Gustavo Padovan <[email protected]>

A2MP doesn't use part of the L2CAP chan ops API so we just create general
empty function instead of the A2MP specific one.

Signed-off-by: Gustavo Padovan <[email protected]>
---
include/net/bluetooth/l2cap.h | 12 ++++++++++++
net/bluetooth/a2mp.c | 23 +++--------------------
2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index a00b43e..b939e90 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
return (seq + 1) % (chan->tx_win_max + 1);
}

+static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
+{
+ return NULL;
+}
+
+static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int err)
+{
+}
+
+static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
+{
+}

extern bool disable_ertm;

diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index e08ca2a..ec1ef2e 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -440,23 +440,6 @@ static struct sk_buff *a2mp_chan_alloc_skb_cb(struct l2cap_chan *chan,
return bt_skb_alloc(len, GFP_KERNEL);
}

-static struct l2cap_chan *a2mp_chan_no_new_conn_cb(struct l2cap_chan *chan)
-{
- BT_ERR("new_connection for chan %p not implemented", chan);
-
- return NULL;
-}
-
-static void a2mp_chan_no_teardown_cb(struct l2cap_chan *chan, int err)
-{
- BT_ERR("teardown for chan %p not implemented", chan);
-}
-
-static void a2mp_chan_no_ready(struct l2cap_chan *chan)
-{
- BT_ERR("ready for chan %p not implemented", chan);
-}
-
static struct l2cap_ops a2mp_chan_ops = {
.name = "L2CAP A2MP channel",
.recv = a2mp_chan_recv_cb,
@@ -465,9 +448,9 @@ static struct l2cap_ops a2mp_chan_ops = {
.alloc_skb = a2mp_chan_alloc_skb_cb,

/* Not implemented for A2MP */
- .new_connection = a2mp_chan_no_new_conn_cb,
- .teardown = a2mp_chan_no_teardown_cb,
- .ready = a2mp_chan_no_ready,
+ .new_connection = l2cap_chan_no_new_connection,
+ .teardown = l2cap_chan_no_teardown,
+ .ready = l2cap_chan_no_ready,
};

static struct l2cap_chan *a2mp_chan_open(struct l2cap_conn *conn)
--
1.7.10.2

2012-05-30 07:05:50

by Gustavo Padovan

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Hi Andrei,

* Andrei Emeltchenko <[email protected]> [2012-05-30 10:02:00 +0300]:

> Hi Gustavo,
>
> On Tue, May 29, 2012 at 01:19:26PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <[email protected]>
> >
> > A2MP doesn't use part of the L2CAP chan ops API so we just create general
> > empty function instead of the A2MP specific one.
> >
> > Signed-off-by: Gustavo Padovan <[email protected]>
> > ---
> > include/net/bluetooth/l2cap.h | 12 ++++++++++++
> > net/bluetooth/a2mp.c | 23 +++--------------------
> > 2 files changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index a00b43e..b939e90 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
> > return (seq + 1) % (chan->tx_win_max + 1);
> > }
> >
> > +static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
> > +{
> > + return NULL;
> > +}
> > +
> > +static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int err)
> > +{
> > +}
> > +
> > +static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
> > +{
> > +}
>
> One note here. Shall we have debug or error message to indicate that
> function is not implemented?

No, these are like NOPs. Shouldn't be noticed by anyone, if you decided to use
the you are saying that you don't wanna know about it.

Gustavo

2012-05-30 07:02:00

by Andrei Emeltchenko

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Hi Gustavo,

On Tue, May 29, 2012 at 01:19:26PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> A2MP doesn't use part of the L2CAP chan ops API so we just create general
> empty function instead of the A2MP specific one.
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 12 ++++++++++++
> net/bluetooth/a2mp.c | 23 +++--------------------
> 2 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index a00b43e..b939e90 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
> return (seq + 1) % (chan->tx_win_max + 1);
> }
>
> +static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
> +{
> + return NULL;
> +}
> +
> +static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int err)
> +{
> +}
> +
> +static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
> +{
> +}

One note here. Shall we have debug or error message to indicate that
function is not implemented?

Best regards
Andrei Emeltchenko

2012-05-29 16:50:28

by Gustavo Padovan

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Hi Mat,

* Mat Martineau <[email protected]> [2012-05-29 09:41:45 -0700]:

>
> Gustavo -
>
> On Tue, 29 May 2012, Gustavo Padovan wrote:
>
> >From: Gustavo Padovan <[email protected]>
> >
> >A2MP doesn't use part of the L2CAP chan ops API so we just create general
> >empty function instead of the A2MP specific one.
> >
> >Signed-off-by: Gustavo Padovan <[email protected]>
> >---
> >include/net/bluetooth/l2cap.h | 12 ++++++++++++
> >net/bluetooth/a2mp.c | 23 +++--------------------
> >2 files changed, 15 insertions(+), 20 deletions(-)
> >
> >diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >index a00b43e..b939e90 100644
> >--- a/include/net/bluetooth/l2cap.h
> >+++ b/include/net/bluetooth/l2cap.h
> >@@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
> > return (seq + 1) % (chan->tx_win_max + 1);
> >}
> >
> >+static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
> >+{
> >+ return NULL;
> >+}
> >+
> >+static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int err)
> >+{
> >+}
> >+
> >+static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
> >+{
> >+}
> >
> >extern bool disable_ertm;
> >
> >diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
> >index e08ca2a..ec1ef2e 100644
> >--- a/net/bluetooth/a2mp.c
> >+++ b/net/bluetooth/a2mp.c
> >@@ -440,23 +440,6 @@ static struct sk_buff *a2mp_chan_alloc_skb_cb(struct l2cap_chan *chan,
> > return bt_skb_alloc(len, GFP_KERNEL);
> >}
> >
> >-static struct l2cap_chan *a2mp_chan_no_new_conn_cb(struct l2cap_chan *chan)
> >-{
> >- BT_ERR("new_connection for chan %p not implemented", chan);
> >-
> >- return NULL;
> >-}
> >-
> >-static void a2mp_chan_no_teardown_cb(struct l2cap_chan *chan, int err)
> >-{
> >- BT_ERR("teardown for chan %p not implemented", chan);
> >-}
> >-
> >-static void a2mp_chan_no_ready(struct l2cap_chan *chan)
> >-{
> >- BT_ERR("ready for chan %p not implemented", chan);
> >-}
> >-
> >static struct l2cap_ops a2mp_chan_ops = {
> > .name = "L2CAP A2MP channel",
> > .recv = a2mp_chan_recv_cb,
> >@@ -465,9 +448,9 @@ static struct l2cap_ops a2mp_chan_ops = {
> > .alloc_skb = a2mp_chan_alloc_skb_cb,
> >
> > /* Not implemented for A2MP */
> >- .new_connection = a2mp_chan_no_new_conn_cb,
> >- .teardown = a2mp_chan_no_teardown_cb,
> >- .ready = a2mp_chan_no_ready,
> >+ .new_connection = l2cap_chan_no_new_connection,
> >+ .teardown = l2cap_chan_no_teardown,
> >+ .ready = l2cap_chan_no_ready,
> >};
> >
> >static struct l2cap_chan *a2mp_chan_open(struct l2cap_conn *conn)
> >--
> >1.7.10.2
>
> If these pointers all need to be populated, will you also remove the
> NULL checks for the callbacks in l2cap_core.c?

Yes, one of my patchset that is on the mailing list do this.

>Maybe the callbacks
> could be validated once when the l2cap_chan is set up.

I think it's pointless, we are the only user of our API, so we can make sure
we won't have NULL pointer floating around.

Gustavo

2012-05-29 16:49:22

by Mat Martineau

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Gustavo -

On Tue, 29 May 2012, Mat Martineau wrote:

>
> Gustavo -
>
> On Tue, 29 May 2012, Gustavo Padovan wrote:
>
>> From: Gustavo Padovan <[email protected]>
>>
>> A2MP doesn't use part of the L2CAP chan ops API so we just create general
>> empty function instead of the A2MP specific one.
>>
>> Signed-off-by: Gustavo Padovan <[email protected]>
>> ---
>> include/net/bluetooth/l2cap.h | 12 ++++++++++++
>> net/bluetooth/a2mp.c | 23 +++--------------------
>> 2 files changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index a00b43e..b939e90 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan
>> *chan, __u16 seq)
>> return (seq + 1) % (chan->tx_win_max + 1);
>> }
>>
>> +static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct
>> l2cap_chan *chan)
>> +{
>> + return NULL;
>> +}
>> +
>> +static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int
>> err)
>> +{
>> +}
>> +
>> +static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
>> +{
>> +}
>>
>> extern bool disable_ertm;
>>
>> diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
>> index e08ca2a..ec1ef2e 100644
>> --- a/net/bluetooth/a2mp.c
>> +++ b/net/bluetooth/a2mp.c
>> @@ -440,23 +440,6 @@ static struct sk_buff *a2mp_chan_alloc_skb_cb(struct
>> l2cap_chan *chan,
>> return bt_skb_alloc(len, GFP_KERNEL);
>> }
>>
>> -static struct l2cap_chan *a2mp_chan_no_new_conn_cb(struct l2cap_chan
>> *chan)
>> -{
>> - BT_ERR("new_connection for chan %p not implemented", chan);
>> -
>> - return NULL;
>> -}
>> -
>> -static void a2mp_chan_no_teardown_cb(struct l2cap_chan *chan, int err)
>> -{
>> - BT_ERR("teardown for chan %p not implemented", chan);
>> -}
>> -
>> -static void a2mp_chan_no_ready(struct l2cap_chan *chan)
>> -{
>> - BT_ERR("ready for chan %p not implemented", chan);
>> -}
>> -
>> static struct l2cap_ops a2mp_chan_ops = {
>> .name = "L2CAP A2MP channel",
>> .recv = a2mp_chan_recv_cb,
>> @@ -465,9 +448,9 @@ static struct l2cap_ops a2mp_chan_ops = {
>> .alloc_skb = a2mp_chan_alloc_skb_cb,
>>
>> /* Not implemented for A2MP */
>> - .new_connection = a2mp_chan_no_new_conn_cb,
>> - .teardown = a2mp_chan_no_teardown_cb,
>> - .ready = a2mp_chan_no_ready,
>> + .new_connection = l2cap_chan_no_new_connection,
>> + .teardown = l2cap_chan_no_teardown,
>> + .ready = l2cap_chan_no_ready,
>> };
>>
>> static struct l2cap_chan *a2mp_chan_open(struct l2cap_conn *conn)
>> --
>> 1.7.10.2
>
> If these pointers all need to be populated, will you also remove the NULL
> checks for the callbacks in l2cap_core.c? Maybe the callbacks could be
> validated once when the l2cap_chan is set up.

Nevermind, you already have a patch for this. I should have caught up
on the rest of the mailing list first...


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

2012-05-29 16:41:45

by Mat Martineau

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Gustavo -

On Tue, 29 May 2012, Gustavo Padovan wrote:

> From: Gustavo Padovan <[email protected]>
>
> A2MP doesn't use part of the L2CAP chan ops API so we just create general
> empty function instead of the A2MP specific one.
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 12 ++++++++++++
> net/bluetooth/a2mp.c | 23 +++--------------------
> 2 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index a00b43e..b939e90 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -740,6 +740,18 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
> return (seq + 1) % (chan->tx_win_max + 1);
> }
>
> +static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
> +{
> + return NULL;
> +}
> +
> +static inline void l2cap_chan_no_teardown(struct l2cap_chan *chan, int err)
> +{
> +}
> +
> +static inline void l2cap_chan_no_ready(struct l2cap_chan *chan)
> +{
> +}
>
> extern bool disable_ertm;
>
> diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
> index e08ca2a..ec1ef2e 100644
> --- a/net/bluetooth/a2mp.c
> +++ b/net/bluetooth/a2mp.c
> @@ -440,23 +440,6 @@ static struct sk_buff *a2mp_chan_alloc_skb_cb(struct l2cap_chan *chan,
> return bt_skb_alloc(len, GFP_KERNEL);
> }
>
> -static struct l2cap_chan *a2mp_chan_no_new_conn_cb(struct l2cap_chan *chan)
> -{
> - BT_ERR("new_connection for chan %p not implemented", chan);
> -
> - return NULL;
> -}
> -
> -static void a2mp_chan_no_teardown_cb(struct l2cap_chan *chan, int err)
> -{
> - BT_ERR("teardown for chan %p not implemented", chan);
> -}
> -
> -static void a2mp_chan_no_ready(struct l2cap_chan *chan)
> -{
> - BT_ERR("ready for chan %p not implemented", chan);
> -}
> -
> static struct l2cap_ops a2mp_chan_ops = {
> .name = "L2CAP A2MP channel",
> .recv = a2mp_chan_recv_cb,
> @@ -465,9 +448,9 @@ static struct l2cap_ops a2mp_chan_ops = {
> .alloc_skb = a2mp_chan_alloc_skb_cb,
>
> /* Not implemented for A2MP */
> - .new_connection = a2mp_chan_no_new_conn_cb,
> - .teardown = a2mp_chan_no_teardown_cb,
> - .ready = a2mp_chan_no_ready,
> + .new_connection = l2cap_chan_no_new_connection,
> + .teardown = l2cap_chan_no_teardown,
> + .ready = l2cap_chan_no_ready,
> };
>
> static struct l2cap_chan *a2mp_chan_open(struct l2cap_conn *conn)
> --
> 1.7.10.2

If these pointers all need to be populated, will you also remove the
NULL checks for the callbacks in l2cap_core.c? Maybe the callbacks
could be validated once when the l2cap_chan is set up.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

2012-06-01 13:10:48

by Johan Hedberg

Subject: Re: [PATCH] Bluetooth: Create empty l2cap ops function

Hi Gustavo,

On Tue, May 29, 2012, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> A2MP doesn't use part of the L2CAP chan ops API so we just create general
> empty function instead of the A2MP specific one.
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 12 ++++++++++++
> net/bluetooth/a2mp.c | 23 +++--------------------
> 2 files changed, 15 insertions(+), 20 deletions(-)

Applied to bluetooth-next. Thanks.

Johan