diff mbox series

[-next] PCI: DWC: Fix cast truncates bits from constant value

Message ID 7743c426ae2c34573d59636d4d6cefaccdb00990.1600378070.git.gustavo.pimentel@synopsys.com
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [-next] PCI: DWC: Fix cast truncates bits from constant value | expand

Commit Message

Gustavo Pimentel Sept. 17, 2020, 9:28 p.m. UTC
Fixes warning given by executing "make C=2 drivers/pci/"

Sparse output:
CHECK drivers/pci/controller/dwc/pcie-designware.c
 drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
 cast truncates bits from constant value (ffffffff7fffffff becomes
 7fffffff)

Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Joao Pinto <jpinto@synopsys.com>
Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bjorn Helgaas Sept. 17, 2020, 9:47 p.m. UTC | #1
On Thu, Sep 17, 2020 at 11:28:03PM +0200, Gustavo Pimentel wrote:
> Fixes warning given by executing "make C=2 drivers/pci/"
> 
> Sparse output:
> CHECK drivers/pci/controller/dwc/pcie-designware.c
>  drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
>  cast truncates bits from constant value (ffffffff7fffffff becomes
>  7fffffff)
> 
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 3c3a4d1..dcb7108 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -429,7 +429,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
>  	}
>  
>  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);

But this cure is worse than the disease.  If this is the only way to
fix the warning, I think I'd rather see the warning ;)  I'm hopeful
there's a nicer way, but I'm not a language lawyer.

>  }
>  
>  int dw_pcie_wait_for_link(struct dw_pcie *pci)
> -- 
> 2.7.4
>
Gustavo Pimentel Sept. 18, 2020, 7:15 a.m. UTC | #2
On Thu, Sep 17, 2020 at 22:47:59, Bjorn Helgaas <helgaas@kernel.org> 
wrote:

> On Thu, Sep 17, 2020 at 11:28:03PM +0200, Gustavo Pimentel wrote:
> > Fixes warning given by executing "make C=2 drivers/pci/"
> > 
> > Sparse output:
> > CHECK drivers/pci/controller/dwc/pcie-designware.c
> >  drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
> >  cast truncates bits from constant value (ffffffff7fffffff becomes
> >  7fffffff)
> > 
> > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Joao Pinto <jpinto@synopsys.com>
> > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 3c3a4d1..dcb7108 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -429,7 +429,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> >  	}
> >  
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);
> 
> But this cure is worse than the disease.  If this is the only way to
> fix the warning, I think I'd rather see the warning ;)  I'm hopeful
> there's a nicer way, but I'm not a language lawyer.

I don't like it either, I tried to see if were another way a clean way 
that didn't imply creating a temporary variable, but I didn't found.
The issue here is that PCIE_ATU_ENABLE is defined as BIT(31) on 
pcie-designware.h. The macro BIT changes its size from u32 to u64 
according to the architecture and by inverting the value on the 64 bits 
architecture causes the value to be transformed into 0xffffffff7fffffff.

The other possibility implies the creation of a temporary u32 variable to 
overcome this issue. It's a little bit overkill, but please share your 
thoughts about it.

void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
                         enum dw_pcie_region_type type)
 {
-       int region;
+       u32 atu = PCIE_ATU_ENABLE;
+       u32 region;

        switch (type) {
        case DW_PCIE_REGION_INBOUND:
@@ -429,7 +430,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int 
index,
        }

        dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
-       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
+       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~atu);
 }

> 
> >  }
> >  
> >  int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > -- 
> > 2.7.4
> >
Gustavo Pimentel Sept. 21, 2020, 11:30 a.m. UTC | #3
On Fri, Sep 18, 2020 at 8:15:48, Gustavo Pimentel <gustavo@synopsys.com> 
wrote:

