[PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage

[PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage

[Deepak R Varma]

Patch set simplifies service_done_flag function by eliminating call to
dim_get_channel_state. Also corrects the misleading dim_ch_state_t variable
type name.

Changes in v4:
   1. Patch set versioning missed earlier. Now added. [feedback from gregkh@linuxfoundation.org]
   2. Patch 1/2 : None.
   3. Patch 2/2 : Correct patch subject and log. [feedback from julia.lawall@inria.fr]

Change in v3:
   1. Patch set introduced since another patch from same area added.


Deepak R Varma (2):
  staging: most: dim2: read done_buffers count locally from HDM channel
  staging: most: dim2: correct misleading struct type name

 drivers/staging/most/dim2/dim2.c | 5 ++---
 drivers/staging/most/dim2/hal.c  | 4 ++--
 drivers/staging/most/dim2/hal.h  | 6 +++---
 3 files changed, 7 insertions(+), 8 deletions(-)

--
2.30.2

[PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

[Deepak R Varma]

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.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---

PLEASE NOTE:
   1. 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.
   2. This was a standalone patch earlier. It is now combined into a patch set
      with another patch for the same driver. Hence I am carry forwarding the
      change log for this patch here:

Changes in v4:
   -- None.

Changes in v3:
   1. The patch log message is further improved. This revised verbiage is as
      thankfully provided by julia.lawall@inria.fr

Changes in v2:
   1. Update patch log message to be more descriptive about the reason for change.
      Feedback provided by julia.lawall@inria.fr


 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

[PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Deepak R Varma]

Correct misleading struct type name dim_ch_state_t to dim_ch_state
since this not a typedef but a normal structure declaration.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---

Changes in v4:
   1. Correct patch subject and log message. Use struct type name instead of
      variable name for the change description. Feedback from julia.lawall@inria.fr

Changes in v3:
   1. Patch introduced in the patch set

 drivers/staging/most/dim2/dim2.c | 2 +-
 drivers/staging/most/dim2/hal.c  | 4 ++--
 drivers/staging/most/dim2/hal.h  | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 4c1f27898a29..a69a61a69283 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
 	struct list_head *head = &hdm_ch->pending_list;
 	struct mbo *mbo;
 	unsigned long flags;
-	struct dim_ch_state_t st;
+	struct dim_ch_state st;

 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);
diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c
index 65282c276862..a5d40b5b138a 100644
--- a/drivers/staging/most/dim2/hal.c
+++ b/drivers/staging/most/dim2/hal.c
@@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
 	return channel_service(ch);
 }

-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr)
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr)
 {
 	if (!ch || !state_ptr)
 		return NULL;
diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h
index 20531449acab..ef10a8741c10 100644
--- a/drivers/staging/most/dim2/hal.h
+++ b/drivers/staging/most/dim2/hal.h
@@ -27,7 +27,7 @@ enum mlb_clk_speed {
 	CLK_8192FS = 7,
 };

-struct dim_ch_state_t {
+struct dim_ch_state {
 	bool ready; /* Shows readiness to enqueue next buffer */
 	u16 done_buffers; /* Number of completed buffers */
 };
@@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);

 u8 dim_service_channel(struct dim_channel *ch);

-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr);
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr);

 u16 dim_dbr_space(struct dim_channel *ch);

--
2.30.2

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Julia Lawall]

On Thu, 20 Oct 2022, Deepak R Varma wrote:

> Correct misleading struct type name dim_ch_state_t to dim_ch_state
> since this not a typedef but a normal structure declaration.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>
> Changes in v4:
>    1. Correct patch subject and log message. Use struct type name instead of
>       variable name for the change description. Feedback from julia.lawall@inria.fr
>
> Changes in v3:
>    1. Patch introduced in the patch set
>
>  drivers/staging/most/dim2/dim2.c | 2 +-
>  drivers/staging/most/dim2/hal.c  | 4 ++--
>  drivers/staging/most/dim2/hal.h  | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> index 4c1f27898a29..a69a61a69283 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
>  	struct list_head *head = &hdm_ch->pending_list;
>  	struct mbo *mbo;
>  	unsigned long flags;
> -	struct dim_ch_state_t st;
> +	struct dim_ch_state st;

