New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cc2538: Add PKA drivers, ECC algorithms and examples #1078

Merged
merged 1 commit into from Oct 21, 2015

Conversation

Projects
None yet
4 participants
@drandreas
Contributor

drandreas commented May 20, 2015

We implemented ECC algorithms and examples following the idea of Benoît Thébaudeau's crypto driver #1071. Our changes in cpu/cc2538/lpm.c do conflict pull request #1071. If both pull request are merged LPM_PERIPH_PERMIT_PM1_FUNCS_MAX has to be changed to 4.

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Jul 17, 2015

Member

Please squash your commits.

There are a few things that do not comply to https://github.com/contiki-os/contiki/wiki/Code-Contributions and https://github.com/contiki-os/contiki/blob/master/doc/code-style.c, e.g. the function names, the layout of function definitions like PKABigNumModStart(). There are also some tab characters used. Please fix. You can use Uncrustify.

There are a lot of useless double parentheses with REG(), obviously coming from TI's code.

For the function pointer parameters, always qualify the pointed type as const if the corresponding arguments are not supposed to be altered by the function.

Always restrict the scope of symbols as much as possible. E.g., nist_p_256_* should be made static. nist_p_256* and the like also seem to have to be const because they should be read-only, so only in flash memory.

It should be PKA_H_, not PKC_H_.

In the doxygen comments:

  • \param must be followed by the parameter name (as you did), but then there should not be a verb, because of the way the generated documentation is laid out.
  • After \return, there should not be Returns:.
  • Instead of having descriptions of return values in a bullet list after \return, you should use \retval for each value, which is the default way of doing the same with doxygen.

The architecture is good. There are many calls to memcpy() in the examples. I have not really looked into it, but this may mean that the API could be improved by using more pointers rather than always copying data.

@alignan: Can you test this after the rework (I do not have hardware)?

Member

bthebaudeau commented Jul 17, 2015

Please squash your commits.

There are a few things that do not comply to https://github.com/contiki-os/contiki/wiki/Code-Contributions and https://github.com/contiki-os/contiki/blob/master/doc/code-style.c, e.g. the function names, the layout of function definitions like PKABigNumModStart(). There are also some tab characters used. Please fix. You can use Uncrustify.

There are a lot of useless double parentheses with REG(), obviously coming from TI's code.

For the function pointer parameters, always qualify the pointed type as const if the corresponding arguments are not supposed to be altered by the function.

Always restrict the scope of symbols as much as possible. E.g., nist_p_256_* should be made static. nist_p_256* and the like also seem to have to be const because they should be read-only, so only in flash memory.

It should be PKA_H_, not PKC_H_.

In the doxygen comments:

  • \param must be followed by the parameter name (as you did), but then there should not be a verb, because of the way the generated documentation is laid out.
  • After \return, there should not be Returns:.
  • Instead of having descriptions of return values in a bullet list after \return, you should use \retval for each value, which is the default way of doing the same with doxygen.

The architecture is good. There are many calls to memcpy() in the examples. I have not really looked into it, but this may mean that the API could be improved by using more pointers rather than always copying data.

@alignan: Can you test this after the rework (I do not have hardware)?

@alignan

This comment has been minimized.

Show comment
Hide comment
@alignan

alignan Jul 18, 2015

Member

Sure, ping me when it's done

Member

alignan commented Jul 18, 2015

Sure, ping me when it's done

@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Aug 9, 2015

Contributor

Sorry for the late response, I was on vacation. I worked on the cc2538 platform as a part of my master thesis. Now, I’m absorbed by another job. I try to do the suggested changes but it will take another week or so.

Contributor

drandreas commented Aug 9, 2015

Sorry for the late response, I was on vacation. I worked on the cc2538 platform as a part of my master thesis. Now, I’m absorbed by another job. I try to do the suggested changes but it will take another week or so.

@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Aug 9, 2015

Contributor