> On Thu, Sep 17, 2020 at 22:47:59, Bjorn Helgaas <helgaas@kernel.org> 
> wrote:
> 
> > On Thu, Sep 17, 2020 at 11:28:03PM +0200, Gustavo Pimentel wrote:
> > > Fixes warning given by executing "make C=2 drivers/pci/"
> > > 
> > > Sparse output:
> > > CHECK drivers/pci/controller/dwc/pcie-designware.c
> > >  drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
> > >  cast truncates bits from constant value (ffffffff7fffffff becomes
> > >  7fffffff)
> > > 
> > > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Joao Pinto <jpinto@synopsys.com>
> > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 3c3a4d1..dcb7108 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -429,7 +429,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > >  	}
> > >  
> > >  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);
> > 
> > But this cure is worse than the disease.  If this is the only way to
> > fix the warning, I think I'd rather see the warning ;)  I'm hopeful
> > there's a nicer way, but I'm not a language lawyer.
> 
> I don't like it either, I tried to see if were another way a clean way 
> that didn't imply creating a temporary variable, but I didn't found.
> The issue here is that PCIE_ATU_ENABLE is defined as BIT(31) on 
> pcie-designware.h. The macro BIT changes its size from u32 to u64 
> according to the architecture and by inverting the value on the 64 bits 
> architecture causes the value to be transformed into 0xffffffff7fffffff.
> 
> The other possibility implies the creation of a temporary u32 variable to 
> overcome this issue. It's a little bit overkill, but please share your 
> thoughts about it.
> 
> void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
>                          enum dw_pcie_region_type type)
>  {
> -       int region;
> +       u32 atu = PCIE_ATU_ENABLE;
> +       u32 region;
> 
>         switch (type) {
>         case DW_PCIE_REGION_INBOUND:
> @@ -429,7 +430,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int 
> index,
>         }
> 
>         dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> -       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> +       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~atu);
>  }

Hi Bjorn,

What is your verdict on this?
If you prefer this approach I can send the corresponding patch.

-Gustavo

> 
> > 
> > >  }
> > >  
> > >  int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > -- 
> > > 2.7.4
> > >
Bjorn Helgaas Sept. 21, 2020, 4:36 p.m. UTC | #4
On Mon, Sep 21, 2020 at 11:30:48AM +0000, Gustavo Pimentel wrote:
> On Fri, Sep 18, 2020 at 8:15:48, Gustavo Pimentel <gustavo@synopsys.com> 
> wrote:
> 
> > On Thu, Sep 17, 2020 at 22:47:59, Bjorn Helgaas <helgaas@kernel.org> 
> > wrote:
> > 
> > > On Thu, Sep 17, 2020 at 11:28:03PM +0200, Gustavo Pimentel wrote:
> > > > Fixes warning given by executing "make C=2 drivers/pci/"
> > > > 
> > > > Sparse output:
> > > > CHECK drivers/pci/controller/dwc/pcie-designware.c
> > > >  drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
> > > >  cast truncates bits from constant value (ffffffff7fffffff becomes
> > > >  7fffffff)
> > > > 
> > > > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Joao Pinto <jpinto@synopsys.com>
> > > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 3c3a4d1..dcb7108 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -429,7 +429,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > > >  	}
> > > >  
> > > >  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > > > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > > > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);
> > > 
> > > But this cure is worse than the disease.  If this is the only way to
> > > fix the warning, I think I'd rather see the warning ;)  I'm hopeful
> > > there's a nicer way, but I'm not a language lawyer.
> > 
> > I don't like it either, I tried to see if were another way a clean way 
> > that didn't imply creating a temporary variable, but I didn't found.
> > The issue here is that PCIE_ATU_ENABLE is defined as BIT(31) on 
> > pcie-designware.h. The macro BIT changes its size from u32 to u64 
> > according to the architecture and by inverting the value on the 64 bits 
> > architecture causes the value to be transformed into 0xffffffff7fffffff.
> > 
> > The other possibility implies the creation of a temporary u32 variable to 
> > overcome this issue. It's a little bit overkill, but please share your 
> > thoughts about it.
> > 
> > void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> >                          enum dw_pcie_region_type type)
> >  {
> > -       int region;
> > +       u32 atu = PCIE_ATU_ENABLE;
> > +       u32 region;
> > 
> >         switch (type) {
> >         case DW_PCIE_REGION_INBOUND:
> > @@ -429,7 +430,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int 
> > index,
> >         }
> > 
> >         dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > -       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > +       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~atu);
> >  }
> 
> Hi Bjorn,
> 
> What is your verdict on this?
> If you prefer this approach I can send the corresponding patch.