Is there another use in service_done_flag?

julia

>
>  	BUG_ON(!hdm_ch);
>  	BUG_ON(!hdm_ch->is_initialized);
> diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c
> index 65282c276862..a5d40b5b138a 100644
> --- a/drivers/staging/most/dim2/hal.c
> +++ b/drivers/staging/most/dim2/hal.c
> @@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
>  	return channel_service(ch);
>  }
>
> -struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> -					     struct dim_ch_state_t *state_ptr)
> +struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
> +					   struct dim_ch_state *state_ptr)
>  {
>  	if (!ch || !state_ptr)
>  		return NULL;
> diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h
> index 20531449acab..ef10a8741c10 100644
> --- a/drivers/staging/most/dim2/hal.h
> +++ b/drivers/staging/most/dim2/hal.h
> @@ -27,7 +27,7 @@ enum mlb_clk_speed {
>  	CLK_8192FS = 7,
>  };
>
> -struct dim_ch_state_t {
> +struct dim_ch_state {
>  	bool ready; /* Shows readiness to enqueue next buffer */
>  	u16 done_buffers; /* Number of completed buffers */
>  };
> @@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);
>
>  u8 dim_service_channel(struct dim_channel *ch);
>
> -struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> -					     struct dim_ch_state_t *state_ptr);
> +struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
> +					   struct dim_ch_state *state_ptr);
>
>  u16 dim_dbr_space(struct dim_channel *ch);
>
> --
> 2.30.2
>
>
>
>
>

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Deepak R Varma]

On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
>
>
> On Thu, 20 Oct 2022, Deepak R Varma wrote:
>
> > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > since this not a typedef but a normal structure declaration.
> >
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >
> > Changes in v4:
> >    1. Correct patch subject and log message. Use struct type name instead of
> >       variable name for the change description. Feedback from julia.lawall@inria.fr
> >
> > Changes in v3:
> >    1. Patch introduced in the patch set
> >
> >  drivers/staging/most/dim2/dim2.c | 2 +-
> >  drivers/staging/most/dim2/hal.c  | 4 ++--
> >  drivers/staging/most/dim2/hal.h  | 6 +++---
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > index 4c1f27898a29..a69a61a69283 100644
> > --- a/drivers/staging/most/dim2/dim2.c
> > +++ b/drivers/staging/most/dim2/dim2.c
> > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> >  	struct list_head *head = &hdm_ch->pending_list;
> >  	struct mbo *mbo;
> >  	unsigned long flags;
> > -	struct dim_ch_state_t st;
> > +	struct dim_ch_state st;
>
> Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function
try_start_dim_transfer where the variable st is used down the line in the
execution. This time the channel state is retrieved by calling
dim_get_channel_state function. The state is simply computed and set. Should I
improve this as well?

If you are asking something different, could you please elaborate?

./drv

>
> julia
>
> >
> >  	BUG_ON(!hdm_ch);
> >  	BUG_ON(!hdm_ch->is_initialized);
> > diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c
> > index 65282c276862..a5d40b5b138a 100644
> > --- a/drivers/staging/most/dim2/hal.c
> > +++ b/drivers/staging/most/dim2/hal.c
> > @@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
> >  	return channel_service(ch);
> >  }
> >
> > -struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> > -					     struct dim_ch_state_t *state_ptr)
> > +struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
> > +					   struct dim_ch_state *state_ptr)
> >  {
> >  	if (!ch || !state_ptr)
> >  		return NULL;
> > diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h
> > index 20531449acab..ef10a8741c10 100644
> > --- a/drivers/staging/most/dim2/hal.h
> > +++ b/drivers/staging/most/dim2/hal.h
> > @@ -27,7 +27,7 @@ enum mlb_clk_speed {
> >  	CLK_8192FS = 7,
> >  };
> >
> > -struct dim_ch_state_t {
> > +struct dim_ch_state {
> >  	bool ready; /* Shows readiness to enqueue next buffer */
> >  	u16 done_buffers; /* Number of completed buffers */
> >  };
> > @@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);
> >
> >  u8 dim_service_channel(struct dim_channel *ch);
> >
> > -struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> > -					     struct dim_ch_state_t *state_ptr);
> > +struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
> > +					   struct dim_ch_state *state_ptr);
> >
> >  u16 dim_dbr_space(struct dim_channel *ch);
> >
> > --
> > 2.30.2
> >
> >
> >
> >
> >

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Deepak R Varma]

