AD review: draft-ietf-adslmib-adsl2-05.txt

Thanks for the new revision, which addresses my earlier
comments. I ended my previeous review with:

   I need to study this more and better before I can complete
   my review.

Which I did and I took a very close look at many things.

Here are my remaining comments (I think there are enough
things below that a another new revision makes sense):


- W.r.t. 
    adsl2TCMIB MODULE-IDENTITY
       ...
         ::= { transmission xxx 2} -- adsl2MIB 2
    -- RFC Ed.: we suggest to put it under { transmission 230 } because
    --          this is the first available number.
  I suspect this may be confusing for IANA, so I suggest:
    adsl2TCMIB MODULE-IDENTITY
       ...
         ::= { transmission xxx 2 } -- adsl2MIB 2
    -- IANA, the xxx here must be the same as the one assigned
    --       to the adsl2MIB below.
    -- RFC Ed.: Pls fill in xxx once assigned by IANA.
  That also means you need to IMPORT mib-2 instead of transmission

- W.r.t. 
    adsl2MIB MODULE-IDENTITY
    ...
          ::= { transmission xxx }
    -- RFC Ed.: we suggest to put it under { transmission 230 } because
    --          this is the first available number.
  I would change that slightly into:
          ::= { transmission xxx } -- To be assigned by IANA
    -- IANA, we suggest to put it under { transmission 230 } because         
    --          this is the first available number.
    -- RFC Ed.: pls fill in xxx once IANA has made the assignment.

- I see:
   Adsl2ConfPmsForce ::= TEXTUAL-CONVENTION
      STATUS      current
      DESCRIPTION
         "Attributes with this syntax are configuration parameters
          that reference the desired power management state for the
          ADSL/ADSL2 or ADSL2+ link:
            l3toL0 (1)         - Perform a transition from L3 to L0
                                 (Full power management state)

            l0toL2 (2)         - Perform a transition from L0 to L2
                                 (Low power management state)
            l0orL2toL3 (3)     - Perform a transition into L3 (Idle
                                 power management state)"

      SYNTAX      INTEGER {
                     l3toL0 (0),
                     l0toL2 (2),
                     l0orL2toL3 (3)
                  }
   It seems the actual INTEGER values are out of sync with the DESCRIPTION.
   l3toL0 (0) should probably be: l3toL0 (1) !!??

