* [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.