| Message ID | 20250102111339.2233101-2-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v2] PCI: dwc: Fix W=1 build warning in dw_pcie_edma_irq_verify() | expand |
Can you make the subject say something about the fix instead of the warning? E.g., something about fixing a potential truncation? On Thu, Jan 02, 2025 at 12:13:40PM +0100, Niklas Cassel wrote: > Change dw_pcie_edma_irq_verify() to print the dma channel as %u. > > While a DWC glue driver could theoretically initialize nr_irqs to a > negative value, doing so would obviously be incorrect, and the later > dw_edma_probe(struct dw_edma_chip *chip) call would fail, since while > the dw_edma_probe() call expects the caller to initialize chip->nr_irqs, > dw_edma_probe() verifies nr_irqs and returns failure if nr_irqs is < 1. > > This fixes the following build warning when compiling with W=1: > > drivers/pci/controller/dwc/pcie-designware.c: In function ‘dw_pcie_edma_detect’: > drivers/pci/controller/dwc/pcie-designware.c:989:50: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 3 [-Wformat-truncation=] > 989 | snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > | ^~ > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > Changes since V1: > -Do not reject negative nr_irqs value in dw_pcie_edma_irq_verify(), > as this will already be done by dw_edma_probe(). > > 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 3c683b6119c3..0a13fb4336f4 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -986,7 +986,7 @@ static int dw_pcie_edma_irq_verify(struct dw_pcie *pci) > } > > for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) { > - snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > + snprintf(name, sizeof(name), "dma%u", pci->edma.nr_irqs); I don't understand this fix. I guess the warning is complaining that sizeof(name) == 6, and "dma" takes up three bytes, so the %d has to fit in the remaining region of size 3? But I don't see how printing nr_irqs as unsigned rather than signed is a fix, since even an unsigned int can be longer than 3 digits. And I don't like using "%u" for a signed value in order to "fix" something. That's asking for a future cleanup to revert the change. What's wrong with just making the name[] buffer big enough? > ret = platform_get_irq_byname_optional(pdev, name); > if (ret <= 0) > -- > 2.47.1 >
On Fri, Jan 03, 2025 at 04:10:56PM -0600, Bjorn Helgaas wrote: > Can you make the subject say something about the fix instead of the > warning? E.g., something about fixing a potential truncation? > > On Thu, Jan 02, 2025 at 12:13:40PM +0100, Niklas Cassel wrote: > > Change dw_pcie_edma_irq_verify() to print the dma channel as %u. > > > > While a DWC glue driver could theoretically initialize nr_irqs to a > > negative value, doing so would obviously be incorrect, and the later > > dw_edma_probe(struct dw_edma_chip *chip) call would fail, since while > > the dw_edma_probe() call expects the caller to initialize chip->nr_irqs, > > dw_edma_probe() verifies nr_irqs and returns failure if nr_irqs is < 1. > > > > This fixes the following build warning when compiling with W=1: > > > > drivers/pci/controller/dwc/pcie-designware.c: In function ‘dw_pcie_edma_detect’: > > drivers/pci/controller/dwc/pcie-designware.c:989:50: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 3 [-Wformat-truncation=] > > 989 | snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > > | ^~ > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > Changes since V1: > > -Do not reject negative nr_irqs value in dw_pcie_edma_irq_verify(), > > as this will already be done by dw_edma_probe(). > > > > 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 3c683b6119c3..0a13fb4336f4 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -986,7 +986,7 @@ static int dw_pcie_edma_irq_verify(struct dw_pcie *pci) > > } > > > > for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) { > > - snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > > + snprintf(name, sizeof(name), "dma%u", pci->edma.nr_irqs); > > I don't understand this fix. I guess the warning is complaining that > sizeof(name) == 6, and "dma" takes up three bytes, so the %d has to > fit in the remaining region of size 3? Yes. > > But I don't see how printing nr_irqs as unsigned rather than signed is > a fix, since even an unsigned int can be longer than 3 digits. You would need to ask GCC authors behind -Wformat-truncation why the warning only seems to care about %d. > > And I don't like using "%u" for a signed value in order to "fix" > something. That's asking for a future cleanup to revert the change. > Well, neither do I. V1 was a nicer solution IMO: https://lore.kernel.org/linux-pci/20241220072328.351329-2-cassel@kernel.org/ But that fix was rejected by another PCI maintainer. > What's wrong with just making the name[] buffer big enough? Sure, I'll do that in v3. Kind regards, Niklas
On Sat, Jan 04, 2025 at 01:19:48AM +0100, Niklas Cassel wrote: > On Fri, Jan 03, 2025 at 04:10:56PM -0600, Bjorn Helgaas wrote: > > Can you make the subject say something about the fix instead of the > > warning? E.g., something about fixing a potential truncation? > > > > On Thu, Jan 02, 2025 at 12:13:40PM +0100, Niklas Cassel wrote: > > > Change dw_pcie_edma_irq_verify() to print the dma channel as %u. > > > > > > While a DWC glue driver could theoretically initialize nr_irqs to a > > > negative value, doing so would obviously be incorrect, and the later > > > dw_edma_probe(struct dw_edma_chip *chip) call would fail, since while > > > the dw_edma_probe() call expects the caller to initialize chip->nr_irqs, > > > dw_edma_probe() verifies nr_irqs and returns failure if nr_irqs is < 1. > > > > > > This fixes the following build warning when compiling with W=1: > > > > > > drivers/pci/controller/dwc/pcie-designware.c: In function ‘dw_pcie_edma_detect’: > > > drivers/pci/controller/dwc/pcie-designware.c:989:50: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 3 [-Wformat-truncation=] > > > 989 | snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > > > | ^~ > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > --- > > > Changes since V1: > > > -Do not reject negative nr_irqs value in dw_pcie_edma_irq_verify(), > > > as this will already be done by dw_edma_probe(). > > > > > > 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 3c683b6119c3..0a13fb4336f4 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -986,7 +986,7 @@ static int dw_pcie_edma_irq_verify(struct dw_pcie *pci) > > > } > > > > > > for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) { > > > - snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); > > > + snprintf(name, sizeof(name), "dma%u", pci->edma.nr_irqs); > > > > I don't understand this fix. I guess the warning is complaining that > > sizeof(name) == 6, and "dma" takes up three bytes, so the %d has to > > fit in the remaining region of size 3? > > Yes. > > > > > > But I don't see how printing nr_irqs as unsigned rather than signed is > > a fix, since even an unsigned int can be longer than 3 digits. > > You would need to ask GCC authors behind -Wformat-truncation why the > warning only seems to care about %d. > > > > > > And I don't like using "%u" for a signed value in order to "fix" > > something. That's asking for a future cleanup to revert the change. > > > > Well, neither do I. V1 was a nicer solution IMO: > https://lore.kernel.org/linux-pci/20241220072328.351329-2-cassel@kernel.org/ > But that fix was rejected by another PCI maintainer. > TBH, my concern with v1 was adding negative value check for a signed variable which doesn't need to be signed. But even so, the fix doesn't fix the underlying issue but just the GCC warning. > > > What's wrong with just making the name[] buffer big enough? > > Sure, I'll do that in v3. > Yeah, this seems to be the right *fix*. - Mani
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 3c683b6119c3..0a13fb4336f4 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -986,7 +986,7 @@ static int dw_pcie_edma_irq_verify(struct dw_pcie *pci) } for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) { - snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); + snprintf(name, sizeof(name), "dma%u", pci->edma.nr_irqs); ret = platform_get_irq_byname_optional(pdev, name); if (ret <= 0)
Change dw_pcie_edma_irq_verify() to print the dma channel as %u. While a DWC glue driver could theoretically initialize nr_irqs to a negative value, doing so would obviously be incorrect, and the later dw_edma_probe(struct dw_edma_chip *chip) call would fail, since while the dw_edma_probe() call expects the caller to initialize chip->nr_irqs, dw_edma_probe() verifies nr_irqs and returns failure if nr_irqs is < 1. This fixes the following build warning when compiling with W=1: drivers/pci/controller/dwc/pcie-designware.c: In function ‘dw_pcie_edma_detect’: drivers/pci/controller/dwc/pcie-designware.c:989:50: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 3 [-Wformat-truncation=] 989 | snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); | ^~ Signed-off-by: Niklas Cassel <cassel@kernel.org> --- Changes since V1: -Do not reject negative nr_irqs value in dw_pcie_edma_irq_verify(), as this will already be done by dw_edma_probe(). drivers/pci/controller/dwc/pcie-designware.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)