From: Max msuraev@sysmocom.de
--- .gitignore | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/.gitignore b/.gitignore index f11bc4d..349cfb0 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,11 @@ src/osmo-bts-sysmo/osmo-bts-sysmo-remote src/osmo-bts-sysmo/sysmobts-mgr src/osmo-bts-sysmo/sysmobts-util
+src/osmo-bts-litecell15/lc15bts-mgr +src/osmo-bts-litecell15/lc15bts-util +src/osmo-bts-litecell15/misc/.dirstamp +src/osmo-bts-litecell15/osmo-bts-lc15 + src/osmo-bts-trx/osmo-bts-trx
src/osmo-bts-octphy/osmo-bts-octphy
From: Max msuraev@sysmocom.de
--- src/osmo-bts-litecell15/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/osmo-bts-litecell15/main.c b/src/osmo-bts-litecell15/main.c index 5f2d052..ef132f8 100644 --- a/src/osmo-bts-litecell15/main.c +++ b/src/osmo-bts-litecell15/main.c @@ -93,6 +93,14 @@ int bts_model_init(struct gsm_bts *bts) return 0; }
+void bts_model_phy_link_set_defaults(struct phy_link *plink) +{ +} + +void bts_model_phy_instance_set_defaults(struct phy_instance *pinst) +{ +} + int bts_model_oml_estab(struct gsm_bts *bts) { return 0;
On Tue, Mar 22, 2016 at 03:40:09PM +0100, msuraev@sysmocom.de wrote:
+void bts_model_phy_link_set_defaults(struct phy_link *plink) +void bts_model_phy_instance_set_defaults(struct phy_instance *pinst)
thanks, applied.
From: Max msuraev@sysmocom.de
Add null pointer check and propagate error. --- include/osmo-bts/phy_link.h | 5 ++++- src/osmo-bts-litecell15/l1_if.c | 6 ++++++ src/osmo-bts-litecell15/l1_if.h | 6 ++++-- 3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/osmo-bts/phy_link.h b/include/osmo-bts/phy_link.h index a559aa3..5ab5d49 100644 --- a/include/osmo-bts/phy_link.h +++ b/include/osmo-bts/phy_link.h @@ -125,7 +125,10 @@ void phy_user_statechg_notif(struct phy_instance *pinst, enum phy_link_state lin
static inline struct phy_instance *trx_phy_instance(struct gsm_bts_trx *trx) { - return trx->role_bts.l1h; + if (trx) + return trx->role_bts.l1h; + + return NULL; }
int bts_model_phy_link_open(struct phy_link *plink); diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c index f625968..d89cc29 100644 --- a/src/osmo-bts-litecell15/l1_if.c +++ b/src/osmo-bts-litecell15/l1_if.c @@ -1242,6 +1242,12 @@ static int reset_compl_cb(struct gsm_bts_trx *trx, struct msgb *resp, Litecell15_Prim_t *sysp = msgb_sysprim(resp); GsmL1_Status_t status = sysp->u.layer1ResetCnf.status;
+ if (!fl1h) { + LOGP(DL1C, LOGL_ERROR, "reset_compl_cb() is unable to get fl1h" + " from trx\n"); + return 1; + } + LOGP(DL1C, LOGL_NOTICE, "Rx L1-RESET.conf (status=%s)\n", get_value_string(lc15bts_l1status_names, status));
diff --git a/src/osmo-bts-litecell15/l1_if.h b/src/osmo-bts-litecell15/l1_if.h index 0c8843b..773840d 100644 --- a/src/osmo-bts-litecell15/l1_if.h +++ b/src/osmo-bts-litecell15/l1_if.h @@ -117,8 +117,10 @@ int l1if_ms_pwr_ctrl(struct gsm_lchan *lchan, const int uplink_target, static inline struct lc15l1_hdl *trx_lc15l1_hdl(struct gsm_bts_trx *trx) { struct phy_instance *pinst = trx_phy_instance(trx); - OSMO_ASSERT(pinst); - return pinst->u.lc15.hdl; + if (pinst) + return pinst->u.lc15.hdl; + + return NULL; }
static inline struct gsm_bts_trx *lc15l1_hdl_trx(struct lc15l1_hdl *fl1h)
On Tue, Mar 22, 2016 at 03:40:10PM +0100, msuraev@sysmocom.de wrote:
--- a/src/osmo-bts-litecell15/l1_if.h +++ b/src/osmo-bts-litecell15/l1_if.h @@ -117,8 +117,10 @@ int l1if_ms_pwr_ctrl(struct gsm_lchan *lchan, const int uplink_target, static inline struct lc15l1_hdl *trx_lc15l1_hdl(struct gsm_bts_trx *trx) { struct phy_instance *pinst = trx_phy_instance(trx);
- OSMO_ASSERT(pinst);
- return pinst->u.lc15.hdl;
- if (pinst)
return pinst->u.lc15.hdl;- return NULL;
So, an OSMO_ASSERT() is typically there to ensure that the code never ends up breaking the assertion. If the OSMO_ASSERT() ever hits, it would mean that there's something fatally wrong elsewhere in the code.
Have you checked in this instance that a NULL pinst may be a valid situation that doesn't need to be asserted upon? If so, I would welcome if that were mentioned in the log message.
The other chunks above look like pretty obvious improvements, yet maybe the reasons why they cause segfaults are also elsewhere in the code? (We don't always check all pointers because we "know" they are fine.)
I have no idea, really, just poking / provoking verbosity ;)
~Neels
Hi Max,
regarding the original patch:
* the commit message fails to explain why this fixes a segfault, and when that segfault occurs * if Neels provides some feedback like the one he did, please follow-up to that to keep progress on this patch going.
If the OSMO_ASSERT() in the original code was wrong, then please explain why it was wrong. This explanation belongs in the commit log message.
Also, as trx_lc15l1_hdl() can now return NULL, did you verify that all callers of that function can actually deal with a NULL return value?
Regards, Harald
From: Max msuraev@sysmocom.de
Use bool type for boolean values. Make if order more natural. --- src/osmo-bts-litecell15/l1_if.c | 44 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c index d89cc29..d810248 100644 --- a/src/osmo-bts-litecell15/l1_if.c +++ b/src/osmo-bts-litecell15/l1_if.c @@ -28,7 +28,7 @@ #include <unistd.h> #include <errno.h> #include <fcntl.h> - +#include <stdbool.h> #include <sys/types.h> #include <sys/stat.h>
@@ -96,7 +96,7 @@ static void l1if_req_timeout(void *data) }
static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, - int is_system_prim, l1if_compl_cb *cb, void *data) + bool is_system_prim, l1if_compl_cb *cb, void *data) { struct wait_l1_conf *wlc; struct osmo_wqueue *wqueue; @@ -108,23 +108,7 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, wlc->cb_data = data;
/* Make sure we actually have received a REQUEST type primitive */ - if (is_system_prim == 0) { - GsmL1_Prim_t *l1p = msgb_l1prim(msg); - - LOGP(DL1P, LOGL_INFO, "Tx L1 prim %s\n", - get_value_string(lc15bts_l1prim_names, l1p->id)); - - if (lc15bts_get_l1prim_type(l1p->id) != L1P_T_REQ) { - LOGP(DL1C, LOGL_ERROR, "L1 Prim %s is not a Request!\n", - get_value_string(lc15bts_l1prim_names, l1p->id)); - talloc_free(wlc); - return -EINVAL; - } - wlc->is_sys_prim = 0; - wlc->conf_prim_id = lc15bts_get_l1prim_conf(l1p->id); - wqueue = &fl1h->write_q[MQ_L1_WRITE]; - timeout_secs = 30; - } else { + if (is_system_prim) { Litecell15_Prim_t *sysp = msgb_sysprim(msg);
LOGP(DL1C, LOGL_INFO, "Tx SYS prim %s\n", @@ -139,9 +123,25 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, wlc->is_sys_prim = 1; wlc->conf_prim_id = lc15bts_get_sysprim_conf(sysp->id); wqueue = &fl1h->write_q[MQ_SYS_WRITE]; - timeout_secs = 30; + } else { + GsmL1_Prim_t *l1p = msgb_l1prim(msg); + + LOGP(DL1P, LOGL_INFO, "Tx L1 prim %s\n", + get_value_string(lc15bts_l1prim_names, l1p->id)); + + if (lc15bts_get_l1prim_type(l1p->id) != L1P_T_REQ) { + LOGP(DL1C, LOGL_ERROR, "L1 Prim %s is not a Request!\n", + get_value_string(lc15bts_l1prim_names, l1p->id)); + talloc_free(wlc); + return -EINVAL; + } + wlc->is_sys_prim = 0; + wlc->conf_prim_id = lc15bts_get_l1prim_conf(l1p->id); + wqueue = &fl1h->write_q[MQ_L1_WRITE]; }
+ timeout_secs = 30; + /* enqueue the message in the queue and add wsc to list */ if (osmo_wqueue_enqueue(wqueue, msg) != 0) { /* So we will get a timeout but the log message might help */ @@ -163,13 +163,13 @@ static int _l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, int l1if_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, l1if_compl_cb *cb, void *data) { - return _l1if_req_compl(fl1h, msg, 1, cb, data); + return _l1if_req_compl(fl1h, msg, true, cb, data); }
int l1if_gsm_req_compl(struct lc15l1_hdl *fl1h, struct msgb *msg, l1if_compl_cb *cb, void *data) { - return _l1if_req_compl(fl1h, msg, 0, cb, data); + return _l1if_req_compl(fl1h, msg, false, cb, data); }
/* allocate a msgb containing a GsmL1_Prim_t */
On Tue, Mar 22, 2016 at 03:40:11PM +0100, msuraev@sysmocom.de wrote:
int is_system_prim, l1if_compl_cb *cb, void *data)
bool is_system_prim, l1if_compl_cb *cb, void *data)
Hey, if Max is using bool, I also want to use bool!
The stance a few months ago was that I would be the first to use the bool type instead of int to indicate boolean values.
Is there a consensus that using bool is fine? Thanks!
~Neels
On Tue, Mar 22, 2016 at 04:39:07PM +0100, Neels Hofmeyr wrote:
Is there a consensus that using bool is fine?
true.
Hi Max,
please refrain from refactoring litecell15 code without also refactoring the osmo-bts-sysmo code (whihc was used as a template for osmo-bts-litecell15) at the same time, too. We don't want those two to diverge any more than absolutely neccessary. Thanks!
On 22 Mar 2016, at 18:40, Harald Welte laforge@gnumonks.org wrote:
Hi Max,
please refrain from refactoring litecell15 code without also refactoring the osmo-bts-sysmo code (whihc was used as a template for osmo-bts-litecell15) at the same time, too. We don't want those two to diverge any more than absolutely neccessary. Thanks!
and I prefer a minimal diff even if you change it to if (!is..) else to avoid moving the two branches.
holger
Shouldn't we move the code in question to src/common than?
On 03/22/2016 06:40 PM, Harald Welte wrote:
Hi Max,
please refrain from refactoring litecell15 code without also refactoring the osmo-bts-sysmo code (whihc was used as a template for osmo-bts-litecell15) at the same time, too. We don't want those two to diverge any more than absolutely neccessary. Thanks!
Hi Max,
On Wed, Mar 23, 2016 at 03:44:08PM +0100, Max wrote:
Shouldn't we move the code in question to src/common than?
No, the code is similar but not identical, and the L1 interfaces of the underlying PHY are not guaranteed to stay as similar as they are.
So some of the general structure can probably be further common-ized, but the details regarding the L1 interface primitives will still stay PHY (bts-model-)specific.
Also, the point is that so far nobody has volunteered to do (or fund) such an effort.
It is typically relatively easy for companies to take care / ownership / funding of the driver for their specific hardware, rather it is for refactoring and general code improvement. This is not specific to OsmoBTS, but you can also see this quite a lot if you look at Linux kernel development until fairly recently.
It takes both an interest in long-term effects as well as a deep understanding about the nature of collaborative free software development processes to invest in the improvement of the (core/common) code.
Regards, Harald
From: Max msuraev@sysmocom.de
Fixes: OS#1665 --- src/osmo-bts-litecell15/l1_if.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c index d810248..3ef2588 100644 --- a/src/osmo-bts-litecell15/l1_if.c +++ b/src/osmo-bts-litecell15/l1_if.c @@ -1388,6 +1388,11 @@ int bts_model_phy_link_open(struct phy_link *plink)
OSMO_ASSERT(pinst);
+ if (!pinst->trx) { + LOGP(DL1C, LOGL_NOTICE, "Ignoring phy link %d instance %d " + "because no TRX associated with it\n", plink->num, pinst->num); + return 0; + } phy_link_state_set(plink, PHY_LINK_CONNECTING);
pinst->u.lc15.hdl = l1if_open(pinst); @@ -1399,5 +1404,7 @@ int bts_model_phy_link_open(struct phy_link *plink) l1if_reset(pinst->u.lc15.hdl);
phy_link_state_set(plink, PHY_LINK_CONNECTED); + + return 0; }
On Tue, Mar 22, 2016 at 03:40:12PM +0100, msuraev@sysmocom.de wrote:
LOGP(DL1C, LOGL_NOTICE, "Ignoring phy link %d instance %d ""because no TRX associated with it\n", plink->num, pinst->num);
"no TRX is associated" ? ^^ oh well, I'm starting to split hairs again.
~Neels
Hi Max,
On Tue, Mar 22, 2016 at 03:40:12PM +0100, msuraev@sysmocom.de wrote:
Fixes: OS#1665
thanks, applied. Patches 3 and 4 out of the series remain up for discussion.
On Tue, Mar 22, 2016 at 03:40:08PM +0100, msuraev@sysmocom.de wrote:
+src/osmo-bts-litecell15/lc15bts-mgr +src/osmo-bts-litecell15/lc15bts-util +src/osmo-bts-litecell15/misc/.dirstamp +src/osmo-bts-litecell15/osmo-bts-lc15
thanks, applied.
-
Harald Welte -
Holger Freyther -
Max -
msuraev＠sysmocom.de
-
Neels Hofmeyr