On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> >
> >
> > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> >
> > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > since this not a typedef but a normal structure declaration.
> > >
> > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > >
> > > Changes in v4:
> > >    1. Correct patch subject and log message. Use struct type name instead of
> > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > >
> > > Changes in v3:
> > >    1. Patch introduced in the patch set
> > >
> > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > index 4c1f27898a29..a69a61a69283 100644
> > > --- a/drivers/staging/most/dim2/dim2.c
> > > +++ b/drivers/staging/most/dim2/dim2.c
> > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > >  	struct list_head *head = &hdm_ch->pending_list;
> > >  	struct mbo *mbo;
> > >  	unsigned long flags;
> > > -	struct dim_ch_state_t st;
> > > +	struct dim_ch_state st;
> >
> > Is there another use in service_done_flag?
>
> Hi,
> I did not understand your question fully. This is from a different function
> try_start_dim_transfer where the variable st is used down the line in the
> execution. This time the channel state is retrieved by calling
> dim_get_channel_state function. The state is simply computed and set. Should I
> improve this as well?
>
> If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

>
> ./drv
>
> >
> > julia
> >
> > >
>
>
>

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Julia Lawall]

On Thu, 20 Oct 2022, Deepak R Varma wrote:

> On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > >
> > > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > > since this not a typedef but a normal structure declaration.
> > > >
> > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > >
> > > > Changes in v4:
> > > >    1. Correct patch subject and log message. Use struct type name instead of
> > > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > > >
> > > > Changes in v3:
> > > >    1. Patch introduced in the patch set
> > > >
> > > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > > index 4c1f27898a29..a69a61a69283 100644
> > > > --- a/drivers/staging/most/dim2/dim2.c
> > > > +++ b/drivers/staging/most/dim2/dim2.c
> > > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > > >  	struct list_head *head = &hdm_ch->pending_list;
> > > >  	struct mbo *mbo;
> > > >  	unsigned long flags;
> > > > -	struct dim_ch_state_t st;
> > > > +	struct dim_ch_state st;
> > >
> > > Is there another use in service_done_flag?
> >
> > Hi,
> > I did not understand your question fully. This is from a different function
> > try_start_dim_transfer where the variable st is used down the line in the
> > execution. This time the channel state is retrieved by calling
> > dim_get_channel_state function. The state is simply computed and set. Should I
> > improve this as well?
> >
> > If you are asking something different, could you please elaborate?
>
> Hi Julia,
> Can you please review and comment on my response?

In my kernel there is an occurrence of the type name in service_done_flag.
But I have the mainline, not Greg's staging tree, so there could be some
differences.

When I do git grep dim_ch_state_t, I get two occurrences in
drivers/staging/most/dim2/dim2.c

julia

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Deepak R Varma]

