* [PATCH 0/2] staging: vc04_services: Improve code readability @ 2017-03-04 20:00 Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 1/2] staging: vc04_services: Remove error message on kmalloc() failure Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 2/2] staging: vc04_services: Refactor conditionals Narcisa Ana Maria Vasile 0 siblings, 2 replies; 4+ messages in thread From: Narcisa Ana Maria Vasile @ 2017-03-04 20:00 UTC (permalink / raw) To: swarren, lee, eric, gregkh; +Cc: outreachy-kernel, Narcisa Ana Maria Vasile Remove redundant error message and refactor conditionals in order to improve code readability. Narcisa Ana Maria Vasile (2): staging: vc04_services: Remove error message on kmalloc() failure staging: vc04_services: Refactor conditionals .../vc04_services/interface/vchiq_arm/vchiq_core.c | 225 ++++++++++----------- 1 file changed, 109 insertions(+), 116 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] staging: vc04_services: Remove error message on kmalloc() failure 2017-03-04 20:00 [PATCH 0/2] staging: vc04_services: Improve code readability Narcisa Ana Maria Vasile @ 2017-03-04 20:02 ` Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 2/2] staging: vc04_services: Refactor conditionals Narcisa Ana Maria Vasile 1 sibling, 0 replies; 4+ messages in thread From: Narcisa Ana Maria Vasile @ 2017-03-04 20:02 UTC (permalink / raw) To: swarren, lee, eric, gregkh; +Cc: outreachy-kernel, Narcisa Ana Maria Vasile Remove 'Out of memory' message because kmalloc already prints a message in case of error. Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com> --- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 3 --- 1 file changed, 3 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..d9622e8 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c @@ -2606,9 +2606,6 @@ static const char *msg_type_str(unsigned int msg_type) 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) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] staging: vc04_services: Refactor conditionals 2017-03-04 20:00 [PATCH 0/2] staging: vc04_services: Improve code readability Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 1/2] staging: vc04_services: Remove error message on kmalloc() failure Narcisa Ana Maria Vasile @ 2017-03-04 20:02 ` Narcisa Ana Maria Vasile 2017-03-05 16:09 ` [Outreachy kernel] " Julia Lawall 1 sibling, 1 reply; 4+ messages in thread From: Narcisa Ana Maria Vasile @ 2017-03-04 20:02 UTC (permalink / raw) To: swarren, lee, eric, gregkh; +Cc: outreachy-kernel, Narcisa Ana Maria Vasile Refactor conditionals to reduce one level of indentation and improve code readability. Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com> --- .../vc04_services/interface/vchiq_arm/vchiq_core.c | 222 ++++++++++----------- 1 file changed, 109 insertions(+), 113 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 d9622e8..dc9f85c2 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c @@ -2574,129 +2574,125 @@ static const char *msg_type_str(unsigned int msg_type) VCHIQ_INSTANCE_T instance, VCHIQ_USERDATA_TERM_T userdata_term) { VCHIQ_SERVICE_T *service; + VCHIQ_SERVICE_T **pservice = NULL; + VCHIQ_SERVICE_QUOTA_T *service_quota; + int i; 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)); - } - - if (service) { - 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. - */ - - mutex_lock(&state->mutex); - - /* 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 (!srv) { - pservice = &state->services[i]; - break; - } + if (!service) + 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)); + + /* 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); + + /* 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 (!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); - } + 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH 2/2] staging: vc04_services: Refactor conditionals 2017-03-04 20:02 ` [PATCH 2/2] staging: vc04_services: Refactor conditionals Narcisa Ana Maria Vasile @ 2017-03-05 16:09 ` Julia Lawall 0 siblings, 0 replies; 4+ messages in thread From: Julia Lawall @ 2017-03-05 16:09 UTC (permalink / raw) To: Narcisa Ana Maria Vasile; +Cc: swarren, lee, eric, gregkh, outreachy-kernel On Sat, 4 Mar 2017, Narcisa Ana Maria Vasile wrote: > Refactor conditionals to reduce one level of indentation and improve > code readability. Indeed the normal situation is to use conditionals to use failure, not success. So this is an improvement in that respect as well. julia > > Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com> > --- > .../vc04_services/interface/vchiq_arm/vchiq_core.c | 222 ++++++++++----------- > 1 file changed, 109 insertions(+), 113 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 d9622e8..dc9f85c2 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c > @@ -2574,129 +2574,125 @@ static const char *msg_type_str(unsigned int msg_type) > VCHIQ_INSTANCE_T instance, VCHIQ_USERDATA_TERM_T userdata_term) > { > VCHIQ_SERVICE_T *service; > + VCHIQ_SERVICE_T **pservice = NULL; > + VCHIQ_SERVICE_QUOTA_T *service_quota; > + int i; > > 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)); > - } > - > - if (service) { > - 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. > - */ > - > - mutex_lock(&state->mutex); > - > - /* 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 (!srv) { > - pservice = &state->services[i]; > - break; > - } > + if (!service) > + 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)); > + > + /* 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); > + > + /* 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 (!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); > - } > + 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 > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/ba8881d259df889680ddc99f1a531bf198f42069.1488657258.git.narcisaanamaria12%40gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-05 16:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-04 20:00 [PATCH 0/2] staging: vc04_services: Improve code readability Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 1/2] staging: vc04_services: Remove error message on kmalloc() failure Narcisa Ana Maria Vasile 2017-03-04 20:02 ` [PATCH 2/2] staging: vc04_services: Refactor conditionals Narcisa Ana Maria Vasile 2017-03-05 16:09 ` [Outreachy kernel] " Julia Lawall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.