[PATCH 1/2]: Renumber PCI error enums to start at zero

[PATCH 1/2]: Renumber PCI error enums to start at zero

[Linas Vepstas]

Greg,

This is a low-prioriity patch to fix an annoying numbering mistake. 
Please apply this (and the next patch) at net convenience.

--linas

Subject: [PATCH 1/2]: Renumber PCI error enums to start at zero

Renumber the PCI error enums to start at zero for "normal/online".
This allows un-initialized pci channel state (which defaults to zero)
to be interpreted as "normal".  Add very simple routine to check
state, just in case this ever has to be fiddled with again.

Signed-off-by: Linas Vepstas <linas@linas.org>

----
 include/linux/pci.h |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6.19-rc4-git3/include/linux/pci.h
===================================================================
--- linux-2.6.19-rc4-git3.orig/include/linux/pci.h	2006-11-01 16:15:49.000000000 -0600
+++ linux-2.6.19-rc4-git3/include/linux/pci.h	2006-11-01 16:20:49.000000000 -0600
@@ -86,15 +86,20 @@ typedef unsigned int __bitwise pci_chann
 
 enum pci_channel_state {
 	/* I/O channel is in normal state */
-	pci_channel_io_normal = (__force pci_channel_state_t) 1,
+	pci_channel_io_normal = (__force pci_channel_state_t) 0,
 
 	/* I/O to channel is blocked */
-	pci_channel_io_frozen = (__force pci_channel_state_t) 2,
+	pci_channel_io_frozen = (__force pci_channel_state_t) 1,
 
 	/* PCI card is dead */
-	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
+	pci_channel_io_perm_failure = (__force pci_channel_state_t) 2,
 };
 
+static inline int pci_channel_offline(pci_channel_state_t state)
+{
+	return (state != pci_channel_io_normal);
+}
+
 typedef unsigned short __bitwise pci_bus_flags_t;
 enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1,

[PATCH 1/2]: Renumber PCI error enums to start at zero

[Linas Vepstas]

Greg, Andrew,

This patch fixes an annoying numbering mistake. 
Please apply this (and the next patch).

--linas

Subject: [PATCH 1/2]: Renumber PCI error enums to start at zero

Renumber the PCI error enums to start at zero for "normal/online".
This allows un-initialized pci channel state (which defaults to zero)
to be interpreted as "normal".  Add very simple routine to check
state, just in case this ever has to be fiddled with again.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 include/linux/pci.h |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6.19-git7/include/linux/pci.h
===================================================================
--- linux-2.6.19-git7.orig/include/linux/pci.h	2006-12-05 17:11:02.000000000 -0600
+++ linux-2.6.19-git7/include/linux/pci.h	2006-12-05 17:12:12.000000000 -0600
@@ -87,13 +87,13 @@ typedef unsigned int __bitwise pci_chann
 
 enum pci_channel_state {
 	/* I/O channel is in normal state */
-	pci_channel_io_normal = (__force pci_channel_state_t) 1,
+	pci_channel_io_normal = (__force pci_channel_state_t) 0,
 
 	/* I/O to channel is blocked */
-	pci_channel_io_frozen = (__force pci_channel_state_t) 2,
+	pci_channel_io_frozen = (__force pci_channel_state_t) 1,
 
 	/* PCI card is dead */
-	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
+	pci_channel_io_perm_failure = (__force pci_channel_state_t) 2,
 };
 
 typedef unsigned short __bitwise pci_bus_flags_t;