- In the TC MIB module I see various INTEGER enumerations that start at 
  value zero. In general we prefer to start at 1, except if there is a good
  reason to use zero (like the fact that the label/value map directly to a
  zero value in some protocol or other specification. May I assume that 
  such is the case for all INTEGER enumerations that start at zero?

-    Adsl2ScMaskUs ::= TEXTUAL-CONVENTION
      STATUS      current
      DESCRIPTION

         "Each one of the 64 bits in this OCTET
         STRING array represents the corresponding bin
         in the downstream direction.  A value of one
         indicates that the bin is not in use."
      SYNTAX      OCTET STRING (SIZE(0..8))
   Most places where you use "Us" in the object descriptor, it is
   for UpStream. So is this ...MarkUs indeed for downstream?

In the adsl2MIB module:

-    adsl2LineCnfgTemplate  OBJECT-TYPE
      SYNTAX      SnmpAdminString (SIZE(1..32))
      MAX-ACCESS  read-write
      STATUS      current
      DESCRIPTION
         "The value of this object identifies the row in the ADSL2 Line
          Configuration Templates Table, (Adsl2ConfTemplatesTable),
          which applies for this ADSL2 line.
   s/(Adsl2ConfTemplatesTable)/(Adsl2ConfTemplateTable)/
   i.s Template is singular.

- adsl2LineStatusLastStateDs
  I do not understand the "counting" in
     DESCRIPTION
        "The last successful transmitted initialization state in
         the downstream direction in the last full initialization
         performed on the line.  States are per the specific ADSL type
         and are counted from 0 (if G.994.1 is used) or 1 (if G.994.1
         is not used) up to Showtime."
  I guess you mean that the values for this stats are numbered from 0
  up to showtime. I also wonder if it is wise to have it here as
  opposed to in the TEXTUAL CONVENTION, so you do not have to repeat
  it every time you use the TC as a SYNTAX. Besides, new values 
  may get added to the TEXTUAL-CONVENTION in the future, so you would
  then possibly also have to update thus text.

- Same for adsl2LineStatusLastStateUs
  there may be others?

- adsl2LineStatusAtur
      SYNTAX      Adsl2LineStatus
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
         "Indicates current state (existing failures) of the ATU-R.
          This is a bit-map of possible conditions.  The various bit
          positions are: noFailure(0), lossOfFraming(1),
          lossOfSignal(2), lossOfPower(3),
          initFailure(4) - never active on ATU-R"
  In the Adsl2LineSTatus, the value zero is labeled as noDefect instead
  of noFailure.
  Again, repeating the values here in the OBJECT seems redundant, plus
  is is prone to error/inconsistencies.

- same for adsl2LineStatusAtuc
  there may be others

- WHen I read this:
   adsl2LineStatusLnAttenDs  OBJECT-TYPE
      SYNTAX      Unsigned32
      UNITS       "0.1 dB"
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
         "The measured difference in the total power transmitted by the

          ATU-C and the total power received by the ATU-R over all sub-
          carriers during diagnostics mode and initialization.  It
          ranges from 0 to 1270 units of 0.1 dB (Physical values
          are 0 to 127 dB).  A value of all 1's indicates the line
          attenuation is out of range to be represented."
  Then I would have used a syntax as follows:
      SYNTAX      Unsigned32 (0..1270 | 4294967295)
  And U would use (in DESCRIPTION clasue):
       A value of 429494967295 indicates the line attenuation is out
       of range to be represented
  That way you make it more readable to non-binary folk.
  But more important, the SYNTAX clause now explicitly and exactly 
  defiones the possible values.

- same for adsl2LineStatusLnAttenUs
  there may be others, in fact next object is also the same
  Looks like a candidate/potential TC to me as well.

- WHen I See this:
   adsl2LineStatusSnrMarginDs  OBJECT-TYPE
      SYNTAX      Integer32
      UNITS       "0.1 dB"
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
         "Downstream SNR Margin is the maximum increase in dB of the
          noise power received at the ATU-R, such that the BER
          requirements are met for all downstream bearer channels.  It
          ranges from -640 to 630 units of 0.1 dB (Physical values are
          -64 to 63 dB).  A value of all 1's indicates the line
          attenuation is out of range to be represented."
  Then I wonder about the "value of all 1's", because I think that is
  minus 1, is it not, and that means it is in the range of -640 to 630.
  So I am confused.
  But anway, I would have done the SYNTAX aka
      SYNTAX    Integer32 (-640..630 | nnn)
  you must choose a vlaue for nnn (ro replace your "all 1's" I think

- In fact you have many "0.1dB" types (some as Integer32, some as Unsigned32).
  Maybe a single TC can be created for Adsl2TenthOfAdB with a syntax of
  Interger32, and then every time you use it, you specify the range that
  is valid for that specific object.

-    adsl2ChStatusPtmStatus  OBJECT-TYPE
      SYNTAX      Adsl2ChPtmStatus
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
         "Indicates current state (existing failures) of the ADSL
          channel in case its Data Path is PTM.  This is a bit-map of
          possible conditions.  The various bit positions are:

             noFailure(0),
             outOfSync (1).
         In case the channel is not of PTM Data Path the object is set
         to '0'."
  The TC however speaks about noDefect instead of noFailure.
  Pls check all labels used in the adsl2MIB with the labels actually defined
  in the TEXTUAL-CONVENTIONs.

- adsl2SCStatusSnr
  is an OCTET STRING and then in DESCRIPTION clause you speak of bytes.
  I know that some ADs have concerns that a byet is not always known
  to be an octet. So maybe (if you do a new rev any way) then use the word
  octets instead of bytes. Maybe this occures multiple times.

- I am completely confused by the following:
    adsl2SCStatusRowStatus  OBJECT-TYPE
        SYNTAX      RowStatus
        MAX-ACCESS  read-write
        DESCRIPTION
        "Row Status.  The SNMP agent should create a row in this
         table for storing the results of a DELT performed on the
         associated line, if such a row does not already exist.
         The SNMP agent may have limited resources; therefore, if
         multiple rows co-exist in the table, it may fail to add
         new rows to the table or allocate memory resources for a new
         DELT process.  If that occurs, the SNMP agent responds with
         either the value 'tableFull' or the value
         'noResources' (for adsl2LineCmndConfLdsfFailReason
         object in adsl2LineTable)
         The management system (the operator) may delete rows according
         to any scheme.  E.g., after retrieving the results."
        ::= { adsl2SCStatusEntry 19 }
  First, I don't think I have ever seen a read-write as MAX-ACCESS
  for RowStatus. I think it is always read-create. In fact you speak about
  row-creation, but only by the agent ??
  The labels of 'tableFull', 'noResources' and such do not exist to RowStatus
  (See RFC2579).  I see that these errors are supposed to be returned in 
  another object in another table... but I'd like to know what happens when
  an SNMP manager gets back when he tries to do a SET for this object
  to say createAndWait or createAndGo.
  So I am not sure what you are doing here.
  Since this is a writable object, does a row persist across reboots?

-    adsl2LInvG994VendorId  OBJECT-TYPE
      SYNTAX      OCTET STRING  (SIZE(0..8))
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
         "The ATU G.994.1 Vendor ID as inserted in the G.994.1 CL/CLR
          message.  It consists of 8 binary octets, including a country
          code followed by a (regionally allocated) provider code, as
          defined in Recommendation T.35."
  If I understand the description clause correctly (and assuming it is 
  complete), then I would expext
      SYNTAX      OCTET STRING  (SIZE(8))
  Otherwsie, what does a zero length octet string mean?
  Same for adsl2LInvSystemVendorId

- adsl2LInvVersionNumber
  at least talks about "up to 16 octets".
  but what does a zerp lenmgth string mean?

- adsl2LInvSelfTestResult
  talks about "coded as a 32-bit integer" yet it is an OCTET STRING ??
  and it seems the octets represent separate values. So in my view
  not coded as a 32-bit integer?

-   adsl2LConfProfRaModeUs  OBJECT-TYPE
      SYNTAX      Adsl2RaMode
      MAX-ACCESS  read-create
      STATUS      current
      DESCRIPTION
         "The mode of operation of a rate-adaptive ATU-R in the transmit
          direction.  The parameter can take three values:
             manual (1),
             raInit (2),
             dynamicRa (3)."
   Not in sync with the TC (capital I in raInit)

- adsl2ChConfProfMinResDataRateDs and adsl2ChConfProfMinResDataRateUs
  InDESCRIPTION clauses:
  s/DynamicRa/dynamicRa" to be consistent.

- adsl2ChConfProfMinProtectionUs
  Seems to be quite out of sync with the TC that is used as SYNTAX

-    adsl2PMLineCurrTable  OBJECT-TYPE
      SYNTAX      SEQUENCE  OF  Adsl2PMLineCurrEntry
      MAX-ACCESS  not-accessible
      STATUS      current
      DESCRIPTION
         "The table adsl2PMLineCurrTable contains current Performance
          Monitoring results of ADSL2 line.  The objects in this table
          are NOT persistent."
  Why is the NOT capitalized? NOT by itself is not rfc2119 terminology?
  Did you mean "MUST NOT be" or "MAY NOT be" ?
  AS you probably know, it is best to be clear what to expect. I.e.
  if you mean "MUST NOT be", that is good.
  If you mean "MAY NOT be", then a manager still does not know what to
  expect. In that cas I think you mean "the persistency behaviour is
  agent implementation dependent". Not so good fof a mgmt station though.
  Now,.. this is a read-only table, so in fact you then do not have
  to discuss persistency per se. The discussion of persistency behaviour
  is specifically important for read-write or read-create tables/objects,
  because a management app needs to know if a SET that succeed will
  survive reboots or not.

  Same for adsl2LineInventoryTable... maybe other tables?

- Maybe I am missing somethingm, but in:
   Adsl2PMLineCurrEntry  ::=
      SEQUENCE {
         adsl2PMLCurrUnit                    Adsl2Unit,
         adsl2PMLCurrValidIntervals          Unsigned32,
         adsl2PMLCurrInvalidIntervals        Unsigned32,
         adsl2PMLCurr15MTimeElapsed          HCPerfTimeElapsed,
         adsl2PMLCurr15MFecs                 Counter32,
         adsl2PMLCurr15MEs                   Counter32,
         adsl2PMLCurr15MSes                  Counter32,
         adsl2PMLCurr15MLoss                 Counter32,
         adsl2PMLCurr15MUas                  Counter32,
         adsl2PMLCurr1DayValidIntervals      Unsigned32,
         adsl2PMLCurr1DayInvalidIntervals    Unsigned32,
         adsl2PMLCurr1DayTimeElapsed         HCPerfTimeElapsed,
         adsl2PMLCurr1DayFecs                Counter32,
         adsl2PMLCurr1DayEs                  Counter32,
         adsl2PMLCurr1DaySes                 Counter32,
         adsl2PMLCurr1DayLoss                Counter32,
         adsl2PMLCurr1DayUas                 Counter32
      }
  This seems to be (to me) a table of 15 minute current intervals.
  So why are the TCs and schem from RFC3593 not used?
  The HCPerfTimeElapsed also confuses me a bit as it makes me think
  we talk about hig-perfromance counters, i.e. 64 bit based?

  Maybe I need more coffee first. Anyway, sending this to the authors/WG
  so that they can comment on this point, because based on the answer, some
  of the other tables in the MIB module would get similar comments from me.

Bert