Staging: ccree: Don't use volatile for monitor_lock

PATCH In review Srishti Sharma 5 participants 10 messages Sep 11, 2017 – Sep 12, 2017

Subsystems (2)

Staging Crypto API

lore

Share & tools

lore

Srishti Sharma

author

2017-09-11 15:59 UTC #0 lore raw

PATCH 1/1 Staging: ccree: Don't use volatile for monitor_lock

The use of volatile for the variable monitor_lock is unnecessary.

Signed-off-by Srishti Sharma <[email protected]>

---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/ccree/ssi_request_mgr.c +1 -1

index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
 	dma_addr_t dummy_comp_buff_dma;
 	struct cc_hw_desc monitor_desc;
 
-	volatile unsigned long monitor_lock;
+	unsigned long monitor_lock;
 #ifdef COMP_IN_WQ
 	struct workqueue_struct *workq;
 	struct delayed_work compwork;

Julia Lawall

2017-09-11 16:03 UTC #1 lore raw

Quoted 1 line · Srishti Sharma

The use of volatile for the variable monitor_lock is unnecessary.

You need to give more evidence of why this is the case. How is the variable used? I guess this comes from checkpatch, but checkpatch has only a local view of things, and doesn't know how the variable is used.

julia

Quoted 29 lines

Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +1 -2
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
 	dma_addr_t dummy_comp_buff_dma;
 	struct cc_hw_desc monitor_desc;
 
-	volatile unsigned long monitor_lock;
+	unsigned long monitor_lock;
 #ifdef COMP_IN_WQ
 	struct workqueue_struct *workq;
 	struct delayed_work compwork;
--
2.7.4
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505145571-11248-1-git-send-email-srishtishar%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Greg KH

maintainer

2017-09-11 16:04 UTC #2 lore raw

In reply to Srishti Sharma

Quoted 12 lines · Srishti Sharma

The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644

drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52

Quoted 5 lines

drivers/staging/ccree/ssi_request_mgr.c +1 -1

@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
 	dma_addr_t dummy_comp_buff_dma;
 	struct cc_hw_desc monitor_desc;
 
-	volatile unsigned long monitor_lock;
+	unsigned long monitor_lock;

While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?

thanks,

greg k-h

Srishti Sharma

author

2017-09-11 16:08 UTC #3 lore raw

Quoted 16 lines · Greg KH

Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 

Quoted 5 lines

drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?

I tried to find where is this variable being used in the code, but I didn't find any usage of it . It might be an important attribute of this structure definition but, I don't see it's value being set to anything or being used somewhere .

Regards,
Srishti

Quoted 4 lines

thanks,
greg k-h

Sean Paul

2017-09-11 16:11 UTC #4 lore raw

Quoted 23 lines · Srishti Sharma

Quoted 16 lines · Greg KH
Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 
Quoted 5 lines
drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?

Quoted 5 lines

I tried to find where is this variable being used in the code, but I didn't find any usage of it . It might be an important attribute of this structure definition but, I don't see it's value being set to anything or being used somewhere .

AFAICT, it's not used. Your patch should just remove it instead :)

Sean

Quoted 13 lines

Regards,
Srishti
Quoted 4 lines
thanks,
greg k-h
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAB3L5oxcyhgyy8EuGuPo9QtJQd-W7JTgQQE1PfopZFmSx58P9g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Julia Lawall

2017-09-11 16:11 UTC #5 lore raw

In reply to Srishti Sharma

Quoted 23 lines · Srishti Sharma

Quoted 16 lines · Greg KH
Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 
Quoted 5 lines
drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?

Quoted 4 lines

I tried to find where is this variable being used in the code, but I didn't find any usage of it . It might be an important attribute of this structure definition but, I don't see it's value being set to anything or being used somewhere .

Try removing it and see if the code still compiles. There is always a danger that a use of something could be constructed using ## in a macro, although given the uses of ## for this driver, it doesn't seem likely here.

julia

Srishti Sharma

author

2017-09-11 16:15 UTC #6 lore raw

Quoted 32 lines · Julia Lawall

Quoted 23 lines · Srishti Sharma
Quoted 16 lines · Greg KH
Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 
Quoted 5 lines
drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?
Quoted 4 lines
I tried to find where is this variable being used in the code, but I didn't find any usage of it . It might be an important attribute of this structure definition but, I don't see it's value being set to anything or being used somewhere .

Quoted 4 lines

Try removing it and see if the code still compiles. There is always a danger that a use of something could be constructed using ## in a macro, although given the uses of ## for this driver, it doesn't seem likely here.

Yes, I'll do that.

Regards,
Srishti

Quoted 2 lines

julia

Srishti Sharma

author

2017-09-11 16:34 UTC #7 lore raw

Quoted 33 lines · Srishti Sharma

Quoted 32 lines · Julia Lawall
Quoted 27 lines · Srishti Sharma
Quoted 16 lines · Greg KH
Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 
Quoted 5 lines
drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
While volatile is not right, odds are, this is still totally wrong as well. How about using a "real" lock instead?
I tried to find where is this variable being used in the code, but I didn't find any usage of it . It might be an important attribute of this structure definition but, I don't see it's value being set to anything or being used somewhere .

Quoted 4 lines

Quoted 4 lines
Try removing it and see if the code still compiles. There is always a danger that a use of something could be constructed using ## in a macro, although given the uses of ## for this driver, it doesn't seem likely here.

It compiles, so I have removed the variable and sent another patch

Thanks,
Srishti

Quoted 7 lines

Yes, I'll do that.
Regards,
Srishti
Quoted 2 lines
julia

Dan Carpenter

2017-09-12 09:19 UTC #8 lore raw

In reply to Srishti Sharma

Quoted 12 lines · Srishti Sharma

The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644

drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52

Quoted 5 lines

drivers/staging/ccree/ssi_request_mgr.c +1 -1

@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
 	dma_addr_t dummy_comp_buff_dma;
 	struct cc_hw_desc monitor_desc;
 
-	volatile unsigned long monitor_lock;
+	unsigned long monitor_lock;

The variable seems unused. Try deleting it instead.

regards,
dan carpenter

Srishti Sharma

author

2017-09-12 12:05 UTC #9 lore raw

Quoted 16 lines · Dan Carpenter

Quoted 15 lines · Srishti Sharma
The use of volatile for the variable monitor_lock is unnecessary.
Signed-off-by Srishti Sharma <[email protected]>
---
 drivers/staging/ccree/ssi_request_mgr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ccree/ssi_request_mgr.c +0 -0
index e5c2f92..7d77941 100644
@@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
      dma_addr_t dummy_comp_buff_dma;
      struct cc_hw_desc monitor_desc;
 

Quoted 4 lines

drivers/staging/ccree/ssi_request_mgr.c struct ssi_request_mgr_handle { · line 52
Quoted 2 lines
drivers/staging/ccree/ssi_request_mgr.c +1 -1
-     volatile unsigned long monitor_lock;
+     unsigned long monitor_lock;
The variable seems unused. Try deleting it instead.

Had already sent a patch for that as v2.

Thanks
Srishti

Quoted 3 lines

regards,
dan carpenter