Bluetooth: Replace custom rand driver with standard entropy #6294

carlescufi · 2018-02-21T10:53:10Z

No description provided.

carlescufi · 2018-02-21T10:53:39Z

cvinayak · 2018-02-21T10:54:32Z

drivers/entropy/Kconfig.nrf5

-	# FIXME: nRF5 RNG driver can't co-exist with Bluetooth's HAL
-	# implementation yet
-	depends on ENTROPY_GENERATOR && !BT
+	depends on ENTROPY_GENERATOR


bump copyright year

this is done.

cvinayak · 2018-02-21T10:55:33Z

drivers/entropy/Kconfig.nrf5

+config ENTROPY_NRF5_PRI
+        int "RNG interrupt priority"
+        depends on ENTROPY_NRF5_RNG
+        range 0 1 if SOC_SERIES_NRF51X


Isnt 3 level of priority available now in Zephyr CM0? and 5 in CM4?

Not in CM4, but you are correct about CM0

cvinayak · 2018-02-21T10:56:06Z

drivers/entropy/Kconfig.nrf5

+        depends on ENTROPY_NRF5_RNG
+        range 0 1 if SOC_SERIES_NRF51X
+        range 0 5 if SOC_SERIES_NRF52X
+        default 1


Let keep this all the way to the lowest

cvinayak · 2018-02-21T10:57:54Z

drivers/entropy/entropy_nrf5.c

+
+#define RAND_DEFINE(name, len) u8_t name[sizeof(struct rand) + len]  __aligned(4)
+
+#define RAND_THREAD_THRESHOLD 4  /* at least access address */


Have these as Kconfig options, and checked by controller Kconfig?

cvinayak · 2018-02-21T10:59:34Z

include/entropy.h

+ * @retval bytes filled on success.
+ * @retval -ERRNO errno code on error.
+ */
+int entropy_nrf5_isr_get_entropy(struct device *device, u8_t *buf, u16_t len);


drop nrf5

Then I would have to include it in the API and I want to avoid this at this point.

Will wait for @galak

codecov-io · 2018-02-21T11:11:37Z

Codecov Report

Merging #6294 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6294   +/-   ##
=======================================
  Coverage   53.51%   53.51%           
=======================================
  Files         437      437           
  Lines       41420    41420           
  Branches     7935     7935           
=======================================
  Hits        22164    22164           
  Misses      16039    16039           
  Partials     3217     3217

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 570be1b...df4390f. Read the comment docs.

carlescufi · 2018-02-21T12:00:40Z

@GiulianoFranchetto this would solve the issues in #6049

cvinayak · 2018-02-21T12:04:45Z

drivers/entropy/entropy_nrf5.c

+
+#define RAND_DEFINE(name, len) u8_t name[sizeof(struct rand) + len]  __aligned(4)
+
+#define RAND_THREAD_LEN (CONFIG_ENTROPY_NRF5_THR_THRESHOLD + 1)


we need two more.... CONFIG_ENTROPY_NRF5_THR/ISR_SIZE, these have the "range CONFIG_ENTROPY_NRF5_THR/ISR_THRESHOLD 255"

and use (CONFIG_ENTROPY_NRF5_THR/ISR_SIZE + 1) as the context-safe ring buffer size.

cvinayak · 2018-02-21T12:05:22Z

drivers/entropy/Kconfig.nrf5

-	# FIXME: nRF5 RNG driver can't co-exist with Bluetooth's HAL
-	# implementation yet
-	depends on ENTROPY_GENERATOR && !BT
+	depends on ENTROPY_GENERATOR


this is done.

cvinayak · 2018-02-21T12:06:32Z

subsys/bluetooth/controller/ll_sw/ctrl.c

