Hi Mahesh,
A few nitpicks.
Mahesh J Salgaonkar <mah...@linux.vnet.ibm.com> writes:
> From: Mahesh Salgaonkar <mah...@linux.vnet.ibm.com>
>
> On pseries, the machine check error details are part of RTAS extended
> event log passed under Machine check exception section. This patch adds
> the definition of rtas MCE event section and related helper
> functions.
>
> Signed-off-by: Mahesh Salgaonkar <mah...@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/rtas.h | 111
> +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
AFIACS none of this ever gets used outside of ras.c, should it should
just go in there.
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 71e393c46a49..adc677c5e3a4 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -326,6 +334,109 @@ struct pseries_hp_errorlog {
> #define PSERIES_HP_ELOG_ID_DRC_COUNT 3
> #define PSERIES_HP_ELOG_ID_DRC_IC 4
>
> +/* RTAS pseries MCE errorlog section */
> +#pragma pack(push, 1)
> +struct pseries_mc_errorlog {
> + __be32 fru_id;
> + __be32 proc_id;
> + uint8_t error_type;
Please use kernel types, so u8.
> + union {
> + struct {
> + uint8_t ue_err_type;
> + /* XXXXXXXX
> + * X 1: Permanent or Transient UE.
> + * X 1: Effective address provided.
> + * X 1: Logical address provided.
> + * XX 2: Reserved.
> + * XXX 3: Type of UE error.
> + */
But which bit is bit 0? And is that the LSB or MSB?
> + uint8_t reserved_1[6];
> + __be64 effective_address;
> + __be64 logical_address;
> + } ue_error;
> + struct {
> + uint8_t soft_err_type;
> + /* XXXXXXXX
> + * X 1: Effective address provided.
> + * XXXXX 5: Reserved.
> + * XX 2: Type of SLB/ERAT/TLB error.
> + */
> + uint8_t reserved_1[6];
> + __be64 effective_address;
> + uint8_t reserved_2[8];
> + } soft_error;
> + } u;
> +};
> +#pragma pack(pop)
Why not __packed ?
> +/* RTAS pseries MCE error types */
> +#define PSERIES_MC_ERROR_TYPE_UE 0x00
> +#define PSERIES_MC_ERROR_TYPE_SLB 0x01
> +#define PSERIES_MC_ERROR_TYPE_ERAT 0x02
> +#define PSERIES_MC_ERROR_TYPE_TLB 0x04
> +#define PSERIES_MC_ERROR_TYPE_D_CACHE 0x05
> +#define PSERIES_MC_ERROR_TYPE_I_CACHE 0x07
Once these are in ras.c they can have less unwieldy names, ie. the
PSERIES at least can be dropped.
> +/* RTAS pseries MCE error sub types */
> +#define PSERIES_MC_ERROR_UE_INDETERMINATE 0
> +#define PSERIES_MC_ERROR_UE_IFETCH 1
> +#define PSERIES_MC_ERROR_UE_PAGE_TABLE_WALK_IFETCH 2
> +#define PSERIES_MC_ERROR_UE_LOAD_STORE 3
> +#define PSERIES_MC_ERROR_UE_PAGE_TABLE_WALK_LOAD_STORE 4
> +
> +#define PSERIES_MC_ERROR_SLB_PARITY 0
> +#define PSERIES_MC_ERROR_SLB_MULTIHIT 1
> +#define PSERIES_MC_ERROR_SLB_INDETERMINATE 2
> +
> +#define PSERIES_MC_ERROR_ERAT_PARITY 1
> +#define PSERIES_MC_ERROR_ERAT_MULTIHIT 2
> +#define PSERIES_MC_ERROR_ERAT_INDETERMINATE 3
> +
> +#define PSERIES_MC_ERROR_TLB_PARITY 1
> +#define PSERIES_MC_ERROR_TLB_MULTIHIT 2
> +#define PSERIES_MC_ERROR_TLB_INDETERMINATE 3
> +
> +static inline uint8_t rtas_mc_error_type(const struct pseries_mc_errorlog
> *mlog)
> +{
> + return mlog->error_type;
> +}
Why not just access it directly?
> +static inline uint8_t rtas_mc_error_sub_type(
> + const struct pseries_mc_errorlog *mlog)
> +{
> + switch (mlog->error_type) {
> + case PSERIES_MC_ERROR_TYPE_UE:
> + return (mlog->u.ue_error.ue_err_type & 0x07);
> + case PSERIES_MC_ERROR_TYPE_SLB:
> + case PSERIES_MC_ERROR_TYPE_ERAT:
> + case PSERIES_MC_ERROR_TYPE_TLB:
> + return (mlog->u.soft_error.soft_err_type & 0x03);
> + default:
> + return 0;
> + }
> +}
> +
> +static inline uint64_t rtas_mc_get_effective_addr(
> + const struct pseries_mc_errorlog *mlog)
> +{
> + uint64_t addr = 0;
That should be __be64.
> +
> + switch (mlog->error_type) {
> + case PSERIES_MC_ERROR_TYPE_UE:
> + if (mlog->u.ue_error.ue_err_type & 0x40)
> + addr = mlog->u.ue_error.effective_address;
> + break;
> + case PSERIES_MC_ERROR_TYPE_SLB:
> + case PSERIES_MC_ERROR_TYPE_ERAT:
> + case PSERIES_MC_ERROR_TYPE_TLB:
> + if (mlog->u.soft_error.soft_err_type & 0x80)
> + addr = mlog->u.soft_error.effective_address;
> + default:
> + break;
> + }
> + return be64_to_cpu(addr);
> +}
> +
> struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
> uint16_t section_id);
>
cheers
