February 2023 - gerrit-log - lists.osmocom.org

[L] Change in osmo-pcu[master]: pcu_l1_if_phy: flexible phy access

by neels

Attention is currently required from: laforge, pespin, fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31341 ) Change subject: pcu_l1_if_phy: flexible phy access ...................................................................... Patch Set 11: (7 comments) Patchset: PS11: i have some more nitpicks, hope you're reading them in a good tone =) File src/pcu_l1_if_phy.h: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/7f96941e_2c445138 PS11, Line 20: obj please clarify void* obj. Please provide detailed API doc about each of the callbacks, describing what they should do and what each parameter means. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/48e19362_a3671b7d PS11, Line 26: confg typo https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b96ca746_efbc4aa3 PS11, Line 27: * initialization functions. The callback may be set to NULL in case no initialization is needed. */ (in general API doc is shorter and clearer in the imperative form: Initialize PHY. May be NULL to skip. For example: add VTY config options, call library initialization. ) https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/ae7b2b03_50e6e0e6 PS11, Line 28: ctx is ctx the same as obj above? please add API doc for it File src/pcu_l1_if_phy.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d6b5bdfe_9e57f45e PS11, Line 94: comment above which comment? https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/41891725_55b73a45 PS11, Line 94: spcified typo "spcified" I guess the entire comment should be "Activate PHY." followed by description of params -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31341 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8692d1bd5d137a17cf596ee2914722f419c9978d Gerrit-Change-Number: 31341 Gerrit-PatchSet: 11 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Mon, 27 Feb 2023 00:45:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[L] Change in osmo-pcu[master]: pcu_l1_if_phy: flexible phy access

by neels

Attention is currently required from: laforge, pespin, fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31341 ) Change subject: pcu_l1_if_phy: flexible phy access ...................................................................... Patch Set 11: (11 comments) Patchset: PS11: how does it work that only one phy ops entry is added? ...by only linking one of the phy implementation .c files, right? File src/osmobts_sock.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b57d8d16_a63bc618 PS11, Line 114: the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h); if (the_pcu->phy_ops->l1if_close_pdch) the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h); in general, cb functions may be left NULL if nothing should happen File src/pcu_l1_if.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/45fc38d9_d5047329 PS11, Line 218: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn, arfcn, block_nr, if (... check against NULL cb https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a55bd44c_0063a6a3 PS11, Line 247: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 1, fn, arfcn, block_nr, data, data_len); check NULL cb https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b6991b0a_1108bab9 PS11, Line 869: if (!bts->trx[trx_nr].fl1h) && cb != NULL File src/pcu_l1_if_phy.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/8a6cd8ba_2d317868 PS11, Line 2: * (C) 2023 by sysmocom s.f.m.c. GmbH <info(a)sysmocom.de> back in the days Holger asked me to add a dash sysmocom - s.f.m.c. GmbH https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/878685d1_413a8772 PS11, Line 51: l1if_phy_entry = talloc_zero(NULL, struct l1if_phy_entry); (i wonder if -fsanitize will report this as leaked memory whenever osmo-pcu exits .. i guess it would be better to either add a ctx arg to talloc the entry from, or to add a pcu_l1if_cleanup() that talloc_free()s all list entries that the main() calls before exiting) https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/450a8b1e_823b4167 PS11, Line 71: LOGPC(DLGLOBAL, LOGL_NOTICE, "\n"); please let's never ever use LOGPC() anymore! It was a messed up concept from the start, and in particular is very awkward in gsmtap_log. either a static buffer with OSMO_STRBUF_PRINTF() or char *msg = NULL; llist_for... { osmo_talloc_asprintf(OTC_SELECT, msg, "foo"); } LOGP(msg) and maybe add, at the start, if (!log_check_level(DLGLOBAL, LOGL_NOTICE)) return; https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b9a9e931_0aeefcdd PS11, Line 75: ops suggest name singleton_ops ...but seems to me the task this return arg solves could be achieved more elegantly? I'd like to see some code that acts on non-singleton phy lists, i guess will come up in another patch. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a3620cd4_a4ecaa24 PS11, Line 81: *ops = NULL; let's add /* make sure a singleton is not mixed with other PHY */ llist_for_each_entry(l1if_phy_entry, &phy_list, list) if (l1if_phy_entry->ops->singleton) OSMO_ASSERT(llist_count(&phy_list) == 1); without this check, below loop would initialize other non-singleton phy before exiting the list on the first singleton phy, and would skip any other entries ... i.e. act weird without complaining. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a8dfec3e_eb6928f0 PS11, Line 100: if (*ops) { so the indication whether a phy is in use depends on the argument that the caller passes in to be zero initialized? seems awkward to me / gives weird errors when forgetting to zero initialize. When i see non-singleton code i might have a suggestion for a more elegant solution... for now i can only complain =) -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31341 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8692d1bd5d137a17cf596ee2914722f419c9978d Gerrit-Change-Number: 31341 Gerrit-PatchSet: 11 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Mon, 27 Feb 2023 00:32:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[M] Change in osmo-bts[master]: common: Have PCU socket connection use osmo_wqueue

