* [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
@ 2020-03-12 12:15 Lourdes Pedrajas
2020-03-12 12:29 ` Greg KH
2020-03-12 12:31 ` [Outreachy kernel] " Stefano Brivio
0 siblings, 2 replies; 6+ messages in thread
From: Lourdes Pedrajas @ 2020-03-12 12:15 UTC (permalink / raw)
To: outreachy-kernel, gregkh, matthias.bgg; +Cc: Lourdes Pedrajas
Change variable declaration volatile unsigned long chan_issued to
unsigned long chan_issued
Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
Issue found with checkpatch.pl
Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
---
drivers/staging/mt7621-dma/mtk-hsdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/mt7621-dma/mtk-hsdma.c b/drivers/staging/mt7621-dma/mtk-hsdma.c
index 20b898954416..e146c5bb5609 100644
--- a/drivers/staging/mt7621-dma/mtk-hsdma.c
+++ b/drivers/staging/mt7621-dma/mtk-hsdma.c
@@ -157,7 +157,7 @@ struct mtk_hsdam_engine {
struct device_dma_parameters dma_parms;
void __iomem *base;
struct tasklet_struct task;
- volatile unsigned long chan_issued;
+ unsigned long chan_issued;
struct mtk_hsdma_chan chan[1];
};
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
2020-03-12 12:15 [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile Lourdes Pedrajas
@ 2020-03-12 12:29 ` Greg KH
2020-03-12 12:31 ` [Outreachy kernel] " Stefano Brivio
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-03-12 12:29 UTC (permalink / raw)
To: Lourdes Pedrajas; +Cc: outreachy-kernel, matthias.bgg
On Thu, Mar 12, 2020 at 01:15:48PM +0100, Lourdes Pedrajas wrote:
> Change variable declaration volatile unsigned long chan_issued to
> unsigned long chan_issued
>
> Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
>
> Issue found with checkpatch.pl
>
> Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
> ---
> drivers/staging/mt7621-dma/mtk-hsdma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/mt7621-dma/mtk-hsdma.c b/drivers/staging/mt7621-dma/mtk-hsdma.c
> index 20b898954416..e146c5bb5609 100644
> --- a/drivers/staging/mt7621-dma/mtk-hsdma.c
> +++ b/drivers/staging/mt7621-dma/mtk-hsdma.c
> @@ -157,7 +157,7 @@ struct mtk_hsdam_engine {
> struct device_dma_parameters dma_parms;
> void __iomem *base;
> struct tasklet_struct task;
> - volatile unsigned long chan_issued;
> + unsigned long chan_issued;
Are you sure this is correct? Usually a change like this also requires
some kind of lock somewhere to ensure things work properly (not that it
is not working properly today...)
But it's not always as simple as just removing 'volatile', otherwise
that would have been done already.
Please do more work to determine if this is a safe change or not and
write it all up in the changelog.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
2020-03-12 12:15 [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile Lourdes Pedrajas
2020-03-12 12:29 ` Greg KH
@ 2020-03-12 12:31 ` Stefano Brivio
2020-03-12 19:43 ` Lourdes Pedrajas
1 sibling, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2020-03-12 12:31 UTC (permalink / raw)
To: Lourdes Pedrajas; +Cc: outreachy-kernel, gregkh, matthias.bgg
On Thu, 12 Mar 2020 13:15:48 +0100
Lourdes Pedrajas <lu@pplo.net> wrote:
> Change variable declaration volatile unsigned long chan_issued to
> unsigned long chan_issued
>
> Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
This needs some more details. From that same guide:
Patches to remove volatile variables are generally welcome - as
long as they come with a justification which shows that the
concurrency issues have been properly thought through.
so, here you need to investigate what that "volatile" was supposed to
achieve, if it is achieved by other means, if some form of locking is
needed instead, if we end up in one of the "exception" cases also
listed in that guide.
--
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
2020-03-12 12:31 ` [Outreachy kernel] " Stefano Brivio
@ 2020-03-12 19:43 ` Lourdes Pedrajas
2020-03-12 20:43 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Lourdes Pedrajas @ 2020-03-12 19:43 UTC (permalink / raw)
To: Stefano Brivio; +Cc: outreachy-kernel, gregkh, matthias.bgg
On Thu, Mar 12, 2020 at 01:29:52PM +0100, Greg KH wrote:
> On Thu, Mar 12, 2020 at 01:15:48PM +0100, Lourdes Pedrajas wrote:
> > Change variable declaration volatile unsigned long chan_issued to
> > unsigned long chan_issued
>
> Are you sure this is correct? Usually a change like this also requires
> some kind of lock somewhere to ensure things work properly (not that it
> is not working properly today...)
>
> But it's not always as simple as just removing 'volatile', otherwise
> that would have been done already.
>
> Please do more work to determine if this is a safe change or not and
> write it all up in the changelog.
>
> thanks,
>
> greg k-h
On Thu, Mar 12, 2020 at 01:31:12PM +0100, Stefano Brivio wrote:
> On Thu, 12 Mar 2020 13:15:48 +0100
> Lourdes Pedrajas <lu@pplo.net> wrote:
>
> > Change variable declaration volatile unsigned long chan_issued to
> > unsigned long chan_issued
> >
> > Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
>
> This needs some more details. From that same guide:
>
> Patches to remove volatile variables are generally welcome - as
> long as they come with a justification which shows that the
> concurrency issues have been properly thought through.
>
> so, here you need to investigate what that "volatile" was supposed to
> achieve, if it is achieved by other means, if some form of locking is
> needed instead, if we end up in one of the "exception" cases also
> listed in that guide.
>
> --
> Stefano
>
Indeed, both of you are right. Thank you for pointing me in the right
direction! However, as someone who is new in programming, I find I lack the
knowledge needed to tackle concurrency and locks. Nevertheless, I will do some
research to learn a bit to see if it can be done in a reasonable amount of
time. Sorry :(
Lourdes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
2020-03-12 19:43 ` Lourdes Pedrajas
@ 2020-03-12 20:43 ` Greg KH
2020-03-12 20:57 ` Lourdes Pedrajas
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2020-03-12 20:43 UTC (permalink / raw)
To: Lourdes Pedrajas; +Cc: Stefano Brivio, outreachy-kernel, matthias.bgg
On Thu, Mar 12, 2020 at 08:43:29PM +0100, Lourdes Pedrajas wrote:
> On Thu, Mar 12, 2020 at 01:29:52PM +0100, Greg KH wrote:
> > On Thu, Mar 12, 2020 at 01:15:48PM +0100, Lourdes Pedrajas wrote:
> > > Change variable declaration volatile unsigned long chan_issued to
> > > unsigned long chan_issued
> >
> > Are you sure this is correct? Usually a change like this also requires
> > some kind of lock somewhere to ensure things work properly (not that it
> > is not working properly today...)
> >
> > But it's not always as simple as just removing 'volatile', otherwise
> > that would have been done already.
> >
> > Please do more work to determine if this is a safe change or not and
> > write it all up in the changelog.
> >
> > thanks,
> >
> > greg k-h
>
> On Thu, Mar 12, 2020 at 01:31:12PM +0100, Stefano Brivio wrote:
> > On Thu, 12 Mar 2020 13:15:48 +0100
> > Lourdes Pedrajas <lu@pplo.net> wrote:
> >
> > > Change variable declaration volatile unsigned long chan_issued to
> > > unsigned long chan_issued
> > >
> > > Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
> >
> > This needs some more details. From that same guide:
> >
> > Patches to remove volatile variables are generally welcome - as
> > long as they come with a justification which shows that the
> > concurrency issues have been properly thought through.
> >
> > so, here you need to investigate what that "volatile" was supposed to
> > achieve, if it is achieved by other means, if some form of locking is
> > needed instead, if we end up in one of the "exception" cases also
> > listed in that guide.
> >
> > --
> > Stefano
> >
>
> Indeed, both of you are right. Thank you for pointing me in the right
> direction! However, as someone who is new in programming, I find I lack the
> knowledge needed to tackle concurrency and locks. Nevertheless, I will do some
> research to learn a bit to see if it can be done in a reasonable amount of
> time. Sorry :(
>
If you are new to programming, I would focus on other things.
Concurrency and locks are not exactly "simple" things, but they are
essencial to learn to be a kernel programmer.
good luck!
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile
2020-03-12 20:43 ` Greg KH
@ 2020-03-12 20:57 ` Lourdes Pedrajas
0 siblings, 0 replies; 6+ messages in thread
From: Lourdes Pedrajas @ 2020-03-12 20:57 UTC (permalink / raw)
To: Greg KH; +Cc: Stefano Brivio, outreachy-kernel, matthias.bgg
On Thu, Mar 12, 2020 at 09:43:16PM +0100, Greg KH wrote:
> On Thu, Mar 12, 2020 at 08:43:29PM +0100, Lourdes Pedrajas wrote:
> > On Thu, Mar 12, 2020 at 01:29:52PM +0100, Greg KH wrote:
> > > On Thu, Mar 12, 2020 at 01:15:48PM +0100, Lourdes Pedrajas wrote:
>
> If you are new to programming, I would focus on other things.
Thank for your advice.
> Concurrency and locks are not exactly "simple" things, but they are
> essencial to learn to be a kernel programmer.
I know, I am willing to learn it. Thanks for your patience!
>
> good luck!
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-12 20:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-12 12:15 [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile Lourdes Pedrajas
2020-03-12 12:29 ` Greg KH
2020-03-12 12:31 ` [Outreachy kernel] " Stefano Brivio
2020-03-12 19:43 ` Lourdes Pedrajas
2020-03-12 20:43 ` Greg KH
2020-03-12 20:57 ` Lourdes Pedrajas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.