mt7621 static check warning

mt7621 static check warning

[Bjorn Helgaas]

Hi Sergio,

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);
	|         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

Bjorn

Re: mt7621 static check warning

[Sergio Paracuellos]

Hi Bjorn,

Thanks for the report.

On Wed, Jan 10, 2024 at 12:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Hi Sergio,
>
> 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.

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);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Best regards,
    Sergio Paracuellos

>
> Bjorn

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