L2CAP COC: Removing a server

[mickeyl]

(Disclaimer: I'm using the esp-nimble fork, but that should be irrelevant in this context)

Usage case: In my application, I have various subsystems (BLE/L2CAP, WiFi, Ethernet) which all provide access points to a central service. Only one of the access points can be used at a certain time. To conserve memory, I always shut down the remaining subsystems whenever a connection is active.

With Nimble, I have an unavoidable memory leak, since there is no removal equivalent to ble_l2cap_coc_create_server. ble_l2cap_coc_create_server allocates a structure and appends it to the global list of L2CAP servers. I don't see why we shouldn't have a corresponding ble_l2cap_coc_remove_server that removes the L2CAP server from the static list and frees the memory allocated for the structure.

[rymanluk]

Thanks @mickeyl , Any chance you could send such patch for review?

[mickeyl]

Sure thing, I'll take a go at it.

[mickeyl]

@rymanluk Before I create a pull request, let me run this through you. Does this look reasonable?

diff --git a/nimble/host/include/host/ble_l2cap.h b/nimble/host/include/host/ble_l2cap.h
index aef9682c..bcdd5c71 100644
--- a/nimble/host/include/host/ble_l2cap.h
+++ b/nimble/host/include/host/ble_l2cap.h
@@ -250,7 +250,7 @@ typedef int ble_l2cap_event_fn(struct ble_l2cap_event *event, void *arg);
 uint16_t ble_l2cap_get_conn_handle(struct ble_l2cap_chan *chan);
 int ble_l2cap_create_server(uint16_t psm, uint16_t mtu,
                             ble_l2cap_event_fn *cb, void *cb_arg);
-
+int ble_l2cap_remove_server(uint16_t psm);
 int ble_l2cap_connect(uint16_t conn_handle, uint16_t psm, uint16_t mtu,
                       struct os_mbuf *sdu_rx,
                       ble_l2cap_event_fn *cb, void *cb_arg);
diff --git a/nimble/host/src/ble_l2cap.c b/nimble/host/src/ble_l2cap.c
index 810d07b3..1773e28a 100644
--- a/nimble/host/src/ble_l2cap.c
+++ b/nimble/host/src/ble_l2cap.c
@@ -150,6 +150,12 @@ ble_l2cap_create_server(uint16_t psm, uint16_t mtu,
     return ble_l2cap_coc_create_server(psm, mtu, cb, cb_arg);
 }

+int
+ble_l2cap_remove_server(uint16_t psm)
+{
+    return ble_l2cap_coc_remove_server(psm);
+}
+
 int
 ble_l2cap_connect(uint16_t conn_handle, uint16_t psm, uint16_t mtu,
                   struct os_mbuf *sdu_rx, ble_l2cap_event_fn *cb, void *cb_arg)
diff --git a/nimble/host/src/ble_l2cap_coc.c b/nimble/host/src/ble_l2cap_coc.c
index dada8b0c..0ace7dda 100644
--- a/nimble/host/src/ble_l2cap_coc.c
+++ b/nimble/host/src/ble_l2cap_coc.c
@@ -96,6 +96,22 @@ ble_l2cap_coc_create_server(uint16_t psm, uint16_t mtu,
     return 0;
 }

+int
+ble_l2cap_coc_remove_server(uint16_t psm)
+{
+    struct ble_l2cap_coc_srv * srv;
+
+    STAILQ_FOREACH(srv, &ble_l2cap_coc_srvs, next) {
+        if (srv->psm == psm) {
+            STAILQ_REMOVE(&ble_l2cap_coc_srvs, srv, ble_l2cap_coc_srv, next);
+            os_memblock_put(&ble_l2cap_coc_srv_pool, srv);
+            return 0;
+        }
+    }
+
+    return 0;
+}
+
 static inline void
 ble_l2cap_set_used_cid(uint32_t *cid_mask, int bit)
 {
diff --git a/nimble/host/src/ble_l2cap_coc_priv.h b/nimble/host/src/ble_l2cap_coc_priv.h
index 5ebdaa05..21fa7b9d 100644
--- a/nimble/host/src/ble_l2cap_coc_priv.h
+++ b/nimble/host/src/ble_l2cap_coc_priv.h
@@ -57,6 +57,7 @@ struct ble_l2cap_coc_srv {
 int ble_l2cap_coc_init(void);
 int ble_l2cap_coc_create_server(uint16_t psm, uint16_t mtu,
                                 ble_l2cap_event_fn *cb, void *cb_arg);
+int ble_l2cap_coc_remove_server(uint16_t psm);
 int ble_l2cap_coc_create_srv_chan(struct ble_hs_conn *conn, uint16_t psm,
                                   struct ble_l2cap_chan **chan);
 struct ble_l2cap_chan * ble_l2cap_coc_chan_alloc(struct ble_hs_conn *conn,

[rymanluk]

Hi @mickeyl

Thanks. I'm just thinking that before server removal we should check if there is no channel allocated for given PSM.

You could use BUSY error when this is detected.
Otherwise it looks good

[mickeyl]

@rymanluk

I'm afraid, I'm a bit out of my depth here. Finding out whether an allocated channel exists would mean I need to iterate over all active connections and all active channels for every connection. The struct ble_l2cap_coc_srv has no helpful pointers there, so I need additional API in ble_hs_conn.c to query an allocated channel by psm?

[mickeyl]

Ok, how about this one:

diff --git a/nimble/host/include/host/ble_l2cap.h b/nimble/host/include/host/ble_l2cap.h
index aef9682c..bcdd5c71 100644
--- a/nimble/host/include/host/ble_l2cap.h
+++ b/nimble/host/include/host/ble_l2cap.h
@@ -250,7 +250,7 @@ typedef int ble_l2cap_event_fn(struct ble_l2cap_event *event, void *arg);
 uint16_t ble_l2cap_get_conn_handle(struct ble_l2cap_chan *chan);
 int ble_l2cap_create_server(uint16_t psm, uint16_t mtu,
                             ble_l2cap_event_fn *cb, void *cb_arg);
-
+int ble_l2cap_remove_server(uint16_t psm);
 int ble_l2cap_connect(uint16_t conn_handle, uint16_t psm, uint16_t mtu,
                       struct os_mbuf *sdu_rx,
                       ble_l2cap_event_fn *cb, void *cb_arg);
diff --git a/nimble/host/src/ble_hs_conn.c b/nimble/host/src/ble_hs_conn.c
index 01325ad6..f3c95b83 100644
--- a/nimble/host/src/ble_hs_conn.c
+++ b/nimble/host/src/ble_hs_conn.c
@@ -50,6 +50,38 @@ ble_hs_conn_can_alloc(void)
            ble_gatts_conn_can_alloc();
 }
 
+struct ble_l2cap_chan *
+ble_hs_conn_chan_exists_for_psm(struct ble_hs_conn *conn, uint16_t psm)
+{
+    struct ble_l2cap_chan *chan;
+
+    SLIST_FOREACH(chan, &conn->bhc_channels, next) {
+        if (chan->psm == psm) {
+            return chan;
+        }
+    }
+
+    return NULL;
+}
+
+struct ble_hs_conn *
+ble_hs_conn_find_by_psm(uint16_t psm)
+{
+    ble_hs_lock();
+
+    struct ble_hs_conn *conn;
+
+    SLIST_FOREACH(conn, &ble_hs_conns, bhc_next) {
+        if (ble_hs_conn_chan_exists_for_psm(conn, psm)) {
+            ble_hs_unlock();
+            return conn;
+        }
+    }
+
+    ble_hs_unlock();
+    return NULL;
+}
+
 struct ble_l2cap_chan *
 ble_hs_conn_chan_find_by_scid(struct ble_hs_conn *conn, uint16_t cid)
 {
diff --git a/nimble/host/src/ble_hs_conn_priv.h b/nimble/host/src/ble_hs_conn_priv.h
index 0e451194..ce1a4db9 100644
--- a/nimble/host/src/ble_hs_conn_priv.h
+++ b/nimble/host/src/ble_hs_conn_priv.h
@@ -120,6 +120,7 @@ struct ble_hs_conn *ble_hs_conn_find(uint16_t conn_handle);
 struct ble_hs_conn *ble_hs_conn_find_assert(uint16_t conn_handle);
 struct ble_hs_conn *ble_hs_conn_find_by_addr(const ble_addr_t *addr);
 struct ble_hs_conn *ble_hs_conn_find_by_idx(int idx);
+struct ble_hs_conn *ble_hs_conn_find_by_psm(uint16_t psm);
 int ble_hs_conn_exists(uint16_t conn_handle);
 struct ble_hs_conn *ble_hs_conn_first(void);
 struct ble_l2cap_chan *ble_hs_conn_chan_find_by_scid(struct ble_hs_conn *conn,
diff --git a/nimble/host/src/ble_l2cap.c b/nimble/host/src/ble_l2cap.c
index 810d07b3..1773e28a 100644
--- a/nimble/host/src/ble_l2cap.c
+++ b/nimble/host/src/ble_l2cap.c
@@ -150,6 +150,12 @@ ble_l2cap_create_server(uint16_t psm, uint16_t mtu,
     return ble_l2cap_coc_create_server(psm, mtu, cb, cb_arg);
 }
 
+int
+ble_l2cap_remove_server(uint16_t psm)
+{
+    return ble_l2cap_coc_remove_server(psm);
+}
+
 int
 ble_l2cap_connect(uint16_t conn_handle, uint16_t psm, uint16_t mtu,
                   struct os_mbuf *sdu_rx, ble_l2cap_event_fn *cb, void *cb_arg)
diff --git a/nimble/host/src/ble_l2cap_coc.c b/nimble/host/src/ble_l2cap_coc.c
index dada8b0c..9cc06991 100644
--- a/nimble/host/src/ble_l2cap_coc.c
+++ b/nimble/host/src/ble_l2cap_coc.c
@@ -96,6 +96,25 @@ ble_l2cap_coc_create_server(uint16_t psm, uint16_t mtu,
     return 0;
 }
 
+int
+ble_l2cap_coc_remove_server(uint16_t psm)
+{
+    struct ble_l2cap_coc_srv * srv;
+
+    if (ble_hs_conn_find_by_psm(psm) != NULL) {
+        return BLE_HS_EBUSY;
+    }
+
+    STAILQ_FOREACH(srv, &ble_l2cap_coc_srvs, next) {
+        if (srv->psm == psm) {
+            STAILQ_REMOVE(&ble_l2cap_coc_srvs, srv, ble_l2cap_coc_srv, next);
+            os_memblock_put(&ble_l2cap_coc_srv_pool, srv);
+            return 0;
+        }
+    }
+    return 0;
+}
+
 static inline void
 ble_l2cap_set_used_cid(uint32_t *cid_mask, int bit)
 {
diff --git a/nimble/host/src/ble_l2cap_coc_priv.h b/nimble/host/src/ble_l2cap_coc_priv.h
index 5ebdaa05..21fa7b9d 100644
--- a/nimble/host/src/ble_l2cap_coc_priv.h
+++ b/nimble/host/src/ble_l2cap_coc_priv.h
@@ -57,6 +57,7 @@ struct ble_l2cap_coc_srv {
 int ble_l2cap_coc_init(void);
 int ble_l2cap_coc_create_server(uint16_t psm, uint16_t mtu,
                                 ble_l2cap_event_fn *cb, void *cb_arg);
+int ble_l2cap_coc_remove_server(uint16_t psm);
 int ble_l2cap_coc_create_srv_chan(struct ble_hs_conn *conn, uint16_t psm,
                                   struct ble_l2cap_chan **chan);
 struct ble_l2cap_chan * ble_l2cap_coc_chan_alloc(struct ble_hs_conn *conn,

I have added the function ble_hs_conn_find_by_psm (analogue to many other find_by_xyz in the same file) that queries the connections by psm (leveraging another new function ble_hs_conn_chan_exists_for_psm that traverses all the channels of a given connection). If it is present, we return BLE_HS_EBUSY.

[rymanluk]

looks good, please go ahead with PR. Thanks

[rymanluk]

Hi @mickeyl don't remember if we have it fixed or this is still todo ?:)

[mickeyl]

It's still open, I didn't have time yet to do the pull request though. The code pasted should work though.

[rymanluk]

Alright, I bet that once you push PR it will be quickly merged.. Keep fingers crossed for you to find that time :D