Having a u32 temporary with no obvious reason for existence is kind of
unpleasant, too.  Surely this is a common situation?  Maybe other
instances just live with the warning, too?

I'd say leave this alone for now.

Bjorn
Gustavo Pimentel Sept. 22, 2020, 9:57 a.m. UTC | #5
On Mon, Sep 21, 2020 at 17:36:24, Bjorn Helgaas <helgaas@kernel.org> 
wrote:

> On Mon, Sep 21, 2020 at 11:30:48AM +0000, Gustavo Pimentel wrote:
> > On Fri, Sep 18, 2020 at 8:15:48, Gustavo Pimentel <gustavo@synopsys.com> 
> > wrote:
> > 
> > > On Thu, Sep 17, 2020 at 22:47:59, Bjorn Helgaas <helgaas@kernel.org> 
> > > wrote:
> > > 
> > > > On Thu, Sep 17, 2020 at 11:28:03PM +0200, Gustavo Pimentel wrote:
> > > > > Fixes warning given by executing "make C=2 drivers/pci/"
> > > > > 
> > > > > Sparse output:
> > > > > CHECK drivers/pci/controller/dwc/pcie-designware.c
> > > > >  drivers/pci/controller/dwc/pcie-designware.c:432:52: warning:
> > > > >  cast truncates bits from constant value (ffffffff7fffffff becomes
> > > > >  7fffffff)
> > > > > 
> > > > > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > Cc: Joao Pinto <jpinto@synopsys.com>
> > > > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 3c3a4d1..dcb7108 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -429,7 +429,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > > > >  	}
> > > > >  
> > > > >  	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > > > > -	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > > > > +	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);
> > > > 
> > > > But this cure is worse than the disease.  If this is the only way to
> > > > fix the warning, I think I'd rather see the warning ;)  I'm hopeful
> > > > there's a nicer way, but I'm not a language lawyer.
> > > 
> > > I don't like it either, I tried to see if were another way a clean way 
> > > that didn't imply creating a temporary variable, but I didn't found.
> > > The issue here is that PCIE_ATU_ENABLE is defined as BIT(31) on 
> > > pcie-designware.h. The macro BIT changes its size from u32 to u64 
> > > according to the architecture and by inverting the value on the 64 bits 
> > > architecture causes the value to be transformed into 0xffffffff7fffffff.
> > > 
> > > The other possibility implies the creation of a temporary u32 variable to 
> > > overcome this issue. It's a little bit overkill, but please share your 
> > > thoughts about it.
> > > 
> > > void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > >                          enum dw_pcie_region_type type)
> > >  {
> > > -       int region;
> > > +       u32 atu = PCIE_ATU_ENABLE;
> > > +       u32 region;
> > > 
> > >         switch (type) {
> > >         case DW_PCIE_REGION_INBOUND:
> > > @@ -429,7 +430,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int 
> > > index,
> > >         }
> > > 
> > >         dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
> > > -       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
> > > +       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~atu);
> > >  }
> > 
> > Hi Bjorn,
> > 
> > What is your verdict on this?
> > If you prefer this approach I can send the corresponding patch.
> 
> Having a u32 temporary with no obvious reason for existence is kind of
> unpleasant, too.  Surely this is a common situation?  Maybe other
> instances just live with the warning, too?
> 
> I'd say leave this alone for now.

Hi Bjorn,

After resting about this topic, I found a easy and clean solution that 
will please for both of us.

-       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
+       dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);

Basically I changed the cast instruction position, this way the 
PCIE_ATU_ENABLE flag defined BIT(31) on a 64 bits architecture will be 
casted into a u32 and the binary one's complement instruction will act 
upon a 32bits variable, avoiding that error.

I'll send the patch for it right away.

-Gustavo

> 
> Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 3c3a4d1..dcb7108 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -429,7 +429,7 @@  void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
 	}
 
 	dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
-	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~PCIE_ATU_ENABLE);
+	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, (u32)~0 & ~PCIE_ATU_ENABLE);
 }
 
 int dw_pcie_wait_for_link(struct dw_pcie *pci)