Message ID | Y05TNQBXLMJMgQ2r@debian-BULLSEYE-live-builder-AMD64 |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp1818678wrs; Tue, 18 Oct 2022 00:21:33 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6Kj8yurgzy5ACbV+AG9baKHsNwSNnd6keEeniJJI9iIrfWTkh/AdQVHLmhU8kEyPQwyl6M X-Received: by 2002:a17:902:d34a:b0:17f:9be0:cdf9 with SMTP id l10-20020a170902d34a00b0017f9be0cdf9mr1831239plk.132.1666077693165; Tue, 18 Oct 2022 00:21:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666077693; cv=none; d=google.com; s=arc-20160816; b=GUVOd/NMcvkxUuVoAZEIL3/yeShNyxY9hwk/WRSdWVIBeVEXTV4yKCkOFShTYKMhCr ywiaLshq5Lhlt1+XKsFoltXDeW4FSJ00G+imbg4/hvFqz9qgL9GrVkqcHc12K2mBKtLE dOmOZAof+Xv9lI+zMTww75Ae7XNNA3qfEx4JdB+1cYmBdZdZrUzZ+lwShPViWFiIETEy Kk0kSCf1BOjC8PIGlmoJUW0mZLI9fsJ9tXjf7pHv493fbrOY4Jh2rQxs4Zi2G+junUq4 1bo14vgy7mFoZr0GpRMmiTX/R+rovrqZ1bjQ81ci7deKSg1hJCaK/f5mOyMSRy4lbt40 ZwAw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=o7xC57TGbSfZkCo5O788+ZGIF5zblK0vzmBxTmBFQSE=; b=savWeADzOqZjdFOuNN/URqxBPQsFEEc6Jv6iFMQDtIdcXLysLkM3uej/h6EzZlKwuI nhwNHZ0XeEEumJq6XNa/msfoa/nLH1cyIP5/ywq2ob8zdZhynejtue1CLuW7YsodraL3 g4jqgDLukkUPTGMnJhb7Q31gbRNHWgAfk6rLF2OykCck+ZJivbSzxE2RQLLREjSv/lSa 6oiLcGL0zHYrtTuB7oJ0Rg4w8eAt7woqwg0suYFqwl6H4ZDg9iaW7qSHuWcG5Vi04jqb ZvAZ26vJgkRUtEk4vdnlJ6IKX+EyRC7JgAZLTqvwW2ZdfgpH/lZnxXb26znM5kVyxca5 jbPA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=DBnQxKQI; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 185-20020a6302c2000000b004573e877892si14211305pgc.769.2022.10.18.00.21.19; Tue, 18 Oct 2022 00:21:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=DBnQxKQI; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230149AbiJRHS3 (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Tue, 18 Oct 2022 03:18:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54176 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229470AbiJRHS1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Oct 2022 03:18:27 -0400 Received: from msg-1.mailo.com (msg-1.mailo.com [213.182.54.11]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0C32DFD4 for <linux-kernel@vger.kernel.org>; Tue, 18 Oct 2022 00:18:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1666077497; bh=JY+F/SHTji7yOJKf7kIwhuyuxqxS/UIyEF/eUKTINHY=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:MIME-Version: Content-Type; b=DBnQxKQIF3BAtTgwgrxXyyez6l9qB4wCoX4YFetoL+wgJLJP6Hu4ZH8d8HCmIPqQJ sJf048JxLgUr78H6dbFjTmJkCEDRxsymNaxvYEP+sAukWZj9Nv9EJNrzARTM8bZTuF lGR1EcyoVbgKkogRl7MFcPJdUoaNpwQxJqxfBH04= Received: by b-1.in.mailobj.net [192.168.90.11] with ESMTP via [213.182.55.206] Tue, 18 Oct 2022 09:18:17 +0200 (CEST) X-EA-Auth: 6rT+wPigKGh1Mdrwk3Lv2jTuS8zK1Zg7IHlGJdXHmCgmvhAy30enuoasUdm5RTgSwa8XSukxbrGq2Pq7eDT29cTRTzcK23u0 Date: Tue, 18 Oct 2022 12:48:13 +0530 From: Deepak R Varma <drv@mailo.com> To: outreachy@lists.linux.dev, gregkh@linuxfoundation.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Cc: kumarpraveen@linux.microsoft.com, saurabh.truth@gmail.com Subject: [PATCH v2] staging: most: dim2: read done_buffers count locally from HDM channel Message-ID: <Y05TNQBXLMJMgQ2r@debian-BULLSEYE-live-builder-AMD64> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1746969788746668408?= X-GMAIL-MSGID: =?utf-8?q?1747009083087831122?= |
Series |
[v2] staging: most: dim2: read done_buffers count locally from HDM channel
|
|
Commit Message
Deepak R Varma
Oct. 18, 2022, 7:18 a.m. UTC
The done_buffer count is already available in the hdm_channel struct.
Calling dim_get_channel_state function to source this value out of
the same structure is unnecessary.
Further, the second parameter struct dim_ch_state_t to this function
is filled by using the hdm_channel inside the function. This filled in
variable is never used in the caller and can be altogether removed.
So, a call to dim_get_channel_state function in this context also
deems expensive.
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Changes in v2:
1. Update patch log message to be more descriptive about the reason for change.
Feedback provided by julia.lawall@inria.fr
PLEASE NOTE: I have only built the module on my machine, but have not tested it.
I am not sure how to test this change. I am willing to test it with appropriate
guidance provided I have the necessary hardware.
drivers/staging/most/dim2/dim2.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--
2.30.2
Comments
On Tue, 18 Oct 2022, Deepak R Varma wrote: > The done_buffer count is already available in the hdm_channel struct. > Calling dim_get_channel_state function to source this value out of > the same structure is unnecessary. > Further, the second parameter struct dim_ch_state_t to this function > is filled by using the hdm_channel inside the function. This filled in > variable is never used in the caller and can be altogether removed. > So, a call to dim_get_channel_state function in this context also > deems expensive. Thanks for the rewrite. I find "source this value out of" hard to understand. I would have written something like the following: The function dim_get_channel_state only serves to initialize the ready and done_buffers fields of the structure passed as its second argument. In service_done_flag, this structure is never used again and the only purpose of the call is to get the value that is put in the done_buffers field. But that value is just the done_sw_buffers_number field of the call's first argument. So the whole call is useless, and we can just replace it with an access to this field. This change implies that the variable st is no longer used, so drop it as well. --- If you want to work some more in this code, the name of the type dim_ch_state_t should not have an _t at the end of it. It looks like it used to be a typedef and someone eliminated the typedef but didn't drop the _t. julia > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > Changes in v2: > 1. Update patch log message to be more descriptive about the reason for change. > Feedback provided by julia.lawall@inria.fr > > PLEASE NOTE: I have only built the module on my machine, but have not tested it. > I am not sure how to test this change. I am willing to test it with appropriate > guidance provided I have the necessary hardware. > > > drivers/staging/most/dim2/dim2.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > index ab72e11ac5ab..4c1f27898a29 100644 > --- a/drivers/staging/most/dim2/dim2.c > +++ b/drivers/staging/most/dim2/dim2.c > @@ -259,7 +259,6 @@ static void retrieve_netinfo(struct dim2_hdm *dev, struct mbo *mbo) > static void service_done_flag(struct dim2_hdm *dev, int ch_idx) > { > struct hdm_channel *hdm_ch = dev->hch + ch_idx; > - struct dim_ch_state_t st; > struct list_head *head; > struct mbo *mbo; > int done_buffers; > @@ -271,7 +270,7 @@ static void service_done_flag(struct dim2_hdm *dev, int ch_idx) > > spin_lock_irqsave(&dim_lock, flags); > > - done_buffers = dim_get_channel_state(&hdm_ch->ch, &st)->done_buffers; > + done_buffers = hdm_ch->ch.done_sw_buffers_number; > if (!done_buffers) { > spin_unlock_irqrestore(&dim_lock, flags); > return; > -- > 2.30.2 > > > > >
On Tue, Oct 18, 2022 at 09:39:08AM +0200, Julia Lawall wrote: > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > The done_buffer count is already available in the hdm_channel struct. > > Calling dim_get_channel_state function to source this value out of > > the same structure is unnecessary. > > Further, the second parameter struct dim_ch_state_t to this function > > is filled by using the hdm_channel inside the function. This filled in > > variable is never used in the caller and can be altogether removed. > > So, a call to dim_get_channel_state function in this context also > > deems expensive. > > Thanks for the rewrite. > > I find "source this value out of" hard to understand. > > I would have written something like the following: > > The function dim_get_channel_state only serves to initialize the ready and > done_buffers fields of the structure passed as its second argument. In > service_done_flag, this structure is never used again and the only purpose > of the call is to get the value that is put in the done_buffers field. > But that value is just the done_sw_buffers_number field of the call's > first argument. So the whole call is useless, and we can just replace it > with an access to this field. > > This change implies that the variable st is no longer used, so drop it as > well. This is really well written. Sounds much structured. Now my own log message sounds a little random :) Is it okay for me to use your verbiage as is in my patch log? Thank you, ./drv > > --- > > If you want to work some more in this code, the name of the type > dim_ch_state_t should not have an _t at the end of it. It looks like it > used to be a typedef and someone eliminated the typedef but didn't drop > the _t. > > julia > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > Changes in v2: > > 1. Update patch log message to be more descriptive about the reason for change. > > Feedback provided by julia.lawall@inria.fr > > > > PLEASE NOTE: I have only built the module on my machine, but have not tested it. > > I am not sure how to test this change. I am willing to test it with appropriate > > guidance provided I have the necessary hardware. > > > > > > drivers/staging/most/dim2/dim2.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > index ab72e11ac5ab..4c1f27898a29 100644 > > --- a/drivers/staging/most/dim2/dim2.c > > +++ b/drivers/staging/most/dim2/dim2.c > > @@ -259,7 +259,6 @@ static void retrieve_netinfo(struct dim2_hdm *dev, struct mbo *mbo) > > static void service_done_flag(struct dim2_hdm *dev, int ch_idx) > > { > > struct hdm_channel *hdm_ch = dev->hch + ch_idx; > > - struct dim_ch_state_t st; > > struct list_head *head; > > struct mbo *mbo; > > int done_buffers; > > @@ -271,7 +270,7 @@ static void service_done_flag(struct dim2_hdm *dev, int ch_idx) > > > > spin_lock_irqsave(&dim_lock, flags); > > > > - done_buffers = dim_get_channel_state(&hdm_ch->ch, &st)->done_buffers; > > + done_buffers = hdm_ch->ch.done_sw_buffers_number; > > if (!done_buffers) { > > spin_unlock_irqrestore(&dim_lock, flags); > > return; > > -- > > 2.30.2 > > > > > > > > > >
On Tue, 18 Oct 2022, Deepak R Varma wrote: > On Tue, Oct 18, 2022 at 09:39:08AM +0200, Julia Lawall wrote: > > > > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > > > The done_buffer count is already available in the hdm_channel struct. > > > Calling dim_get_channel_state function to source this value out of > > > the same structure is unnecessary. > > > Further, the second parameter struct dim_ch_state_t to this function > > > is filled by using the hdm_channel inside the function. This filled in > > > variable is never used in the caller and can be altogether removed. > > > So, a call to dim_get_channel_state function in this context also > > > deems expensive. > > > > Thanks for the rewrite. > > > > I find "source this value out of" hard to understand. > > > > I would have written something like the following: > > > > The function dim_get_channel_state only serves to initialize the ready and > > done_buffers fields of the structure passed as its second argument. In > > service_done_flag, this structure is never used again and the only purpose > > of the call is to get the value that is put in the done_buffers field. > > But that value is just the done_sw_buffers_number field of the call's > > first argument. So the whole call is useless, and we can just replace it > > with an access to this field. > > > > This change implies that the variable st is no longer used, so drop it as > > well. > > This is really well written. Sounds much structured. Now my own log message > sounds a little random :) > > Is it okay for me to use your verbiage as is in my patch log? Yes. julia
On Tue, Oct 18, 2022 at 02:52:21PM +0200, Julia Lawall wrote: > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > On Tue, Oct 18, 2022 at 09:39:08AM +0200, Julia Lawall wrote: > > > > > > > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > > > > > The done_buffer count is already available in the hdm_channel struct. > > > > Calling dim_get_channel_state function to source this value out of > > > > the same structure is unnecessary. > > > > Further, the second parameter struct dim_ch_state_t to this function > > > > is filled by using the hdm_channel inside the function. This filled in > > > > variable is never used in the caller and can be altogether removed. > > > > So, a call to dim_get_channel_state function in this context also > > > > deems expensive. > > > > > > Thanks for the rewrite. > > > > > > I find "source this value out of" hard to understand. > > > > > > I would have written something like the following: > > > > > > The function dim_get_channel_state only serves to initialize the ready and > > > done_buffers fields of the structure passed as its second argument. In > > > service_done_flag, this structure is never used again and the only purpose > > > of the call is to get the value that is put in the done_buffers field. > > > But that value is just the done_sw_buffers_number field of the call's > > > first argument. So the whole call is useless, and we can just replace it > > > with an access to this field. > > > > > > This change implies that the variable st is no longer used, so drop it as > > > well. > > > > This is really well written. Sounds much structured. Now my own log message > > sounds a little random :) > > > > Is it okay for me to use your verbiage as is in my patch log? > > Yes. Thank you. Can I convert this into a patch set and include the other suggestion from you to correct the dim_ch_state_t struct name? Or should these be separate patches now? ./drv > > julia >
On Tue, 18 Oct 2022, Deepak R Varma wrote: > On Tue, Oct 18, 2022 at 02:52:21PM +0200, Julia Lawall wrote: > > > > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > > > On Tue, Oct 18, 2022 at 09:39:08AM +0200, Julia Lawall wrote: > > > > > > > > > > > > On Tue, 18 Oct 2022, Deepak R Varma wrote: > > > > > > > > > The done_buffer count is already available in the hdm_channel struct. > > > > > Calling dim_get_channel_state function to source this value out of > > > > > the same structure is unnecessary. > > > > > Further, the second parameter struct dim_ch_state_t to this function > > > > > is filled by using the hdm_channel inside the function. This filled in > > > > > variable is never used in the caller and can be altogether removed. > > > > > So, a call to dim_get_channel_state function in this context also > > > > > deems expensive. > > > > > > > > Thanks for the rewrite. > > > > > > > > I find "source this value out of" hard to understand. > > > > > > > > I would have written something like the following: > > > > > > > > The function dim_get_channel_state only serves to initialize the ready and > > > > done_buffers fields of the structure passed as its second argument. In > > > > service_done_flag, this structure is never used again and the only purpose > > > > of the call is to get the value that is put in the done_buffers field. > > > > But that value is just the done_sw_buffers_number field of the call's > > > > first argument. So the whole call is useless, and we can just replace it > > > > with an access to this field. > > > > > > > > This change implies that the variable st is no longer used, so drop it as > > > > well. > > > > > > This is really well written. Sounds much structured. Now my own log message > > > sounds a little random :) > > > > > > Is it okay for me to use your verbiage as is in my patch log? > > > > Yes. > > Thank you. Can I convert this into a patch set and include the other suggestion > from you to correct the dim_ch_state_t struct name? Or should these be separate > patches now? You can make a patch set. julia
diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c index ab72e11ac5ab..4c1f27898a29 100644 --- a/drivers/staging/most/dim2/dim2.c +++ b/drivers/staging/most/dim2/dim2.c @@ -259,7 +259,6 @@ static void retrieve_netinfo(struct dim2_hdm *dev, struct mbo *mbo) static void service_done_flag(struct dim2_hdm *dev, int ch_idx) { struct hdm_channel *hdm_ch = dev->hch + ch_idx; - struct dim_ch_state_t st; struct list_head *head; struct mbo *mbo; int done_buffers; @@ -271,7 +270,7 @@ static void service_done_flag(struct dim2_hdm *dev, int ch_idx) spin_lock_irqsave(&dim_lock, flags); - done_buffers = dim_get_channel_state(&hdm_ch->ch, &st)->done_buffers; + done_buffers = hdm_ch->ch.done_sw_buffers_number; if (!done_buffers) { spin_unlock_irqrestore(&dim_lock, flags); return;