by arehbein

arehbein has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/31533 ) Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... common: Have PCU socket connection use osmo_wqueue Fixes memleak in case of connected PCU process being suspended without proper close on socket Related: OS#5774 Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 --- M include/osmo-bts/bts.h M include/osmo-bts/pcuif_proto.h M src/common/pcu_sock.c 3 files changed, 57 insertions(+), 68 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/33/31533/1 diff --git a/include/osmo-bts/bts.h b/include/osmo-bts/bts.h index 0ecec1e..2c36c55 100644 --- a/include/osmo-bts/bts.h +++ b/include/osmo-bts/bts.h @@ -360,6 +360,7 @@ struct { char *sock_path; + int sock_qlength_max; } pcu; /* GSMTAP Um logging (disabled by default) */ diff --git a/include/osmo-bts/pcuif_proto.h b/include/osmo-bts/pcuif_proto.h index 0fece48..7e854ce 100644 --- a/include/osmo-bts/pcuif_proto.h +++ b/include/osmo-bts/pcuif_proto.h @@ -4,7 +4,8 @@ #include <osmocom/gsm/l1sap.h> #include <arpa/inet.h> -#define PCU_SOCK_DEFAULT "/tmp/pcu_bts" +#define PCU_SOCK_DEFAULT "/tmp/pcu_bts" +#define PCU_SOCK_QLENGTH_MAX_DEFAULT 10 #define PCU_IF_VERSION 0x0a #define TXT_MAX_LEN 128 diff --git a/src/common/pcu_sock.c b/src/common/pcu_sock.c index 8f8b3af..1d9fd2e 100644 --- a/src/common/pcu_sock.c +++ b/src/common/pcu_sock.c @@ -30,6 +30,7 @@ #include <osmocom/core/talloc.h> #include <osmocom/core/select.h> #include <osmocom/core/socket.h> +#include <osmocom/core/write_queue.h> #include <osmocom/gsm/gsm23003.h> #include <osmocom/gsm/abis_nm.h> #include <osmo-bts/logging.h> @@ -956,10 +957,11 @@ struct pcu_sock_state { struct gsm_network *net; struct osmo_fd listen_bfd; /* fd for listen socket */ - struct osmo_fd conn_bfd; /* fd for connection to lcr */ - struct llist_head upqueue; /* queue for sending messages */ + struct osmo_wqueue upqueue; /* For sending messages; has fd for conn. to PCU */ }; +static void pcu_sock_close(struct pcu_sock_state *state); + int pcu_sock_send(struct gsm_network *net, struct msgb *msg) { struct pcu_sock_state *state = net->pcu_state; @@ -974,7 +976,7 @@ msgb_free(msg); return -EINVAL; } - conn_bfd = &state->conn_bfd; + conn_bfd = &state->upqueue.bfd; if (conn_bfd->fd <= 0) { if (pcu_prim->msg_type != PCU_IF_MSG_TIME_IND && pcu_prim->msg_type != PCU_IF_MSG_INTERF_IND) @@ -983,15 +985,19 @@ msgb_free(msg); return -EIO; } - msgb_enqueue(&state->upqueue, msg); - osmo_fd_write_enable(conn_bfd); - + if (osmo_wqueue_enqueue(&state->upqueue, msg) == -ENOSPC) { + LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %d messages waiting). Closing connection\n", + state->upqueue.max_length); + pcu_sock_close(state); + } else { + osmo_fd_write_enable(conn_bfd); + } return 0; } static void pcu_sock_close(struct pcu_sock_state *state) { - struct osmo_fd *bfd = &state->conn_bfd; + struct osmo_fd *bfd = &state->upqueue.bfd; struct gsm_bts *bts; struct gsm_bts_trx *trx; unsigned int tn; @@ -1037,11 +1043,7 @@ } } - /* flush the queue */ - while (!llist_empty(&state->upqueue)) { - struct msgb *msg = msgb_dequeue(&state->upqueue); - msgb_free(msg); - } + osmo_wqueue_clear(&state->upqueue); } static int pcu_sock_read(struct osmo_fd *bfd) @@ -1090,45 +1092,30 @@ return -1; } -static int pcu_sock_write(struct osmo_fd *bfd) +static int pcu_sock_write(struct osmo_fd *bfd, struct msgb *m) { struct pcu_sock_state *state = bfd->data; int rc; + struct gsm_pcu_if *pcu_prim; - while (!llist_empty(&state->upqueue)) { - struct msgb *msg, *msg2; - struct gsm_pcu_if *pcu_prim; - - /* peek at the beginning of the queue */ - msg = llist_entry(state->upqueue.next, struct msgb, list); - pcu_prim = (struct gsm_pcu_if *)msg->data; - - osmo_fd_write_disable(bfd); - - /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ - if (!msgb_length(msg)) { - LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " - "bytes!\n", pcu_prim->msg_type); - goto dontsend; + pcu_prim = (struct gsm_pcu_if *)m->data; + osmo_fd_write_disable(&state->upqueue.bfd); + /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ + if (!msgb_length(m)) { + LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " + "bytes!\n", pcu_prim->msg_type); + return 0; + } + /* try to send it over the socket */ + rc = write(bfd->fd, msgb_data(m), msgb_length(m)); + if (rc == 0) + goto close; + if (rc < 0) { + if (errno == EAGAIN) { + osmo_fd_write_enable(bfd); + return -EAGAIN; } - - /* try to send it over the socket */ - rc = write(bfd->fd, msgb_data(msg), msgb_length(msg)); - if (rc == 0) - goto close; - if (rc < 0) { - if (errno == EAGAIN) { - osmo_fd_write_enable(bfd); - break; - } - goto close; - } - -dontsend: - /* _after_ we send it, we can deueue */ - msg2 = msgb_dequeue(&state->upqueue); - assert(msg == msg2); - msgb_free(msg); + goto close; } return 0; @@ -1138,26 +1125,11 @@ return -1; } -static int pcu_sock_cb(struct osmo_fd *bfd, unsigned int flags) -{ - int rc = 0; - - if (flags & OSMO_FD_READ) - rc = pcu_sock_read(bfd); - if (rc < 0) - return rc; - - if (flags & OSMO_FD_WRITE) - rc = pcu_sock_write(bfd); - - return rc; -} - /* accept connection coming from PCU */ static int pcu_sock_accept(struct osmo_fd *bfd, unsigned int flags) { struct pcu_sock_state *state = (struct pcu_sock_state *)bfd->data; - struct osmo_fd *conn_bfd = &state->conn_bfd; + struct osmo_fd *conn_bfd = &state->upqueue.bfd; struct sockaddr_un un_addr; socklen_t len; int rc; @@ -1178,7 +1150,7 @@ return 0; } - osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, pcu_sock_cb, state, 0); + osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, osmo_wqueue_bfd_cb, state, 0); if (osmo_fd_register(conn_bfd) != 0) { LOGP(DPCU, LOGL_ERROR, "Failed to register new connection " @@ -1206,9 +1178,12 @@ if (!state) return -ENOMEM; - INIT_LLIST_HEAD(&state->upqueue); + osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); + state->upqueue.read_cb = pcu_sock_read; + state->upqueue.write_cb = pcu_sock_write; + state->upqueue.bfd.fd = -1; + state->net = &bts_gsmnet; - state->conn_bfd.fd = -1; bfd = &state->listen_bfd; @@ -1249,7 +1224,7 @@ return; osmo_signal_unregister_handler(SS_GLOBAL, pcu_if_signal_cb, NULL); - conn_bfd = &state->conn_bfd; + conn_bfd = &state->upqueue.bfd; if (conn_bfd->fd > 0) pcu_sock_close(state); bfd = &state->listen_bfd; @@ -1265,7 +1240,7 @@ if (!state) return false; - if (state->conn_bfd.fd <= 0) + if (state->upqueue.bfd.fd <= 0) return false; return true; } -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 1 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-MessageType: newchange

