| Message ID | 20240610210220.95524-8-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: vc04_services: Random cleanups | expand |
On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: > The service_callback has 5 levels of indentation, which makes it > hard to read. Reduce this by splitting the code in a new function > service_single_message() as suggested by Laurent Pinchart. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > .../interface/vchiq_arm/vchiq_arm.c | 68 +++++++++++-------- > 1 file changed, 39 insertions(+), 29 deletions(-) > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > index 0ba52c9d8bc3..706bfc7a0b90 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason, > return 0; > } > > +static int > +service_single_message(struct vchiq_instance *instance, > + enum vchiq_reason reason, > + struct vchiq_service *service, void *bulk_userdata) > +{ > + struct user_service *user_service; > + > + user_service = (struct user_service *)service->base.userdata; > + > + dev_dbg(service->state->dev, "arm: msg queue full\n"); > + /* > + * If there is no MESSAGE_AVAILABLE in the completion > + * queue, add one > + */ > + if ((user_service->message_available_pos - > + instance->completion_remove) < 0) { > + dev_dbg(instance->state->dev, > + "arm: Inserting extra MESSAGE_AVAILABLE\n"); > + return add_completion(instance, reason, NULL, user_service, > + bulk_userdata); In the original code we added a completion and > + } > + > + if (wait_for_completion_interruptible(&user_service->remove_event)) { then waited for it here... Why did this change? regards, dan carpenter > + dev_dbg(instance->state->dev, "arm: interrupted\n"); > + return -EAGAIN; > + } else if (instance->closing) { > + dev_dbg(instance->state->dev, "arm: closing\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > int > service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, > struct vchiq_header *header, unsigned int handle, void *bulk_userdata) > @@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, > spin_lock(&service->state->msg_queue_spinlock); > while (user_service->msg_insert == > (user_service->msg_remove + MSG_QUEUE_SIZE)) { > + int ret; > + > spin_unlock(&service->state->msg_queue_spinlock); > DEBUG_TRACE(SERVICE_CALLBACK_LINE); > DEBUG_COUNT(MSG_QUEUE_FULL_COUNT); > - dev_dbg(service->state->dev, "arm: msg queue full\n"); > - /* > - * If there is no MESSAGE_AVAILABLE in the completion > - * queue, add one > - */ > - if ((user_service->message_available_pos - > - instance->completion_remove) < 0) { > - int ret; > - > - dev_dbg(instance->state->dev, > - "arm: Inserting extra MESSAGE_AVAILABLE\n"); > - DEBUG_TRACE(SERVICE_CALLBACK_LINE); > - ret = add_completion(instance, reason, NULL, user_service, > - bulk_userdata); > - if (ret) { > - DEBUG_TRACE(SERVICE_CALLBACK_LINE); > - vchiq_service_put(service); > - return ret; > - } > - } > > - DEBUG_TRACE(SERVICE_CALLBACK_LINE); > - if (wait_for_completion_interruptible(&user_service->remove_event)) { > - dev_dbg(instance->state->dev, "arm: interrupted\n"); > - DEBUG_TRACE(SERVICE_CALLBACK_LINE); > - vchiq_service_put(service); > - return -EAGAIN; > - } else if (instance->closing) { > - dev_dbg(instance->state->dev, "arm: closing\n"); > + ret = service_single_message(instance, reason, > + service, bulk_userdata); > + if (ret) { > DEBUG_TRACE(SERVICE_CALLBACK_LINE); > vchiq_service_put(service); > - return -EINVAL; > + return ret; > } > DEBUG_TRACE(SERVICE_CALLBACK_LINE); > spin_lock(&service->state->msg_queue_spinlock); > -- > 2.34.1 >
On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: > The service_callback has 5 levels of indentation, which makes it > hard to read. Reduce this by splitting the code in a new function > service_single_message() as suggested by Laurent Pinchart. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> To be honest, I liked the version of this patch which Laurent didn't like. Even after pulling this code into a separate function, I'd still support flipping the conditions around and adding the goto... Laurent, you really didn't like the goto? regards, dan carpenter
Hi Dan, Am 11.06.24 um 08:01 schrieb Dan Carpenter: > On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: >> The service_callback has 5 levels of indentation, which makes it >> hard to read. Reduce this by splitting the code in a new function >> service_single_message() as suggested by Laurent Pinchart. >> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> .../interface/vchiq_arm/vchiq_arm.c | 68 +++++++++++-------- >> 1 file changed, 39 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> index 0ba52c9d8bc3..706bfc7a0b90 100644 >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> @@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason, >> return 0; >> } >> >> +static int >> +service_single_message(struct vchiq_instance *instance, >> + enum vchiq_reason reason, >> + struct vchiq_service *service, void *bulk_userdata) >> +{ >> + struct user_service *user_service; >> + >> + user_service = (struct user_service *)service->base.userdata; >> + >> + dev_dbg(service->state->dev, "arm: msg queue full\n"); >> + /* >> + * If there is no MESSAGE_AVAILABLE in the completion >> + * queue, add one >> + */ >> + if ((user_service->message_available_pos - >> + instance->completion_remove) < 0) { >> + dev_dbg(instance->state->dev, >> + "arm: Inserting extra MESSAGE_AVAILABLE\n"); >> + return add_completion(instance, reason, NULL, user_service, >> + bulk_userdata); > In the original code we added a completion and > >> + } >> + >> + if (wait_for_completion_interruptible(&user_service->remove_event)) { > then waited for it here... Why did this change? Oops, this wasn't intended. Thanks for catching this. > > regards, > dan carpenter > >> + dev_dbg(instance->state->dev, "arm: interrupted\n"); >> + return -EAGAIN; >> + } else if (instance->closing) { >> + dev_dbg(instance->state->dev, "arm: closing\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> int >> service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, >> struct vchiq_header *header, unsigned int handle, void *bulk_userdata) >> @@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, >> spin_lock(&service->state->msg_queue_spinlock); >> while (user_service->msg_insert == >> (user_service->msg_remove + MSG_QUEUE_SIZE)) { >> + int ret; >> + >> spin_unlock(&service->state->msg_queue_spinlock); >> DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> DEBUG_COUNT(MSG_QUEUE_FULL_COUNT); >> - dev_dbg(service->state->dev, "arm: msg queue full\n"); >> - /* >> - * If there is no MESSAGE_AVAILABLE in the completion >> - * queue, add one >> - */ >> - if ((user_service->message_available_pos - >> - instance->completion_remove) < 0) { >> - int ret; >> - >> - dev_dbg(instance->state->dev, >> - "arm: Inserting extra MESSAGE_AVAILABLE\n"); >> - DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> - ret = add_completion(instance, reason, NULL, user_service, >> - bulk_userdata); >> - if (ret) { >> - DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> - vchiq_service_put(service); >> - return ret; >> - } >> - } >> >> - DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> - if (wait_for_completion_interruptible(&user_service->remove_event)) { >> - dev_dbg(instance->state->dev, "arm: interrupted\n"); >> - DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> - vchiq_service_put(service); >> - return -EAGAIN; >> - } else if (instance->closing) { >> - dev_dbg(instance->state->dev, "arm: closing\n"); >> + ret = service_single_message(instance, reason, >> + service, bulk_userdata); >> + if (ret) { >> DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> vchiq_service_put(service); >> - return -EINVAL; >> + return ret; >> } >> DEBUG_TRACE(SERVICE_CALLBACK_LINE); >> spin_lock(&service->state->msg_queue_spinlock); >> -- >> 2.34.1 >>
On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote: > On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: > > The service_callback has 5 levels of indentation, which makes it > > hard to read. Reduce this by splitting the code in a new function > > service_single_message() as suggested by Laurent Pinchart. > > > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > > To be honest, I liked the version of this patch which Laurent didn't > like. Even after pulling this code into a separate function, I'd still > support flipping the conditions around and adding the goto... > > Laurent, you really didn't like the goto? I won't nack it, but I really think gotos seriously hinder readability, with the exception of error paths.
Hi, Am 11.06.24 um 23:23 schrieb Laurent Pinchart: > On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote: >> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: >>> The service_callback has 5 levels of indentation, which makes it >>> hard to read. Reduce this by splitting the code in a new function >>> service_single_message() as suggested by Laurent Pinchart. >>> >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> To be honest, I liked the version of this patch which Laurent didn't >> like. Even after pulling this code into a separate function, I'd still >> support flipping the conditions around and adding the goto... >> >> Laurent, you really didn't like the goto? > I won't nack it, but I really think gotos seriously hinder readability, > with the exception of error paths. > so at least you both are fine with the split approach here (assuming the Dan's comment will be fixed in V3)?
On Wed, Jun 12, 2024 at 12:23:47AM +0300, Laurent Pinchart wrote: > On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote: > > On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: > > > The service_callback has 5 levels of indentation, which makes it > > > hard to read. Reduce this by splitting the code in a new function > > > service_single_message() as suggested by Laurent Pinchart. > > > > > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > > > > To be honest, I liked the version of this patch which Laurent didn't > > like. Even after pulling this code into a separate function, I'd still > > support flipping the conditions around and adding the goto... > > > > Laurent, you really didn't like the goto? > > I won't nack it, but I really think gotos seriously hinder readability, > with the exception of error paths. Hm... I guess I only hate backwards gotos (ones that go up instead of down). To me this one is kind of like a goto unlock. It's slightly more complicated than just an unlock because we add a completion as well... I don't have strong feelings about it, I was just interested. regards, dan carpenter
On Wed, Jun 12, 2024 at 07:16:16AM +0200, Stefan Wahren wrote: > Hi, > > Am 11.06.24 um 23:23 schrieb Laurent Pinchart: > > On Tue, Jun 11, 2024 at 09:04:28AM +0300, Dan Carpenter wrote: > >> On Mon, Jun 10, 2024 at 11:02:17PM +0200, Stefan Wahren wrote: > >>> The service_callback has 5 levels of indentation, which makes it > >>> hard to read. Reduce this by splitting the code in a new function > >>> service_single_message() as suggested by Laurent Pinchart. > >>> > >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > >> To be honest, I liked the version of this patch which Laurent didn't > >> like. Even after pulling this code into a separate function, I'd still > >> support flipping the conditions around and adding the goto... > >> > >> Laurent, you really didn't like the goto? > > I won't nack it, but I really think gotos seriously hinder readability, > > with the exception of error paths. > > > so at least you both are fine with the split approach here (assuming the > Dan's comment will be fixed in V3)? Fine with me.
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c index 0ba52c9d8bc3..706bfc7a0b90 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -1055,6 +1055,39 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason, return 0; } +static int +service_single_message(struct vchiq_instance *instance, + enum vchiq_reason reason, + struct vchiq_service *service, void *bulk_userdata) +{ + struct user_service *user_service; + + user_service = (struct user_service *)service->base.userdata; + + dev_dbg(service->state->dev, "arm: msg queue full\n"); + /* + * If there is no MESSAGE_AVAILABLE in the completion + * queue, add one + */ + if ((user_service->message_available_pos - + instance->completion_remove) < 0) { + dev_dbg(instance->state->dev, + "arm: Inserting extra MESSAGE_AVAILABLE\n"); + return add_completion(instance, reason, NULL, user_service, + bulk_userdata); + } + + if (wait_for_completion_interruptible(&user_service->remove_event)) { + dev_dbg(instance->state->dev, "arm: interrupted\n"); + return -EAGAIN; + } else if (instance->closing) { + dev_dbg(instance->state->dev, "arm: closing\n"); + return -EINVAL; + } + + return 0; +} + int service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, struct vchiq_header *header, unsigned int handle, void *bulk_userdata) @@ -1104,41 +1137,18 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason, spin_lock(&service->state->msg_queue_spinlock); while (user_service->msg_insert == (user_service->msg_remove + MSG_QUEUE_SIZE)) { + int ret; + spin_unlock(&service->state->msg_queue_spinlock); DEBUG_TRACE(SERVICE_CALLBACK_LINE); DEBUG_COUNT(MSG_QUEUE_FULL_COUNT); - dev_dbg(service->state->dev, "arm: msg queue full\n"); - /* - * If there is no MESSAGE_AVAILABLE in the completion - * queue, add one - */ - if ((user_service->message_available_pos - - instance->completion_remove) < 0) { - int ret; - - dev_dbg(instance->state->dev, - "arm: Inserting extra MESSAGE_AVAILABLE\n"); - DEBUG_TRACE(SERVICE_CALLBACK_LINE); - ret = add_completion(instance, reason, NULL, user_service, - bulk_userdata); - if (ret) { - DEBUG_TRACE(SERVICE_CALLBACK_LINE); - vchiq_service_put(service); - return ret; - } - } - DEBUG_TRACE(SERVICE_CALLBACK_LINE); - if (wait_for_completion_interruptible(&user_service->remove_event)) { - dev_dbg(instance->state->dev, "arm: interrupted\n"); - DEBUG_TRACE(SERVICE_CALLBACK_LINE); - vchiq_service_put(service); - return -EAGAIN; - } else if (instance->closing) { - dev_dbg(instance->state->dev, "arm: closing\n"); + ret = service_single_message(instance, reason, + service, bulk_userdata); + if (ret) { DEBUG_TRACE(SERVICE_CALLBACK_LINE); vchiq_service_put(service); - return -EINVAL; + return ret; } DEBUG_TRACE(SERVICE_CALLBACK_LINE); spin_lock(&service->state->msg_queue_spinlock);
The service_callback has 5 levels of indentation, which makes it hard to read. Reduce this by splitting the code in a new function service_single_message() as suggested by Laurent Pinchart. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- .../interface/vchiq_arm/vchiq_arm.c | 68 +++++++++++-------- 1 file changed, 39 insertions(+), 29 deletions(-) -- 2.34.1