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

[Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>]

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
> >