From: Denis Kenzior <denkenz@gmail.com>
To: Steve Schrock <steve.schrock@getcruise.com>, ofono@lists.linux.dev
Subject: Re: [PATCH 2/2] qmimodem: Use l_queue instead of GList for pending service creation
Date: Fri, 23 Feb 2024 10:20:57 -0600 [thread overview]
Message-ID: <d813fdc1-d528-4648-a92f-a06103c2afdb@gmail.com> (raw)
In-Reply-To: <20240222235356.32485-2-steve.schrock@getcruise.com>
Hi Steve,
On 2/22/24 17:53, Steve Schrock wrote:
> ---
> drivers/qmimodem/qmi.c | 43 ++++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 168dee1a..99b3d84d 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -1382,24 +1382,27 @@ static void service_create_shared_reply(struct l_idle *idle, void *user_data)
> __qmi_device_discovery_complete(data->device, &data->super);
> }
>
> +static void service_create_shared_schedule_callback(void *data, void *user_data)
> +{
> + struct service_create_shared_data *shared_data = data;
> + struct qmi_service *service = user_data;
> +
> + shared_data->service = qmi_service_ref(service);
> + shared_data->idle = l_idle_create(service_create_shared_reply,
> + shared_data, NULL);
> +
nit: empty line at end of function
> +}
> +
> static void service_create_shared_pending_reply(struct qmi_device *device,
> unsigned int type,
> struct qmi_service *service)
> {
> - gpointer key = L_UINT_TO_PTR(type | 0x80000000);
> - GList **shared = l_hashmap_remove(device->service_list, key);
> - GList *l;
> -
> - for (l = *shared; l; l = l->next) {
> - struct service_create_shared_data *shared_data = l->data;
> -
> - shared_data->service = qmi_service_ref(service);
> - shared_data->idle = l_idle_create(service_create_shared_reply,
> - shared_data, NULL);
> - }
> + void *key = L_UINT_TO_PTR(type | 0x80000000);
> + struct l_queue *shared = l_hashmap_remove(device->service_list, key);
>
> - g_list_free(*shared);
> - l_free(shared);
> + l_queue_foreach(shared, service_create_shared_schedule_callback,
> + service);
You could use l_queue_get_entries() here to avoid a foreach / creating a new
function if you think that will look better. Something like:
const struct l_queue_entry *entry;
for (entry = l_queue_get_entries(q); entry; entry = entry->next)
...
It looks like this function is always called from two places:
- qmux_client_create_reply()
This call site seems to be invoked on a timeout with a NULL service. Don't
think that makes sense? Should this invocation just be dropped?
- qmux_client_create_callback()
This call site happens when we receive a reply, which should be on a different
iteration of the event loop compared to qmi_device_qmux_client_create(). Can we
invoke the callbacks directly here instead of scheduling another l_idle callback?
> + l_queue_destroy(shared, NULL);
> }
>
> static void service_create_shared_data_free(gpointer user_data)
<snip>
> @@ -2148,7 +2151,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
> qmi_create_func_t func, void *user_data,
> qmi_destroy_func_t destroy)
> {
> - GList **l = NULL;
> + struct l_queue *shared = NULL;
nit: This doesn't need to be initialized. Let the compiler warn us in case
uninitiated variables are used.
> struct qmi_service *service = NULL;
> unsigned int type_val = type;
> int r;
Regards,
-Denis
next prev parent reply other threads:[~2024-02-23 16:20 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 23:53 [PATCH 1/2] qmimodem: Use l_queue instead of GList for notifications Steve Schrock
2024-02-22 23:53 ` [PATCH 2/2] qmimodem: Use l_queue instead of GList for pending service creation Steve Schrock
2024-02-23 16:20 ` Denis Kenzior [this message]
2024-02-23 16:10 ` [PATCH 1/2] qmimodem: Use l_queue instead of GList for notifications patchwork-bot+ofono
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d813fdc1-d528-4648-a92f-a06103c2afdb@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@lists.linux.dev \
--cc=steve.schrock@getcruise.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).