@bthebaudeau The API is designed to fit TinyDTLS. For this reason all the structs allocate space.
If used with TinyDTLS, memcpy() is replaced with glue code, that converts between TinyDTLS' format and PKA's format.

Contributor

drandreas commented Aug 9, 2015

@bthebaudeau The API is designed to fit TinyDTLS. For this reason all the structs allocate space.
If used with TinyDTLS, memcpy() is replaced with glue code, that converts between TinyDTLS' format and PKA's format.

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Sep 6, 2015

Member

@drandreas

Maybe OK for the API/memcpy(), but why should Contiki have to care about tinyDTLS? I mean: if tinyDTLS wants to use Contiki, isn't there a way of adding glue code to tinyDTLS that does not impact Contiki? That's a minor issue though.

In your latest push, you have squashed your commits as requested, but you have ignored all the other comments (not only the API/memcpy() thing). Why?

Member

bthebaudeau commented Sep 6, 2015

@drandreas

Maybe OK for the API/memcpy(), but why should Contiki have to care about tinyDTLS? I mean: if tinyDTLS wants to use Contiki, isn't there a way of adding glue code to tinyDTLS that does not impact Contiki? That's a minor issue though.

In your latest push, you have squashed your commits as requested, but you have ignored all the other comments (not only the API/memcpy() thing). Why?

@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Sep 8, 2015

Contributor

I meant to do the remaining clean up after the rebase. I simply wasn't able reach the required quality yet. How long are you willing to wait? Either way all drivers are publicly available now (https://github.com/hosseinsh/Talos).

Contributor

drandreas commented Sep 8, 2015

I meant to do the remaining clean up after the rebase. I simply wasn't able reach the required quality yet. How long are you willing to wait? Either way all drivers are publicly available now (https://github.com/hosseinsh/Talos).

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Sep 9, 2015

Member

Please take the time you need. There's no hurry. The important thing is that we know what you intend to do. Thanks for your efforts.

Member

bthebaudeau commented Sep 9, 2015

Please take the time you need. There's no hurry. The important thing is that we know what you intend to do. Thanks for your efforts.

@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Oct 7, 2015

Contributor

@bthebaudeau I finished the rework. Could you please take another look at it?

Contributor

drandreas commented Oct 7, 2015

@bthebaudeau I finished the rework. Could you please take another look at it?

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Oct 7, 2015

Member

@drandreas OK, thanks, will do. Give me a few days.

Member

bthebaudeau commented Oct 7, 2015

@drandreas OK, thanks, will do. Give me a few days.

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Oct 10, 2015

Member

@drandreas That's much better! There are still a few things to fix:

  • extern can be dropped from everywhere in this commit.
  • There are still a lot of occurrences of REG((...)) that should be replaced with REG(...).
  • \retval must be directly followed by the return value, then by the description of this return value. Otherwise, the generated documentation is broken. So:
    • Remove the extra \c from \retval \c PKA_STATUS_ in cpu/cc2538/dev/bignum-driver.h.
    • Remove the commas following some \retval values, e.g. \retval PKA_STATUS_OPERATION_INPRG,.
  • There is a typo in cpu/cc2538/dev/bignum-driver.h: missing P in KA_STATUS_OPERATION_INPRG.
Member

bthebaudeau commented Oct 10, 2015

@drandreas That's much better! There are still a few things to fix:

  • extern can be dropped from everywhere in this commit.
  • There are still a lot of occurrences of REG((...)) that should be replaced with REG(...).
  • \retval must be directly followed by the return value, then by the description of this return value. Otherwise, the generated documentation is broken. So:
    • Remove the extra \c from \retval \c PKA_STATUS_ in cpu/cc2538/dev/bignum-driver.h.
    • Remove the commas following some \retval values, e.g. \retval PKA_STATUS_OPERATION_INPRG,.
  • There is a typo in cpu/cc2538/dev/bignum-driver.h: missing P in KA_STATUS_OPERATION_INPRG.
