[ofa-general] Re: [PATCH 1/2] libibumad: fix partition support

[Hal Rosenstock]

On Fri, 2007-06-22 at 11:55, Sean Hefty wrote:
> > Just two things:
> > 1. It might be better if the ABI version 5 warning message for only
> > pkey_index 0 being supported comes out at umad_init time rather than
> > umad_set_pkey time so that the user is not swamped with these.
> 
> Placing the warning in umad_init would display it even if the app only 
> used pkey_index 0, so keeping it in umad_set_pkey seems better to me. 
> We could make it so that the warning message only displays once though.

Sure. That would be better IMO too.

> > 2. There is one pathological combination. It would be using 2.6.23 (with
> > the new user_mad ABI version 6), an updated libibumad would be required,
> > but an older libvendor (osm_vendor_ibumad.c without your one line
> > change). That might be the case with someone who swapped back and forth
> > between OFED 1.2 and master in some scenarios.
> 
> I don't know how we can support all combinations, especially since the 
> return codes aren't being checked.  We can make a special case when 
> umad_set_pkey() is called with 0xffff on ABI 6, and display a warning 
> message and/or convert it to the correct index.

Yes, but this would eliminate the case where some implementation
supported the max pkeys. That's purely theoretical and no one is even
close to that max yet.

> > Also, this does not quite work as expected. An error was returned based
> > on the bad pkey index but I do see a send on the IB link (with a bad
> > pkey). I wouldn't have expected the latter part. Maybe this is a driver
> > or firmware issue. Not sure yet. I suppose there should be some
> > pkey_index validation (to make sure it is within the device's valid
> > range) and that should also ultimately get added to libibumad or should
> > such validation go into the user_mad kernel module ?
> 
> I think if we want to validate that the pkey_index is reasonable, the 
> check should go in the kernel.

Yes, that was my thinking too.

-- Hal

> - Sean