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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxx>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Andrew Patterson
Hewlett-Packard
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
