diff mbox

[3/8] pcie, aer: rework MASK macros in aerdrv_errprint.c

Message ID 4A8E188B.7000601@jp.fujitsu.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Hidetoshi Seto Aug. 21, 2009, 3:46 a.m. UTC 
Definitions of MASK macros in aerdrv_errprint.c are tricky and unsafe.

For example, AER_AGENT_TRANSMITTER_MASK(_sev, _stat) does work like:
  static inline func(int _sev, int _stat)
  {
    if (_sev == AER_CORRECTABLE)
      return (_stat & (PCI_ERR_COR_REP_ROLL|PCI_ERR_COR_REP_TIMER));
    else
      return (_stat & PCI_ERR_COR_REP_ROLL);
  }
In case of else path here, for uncorrectable errors, testing bits in
_stat by PCI_ERR_COR_* does not make sense because _stat should have only
PCI_ERR_UNC_* bits originated in uncorrectable error status register.
But at this time this is safe because uncorrectable error using bit
position same to PCI_ERR_COR_REP_ROLL(= bit position 8) is not defined.
Likewise, AER_AGENT_COMPLETER_MASK is always PCI_ERR_UNC_COMP_ABORT but
it works because bit 15 of correctable error status is not defined.

It means that these MASK macros will turn to be wrong once if new error
is defined. (In fact, bit 15 of correctable is now defined in PCIe 2.1)

This patch changes these MASK macros to be more strict, not to return
PCI_ERR_COR_* bits for uncorrectable error status and vise versa.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 drivers/pci/pcie/aer/aerdrv_errprint.c |   46 ++++++++++++++-----------------
 1 files changed, 21 insertions(+), 25 deletions(-)

Comments

