Signed-off-by: Marcel Holtmann <[email protected]>
---
include/net/bluetooth/l2cap.h | 35 +++++++++++++++++++++++++++++++++++
net/bluetooth/a2mp.c | 18 ++++--------------
net/bluetooth/l2cap_core.c | 6 ++++--
net/bluetooth/l2cap_sock.c | 34 +++++++++++++++++++++-------------
4 files changed, 64 insertions(+), 29 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92511034d1d4..e4f4ca69e301 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -602,6 +602,9 @@ struct l2cap_ops {
struct sk_buff *(*alloc_skb) (struct l2cap_chan *chan,
unsigned long hdr_len,
unsigned long len, int nb);
+ int (*memcpy_fromiovec) (struct l2cap_chan *chan,
+ unsigned char *kdata,
+ struct iovec *iov, int len);
};
struct l2cap_conn {
@@ -857,6 +860,38 @@ static inline long l2cap_chan_no_get_sndtimeo(struct l2cap_chan *chan)
return 0;
}
+static inline struct sk_buff *l2cap_chan_no_alloc_skb(struct l2cap_chan *chan,
+ unsigned long hdr_len,
+ unsigned long len, int nb)
+{
+ struct sk_buff *skb;
+
+ skb = bt_skb_alloc(hdr_len + len, GFP_KERNEL);
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
+
+ return skb;
+}
+
+static inline int l2cap_chan_no_memcpy_fromiovec(struct l2cap_chan *chan,
+ unsigned char *kdata,
+ struct iovec *iov, int len)
+{
+ while (len > 0) {
+ if (iov->iov_len) {
+ int copy = min_t(unsigned int, len, iov->iov_len);
+ memcpy(kdata, iov->iov_base, copy);
+ len -= copy;
+ kdata += copy;
+ iov->iov_base += copy;
+ iov->iov_len -= copy;
+ }
+ iov++;
+ }
+
+ return 0;
+}
+
extern bool disable_ertm;
int l2cap_init_sockets(void);
diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index 0fd8d1dda709..4cda6213cfc3 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -692,25 +692,15 @@ static void a2mp_chan_state_change_cb(struct l2cap_chan *chan, int state,
}
}
-static struct sk_buff *a2mp_chan_alloc_skb_cb(struct l2cap_chan *chan,
- unsigned long hdr_len,
- unsigned long len, int nb)
-{
- struct sk_buff *skb;
-
- skb = bt_skb_alloc(hdr_len + len, GFP_KERNEL);
- if (!skb)
- return ERR_PTR(-ENOMEM);
-
- return skb;
-}
-
static const struct l2cap_ops a2mp_chan_ops = {
.name = "L2CAP A2MP channel",
.recv = a2mp_chan_recv_cb,
.close = a2mp_chan_close_cb,
.state_change = a2mp_chan_state_change_cb,
- .alloc_skb = a2mp_chan_alloc_skb_cb,
+
+ /* Use default kernel implementation */
+ .alloc_skb = l2cap_chan_no_alloc_skb,
+ .memcpy_fromiovec = l2cap_chan_no_memcpy_fromiovec,
/* Not implemented for A2MP */
.new_connection = l2cap_chan_no_new_connection,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b093e98aa968..eb0d43da80d5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2112,7 +2112,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
struct sk_buff **frag;
int sent = 0;
- if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
+ if (chan->ops->memcpy_fromiovec(chan, skb_put(skb, count),
+ msg->msg_iov, count))
return -EFAULT;
sent += count;
@@ -2132,7 +2133,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
*frag = tmp;
- if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+ if (chan->ops->memcpy_fromiovec(chan, skb_put(*frag, count),
+ msg->msg_iov, count))
return -EFAULT;
sent += count;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 6ab282212d5c..e64e1bbba14d 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1318,6 +1318,13 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
return skb;
}
+static int l2cap_sock_memcpy_fromiovec_cb(struct l2cap_chan *chan,
+ unsigned char *kdata,
+ struct iovec *iov, int len)
+{
+ return memcpy_fromiovec(kdata, iov, len);
+}
+
static void l2cap_sock_ready_cb(struct l2cap_chan *chan)
{
struct sock *sk = chan->data;
@@ -1384,19 +1391,20 @@ static void l2cap_sock_suspend_cb(struct l2cap_chan *chan)
}
static const struct l2cap_ops l2cap_chan_ops = {
- .name = "L2CAP Socket Interface",
- .new_connection = l2cap_sock_new_connection_cb,
- .recv = l2cap_sock_recv_cb,
- .close = l2cap_sock_close_cb,
- .teardown = l2cap_sock_teardown_cb,
- .state_change = l2cap_sock_state_change_cb,
- .ready = l2cap_sock_ready_cb,
- .defer = l2cap_sock_defer_cb,
- .resume = l2cap_sock_resume_cb,
- .suspend = l2cap_sock_suspend_cb,
- .set_shutdown = l2cap_sock_set_shutdown_cb,
- .get_sndtimeo = l2cap_sock_get_sndtimeo_cb,
- .alloc_skb = l2cap_sock_alloc_skb_cb,
+ .name = "L2CAP Socket Interface",
+ .new_connection = l2cap_sock_new_connection_cb,
+ .recv = l2cap_sock_recv_cb,
+ .close = l2cap_sock_close_cb,
+ .teardown = l2cap_sock_teardown_cb,
+ .state_change = l2cap_sock_state_change_cb,
+ .ready = l2cap_sock_ready_cb,
+ .defer = l2cap_sock_defer_cb,
+ .resume = l2cap_sock_resume_cb,
+ .suspend = l2cap_sock_suspend_cb,
+ .set_shutdown = l2cap_sock_set_shutdown_cb,
+ .get_sndtimeo = l2cap_sock_get_sndtimeo_cb,
+ .alloc_skb = l2cap_sock_alloc_skb_cb,
+ .memcpy_fromiovec = l2cap_sock_memcpy_fromiovec_cb,
};
static void l2cap_sock_destruct(struct sock *sk)
--
1.9.3
Hi Jukka,
>> so this patch is just the groundwork to deal with the socket vs kernel internal handling of TX path. And this patch should most likely also be split into providing the default for alloc_skb and then adding memcpy_fromiovec.
>>
>> And I have just compile-tested this patch. I have not run it yet. I leave this to someone else to actually test if this works ;)
>
> just tested this and it works ok so ACK from me.
I split the patches now and added a proper commit message to them. Just keep in mind this is just the first step here. We should investigate on how we can combine alloc_skb and mempcy_fromiovec callbacks. This interim step is just to no block 6loWPAN support until we have figured it out.
Regards
Marcel
Hi Marcel,
On ke, 2014-06-11 at 18:00 +0200, Marcel Holtmann wrote:
> Hello,
>
> so this patch is just the groundwork to deal with the socket vs kernel internal handling of TX path. And this patch should most likely also be split into providing the default for alloc_skb and then adding memcpy_fromiovec.
>
> And I have just compile-tested this patch. I have not run it yet. I leave this to someone else to actually test if this works ;)
just tested this and it works ok so ACK from me.
Cheers,
Jukka
Hi Jukka,
>> so this patch is just the groundwork to deal with the socket vs kernel internal handling of TX path. And this patch should most likely also be split into providing the default for alloc_skb and then adding memcpy_fromiovec.
>
> Earlier in 6lowpan coc patches, you suggested that we would not use
> msghdr for kernel internal data because of extra memory copy that is
> needed in 6lowpan side. Is that suggestion now reverted?
you can allocate msghdr and iovec on the stack and just have it point to a single buffer you have your message in. The msghdr will be modified by the underlying default implementation, but besides having to allocate the message itself you do not have an extra allocation. Look at how A2MP does it.
In the future steps we should look into how we can take a SKB with the existing message and clone or copy it efficiently so it will split over multiple ACL fragments.
As I mentioned before, I never realized how well optimized our L2CAP is to deal with iovec structs from userspace. It never occurred to me that kernel internal TX path is non existent and that A2MP is actually buggy.
Regards
Marcel
Hi Marcel,
On ke, 2014-06-11 at 18:00 +0200, Marcel Holtmann wrote:
> Hello,
>
> so this patch is just the groundwork to deal with the socket vs kernel internal handling of TX path. And this patch should most likely also be split into providing the default for alloc_skb and then adding memcpy_fromiovec.
Earlier in 6lowpan coc patches, you suggested that we would not use
msghdr for kernel internal data because of extra memory copy that is
needed in 6lowpan side. Is that suggestion now reverted?
Cheers,
Jukka
Hello,
so this patch is just the groundwork to deal with the socket vs kernel internal handling of TX path. And this patch should most likely also be split into providing the default for alloc_skb and then adding memcpy_fromiovec.
And I have just compile-tested this patch. I have not run it yet. I leave this to someone else to actually test if this works ;)
> Signed-off-by: Marcel Holtmann <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 35 +++++++++++++++++++++++++++++++++++
> net/bluetooth/a2mp.c | 18 ++++--------------
> net/bluetooth/l2cap_core.c | 6 ++++--
> net/bluetooth/l2cap_sock.c | 34 +++++++++++++++++++++-------------
> 4 files changed, 64 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 92511034d1d4..e4f4ca69e301 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -602,6 +602,9 @@ struct l2cap_ops {
> struct sk_buff *(*alloc_skb) (struct l2cap_chan *chan,
> unsigned long hdr_len,
> unsigned long len, int nb);
> + int (*memcpy_fromiovec) (struct l2cap_chan *chan,
> + unsigned char *kdata,
> + struct iovec *iov, int len);
> };
>
This is just the first step on how we should deal with addressing this. With this step however further work on 6loWPAN and using l2cap_chan for SMP will become possible. It is also fixing the copy_from_user issue with A2MP that we already have right now and nobody noticed.
Long term I think alloc_skb and memcpy_fromiovec should be combined into a single callback. The problem here is the L2CAP header and in more detail the ERTM / I-Frame headers. It needs a bit clearly work on how to allocate the SKB and reserve the proper headroom for the header. In addition we should properly use appropriate network and transport header pointers of the SKB to map L2CAP headers and HCI ACL headers.
I still have to figure our why the FCS with ERTM and fragmented ACL data actually works since that code is still a bit unclear to me. At this level with might want to add kernel tracing support to L2CAP so that we can use ftrace here. So if anybody is looking for a hobby kernel project, here is one ;)
Regards
Marcel