[PATCH] staging: vc04_services: Refactor multiple conditionals
Daniel Baluta
daniel.baluta at gmail.com
Fri Mar 3 16:09:47 EET 2017
On Fri, Mar 3, 2017 at 3:44 PM, Narcisa Ana Maria Vasile
<narcisaanamaria12 at gmail.com> wrote:
> Check whether kmalloc() call was successful and in case of failure
> throw an 'Out of memory' error and return NULL.
No need to throw 'Out of memory' message since kmalloc already does this.
So I would split your patch in two:
1/2 - remove 'Out of error message' because kmalloc already prints a
message in case of an error.
2/2 - refactor conditional to reduce one level of indentation.
Please remember that a commit message should point WHY a patch is need
and NOT WHAT the patch does.
'What' the patch does should be obvious from the code. Hence:
in 1/2 patch please mention that 'Out of memory' message was removed *because*
kmalloc already does that.
2/2 - refactoring is done to reduce one level of indentation and make
code easier to read.
So, redo this patch and send it to firefly.
thanks,
Daniel.
>
> If call was successful, there is no need to check the 'service' variable
> again. Therefor, multiple identical if conditionals were removed.
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12 at gmail.com>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 219 ++++++++++-----------
> 1 file changed, 108 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index ff0a1ff..5274862 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2576,130 +2576,127 @@ static const char *msg_type_str(unsigned int msg_type)
> VCHIQ_SERVICE_T *service;
>
> service = kmalloc(sizeof(VCHIQ_SERVICE_T), GFP_KERNEL);
> - if (service) {
> - service->base.fourcc = params->fourcc;
> - service->base.callback = params->callback;
> - service->base.userdata = params->userdata;
> - service->handle = VCHIQ_SERVICE_HANDLE_INVALID;
> - service->ref_count = 1;
> - service->srvstate = VCHIQ_SRVSTATE_FREE;
> - service->userdata_term = userdata_term;
> - service->localport = VCHIQ_PORT_FREE;
> - service->remoteport = VCHIQ_PORT_FREE;
> -
> - service->public_fourcc = (srvstate == VCHIQ_SRVSTATE_OPENING) ?
> - VCHIQ_FOURCC_INVALID : params->fourcc;
> - service->client_id = 0;
> - service->auto_close = 1;
> - service->sync = 0;
> - service->closing = 0;
> - service->trace = 0;
> - atomic_set(&service->poll_flags, 0);
> - service->version = params->version;
> - service->version_min = params->version_min;
> - service->state = state;
> - service->instance = instance;
> - service->service_use_count = 0;
> - init_bulk_queue(&service->bulk_tx);
> - init_bulk_queue(&service->bulk_rx);
> - sema_init(&service->remove_event, 0);
> - sema_init(&service->bulk_remove_event, 0);
> - mutex_init(&service->bulk_mutex);
> - memset(&service->stats, 0, sizeof(service->stats));
> - } else {
> - vchiq_log_error(vchiq_core_log_level,
> - "Out of memory");
> - }
> -
> - if (service) {
> - VCHIQ_SERVICE_T **pservice = NULL;
> - int i;
> + if (!service) {
> + vchiq_log_error(vchiq_core_log_level, "Out of memory");
> + return service;
> + }
> +
> + service->base.fourcc = params->fourcc;
> + service->base.callback = params->callback;
> + service->base.userdata = params->userdata;
> + service->handle = VCHIQ_SERVICE_HANDLE_INVALID;
> + service->ref_count = 1;
> + service->srvstate = VCHIQ_SRVSTATE_FREE;
> + service->userdata_term = userdata_term;
> + service->localport = VCHIQ_PORT_FREE;
> + service->remoteport = VCHIQ_PORT_FREE;
> +
> + service->public_fourcc = (srvstate == VCHIQ_SRVSTATE_OPENING) ?
> + VCHIQ_FOURCC_INVALID : params->fourcc;
> + service->client_id = 0;
> + service->auto_close = 1;
> + service->sync = 0;
> + service->closing = 0;
> + service->trace = 0;
> + atomic_set(&service->poll_flags, 0);
> + service->version = params->version;
> + service->version_min = params->version_min;
> + service->state = state;
> + service->instance = instance;
> + service->service_use_count = 0;
> + init_bulk_queue(&service->bulk_tx);
> + init_bulk_queue(&service->bulk_rx);
> + sema_init(&service->remove_event, 0);
> + sema_init(&service->bulk_remove_event, 0);
> + mutex_init(&service->bulk_mutex);
> + memset(&service->stats, 0, sizeof(service->stats));
> +
> + VCHIQ_SERVICE_T **pservice = NULL;
> + int i;
>
> - /* Although it is perfectly possible to use service_spinlock
> - ** to protect the creation of services, it is overkill as it
> - ** disables interrupts while the array is searched.
> - ** The only danger is of another thread trying to create a
> - ** service - service deletion is safe.
> - ** Therefore it is preferable to use state->mutex which,
> - ** although slower to claim, doesn't block interrupts while
> - ** it is held.
> - */
> + /* Although it is perfectly possible to use service_spinlock
> + ** to protect the creation of services, it is overkill as it
> + ** disables interrupts while the array is searched.
> + ** The only danger is of another thread trying to create a
> + ** service - service deletion is safe.
> + ** Therefore it is preferable to use state->mutex which,
> + ** although slower to claim, doesn't block interrupts while
> + ** it is held.
> + */
>
> - mutex_lock(&state->mutex);
> + mutex_lock(&state->mutex);
>
> - /* Prepare to use a previously unused service */
> - if (state->unused_service < VCHIQ_MAX_SERVICES)
> - pservice = &state->services[state->unused_service];
> + /* Prepare to use a previously unused service */
> + if (state->unused_service < VCHIQ_MAX_SERVICES)
> + pservice = &state->services[state->unused_service];
>
> - if (srvstate == VCHIQ_SRVSTATE_OPENING) {
> - for (i = 0; i < state->unused_service; i++) {
> - VCHIQ_SERVICE_T *srv = state->services[i];
> + if (srvstate == VCHIQ_SRVSTATE_OPENING) {
> + for (i = 0; i < state->unused_service; i++) {
> + VCHIQ_SERVICE_T *srv = state->services[i];
>
> - if (!srv) {
> - pservice = &state->services[i];
> - break;
> - }
> + if (!srv) {
> + pservice = &state->services[i];
> + break;
> }
> - } else {
> - for (i = (state->unused_service - 1); i >= 0; i--) {
> - VCHIQ_SERVICE_T *srv = state->services[i];
> -
> - if (!srv)
> - pservice = &state->services[i];
> - else if ((srv->public_fourcc == params->fourcc)
> - && ((srv->instance != instance) ||
> - (srv->base.callback !=
> - params->callback))) {
> - /* There is another server using this
> - ** fourcc which doesn't match. */
> - pservice = NULL;
> - break;
> - }
> + }
> + } else {
> + for (i = (state->unused_service - 1); i >= 0; i--) {
> + VCHIQ_SERVICE_T *srv = state->services[i];
> +
> + if (!srv)
> + pservice = &state->services[i];
> + else if ((srv->public_fourcc == params->fourcc)
> + && ((srv->instance != instance) ||
> + (srv->base.callback !=
> + params->callback))) {
> + /* There is another server using this
> + ** fourcc which doesn't match. */
> + pservice = NULL;
> + break;
> }
> }
> + }
>
> - if (pservice) {
> - service->localport = (pservice - state->services);
> - if (!handle_seq)
> - handle_seq = VCHIQ_MAX_STATES *
> - VCHIQ_MAX_SERVICES;
> - service->handle = handle_seq |
> - (state->id * VCHIQ_MAX_SERVICES) |
> - service->localport;
> - handle_seq += VCHIQ_MAX_STATES * VCHIQ_MAX_SERVICES;
> - *pservice = service;
> - if (pservice == &state->services[state->unused_service])
> - state->unused_service++;
> - }
> + if (pservice) {
> + service->localport = (pservice - state->services);
> + if (!handle_seq)
> + handle_seq = VCHIQ_MAX_STATES *
> + VCHIQ_MAX_SERVICES;
> + service->handle = handle_seq |
> + (state->id * VCHIQ_MAX_SERVICES) |
> + service->localport;
> + handle_seq += VCHIQ_MAX_STATES * VCHIQ_MAX_SERVICES;
> + *pservice = service;
> + if (pservice == &state->services[state->unused_service])
> + state->unused_service++;
> + }
>
> - mutex_unlock(&state->mutex);
> + mutex_unlock(&state->mutex);
>
> - if (!pservice) {
> - kfree(service);
> - service = NULL;
> - }
> + if (!pservice) {
> + kfree(service);
> + service = NULL;
> }
>
> - if (service) {
> - VCHIQ_SERVICE_QUOTA_T *service_quota =
> - &state->service_quotas[service->localport];
> - service_quota->slot_quota = state->default_slot_quota;
> - service_quota->message_quota = state->default_message_quota;
> - if (service_quota->slot_use_count == 0)
> - service_quota->previous_tx_index =
> - SLOT_QUEUE_INDEX_FROM_POS(state->local_tx_pos)
> - - 1;
> -
> - /* Bring this service online */
> - vchiq_set_service_state(service, srvstate);
> -
> - vchiq_log_info(vchiq_core_msg_log_level,
> - "%s Service %c%c%c%c SrcPort:%d",
> - (srvstate == VCHIQ_SRVSTATE_OPENING)
> - ? "Open" : "Add",
> - VCHIQ_FOURCC_AS_4CHARS(params->fourcc),
> - service->localport);
> - }
> + VCHIQ_SERVICE_QUOTA_T *service_quota =
> + &state->service_quotas[service->localport];
> + service_quota->slot_quota = state->default_slot_quota;
> + service_quota->message_quota = state->default_message_quota;
> + if (service_quota->slot_use_count == 0)
> + service_quota->previous_tx_index =
> + SLOT_QUEUE_INDEX_FROM_POS(state->local_tx_pos)
> + - 1;
> +
> + /* Bring this service online */
> + vchiq_set_service_state(service, srvstate);
> +
> + vchiq_log_info(vchiq_core_msg_log_level,
> + "%s Service %c%c%c%c SrcPort:%d",
> + (srvstate == VCHIQ_SRVSTATE_OPENING)
> + ? "Open" : "Add",
> + VCHIQ_FOURCC_AS_4CHARS(params->fourcc),
> + service->localport);
> +
>
> /* Don't unlock the service - leave it with a ref_count of 1. */
>
> --
> 1.9.1
>
More information about the firefly
mailing list