2 years, 3 months

1
0
0 0

[M] Change in osmo-bsc[master]: pcu_sock: activate/deactivate PDCH on pcu reconnect

by neels

Attention is currently required from: fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 ) Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ...................................................................... Patch Set 2: (1 comment) File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/3f4fc741_58a272cd PS2, Line 940: /* FIXME: allow multiple BTS */ > what makes it hard to implement this now instead of adding a FIXME comment? i understand now, so far the entire pcu_sock.c supports only exactly one (Ericsson RBS) BTS served with a BSC co-located PCU .. same seen in pcu_rx(). -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Gerrit-Change-Number: 31497 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 23:54:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[S] Change in osmo-bsc[master]: timeslot_fsm: fix sourcecode formatting

by neels

Attention is currently required from: dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31498 ) Change subject: timeslot_fsm: fix sourcecode formatting ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31498/comment/10fdebc1_ea46f420 PS2, Line 7: sourcecode formatting indenting =) -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31498 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I1cfd377cd8014e42aa45f9f0da664598ee88e02d Gerrit-Change-Number: 31498 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 23:49:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[M] Change in osmo-bsc[master]: pcu_sock: activate/deactivate PDCH on pcu reconnect

by neels

Attention is currently required from: fixeria, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 ) Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ...................................................................... Patch Set 2: (19 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/ed4cd07a_aeea2adf PS2, Line 10: po typo Patchset: PS2: looks good in general! let's just polish some details... File doc/timeslot.msc: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9c435994_9ac70549 PS2, Line 95: bsc_ts abox bsc_ts [label="UNUSED"]; a timeslot FSM cannot transition from ST_PDCH directly to ST_UNUSED. Please insert here the actual transition as i see in the implementation: bsc_ts abox bsc_ts [label="WAIT_PDCH_DEACT (4s, T23001)"]; bts <= bsc_ts [label="RSL RF Chan Release of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Release ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="UNUSED"]; Same below for PDCH activation... https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/8c881ef0_8db1a260 PS2, Line 98: " \nfor all timeslots in state UNUSED:" https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f329b054_31e0657c PS2, Line 99: PDCH UNUSED https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/63d8ad9a_d24aca23 PS2, Line 100: bsc_ts <- pcu_sock [label="TS_EV_PDCH_ACT"]; bsc_ts abox bsc_ts [label="WAIT_PDCH_ACT (4s, T23001)"]; bts <= bsc_ts [label="RSL Chan Activ of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Activ ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="PDCH"]; File include/osmocom/bsc/timeslot_fsm.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9ecfa52e_5e39de9d PS2, Line 38: TS_EV_LCHAN_UNUSED, Would be nice to add comments to clarify how the "TS_EV_PDCH_*" are fundamentally different: /* RSL responses received from the BTS: */ https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/43ad28b0_6c1a3956 PS2, Line 42: TS_EV_PDCH_DEACT_NACK, /* An Ericsson PCU has disconnected from OsmoBSC, deactivate PDCH: */ https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/895197c7_ad141738 PS2, Line 43: TS_EV_PDCH_DEACT, /* An Ericsson PCU has reconnected to OsmoBSC, re-activate PDCH: */ File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/20e5fa50_64da4632 PS2, Line 801: printf("l1sap_chan_rel(trx,gsm_lchan2chan_nr(ts->lchan));\n"); either keep this printf, or if it should be dropped, drop it in a separate patch? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/18d0ddda_bea86340 PS2, Line 794: for (i = 0; i < PCU_IF_NUM_TRX; i++) { this is a fix of a magic number, very good but rather in a separate patch https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/a5e5a216_bf4db00c PS2, Line 801: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN Why does this apply only to dynamic timeslots? I think I understand why, but I'd like to see a code comment saying so explicitly. How about GSM_PCHAN_TCH_F_PDCH dynamic timeslots? I guess they don't apply to Ericsson RBS -- also good to clarify that in a comment. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/013eb5ea_4a1592c5 PS2, Line 802: ts->fi->state == TS_ST_PDCH > Could you clarify why checking the FSM state is necessary? It's a bit weird to see code checking `fi […] generally true, but here i agree with checking the state explicitly: Receiving EV_PDCH_DEACT only makes sense when in ST_PDCH. We could add an allstate-event handling in timeslot_fsm.c that does this same check, but emitting EV_PDCH_DEACT logging for every single timeslot, in ST_PDCH or not, would be spammy. I think my favorite would be exactly this code, but in a function implemented in timeslot_fsm.c and called from here: void ts_pdch_deact(...) { if (ts->fi->state != TS_ST_PDCH) return; osmo_fsm_inst_dispatch(ts->fi, TS_EV_PDCH_DEACT); } so exactly this code, but kept closer to the ts fsm implementation instead of spreading ts fsm behavior details into pcu_sock.c. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/39790d92_377bf241 PS2, Line 940: /* FIXME: allow multiple BTS */ what makes it hard to implement this now instead of adding a FIXME comment? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/55eb9537_4ea93938 PS2, Line 941: bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list); rather use llist_first_entry_or_null() but ... instead of always acting on the first BTS, shouldn't this find the exact BTS that the specific PCU is responsible for? .. I think I understand that this is a special case for RBS where the PCU is running next to the BSC instead of the usual next-to-BTS. Right? Add a code comment to clarify? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f16b4e18_e7f5e3c3 PS2, Line 973: gsm_bts_trx_num > As I already commented on one of your previous patches, using `gsm_bts_trx_num()` in a loop is not a […] agree https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/b2c9d9ed_dc64dc7a PS2, Line 976: 8 > ARRAY_SIZE(trx->ts) could also use TRX_NR_TS, but ARRAY_SIZE is also my favorite; TRX_NR_TS is our legacy way, these days osmo code uses ARRAY_SIZE https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/cc3a9e76_beaeb054 PS2, Line 979: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN && ts->fi->state == TS_ST_UNUSED) { Also for GSM_PCHAN_TCH_F_PDCH ? ...complementary to above, here i would love to see a function ts_pdch_act() called, function implementation in timeslot_fsm.c, so that we don't add ts_fsm state logic outside of timeslot_fsm.c File src/osmo-bsc/timeslot_fsm.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/d0917b6a_27105e03 PS2, Line 326: static void ts_fsm_unused_pdch_act(struct osmo_fsm_inst *fi) it would be cool to add this function in a separate patch before this patch, so that in this patch the reader can easily see that the "if is_ericsson" part is being added and the rest remains unchanged (also if someone from the future reviews the git log) -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Gerrit-Change-Number: 31497 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 23:45:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[S] Change in libosmo-abis[master]: trau_pcu_ericsson: add comment about uplink blocks

by neels

Attention is currently required from: dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/31475 ) Change subject: trau_pcu_ericsson: add comment about uplink blocks ...................................................................... Patch Set 2: (4 comments) File include/osmocom/trau/trau_pcu_ericsson.h: https://gerrit.osmocom.org/c/libosmo-abis/+/31475/comment/906b1736_b2f68282 PS2, Line 285: ss typo https://gerrit.osmocom.org/c/libosmo-abis/+/31475/comment/98016bfe_f665212f PS2, Line 287: "," https://gerrit.osmocom.org/c/libosmo-abis/+/31475/comment/7e411722_b01fe52b PS2, Line 290: ", using CCU version N in lab tests," ? https://gerrit.osmocom.org/c/libosmo-abis/+/31475/comment/a169e391_d72c89a9 PS2, Line 302: * The observations above were made with an RRUS 02 B3 oh. maybe this above. or nevermind. -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/31475 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: I35b9a35c9b583378559daa400e105537338ee68a Gerrit-Change-Number: 31475 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 22:43:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[XL] Change in libosmo-abis[master]: trau_pcu_ericsson: add testvectors for MCS1-MCS8

by neels

Attention is currently required from: fixeria, msuraev, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/31454 ) Change subject: trau_pcu_ericsson: add testvectors for MCS1-MCS8 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: would be nice maybe to say in the commit log how the new test vectors were obtained... manually? -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/31454 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: I7e7e35930a373c9db74faef24f6c404eb5516278 Gerrit-Change-Number: 31454 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 22:37:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[XL] Change in libosmo-abis[master]: trau_pcu_ericsson: add testvectors for MCS1-MCS8

by neels

Attention is currently required from: fixeria, msuraev, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/31454 ) Change subject: trau_pcu_ericsson: add testvectors for MCS1-MCS8 ...................................................................... Patch Set 2: Code-Review+1 (1 comment) File tests/trau_pcu_ericsson/trau_pcu_ericsson_test.c: https://gerrit.osmocom.org/c/libosmo-abis/+/31454/comment/d75275bd_ef1abc6b PS2, Line 2087: printf(" ccu_data_ind.tav=%02x\n", frame.u.ccu_data_ind.tav); > The usual approach would be grouping all test vectors into an array, and simply iterating over it in […] that is true, and i wondered the same: why does this unit test not simply have an array with all test vectors. But this patch's aim is to simply add test vectors, so this is fine as it is. Improving the way the regression test is organized would be a separate patch and semantically orthogonal to this patch. -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/31454 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: I7e7e35930a373c9db74faef24f6c404eb5516278 Gerrit-Change-Number: 31454 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 22:36:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: msuraev <msuraev(a)sysmocom.de> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

[S] Change in libosmocore[master]: bts_features: Add features for HR formats (TS 101813 vs. RFC5993)

by neels

Attention is currently required from: fixeria, pespin, msuraev, dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31415 ) Change subject: bts_features: Add features for HR formats (TS 101813 vs. RFC5993) ...................................................................... Patch Set 8: (1 comment) File include/osmocom/gsm/bts_features.h: https://gerrit.osmocom.org/c/libosmocore/+/31415/comment/7df236ee_597dcb40 PS7, Line 39: _TX > I'm confused: if it's easy to convert between the formats than it can be BTS' runtime config option. […] Max, you still write "than" instead of "then" =) For example, a sysmobts emits RTP directly from the DSP in one specific format, i.e. the RTP does not traverse osmo code. AFAIU the on-the-fly conversion can happen only in osmo-mgw, and the BSC has to know which RTP format the BTS expects, in order to instruct the MGW (not sure which MGCP item indicates the format, hope i am understanding correctly). It could be an osmo-bsc.cfg option per 'bts', but if the BTS indicates the format in BTS_FEAT, that is implicit and magically always correct. If this is correct, I'm with fixeria. Let the BTS_FEAT say which RTP format the PHY emits and expects to receive, so drop the "TX" from the name. If we are misunderstanding something, plz explain? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31415 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I05e4ed7a85f3a0451de7cd07380503a7ac76d043 Gerrit-Change-Number: 31415 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Sun, 26 Feb 2023 22:31:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: msuraev <msuraev(a)sysmocom.de> Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 3 months

1
0
0 0

2025

2024

2023

2022

gerrit-log February 2023