Re: mt7621 static check warning

Re: mt7621 static check warning

[Bjorn Helgaas]

[+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@kernel.org> 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