-		     pdu_ctrl_tx->llctrl.enc_rsp.skds);
-	rand_isr_get(sizeof(pdu_ctrl_tx->llctrl.enc_rsp.ivs),
-		     pdu_ctrl_tx->llctrl.enc_rsp.ivs);
+	entropy_nrf5_isr_get_entropy(_radio.entropy,


check if these additional parameter passing has ISR overheads.

carlescufi · 2018-02-21T13:24:33Z

@cvinayak addressed all your comments, including the generic API instead of an nrf5 one

galak

Some changes to consider.

galak · 2018-02-21T14:00:35Z

subsys/bluetooth/controller/crypto/crypto.c

-		}
+	struct device *entropy;
+
+	entropy = device_get_binding(CONFIG_ENTROPY_NAME);


Can this either use the entropy handle from the radio, or is there somewhere we can initialize this once, so we don't have to call device_get_binding on every call to bt_rand()

I plan to change bt_rand later anyway, and this is only used by the BLE Host sparingly.

galak · 2018-02-21T14:00:59Z

include/entropy.h

+ * @retval bytes filled on success.
+ * @retval -ERRNO errno code on error.
+ */
+static inline int entropy_isr_get_entropy(struct device *dev, u8_t *buffer,


How about we have _isr at the end of the function as the naming convention, I think that will be cleaner going forward (entropy_get_entropy_isr)

Agreed, will change

galak · 2018-02-21T14:01:33Z

include/entropy.h

+{
+	const struct entropy_driver_api *api = dev->driver_api;
+
+	__ASSERT(api->isr_get_entropy, "Callback pointer should not be NULL");


I wonder if we should do something like:

if (api->isr_get_entropy) { return ... } else { return -ENOTSUP; }

Since not every driver might support this.

Agreed, will change

this is one additional branching in ISR.

Only with kernel asserts enabled (CONFIG_DEBUG)

@cvinayak Sorry, I misread. @galak on second thought this is an ISR call, so usually critical. I'd rather __ASSERT() than return an error here.

cvinayak · 2018-02-21T14:00:16Z

drivers/entropy/entropy_nrf5.c


 struct entropy_nrf5_dev_data {
-	atomic_t user_count;
+


redundant line.

cvinayak · 2018-02-21T14:04:47Z

subsys/bluetooth/controller/Kconfig

+	select ENTROPY_GENERATOR
+	select ENTROPY_NRF5_RNG if SOC_FAMILY_NRF5
+	select ENTROPY_NRF5_BIAS_CORRECTION if SOC_FAMILY_NRF5
+


redundant line?

cvinayak · 2018-02-21T14:06:23Z

subsys/bluetooth/controller/ll_sw/ctrl.c

+	entropy_isr_get_entropy(_radio.entropy,
+				     pdu_ctrl_tx->llctrl.enc_rsp.skds,
+				     sizeof(pdu_ctrl_tx->llctrl.enc_rsp.skds));
+


redundant line

cvinayak · 2018-02-21T14:06:32Z

subsys/bluetooth/controller/ll_sw/ctrl.c

+	entropy_isr_get_entropy(_radio.entropy,
+				     pdu_ctrl_tx->llctrl.enc_req.skdm,
+				     sizeof(pdu_ctrl_tx->llctrl.enc_req.skdm));
+


redundant line

carlescufi · 2018-02-21T15:13:07Z

@galak @cvinayak addressed your comments

nvlsianpu · 2018-02-21T15:16:30Z

I'm analyzing the algorithm. I will put opinion soon.

nvlsianpu · 2018-02-21T15:41:57Z

drivers/entropy/entropy_nrf5.c

+
+		ret = isr((struct rand *)dev_data->isr, true);
+		if (ret != -EBUSY) {
+			ret = isr((struct rand *)dev_data->thr,


Why this call is made? Is this for flush random value in case of the race condition occurrence?

^^I mean the second param value - -ENOBUFS might be returned in two cases.

-ENOBUFS is returned to convey that a new rng value has not been put into ring buffer due to the ISR ring buffer being full (or no ISR ring buffer initialized). Hence, conveyed as "store" param to the next ring buffer (thread).

Same pattern of nested if-clause can be used to scale to any number of ring buffer population.

Thanks. Then i understood this properly. Everything is right here.

nvlsianpu · 2018-02-21T16:09:03Z

drivers/entropy/entropy_nrf5.c

+	last = rng->last;
+
+	if (first <= last) {
+		u8_t *d, *s;


u8_t *d, *s; u8_t avail; d = &rand[octets]; s = &rng->rand[first];

Put before if statement - this are code duplication.

carlescufi · 2018-02-21T16:10:45Z

@nvlsianpu I didn't write the driver, just ported it, so I will let @cvinayak speak about the implementation details.

nvlsianpu · 2018-02-21T16:19:02Z

drivers/entropy/entropy_nrf5.c

+		d = &rand[octets];
+		s = &rng->rand[first];
+
+		avail = rng->count - first;


can we have a comment here? - like fetch bytes form upper part of random buffer.

nvlsianpu · 2018-02-21T16:21:58Z

drivers/entropy/entropy_nrf5.c

+		}
+
+		if (octets && last) {
+			s = &rng->rand[0];


can we have a comment here? - like fetch bytes form lower part of random buffer.

nvlsianpu · 2018-02-21T16:32:31Z

drivers/entropy/entropy_nrf5.c

 };

 #define DEV_DATA(dev) \
 	((struct entropy_nrf5_dev_data *)(dev)->driver_data)

-static inline u8_t entropy_nrf5_get_u8(void)
+static size_t get(struct rand *rng, size_t octets, u8_t *rand)


It is possible to reorganize get algorithm to two if statement:

if (last < first) { // fetch bytes from upper rng buffer // update first, last, octets } if (octets && (first <= last) { // fetch bytes from lower rng buffer }

I don't see a big sense in do so - so I just I signal this.

Your suggestion translates to:
1 compare and 1 branch
plus
1 branch, 1 compare and 1 branch

whereas my top level if-clause translates to just 1 compare and 1 branch (my understanding).

Yes, something like this. Code would be more compact, but cons is increased execution time.
so keep as is.

cvinayak · 2018-02-21T19:14:54Z

drivers/entropy/entropy_nrf5.c

 };

 #define DEV_DATA(dev) \
 	((struct entropy_nrf5_dev_data *)(dev)->driver_data)

-static inline u8_t entropy_nrf5_get_u8(void)
+static size_t get(struct rand *rng, size_t octets, u8_t *rand)


Your suggestion translates to:
1 compare and 1 branch
plus
1 branch, 1 compare and 1 branch

whereas my top level if-clause translates to just 1 compare and 1 branch (my understanding).

cvinayak · 2018-02-21T19:20:55Z

drivers/entropy/entropy_nrf5.c

+	last = rng->last;
+
+	if (first <= last) {
+		u8_t *d, *s;


cvinayak · 2018-02-21T19:35:26Z

drivers/entropy/entropy_nrf5.c

+
+		ret = isr((struct rand *)dev_data->isr, true);
+		if (ret != -EBUSY) {
+			ret = isr((struct rand *)dev_data->thr,


-ENOBUFS is returned to convey that a new rng value has not been put into ring buffer due to the ISR ring buffer being full (or no ISR ring buffer initialized). Hence, conveyed as "store" param to the next ring buffer (thread).

Same pattern of nested if-clause can be used to scale to any number of ring buffer population.

carlescufi · 2018-02-22T01:01:53Z

@cvinayak pushed a fix for the crash. The issue was the wrong pointer passed to IRQ_CONNECT()

carlescufi · 2018-02-22T01:02:12Z

@nvlsianpu @galak this should be ready now

carlescufi · 2018-02-22T14:56:50Z

@cvinayak I'm happy with your changes but can't approve because I submitted the PR. Can you approve instead?
@galak can you take one last look at this? we've reworked this a bit

galak · 2018-02-23T15:10:47Z

include/drivers/entropy/nrf5_entropy.h

+#ifndef _NRF5_ENTROPY_H_
+#define _NRF5_ENTROPY_H_
+
+int entropy_get_entropy_thr(u8_t *buf, u16_t len);


Since you are using these outside of the driver, should have a bit more comment/api docs.

Agreed, fixed

galak

Why aren't we adding these APIs entropy_get_entropy_isr & entropy_get_entropy_thr to include/entropy.h?

If they are not going to be added there, they should have an nrf namespace.

carlescufi · 2018-02-23T15:23:55Z

Why aren't we adding these APIs entropy_get_entropy_isr & entropy_get_entropy_thr to include/entropy.h?

Because the issue is removing the device* parameter so that the call is faster.

galak · 2018-02-23T15:30:00Z

Because the issue is removing the device* parameter so that the call is faster.

How much faster? Faster that what other implementation? One in which we pass a pointer to the entropy device?

carlescufi · 2018-02-23T15:32:55Z

How much faster? Faster that what other implementation? One in which we pass a pointer to the entropy device?

The issue here is that we want to be able to use a standard entropy driver for the Link Layer, which has strict timing requirements. What @cvinayak has observed is that if we don't remove the parameter from the function call, as well as optimizing with -Ofast, the ISR does not have time to complete. We are trying to reach a compromise where we use a standard driver instead of having a BLE-specific one in subsys/bluetooth/controller/hal, but at the same time we can still perform fast encryption on an nRF51.

carlescufi · 2018-02-23T15:34:22Z

drivers/entropy/entropy_nrf5.c

+	return 0;
+}
+
+int entropy_get_entropy_thr(u8_t *buf, u16_t len)


@cvinayak do we really need this one? In thread mode we could use the standard one really

carlescufi · 2018-02-23T21:37:04Z

@cvinayak @galak updated after lengthy discussion

cvinayak · 2018-02-23T21:48:37Z

subsys/bluetooth/controller/ll_sw/ctrl.c

 #include "hal/ecb.h"
 #include "hal/ccm.h"
 #include "hal/radio.h"
 #include "hal/debug.h"
+#include "hal/radio.h"


cvinayak · 2018-02-23T21:52:12Z

subsys/bluetooth/controller/crypto/crypto.c

@@ -6,23 +6,28 @@

 #include <soc.h>

+#if defined(CONFIG_SOC_FAMILY_NRF5)


you dont need this here

cvinayak · 2018-02-23T21:56:00Z

subsys/bluetooth/controller/ll_sw/ctrl.c

@@ -5831,7 +5833,7 @@ static u32_t access_addr_get(void)
 	LL_ASSERT(retry);
 	retry--;

-	bt_rand(&access_addr, sizeof(u32_t));
+	bt_rand((u8_t *)&access_addr, sizeof(u32_t));


redundant change

cvinayak

LGTM

carlescufi · 2018-03-02T15:42:02Z

@galak we leave this out of 1.11, ready for 1.12. That said, it still needs your approval, I think all of your initial objections have been addressed.

durub · 2018-03-08T14:36:34Z

LGTM.

Thanks, @carlescufi! I was just needing this yesterday, having this ready for 1.12 is great news -- good job.

cvinayak

LGTM

The rand driver present in the BLE Link Layer is able to provide entropy for both threads and ISRs, and so it is more suited to be used as a generic nRF5x entropy driver instead of the current one. This will allow applications and the Link Layer to use the same driver without duplicating it. Signed-off-by: Carles Cufi <[email protected]> Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

After porting the rand driver to drivers/entropy, replace the usage of the old, Buetooth-specific driver with the generic entropy one. Signed-off-by: Carles Cufi <[email protected]> Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

carlescufi requested a review from cvinayak as a code owner February 21, 2018 10:53

carlescufi requested review from nvlsianpu and jhedberg February 21, 2018 10:53

carlescufi requested a review from anangl February 21, 2018 11:06

cvinayak requested changes Feb 21, 2018

View reviewed changes

cvinayak requested a review from galak February 21, 2018 11:13

carlescufi force-pushed the rand_entropy branch from ec344da to 4c14298 Compare February 21, 2018 11:57

cvinayak requested changes Feb 21, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from 4c14298 to 9e2510f Compare February 21, 2018 13:22

carlescufi force-pushed the rand_entropy branch from 9e2510f to c0cc6df Compare February 21, 2018 13:56

galak requested changes Feb 21, 2018

View reviewed changes

cvinayak requested changes Feb 21, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from c0cc6df to bc9d583 Compare February 21, 2018 15:13

nvlsianpu reviewed Feb 21, 2018

View reviewed changes

cvinayak requested changes Feb 21, 2018

View reviewed changes

cvinayak force-pushed the rand_entropy branch from bc9d583 to a721f29 Compare February 21, 2018 20:14

carlescufi force-pushed the rand_entropy branch from a721f29 to 93319e3 Compare February 22, 2018 01:01

carlescufi mentioned this pull request Feb 22, 2018

entropy: Clarify API contract #6187

Open

cvinayak force-pushed the rand_entropy branch from 93319e3 to 0eeeb27 Compare February 22, 2018 13:27

nvlsianpu approved these changes Feb 22, 2018

View reviewed changes

cvinayak approved these changes Feb 23, 2018

View reviewed changes

galak reviewed Feb 23, 2018

View reviewed changes

galak requested changes Feb 23, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from 0eeeb27 to 2e6a132 Compare February 23, 2018 15:22

carlescufi commented Feb 23, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from 2e6a132 to c52be4c Compare February 23, 2018 21:36

cvinayak requested changes Feb 23, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from c52be4c to 8aa8a2a Compare February 24, 2018 12:54

cvinayak approved these changes Feb 26, 2018

View reviewed changes

carlescufi modified the milestones: v1.11.0, v1.12.0 Mar 8, 2018

carlescufi force-pushed the rand_entropy branch 2 times, most recently from 9803006 to b43d63e Compare March 13, 2018 19:40

cvinayak approved these changes Mar 13, 2018

View reviewed changes

carlescufi force-pushed the rand_entropy branch from b43d63e to d884038 Compare March 13, 2018 22:04

carlescufi force-pushed the rand_entropy branch from d884038 to df4390f Compare March 14, 2018 09:34

galak approved these changes Mar 14, 2018

View reviewed changes

carlescufi merged commit fb3387a into zephyrproject-rtos:master Mar 14, 2018


		#define RAND_DEFINE(name, len) u8_t name[sizeof(struct rand) + len] __aligned(4)

		#define RAND_THREAD_THRESHOLD 4 /* at least access address */


		#define RAND_DEFINE(name, len) u8_t name[sizeof(struct rand) + len] __aligned(4)

		#define RAND_THREAD_LEN (CONFIG_ENTROPY_NRF5_THR_THRESHOLD + 1)

		@@ -6,23 +6,28 @@

		#include <soc.h>

		#if defined(CONFIG_SOC_FAMILY_NRF5)

Bluetooth: Replace custom rand driver with standard entropy #6294

Bluetooth: Replace custom rand driver with standard entropy #6294

Conversation

carlescufi commented Feb 21, 2018

carlescufi commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 21, 2018 • edited Loading

Codecov Report

carlescufi commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Feb 21, 2018

galak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Feb 21, 2018

nvlsianpu commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvlsianpu Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

cvinayak Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvinayak Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Feb 22, 2018

carlescufi commented Feb 22, 2018

carlescufi commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

carlescufi commented Feb 23, 2018

galak commented Feb 23, 2018

carlescufi commented Feb 23, 2018

Choose a reason for hiding this comment

carlescufi commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

carlescufi commented Mar 2, 2018

durub commented Mar 8, 2018

codecov-io commented Feb 21, 2018 •

edited

Loading

nvlsianpu Feb 21, 2018 •

edited

Loading

cvinayak Feb 21, 2018 •

edited

Loading

cvinayak Feb 21, 2018 •

edited

Loading