2016-03-08, 20:52:48 +0100, Johannes Berg wrote:

On Mon, 2016-03-07 at 18:12 +0100, Sabrina Dubroca wrote:

quoted

+++ b/include/uapi/linux/if_macsec.h

Some bits of documentation in this file, regarding all the various
operations and attributes, might be nice. At least the netlink types?

ok. Most of them are already indicated in the policies, but I can add
some comments here.

E.g., given this:

quoted

+#define DEFAULT_CIPHER_NAME "GCM-AES-128"
+#define DEFAULT_CIPHER_ID   0x0080020001000001ULL
+#define DEFAULT_CIPHER_ALT  0x0080C20001000001ULL

quoted

+enum macsec_attrs {

[...]

quoted

+	MACSEC_ATTR_CIPHER_SUITE,

should that be the ID, the alternate ID, or the string?

uh, the string is never actually used, I could get rid of it.

quoted

+	MACSEC_ATTR_ICV_LEN,
+	MACSEC_TXSA_LIST,
+	MACSEC_RXSC_LIST,
+	MACSEC_TXSC_STATS,
+	MACSEC_SECY_STATS,
+	MACSEC_ATTR_PROTECT,

This seems a bit inconsistent, MACSEC_ATTR_* vs. MACSEC_*?

Only the MACSEC_ATTR_* can be set, the others are just for dumping.

quoted

+enum macsec_sa_list_attrs {
+	MACSEC_SA_LIST_UNSPEC,
+	MACSEC_SA,
+	__MACSEC_ATTR_SA_LIST_MAX,
+	MACSEC_ATTR_SA_LIST_MAX = __MACSEC_ATTR_SA_LIST_MAX - 1,
+};

Again, without documentation, it seems odd to have an enum with just a
single useful entry? If you just wanted an array you don't need this at
all? The netlink nesting properties could be specified somewhere.

Yes, in dump_secy(), I nest the TXSA_LIST, and then each SA underneath
it.  I'm not sure how that would work without the list.  Can you have
an array without the dummy level of nesting?

quoted

+enum macsec_rxsc_list_attrs {
+	MACSEC_RXSC_LIST_UNSPEC,
+	MACSEC_RXSC,

similarly here

quoted

+enum macsec_rxsc_attrs {
+	MACSEC_ATTR_SC_UNSPEC,
+	/* use the same value to allow generic helper function, see
+	 * get_*_from_nl in drivers/net/macsec.c */
+	MACSEC_ATTR_SC_IFINDEX = MACSEC_ATTR_IFINDEX,
+	MACSEC_ATTR_SC_SCI = MACSEC_ATTR_SCI,

This seems odd, this must be nested inside the top-level attributes
since it's a single genl family, so why not use the top-level
attributes to start with?

If you need multiple you can use dump with multiple messages.

quoted

+enum macsec_sa_attrs {
+	MACSEC_ATTR_SA_UNSPEC,
+	/* use the same value to allow generic helper function, see
+	 * get_*_from_nl in drivers/net/macsec.c */
+	MACSEC_ATTR_SA_IFINDEX = MACSEC_ATTR_IFINDEX,
+	MACSEC_ATTR_SA_SCI = MACSEC_ATTR_SCI,

likewise here

quoted

+enum validation_type {
+	MACSEC_VALIDATE_DISABLED = 0,
+	MACSEC_VALIDATE_CHECK = 1,
+	MACSEC_VALIDATE_STRICT = 2,
+	__MACSEC_VALIDATE_MAX,
+};
+#define MACSEC_VALIDATE_MAX (__MACSEC_VALIDATE_MAX - 1)

everywhere else you put that into the enum, why not here?

Will fix.

quoted

+struct macsec_rx_sc_stats {
+	__u64 InOctetsValidated;
+	__u64 InOctetsDecrypted;
+	__u64 InPktsUnchecked;
+	__u64 InPktsDelayed;
+	__u64 InPktsOK;
+	__u64 InPktsInvalid;
+	__u64 InPktsLate;
+	__u64 InPktsNotValid;
+	__u64 InPktsNotUsingSA;
+	__u64 InPktsUnusedSA;
+};

It might be worth splitting those into attributes so that, if some
hardware offload can't provide all of the counters in the future, they
can just be left out of the netlink message?

These stats are defined by the standard, but marked optional.
A hardware device that doesn't implement some stat could just ignore
it and export 0.
I don't have a strong opinion about this.


Thanks,

-- 
Sabrina