Re: [Outreachy kernel] Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock
On Mon, Sep 11, 2017 at 12:08 PM, Srishti Sharma wrote: > On Mon, Sep 11, 2017 at 9:34 PM, Greg KH wrote: >> On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote: >>> The use of volatile for the variable monitor_lock is unnecessary. >>> >>> Signed-off-by: Srishti Sharma >>> --- >>> drivers/staging/ccree/ssi_request_mgr.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/staging/ccree/ssi_request_mgr.c >>> b/drivers/staging/ccree/ssi_request_mgr.c >>> index e5c2f92..7d77941 100644 >>> --- a/drivers/staging/ccree/ssi_request_mgr.c >>> +++ b/drivers/staging/ccree/ssi_request_mgr.c >>> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle { >>> dma_addr_t dummy_comp_buff_dma; >>> struct cc_hw_desc monitor_desc; >>> >>> - volatile unsigned long monitor_lock; >>> + unsigned long monitor_lock; >> >> While volatile is not right, odds are, this is still totally wrong as >> well. How about using a "real" lock instead? > > I tried to find where is this variable being used in the code, but I > didn't find any usage of it . It might be an important attribute of > this structure definition but, I don't see it's value being set to > anything or being used somewhere . > AFAICT, it's not used. Your patch should just remove it instead :) Sean > Regards, > Srishti >> >> thanks, >> >> greg k-h > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/CAB3L5oxcyhgyy8EuGuPo9QtJQd-W7JTgQQE1PfopZFmSx58P9g%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock
On Mon, 11 Sep 2017, Srishti Sharma wrote: > On Mon, Sep 11, 2017 at 9:34 PM, Greg KH wrote: > > On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote: > >> The use of volatile for the variable monitor_lock is unnecessary. > >> > >> Signed-off-by: Srishti Sharma > >> --- > >> drivers/staging/ccree/ssi_request_mgr.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/ccree/ssi_request_mgr.c > >> b/drivers/staging/ccree/ssi_request_mgr.c > >> index e5c2f92..7d77941 100644 > >> --- a/drivers/staging/ccree/ssi_request_mgr.c > >> +++ b/drivers/staging/ccree/ssi_request_mgr.c > >> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle { > >> dma_addr_t dummy_comp_buff_dma; > >> struct cc_hw_desc monitor_desc; > >> > >> - volatile unsigned long monitor_lock; > >> + unsigned long monitor_lock; > > > > While volatile is not right, odds are, this is still totally wrong as > > well. How about using a "real" lock instead? > > I tried to find where is this variable being used in the code, but I > didn't find any usage of it . It might be an important attribute of > this structure definition but, I don't see it's value being set to > anything or being used somewhere . Try removing it and see if the code still compiles. There is always a danger that a use of something could be constructed using ## in a macro, although given the uses of ## for this driver, it doesn't seem likely here. julia
Re: [Outreachy kernel] Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock
On Mon, Sep 11, 2017 at 9:41 PM, Julia Lawall wrote: > > > On Mon, 11 Sep 2017, Srishti Sharma wrote: > >> On Mon, Sep 11, 2017 at 9:34 PM, Greg KH wrote: >> > On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote: >> >> The use of volatile for the variable monitor_lock is unnecessary. >> >> >> >> Signed-off-by: Srishti Sharma >> >> --- >> >> drivers/staging/ccree/ssi_request_mgr.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/staging/ccree/ssi_request_mgr.c >> >> b/drivers/staging/ccree/ssi_request_mgr.c >> >> index e5c2f92..7d77941 100644 >> >> --- a/drivers/staging/ccree/ssi_request_mgr.c >> >> +++ b/drivers/staging/ccree/ssi_request_mgr.c >> >> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle { >> >> dma_addr_t dummy_comp_buff_dma; >> >> struct cc_hw_desc monitor_desc; >> >> >> >> - volatile unsigned long monitor_lock; >> >> + unsigned long monitor_lock; >> > >> > While volatile is not right, odds are, this is still totally wrong as >> > well. How about using a "real" lock instead? >> >> I tried to find where is this variable being used in the code, but I >> didn't find any usage of it . It might be an important attribute of >> this structure definition but, I don't see it's value being set to >> anything or being used somewhere . > > Try removing it and see if the code still compiles. There is always a > danger that a use of something could be constructed using ## in a macro, > although given the uses of ## for this driver, it doesn't seem likely > here. Yes, I'll do that. Regards, Srishti > > julia
Re: [Outreachy kernel] Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock
On Mon, Sep 11, 2017 at 9:45 PM, Srishti Sharma wrote: > On Mon, Sep 11, 2017 at 9:41 PM, Julia Lawall wrote: >> >> >> On Mon, 11 Sep 2017, Srishti Sharma wrote: >> >>> On Mon, Sep 11, 2017 at 9:34 PM, Greg KH wrote: >>> > On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote: >>> >> The use of volatile for the variable monitor_lock is unnecessary. >>> >> >>> >> Signed-off-by: Srishti Sharma >>> >> --- >>> >> drivers/staging/ccree/ssi_request_mgr.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/drivers/staging/ccree/ssi_request_mgr.c >>> >> b/drivers/staging/ccree/ssi_request_mgr.c >>> >> index e5c2f92..7d77941 100644 >>> >> --- a/drivers/staging/ccree/ssi_request_mgr.c >>> >> +++ b/drivers/staging/ccree/ssi_request_mgr.c >>> >> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle { >>> >> dma_addr_t dummy_comp_buff_dma; >>> >> struct cc_hw_desc monitor_desc; >>> >> >>> >> - volatile unsigned long monitor_lock; >>> >> + unsigned long monitor_lock; >>> > >>> > While volatile is not right, odds are, this is still totally wrong as >>> > well. How about using a "real" lock instead? >>> >>> I tried to find where is this variable being used in the code, but I >>> didn't find any usage of it . It might be an important attribute of >>> this structure definition but, I don't see it's value being set to >>> anything or being used somewhere . >> >> Try removing it and see if the code still compiles. There is always a >> danger that a use of something could be constructed using ## in a macro, >> although given the uses of ## for this driver, it doesn't seem likely >> here. It compiles, so I have removed the variable and sent another patch Thanks, Srishti > > Yes, I'll do that. > > Regards, > Srishti >> >> julia