Andrew Patterson Aug. 26, 2009, 10:37 p.m. UTC | #1 
On Fri, 2009-08-21 at 12:46 +0900, Hidetoshi Seto wrote:
> Definitions of MASK macros in aerdrv_errprint.c are tricky and unsafe.
> 
> For example, AER_AGENT_TRANSMITTER_MASK(_sev, _stat) does work like:
>   static inline func(int _sev, int _stat)
>   {
>     if (_sev == AER_CORRECTABLE)
>       return (_stat & (PCI_ERR_COR_REP_ROLL|PCI_ERR_COR_REP_TIMER));
>     else
>       return (_stat & PCI_ERR_COR_REP_ROLL);
>   }
> In case of else path here, for uncorrectable errors, testing bits in
> _stat by PCI_ERR_COR_* does not make sense because _stat should have only
> PCI_ERR_UNC_* bits originated in uncorrectable error status register.
> But at this time this is safe because uncorrectable error using bit
> position same to PCI_ERR_COR_REP_ROLL(= bit position 8) is not defined.
> Likewise, AER_AGENT_COMPLETER_MASK is always PCI_ERR_UNC_COMP_ABORT but
> it works because bit 15 of correctable error status is not defined.
> 
> It means that these MASK macros will turn to be wrong once if new error
> is defined. (In fact, bit 15 of correctable is now defined in PCIe 2.1)
> 
> This patch changes these MASK macros to be more strict, not to return
> PCI_ERR_COR_* bits for uncorrectable error status and vise versa.
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
>  drivers/pci/pcie/aer/aerdrv_errprint.c |   46 ++++++++++++++-----------------
>  1 files changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
> index 7fb5a2c..48f70fa 100644
> --- a/drivers/pci/pcie/aer/aerdrv_errprint.c
> +++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
> @@ -27,39 +27,35 @@
>  #define AER_AGENT_COMPLETER		2
>  #define AER_AGENT_TRANSMITTER		3
>  
> -#define AER_AGENT_REQUESTER_MASK	(PCI_ERR_UNC_COMP_TIME|	\
> -					PCI_ERR_UNC_UNSUP)
> -
> -#define AER_AGENT_COMPLETER_MASK	PCI_ERR_UNC_COMP_ABORT
> -
> -#define AER_AGENT_TRANSMITTER_MASK(t, e) (e & (PCI_ERR_COR_REP_ROLL| \
> -	((t == AER_CORRECTABLE) ? PCI_ERR_COR_REP_TIMER : 0)))
> +#define AER_AGENT_REQUESTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
> +	0 : (PCI_ERR_UNC_COMP_TIME|PCI_ERR_UNC_UNSUP))
> +#define AER_AGENT_COMPLETER_MASK(t)	((t == AER_CORRECTABLE) ?	\
> +	0 : PCI_ERR_UNC_COMP_ABORT)
> +#define AER_AGENT_TRANSMITTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
> +	(PCI_ERR_COR_REP_ROLL|PCI_ERR_COR_REP_TIMER) : 0)
>  
>  #define AER_GET_AGENT(t, e)						\
> -	((e & AER_AGENT_COMPLETER_MASK) ? AER_AGENT_COMPLETER :		\
> -	(e & AER_AGENT_REQUESTER_MASK) ? AER_AGENT_REQUESTER :		\
> -	(AER_AGENT_TRANSMITTER_MASK(t, e)) ? AER_AGENT_TRANSMITTER :	\
> +	((e & AER_AGENT_COMPLETER_MASK(t)) ? AER_AGENT_COMPLETER :	\
> +	(e & AER_AGENT_REQUESTER_MASK(t)) ? AER_AGENT_REQUESTER :	\
> +	(e & AER_AGENT_TRANSMITTER_MASK(t)) ? AER_AGENT_TRANSMITTER :	\
>  	AER_AGENT_RECEIVER)
>  
> -#define AER_PHYSICAL_LAYER_ERROR_MASK	PCI_ERR_COR_RCVR
> -#define AER_DATA_LINK_LAYER_ERROR_MASK(t, e)	\
> -		(PCI_ERR_UNC_DLP|		\
> -		PCI_ERR_COR_BAD_TLP|		\
> -		PCI_ERR_COR_BAD_DLLP|		\
> -		PCI_ERR_COR_REP_ROLL|		\
> -		((t == AER_CORRECTABLE) ?	\
> -		PCI_ERR_COR_REP_TIMER : 0))
> -
>  #define AER_PHYSICAL_LAYER_ERROR	0
>  #define AER_DATA_LINK_LAYER_ERROR	1
>  #define AER_TRANSACTION_LAYER_ERROR	2
>  
> -#define AER_GET_LAYER_ERROR(t, e)				\
> -	((e & AER_PHYSICAL_LAYER_ERROR_MASK) ?			\
> -	AER_PHYSICAL_LAYER_ERROR :				\
> -	(e & AER_DATA_LINK_LAYER_ERROR_MASK(t, e)) ?		\
> -		AER_DATA_LINK_LAYER_ERROR :			\
> -		AER_TRANSACTION_LAYER_ERROR)
> +#define AER_PHYSICAL_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
> +	PCI_ERR_COR_RCVR : 0)
> +#define AER_DATA_LINK_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
> +	(PCI_ERR_COR_BAD_TLP|						\
> +	PCI_ERR_COR_BAD_DLLP|						\
> +	PCI_ERR_COR_REP_ROLL|						\
> +	PCI_ERR_COR_REP_TIMER) : PCI_ERR_UNC_DLP)
> +
> +#define AER_GET_LAYER_ERROR(t, e)					\
> +	((e & AER_PHYSICAL_LAYER_ERROR_MASK(t)) ? AER_PHYSICAL_LAYER_ERROR : \
> +	(e & AER_DATA_LINK_LAYER_ERROR_MASK(t)) ? AER_DATA_LINK_LAYER_ERROR : \
> +	AER_TRANSACTION_LAYER_ERROR)
>  
>  #define AER_PR(info, fmt, args...)				\
>  	printk("%s" fmt, (info->severity == AER_CORRECTABLE) ?	\
> -- 
> 1.6.4
> 

Reviewed-by: Andrew Patterson <andrew.patterson@hp.com>

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
diff mbox

Patch

diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index 7fb5a2c..48f70fa 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -27,39 +27,35 @@ 
 #define AER_AGENT_COMPLETER		2
 #define AER_AGENT_TRANSMITTER		3
 
-#define AER_AGENT_REQUESTER_MASK	(PCI_ERR_UNC_COMP_TIME|	\
-					PCI_ERR_UNC_UNSUP)
-
-#define AER_AGENT_COMPLETER_MASK	PCI_ERR_UNC_COMP_ABORT
-
-#define AER_AGENT_TRANSMITTER_MASK(t, e) (e & (PCI_ERR_COR_REP_ROLL| \
-	((t == AER_CORRECTABLE) ? PCI_ERR_COR_REP_TIMER : 0)))
+#define AER_AGENT_REQUESTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+	0 : (PCI_ERR_UNC_COMP_TIME|PCI_ERR_UNC_UNSUP))
+#define AER_AGENT_COMPLETER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+	0 : PCI_ERR_UNC_COMP_ABORT)
+#define AER_AGENT_TRANSMITTER_MASK(t)	((t == AER_CORRECTABLE) ?	\
+	(PCI_ERR_COR_REP_ROLL|PCI_ERR_COR_REP_TIMER) : 0)
 
 #define AER_GET_AGENT(t, e)						\
