Patch Set 2: Code-Review-1

File include/osmocom/bsc/acc_ramp.h:

Line 1: /* (C) 2018 Stefan Sperling <>
(C) sysmocom / Author: Stefan Sperling, please look at other examples, thanks.

Line 41:        struct gsm_bts *bts; /* backpointer to BTS using this ACC ramp 
one could avoid the back-pointer if we used offsetof() to go back from acc_ramp 
to gsm_bts, assuming that the acc_ramp is always embedded there.  Or one could 
even modify the API to directly operate on gsm_bts rather than acc_ramp.  But 
these are just random thoughts, we can keep it as is.  It's just a bit unusual 
for Osmocom to have a static, embedded structure which then has a back-pointer.
File include/osmocom/bsc/gsm_data_shared.h:

Line 792:       struct acc_ramp acc_ramp;
continuing form my previous comment: If the struct acc_ramp was dynamically 
allocated, then one would need (and expect) a back pointer.  And here one could 
remove the acc_ramping_enabled member and simply check for if (bts->acc_ramp) ? 
Again just ideas, no requirement to change.
File src/libbsc/acc_ramp.c:

Line 68: static void allow_all_allowed_accs(struct acc_ramp *acc_ramp)
the naming is a bit ... odd.

PS2, Line 93: DRLL
this is not related to the Radio Link Layer (which is a sub-layer of RSL).  We 
need to find a more matching log subsystem.

Line 98: static void send_bts_system_info(struct gsm_bts *bts)
this should become a public function that's part of the general bts/trx/sysinfo 
code, rather than a private/static function here.

Line 107: static void do_ramping_step(void *data)
'acc' in the name might be useful in backtraces/debugging/... to indicate which 
of the various ramps this function adrresses

Line 169:       LOGP(DRLL, LOGL_DEBUG, "(bts=%d) ACC RAMP: ramping step size 
set to %u\n", acc_ramp->bts->nr, step_size);
see above, RLL is not applicable in all the log statements in this file.
File src/libbsc/bsc_init.c:

Line 335:               acc_ramp_start(&trx->bts->acc_ramp);
your first ramp step is executed before the call to 
gsm_bts_trx_set_system_infos().  So you will send system information messages 
to the BTS which might contain lots of uninitialized fields, as far as I can 
tell.  Maybe we need to split the "set the systeminfo buffers from vty config 
data" and the "send system infos to BTS" parts into separate functions?
File src/libbsc/bsc_vty.c:

PS2, Line 3135: enabled (0|1)
while we have some commands like this dating back to the early days, it is 
discouraged these days. "(enabled|disabled)" would be an alternative.  And the 
true "Switch/Router style" would be to have "access-control-class-ramping" and 
"no access-control-class-ramping".
File src/libbsc/system_information.c:

Line 662: static void apply_acc_ramp(struct gsm48_rach_control *rach_control, 
struct acc_ramp *acc_ramp)
I think that should be part of the acc related code, not here?  At that point, 
for exported/non-static functions, we prefer to have the object name first, 
i.e. acc_ramp_apply().

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0a5ac3a08f992f326435944f17e0a9171911afb0
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <>
Gerrit-Reviewer: Harald Welte <>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Stefan Sperling <>
Gerrit-Reviewer: Vadim Yanitskiy <>
Gerrit-HasComments: Yes

Reply via email to