Minor issue. PM is defined in the spec glossary to be "Partition Manager". :-( Therefore I have refrained from using "PM" or "pm" in structs or defines. I would not mind settling on an abbreviation; "PMA" perhaps?
Ira On Wed, 15 Jun 2011 16:10:24 -0700 "Hefty, Sean" <[email protected]> wrote: > 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 -- Ira Weiny Math Programmer/Computer Scientist Lawrence Livermore National Lab 925-423-8008 [email protected] -- 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
