* Re: [PATCH] staging:slicoss:slicoss.h remove volatile variables
2015-06-26 19:57 ` [PATCH] staging:slicoss:slicoss.h remove volatile variables Vikul Gupta
@ 2015-06-26 20:09 ` Greg Kroah-Hartman
2015-06-26 23:10 ` David Matlack
1 sibling, 0 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2015-06-26 20:09 UTC (permalink / raw)
To: Vikul Gupta; +Cc: Cc: Lior Dotan, Christopher Harrer, devel, linux-kernel
On Fri, Jun 26, 2015 at 12:57:32PM -0700, Vikul Gupta wrote:
> 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.
> I fixed the slicoss sub-system.
The whole subsystem, nice? :)
Seriously, this sentance is not needed.
> 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?
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] staging:slicoss:slicoss.h remove volatile variables
2015-06-26 19:57 ` [PATCH] staging:slicoss:slicoss.h remove volatile variables Vikul Gupta
2015-06-26 20:09 ` Greg Kroah-Hartman
@ 2015-06-26 23:10 ` David Matlack
1 sibling, 0 replies; 3+ messages in thread
From: David Matlack @ 2015-06-26 23:10 UTC (permalink / raw)
To: Vikul Gupta
Cc: Greg Kroah-Hartman, Cc: Lior Dotan, Christopher Harrer, devel,
linux-kernel@vger.kernel.org
Hi Vikul, welcome! See my comment below...
On Fri, Jun 26, 2015 at 12:57 PM, Vikul Gupta <guptav@linux.vnet.ibm.com> wrote:
> 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.
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.
>
> 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 <guptav@linux.vnet.ibm.com>
>
> diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
> index 3a5aa88..f19f86a 100644
> --- a/drivers/staging/slicoss/slic.h
> +++ b/drivers/staging/slicoss/slic.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 3+ messages in thread