Other than that one nit, the other patches in this series look good to me. See
below for comments on this one.
> +/*
> + * PMA class portinfo capability mask bits
> + */
> +#define IB_PMA_CLASS_CAP_ALLPORTSELECT cpu_to_be16(1 << 8)
> +#define IB_PMA_CLASS_CAP_EXT_WIDTH cpu_to_be16(1 << 9)
> +#define IB_PMA_CLASS_CAP_XMIT_WAIT cpu_to_be16(1 << 12)
> +
> +#define IB_PMA_CLASS_PORT_INFO cpu_to_be16(0x0001)
Hmm... we're defining attributes in different ways (see ib_sa.h and ib_smi.h).
Someday we may want to make things consistent.
> +#define IB_PMA_PORT_SAMPLES_CONTROL cpu_to_be16(0x0010)
> +#define IB_PMA_PORT_SAMPLES_RESULT cpu_to_be16(0x0011)
> +#define IB_PMA_PORT_COUNTERS cpu_to_be16(0x0012)
> +#define IB_PMA_PORT_COUNTERS_EXT cpu_to_be16(0x001D)
> +#define IB_PMA_PORT_SAMPLES_RESULT_EXT cpu_to_be16(0x001E)
> +#define IB_PMA_PORT_COUNTERS_CONG cpu_to_be16(0xFF00)
> +
> +struct ib_perf {
> + u8 base_version;
> + u8 mgmt_class;
> + u8 class_version;
> + u8 method;
> + __be16 status;
> + __be16 unused;
> + __be64 tid;
> + __be16 attr_id;
> + __be16 resv;
> + __be32 attr_mod;
> + u8 reserved[40];
> + u8 data[192];
> +} __attribute__ ((packed));
This should use struct ib_mad_hdr, possibly adding appropriate values to
ib_mad.h for the 40 and 192. See the enum in ib_mad.h with IB_MGMT_* values.
I would also rename this to struct ib_pm_mad to better match the existing mad
definitions.
> +
> +struct ib_pma_classportinfo {
> + u8 base_version;
> + u8 class_version;
> + __be16 cap_mask;
> + u8 reserved[3];
> + u8 resp_time_value; /* only lower 5 bits */
> + union ib_gid redirect_gid;
> + __be32 redirect_tc_sl_fl; /* 8, 4, 20 bits respectively */
> + __be16 redirect_lid;
> + __be16 redirect_pkey;
> + __be32 redirect_qp; /* only lower 24 bits */
> + __be32 redirect_qkey;
> + union ib_gid trap_gid;
> + __be32 trap_tc_sl_fl; /* 8, 4, 20 bits respectively */
> + __be16 trap_lid;
> + __be16 trap_pkey;
> + __be32 trap_hl_qp; /* 8, 24 bits respectively */
> + __be32 trap_qkey;
> +} __attribute__ ((packed));
This needs to be replaced with struct ib_class_port_info in ib_mad.h.
> +
> +struct ib_pma_portsamplescontrol {
> + u8 opcode;
> + u8 port_select;
> + u8 tick;
> + u8 counter_width; /* only lower 3 bits */
> + __be32 counter_mask0_9; /* 2, 10 * 3, bits */
> + __be16 counter_mask10_14; /* 1, 5 * 3, bits */
Comments for the bit fields aren't very clear, maybe say something more like
'10 3-bit fields'. For things like counter_width, my preference is something
like /* resv: 7:3, counter width: 2:0 */ to clarify which bits are used and
indicate that the others are currently reserved.
> + u8 sample_mechanisms;
> + u8 sample_status; /* only lower 2 bits */
> + __be64 option_mask;
> + __be64 vendor_mask;
> + __be32 sample_start;
> + __be32 sample_interval;
> + __be16 tag;
> + __be16 counter_select[15];
> +} __attribute__ ((packed));
There are fields missing at the end: reserved 32-bits, SamplesOnlyOptionMask
64-bit, reserved 896-bits. This should make the structure naturally aligned,
which could remove the packed attribute.
> +struct ib_pma_portsamplesresult {
> + __be16 tag;
> + __be16 sample_status; /* only lower 2 bits */
> + __be32 counter[15];
> +} __attribute__ ((packed));
this is naturally aligned
> +struct ib_pma_portsamplesresult_ext {
> + __be16 tag;
> + __be16 sample_status; /* only lower 2 bits */
> + __be32 extended_width; /* only upper 2 bits */
> + __be64 counter[15];
> +} __attribute__ ((packed));
this is naturally aligned
> +struct ib_pma_portcounters {
> + u8 reserved;
> + u8 port_select;
> + __be16 counter_select;
> + __be16 symbol_error_counter;
> + u8 link_error_recovery_counter;
> + u8 link_downed_counter;
> + __be16 port_rcv_errors;
> + __be16 port_rcv_remphys_errors;
> + __be16 port_rcv_switch_relay_errors;
> + __be16 port_xmit_discards;
> + u8 port_xmit_constraint_errors;
> + u8 port_rcv_constraint_errors;
> + u8 reserved1;
> + u8 lli_ebor_errors; /* 4, 4, bits */
lli_ebor? what about link_overrun_errors instead, or at least something a
little more descriptive?
Enhancing the comment can also help: /* LocalLink: 7:4, BufferOverrun: 3:0 */
> + __be16 reserved2;
> + __be16 vl15_dropped;
> + __be32 port_xmit_data;
> + __be32 port_rcv_data;
> + __be32 port_xmit_packets;
> + __be32 port_rcv_packets;
> +} __attribute__ ((packed));
missing PortXmitWait 32-bits at end
> +
> +struct ib_pma_portcounters_cong {
> + u8 reserved;
> + u8 reserved1;
> + __be16 port_check_rate;
> + __be16 symbol_error_counter;
> + u8 link_error_recovery_counter;
> + u8 link_downed_counter;
> + __be16 port_rcv_errors;
> + __be16 port_rcv_remphys_errors;
> + __be16 port_rcv_switch_relay_errors;
> + __be16 port_xmit_discards;
> + u8 port_xmit_constraint_errors;
> + u8 port_rcv_constraint_errors;
> + u8 reserved2;
> + u8 lli_ebor_errors; /* 4, 4, bits */
> + __be16 reserved3;
> + __be16 vl15_dropped;
> + __be64 port_xmit_data;
> + __be64 port_rcv_data;
> + __be64 port_xmit_packets;
> + __be64 port_rcv_packets;
> + __be64 port_xmit_wait;
> + __be64 port_adr_events;
> +} __attribute__ ((packed));
> +
> +#define IB_PMA_CONG_HW_CONTROL_TIMER 0x00
> +#define IB_PMA_CONG_HW_CONTROL_SAMPLE 0x01
>
> +#define IB_PMA_SEL_SYMBOL_ERROR cpu_to_be16(0x0001)
> +#define IB_PMA_SEL_LINK_ERROR_RECOVERY cpu_to_be16(0x0002)
> +#define IB_PMA_SEL_LINK_DOWNED cpu_to_be16(0x0004)
> +#define IB_PMA_SEL_PORT_RCV_ERRORS cpu_to_be16(0x0008)
> +#define IB_PMA_SEL_PORT_RCV_REMPHYS_ERRORS cpu_to_be16(0x0010)
> +#define IB_PMA_SEL_PORT_XMIT_DISCARDS cpu_to_be16(0x0040)
> +#define IB_PMA_SEL_LOCAL_LINK_INTEGRITY_ERRORS cpu_to_be16(0x0200)
> +#define IB_PMA_SEL_EXCESSIVE_BUFFER_OVERRUNS cpu_to_be16(0x0400)
> +#define IB_PMA_SEL_PORT_VL15_DROPPED cpu_to_be16(0x0800)
> +#define IB_PMA_SEL_PORT_XMIT_DATA cpu_to_be16(0x1000)
> +#define IB_PMA_SEL_PORT_RCV_DATA cpu_to_be16(0x2000)
> +#define IB_PMA_SEL_PORT_XMIT_PACKETS cpu_to_be16(0x4000)
> +#define IB_PMA_SEL_PORT_RCV_PACKETS cpu_to_be16(0x8000)
> +
> +#define IB_PMA_SEL_CONG_ALL 0x01
> +#define IB_PMA_SEL_CONG_PORT_DATA 0x02
> +#define IB_PMA_SEL_CONG_XMIT 0x04
> +#define IB_PMA_SEL_CONG_ROUTING 0x08
> +
> +struct ib_pma_portcounters_ext {
> + u8 reserved;
> + u8 port_select;
> + __be16 counter_select;
> + __be32 reserved1;
> + __be64 port_xmit_data;
> + __be64 port_rcv_data;
> + __be64 port_xmit_packets;
> + __be64 port_rcv_packets;
> + __be64 port_unicast_xmit_packets;
> + __be64 port_unicast_rcv_packets;
> + __be64 port_multicast_xmit_packets;
> + __be64 port_multicast_rcv_packets;
> +} __attribute__ ((packed));
> +
> +#define IB_PMA_SELX_PORT_XMIT_DATA cpu_to_be16(0x0001)
> +#define IB_PMA_SELX_PORT_RCV_DATA cpu_to_be16(0x0002)
> +#define IB_PMA_SELX_PORT_XMIT_PACKETS cpu_to_be16(0x0004)
> +#define IB_PMA_SELX_PORT_RCV_PACKETS cpu_to_be16(0x0008)
> +#define IB_PMA_SELX_PORT_UNI_XMIT_PACKETS cpu_to_be16(0x0010)
> +#define IB_PMA_SELX_PORT_UNI_RCV_PACKETS cpu_to_be16(0x0020)
> +#define IB_PMA_SELX_PORT_MULTI_XMIT_PACKETS cpu_to_be16(0x0040)
> +#define IB_PMA_SELX_PORT_MULTI_RCV_PACKETS cpu_to_be16(0x0080)
Rather than having IB_PMA_SEL_* and IB_PMA_SELX_*, we could follow what's done
in ib_sa.h:
IB_PM_<structure>_<field>
- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
