Attention is currently required from: osmith, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/33970 )
Change subject: sm: Introduce APIs to enc/dec QoS Profile
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS1:
> Regarding endianness, should we disable the check? it will keep on failing in all future patches if […]
I think it's safe to disable the check and then subsequently at low priority submit patches to the various projects to remove the related generated code.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/33970
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: I6c0676e55bb1f0f424f41d8d04d4f5e5bf376f4f
Gerrit-Change-Number: 33970
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 29 Jul 2023 08:40:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: osmith <osmith(a)sysmocom.de>
Gerrit-MessageType: comment
laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/34006 )
Change subject: cosmetic: Document foce_two_phase feature based on specs
......................................................................
cosmetic: Document foce_two_phase feature based on specs
Change-Id: I4f8e3d0dcb721d51838b50aba5b40d0551c8d0c5
---
M src/bts.cpp
1 file changed, 15 insertions(+), 0 deletions(-)
Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, approved
diff --git a/src/bts.cpp b/src/bts.cpp
index 177544a..fe48018 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -970,6 +970,12 @@
LOGP(DRLCMAC, LOGL_DEBUG, "MS requests single TS uplink transmission "
"(one phase packet access)\n");
if (bts->pcu->vty.force_two_phase) {
+ /* 3GPP TS 44.018 3.5.2.1.3.1: "If the establishment cause in the
+ * CHANNEL REQUEST message indicates a request for one phase packet access,
+ * the network may grant either a one phase packet access or a single block
+ * packet access for the mobile station. If a single block packet access is
+ * granted, it forces the mobile station to perform a two phase packet access."
+ */
LOGP(DRLCMAC, LOGL_DEBUG, "Forcing two phase access\n");
chan_req.single_block = true;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/34006
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I4f8e3d0dcb721d51838b50aba5b40d0551c8d0c5
Gerrit-Change-Number: 34006
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: merged
Attention is currently required from: wbokslag.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/34001 )
Change subject: Added stub for decryption and full keystream application functions
......................................................................
Patch Set 2:
(2 comments)
File src/crypto/tetra_crypto.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34001/comment/87f12f9f_714dba42
PS2, Line 207: case KSG_TEA1:
: tea1_stub(iv, eck, num_bytes, ks_bytes);
: break;
: case KSG_TEA2:
: tea2_stub(iv, eck, num_bytes, ks_bytes);
: break;
: case KSG_TEA3:
: tea3_stub(iv, eck, num_bytes, ks_bytes);
: break;
I think it's a bit weird to add the stub functions that don't do anything in a commit here. This leads to situations that if somebody calls the function now, it returns success (true) despite not having generated the keystream. I think it would be wise to not merge the stub functions at all, and have generate_keystream run into the default-clause below which gives a proper error message and return code instead of silent failure.
https://gerrit.osmocom.org/c/osmo-tetra/+/34001/comment/cea1cccd_ff18f74c
PS2, Line 305: /
we typically prefer /* */ comments. Feels especially inconsistenc since this very same patch is adding multiple comments in different styles.
--
To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34001
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: I4e6147f206ad6046f32e08015ec9721b64382ca1
Gerrit-Change-Number: 34001
Gerrit-PatchSet: 2
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Sat, 29 Jul 2023 08:37:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: wbokslag.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/34000 )
Change subject: Fixups and clarifying comments for msgb tail modifications
......................................................................
Patch Set 2:
(6 comments)
File src/lower_mac/tetra_lower_mac.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/c28a8e8e_e37afbcb
PS2, Line 347: msg->len = msg
in general we don't ever manually manipulate msgb's this way in other osmocom software. We use the various msgb.[ch] functions as high-level operations on the buffer (like msgb_put/pull/push/get/trim/reserve/...), but don't consider manipulation of the individual fields as good code.
I don't know the specific use case here (and have too many other tasks to review in detail) so I don't have a concrete suggestion, just sharing general thoughts.
File src/tetra_llc.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/e33e4841_ee2edb49
PS2, Line 124: msg->tail = msg->l3h + lpp.tl_sdu_len; // Strips off FCS (if present)
: msg->len = msg->tail - msg->head;
this looks a bit like openn-coding a msgb_get(), or probably even more a msgb_l3trim() ?
File src/tetra_upper_mac.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/68250cd6_9fecc52e
PS2, Line 178: msg->len = m
same here. msgb_trim?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/7b1ffe59_7871231b
PS2, Line 185: msg->len = m
same here. msgb_trim?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/edec0a07_0a8865b4
PS2, Line 306: msg->len = m
yet another msgb_trim? Also, the entire get_num_fill_bits + following operations happens several times in the code, so it might make sense to create a function to cover the sequence of getting the number of fill-bits and then trimming the message to exclude those?
https://gerrit.osmocom.org/c/osmo-tetra/+/34000/comment/46364861_fc916427
PS2, Line 359: msg->len = m
again another fill-bits-stripping situation.
--
To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/34000
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: Ia725edbeafe26bd2ea9b5a1810d0b26bc79d84db
Gerrit-Change-Number: 34000
Gerrit-PatchSet: 2
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Sat, 29 Jul 2023 08:34:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment