diff mbox

[OPW,kernel] staging:slicoss: Fix Use of volatile is usually wrong in slic.h

Message ID 1412009694-5201-1-git-send-email-yeliztaneroglu@gmail.com
State New, archived
Headers show

Commit Message

Yeliz Taneroglu Sept. 29, 2014, 4:54 p.m. UTC
The following patch the checkpatch.pl warning:

drivers/staging/slicoss/slic.h:360:361:362 Warning: Use of volatile is
usually wrong

Signed-off-by: Yeliz Taneroglu <yeliztaneroglu@gmail.com>
---
 drivers/staging/slicoss/slic.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Greg Kroah-Hartman Sept. 29, 2014, 5:10 p.m. UTC | #1
On Mon, Sep 29, 2014 at 07:54:54PM +0300, Yeliz Taneroglu wrote:
> The following patch the checkpatch.pl warning:
> 
> drivers/staging/slicoss/slic.h:360:361:362 Warning: Use of volatile is
> usually wrong
> 
> Signed-off-by: Yeliz Taneroglu <yeliztaneroglu@gmail.com>
> ---
>  drivers/staging/slicoss/slic.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
> index 3a5aa88..3d875c6 100644
> --- a/drivers/staging/slicoss/slic.h
> +++ b/drivers/staging/slicoss/slic.h
> @@ -357,9 +357,9 @@ struct base_driver {
>  };
>  
>  struct slic_shmem {
> -	volatile u32          isr;
> -	volatile u32          linkstatus;
> -	volatile struct slic_stats     inicstats;
> +	u32          isr;
> +	u32          linkstatus;
> +	struct slic_stats     inicstats;
>  };

It is "usually" wrong, have you ensured that it really is wrong here?
What lock protects these values now?  Not that volatile was "protecting"
anything really before, but just removing the word is not always
correct, I need to see some kind of validation here before being able to
accept this.

thanks,

greg k-h
Daniel Vetter Nov. 3, 2014, 10:49 p.m. UTC | #2
On Mon, Sep 29, 2014 at 01:10:04PM -0400, Greg KH wrote:
> On Mon, Sep 29, 2014 at 07:54:54PM +0300, Yeliz Taneroglu wrote:
> > The following patch the checkpatch.pl warning:
> > 
> > drivers/staging/slicoss/slic.h:360:361:362 Warning: Use of volatile is
> > usually wrong
> > 
> > Signed-off-by: Yeliz Taneroglu <yeliztaneroglu@gmail.com>
> > ---
> >  drivers/staging/slicoss/slic.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
> > index 3a5aa88..3d875c6 100644
> > --- a/drivers/staging/slicoss/slic.h
> > +++ b/drivers/staging/slicoss/slic.h
> > @@ -357,9 +357,9 @@ struct base_driver {
> >  };
> >  
> >  struct slic_shmem {
> > -	volatile u32          isr;
> > -	volatile u32          linkstatus;
> > -	volatile struct slic_stats     inicstats;
> > +	u32          isr;
> > +	u32          linkstatus;
> > +	struct slic_stats     inicstats;
> >  };
> 
> It is "usually" wrong, have you ensured that it really is wrong here?
> What lock protects these values now?  Not that volatile was "protecting"
> anything really before, but just removing the word is not always
> correct, I need to see some kind of validation here before being able to
> accept this.

I've made the error of trying to understand this. It's a
pci_alloc_coherent chunk of memory, where the dma_addr_t is cast to a cpu
pointer and used locally. Then that cpu pointers gets diced up by macros
and shovelled into hw registers, so I think (minor loosing the high bits
on 32bit archs) this might actually work. At that point I've stopped
reading, so I have no clue what actually needs to be all fixed.

But it looks like major surgery, so perhaps add it to the TODO?
-Daniel
Greg Kroah-Hartman Nov. 3, 2014, 11:06 p.m. UTC | #3
On Mon, Nov 03, 2014 at 11:49:20PM +0100, Daniel Vetter wrote:
> On Mon, Sep 29, 2014 at 01:10:04PM -0400, Greg KH wrote:
> > On Mon, Sep 29, 2014 at 07:54:54PM +0300, Yeliz Taneroglu wrote:
> > > The following patch the checkpatch.pl warning:
> > > 
> > > drivers/staging/slicoss/slic.h:360:361:362 Warning: Use of volatile is
> > > usually wrong
> > > 
> > > Signed-off-by: Yeliz Taneroglu <yeliztaneroglu@gmail.com>
> > > ---
> > >  drivers/staging/slicoss/slic.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
> > > index 3a5aa88..3d875c6 100644
> > > --- a/drivers/staging/slicoss/slic.h
> > > +++ b/drivers/staging/slicoss/slic.h
> > > @@ -357,9 +357,9 @@ struct base_driver {
> > >  };
> > >  
> > >  struct slic_shmem {
> > > -	volatile u32          isr;
> > > -	volatile u32          linkstatus;
> > > -	volatile struct slic_stats     inicstats;
> > > +	u32          isr;
> > > +	u32          linkstatus;
> > > +	struct slic_stats     inicstats;
> > >  };
> > 
> > It is "usually" wrong, have you ensured that it really is wrong here?
> > What lock protects these values now?  Not that volatile was "protecting"
> > anything really before, but just removing the word is not always
> > correct, I need to see some kind of validation here before being able to
> > accept this.
> 
> I've made the error of trying to understand this. It's a
> pci_alloc_coherent chunk of memory, where the dma_addr_t is cast to a cpu
> pointer and used locally. Then that cpu pointers gets diced up by macros
> and shovelled into hw registers, so I think (minor loosing the high bits
> on 32bit archs) this might actually work. At that point I've stopped
> reading, so I have no clue what actually needs to be all fixed.
> 
> But it looks like major surgery, so perhaps add it to the TODO?

Feel free to send a patch :)

Or just delete the driver, odds are no one is even using it...
Daniel Vetter Nov. 3, 2014, 11:24 p.m. UTC | #4
On Mon, Nov 03, 2014 at 03:06:26PM -0800, Greg KH wrote:
> On Mon, Nov 03, 2014 at 11:49:20PM +0100, Daniel Vetter wrote:
> > On Mon, Sep 29, 2014 at 01:10:04PM -0400, Greg KH wrote:
> > > On Mon, Sep 29, 2014 at 07:54:54PM +0300, Yeliz Taneroglu wrote:
> > > > The following patch the checkpatch.pl warning:
> > > > 
> > > > drivers/staging/slicoss/slic.h:360:361:362 Warning: Use of volatile is
> > > > usually wrong
> > > > 
> > > > Signed-off-by: Yeliz Taneroglu <yeliztaneroglu@gmail.com>
> > > > ---
> > > >  drivers/staging/slicoss/slic.h | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
> > > > index 3a5aa88..3d875c6 100644
> > > > --- a/drivers/staging/slicoss/slic.h
> > > > +++ b/drivers/staging/slicoss/slic.h
> > > > @@ -357,9 +357,9 @@ struct base_driver {
> > > >  };
> > > >  
> > > >  struct slic_shmem {
> > > > -	volatile u32          isr;
> > > > -	volatile u32          linkstatus;
> > > > -	volatile struct slic_stats     inicstats;
> > > > +	u32          isr;
> > > > +	u32          linkstatus;
> > > > +	struct slic_stats     inicstats;
> > > >  };
> > > 
> > > It is "usually" wrong, have you ensured that it really is wrong here?
> > > What lock protects these values now?  Not that volatile was "protecting"
> > > anything really before, but just removing the word is not always
> > > correct, I need to see some kind of validation here before being able to
> > > accept this.
> > 
> > I've made the error of trying to understand this. It's a
> > pci_alloc_coherent chunk of memory, where the dma_addr_t is cast to a cpu
> > pointer and used locally. Then that cpu pointers gets diced up by macros
> > and shovelled into hw registers, so I think (minor loosing the high bits
> > on 32bit archs) this might actually work. At that point I've stopped
> > reading, so I have no clue what actually needs to be all fixed.
> > 
> > But it looks like major surgery, so perhaps add it to the TODO?
> 
> Feel free to send a patch :)
> 
> Or just delete the driver, odds are no one is even using it...

So a bit of git blaming and git log shows that since

commit 1a92e82a86556727da1626393f2a6becf7e62f39
Author: Stephen Hemminger <shemminger@vyatta.com>
Date:   Wed Apr 15 16:52:16 2009 -0700

    staging: slicoss: update README
    
    I looked, I gagged, I left
    
    Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

(which added all the substantial parts of the TODO) no one really worked
on the fundamental issues of this driver. The only todo item that was
addressed is removed all the useless debug code the driver had. So yeah I
vote to remove this one. But it's really late here and I don't have
staging-testing handy to do the patch.

Yeliz (or anyone else really), feel like looking _really_ good on this
kernels diffstat by binning this driver?

If you do so please Cc: the people who might be interested in the driver
using scripts/get_maintainers.pl on your patch. And you can cite this mail
in the commit message as justification for removing staging/slicoss.

Cheers, Daniel
diff mbox

Patch

diff --git a/drivers/staging/slicoss/slic.h b/drivers/staging/slicoss/slic.h
index 3a5aa88..3d875c6 100644
--- a/drivers/staging/slicoss/slic.h
+++ b/drivers/staging/slicoss/slic.h
@@ -357,9 +357,9 @@  struct base_driver {
 };
 
 struct slic_shmem {
-	volatile u32          isr;
-	volatile u32          linkstatus;
-	volatile struct slic_stats     inicstats;
+	u32          isr;
+	u32          linkstatus;
+	struct slic_stats     inicstats;
 };
 
 struct slic_upr {