Skip to contentSkip to conversation

staging:slicoss:slicoss.h remove volatile variables

PATCH In review Vikul Gupta 3 participants 3 messages Jun 26, 2015 2 messages not in the archive Staging

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Vikul Gupta

author
#2 lore raw

PATCH 1/1 staging:slicoss:slicoss.h remove volatile variables

I am a high school student trying to become familiar with the opensource process and linux kernel. This is my first submission to the mailing list.

I fixed the slicoss sub-system. The TODO file asks to remove volatile variables - also, checkpatch.pl warnings included volatile variables.

I removed "volatile" from the variables /isr /and /linkstatus/ in the header file, because they are not needed. The two variables are used in the slicoss.c file, where /isr/ is used as function parameters, string outputs, pointers, logic, and one assignment, while /linkstatus /is used as pointers, logic, and one assignment. All but the assignments will not change these variables, and the assignment does not warrant a volatile qualifier.

To make sure the changes were correct, I ran the files with checkpatch.pl again, test built it, and rebooted it.

Signed-off-by Vikul Gupta <[email protected]>
drivers/staging/slicoss/slic.h +2 -2
index 3a5aa88..f19f86a 100644
@@ -357,8 +357,8 @@ struct base_driver {
 };
 
 struct slic_shmem {
-    volatile u32          isr;
-    volatile u32          linkstatus;
+    u32          isr;
+    u32          linkstatus;
     volatile struct slic_stats     inicstats;
 };
maintainer
#3 lore raw
Quoted 2 lines · Vikul Gupta

I am a high school student trying to become familiar with the opensource process and linux kernel. This is my first submission to the mailing list.

Great, but this paragraph doesn't belong in here, as this whole thing will end up in the changelog body. You can put any comments / discussion below the --- line and it will not show up in the changelog when I apply the patch.

Quoted 1 line

I fixed the slicoss sub-system.

The whole subsystem, nice? :)

Seriously, this sentance is not needed.

Quoted 4 lines

The TODO file asks to remove volatile
variables - also, checkpatch.pl warnings included volatile variables.

I removed "volatile" from the variables /isr /and /linkstatus/ in the header

what's with the '/' usage?

Quoted 8 lines

file, because they are not needed. The two variables are used in the slicoss.c file, where /isr/ is used as function parameters, string outputs, pointers, logic, and one assignment, while /linkstatus /is used as pointers, logic, and one assignment. All but the assignments will not change these variables, and the assignment does not warrant a volatile qualifier.

To make sure the changes were correct, I ran the files with checkpatch.pl again, test built it, and rebooted it.

can you test this code? rebooting and not loading the driver isn't much proof of much other than the code still builds (which is a good thing to test.) Also, this paragraph is not needed in the changelog either.

Care to fix this all up and resend?

thanks,

greg k-h

#4 lore raw
In reply to Vikul Gupta

Hi Vikul, welcome! See my comment below...

Quoted 6 lines · Vikul Gupta

I am a high school student trying to become familiar with the opensource process and linux kernel. This is my first submission to the mailing list.

I fixed the slicoss sub-system. The TODO file asks to remove volatile variables - also, checkpatch.pl warnings included volatile variables.

Quoted 6 lines

I removed "volatile" from the variables /isr /and /linkstatus/ in the header file, because they are not needed. The two variables are used in the slicoss.c file, where /isr/ is used as function parameters, string outputs, pointers, logic, and one assignment, while /linkstatus /is used as pointers, logic, and one assignment. All but the assignments will not change these variables, and the assignment does not warrant a volatile qualifier.

It is not safe to simply drop volatile from these fields. For example, slic_card_init polls on isr waiting for the device to write to it. If you drop volatile, the compiler is within its rights to pull the read out of the loop.

Quoted 26 lines

To make sure the changes were correct, I ran the files with checkpatch.pl again, test built it, and rebooted it.

Signed-off-by Vikul Gupta <[email protected]>
drivers/staging/slicoss/slic.h +2 -3
index 3a5aa88..f19f86a 100644
@@ -357,8 +357,8 @@ struct base_driver {
 };
 
 struct slic_shmem {
-    volatile u32          isr;
-    volatile u32          linkstatus;
+    u32          isr;
+    u32          linkstatus;
     volatile struct slic_stats     inicstats;
 };
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/