<p>pespin <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/15615">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, but someone else must approve
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bts-trx: Get rid of messy transceiver_available state handler<br><br>This variable meaning has been changing its exact meaning over time<br>until finally not being really clear which kind of state it holds.<br>Initially it seemed to be used to identfy whether CLOCK IND were being<br>received. However, over time both osmo-bts and osmo-trx have evolved and<br>were fixed so that clock indications are only sent by osmo-trx after<br>POWERON command is sent. As a result, this state can be checked simply by<br>looking at the "powered" phy_link variable, which is only set to true<br>once the TRX has confirmed the POWERON command, and hence it is sending<br>CLOCK IND.<br>On the other hand, at some point in time "available" started to be set to 1<br>in bts_model_phy_link_open(), which means available is nowadays almost<br>always 1 from startup until the end (only dropped during bts_shutdown(),<br>when we are already exiting the process anyway).<br>As a result, !available condition in scheduler_trx.c:trx_fn_timer_cb can<br>almost never happen, because available is set to true already. Only<br>possibility would be if an old process of osmo-trx (not set up by this<br>BTS process) is still sending CLOCK INDs, but in that case we for sure<br>don't want to configure the BTS based on that, but ignore them until<br>this BTS process has again configured the TRX. So that whole check can<br>be dropped. We are better checking for "powered" state, which is far<br>more accurate, and we better do that in trx_if.c before calling<br>trx_fn_timer_cb().<br><br>Other uses of "transceiver_available" being used can be changed to use<br>plink->state!= PHY_LINK_SHUTDOWN, since available was already being set<br>to 1 at the same time the plink->state was being set to<br>PHY_LINK_CONNECTING.<br><br>As a result of this state handling re-arrangement, OS#4215 is fixed<br>since trx_if_powered() is used instead of previous state condition to<br>check whether data frames should be sent.<br><br>Related: OS#4215<br>Change-Id: I35f4697bd33dbe8a4c76c9500b82c16589c701d4<br>---<br>M src/osmo-bts-trx/l1_if.c<br>M src/osmo-bts-trx/scheduler_trx.c<br>M src/osmo-bts-trx/trx_if.c<br>M src/osmo-bts-trx/trx_if.h<br>M src/osmo-bts-trx/trx_vty.c<br>5 files changed, 21 insertions(+), 40 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/osmo-bts-trx/l1_if.c b/src/osmo-bts-trx/l1_if.c</span><br><span>index f564dc5..221d88b 100644</span><br><span>--- a/src/osmo-bts-trx/l1_if.c</span><br><span>+++ b/src/osmo-bts-trx/l1_if.c</span><br><span>@@ -204,8 +204,18 @@</span><br><span> struct phy_instance *pinst = l1h->phy_inst;</span><br><span> struct phy_link *plink = pinst->phy_link;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!transceiver_available)</span><br><span style="color: hsl(120, 100%, 40%);">+ /* During setup, pinst may still not be associated to a TRX nr */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!pinst->trx) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPPHI(pinst, DL1C, LOGL_INFO,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Delaying provision, TRX not yet assigned to phy instance\n");</span><br><span> return -EIO;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (phy_link_state_get(plink) == PHY_LINK_SHUTDOWN) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPPHI(pinst, DL1C, LOGL_INFO,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Delaying provision, TRX not yet available\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EIO;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> </span><br><span> if (l1h->config.enabled</span><br><span> && l1h->config.tsc_valid</span><br><span>@@ -390,6 +400,7 @@</span><br><span> </span><br><span> llist_for_each_entry(trx, &bts->trx_list, list) {</span><br><span> struct phy_instance *pinst = trx_phy_instance(trx);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct phy_link *plink = pinst->phy_link;</span><br><span> struct trx_l1h *l1h = pinst->u.osmotrx.hdl;</span><br><span> if (l1h->config.bsic != bsic || !l1h->config.bsic_valid) {</span><br><span> l1h->config.bsic = bsic;</span><br><span>@@ -397,7 +408,7 @@</span><br><span> l1h->config.bsic_sent = 0;</span><br><span> l1if_provision_transceiver_trx(l1h);</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- check_transceiver_availability_trx(l1h, transceiver_available);</span><br><span style="color: hsl(120, 100%, 40%);">+ check_transceiver_availability_trx(l1h, phy_link_state_get(plink) != PHY_LINK_SHUTDOWN);</span><br><span> }</span><br><span> </span><br><span> return 0;</span><br><span>diff --git a/src/osmo-bts-trx/scheduler_trx.c b/src/osmo-bts-trx/scheduler_trx.c</span><br><span>index b3b656a..af639e2 100644</span><br><span>--- a/src/osmo-bts-trx/scheduler_trx.c</span><br><span>+++ b/src/osmo-bts-trx/scheduler_trx.c</span><br><span>@@ -1721,21 +1721,6 @@</span><br><span> </span><br><span> clock_gettime(CLOCK_MONOTONIC, &tv_now);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* clock becomes valid */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!transceiver_available) {</span><br><span style="color: hsl(0, 100%, 40%);">- LOGP(DL1C, LOGL_NOTICE, "initial GSM clock received: fn=%u\n", fn);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- transceiver_available = 1;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- /* start provisioning transceiver */</span><br><span style="color: hsl(0, 100%, 40%);">- l1if_provision_transceiver(bts);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- /* tell BSC */</span><br><span style="color: hsl(0, 100%, 40%);">- check_transceiver_availability(bts, 1);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- return trx_setup_clock(bts, tcs, &tv_now, &interval, fn);</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /* calculate elapsed time +fn since last timer */</span><br><span> elapsed_us = compute_elapsed_us(&tcs->last_fn_timer.tv, &tv_now);</span><br><span> elapsed_fn = compute_elapsed_fn(tcs->last_fn_timer.fn, fn);</span><br><span>diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c</span><br><span>index a260919..9933109 100644</span><br><span>--- a/src/osmo-bts-trx/trx_if.c</span><br><span>+++ b/src/osmo-bts-trx/trx_if.c</span><br><span>@@ -48,8 +48,6 @@</span><br><span> #include "l1_if.h"</span><br><span> #include "trx_if.h"</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-int transceiver_available = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /*</span><br><span> * socket helper functions</span><br><span> */</span><br><span>@@ -125,6 +123,10 @@</span><br><span> "wrapping correctly, correcting to fn=%u\n", fn);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (!plink->u.osmotrx.powered) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPPHI(pinst, DTRX, LOGL_NOTICE, "Ignoring CLOCK IND %u, TRX not yet powered on\n", fn);</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> /* inform core TRX clock handling code that a FN has been received */</span><br><span> trx_sched_clock(pinst->trx->bts, fn);</span><br><span> </span><br><span>@@ -201,13 +203,6 @@</span><br><span> va_list ap;</span><br><span> int pending;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!transceiver_available &&</span><br><span style="color: hsl(0, 100%, 40%);">- !(!strcmp(cmd, "POWEROFF") || !strcmp(cmd, "POWERON"))) {</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPPHI(l1h->phy_inst, DTRX, LOGL_ERROR, "CTRL %s ignored: No clock from "</span><br><span style="color: hsl(0, 100%, 40%);">- "transceiver, please fix!\n", cmd);</span><br><span style="color: hsl(0, 100%, 40%);">- return -EIO;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> pending = !llist_empty(&l1h->trx_ctrl_list);</span><br><span> </span><br><span> /* create message */</span><br><span>@@ -1097,12 +1092,11 @@</span><br><span> /* copy ubits {0,1} */</span><br><span> memcpy(buf + 6, bits, nbits);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* we must be sure that we have clock, and we have sent all control</span><br><span style="color: hsl(0, 100%, 40%);">- * data */</span><br><span style="color: hsl(0, 100%, 40%);">- if (transceiver_available && llist_empty(&l1h->trx_ctrl_list)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* we must be sure that TRX is on */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (trx_if_powered(l1h)) {</span><br><span> send(l1h->trx_ofd_data.fd, buf, nbits + 6, 0);</span><br><span> } else</span><br><span style="color: hsl(0, 100%, 40%);">- LOGPPHI(l1h->phy_inst, DTRX, LOGL_ERROR, "Ignoring TX data, transceiver offline.\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGPPHI(l1h->phy_inst, DTRX, LOGL_ERROR, "Ignoring TX data, transceiver powered off.\n");</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span>@@ -1255,8 +1249,7 @@</span><br><span> if (trx_phy_inst_open(pinst) < 0)</span><br><span> goto cleanup;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- /* FIXME: is there better way to check/report TRX availability? */</span><br><span style="color: hsl(0, 100%, 40%);">- transceiver_available = 1;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> return 0;</span><br><span> </span><br><span> cleanup:</span><br><span>diff --git a/src/osmo-bts-trx/trx_if.h b/src/osmo-bts-trx/trx_if.h</span><br><span>index 3325c62..fd0077d 100644</span><br><span>--- a/src/osmo-bts-trx/trx_if.h</span><br><span>+++ b/src/osmo-bts-trx/trx_if.h</span><br><span>@@ -1,8 +1,6 @@</span><br><span> #ifndef TRX_IF_H</span><br><span> #define TRX_IF_H</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-extern int transceiver_available;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> struct trx_l1h;</span><br><span> </span><br><span> struct trx_ctrl_msg {</span><br><span>diff --git a/src/osmo-bts-trx/trx_vty.c b/src/osmo-bts-trx/trx_vty.c</span><br><span>index 993c780..86f5712 100644</span><br><span>--- a/src/osmo-bts-trx/trx_vty.c</span><br><span>+++ b/src/osmo-bts-trx/trx_vty.c</span><br><span>@@ -58,12 +58,6 @@</span><br><span> struct gsm_bts_trx *trx;</span><br><span> struct trx_l1h *l1h;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!transceiver_available) {</span><br><span style="color: hsl(0, 100%, 40%);">- vty_out(vty, "transceiver is not connected%s", VTY_NEWLINE);</span><br><span style="color: hsl(0, 100%, 40%);">- } else {</span><br><span style="color: hsl(0, 100%, 40%);">- vty_out(vty, "transceiver is connected%s", VTY_NEWLINE);</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> llist_for_each_entry(trx, &bts->trx_list, list) {</span><br><span> struct phy_instance *pinst = trx_phy_instance(trx);</span><br><span> struct phy_link *plink = pinst->phy_link;</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/15615">change 15615</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bts/+/15615"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I35f4697bd33dbe8a4c76c9500b82c16589c701d4 </div>
<div style="display:none"> Gerrit-Change-Number: 15615 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>