@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Oct 11, 2015

Contributor

Thank you for your remarks. I didn't squash the commits to make it easier for you. I will do it at the end.

I found five more REG((...)) statements where I consider dropping the brackets to be safe.
However in all other statements "sums" are passed to REG(...). Dropping brackets around arguments, with multiple elements, to Macros sounds unsafe to me?!

Contributor

drandreas commented Oct 11, 2015

Thank you for your remarks. I didn't squash the commits to make it easier for you. I will do it at the end.

I found five more REG((...)) statements where I consider dropping the brackets to be safe.
However in all other statements "sums" are passed to REG(...). Dropping brackets around arguments, with multiple elements, to Macros sounds unsafe to me?!

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Oct 11, 2015

Member

However in all other statements "sums" are passed to REG(...). Dropping brackets around arguments, with multiple elements, to Macros sounds unsafe to me?!

No, it's the macro definitions that have to be made safe by parenthesizing the used parameters, not the macro invocations. Here, the first such occurrence is REG((PKA_RAM_BASE + offset + 4 * i)) = pui32BNum[i];, and REG(x) is defined as (*((volatile unsigned long *)(x))), so the sum would already be parenthesized after macro expansion with a single level of parentheses.

4ff1e83 is fine for me, so you can now squash your commits.

Thanks.

Member

bthebaudeau commented Oct 11, 2015

However in all other statements "sums" are passed to REG(...). Dropping brackets around arguments, with multiple elements, to Macros sounds unsafe to me?!

No, it's the macro definitions that have to be made safe by parenthesizing the used parameters, not the macro invocations. Here, the first such occurrence is REG((PKA_RAM_BASE + offset + 4 * i)) = pui32BNum[i];, and REG(x) is defined as (*((volatile unsigned long *)(x))), so the sum would already be parenthesized after macro expansion with a single level of parentheses.

4ff1e83 is fine for me, so you can now squash your commits.

Thanks.

@drandreas

This comment has been minimized.

Show comment
Hide comment
@drandreas

drandreas Oct 12, 2015

Contributor

@alignan: Could you please confirm the functionality?

Contributor

drandreas commented Oct 12, 2015

@alignan: Could you please confirm the functionality?

@alignan

This comment has been minimized.

Show comment
Hide comment
@alignan

alignan Oct 12, 2015

Member

Verified just now and all three examples work, the timing is consistent in every run as well.

Member

alignan commented Oct 12, 2015

Verified just now and all three examples work, the timing is consistent in every run as well.

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau

bthebaudeau Oct 20, 2015

Member

@drandreas Thanks for your changes. When you push something, please add a comment to the pull request. Otherwise, we don't get notifications, and you might have to wait quite some time before we notice.

@alignan Your comments have now been addressed. Anything else to add before merging this PR?

Member

bthebaudeau commented Oct 20, 2015

@drandreas Thanks for your changes. When you push something, please add a comment to the pull request. Otherwise, we don't get notifications, and you might have to wait quite some time before we notice.

@alignan Your comments have now been addressed. Anything else to add before merging this PR?

@alignan

This comment has been minimized.

Show comment
Hide comment
@alignan

alignan Oct 21, 2015

Member

Nope, it is great to have this as it is now 👍

Member

alignan commented Oct 21, 2015

Nope, it is great to have this as it is now 👍

@bthebaudeau

This comment has been minimized.

Show comment
Hide comment
@bthebaudeau
Member

bthebaudeau commented Oct 21, 2015

👍

bthebaudeau added a commit that referenced this pull request Oct 21, 2015

Merge pull request #1078 from drandreas/cc2538-crypto
cc2538: Add PKA drivers, ECC algorithms and examples

@bthebaudeau bthebaudeau merged commit bf41de1 into contiki-os:master Oct 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@drandreas drandreas deleted the drandreas:cc2538-crypto branch Oct 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment