<p style="white-space: pre-wrap; word-wrap: break-word;">(let me first post this and follow up with review on the remaining patch later)</p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/12625">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h@121">Patch Set #2, Line 121:</a> <code style="font-family:monospace,monospace">    struct channel_mode_and_rate ch_mode_rate_alt;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are there really only two possible mode,rate combinations?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c@1391">Patch Set #2, Line 1391:</a> <code style="font-family:monospace,monospace">   if (!lchan) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">These changes above make up a separate fix by themselves. I personally understand this change because I explained the mistake I made here to you, but the reason definitely should appear in the commit log.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's put this in a separate patch and explain like this:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Fix TCH-as-SDCCH allocation on Channel Request</p><p style="white-space: pre-wrap; word-wrap: break-word;">On rsl_rx_chan_rqd(), so far osmo-bsc tried to preferably assign the lchan type<br>that was asked for in the RACH. Firstly, this contained a bug, and secondly,<br>it does not make sense to heed that preference, since we do late assignment.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Ignore the preference for the MS' TCH kind.<br>We do late assignment to avoid codec mismatches. In the "old days", we would<br>heed the MS' TCH channel kind, even if the MSC or BSC didn't actually allow<br>or prefer that channel kind. Hence, in the presence of both TCH/F and TCH/H,<br>the MS could ask for TCH/F (which we would grant on the MO side) and the BSC<br>or MSC could prefer TCH/H (which we would apply on the MT side), and hence<br>fabricate a codec mismatch. Instead, since quite some time now, we *always*<br>assign an SDCCH first, and only later on do another Assignment to hand out<br>a proper voice lchan that heeds the MS capability bits as well as MSC's and<br>BSC's preferences.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">By completely ignoring the channel kind the MS asked for in the RACH, we<br>also eliminate this bug in rsl_rx_chan_rqd():<br>- If the first "lchan_select_by_type(GSM_LCHAN_SDDCH)" fails (all SDDCH in use),<br>  we should try to fall back to any TCH instead, to serve as SDCCH.<br>- the first "if (!lchan && lctype != GSM_LCHAN_SDCCH)" was an attempt to prefer<br>  a fallback to the lchan type the MS requested.<br>- the remaining two "if (!lchan && lctype == GSM_LCHAN_SDCCH)" were obviously<br>  only applied if the MS asked for an SDCCH, and skipped if the type was TCH/*.<br>- hence, missing was: if the MS asked for a TCH, to also try the *other* TCH<br>  kind that the MS did not ask for. (Example: all SDCCH in use, MS asks for<br>  TCH/F, but BSC has only TCH/H lchans; we should assign TCH/H as SDCCH, instead<br>  we said "no resources")</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12625">change 12625</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/12625"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 </div>
<div style="display:none"> Gerrit-Change-Number: 12625 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 21 Jan 2019 13:52:44 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>