[osmo-bts,3/3] main: Added support for changing the max_power_red in sbts2050

From: Álvaro Neira Ayuso <anayuso@sysmocom.de>

Added functions for changing the max_power_red in case of we receive a Failure
Event report from the manager. The sbts2050 change the max_power_red to the value
that we have configured in the manager and the sbts2050 restore this value to the
initial value if the bts don't receive another Failure Event report in the same time
that we check the temperature in the manager.

Signed-off-by: Alvaro Neira Ayuso <anayuso@sysmocom.de>
---
 src/osmo-bts-sysmo/main.c |   67 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

On Tue, Apr 08, 2014 at 02:40:35PM +0200, Alvaro Neira Ayuso wrote:
> From: Álvaro Neira Ayuso <anayuso@sysmocom.de>
> 
> Added functions for changing the max_power_red in case of we receive a Failure
> Event report from the manager. The sbts2050 change the max_power_red to the value
> that we have configured in the manager and the sbts2050 restore this value to the
> initial value if the bts don't receive another Failure Event report in the same time
> that we check the temperature in the manager.
> 
> Signed-off-by: Alvaro Neira Ayuso <anayuso@sysmocom.de>
> ---
>  src/osmo-bts-sysmo/main.c |   67 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/src/osmo-bts-sysmo/main.c b/src/osmo-bts-sysmo/main.c
> index 61a5716..b492327 100644
> --- a/src/osmo-bts-sysmo/main.c
> +++ b/src/osmo-bts-sysmo/main.c
> @@ -39,6 +39,7 @@
>  #include <osmocom/vty/telnet_interface.h>
>  #include <osmocom/vty/logging.h>
>  #include <osmocom/gsm/protocol/ipaccess.h>
> +#include <osmocom/gsm/abis_nm.h>
>  
>  #include <osmo-bts/gsm_data.h>
>  #include <osmo-bts/logging.h>
> @@ -55,11 +56,14 @@
>  #define IPA_HEADER_SIZE		3
>  #define IPA_OML_PROTO		0xFF
>  
> +#define TEMP_TIMER_SECS		(6 * 3600) + 1
> +
>  #include "utils.h"
>  #include "eeprom.h"
>  #include "l1_if.h"
>  #include "hw_misc.h"
>  #include "btsconfig.h"
> +#include <misc/sysmobts_misc.h>
>  
>  /* FIXME: read from real hardware */
>  const uint8_t abis_mac[6] = { 0,1,2,3,4,5 };
> @@ -297,6 +301,60 @@ static int write_pid_file(char *procname)
>  	return 0;
>  }
>  #ifdef BUILD_SBTS2050
> +static int init_max_power_red;
> +static int new_max_power_red;
> +static int status_change_power_red = 0;
> +
> +enum {
> +	SBTS2050_DISABLE_CHANGE_POWER = 0,
> +	SBTS2050_ENABLE_CHANGE_POWER,
> +};
> +
> +#define oml_tlv_parse(dec, buf, len)	\
> +			tlv_parse(dec, &abis_nm_att_tlvdef, buf, len, 0, 0)
> +
> +/* Size of the struct plus 1 Bytes of tag and 2 Bytes for the length */
> +#define tlv_add_info_len()     (sizeof(struct sbts2050_config_info) + 3)
                           ^^

Better?

#define TLV_INFO_LEN    (sizeof(struct sbts2050_config_info) + 3)

The remaning part looks good to me.

Thanks Alvaro.