On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote:
>
>
> On Thu, 20 Oct 2022, Deepak R Varma wrote:
>
> > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > > >
> > > > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > > > since this not a typedef but a normal structure declaration.
> > > > >
> > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > > ---
> > > > >
> > > > > Changes in v4:
> > > > >    1. Correct patch subject and log message. Use struct type name instead of
> > > > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > > > >
> > > > > Changes in v3:
> > > > >    1. Patch introduced in the patch set
> > > > >
> > > > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > > > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > > > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > > > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > > > index 4c1f27898a29..a69a61a69283 100644
> > > > > --- a/drivers/staging/most/dim2/dim2.c
> > > > > +++ b/drivers/staging/most/dim2/dim2.c
> > > > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > > > >  	struct list_head *head = &hdm_ch->pending_list;
> > > > >  	struct mbo *mbo;
> > > > >  	unsigned long flags;
> > > > > -	struct dim_ch_state_t st;
> > > > > +	struct dim_ch_state st;
> > > >
> > > > Is there another use in service_done_flag?
> > >
> > > Hi,
> > > I did not understand your question fully. This is from a different function
> > > try_start_dim_transfer where the variable st is used down the line in the
> > > execution. This time the channel state is retrieved by calling
> > > dim_get_channel_state function. The state is simply computed and set. Should I
> > > improve this as well?
> > >
> > > If you are asking something different, could you please elaborate?
> >
> > Hi Julia,
> > Can you please review and comment on my response?
>
> In my kernel there is an occurrence of the type name in service_done_flag.
> But I have the mainline, not Greg's staging tree, so there could be some
> differences.
>
> When I do git grep dim_ch_state_t, I get two occurrences in
> drivers/staging/most/dim2/dim2.c

Okay. Still unclear. Following snip is what I see in my local staging-testing branch.

<snip>
	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
	drv@debian:~/git/kernels/staging$
</snip>

Does that help?

./drv


>
> julia

Re: [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

[Greg KH]

On Thu, Oct 20, 2022 at 01:24:53AM +0530, Deepak R Varma wrote:
> 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.

Are you sure it is useless?

You have changed the logic here, you are now thinking that this value
can never change, while before you were ensured of getting the "correct"
value as it is under the lock when the function is called.

I can't take this type of change as a "cleanup" patch for outreachy
unless you have the hardware as it is NOT a basic "checkpatch" style
cleanup at all.

If you want to get this change accepted, please work with the maintainer
of the code and get them to agree that the change is correct.  And if it
is, odds are more things also would need to be cleaned up at the same
time, right?

thanks,

greg k-h

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Greg KH]

On Thu, Oct 20, 2022 at 06:26:12PM +0530, Deepak R Varma wrote:
> On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote:
> >
> >
> > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> >
> > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > > > >
> > > > > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > > > > since this not a typedef but a normal structure declaration.
> > > > > >
> > > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v4:
> > > > > >    1. Correct patch subject and log message. Use struct type name instead of
> > > > > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > > > > >
> > > > > > Changes in v3:
> > > > > >    1. Patch introduced in the patch set
> > > > > >
> > > > > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > > > > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > > > > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > > > > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > > > > index 4c1f27898a29..a69a61a69283 100644
> > > > > > --- a/drivers/staging/most/dim2/dim2.c
> > > > > > +++ b/drivers/staging/most/dim2/dim2.c
> > > > > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > > > > >  	struct list_head *head = &hdm_ch->pending_list;
> > > > > >  	struct mbo *mbo;
> > > > > >  	unsigned long flags;
> > > > > > -	struct dim_ch_state_t st;
> > > > > > +	struct dim_ch_state st;
> > > > >
> > > > > Is there another use in service_done_flag?
> > > >
> > > > Hi,
> > > > I did not understand your question fully. This is from a different function
> > > > try_start_dim_transfer where the variable st is used down the line in the
> > > > execution. This time the channel state is retrieved by calling
> > > > dim_get_channel_state function. The state is simply computed and set. Should I
> > > > improve this as well?
> > > >
> > > > If you are asking something different, could you please elaborate?
> > >
> > > Hi Julia,
> > > Can you please review and comment on my response?
> >
> > In my kernel there is an occurrence of the type name in service_done_flag.
> > But I have the mainline, not Greg's staging tree, so there could be some
> > differences.
> >
> > When I do git grep dim_ch_state_t, I get two occurrences in
> > drivers/staging/most/dim2/dim2.c
> 
> Okay. Still unclear. Following snip is what I see in my local staging-testing branch.
> 
> <snip>
> 	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
> 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> 	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> 	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
> 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
> 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> 	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
> 	drv@debian:~/git/kernels/staging$
> </snip>
> 
> Does that help?

