linux-kernel - Re: [PATCH] staging: most: dim2: read done_buffers count locally from HDM channel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2210180733330.2938@hadrien>
Date:   Tue, 18 Oct 2022 07:35:29 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Deepak R Varma <drv@...lo.com>
cc:     outreachy@...ts.linux.dev, gregkh@...uxfoundation.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        kumarpraveen@...ux.microsoft.com, saurabh.truth@...il.com
Subject: Re: [PATCH] staging: most: dim2: read done_buffers count locally
 from HDM channel



On Tue, 18 Oct 2022, Deepak R Varma wrote:

> On Mon, Oct 17, 2022 at 10:56:03PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 18 Oct 2022, Deepak R Varma wrote:
> >
> > > The done_buffer count can be directly read from HDM channel instead of
> > > calling the dim_get_channel_state function. This change also results in
> > > obsoleting the dim_channel_state local structure variable.
> > >
> > > Signed-off-by: Deepak R Varma <drv@...lo.com>
> > > ---
> > >
> > > 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.
> >
> > For non experts, maybe it would be helpful to explain what motivated you
> > to do this?
>
> I was actually trying to understand the implementation of this module to
> determine if I can replace BUG_ON calls by WARN_ON_ONCE. A "ctrl+]" navigation
> took me to this function and I started wondering about why the function call
> would be necessary. Hence the change.

OK, I agree with you that this story might not be super interesting.

But the log message still seems too concise.  You have acquired some
knowledge about why this changeis correct, but that knowledge is not at
all reflected in the log message.  Try to explain in more detail why the
function call is not necessary.

julia

>
> While reading the Documentation under dim2 directory, I realised this module may
> need a specialised hardware for testing [automotive???]. Hence I was not sure
> how to test the same.
>
> Are you suggesting I mention this in the patch description (the motivation)?
>
> Thank you,
> ./drv
>
> >
> > julia
> >
> > >
> > >  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
> > >
> > >
> > >
> > >
> > >
>
>
>