Project

General

Profile

Bug #3854

OsmoPCU uses wrong CellID in BSSGP

Added by laforge 4 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Target version:
-
Start date:
03/21/2019
Due date:
% Done:

80%

Spec Reference:

Description

While writing some unrelated PCU tests, I discovered that somehow the CellID on the BSSGP side is wrong.

Whatever the BTS passes in on the PCU interface side ends uo endian-swapped in the BVC-RESET on BSSGP.

This is really odd, as all of PCUIF is in host byte order.

Looking at the commitlog, it appears that the following patch is to be blamed:

commit bdc55fad62e333951a0eb1fc7b96aaaec16dd6ff
Author: Neels Hofmeyr <neels@hofmeyr.de>
Date:   Wed Feb 21 00:39:07 2018 +0100

    implement support for 3-digit MNC with leading zeros

    Receive the mnc_3_digits flag from the PCU interface.

    Bump the PCU interface to 9.
    This is one part of the three identical pcuif_proto.h patches:
    - I49cd762c3c9d7ee6a82451bdf3ffa2a060767947 (osmo-bts)
    - I787fed84a7b613158a5618dd5cffafe4e4927234 (osmo-pcu)
    - I78f30aef7aa224b2e9db54c3a844d8f520b3aee0 (osmo-bsc)

    Add 3-digit flags and use the new RAI and LAI API from libosmocore throughout
    the code base to be able to handle an MNC < 100 that has three digits (leading
    zeros).

    Depends: Id2240f7f518494c9df6c8bda52c0d5092f90f221 (libosmocore),
             Ib7176b1d65a03b76f41f94bc9d3293a8a07d24c6 (libosmocore)

    Change-Id: I787fed84a7b613158a5618dd5cffafe4e4927234

And with all due respect, it's a prime example of how we fail

  1. a completely unrelated change (changing the endinanness of cell_id) is made as part of implementing 3 digits MNC. This should clearly have been unrelated patches
  2. the reviewer (myself in this case) didn't spot or at least not reject this "making two unrelated changes in one commit"
  3. not having test coverage for OsmoPCU

So for more than one year, we've been reporting the wrong CellID to the SGSN:/

We should learn from this that we absolutely never accept unrelated changes in the same patch. In order to introduce 3 digit MNC support, there was no need to shift the htons() around.

History

#1 Updated by laforge 4 months ago

  • Status changed from New to In Progress

#2 Updated by laforge 4 months ago

  • % Done changed from 0 to 80

#3 Updated by laforge 4 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)