[v3,12/27] net/nfp: refact the hwinfo module
Checks
Commit Message
Move the definition of data structure and macro into the implement file.
Also remove the unneeded header file include statements.
Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
drivers/net/nfp/nfpcore/nfp_hwinfo.c | 84 +++++++++++++++++++++++++---
drivers/net/nfp/nfpcore/nfp_hwinfo.h | 71 +----------------------
2 files changed, 77 insertions(+), 78 deletions(-)
Comments
On 9/15/2023 10:15 AM, Chaoyong He wrote:
> Move the definition of data structure and macro into the implement file.
>
What is the motivation for this move, I can see same is done in multiple
other patches?
Header file is still included by .c file, what is the benefit of moving
from header to .c file?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Friday, September 15, 2023 9:46 PM
> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
> <niklas.soderlund@corigine.com>
> Subject: Re: [PATCH v3 12/27] net/nfp: refact the hwinfo module
>
> On 9/15/2023 10:15 AM, Chaoyong He wrote:
> > Move the definition of data structure and macro into the implement file.
> >
>
> What is the motivation for this move, I can see same is done in multiple other
> patches?
> Header file is still included by .c file, what is the benefit of moving from header
> to .c file?
We try to make the interface between modules as small as possible.
We meet some problem when we add new features, too much content in the header files make it difficult to form a identify dependencies among modules.
When we try to add a new data field in a structure or add a new data type in one header file, we had to modify many unrelated files just because it includes this header file, it's not good.
This is the direct motivation for this move, this will make the development easier.
On 9/18/2023 2:39 AM, Chaoyong He wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Friday, September 15, 2023 9:46 PM
>> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
>> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
>> <niklas.soderlund@corigine.com>
>> Subject: Re: [PATCH v3 12/27] net/nfp: refact the hwinfo module
>>
>> On 9/15/2023 10:15 AM, Chaoyong He wrote:
>>> Move the definition of data structure and macro into the implement file.
>>>
>>
>> What is the motivation for this move, I can see same is done in multiple other
>> patches?
>> Header file is still included by .c file, what is the benefit of moving from header
>> to .c file?
>
> We try to make the interface between modules as small as possible.
>
> We meet some problem when we add new features, too much content in the header files make it difficult to form a identify dependencies among modules.
> When we try to add a new data field in a structure or add a new data type in one header file, we had to modify many unrelated files just because it includes this header file, it's not good.
>
> This is the direct motivation for this move, this will make the development easier.>
If you can move a struct or data type to .c file, it means it is not
shared so updating it should not cause change anyway.
I am not against it, but I didn't get your reasoning.
When there are 'nfp_hwinfo.c' and 'nfp_hwinfo.h' files, I think that is
reasonable to have "nfp_hwinfo*" macros and structs in the .h file, it
looks like you want to separate data structures that are shared and ones
that are only used by 'nfp_hwinfo.c' but I am not sure how it helps.
Anyway if you know what you are doing, it is OK, I just want to double
check if this is done on purpose.
@@ -17,17 +17,82 @@
* (ie, in this example, ME 39 has been reserved by boardconfig.)
*/
-#include <stdio.h>
-#include <time.h>
+#include "nfp_hwinfo.h"
-#include "nfp_cpp.h"
+#include "nfp_crc.h"
#include "nfp_logs.h"
-#include "nfp6000/nfp6000.h"
#include "nfp_resource.h"
-#include "nfp_hwinfo.h"
-#include "nfp_crc.h"
-static int
+#define HWINFO_SIZE_MIN 0x100
+
+/*
+ * The Hardware Info Table defines the properties of the system.
+ *
+ * HWInfo v1 Table (fixed size)
+ *
+ * 0x0000: uint32_t version Hardware Info Table version (1.0)
+ * 0x0004: uint32_t size Total size of the table, including the
+ * CRC32 (IEEE 802.3)
+ * 0x0008: uint32_t jumptab Offset of key/value table
+ * 0x000c: uint32_t keys Total number of keys in the key/value table
+ * NNNNNN: Key/value jump table and string data
+ * (size - 4): uint32_t crc32 CRC32 (same as IEEE 802.3, POSIX csum, etc)
+ * CRC32("",0) = ~0, CRC32("a",1) = 0x48C279FE
+ *
+ * HWInfo v2 Table (variable size)
+ *
+ * 0x0000: uint32_t version Hardware Info Table version (2.0)
+ * 0x0004: uint32_t size Current size of the data area, excluding CRC32
+ * 0x0008: uint32_t limit Maximum size of the table
+ * 0x000c: uint32_t reserved Unused, set to zero
+ * NNNNNN: Key/value data
+ * (size - 4): uint32_t crc32 CRC32 (same as IEEE 802.3, POSIX csum, etc)
+ * CRC32("",0) = ~0, CRC32("a",1) = 0x48C279FE
+ *
+ * If the HWInfo table is in the process of being updated, the low bit of
+ * version will be set.
+ *
+ * HWInfo v1 Key/Value Table
+ * -------------------------
+ *
+ * The key/value table is a set of offsets to ASCIIZ strings which have
+ * been strcmp(3) sorted (yes, please use bsearch(3) on the table).
+ *
+ * All keys are guaranteed to be unique.
+ *
+ * N+0: uint32_t key_1 Offset to the first key
+ * N+4: uint32_t val_1 Offset to the first value
+ * N+8: uint32_t key_2 Offset to the second key
+ * N+c: uint32_t val_2 Offset to the second value
+ * ...
+ *
+ * HWInfo v2 Key/Value Table
+ * -------------------------
+ *
+ * Packed UTF8Z strings, ie 'key1\000value1\000key2\000value2\000'
+ * Unsorted.
+ *
+ * Note: Only the HwInfo v2 Table be supported now.
+ */
+
+#define NFP_HWINFO_VERSION_1 ('H' << 24 | 'I' << 16 | 1 << 8 | 0 << 1 | 0)
+#define NFP_HWINFO_VERSION_2 ('H' << 24 | 'I' << 16 | 2 << 8 | 0 << 1 | 0)
+#define NFP_HWINFO_VERSION_UPDATING RTE_BIT32(0)
+
+struct nfp_hwinfo {
+ uint8_t start[0];
+
+ uint32_t version;
+ uint32_t size;
+
+ /* V2 specific fields */
+ uint32_t limit;
+ uint32_t resv;
+
+ char data[];
+};
+
+static bool
nfp_hwinfo_is_updating(struct nfp_hwinfo *hwinfo)
{
return hwinfo->version & NFP_HWINFO_VERSION_UPDATING;
@@ -120,7 +185,7 @@ nfp_hwinfo_try_fetch(struct nfp_cpp *cpp,
goto exit_free;
}
- header = (void *)db;
+ header = (struct nfp_hwinfo *)db;
if (nfp_hwinfo_is_updating(header))
goto exit_free;
@@ -133,7 +198,8 @@ nfp_hwinfo_try_fetch(struct nfp_cpp *cpp,
/* NULL-terminate for safety */
db[*cpp_size] = '\0';
- return (void *)db;
+ return (struct nfp_hwinfo *)db;
+
exit_free:
rte_free(db);
return NULL;
@@ -6,76 +6,9 @@
#ifndef __NFP_HWINFO_H__
#define __NFP_HWINFO_H__
-#include <inttypes.h>
+#include "nfp_cpp.h"
-#define HWINFO_SIZE_MIN 0x100
-
-/*
- * The Hardware Info Table defines the properties of the system.
- *
- * HWInfo v1 Table (fixed size)
- *
- * 0x0000: uint32_t version Hardware Info Table version (1.0)
- * 0x0004: uint32_t size Total size of the table, including the
- * CRC32 (IEEE 802.3)
- * 0x0008: uint32_t jumptab Offset of key/value table
- * 0x000c: uint32_t keys Total number of keys in the key/value table
- * NNNNNN: Key/value jump table and string data
- * (size - 4): uint32_t crc32 CRC32 (same as IEEE 802.3, POSIX csum, etc)
- * CRC32("",0) = ~0, CRC32("a",1) = 0x48C279FE
- *
- * HWInfo v2 Table (variable size)
- *
- * 0x0000: uint32_t version Hardware Info Table version (2.0)
- * 0x0004: uint32_t size Current size of the data area, excluding CRC32
- * 0x0008: uint32_t limit Maximum size of the table
- * 0x000c: uint32_t reserved Unused, set to zero
- * NNNNNN: Key/value data
- * (size - 4): uint32_t crc32 CRC32 (same as IEEE 802.3, POSIX csum, etc)
- * CRC32("",0) = ~0, CRC32("a",1) = 0x48C279FE
- *
- * If the HWInfo table is in the process of being updated, the low bit of
- * version will be set.
- *
- * HWInfo v1 Key/Value Table
- * -------------------------
- *
- * The key/value table is a set of offsets to ASCIIZ strings which have
- * been strcmp(3) sorted (yes, please use bsearch(3) on the table).
- *
- * All keys are guaranteed to be unique.
- *
- * N+0: uint32_t key_1 Offset to the first key
- * N+4: uint32_t val_1 Offset to the first value
- * N+8: uint32_t key_2 Offset to the second key
- * N+c: uint32_t val_2 Offset to the second value
- * ...
- *
- * HWInfo v2 Key/Value Table
- * -------------------------
- *
- * Packed UTF8Z strings, ie 'key1\000value1\000key2\000value2\000'
- * Unsorted.
- *
- * Note: Only the HwInfo v2 Table be supported now.
- */
-
-#define NFP_HWINFO_VERSION_1 ('H' << 24 | 'I' << 16 | 1 << 8 | 0 << 1 | 0)
-#define NFP_HWINFO_VERSION_2 ('H' << 24 | 'I' << 16 | 2 << 8 | 0 << 1 | 0)
-#define NFP_HWINFO_VERSION_UPDATING RTE_BIT32(0)
-
-struct nfp_hwinfo {
- uint8_t start[0];
-
- uint32_t version;
- uint32_t size;
-
- /* v2 specific fields */
- uint32_t limit;
- uint32_t resv;
-
- char data[];
-};
+struct nfp_hwinfo;
struct nfp_hwinfo *nfp_hwinfo_read(struct nfp_cpp *cpp);