sfc: an enumeration is not a bitmask

sfc: an enumeration is not a bitmask

[David Miller]

Ben can you please get rid of "enum efx_fc_type"?

drivers/net/sfc/mcdi_mac.c: In function ‘efx_mcdi_set_mac’:
drivers/net/sfc/mcdi_mac.c:36:2: warning: case value ‘3’ not in enumerated type ‘enum efx_fc_type’

An enumeration is not a bitmask, instead it means one out of the set
of enumerated values will be used.  This means that the warning
here about:

	switch (efx->wanted_fc) {
	case EFX_FC_RX | EFX_FC_TX:

is completely legitimate.

Thanks.

Re: sfc: an enumeration is not a bitmask

[Ben Hutchings]

On Tue, 2011-05-17 at 14:14 -0400, David Miller wrote:
> Ben can you please get rid of "enum efx_fc_type"?
> 
> drivers/net/sfc/mcdi_mac.c: In function ‘efx_mcdi_set_mac’:
> drivers/net/sfc/mcdi_mac.c:36:2: warning: case value ‘3’ not in enumerated type ‘enum efx_fc_type’
> 
> An enumeration is not a bitmask, instead it means one out of the set
> of enumerated values will be used.  This means that the warning
> here about:
> 
> 	switch (efx->wanted_fc) {
> 	case EFX_FC_RX | EFX_FC_TX:
> 
> is completely legitimate.

I think this is common practice in C.  I filed a bug regarding the
warning at <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46457>, which
has not yet been resolved either way.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: sfc: an enumeration is not a bitmask

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 17 May 2011 19:18:39 +0100

> On Tue, 2011-05-17 at 14:14 -0400, David Miller wrote:
>> Ben can you please get rid of "enum efx_fc_type"?
>> 
>> drivers/net/sfc/mcdi_mac.c: In function ‘efx_mcdi_set_mac’:
>> drivers/net/sfc/mcdi_mac.c:36:2: warning: case value ‘3’ not in enumerated type ‘enum efx_fc_type’
>> 
>> An enumeration is not a bitmask, instead it means one out of the set
>> of enumerated values will be used.  This means that the warning
>> here about:
>> 
>> 	switch (efx->wanted_fc) {
>> 	case EFX_FC_RX | EFX_FC_TX:
>> 
>> is completely legitimate.
> 
> I think this is common practice in C.  I filed a bug regarding the
> warning at <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46457>, which
> has not yet been resolved either way.

I've been seeing this warning for at least a year, it could take yet
another year before it's "resolved" in a way that people will see the
warning go away on their computers, more likely longer.

I'm not waiting a year, or more, for something as trivial as this
warning to get cured, please fix this the way that I asked.

Thanks.

[PATCH net-next-2.6] sfc: Suppress warning about combining enum flags by gcc 4.5

[Ben Hutchings]

gcc 4.5 warns about switch statements on enumerated types containing
case values that are a bitwise-or of two enumerators for that type.
The use of enumerators as flags is common practice, so I think the
warning is wrong.  Keep the compiler quiet by casting the switch
value.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Will this do?

Ben.

 drivers/net/sfc/mcdi_mac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sfc/mcdi_mac.c b/drivers/net/sfc/mcdi_mac.c
index 50c2077..316745b 100644
--- a/drivers/net/sfc/mcdi_mac.c
+++ b/drivers/net/sfc/mcdi_mac.c
@@ -32,7 +32,7 @@ static int efx_mcdi_set_mac(struct efx_nic *efx)
 		(1 << MC_CMD_SET_MAC_IN_REJECT_UNCST_LBN);
 	MCDI_SET_DWORD(cmdbytes, SET_MAC_IN_REJECT, reject);
 
-	switch (efx->wanted_fc) {
+	switch ((unsigned)efx->wanted_fc) {
 	case EFX_FC_RX | EFX_FC_TX:
 		fcntl = MC_CMD_FCNTL_BIDIR;
 		break;
-- 
1.7.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6] sfc: Suppress warning about combining enum flags by gcc 4.5

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 17 May 2011 19:48:41 +0100

> gcc 4.5 warns about switch statements on enumerated types containing
> case values that are a bitwise-or of two enumerators for that type.
> The use of enumerators as flags is common practice, so I think the
> warning is wrong.  Keep the compiler quiet by casting the switch
> value.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> Will this do?

What part of "get rid of the enum" is so hard to understand?

I'll say it again: Please get rid of the enum efx_fc_type

You can define bit positions if you like, because those will
be used as a proper enum, as unique bit positions within
a set of bits.  Then you can define macros as (1 << XXX)

Thanks.

[PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Ben Hutchings]

gcc 4.5 doesn't like enumerators as flags, and neither does davem.
Replace the enumerated type with a 'bitwise' type alias which makes it
clear where these flags are being used and allows sparse to validate
their use.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
You wrote:
> What part of "get rid of the enum" is so hard to understand?
>
> I'll say it again: Please get rid of the enum efx_fc_type

OK, done.

> You can define bit positions if you like, because those will
> be used as a proper enum, as unique bit positions within
> a set of bits.  Then you can define macros as (1 << XXX)

But that seems to result in discarding all type information for the
flags. I would prefer to improve type checking.

Ben.

 drivers/net/sfc/efx.c        |    2 +-
 drivers/net/sfc/efx.h        |    2 +-
 drivers/net/sfc/ethtool.c    |    2 +-
 drivers/net/sfc/mdio_10g.c   |   13 ++++++++-----
 drivers/net/sfc/mdio_10g.h   |    2 +-
 drivers/net/sfc/net_driver.h |   15 +++++++--------
 6 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 05502b3..f840879 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -833,7 +833,7 @@ void efx_link_set_advertising(struct efx_nic *efx, u32 advertising)
 	}
 }
 
-void efx_link_set_wanted_fc(struct efx_nic *efx, enum efx_fc_type wanted_fc)
+void efx_link_set_wanted_fc(struct efx_nic *efx, efx_fc_mode wanted_fc)
 {
 	efx->wanted_fc = wanted_fc;
 	if (efx->link_advertising) {
diff --git a/drivers/net/sfc/efx.h b/drivers/net/sfc/efx.h
index 3d83a1f..242d68b 100644
--- a/drivers/net/sfc/efx.h
+++ b/drivers/net/sfc/efx.h
@@ -142,6 +142,6 @@ static inline void efx_schedule_channel(struct efx_channel *channel)
 
 extern void efx_link_status_changed(struct efx_nic *efx);
 extern void efx_link_set_advertising(struct efx_nic *efx, u32);
-extern void efx_link_set_wanted_fc(struct efx_nic *efx, enum efx_fc_type);
+extern void efx_link_set_wanted_fc(struct efx_nic *efx, efx_fc_mode);
 
 #endif /* EFX_EFX_H */
diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
index 348437a..114829d 100644
--- a/drivers/net/sfc/ethtool.c
+++ b/drivers/net/sfc/ethtool.c
@@ -698,7 +698,7 @@ static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
 				      struct ethtool_pauseparam *pause)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
-	enum efx_fc_type wanted_fc, old_fc;
+	efx_fc_mode wanted_fc, old_fc;
 	u32 old_adv;
 	bool reset;
 	int rc = 0;
diff --git a/drivers/net/sfc/mdio_10g.c b/drivers/net/sfc/mdio_10g.c
index 7115914..9be5da6 100644
--- a/drivers/net/sfc/mdio_10g.c
+++ b/drivers/net/sfc/mdio_10g.c
@@ -284,18 +284,21 @@ void efx_mdio_an_reconfigure(struct efx_nic *efx)
 	efx_mdio_write(efx, MDIO_MMD_AN, MDIO_CTRL1, reg);
 }
 
-enum efx_fc_type efx_mdio_get_pause(struct efx_nic *efx)
+efx_fc_mode efx_mdio_get_pause(struct efx_nic *efx)
 {
-	BUILD_BUG_ON(EFX_FC_AUTO & (EFX_FC_RX | EFX_FC_TX));
+	BUILD_BUG_ON((__force u8)EFX_FC_TX != FLOW_CTRL_TX);
+	BUILD_BUG_ON((__force u8)EFX_FC_RX != FLOW_CTRL_RX);
 
 	if (!(efx->wanted_fc & EFX_FC_AUTO))
 		return efx->wanted_fc;
 
 	WARN_ON(!(efx->mdio.mmds & MDIO_DEVS_AN));
 
-	return mii_resolve_flowctrl_fdx(
-		mii_advertise_flowctrl(efx->wanted_fc),
-		efx_mdio_read(efx, MDIO_MMD_AN, MDIO_AN_LPA));
+	return (__force efx_fc_mode)
+		mii_resolve_flowctrl_fdx(
+			mii_advertise_flowctrl((__force int)
+					       (efx->wanted_fc & ~EFX_FC_AUTO)),
+			efx_mdio_read(efx, MDIO_MMD_AN, MDIO_AN_LPA));
 }
 
 int efx_mdio_test_alive(struct efx_nic *efx)
diff --git a/drivers/net/sfc/mdio_10g.h b/drivers/net/sfc/mdio_10g.h
index df07039..1616b13 100644
--- a/drivers/net/sfc/mdio_10g.h
+++ b/drivers/net/sfc/mdio_10g.h
@@ -92,7 +92,7 @@ extern void efx_mdio_an_reconfigure(struct efx_nic *efx);
 /* Get pause parameters from AN if available (otherwise return
  * requested pause parameters)
  */
-enum efx_fc_type efx_mdio_get_pause(struct efx_nic *efx);
+extern efx_fc_mode efx_mdio_get_pause(struct efx_nic *efx);
 
 /* Wait for specified MMDs to exit reset within a timeout */
 extern int efx_mdio_wait_reset_mmds(struct efx_nic *efx,
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index ce9697b..1c29775 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -448,12 +448,11 @@ enum nic_state {
 /* Forward declaration */
 struct efx_nic;
 
-/* Pseudo bit-mask flow control field */
-enum efx_fc_type {
-	EFX_FC_RX = FLOW_CTRL_RX,
-	EFX_FC_TX = FLOW_CTRL_TX,
-	EFX_FC_AUTO = 4,
-};
+/* Flow control flags */
+typedef unsigned int __bitwise efx_fc_mode;
+#define EFX_FC_TX	((__force efx_fc_mode) 1)
+#define EFX_FC_RX	((__force efx_fc_mode) 2)
+#define EFX_FC_AUTO	((__force efx_fc_mode) 4)
 
 /**
  * struct efx_link_state - Current state of the link
@@ -465,7 +464,7 @@ enum efx_fc_type {
 struct efx_link_state {
 	bool up;
 	bool fd;
-	enum efx_fc_type fc;
+	efx_fc_mode fc;
 	unsigned int speed;
 };
 
@@ -784,7 +783,7 @@ struct efx_nic {
 
 	bool promiscuous;
 	union efx_multicast_hash multicast_hash;
-	enum efx_fc_type wanted_fc;
+	efx_fc_mode wanted_fc;
 
 	atomic_t rx_reset;
 	enum efx_loopback_mode loopback_mode;
-- 
1.7.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 17 May 2011 22:06:19 +0100

> But that seems to result in discarding all type information for the
> flags. I would prefer to improve type checking.

That's why I don't want a solution like your cast patch, as that
takes the typing information away.

Accept that the compiler currently doesn't want to allow enums to be
used as bit-masks, don't paper around it.

I'm not applying this patch either, you think all of this "__force"
casting stuff is better?  No way.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Joe Perches]

On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
> Accept that the compiler currently doesn't want to allow enums to be
> used as bit-masks, don't paper around it.

A patch applied yesterday using enums as bitmasks:
http://patchwork.ozlabs.org/patch/95802/

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 17 May 2011 14:22:58 -0700

> On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
>> Accept that the compiler currently doesn't want to allow enums to be
>> used as bit-masks, don't paper around it.
> 
> A patch applied yesterday using enums as bitmasks:
> http://patchwork.ozlabs.org/patch/95802/

Thanks, fixed:

--------------------
ipv4: Don't use enums as bitmasks in ip_fragment.c

Noticed by Joe Perches.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/ip_fragment.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 9e1458d..0ad6035 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -81,12 +81,10 @@ struct ipq {
  * We want to check ECN values of all fragments, do detect invalid combinations.
  * In ipq->ecn, we store the OR value of each ip4_frag_ecn() fragment value.
  */
-enum {
-	IPFRAG_ECN_NOT_ECT	= 0x01, /* one frag had ECN_NOT_ECT */
-	IPFRAG_ECN_ECT_1	= 0x02, /* one frag had ECN_ECT_1 */
-	IPFRAG_ECN_ECT_0	= 0x04, /* one frag had ECN_ECT_0 */
-	IPFRAG_ECN_CE		= 0x08, /* one frag had ECN_CE */
-};
+#define	IPFRAG_ECN_NOT_ECT	0x01 /* one frag had ECN_NOT_ECT */
+#define	IPFRAG_ECN_ECT_1	0x02 /* one frag had ECN_ECT_1 */
+#define	IPFRAG_ECN_ECT_0	0x04 /* one frag had ECN_ECT_0 */
+#define	IPFRAG_ECN_CE		0x08 /* one frag had ECN_CE */
 
 static inline u8 ip4_frag_ecn(u8 tos)
 {
-- 
1.7.4.4

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Michał Mirosław]

2011/5/17 Joe Perches <joe@perches.com>:
> On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
>> Accept that the compiler currently doesn't want to allow enums to be
>> used as bit-masks, don't paper around it.
> A patch applied yesterday using enums as bitmasks:
> http://patchwork.ozlabs.org/patch/95802/

The compiler warning is about using enum-masks in switch() statement,
IIRC. In this case it might be enough to just add the mask to the
enum.

Best Regards,
Michał Mirosław

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Eric Dumazet]

Le mardi 17 mai 2011 à 14:22 -0700, Joe Perches a écrit :
> On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
> > Accept that the compiler currently doesn't want to allow enums to be
> > used as bit-masks, don't paper around it.
> 
> A patch applied yesterday using enums as bitmasks:
> http://patchwork.ozlabs.org/patch/95802/

Well, quite frankly I focused on solving a bug, not making beautiful
code. After reading convoluted RFC, going back to C is not exactly
straightforward.

I filled a (small) table with C99 initializers, its not the problem
David raised with :

switch (value) {
case XXX | YYY:   /* gcc warning */

To be honest, I was using plain #define and right before sending patch I
added one enum just because its less chars on screen for the reader.

No strong opinion on this.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Joe Perches]

On Tue, 2011-05-17 at 23:36 +0200, Eric Dumazet wrote:
> Le mardi 17 mai 2011 à 14:22 -0700, Joe Perches a écrit :
> > On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
> > > Accept that the compiler currently doesn't want to allow enums to be
> > > used as bit-masks, don't paper around it.
> > A patch applied yesterday using enums as bitmasks:
> > http://patchwork.ozlabs.org/patch/95802/
> Well, quite frankly I focused on solving a bug, not making beautiful
> code. After reading convoluted RFC, going back to C is not exactly
> straightforward.

Given some RFCs, not necessarily possible either.

> I filled a (small) table with C99 initializers, its not the problem
> David raised

True, but it was a similar use of or'd enums.

> To be honest, I was using plain #define and right before sending patch I
> added one enum just because its less chars on screen for the reader.
> No strong opinion on this.

Nor I honestly.  I don't mind either.

cheers, Joe

[PATCH net-next-2.6] sfc: Replace enum efx_fc_type with macros and type alias

[Ben Hutchings]

gcc 4.5 doesn't like enumerators as flags, and neither does davem.
Replace the enumerated type with macros and a type alias which makes
it clear where the flags are being used.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
On Tue, 2011-05-17 at 17:14 -0400, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 17 May 2011 22:06:19 +0100
> 
> > But that seems to result in discarding all type information for the
> > flags. I would prefer to improve type checking.
> 
> That's why I don't want a solution like your cast patch, as that
> takes the typing information away.
> 
> Accept that the compiler currently doesn't want to allow enums to be
> used as bit-masks, don't paper around it.
> 
> I'm not applying this patch either, you think all of this "__force"
> casting stuff is better?  No way.

OK, here it is with an ordinary type alias.  I still don't think this is
the right way to go, as neither gcc nor sparse can do any meaningful
type-checking on it.  The type alias should at least help readers.

Ben.

 drivers/net/sfc/efx.c        |    2 +-
 drivers/net/sfc/efx.h        |    2 +-
 drivers/net/sfc/ethtool.c    |    2 +-
 drivers/net/sfc/mdio_10g.c   |    2 +-
 drivers/net/sfc/mdio_10g.h   |    2 +-
 drivers/net/sfc/net_driver.h |   15 +++++++--------
 6 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 05502b3..f840879 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -833,7 +833,7 @@ void efx_link_set_advertising(struct efx_nic *efx, u32 advertising)
 	}
 }
 
-void efx_link_set_wanted_fc(struct efx_nic *efx, enum efx_fc_type wanted_fc)
+void efx_link_set_wanted_fc(struct efx_nic *efx, efx_fc_mode wanted_fc)
 {
 	efx->wanted_fc = wanted_fc;
 	if (efx->link_advertising) {
diff --git a/drivers/net/sfc/efx.h b/drivers/net/sfc/efx.h
index 3d83a1f..242d68b 100644
--- a/drivers/net/sfc/efx.h
+++ b/drivers/net/sfc/efx.h
@@ -142,6 +142,6 @@ static inline void efx_schedule_channel(struct efx_channel *channel)
 
 extern void efx_link_status_changed(struct efx_nic *efx);
 extern void efx_link_set_advertising(struct efx_nic *efx, u32);
-extern void efx_link_set_wanted_fc(struct efx_nic *efx, enum efx_fc_type);
+extern void efx_link_set_wanted_fc(struct efx_nic *efx, efx_fc_mode);
 
 #endif /* EFX_EFX_H */
diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
index 348437a..114829d 100644
--- a/drivers/net/sfc/ethtool.c
+++ b/drivers/net/sfc/ethtool.c
@@ -698,7 +698,7 @@ static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
 				      struct ethtool_pauseparam *pause)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
-	enum efx_fc_type wanted_fc, old_fc;
+	efx_fc_mode wanted_fc, old_fc;
 	u32 old_adv;
 	bool reset;
 	int rc = 0;
diff --git a/drivers/net/sfc/mdio_10g.c b/drivers/net/sfc/mdio_10g.c
index 7115914..1e748cf 100644
--- a/drivers/net/sfc/mdio_10g.c
+++ b/drivers/net/sfc/mdio_10g.c
@@ -284,7 +284,7 @@ void efx_mdio_an_reconfigure(struct efx_nic *efx)
 	efx_mdio_write(efx, MDIO_MMD_AN, MDIO_CTRL1, reg);
 }
 
-enum efx_fc_type efx_mdio_get_pause(struct efx_nic *efx)
+efx_fc_mode efx_mdio_get_pause(struct efx_nic *efx)
 {
 	BUILD_BUG_ON(EFX_FC_AUTO & (EFX_FC_RX | EFX_FC_TX));
 
diff --git a/drivers/net/sfc/mdio_10g.h b/drivers/net/sfc/mdio_10g.h
index df07039..1616b13 100644
--- a/drivers/net/sfc/mdio_10g.h
+++ b/drivers/net/sfc/mdio_10g.h
@@ -92,7 +92,7 @@ extern void efx_mdio_an_reconfigure(struct efx_nic *efx);
 /* Get pause parameters from AN if available (otherwise return
  * requested pause parameters)
  */
-enum efx_fc_type efx_mdio_get_pause(struct efx_nic *efx);
+extern efx_fc_mode efx_mdio_get_pause(struct efx_nic *efx);
 
 /* Wait for specified MMDs to exit reset within a timeout */
 extern int efx_mdio_wait_reset_mmds(struct efx_nic *efx,
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index ce9697b..a5aeba8 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -448,12 +448,11 @@ enum nic_state {
 /* Forward declaration */
 struct efx_nic;
 
-/* Pseudo bit-mask flow control field */
-enum efx_fc_type {
-	EFX_FC_RX = FLOW_CTRL_RX,
-	EFX_FC_TX = FLOW_CTRL_TX,
-	EFX_FC_AUTO = 4,
-};
+/* Flow control flags */
+typedef unsigned int efx_fc_mode;
+#define EFX_FC_RX	FLOW_CTRL_RX
+#define EFX_FC_TX	FLOW_CTRL_TX
+#define EFX_FC_AUTO	4
 
 /**
  * struct efx_link_state - Current state of the link
@@ -465,7 +464,7 @@ enum efx_fc_type {
 struct efx_link_state {
 	bool up;
 	bool fd;
-	enum efx_fc_type fc;
+	efx_fc_mode fc;
 	unsigned int speed;
 };
 
@@ -784,7 +783,7 @@ struct efx_nic {
 
 	bool promiscuous;
 	union efx_multicast_hash multicast_hash;
-	enum efx_fc_type wanted_fc;
+	efx_fc_mode wanted_fc;
 
 	atomic_t rx_reset;
 	enum efx_loopback_mode loopback_mode;
-- 
1.7.4

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with macros and type alias

[David Miller]

Too late Ben, I've already written this patch myself using "u8"
throughout since that also matches the return type of the mii_*()
routines that feed this value.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Jeff Garzik]

On 05/17/2011 05:14 PM, David Miller wrote:
> Accept that the compiler currently doesn't want to allow enums to be
> used as bit-masks, don't paper around it.

This practice is all over the kernel.

It's a bit silly to make such a big deal, because of that.

	Jeff

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Tue, 17 May 2011 22:01:01 -0400

> On 05/17/2011 05:14 PM, David Miller wrote:
>> Accept that the compiler currently doesn't want to allow enums to be
>> used as bit-masks, don't paper around it.
> 
> This practice is all over the kernel.
> 
> It's a bit silly to make such a big deal, because of that.

"We do something stupid everywhere" is never a good
argument for anything.

Re: [PATCH net-next-2.6] sfc: Replace enum efx_fc_type with a 'bitwise' type

[Jeff Garzik]

On 05/17/2011 10:23 PM, David Miller wrote:
> From: Jeff Garzik<jeff@garzik.org>
>
>> On 05/17/2011 05:14 PM, David Miller wrote:
>>> Accept that the compiler currently doesn't want to allow enums to be
>>> used as bit-masks, don't paper around it.
>>
>> This practice is all over the kernel.
>>
>> It's a bit silly to make such a big deal, because of that.
>
> "We do something stupid everywhere" is never a good
> argument for anything.

grepping across the kernel, it is clearly a useful shorthand technique 
for defining typed, named constants, including bitmasks, used widely 
across multiple subsystems, drivers and include/linux.

Given that this technique has passed review time and again, and that 
kernel hackers -do- find it useful for valid reasons, I guess the only 
remaining defense is to call something "stupid."

	Jeff

Re: sfc: an enumeration is not a bitmask

[Jeff Garzik]

2011/5/17 David Miller <davem@davemloft.net>:
> An enumeration is not a bitmask, instead it means one out of the set
> of enumerated values will be used.

It's a decade-old kernel practice to use 'enum' to define typed
constants, preferred over  macros that convey no type information and
disappear after cpp phase.

So your assertion about enumerations is demonstrably not true, as it
is often used in the kernel.  Call it enum abuse if you want, but it
is consistent with code all over the kernel.

That said, I agree that warnings should of course be addressed in some manner.

    Jeff

Re: sfc: an enumeration is not a bitmask

[Michał Mirosław]

2011/5/17 Jeff Garzik <jgarzik@pobox.com>:
> 2011/5/17 David Miller <davem@davemloft.net>:
>> An enumeration is not a bitmask, instead it means one out of the set
>> of enumerated values will be used.
>
> It's a decade-old kernel practice to use 'enum' to define typed
> constants, preferred over  macros that convey no type information and
> disappear after cpp phase.
>
> So your assertion about enumerations is demonstrably not true, as it
> is often used in the kernel.  Call it enum abuse if you want, but it
> is consistent with code all over the kernel.
>
> That said, I agree that warnings should of course be addressed in some manner.

Old age of the mistake doesn't make it correct.

Disappearance of the #defines can be resolved by using enum of bit
positions (and maybe field lengths) and #define of (1 << bit_position)
if it is useful for something to remain after preprocessing.

Best Regards,
Michał Mirosław

Re: sfc: an enumeration is not a bitmask

[Ben Hutchings]

On Tue, 2011-05-17 at 21:09 +0200, Michał Mirosław wrote:
> 2011/5/17 Jeff Garzik <jgarzik@pobox.com>:
> > 2011/5/17 David Miller <davem@davemloft.net>:
> >> An enumeration is not a bitmask, instead it means one out of the set
> >> of enumerated values will be used.
> >
> > It's a decade-old kernel practice to use 'enum' to define typed
> > constants, preferred over  macros that convey no type information and
> > disappear after cpp phase.
> >
> > So your assertion about enumerations is demonstrably not true, as it
> > is often used in the kernel.  Call it enum abuse if you want, but it
> > is consistent with code all over the kernel.
> >
> > That said, I agree that warnings should of course be addressed in some manner.
> 
> Old age of the mistake doesn't make it correct.
> 
> Disappearance of the #defines can be resolved by using enum of bit
> positions (and maybe field lengths) and #define of (1 << bit_position)
> if it is useful for something to remain after preprocessing.

The point is that there is no specific type information for macros,
whether they are simple literal numbers or left-shift expressions.

The type of an enumerator in C is actually that of the underlying
integer type, not the enumeration type as in C++.  However, a compiler
or static analysis tool (such as sparse) may keep track of both the
language-specified type and some other associated type of an expression
in order to diagnose possible type errors.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: sfc: an enumeration is not a bitmask

[Jeff Garzik]

On 05/17/2011 03:09 PM, Michał Mirosław wrote:
> 2011/5/17 Jeff Garzik<jgarzik@pobox.com>:
>> 2011/5/17 David Miller<davem@davemloft.net>:
>>> An enumeration is not a bitmask, instead it means one out of the set
>>> of enumerated values will be used.
>>
>> It's a decade-old kernel practice to use 'enum' to define typed
>> constants, preferred over  macros that convey no type information and
>> disappear after cpp phase.
>>
>> So your assertion about enumerations is demonstrably not true, as it
>> is often used in the kernel.  Call it enum abuse if you want, but it
>> is consistent with code all over the kernel.

> Old age of the mistake doesn't make it correct.

It is not a mistake, but a useful coding tool.

	Jeff

Re: sfc: an enumeration is not a bitmask

[Michał Mirosław]

W dniu 18 maja 2011 00:00 użytkownik Jeff Garzik <jgarzik@pobox.com> napisał:
> On 05/17/2011 03:09 PM, Michał Mirosław wrote:
>> 2011/5/17 Jeff Garzik<jgarzik@pobox.com>:
>>> 2011/5/17 David Miller<davem@davemloft.net>:
>>>> An enumeration is not a bitmask, instead it means one out of the set
>>>> of enumerated values will be used.
>>> It's a decade-old kernel practice to use 'enum' to define typed
>>> constants, preferred over  macros that convey no type information and
>>> disappear after cpp phase.
>>>
>>> So your assertion about enumerations is demonstrably not true, as it
>>> is often used in the kernel.  Call it enum abuse if you want, but it
>>> is consistent with code all over the kernel.
>> Old age of the mistake doesn't make it correct.
> It is not a mistake, but a useful coding tool.

Being it a mistake does not contradict usefulness. ;-)

C has bitfields for the uses where masks and shifts are usually used.
Unfortunately, those usually require more typing and sometimes compile
to worse code.

Best Regards,
Michał Mirosław