[ofa-general] Issues with osm_lin_fwd_tbl.h:osm_lin_fwd_tbl_get_lids_per_block()

[Hal Rosenstock]

Hi Vincent,

On Wed, 2008-07-09 at 15:37 +0200, Vincent Ficet wrote:
> Hello,
> 
> The function osm_lin_fwd_tbl.h:osm_lin_fwd_tbl_get_lids_per_block() is 
> coded as follows:
> 
> static inline uint16_t
> osm_lin_fwd_tbl_get_lids_per_block(IN const osm_lin_fwd_tbl_t * const p_tbl)
> {
>     UNUSED_PARAM(p_tbl);
>     return (64);
> }
> 
> Given that the block used comes from the 'data' (payload) field of the 
> ib_smp_t structure (defined in opensm/include/iba/ib_types.h) which is:
> 
> uint8_t data[IB_SMP_DATA_SIZE];
> 
> where IB_SMP_DATA_SIZE is defined as:
> 
> #define IB_SMP_DATA_SIZE 64
> 
> Shouldn't osm_lin_fwd_tbl_get_lids_per_block() be implemented as follows:
> 
> {
>     UNUSED_PARAM(p_tbl);
>     return IB_SMP_DATA_SIZE;
> }

It is bound by the SMP MAD data size but is actually defined in the LFT
SM attribute.

> Also, a LID beeing 16 bit wide, the function name is rather misleading 
> if not wrong:
> In actual fact, it should rather be called 
> osm_lin_fwd_tbl_get_ports_per_block(), since output ports are encoded as 
> uint8_t in osm_lin_fwd_tbl.h:
> 
> typedef struct osm_lin_fwdbl {
>     uint16_t size;
>     uint8_t port_tbl[1];
> } osm_lin_fwd_tbl_t;
> 
> I think the confusion here is the same as a previous issue I raised in 
> commit a4934944004586759b3519afa71db5fddebb6541
> Do you agree ? If so, I can post a patch if you wish.

LIDs are the input to the table lookup and port number is the output.
I'm not sure which is better/less confusing to use in the API name.

> Finally, what is the point in keeping the unused parameter p_tbl ?

Yes, passing that parameter doesn't appear to be needed and could be
eliminated. In fact, that whole routine could be eliminated and replaced
by a define IMO and then have that ripple back into/simplify any
routines which use it.

Just my $0.02 on this.

-- Hal

> Thanks for your help,
> 
> Vincent
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general