staging: most: dim2: remove unnecessary function call and variable usage

PATCH In review Deepak R Varma 3 participants 13 messages Oct 19, 2022 – Oct 20, 2022

Subsystems (2)

Staging Most(r) technology driver

lore

Share & tools

lore

Deepak R Varma

author

2022-10-19 19:53 UTC #0 lore raw

v4 cover staging: most: dim2: remove unnecessary function call and variable usage

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 [email protected]]
2. Patch 1/2 : None.
3. Patch 2/2 : Correct patch subject and log. [feedback from [email protected]]

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

Deepak R Varma

author

2022-10-19 19:55 UTC #1 lore raw

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

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 <[email protected]>

---

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 [email protected]

Changes in v2:
1. Update patch log message to be more descriptive about the reason for change. Feedback provided by [email protected]

 drivers/staging/most/dim2/dim2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/most/dim2/dim2.c +1 -2

index ab72e11ac5ab..4c1f27898a29 100644
@@ -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

Deepak R Varma

author

2022-10-19 19:56 UTC #4 lore raw

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

In reply to 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.

Signed-off-by Deepak R Varma <[email protected]>

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

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(-)

drivers/staging/most/dim2/dim2.c +1 -1

index 4c1f27898a29..a69a61a69283 100644
@@ -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);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -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;

drivers/staging/most/dim2/hal.h +3 -3

index 20531449acab..ef10a8741c10 100644
@@ -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

Julia Lawall

2022-10-19 20:08 UTC #5 lore raw

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

Quoted 24 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
drivers/staging/most/dim2/dim2.c +0 -0
index 4c1f27898a29..a69a61a69283 100644

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 5 lines

drivers/staging/most/dim2/dim2.c +1 -1

@@ -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

Quoted 49 lines

drivers/staging/most/dim2/dim2.c +0 -0

 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -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;

drivers/staging/most/dim2/hal.h +3 -4

index 20531449acab..ef10a8741c10 100644
@@ -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

Deepak R Varma

author

2022-10-19 20:30 UTC #6 lore raw

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

Quoted 23 lines · Julia Lawall

Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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

Quoted 52 lines

julia

Quoted 49 lines

drivers/staging/most/dim2/dim2.c +0 -0

 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -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;

drivers/staging/most/dim2/hal.h +3 -4

index 20531449acab..ef10a8741c10 100644
@@ -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

Deepak R Varma

author

2022-10-20 11:48 UTC #7 lore raw

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

Quoted 37 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?

Quoted 8 lines

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?

Quoted 10 lines

./drv
Quoted 4 lines
julia
Quoted 1 line

Julia Lawall

2022-10-20 12:06 UTC #8 lore raw

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

Quoted 45 lines · Deepak R Varma

Quoted 44 lines · Deepak R Varma
Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?

Quoted 4 lines

Quoted 1 line
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

Deepak R Varma

author

2022-10-20 12:56 UTC #9 lore raw

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

Quoted 54 lines · Julia Lawall

Quoted 45 lines · Deepak R Varma
Quoted 44 lines · Deepak R Varma
Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?
Quoted 4 lines
Quoted 1 line
If you are asking something different, could you please elaborate?
Hi Julia,
Can you please review and comment on my response?

Quoted 6 lines

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

Quoted 2 lines

julia

Greg KH

maintainer

2022-10-20 15:03 UTC #2 lore raw

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

In reply to Deepak R Varma

Quoted 7 lines · 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.

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

Greg KH

maintainer

2022-10-20 15:04 UTC #10 lore raw

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

In reply to Deepak R Varma

Quoted 76 lines · Deepak R Varma

Quoted 54 lines · Julia Lawall
Quoted 45 lines · Deepak R Varma
Quoted 44 lines · Deepak R Varma
Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?
Quoted 4 lines
Quoted 1 line
If you are asking something different, could you please elaborate?
Hi Julia,
Can you please review and comment on my response?
Quoted 6 lines
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>

Quoted 1 line

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]

Greg KH

maintainer

2022-10-20 15:05 UTC #11 lore raw

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

Quoted 81 lines · Greg KH

Quoted 76 lines · Deepak R Varma
Quoted 54 lines · Julia Lawall
Quoted 45 lines · Deepak R Varma
Quoted 44 lines · Deepak R Varma
Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?
Quoted 4 lines
Quoted 1 line
If you are asking something different, could you please elaborate?
Hi Julia,
Can you please review and comment on my response?
Quoted 6 lines
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>
Quoted 1 line
Does that help?
Not at all, as you did not test with your change applied:

Quoted 9 lines

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

Julia Lawall

2022-10-20 15:10 UTC #12 lore raw

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

In reply to Deepak R Varma

Quoted 76 lines · Deepak R Varma

Quoted 54 lines · Julia Lawall
Quoted 45 lines · Deepak R Varma
Quoted 44 lines · Deepak R Varma
Quoted 23 lines · Julia Lawall
Quoted 19 lines · 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 <[email protected]>
Signed-off-by Deepak R Varma <[email protected]>
---
Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]
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(-)
Quoted 12 lines
drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164
Quoted 10 lines
drivers/staging/most/dim2/dim2.c +1 -1
index 4c1f27898a29..a69a61a69283 100644
@@ -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?
Quoted 4 lines
Quoted 1 line
If you are asking something different, could you please elaborate?
Hi Julia,
Can you please review and comment on my response?
Quoted 6 lines
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>

Quoted 1 line

Does that help?

You also have two occurrences in dim2.c.

julia

Deepak R Varma

author

2022-10-20 16:41 UTC #3 lore raw

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

In reply to Greg KH

Quoted 10 lines · Greg KH

Quoted 7 lines · 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.
Are you sure it is useless?

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 5 lines

thanks,
greg k-h