Not at all, as you did not test with your change applied:

  CC [M]  drivers/gpu/drm/vmwgfx/vmwgfx_drv.o
drivers/staging/most/dim2/dim2.c: In function ‘service_done_flag’:
drivers/staging/most/dim2/dim2.c:262:31: error: storage size of ‘st’ isn’t known
  262 |         struct dim_ch_state_t st;
      |                               ^~
drivers/staging/most/dim2/dim2.c:262:31: error: unused variable ‘st’ [-Werror=unused-variable]

:(

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Greg KH]

On Thu, Oct 20, 2022 at 05:04:52PM +0200, Greg KH wrote:
> On Thu, Oct 20, 2022 at 06:26:12PM +0530, Deepak R Varma wrote:
> > On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > >
> > > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> > > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> > > > > >
> > > > > >
> > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > > > > >
> > > > > > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > > > > > since this not a typedef but a normal structure declaration.
> > > > > > >
> > > > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v4:
> > > > > > >    1. Correct patch subject and log message. Use struct type name instead of
> > > > > > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > > > > > >
> > > > > > > Changes in v3:
> > > > > > >    1. Patch introduced in the patch set
> > > > > > >
> > > > > > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > > > > > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > > > > > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > > > > > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > > > > > index 4c1f27898a29..a69a61a69283 100644
> > > > > > > --- a/drivers/staging/most/dim2/dim2.c
> > > > > > > +++ b/drivers/staging/most/dim2/dim2.c
> > > > > > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > > > > > >  	struct list_head *head = &hdm_ch->pending_list;
> > > > > > >  	struct mbo *mbo;
> > > > > > >  	unsigned long flags;
> > > > > > > -	struct dim_ch_state_t st;
> > > > > > > +	struct dim_ch_state st;
> > > > > >
> > > > > > Is there another use in service_done_flag?
> > > > >
> > > > > Hi,
> > > > > I did not understand your question fully. This is from a different function
> > > > > try_start_dim_transfer where the variable st is used down the line in the
> > > > > execution. This time the channel state is retrieved by calling
> > > > > dim_get_channel_state function. The state is simply computed and set. Should I
> > > > > improve this as well?
> > > > >
> > > > > If you are asking something different, could you please elaborate?
> > > >
> > > > Hi Julia,
> > > > Can you please review and comment on my response?
> > >
> > > In my kernel there is an occurrence of the type name in service_done_flag.
> > > But I have the mainline, not Greg's staging tree, so there could be some
> > > differences.
> > >
> > > When I do git grep dim_ch_state_t, I get two occurrences in
> > > drivers/staging/most/dim2/dim2.c
> > 
> > Okay. Still unclear. Following snip is what I see in my local staging-testing branch.
> > 
> > <snip>
> > 	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
> > 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> > 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> > 	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> > 	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
> > 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
> > 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> > 	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
> > 	drv@debian:~/git/kernels/staging$
> > </snip>
> > 
> > Does that help?
> 
> Not at all, as you did not test with your change applied:
> 
>   CC [M]  drivers/gpu/drm/vmwgfx/vmwgfx_drv.o
> drivers/staging/most/dim2/dim2.c: In function ‘service_done_flag’:
> drivers/staging/most/dim2/dim2.c:262:31: error: storage size of ‘st’ isn’t known
>   262 |         struct dim_ch_state_t st;
>       |                               ^~
> drivers/staging/most/dim2/dim2.c:262:31: error: unused variable ‘st’ [-Werror=unused-variable]
> 
> :(
> 

Ah, that was because I rejected patch 1/2 here. So this one will not
work as-is, my fault.

Please fix up and resend only this one if you still want to see it
applied.

thanks,

greg k-h

Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name

[Julia Lawall]

On Thu, 20 Oct 2022, Deepak R Varma wrote:

> On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote:
> >
> >
> > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> >
> > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote:
> > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote:
> > > > >
> > > > > > Correct misleading struct type name dim_ch_state_t to dim_ch_state
> > > > > > since this not a typedef but a normal structure declaration.
> > > > > >
> > > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v4:
> > > > > >    1. Correct patch subject and log message. Use struct type name instead of
> > > > > >       variable name for the change description. Feedback from julia.lawall@inria.fr
> > > > > >
> > > > > > Changes in v3:
> > > > > >    1. Patch introduced in the patch set
> > > > > >
> > > > > >  drivers/staging/most/dim2/dim2.c | 2 +-
> > > > > >  drivers/staging/most/dim2/hal.c  | 4 ++--
> > > > > >  drivers/staging/most/dim2/hal.h  | 6 +++---
> > > > > >  3 files changed, 6 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> > > > > > index 4c1f27898a29..a69a61a69283 100644
> > > > > > --- a/drivers/staging/most/dim2/dim2.c
> > > > > > +++ b/drivers/staging/most/dim2/dim2.c
> > > > > > @@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
> > > > > >  	struct list_head *head = &hdm_ch->pending_list;
> > > > > >  	struct mbo *mbo;
> > > > > >  	unsigned long flags;
> > > > > > -	struct dim_ch_state_t st;
> > > > > > +	struct dim_ch_state st;
> > > > >
> > > > > Is there another use in service_done_flag?
> > > >
> > > > Hi,
> > > > I did not understand your question fully. This is from a different function
> > > > try_start_dim_transfer where the variable st is used down the line in the
> > > > execution. This time the channel state is retrieved by calling
> > > > dim_get_channel_state function. The state is simply computed and set. Should I
> > > > improve this as well?
> > > >
> > > > If you are asking something different, could you please elaborate?
> > >
> > > Hi Julia,
> > > Can you please review and comment on my response?
> >
> > In my kernel there is an occurrence of the type name in service_done_flag.
> > But I have the mainline, not Greg's staging tree, so there could be some
> > differences.
> >
> > When I do git grep dim_ch_state_t, I get two occurrences in
> > drivers/staging/most/dim2/dim2.c
>
> Okay. Still unclear. Following snip is what I see in my local staging-testing branch.
>
> <snip>
> 	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
> 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> 	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
> 	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> 	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
> 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
> 	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
> 	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
> 	drv@debian:~/git/kernels/staging$
> </snip>
>
> Does that help?

You also have two occurrences in dim2.c.

julia

Re: [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

[Deepak R Varma]

On Thu, Oct 20, 2022 at 05:03:14PM +0200, Greg KH wrote:
> On Thu, Oct 20, 2022 at 01:24:53AM +0530, Deepak R Varma wrote:
> > 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.
>
> Are you sure it is useless?

Hello,
pardon my limited understanding, but I think this function call is not
necessary.

>
> You have changed the logic here, you are now thinking that this value
> can never change, while before you were ensured of getting the "correct"
> value as it is under the lock when the function is called.

I may be wrong, but I do not think there is a change in the long, but I may
entirely wrong. The function was called from inside the lock scope, now we are
extracting the value directly, still inside the lock scope. This should be safe.

>
> I can't take this type of change as a "cleanup" patch for outreachy
> unless you have the hardware as it is NOT a basic "checkpatch" style
> cleanup at all.

Sure. That is fine.

>
> If you want to get this change accepted, please work with the maintainer
> of the code and get them to agree that the change is correct.  And if it
> is, odds are more things also would need to be cleaned up at the same
> time, right?

I am eagerly waiting for a feedback from the maintainer. If they agree with my
viewpoint, I will continue to work on this change outside of the clean up patch
tasks. I will be happy to :)

Thank you Greg!
./drv

>
> thanks,
>
> greg k-h
>