@@ -181,6 +181,11 @@ struct pci_dev {
 #define	to_pci_dev(n) container_of(n, struct pci_dev, dev)
 #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
 
+static inline int pci_channel_offline(struct pci_dev *pdev)
+{
+	return (pdev->error_state != pci_channel_io_normal);
+}
+
 static inline struct pci_cap_saved_state *pci_find_saved_cap(
 	struct pci_dev *pci_dev,char cap)
 {

[PATCH 2/2]: Use newly defined PCI channel offline routine

[Linas Vepstas]

Use newly minted routine to access the PCI channel state.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 drivers/net/e1000/e1000_main.c |    2 +-
 drivers/net/ixgb/ixgb_main.c   |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6.19-git7/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.19-git7.orig/drivers/net/e1000/e1000_main.c	2006-12-05 17:11:01.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/e1000/e1000_main.c	2006-12-05 17:13:31.000000000 -0600
@@ -3449,7 +3449,7 @@ e1000_update_stats(struct e1000_adapter 
 	 */
 	if (adapter->link_speed == 0)
 		return;
-	if (pdev->error_state && pdev->error_state != pci_channel_io_normal)
+	if (pci_channel_offline(pdev))
 		return;
 
 	spin_lock_irqsave(&adapter->stats_lock, flags);
Index: linux-2.6.19-git7/drivers/net/ixgb/ixgb_main.c
===================================================================
--- linux-2.6.19-git7.orig/drivers/net/ixgb/ixgb_main.c	2006-12-05 17:11:01.000000000 -0600
+++ linux-2.6.19-git7/drivers/net/ixgb/ixgb_main.c	2006-12-05 17:13:31.000000000 -0600
@@ -1564,7 +1564,7 @@ ixgb_update_stats(struct ixgb_adapter *a
 	struct pci_dev *pdev = adapter->pdev;
 
 	/* Prevent stats update while adapter is being reset */
-	if (pdev->error_state && pdev->error_state != pci_channel_io_normal)
+	if (pci_channel_offline(pdev))
 		return;
 
 	if((netdev->flags & IFF_PROMISC) || (netdev->flags & IFF_ALLMULTI) ||

Re: [PATCH 1/2]: Renumber PCI error enums to start at zero

[Greg KH]

On Tue, Dec 12, 2006 at 01:55:24PM -0600, Linas Vepstas wrote:
> 
> Greg, Andrew,
> 
> This patch fixes an annoying numbering mistake. 
> Please apply this (and the next patch).
> 
> --linas
> 
> Subject: [PATCH 1/2]: Renumber PCI error enums to start at zero
> 
> Renumber the PCI error enums to start at zero for "normal/online".
> This allows un-initialized pci channel state (which defaults to zero)
> to be interpreted as "normal".  Add very simple routine to check
> state, just in case this ever has to be fiddled with again.

No, as you have a specific type for this state, never test it against
"zero".  That just defeats the whole issue of having a special type for
this state.

So you should not have to change the values of the enumerated type for
the code to work properly.  In fact, I would argue that we should not
change it for this very reason :)

So, care to respin these without the enum changes to provide the new
function and use it?

thanks,

greg k-h

Re: [PATCH 2/2]: Use newly defined PCI channel offline routine

[Auke Kok]

Linas Vepstas wrote:
> Use newly minted routine to access the PCI channel state.
> 
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

ACK, thanks Linas.

If it doesn't get picked up I can stack it on my queue for netdev later.

Auke


> 
> ----
>  drivers/net/e1000/e1000_main.c |    2 +-
>  drivers/net/ixgb/ixgb_main.c   |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.19-git7/drivers/net/e1000/e1000_main.c
> ===================================================================
> --- linux-2.6.19-git7.orig/drivers/net/e1000/e1000_main.c	2006-12-05 17:11:01.000000000 -0600
> +++ linux-2.6.19-git7/drivers/net/e1000/e1000_main.c	2006-12-05 17:13:31.000000000 -0600
> @@ -3449,7 +3449,7 @@ e1000_update_stats(struct e1000_adapter 
>  	 */
>  	if (adapter->link_speed == 0)
>  		return;
> -	if (pdev->error_state && pdev->error_state != pci_channel_io_normal)
> +	if (pci_channel_offline(pdev))
>  		return;
>  
>  	spin_lock_irqsave(&adapter->stats_lock, flags);
> Index: linux-2.6.19-git7/drivers/net/ixgb/ixgb_main.c
> ===================================================================
> --- linux-2.6.19-git7.orig/drivers/net/ixgb/ixgb_main.c	2006-12-05 17:11:01.000000000 -0600
> +++ linux-2.6.19-git7/drivers/net/ixgb/ixgb_main.c	2006-12-05 17:13:31.000000000 -0600
> @@ -1564,7 +1564,7 @@ ixgb_update_stats(struct ixgb_adapter *a
>  	struct pci_dev *pdev = adapter->pdev;
>  
>  	/* Prevent stats update while adapter is being reset */
> -	if (pdev->error_state && pdev->error_state != pci_channel_io_normal)
> +	if (pci_channel_offline(pdev))
>  		return;
>  
>  	if((netdev->flags & IFF_PROMISC) || (netdev->flags & IFF_ALLMULTI) ||

Re: [PATCH 1/2]: Renumber PCI error enums to start at zero

[Linas Vepstas]

On Tue, Dec 12, 2006 at 12:35:43PM -0800, Greg KH wrote:
> On Tue, Dec 12, 2006 at 01:55:24PM -0600, Linas Vepstas wrote:
> > 
> > Subject: [PATCH 1/2]: Renumber PCI error enums to start at zero
> > 
> > Renumber the PCI error enums to start at zero for "normal/online".
> > This allows un-initialized pci channel state (which defaults to zero)
> > to be interpreted as "normal".  Add very simple routine to check
> > state, just in case this ever has to be fiddled with again.
> 
> No, as you have a specific type for this state, never test it against
> "zero".  That just defeats the whole issue of having a special type for
> this state.

Yes, well, I guess that was my initial thinking, which is why it got
coded that way. But "in real life", the value in the struct isn't
initialized (thus taking a value of zero). Its not initialized 
in deference to the traditional idea that "just saying bzero() 
should be enough".  

However, that turned the test for error into a dorky double test:
if(pdev->error_state && pdev->error_state != pci_channel_io_normal)
which struck me as lame. 

So, I'll ask: is it better to test for (state!=0 && state!=1) or,
to initialize pdev->error_state = pci_channel_io_normal; in the driver 
probe code?

--linas

Re: [PATCH 1/2]: Renumber PCI error enums to start at zero

[Greg KH]

On Tue, Dec 12, 2006 at 03:35:42PM -0600, Linas Vepstas wrote:
> On Tue, Dec 12, 2006 at 12:35:43PM -0800, Greg KH wrote:
> > On Tue, Dec 12, 2006 at 01:55:24PM -0600, Linas Vepstas wrote:
> > > 
> > > Subject: [PATCH 1/2]: Renumber PCI error enums to start at zero
> > > 
> > > Renumber the PCI error enums to start at zero for "normal/online".
> > > This allows un-initialized pci channel state (which defaults to zero)
> > > to be interpreted as "normal".  Add very simple routine to check
> > > state, just in case this ever has to be fiddled with again.
> > 
> > No, as you have a specific type for this state, never test it against
> > "zero".  That just defeats the whole issue of having a special type for
> > this state.
> 
> Yes, well, I guess that was my initial thinking, which is why it got
> coded that way. But "in real life", the value in the struct isn't
> initialized (thus taking a value of zero). Its not initialized 
> in deference to the traditional idea that "just saying bzero() 
> should be enough".  

Then properly initialize the thing.

> However, that turned the test for error into a dorky double test:
> if(pdev->error_state && pdev->error_state != pci_channel_io_normal)
> which struck me as lame. 

Again, don't test an explicit enumerated type against "0", that's just
foolish.  Why have the explicit type if you are going to do that?  Does
sparse complain about it?  It should if it doesn't...

> So, I'll ask: is it better to test for (state!=0 && state!=1) or,
> to initialize pdev->error_state = pci_channel_io_normal; in the driver 
> probe code?

Initialize the state in the probe.  Then check against the enumerated
type, not an integer value.  Sparse should complain if you try to do
that.

Or, if you really want to test against integers, rip out those types,
otherwise they are useless as you keep trying to go around them...

thanks,

greg k-h