From a8ef1ee984cee8ec0613df3161945e97426a6c33 Mon Sep 17 00:00:00 2001 From: Trond Einar Snekvik Date: Thu, 15 Apr 2021 12:28:20 +0200 Subject: [PATCH 1/2] Bluetooth: Mesh: Rework publication timer Periodic publication would previously build and send the first publication inside the bt_mesh_model_pub() function, before cancelling and rescheduling the next publication. The timer handler would only handle retransmissions, and would abandon the rest of the publication event if one of the packets failed to send. This design has three issues: - If the initial timer cancel fails, the publication would interfer with the periodic publication management, which might skip an event or send too many packets. - If any of the messages fail to publish, the full publication event would be abandoned. This is not predictable or expected from the API. - bt_mesh_model_pub() required 384 bytes of stack to build the message, which has to be factored into all calling threads. This patch moves all transmission into the publication timer by replacing k_work_cancel with a single k_work_reschedule(K_NO_WAIT). It also changes the error recovery behavior to attempt to finish the full publication event even if some of the transmissions fail. Split out from #33782. Signed-off-by: Trond Einar Snekvik --- include/bluetooth/mesh/access.h | 14 +-- subsys/bluetooth/mesh/access.c | 174 +++++++++++++------------------- subsys/bluetooth/mesh/cfg_srv.c | 14 ++- subsys/bluetooth/mesh/main.c | 9 +- 4 files changed, 95 insertions(+), 116 deletions(-) diff --git a/include/bluetooth/mesh/access.h b/include/bluetooth/mesh/access.h index 90de7aa41a91..294fd215fbdd 100644 --- a/include/bluetooth/mesh/access.h +++ b/include/bluetooth/mesh/access.h @@ -340,17 +340,17 @@ struct bt_mesh_model_pub { /** The model the context belongs to. Initialized by the stack. */ struct bt_mesh_model *mod; - uint16_t addr; /**< Publish Address. */ - uint16_t key:12, /**< Publish AppKey Index. */ - cred:1, /**< Friendship Credentials Flag. */ - send_rel:1; /**< Force reliable sending (segment acks) */ + uint16_t addr; /**< Publish Address. */ + uint16_t key:12, /**< Publish AppKey Index. */ + cred:1, /**< Friendship Credentials Flag. */ + send_rel:1, /**< Force reliable sending (segment acks) */ + fast_period:1; /**< Use FastPeriodDivisor */ uint8_t ttl; /**< Publish Time to Live. */ uint8_t retransmit; /**< Retransmit Count & Interval Steps. */ uint8_t period; /**< Publish Period. */ uint8_t period_div:4, /**< Divisor for the Period. */ - fast_period:1,/**< Use FastPeriodDivisor */ - count:3; /**< Retransmissions left. */ + count:4; /**< Transmissions left. */ uint32_t period_start; /**< Start of the current period. */ @@ -381,7 +381,7 @@ struct bt_mesh_model_pub { int (*update)(struct bt_mesh_model *mod); /** Publish Period Timer. Only for stack-internal use. */ - struct k_delayed_work timer; + struct k_work_delayable timer; }; /** @def BT_MESH_MODEL_PUB_DEFINE diff --git a/subsys/bluetooth/mesh/access.c b/subsys/bluetooth/mesh/access.c index dcc9adc7b3bc..65c1b6fb6709 100644 --- a/subsys/bluetooth/mesh/access.c +++ b/subsys/bluetooth/mesh/access.c @@ -150,7 +150,10 @@ static void publish_sent(int err, void *user_data) if (delay) { BT_DBG("Publishing next time in %dms", delay); - k_delayed_work_submit(&mod->pub->timer, K_MSEC(delay)); + /* Using schedule() in case the application has already called + * bt_mesh_publish, and a publication is pending. + */ + k_work_schedule(&mod->pub->timer, K_MSEC(delay)); } } @@ -161,6 +164,7 @@ static void publish_start(uint16_t duration, int err, void *user_data) if (err) { BT_ERR("Failed to publish: err %d", err); + publish_sent(err, user_data); return; } @@ -175,7 +179,7 @@ static const struct bt_mesh_send_cb pub_sent_cb = { .end = publish_sent, }; -static int publish_retransmit(struct bt_mesh_model *mod) +static int publish_transmit(struct bt_mesh_model *mod) { NET_BUF_SIMPLE_DEFINE(sdu, BT_MESH_TX_SDU_MAX); struct bt_mesh_model_pub *pub = mod->pub; @@ -192,67 +196,68 @@ static int publish_retransmit(struct bt_mesh_model *mod) net_buf_simple_add_mem(&sdu, pub->msg->data, pub->msg->len); - pub->count--; - return bt_mesh_trans_send(&tx, &sdu, &pub_sent_cb, mod); } -static void publish_retransmit_end(int err, struct bt_mesh_model_pub *pub) +static int pub_period_start(struct bt_mesh_model_pub *pub) { - /* Cancel all retransmits for this publish attempt */ - pub->count = 0U; - /* Make sure the publish timer gets reset */ - publish_sent(err, pub->mod); + int err; + + pub->count = BT_MESH_PUB_TRANSMIT_COUNT(pub->retransmit); + + if (!pub->update) { + return 0; + } + + err = pub->update(pub->mod); + if (err) { + /* Skip this publish attempt. */ + BT_DBG("Update failed, skipping publish (err: %d)", err); + pub->count = 0; + pub->period_start = k_uptime_get_32(); + publish_sent(err, pub); + return err; + } + + return 0; } static void mod_publish(struct k_work *work) { - struct bt_mesh_model_pub *pub = CONTAINER_OF(work, + struct k_work_delayable *dwork = k_work_delayable_from_work(work); + struct bt_mesh_model_pub *pub = CONTAINER_OF(dwork, struct bt_mesh_model_pub, - timer.work); - int32_t period_ms; + timer); int err; - BT_DBG(""); + if (pub->addr == BT_MESH_ADDR_UNASSIGNED || + atomic_test_bit(bt_mesh.flags, BT_MESH_SUSPENDED)) { + /* Publication is no longer active, but the cancellation of the + * delayed work failed. Abandon recurring timer. + */ + return; + } - period_ms = bt_mesh_model_pub_period_get(pub->mod); - BT_DBG("period %u ms", period_ms); + BT_DBG(""); if (pub->count) { - err = publish_retransmit(pub->mod); + pub->count--; + } else { + /* First publication in this period */ + err = pub_period_start(pub); if (err) { - BT_ERR("Failed to retransmit (err %d)", err); - - pub->count = 0U; - - /* Continue with normal publication */ - if (period_ms) { - k_delayed_work_submit(&pub->timer, - K_MSEC(period_ms)); - } + return; } - - return; } - if (!period_ms) { - return; - } - - __ASSERT_NO_MSG(pub->update != NULL); - - err = pub->update(pub->mod); + err = publish_transmit(pub->mod); if (err) { - /* Cancel this publish attempt. */ - BT_DBG("Update failed, skipping publish (err: %d)", err); - pub->period_start = k_uptime_get_32(); - publish_retransmit_end(err, pub); - return; - } + BT_ERR("Failed to publish (err %d)", err); + if (pub->count == BT_MESH_PUB_TRANSMIT_COUNT(pub->retransmit)) { + pub->period_start = k_uptime_get_32(); + } - err = bt_mesh_model_publish(pub->mod); - if (err) { - BT_ERR("Publishing failed (err %d)", err); + publish_sent(err, pub->mod); } } @@ -301,7 +306,7 @@ static void mod_init(struct bt_mesh_model *mod, struct bt_mesh_elem *elem, if (mod->pub) { mod->pub->mod = mod; - k_delayed_work_init(&mod->pub->timer, mod_publish); + k_work_init_delayable(&mod->pub->timer, mod_publish); } for (i = 0; i < ARRAY_SIZE(mod->keys); i++) { @@ -630,13 +635,17 @@ void bt_mesh_model_recv(struct bt_mesh_net_rx *rx, struct net_buf_simple *buf) } } -static int model_send(struct bt_mesh_model *model, - struct bt_mesh_net_tx *tx, bool implicit_bind, - struct net_buf_simple *msg, - const struct bt_mesh_send_cb *cb, void *cb_data) +int bt_mesh_model_send(struct bt_mesh_model *model, struct bt_mesh_msg_ctx *ctx, + struct net_buf_simple *msg, + const struct bt_mesh_send_cb *cb, void *cb_data) { - BT_DBG("net_idx 0x%04x app_idx 0x%04x dst 0x%04x", tx->ctx->net_idx, - tx->ctx->app_idx, tx->ctx->addr); + struct bt_mesh_net_tx tx = { + .ctx = ctx, + .src = bt_mesh_model_elem(model)->addr, + }; + + BT_DBG("net_idx 0x%04x app_idx 0x%04x dst 0x%04x", tx.ctx->net_idx, + tx.ctx->app_idx, tx.ctx->addr); BT_DBG("len %u: %s", msg->len, bt_hex(msg->data, msg->len)); if (!bt_mesh_is_provisioned()) { @@ -644,88 +653,49 @@ static int model_send(struct bt_mesh_model *model, return -EAGAIN; } - if (net_buf_simple_tailroom(msg) < 4) { - BT_ERR("Not enough tailroom for TransMIC"); + if (!model_has_key(model, tx.ctx->app_idx)) { + BT_ERR("Model not bound to AppKey 0x%04x", tx.ctx->app_idx); return -EINVAL; } - if (msg->len > BT_MESH_TX_SDU_MAX - 4) { - BT_ERR("Too big message"); - return -EMSGSIZE; - } - - if (!implicit_bind && !model_has_key(model, tx->ctx->app_idx)) { - BT_ERR("Model not bound to AppKey 0x%04x", tx->ctx->app_idx); - return -EINVAL; - } - - return bt_mesh_trans_send(tx, msg, cb, cb_data); -} - -int bt_mesh_model_send(struct bt_mesh_model *model, - struct bt_mesh_msg_ctx *ctx, - struct net_buf_simple *msg, - const struct bt_mesh_send_cb *cb, void *cb_data) -{ - struct bt_mesh_net_tx tx = { - .ctx = ctx, - .src = bt_mesh_model_elem(model)->addr, - }; - - return model_send(model, &tx, false, msg, cb, cb_data); + return bt_mesh_trans_send(&tx, msg, cb, cb_data); } int bt_mesh_model_publish(struct bt_mesh_model *model) { - NET_BUF_SIMPLE_DEFINE(sdu, BT_MESH_TX_SDU_MAX); struct bt_mesh_model_pub *pub = model->pub; if (!pub) { return -ENOTSUP; } - struct bt_mesh_msg_ctx ctx = { - .addr = pub->addr, - .send_ttl = pub->ttl, - .send_rel = pub->send_rel, - .app_idx = pub->key, - }; - struct bt_mesh_net_tx tx = { - .ctx = &ctx, - .src = bt_mesh_model_elem(model)->addr, - }; - int err; - BT_DBG(""); if (pub->addr == BT_MESH_ADDR_UNASSIGNED) { return -EADDRNOTAVAIL; } - if (pub->msg->len + 4 > BT_MESH_TX_SDU_MAX) { + if (!pub->msg || !pub->msg->len) { + BT_ERR("No publication message"); + return -EINVAL; + } + + if (pub->msg->len + BT_MESH_MIC_SHORT > BT_MESH_TX_SDU_MAX) { BT_ERR("Message does not fit maximum SDU size"); return -EMSGSIZE; } if (pub->count) { BT_WARN("Clearing publish retransmit timer"); - k_delayed_work_cancel(&pub->timer); } - net_buf_simple_add_mem(&sdu, pub->msg->data, pub->msg->len); - - tx.friend_cred = pub->cred; - - pub->count = BT_MESH_PUB_TRANSMIT_COUNT(pub->retransmit); + /* Account for initial transmission */ + pub->count = BT_MESH_PUB_TRANSMIT_COUNT(pub->retransmit) + 1; BT_DBG("Publish Retransmit Count %u Interval %ums", pub->count, BT_MESH_PUB_TRANSMIT_INT(pub->retransmit)); - err = model_send(model, &tx, true, &sdu, &pub_sent_cb, model); - if (err) { - publish_retransmit_end(err, pub); - return err; - } + k_work_reschedule(&pub->timer, K_NO_WAIT); return 0; } @@ -1208,7 +1178,7 @@ static void commit_mod(struct bt_mesh_model *mod, struct bt_mesh_elem *elem, if (ms > 0) { BT_DBG("Starting publish timer (period %u ms)", ms); - k_delayed_work_submit(&mod->pub->timer, K_MSEC(ms)); + k_work_schedule(&mod->pub->timer, K_MSEC(ms)); } } diff --git a/subsys/bluetooth/mesh/cfg_srv.c b/subsys/bluetooth/mesh/cfg_srv.c index c62ebb490c7b..589d85d19a20 100644 --- a/subsys/bluetooth/mesh/cfg_srv.c +++ b/subsys/bluetooth/mesh/cfg_srv.c @@ -201,7 +201,10 @@ static uint8_t _mod_pub_set(struct bt_mesh_model *model, uint16_t pub_addr, model->pub->count = 0U; if (model->pub->update) { - k_delayed_work_cancel(&model->pub->timer); + /* If this fails, the timer will check pub->addr and + * exit without transmitting. + */ + (void)k_work_cancel_delayable(&model->pub->timer); } if (IS_ENABLED(CONFIG_BT_SETTINGS) && store) { @@ -239,10 +242,13 @@ static uint8_t _mod_pub_set(struct bt_mesh_model *model, uint16_t pub_addr, BT_DBG("period %u ms", period_ms); if (period_ms > 0) { - k_delayed_work_submit(&model->pub->timer, - K_MSEC(period_ms)); + k_work_reschedule(&model->pub->timer, + K_MSEC(period_ms)); } else { - k_delayed_work_cancel(&model->pub->timer); + /* If this fails, publication will stop after the + * ongoing set of retransmits. + */ + (void)k_work_cancel_delayable(&model->pub->timer); } } diff --git a/subsys/bluetooth/mesh/main.c b/subsys/bluetooth/mesh/main.c index d470f7f8adea..0f16e0c7c4df 100644 --- a/subsys/bluetooth/mesh/main.c +++ b/subsys/bluetooth/mesh/main.c @@ -227,7 +227,10 @@ static void model_suspend(struct bt_mesh_model *mod, struct bt_mesh_elem *elem, { if (mod->pub && mod->pub->update) { mod->pub->count = 0U; - k_delayed_work_cancel(&mod->pub->timer); + /* If this fails, the work handler will check the suspend call + * and exit without transmitting. + */ + (void)k_work_cancel_delayable(&mod->pub->timer); } } @@ -268,8 +271,8 @@ static void model_resume(struct bt_mesh_model *mod, struct bt_mesh_elem *elem, int32_t period_ms = bt_mesh_model_pub_period_get(mod); if (period_ms) { - k_delayed_work_submit(&mod->pub->timer, - K_MSEC(period_ms)); + k_work_reschedule(&mod->pub->timer, + K_MSEC(period_ms)); } } } From 799c8045cc714cfbe5211bc987ac66ec6603c9b1 Mon Sep 17 00:00:00 2001 From: Trond Einar Snekvik Date: Thu, 15 Apr 2021 14:17:52 +0200 Subject: [PATCH 2/2] Bluetooth: Mesh: Transport length checks should account for MIC The Transport layer would previously rely on the access layer to check whether there's room for the full message and a MIC in the available buffer space, and its own checks would ignore the MIC. This should be handled by the Transport layer checks, so the access layer doesn't have to. Signed-off-by: Trond Einar Snekvik --- subsys/bluetooth/mesh/transport.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/subsys/bluetooth/mesh/transport.c b/subsys/bluetooth/mesh/transport.c index eb519b6b16b2..1564969a0b73 100644 --- a/subsys/bluetooth/mesh/transport.c +++ b/subsys/bluetooth/mesh/transport.c @@ -632,12 +632,12 @@ int bt_mesh_trans_send(struct bt_mesh_net_tx *tx, struct net_buf_simple *msg, return -EINVAL; } - if (msg->len > BT_MESH_TX_SDU_MAX) { - BT_ERR("Not enough segment buffers for length %u", msg->len); + if (msg->len > BT_MESH_TX_SDU_MAX - BT_MESH_MIC_SHORT) { + BT_ERR("Message too big: %u", msg->len); return -EMSGSIZE; } - if (net_buf_simple_tailroom(msg) < 4) { + if (net_buf_simple_tailroom(msg) < BT_MESH_MIC_SHORT) { BT_ERR("Insufficient tailroom for Transport MIC"); return -EINVAL; }