Bug #3833


Fix storage location of AMR S15-S0 bits

Added by dexter over 4 years ago. Updated 12 months ago.

Target version:
Start date:
Due date:
% Done:


Spec Reference:


The current storage location of s15_s0 (lchan-> is incorrect by the current memory model. This should be fixed by finding a proper storage location that fits into the current memory model.

See also:

Related issues

Related to OsmoBSC - Bug #3787: Assignment Request modifies conn->codec_list before the Assignment is successfulNew02/07/2019

Actions #1

Updated by laforge over 4 years ago

  • Status changed from New to Feedback

both patches are merged. Does that fix/resolved this issue?

Actions #2

Updated by dexter over 4 years ago

Its a "cosmetic" problem. Neels has some model in mind where temporary and permanent values should be stored. I am not sure that I understand every aspect of this right. I think we should wait until neels is back and then we can fix this together. There is no pressure on this, technically its just moving a struct member to the correct sub struct to have everything consistent.

Actions #3

Updated by neels over 4 years ago

lchan-> = the settings that were requested on lchan_activate(). So lchan->activate.* is preliminary and volatile, used before the lchan actually is activated successfully.

As soon as the lchan activation is successful, all valid information should end up in lchan->foo fields (outside of lchan->activate),
in this case probably lchan->s15_s0.

The distinction is quite trivial. lchan->activate contains the settings that are requested before setting up the lchan, and aren't verified to be really active.

At any time, some code (bug?) might try to invoke another activation request and overwrite lchan->activate.* data.
So any items that describe the actually active lchan should not be kept in lchan->activate.*

A similar concept exists in various other FSMs, it is an important mechanism that separates possibly invalid manipulation requests from the actually verified active settings.

Actions #4

Updated by laforge almost 4 years ago

  • Priority changed from Normal to Low
Actions #5

Updated by neels almost 4 years ago

  • Status changed from Feedback to New
  • Assignee changed from dexter to neels

laforge wrote:

both patches are merged. Does that fix/resolved this issue?

The linked issue was merged still containing the problem.
It is however "merely" a cosmetic / code maintenance problem, not distinguishable by the user.
I guess I should just clean this up since original authors clearly can't be bothered.

Actions #6

Updated by neels almost 4 years ago

  • Related to Bug #3787: Assignment Request modifies conn->codec_list before the Assignment is successful added
Actions #7

Updated by neels about 3 years ago

quoting G#13039 comment:

The assignment_fsm.c stores the chosen ch_mode_rate in lchan->ch_mode_rate, even though the assignment or activation are not complete yet, which violates above ideas and confuses transitory and accepted config.

So assignment_fsm_start() should not leak its state into lchan->*, but should keep a separate ch_mode_rate somewhere else, I'd say in conn->assignment.new_lchan_ch_mode_rate or so. (If it really needs to store it at all.)

This ch_mode_rate should get passed to lchan activation, where lchan_fsm.c then first stores it in lchan->activate.* as transitory state, and when the activation was ACKed copies it to lchan->ch_mode_rate.

That's how we ensure the lchan->* items reflect what is currently used, and transitory requests make their potential mess elsewhere.

Actions #8

Updated by neels over 2 years ago

  • Status changed from New to In Progress

cleaning up (part of?) this in the process of #3277

The reason why this is required is that for a re-assignment, the lchan needs to reflect the currently activated chan_mode_rate.
So far the chosen chan_mode_rate is only stored in the previous lchan data, which defies all logic.

A set of chan_mode_rates shall be chosen in the Assignment procedure, placed in a struct assignment_request,
one of them shall be chosen and placed in struct assignment_fsm_data.
Upon channel activation/modify the chosen chan_mode_rate must pass through struct lchan_activate_info or lchan_modify_info,
and finally, when the activation/modify is ACKed, the new lchan should reflect the actually used chan_mode_rate.
Hence, a failed activation/modify will neither change the previous lchan nor the new lchan,
and after a successful activation/modify, subsequent re-assignment/modify procedures know an lchan's current mode and rate accurately.

(struct channel_mode_and_rate includes the S15-S0 bits)

Actions #9

Updated by neels over 2 years ago

neels wrote:

cleaning up (part of?) this in the process of #3357

That should have said #3277

Actions #10

Updated by fixeria 12 months ago

  • Priority changed from Low to High

A year in progress? This is potentially affecting one of our customers provoking a crash of osmo-bsc. Increasing priority.

Actions #11

Updated by neels 12 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100


Solved by: Ie0da36124d73efc28a8809b63d7c96e2167fc412
more than a year ago. I forgot to update this issue.


Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)