-	((e & AER_AGENT_COMPLETER_MASK) ? AER_AGENT_COMPLETER :		\
-	(e & AER_AGENT_REQUESTER_MASK) ? AER_AGENT_REQUESTER :		\
-	(AER_AGENT_TRANSMITTER_MASK(t, e)) ? AER_AGENT_TRANSMITTER :	\
+	((e & AER_AGENT_COMPLETER_MASK(t)) ? AER_AGENT_COMPLETER :	\
+	(e & AER_AGENT_REQUESTER_MASK(t)) ? AER_AGENT_REQUESTER :	\
+	(e & AER_AGENT_TRANSMITTER_MASK(t)) ? AER_AGENT_TRANSMITTER :	\
 	AER_AGENT_RECEIVER)
 
-#define AER_PHYSICAL_LAYER_ERROR_MASK	PCI_ERR_COR_RCVR
-#define AER_DATA_LINK_LAYER_ERROR_MASK(t, e)	\
-		(PCI_ERR_UNC_DLP|		\
-		PCI_ERR_COR_BAD_TLP|		\
-		PCI_ERR_COR_BAD_DLLP|		\
-		PCI_ERR_COR_REP_ROLL|		\
-		((t == AER_CORRECTABLE) ?	\
-		PCI_ERR_COR_REP_TIMER : 0))
-
 #define AER_PHYSICAL_LAYER_ERROR	0
 #define AER_DATA_LINK_LAYER_ERROR	1
 #define AER_TRANSACTION_LAYER_ERROR	2
 
-#define AER_GET_LAYER_ERROR(t, e)				\
-	((e & AER_PHYSICAL_LAYER_ERROR_MASK) ?			\
-	AER_PHYSICAL_LAYER_ERROR :				\
-	(e & AER_DATA_LINK_LAYER_ERROR_MASK(t, e)) ?		\
-		AER_DATA_LINK_LAYER_ERROR :			\
-		AER_TRANSACTION_LAYER_ERROR)
+#define AER_PHYSICAL_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
+	PCI_ERR_COR_RCVR : 0)
+#define AER_DATA_LINK_LAYER_ERROR_MASK(t) ((t == AER_CORRECTABLE) ?	\
+	(PCI_ERR_COR_BAD_TLP|						\
+	PCI_ERR_COR_BAD_DLLP|						\
+	PCI_ERR_COR_REP_ROLL|						\
+	PCI_ERR_COR_REP_TIMER) : PCI_ERR_UNC_DLP)
+
+#define AER_GET_LAYER_ERROR(t, e)					\
+	((e & AER_PHYSICAL_LAYER_ERROR_MASK(t)) ? AER_PHYSICAL_LAYER_ERROR : \
+	(e & AER_DATA_LINK_LAYER_ERROR_MASK(t)) ? AER_DATA_LINK_LAYER_ERROR : \
+	AER_TRANSACTION_LAYER_ERROR)
 
 #define AER_PR(info, fmt, args...)				\
 	printk("%s" fmt, (info->severity == AER_CORRECTABLE) ?	\