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

16 Feb 2023

Attention is currently required from: pespin, fixeria.
dexter 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 7:

(6 comments)

File src/osmobts_sock.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d16c7aca_559bdce4 
PS3, Line 113: 			if (the_pcu->phy.l1if_close_pdch && bts->trx[trx].fl1h) {
...
  not criticial but less confusing: I'd first check
for f1h and finally for l1if_close_pdch. […] (see my other comments.)

File src/osmobts_sock.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d1af27a2_7a83777c 
PS5, Line 115: 				bts->trx[trx].fl1h = NULL;
...
  After another looks, this looks even worse, because
you are not skipping setting fl1h to NULL if phy […] l1if_close_pdch exists when
the_pcu->phy_ops (see above). I think it should be ok.

File src/pcu_l1_if.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/f635e26f_3696ac8e 
PS5, Line 218: 		the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn,
arfcn, block_nr,
...
  guard this with "if
(the_pcu->phy_ops->l1if_pdch_req)". Similar for all other function calls.
It checks for the_pcu->phy_ops. That should be enough since all callbacks except
l1if_init_phy must be populated. I used the pointer to l1if_pdch_req to guard this before
but this is no longer required. On all other function I also only use the_pcu->phy_ops
now.

File src/pcu_l1_if_phy.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/4ece0901_e9a8accb 
PS5, Line 15: 	struct llist_head list;
...
  You can really put this inside ops too, and get rid of
l1if_phy_entry. When I do that then the ops can not be declared as static const
inside the module because the list head would have to writable. I have kept the
l1if_phy_entry list now but the only member is the list head and the pointer to ops.

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/06d76c9f_4bee4d97 
PS5, Line 18: 	const char *name;
...
  this can really be in li1f_phy_ops and assign the
pointer statically, there's no need to have it sep […] Done

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/ed00614f_720c8438 
PS5, Line 27: 	bool singleton;
...
  this too can be set in phy_ops. […] Done

-- 
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: 7
Gerrit-Owner: dexter &lt;pmaier(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Comment-Date: Thu, 16 Feb 2023 16:25:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-MessageType: comment