#1695 bad return from saHpiHotSwapStateGet in ipmidirect plugin

3.1.x
open
nobody
5
2015-06-10
2012-02-15
David McKinley
No

When a resource that is a FRU does not have a hotswap sensor, so that SAHPI_CAPABILITY_MANAGED_HOTSWAP is not set, then saHpiHotSwapStateGet should return SAHPI_HS_STATE_ACTIVE per HPI B.03.01+. The ipmidirect plugin returns SA_ERR_HPI_INVALID_PARAMS in this case.

See bug #2948127; it suggests that plugins may have this issue. Indeed, the ipmidirect plugin does.

Discussion

  • Anton Pak
    Anton Pak
    2012-04-20

    • milestone: 2040045 --> 3.1.x
     
  • Anton Pak
    Anton Pak
    2015-06-08

    The bug was always shadowed by the fact, that a hotswap sensor is mandatory for an ATCA FRU.

    If ipmidirect claims HPI-to-ATCA compliance, then it should never happen.

    Though it will happen :)

     
  • Anton Pak
    Anton Pak
    2015-06-08

    Also there is a case when sensor repository reading is in progress and we haven't read the hotswap sensor yet. Usually in such case the FRU state is INACTIVE or INS_PENDING.

     
  • dr_mohan
    dr_mohan
    2015-06-10

    Thanks for David for the following comments

    The bug is (or at least was, as of release 3.0, which is the latest source I have easy access to) in the hotswap.cpp module, in function cIpmi::IfGetHotswapState. There, around line 50, it says:

    SaErrorT
    cIpmi::IfGetHotswapState( cIpmiResource res, SaHpiHsStateT &state )
    {
    // get hotswap sensor
    cIpmiSensorHotswap
    hs = res->GetHotswapSensor();

    if ( !hs )
    return SA_ERR_HPI_INVALID_PARAMS;

    // get hotswap state
    return hs->GetHpiState( state );
    }

    In the case of a FRU with unmanaged Hotswap (SAHPI_CAPABILITY_FRU set, but SAHPI_CAPABILITY_MANAGED_HOTSWAP not set), the spec says that saHpiHotSwapStateGet() should be supported, and by implication, should return SAHPI_HS_STATE_ACTIVE when the resource is present.
    The absence of a "hot swap sensor" for the resource indicates just this condition, so I believe that the code should be more like this:

    SaErrorT
    cIpmi::IfGetHotswapState( cIpmiResource res, SaHpiHsStateT &state )
    {
    // get hotswap sensor
    cIpmiSensorHotswap
    hs = res->GetHotswapSensor();

    if ( !hs )
    {
    state = SAHPI_HS_STATE_ACTIVE;
    return SA_OK;
    }

    // get hotswap state
    return hs->GetHpiState( state );
    }