> +
> +static struct osmo_timer_list power_timer;
> +static void change_max_power_red_cb(void *data)
> +{
> +	struct gsm_bts *bts = data;
> +
> +	if (status_change_power_red == SBTS2050_ENABLE_CHANGE_POWER) {
> +		if (bts->c0->max_power_red != new_max_power_red) {
> +			init_max_power_red = bts->c0->max_power_red;
> +			bts->c0->max_power_red = new_max_power_red;
> +		}
> +		status_change_power_red = SBTS2050_DISABLE_CHANGE_POWER;
> +		osmo_timer_schedule(&power_timer, TEMP_TIMER_SECS, 0);
> +	} else if (status_change_power_red == SBTS2050_DISABLE_CHANGE_POWER)
> +		bts->c0->max_power_red = init_max_power_red;
> +}
> +
> +static int take_max_power_red(struct msgb *msg)
> +{
> +	struct sbts2050_config_info *config_info;
> +	struct tlv_parsed tlv_out;
> +	int rc;
> +
> +	rc = oml_tlv_parse(&tlv_out, msg->tail - tlv_add_info_len(),
> +			   tlv_add_info_len());
> +
> +	if (rc < 0)
> +		return -1;
> +
> +	config_info =
> +	    (struct sbts2050_config_info *) TLVP_VAL(&tlv_out, NM_ATT_ADD_INFO);
> +
> +	new_max_power_red = config_info->max_power_red;
> +
> +	status_change_power_red = SBTS2050_ENABLE_CHANGE_POWER;
> +	change_max_power_red_cb(bts);
> +
> +	return 0;
> +}
> +
>  static int test_recv_msg(struct msgb *msg)
>  {
>  	struct ipaccess_head *hh = (struct ipaccess_head *)msg->data;
> @@ -355,6 +413,11 @@ static int read_sock(struct osmo_fd *fd, unsigned int what)
>  		return -1;
>  	}
>  
> +	if (take_max_power_red(msg) < 0) {
> +		LOGP(DL1C, LOGL_NOTICE, "Failed Add_info: Malformed message");
> +		return -1;
> +	}
> +
>  	mo = &bts->mo;
>  	msg->trx = mo->bts->c0;
>  
> @@ -478,6 +541,10 @@ int main(int argc, char **argv)
>  	read_fd.fd = -1;
>  	accept_fd.data = &read_fd;
>  
> +	init_max_power_red = bts->c0->max_power_red;
> +	power_timer.cb = change_max_power_red_cb;
> +	power_timer.data = bts;
> +
>  	rc = osmo_sock_unix_init_ofd(&accept_fd, SOCK_SEQPACKET, 0, SOCKET_PATH,
>  				     OSMO_SOCK_F_BIND | OSMO_SOCK_F_NONBLOCK);
>  	if (rc < 0) {
> -- 
> 1.7.10.4
> 
>

Hi Alvaro,


On Tue, Apr 08, 2014 at 02:40:35PM +0200, Alvaro Neira Ayuso wrote:
> +#define oml_tlv_parse(dec, buf, len)	\
> +			tlv_parse(dec, &abis_nm_att_tlvdef, buf, len, 0, 0)

by doing so, you are using abis_nm_att_tlvdef, which is defining 0x01 as
NM_ATT_ABIS_CHANNEL, which is a fixed-size attribute of 3 bytes length.
That's quite something else than what you need, isn't it?

I've introduced abis_nm_att_osmo_tlvdef[] in libosmocore.  Please either
use tlv_def_patch() to merge abis_nm_att_osmo_tlvdef with
abis_nm_att_tlvdef before calling tlv_parse from generic code.  Or, if
you are sure that only a org.osmocom specific message can arrive, you
may use abis_nm_att_osmo_tlvdef directly.

> +static int take_reduce_power(struct msgb *msg)

this function takes the msgb pointer, even though from where it is
called in read_sock():

+       fom = (struct abis_om_fom_hdr *) msg->l3h;
 
+       switch (fom->msg_type) {
+       case NM_MT_SET_RADIO_ATTR:
+               rc = take_reduce_power(msg);

we already had computed the exact location of 'fom'.

Now in take_reduce_power, you do the following:

+static int take_reduce_power(struct msgb *msg)
+{
+       int recv_reduce_power;
+       struct tlv_parsed tlv_out;
+       struct gsm_bts_trx *trx = bts->c0;
+       int rc, abis_oml_hdr_len;
+
+       abis_oml_hdr_len = sizeof(struct abis_om_hdr);
+       abis_oml_hdr_len += sizeof(struct abis_om_fom_hdr);
+       abis_oml_hdr_len += sizeof(osmocom_magic) + 1;

so you basically make blind assumptions that there is na osmocom_magic
and its associated length.

However, the check_oml_msg() that you wrote accepts both the
com.ipaccess and the org.osmocom magic values.  So

a) a com.ipaccess message might end up in take_reduce_power()
b) your static assumption that an osmocom magic being present is not
   true.

Please properly think about such logic.

It might be best if the check_oml_message or similar would return some
information (or even store it in the msgb_cb() somewhere) which of the
vendor specific messages (if any) it has detected.  Later code could
then base its processing on that distinction.

Also, as you had already resolved the fom pointer (from msg->l3,), thre
is no point in starting to calculate abis_oml_hdr_len again (and
wrongly).

Finally, regarding check_oml_msg(): It actually doedn't only check an
OML message, but it checks also the IPA headre in front.  Please create
one function for one purpose.   The check_oml_msg() shouldn't care about
the ipaccess_head in front.  If we make it more generic, we can move it
to libosmocore as a general OML header sanity checking function.  But
that's not possible right now as it depends on an ipaccess_head, which
is not present in E1 based OML.

Also,
+int add_manufacturer_id_label(struct msgb *msg, int manuf_type_id)

why use an int there?  You have defined an 'enum manuf_type_id', please
use it :)

Regards,
	Harald