Re: [Outreachy kernel] [PATCH] staging: mt7621-dma: mtk-hsdma: make a variable non-volatile

[Lourdes Pedrajas <lu@pplo.net>]

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