[PATCH 03/33] Add BCCH message to PH-/MPH-/TCH-SAP interface

Holger Hans Peter Freyther holger at freyther.de
Wed Mar 25 22:39:32 UTC 2015


On Wed, Aug 27, 2014 at 11:54:22PM +0200, Harald Welte wrote:

> +	/* don't free, because we forwarded data */
> +	return 1;

Maybe we can make that a define for the ownership handling
instead of a magic number.

> +static int ph_data_req(struct gsm_bts_trx *trx, struct msgb *msg,
> +		       struct osmo_phsap_prim *l1sap)
> +{
...

> +	} else {
> +		LOGP(DL1C, LOGL_NOTICE, "unknown prim %d op %d "
> +			"chan_nr %d link_id %d\n", l1sap->oph.primitive,
> +			l1sap->oph.operation, chan_nr, link_id);

		sure this should not happen in normal operation

		msgb_free(l1sap);

> +		return -EINVAL;
> +	}


> +		/* wrap zeroed l1p structure arrount payload

					around...

> +		 * this must be done in three steps, since the actual
> +		 * payload is not at the end but inside the l1p structure. */


maybe we can use the pull_to_l3 method hre?


> +
> +	/* send message to DSP's queue */
> +	osmo_wqueue_enqueue(&fl1->write_q[MQ_L1_WRITE], msg);


The existing code doesn't do this either but wqueue_enqueue is
written in a way that if the queue has reached the max depth the
message will not be added. This was done to let the caller decide
if it should clear the queue or wants to print the msgb. The result
is that the result of enqueue should be checked and if the queuing
was rejected msgb_free must be called.


> +int bts_model_l1sap_down(struct gsm_bts_trx *trx, struct osmo_phsap_prim *l1sap)
> +{
> +	struct msgb *msg = l1sap->oph.msg;
> +	int rc = 0;
> +
> +	switch (OSMO_PRIM_HDR(&l1sap->oph)) {
> +	case OSMO_PRIM(PRIM_PH_DATA, PRIM_OP_REQUEST):
> +		rc = ph_data_req(trx, msg, l1sap);
> +		break;
> +	default:
> +		LOGP(DL1C, LOGL_NOTICE, "unknown prim %d op %d\n",
> +			l1sap->oph.primitive, l1sap->oph.operation);
> +		rc = -EINVAL;
> +	}
> +
> +	if (rc)
> +		msgb_free(msg);


did the rc == 1 turn into a rc != 0 here?


> @@ -598,6 +740,7 @@ tx:
>  	/* transmit */
>  	osmo_wqueue_enqueue(&fl1->write_q[MQ_L1_WRITE], resp_msg);


the existing osmo_queue_enquire issue.

> @@ -952,8 +1095,8 @@ static int l1if_handle_ind(struct femtol1_hdl *fl1, struct msgb *msg)
>  	case GsmL1_PrimId_PhConnectInd:
>  		break;
>  	case GsmL1_PrimId_PhReadyToSendInd:
> -		rc = handle_ph_readytosend_ind(fl1, &l1p->u.phReadyToSendInd);
> -		break;
> +		return handle_ph_readytosend_ind(fl1, &l1p->u.phReadyToSendInd,
> +					       msg);

maybe it is better to split the switch into two cases to show the new
and old memory semantics?




More information about the OpenBSC mailing list