| Message ID | 7743c426ae2c34573d59636d4d6cefaccdb00990.1600378070.git.gustavo.pimentel@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [-next] PCI: DWC: Fix cast truncates bits from constant value | expand |
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 >
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 > >
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 > > >
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
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 --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)
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(-)