Attention is currently required from: neels, laforge, pespin.

dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31618 )

Change subject: pcu_sock: handle multiple BTSs with multiple BSC co-located PCUs ......................................................................

Patch Set 6:

(10 comments)

Commit Message:

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/6d9ced58_61673261 PS5, Line 15: RBS) are configured.

how about an osmo-bsc with one Ericsson RBS co-located, and then a bunch of other types of BTS with […]

It shouldn't disturb other (ip Access) BTSs. (see my comment above)

Patchset:

PS5:

pau, can you point out the place? i don't see it...

The commit message was not updated. It now should support multiple Ericsson BTS along with other BTSs. It doesn't break other BTSs since the we filter for Ericsson BTSs before we send any update to the PCU. We also filter when we activate/deactivate the PDCH on PCU reconnects.

File src/osmo-bsc/pcu_sock.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/a4f2103a_4a9892a7 PS5, Line 367: pcu_tx_info_ind(bts);

@neels: here pcu_tx_info_ind is inly called for ericsson bts now.

That is correct. We must not send any info indications to the PCU for BTSs that have a built in PCU. However in a follow up patch I will change this so that it only relies on pcu_connected() since BTSs with built in PCU must not be able to accept connections from a BSC co located PCU anyway.

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/53363abf_26e7aa92 PS5, Line 810: llist_for_each_entry(bts, &state->net->bts_list, list) {

could there be several RBS with several separate co-located PCUs, each with their own pcu_sock? This […]

(see comment below)

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/99071479_17e1c76c PS5, Line 960: rc

(would prefer if the return val were named 'fd', because 'man accept' says […]

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/333db815_734e64e8 PS5, Line 960:

(unusual whitespace after the type cast brace)

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/fe33bcfd_64f5eb23 PS5, Line 966: if (conn_bfd->fd >= 0) {

should this check happen before accept(), so we don't even accept a new connection when there alread […]

(see below)

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/802b5202_43cb366c PS5, Line 969: osmo_fd_read_disable(&state->listen_bfd);

so this disables the other active connection??

This is borrowed from osmo-bts. To me this looks like a bug. conn_bfd->fd should be closed instead so that the new connection replaces the old one. I have also fixed this in osmo-bts now.

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/2e7be381_9fd629a2 PS5, Line 986: llist_for_each_entry(bts, &state->net->bts_list, list) {

same question as above, pcu to bts relation is 1:1 or 1:N? I guess you should pass the bts pointer t […]

Thanks. This makes sense. I also found that the VTY code already allows one socket path per BTS but the status struct had only a pointer to net. Yes, we can just use a backpointer to the BTS in the status struct and we will have one PCU connection per BTS (1:1) and that also means we can run as many Ericsson BTSs we want in parallel, each one with its individual PCU.

https://gerrit.osmocom.org/c/osmo-bsc/+/31618/comment/fd69d95f_1fdba550 PS5, Line 1032: bts->pcu_state = state;

here it is pcu to bts relation 1:1?

Thats correct. (see comment above)