search

openbmc / libpldm

Apache License 2.0
9 stars 12 forks source link

Base.h transfer_resp_flag enum values are incorrect #11

Closed jmc5113 closed 1 year ago

jmc5113 commented 1 year ago

enum transfer_resp_flag { PLDM_START = 0x01, PLDM_MIDDLE = 0x02, PLDM_END = 0x04, PLDM_START_AND_END = 0x05, };

These are defined in the PLDM PMC spec as

start = 0 middle = 1 end = 4 start_and_end = 5

amboar commented 1 year ago

My apologies, it seems we struggle a bit with keeping track of issues filed here. Let me track down the spec you suggest and push a patch.

amboar commented 1 year ago

@jmc5113 Can you please clarify your reference? Ideally that clarification would be the specification ID and version (e.g. DSP0248 v1.0.0), and the section number and name or table number in the case of a breakdown of a specific message type.

I ask that for several reasons:

  1. I interpret your "PLDM PMC" statement to mean DSP0248, Platform Monitoring and Control Specification
  2. It shouldn't be the case that we have DSP0248 enums defined in include/libpldm/base.h, as that file is specific to structures and constants defined in DSP0240, Platform Level Data Model Base Specification.

As it turns out, both DSP0240 and DSP0248 define a TransferFlag field at various points:

  1. DSP0240 v1.1.0, Table 10 - GetPLDMVersion request and response message format:

    TransferFlag This field is the transfer flag that indicates what part of the transfer this response represents. Possible values: {Start=1, Middle=2, End=4, StartAndEnd = 5}

  2. DSP0248 v1.2.0, Table 16 - PollForPlatformEventMessage command format

    TransferFlag The transfer flag that indicates what part of the transfer this response represents. Possible values: {Start=0x00, Middle=0x01, End=0x04, StartAndEnd=0x05} This field shall be omitted if eventID is 0x0000 or 0xFFFF.

Given the transfer_resp_flag is defined in include/libpldm/base.h it should be interpreted as referring to the DSP0240 definition for TransferFlag, in which case it is not incorrect.

That said, it's kinda awkward that PMCI defined multiple values for conceptually the same problem.

jmc5113 commented 1 year ago

Oh wow, thank you for the explanation. For my current project, I picked up some PLDM work and have been focused on DSP0248. In particular the polled platform events. (They're strangely spec'd) and this was one issue I had run into after working with the code. I wasn't aware that there was a second similar but different enum values.

Thank you for the breakdown and explaination.