[+cc Vignesh for dra7xx snprintf issue and CONFIG_TI_PIPE3 question]
On Wed, Jan 10, 2024 at 08:16:33AM +0100, Sergio Paracuellos wrote:
> On Wed, Jan 10, 2024 at 12:51 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > FYI:
> >
> > $ make W=1 drivers/pci/
> > CC drivers/pci/controller/pcie-mt7621.o
> > drivers/pci/controller/pcie-mt7621.c: In function ‘mt7621_pcie_probe’:
> > drivers/pci/controller/pcie-mt7621.c:228:49: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
> > 228 | snprintf(name, sizeof(name), "pcie-phy%d", slot);
> > | ^
> > drivers/pci/controller/pcie-mt7621.c:228:9: note: ‘snprintf’ output between 10 and 11 bytes into a destination of size 10
> > 228 | snprintf(name, sizeof(name), "pcie-phy%d", slot);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
>
> Would you be happy if I just increment the buffer as follows?
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c
> b/drivers/pci/controller/pcie-mt7621.c
> index 79e225edb42a..d97b956e6e57 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -202,7 +202,7 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> struct mt7621_pcie_port *port;
> struct device *dev = pcie->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - char name[10];
> + char name[11];
> int err;
>
> port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>
> Or should I use scnprintf instead? Since the statement is not using
> function return value at all snprintf looks more correct and simpler
> at a first glance.
I don't know enough to have an opinion, but grep says all similar
cases in drivers/pci/ use snprintf(), so I guess I would follow the
crowd. If there's an argument for scnprintf() instead, we can convert
them all at once.
> diff --git a/drivers/pci/controller/pcie-mt7621.c
> b/drivers/pci/controller/pcie-mt7621.c
> index 79e225edb42a..0eae1b5b079e 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -225,7 +225,7 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> return PTR_ERR(port->pcie_rst);
> }
>
> - snprintf(name, sizeof(name), "pcie-phy%d", slot);
> + scnprintf(name, sizeof(name), "pcie-phy%d", slot);
> port->phy = devm_of_phy_get(dev, node, name);
> if (IS_ERR(port->phy)) {
> dev_err(dev, "failed to get pcie-phy%d\n", slot);
>
> Both of them silence the warning, so let me know your preference here.
>
> > I know we'll never actually hit this, but it'd be nice to clean this
> > up, and I don't think it would really cost us anything. I think it's
> > currently the only "W=1" warning in drivers/pci/.
>
> I am also getting this:
>
> drivers/pci/controller/dwc/pci-dra7xx.c: In function ‘dra7xx_pcie_probe’:
> drivers/pci/controller/dwc/pci-dra7xx.c:754:41: error: ‘%d’ directive
> output may be truncated writing between 1 and 10 bytes into a region
> of size 2 [-Werror=format-truncation=]
> 754 | snprintf(name, sizeof(name), "pcie-phy%d", i);
> | ^~
> drivers/pci/controller/dwc/pci-dra7xx.c:754:32: note: directive
> argument in the range [0, 2147483646]
> 754 | snprintf(name, sizeof(name), "pcie-phy%d", i);
> | ^~~~~~~~~~~~
> drivers/pci/controller/dwc/pci-dra7xx.c:754:3: note: ‘snprintf’ output
> between 10 and 19 bytes into a destination of size 10
> 754 | snprintf(name, sizeof(name), "pcie-phy%d", i);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Oh, thanks for this. I didn't have CONFIG_TI_PIPE3=y in my .config,
which CONFIG_PCI_DRA7XX depends on.
I didn't go to the trouble of trying to figure out exactly what
CONFIG_TI_PIPE3=y enables, but with the patch below, I *was* able to
successfully build and link a kernel with:
CONFIG_COMPILE_TEST=y
CONFIG_PCI_DRA7XX=y
CONFIG_PCI_DRA7XX_HOST=y
CONFIG_PCI_DRA7XX_EP=y
# CONFIG_TI_PIPE3 is not set
So maybe there's a way to write these dependencies in a way that would
give us better compile-testing?
Bjorn
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 5ac021dbd46a..8b837b183981 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -376,7 +376,7 @@ config PCI_DRA7XX
config PCI_DRA7XX_HOST
tristate "TI DRA7xx PCIe controller (host mode)"
depends on SOC_DRA7XX || COMPILE_TEST
- depends on OF && HAS_IOMEM && TI_PIPE3
+ depends on OF && HAS_IOMEM
depends on PCI_MSI
select PCIE_DW_HOST
select PCI_DRA7XX
@@ -392,7 +392,7 @@ config PCI_DRA7XX_HOST
config PCI_DRA7XX_EP
tristate "TI DRA7xx PCIe controller (endpoint mode)"
depends on SOC_DRA7XX || COMPILE_TEST
- depends on OF && HAS_IOMEM && TI_PIPE3
+ depends on OF && HAS_IOMEM
depends on PCI_ENDPOINT
select PCIE_DW_EP
select PCI_DRA7XX
