Skip to content
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

drivers/cpu: add function to get CPU id/serial number #854

Merged
merged 2 commits into from
Jul 31, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 4, 2014

See OlegHahm/thirdparty_cpu#19 for real world example.

@LudwigKnuepfer
Copy link
Member

Where is the connection to #19 ?

@LudwigKnuepfer
Copy link
Member

Ah I guess you used the wrong repo but the right number...

@LudwigKnuepfer
Copy link
Member

Still it's circular reasoning - what should this be used for?

@OlegHahm
Copy link
Member

OlegHahm commented Mar 4, 2014

I think it might be used to generate a host UID or EUI for the network interface.

@miri64
Copy link
Member Author

miri64 commented Mar 4, 2014

First: we discussed some time ago to use the cpu id as a (bad) source of randomness. Second: see #837

@miri64
Copy link
Member Author

miri64 commented Mar 4, 2014

(maybe @haukepetersen and @thomaseichinger could look over it, too, and give suggestions for other CPU implementations)

@OlegHahm
Copy link
Member

OlegHahm commented Mar 4, 2014

ACK

@@ -27,6 +27,9 @@
/* TODO: choose better value? */
#define F_CPU 1000000

#define CPU_ID(id) id[0] = 1; id[1] = 2; id[2] = 3; id[3] = 4; \
id[4] = 5; id[5] = 6; id[6] = 7; id[7] = 8; // TODO: find a source.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with the process ID (compare #700) for starters.

@miri64
Copy link
Member Author

miri64 commented Mar 4, 2014

Applied @LudwigOrtmann's and @kaspar030's comments.

@kaspar030
Copy link
Contributor

ACK for the core change.

pid_t pid = getpid();

memset(id, 0, id_len);
memcpy(&id[0], &pid, id_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a double memset?
why &id[0] same as id ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #854 (comment) is integrated it's needed.

@miri64
Copy link
Member Author

miri64 commented Mar 5, 2014

Added a commit with an alternative suggestion based on OlegHahm/thirdparty_cpu#21 (comment) please review.

*/
typedef struct {
char cpu_id[CPU_ID_MAX_LEN];
int cpu_id_len;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is cpu_id_len needed for? Can't the cpu set CPU_ID_MAX_LEN?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean - without cpu_id_len one could simply use sizeof(cpu_id_t)..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then how do you propose to set CPU_ID_MAX_LEN to sizeof(pid_t) on native without including unistd.h and risking this way to come into conflict with our own posix port?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah hm uhm eh... one could get nasty here and define it via make

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean - have a test-program that determines this...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long can only be trusted to be >= int.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But at least for the *nix systems we support it seems to be the source for the typedef of pid_t

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we can just say cpu_id_len may allways be 4 in native, and if the pid does not supply enough bytes for the rest is set to 0xff or 0x00 or whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason to trust pid_t to fit in long would be that:

blksize_t, pid_t, and ssize_t shall be signed integer types

The implementation shall support one or more programming environments in which the widths of blksize_t, pid_t, size_t, ssize_t, suseconds_t, and useconds_t are no greater than the width of type long.

(http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/types.h.html)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, I'd memset it before like @authmillenon said.

@miri64
Copy link
Member Author

miri64 commented Mar 6, 2014

Applied discussed changes and rebased. Also: updated #837 accordingly.

@LudwigKnuepfer
Copy link
Member

ACK

@miri64
Copy link
Member Author

miri64 commented Mar 7, 2014

Needs second ACK because of core change (Re-ACK from @kaspar030?)

extern void native_cpu_id(cpu_id_t *id);

#undef GET_CPU_ID
#define GET_CPU_ID(id) native_cpu_id(&id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put parentheses around the macro argument.

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2014

Rebased, squashed and applied suggestions.

@thomaseichinger
Copy link
Member

needs rebase

@miri64
Copy link
Member Author

miri64 commented May 6, 2014

done

@Kijewski
Copy link
Contributor

Kijewski commented May 6, 2014

(still/again) needs rebase

@LudwigKnuepfer
Copy link
Member

@authmillenon ping

@OlegHahm
Copy link
Member

Please rebase.

@miri64
Copy link
Member Author

miri64 commented Jun 23, 2014

Done

@miri64
Copy link
Member Author

miri64 commented Jun 24, 2014

Uses functionality of #1192

@miri64
Copy link
Member Author

miri64 commented Jun 24, 2014

And addressed comments in #837

@@ -0,0 +1,41 @@
/*
* Copyright (C) 2013 Freie Universität Berlin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2014

@OlegHahm
Copy link
Member

re-ACK

@LudwigKnuepfer
Copy link
Member

Maybe you can refresh my memory - why is GET_CPU_ID realized as a macro?

If it really should be implemented as a macro I'd opt for the documentation to indicate that it can't be trusted.
And wouldn't it be good to at least warn if it is used?

@haukepetersen
Copy link
Contributor

Just was thinking the same thing as @LudwigOrtmann: what reason did we have to implement it as a macro, and why should it be part of the kernel?

Now that I look at it: wouldn't it make more sense to specify a driver interface that reads out the cpu_id?

@miri64
Copy link
Member Author

miri64 commented Jul 7, 2014

I think my reasoning behind using a macro was that it mostly is just the reading of some registers, but true: this can also be done in a function. For the driver: You might be right. I'll look into it.

@LudwigKnuepfer
Copy link
Member

You a-lookin' yet?

@miri64
Copy link
Member Author

miri64 commented Jul 26, 2014

Since there are more pressing issues right now, I postponed that. ;-)

But the general idea is: yes it will be some sort of cpu or random-source "driver".

@LudwigKnuepfer
Copy link
Member

I just thought the only thing left to do was creating this function..

@LudwigKnuepfer
Copy link
Member

... and this is a fix me first ...

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2014

I discussed this with @haukepetersen and we both agreed that it would make much more sence to put this function into a driver header instead of kernel.h.

Apart from that, the more pressing issues I'm working are also fix me firsts and are much more needed functionality (working ND, working Border Router) ;-)

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2014

You can close this PR, if it irritates you...

@miri64
Copy link
Member Author

miri64 commented Jul 31, 2014

getter is now a function in its own driver module

@LudwigKnuepfer
Copy link
Member

You made Travis cry.

@@ -57,4 +57,6 @@
/* for nativenet */
#define NATIVE_ETH_PROTO 0x1234

#define CPUID_ID_LEN (4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest a comment on this define.

@LudwigKnuepfer
Copy link
Member

Æck!

@miri64 miri64 changed the title core: add function to get CPU id/serial number drivers/cpu: add function to get CPU id/serial number Jul 31, 2014
@haukepetersen
Copy link
Contributor

ACK

miri64 added a commit that referenced this pull request Jul 31, 2014
drivers/cpu: add function to get CPU id/serial number
@miri64 miri64 merged commit 7516c94 into RIOT-OS:master Jul 31, 2014
@miri64 miri64 deleted the cpu-id branch August 14, 2014 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: native Platform: This PR/issue effects the native platform Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants