[PATCH 1/3] PCI: tegra: Fix OF node reference leak

[PATCH 1/3] PCI: tegra: Fix OF node reference leak

[Christophe JAILLET]

Commit 9e38e690ace3 ("PCI: tegra: Fix OF node reference leak") has fixed
some node reference leaks in this function but missed some of them.

In fact, having 'port' referenced in the 'rp' structure is not enough to
prevent the leak, until 'rp' is actually added in the 'pcie->ports' list.

Add the missing 'goto err_node_put' accordingly.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/pci/controller/pci-tegra.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 8069bd9232d4..006bf0346dec 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2193,13 +2193,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		rp->np = port;
 
 		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
-		if (IS_ERR(rp->base))
-			return PTR_ERR(rp->base);
+		if (IS_ERR(rp->base)) {
+			err = PTR_ERR(rp->base);
+			goto err_node_put;
+		}
 
 		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
 		if (!label) {
-			dev_err(dev, "failed to create reset GPIO label\n");
-			return -ENOMEM;
+			err = -ENOMEM;
+			goto err_node_put;
 		}
 
 		/*
@@ -2217,7 +2219,8 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 			} else {
 				dev_err(dev, "failed to get reset GPIO: %ld\n",
 					PTR_ERR(rp->reset_gpio));
-				return PTR_ERR(rp->reset_gpio);
+				err = PTR_ERR(rp->reset_gpio);
+				goto err_node_put;
 			}
 		}
 
-- 
2.30.2

[PATCH 2/3] PCI: tegra: Use 'seq_puts' instead of 'seq_printf'

[Christophe JAILLET]

As spotted by checkpatch, use 'seq_puts' instead of 'seq_printf' when
possible.
It is slightly more efficient.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/pci/controller/pci-tegra.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 006bf0346dec..fe8e21ce3e3d 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -2550,7 +2550,7 @@ static void *tegra_pcie_ports_seq_start(struct seq_file *s, loff_t *pos)
 	if (list_empty(&pcie->ports))
 		return NULL;
 
-	seq_printf(s, "Index  Status\n");
+	seq_puts(s, "Index  Status\n");
 
 	return seq_list_start(&pcie->ports, *pos);
 }
@@ -2587,16 +2587,16 @@ static int tegra_pcie_ports_seq_show(struct seq_file *s, void *v)
 	seq_printf(s, "%2u     ", port->index);
 
 	if (up)
-		seq_printf(s, "up");
+		seq_puts(s, "up");
 
 	if (active) {
 		if (up)
-			seq_printf(s, ", ");
+			seq_puts(s, ", ");
 
-		seq_printf(s, "active");
+		seq_puts(s, "active");
 	}
 
-	seq_printf(s, "\n");
+	seq_puts(s, "\n");
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 2/3] PCI: tegra: Use 'seq_puts' instead of 'seq_printf'

[Vidya Sagar]

Reviewed-by: Vidya Sagar <vidyas@nvidia.com>

On 5/4/2021 10:47 PM, Christophe JAILLET wrote:
> As spotted by checkpatch, use 'seq_puts' instead of 'seq_printf' when
> possible.
> It is slightly more efficient.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/pci/controller/pci-tegra.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 006bf0346dec..fe8e21ce3e3d 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2550,7 +2550,7 @@ static void *tegra_pcie_ports_seq_start(struct seq_file *s, loff_t *pos)
>   	if (list_empty(&pcie->ports))
>   		return NULL;
>   
> -	seq_printf(s, "Index  Status\n");
> +	seq_puts(s, "Index  Status\n");
>   
>   	return seq_list_start(&pcie->ports, *pos);
>   }
> @@ -2587,16 +2587,16 @@ static int tegra_pcie_ports_seq_show(struct seq_file *s, void *v)
>   	seq_printf(s, "%2u     ", port->index);
>   
>   	if (up)
> -		seq_printf(s, "up");
> +		seq_puts(s, "up");
>   
>   	if (active) {
>   		if (up)
> -			seq_printf(s, ", ");
> +			seq_puts(s, ", ");
>   
> -		seq_printf(s, "active");
> +		seq_puts(s, "active");
>   	}
>   
> -	seq_printf(s, "\n");
> +	seq_puts(s, "\n");
>   	return 0;
>   }
>   
> 

[PATCH 3/3] PCI: tegra: make const array err_msg static

[Christophe JAILLET]

Don't populate the array err_msg on the stack but instead make it
static. Makes the object code smaller by 64 bytes.

While at it, add a missing const, as reported by checkpatch.

Compiled with gcc 11.0.1

Before:
$ size drivers/pci/controller/pci-tegra.o
   text	   data	    bss	    dec	    hex	filename
  25623	   2844	     32	  28499	   6f53	drivers/pci/controller/pci-tegra.o

After:
$ size drivers/pci/controller/pci-tegra.o
   text	   data	    bss	    dec	    hex	filename
  25559	   2844	     32	  28435	   6f13	drivers/pci/controller/pci-tegra.o

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/pci/controller/pci-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index fe8e21ce3e3d..b1250b15d290 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -764,7 +764,7 @@ static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 
 static irqreturn_t tegra_pcie_isr(int irq, void *arg)
 {
-	const char *err_msg[] = {
+	static const char * const err_msg[] = {
 		"Unknown",
 		"AXI slave error",
 		"AXI decode error",
-- 
2.30.2

Re: [PATCH 3/3] PCI: tegra: make const array err_msg static

[Vidya Sagar]

Reviewed-by: Vidya Sagar <vidyas@nvidia.com>

On 5/4/2021 10:48 PM, Christophe JAILLET wrote:
> Don't populate the array err_msg on the stack but instead make it
> static. Makes the object code smaller by 64 bytes.
> 
> While at it, add a missing const, as reported by checkpatch.
> 
> Compiled with gcc 11.0.1
> 
> Before:
> $ size drivers/pci/controller/pci-tegra.o
>     text	   data	    bss	    dec	    hex	filename
>    25623	   2844	     32	  28499	   6f53	drivers/pci/controller/pci-tegra.o
> 
> After:
> $ size drivers/pci/controller/pci-tegra.o
>     text	   data	    bss	    dec	    hex	filename
>    25559	   2844	     32	  28435	   6f13	drivers/pci/controller/pci-tegra.o
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/pci/controller/pci-tegra.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index fe8e21ce3e3d..b1250b15d290 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -764,7 +764,7 @@ static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
>   
>   static irqreturn_t tegra_pcie_isr(int irq, void *arg)
>   {
> -	const char *err_msg[] = {
> +	static const char * const err_msg[] = {
>   		"Unknown",
>   		"AXI slave error",
>   		"AXI decode error",
> 

Re: [PATCH 3/3] PCI: tegra: make const array err_msg static

[Krzysztof Wilczyński]

Hi Christophe,

Thank you for sending the patches over and taking care about these!

I was wondering whether you will be willing to send a v2 of this series
that would include fixes to everything the checkpatch.pl script reports
against this driver?  There aren't a lot of things to fix, thus the idea
to squash everything at once.  These warnings would be as follows
(excluding the ones you taken care of in this series):

  drivers/pci/controller/pci-tegra.c:1661: WARNING: please, no space before tabs
  drivers/pci/controller/pci-tegra.c:1890: WARNING: quoted string split across lines
  drivers/pci/controller/pci-tegra.c:1891: WARNING: quoted string split across lines
  drivers/pci/controller/pci-tegra.c:2619: WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.

These should be trivial to fix.  The two pertaining to "quoted string
split across lines" would be something that we might or might not decide
to do anything about this - technically, as per the Linux kernel coding
style [1], we ought to fix this... but, this particular case is not
a terrible example, so I will leave this at your discretion.

What do you think?

Also, don't worry if you don't have the time or otherwise, as these are
trivial things and it would only be a bonus to take care of them.

1. https://www.kernel.org/doc/html/v4.10/process/coding-style.html#breaking-long-lines-and-strings

	Krzysztof

Re: [PATCH 3/3] PCI: tegra: make const array err_msg static

[Christophe JAILLET]

Le 06/07/2021 à 00:31, Krzysztof Wilczyński a écrit :
> Hi Christophe,
> 
> Thank you for sending the patches over and taking care about these!
> 
> I was wondering whether you will be willing to send a v2 of this series
> that would include fixes to everything the checkpatch.pl script reports
> against this driver?  There aren't a lot of things to fix, thus the idea
> to squash everything at once.  These warnings would be as follows
> (excluding the ones you taken care of in this series):
> 
>    drivers/pci/controller/pci-tegra.c:1661: WARNING: please, no space before tabs
>    drivers/pci/controller/pci-tegra.c:1890: WARNING: quoted string split across lines
>    drivers/pci/controller/pci-tegra.c:1891: WARNING: quoted string split across lines
>    drivers/pci/controller/pci-tegra.c:2619: WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> 
> These should be trivial to fix.  The two pertaining to "quoted string
> split across lines" would be something that we might or might not decide
> to do anything about this - technically, as per the Linux kernel coding
> style [1], we ought to fix this... but, this particular case is not
> a terrible example, so I will leave this at your discretion.
> 
> What do you think?

Hi,
I don't think it worth it.

Even for patch 2/3 about 'seq_printf' --> 'seq_puts' conversion, I'm not 
fully convinced myself that is useful.

Too trivial clean-ups only mess-up 'git blame' for no real added value.

If you want these clean-ups, I can send a patch for it, but checkpatch 
output need sometimes to be ignored on files already in the tree. At 
least, this is my point of view.

CJ



> Also, don't worry if you don't have the time or otherwise, as these are
> trivial things and it would only be a bonus to take care of them.
> 
> 1. https://www.kernel.org/doc/html/v4.10/process/coding-style.html#breaking-long-lines-and-strings
> 
> 	Krzysztof
> 

Re: [PATCH 3/3] PCI: tegra: make const array err_msg static

[Krzysztof Wilczyński]

Hi Christophe,

[...]
> > These should be trivial to fix.  The two pertaining to "quoted string
> > split across lines" would be something that we might or might not decide
> > to do anything about this - technically, as per the Linux kernel coding
> > style [1], we ought to fix this... but, this particular case is not
> > a terrible example, so I will leave this at your discretion.
> > 
> > What do you think?
> 
> Hi,
> I don't think it worth it.
> 
> Even for patch 2/3 about 'seq_printf' --> 'seq_puts' conversion, I'm not
> fully convinced myself that is useful.

I personally believe it's a good change.

For a literal string without any formatting using the seq_printf() is
much more involved for no reason, but aside of this small performance
improvement, it also has some value in demonstrating the correct usage
patterns - people spent more time reading kernel code and looking at how
to do things and use things to base their work on, so setting some
example is not a bad idea.

Albeit, it's a matter of point of view too, I suppose.

> Too trivial clean-ups only mess-up 'git blame' for no real added value.

Yes, there is a fine line with these.

> If you want these clean-ups, I can send a patch for it, but checkpatch
> output need sometimes to be ignored on files already in the tree. At least,
> this is my point of view.

No worries!  Thank you for giving it some thought!  I appreciate it. :)

	Krzysztof

Re: [PATCH 1/3] PCI: tegra: Fix OF node reference leak

[Lorenzo Pieralisi]

On Tue, May 04, 2021 at 07:17:42PM +0200, Christophe JAILLET wrote:
> Commit 9e38e690ace3 ("PCI: tegra: Fix OF node reference leak") has fixed
> some node reference leaks in this function but missed some of them.
> 
> In fact, having 'port' referenced in the 'rp' structure is not enough to
> prevent the leak, until 'rp' is actually added in the 'pcie->ports' list.
> 
> Add the missing 'goto err_node_put' accordingly.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/pci/controller/pci-tegra.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Thierry, Jon, Vidya,

please review this series when you have time, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 8069bd9232d4..006bf0346dec 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2193,13 +2193,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		rp->np = port;
>  
>  		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
> -		if (IS_ERR(rp->base))
> -			return PTR_ERR(rp->base);
> +		if (IS_ERR(rp->base)) {
> +			err = PTR_ERR(rp->base);
> +			goto err_node_put;
> +		}
>  
>  		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
>  		if (!label) {
> -			dev_err(dev, "failed to create reset GPIO label\n");
> -			return -ENOMEM;
> +			err = -ENOMEM;
> +			goto err_node_put;
>  		}
>  
>  		/*
> @@ -2217,7 +2219,8 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  			} else {
>  				dev_err(dev, "failed to get reset GPIO: %ld\n",
>  					PTR_ERR(rp->reset_gpio));
> -				return PTR_ERR(rp->reset_gpio);
> +				err = PTR_ERR(rp->reset_gpio);
> +				goto err_node_put;
>  			}
>  		}
>  
> -- 
> 2.30.2
> 

Re: [PATCH 1/3] PCI: tegra: Fix OF node reference leak

[Vidya Sagar]

Reviewed-by: Vidya Sagar <vidyas@nvidia.com>

On 5/4/2021 10:47 PM, Christophe JAILLET wrote:
> Commit 9e38e690ace3 ("PCI: tegra: Fix OF node reference leak") has fixed
> some node reference leaks in this function but missed some of them.
> 
> In fact, having 'port' referenced in the 'rp' structure is not enough to
> prevent the leak, until 'rp' is actually added in the 'pcie->ports' list.
> 
> Add the missing 'goto err_node_put' accordingly.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/pci/controller/pci-tegra.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 8069bd9232d4..006bf0346dec 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -2193,13 +2193,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>   		rp->np = port;
>   
>   		rp->base = devm_pci_remap_cfg_resource(dev, &rp->regs);
> -		if (IS_ERR(rp->base))
> -			return PTR_ERR(rp->base);
> +		if (IS_ERR(rp->base)) {
> +			err = PTR_ERR(rp->base);
> +			goto err_node_put;
> +		}
>   
>   		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
>   		if (!label) {
> -			dev_err(dev, "failed to create reset GPIO label\n");
> -			return -ENOMEM;
> +			err = -ENOMEM;
> +			goto err_node_put;
>   		}
>   
>   		/*
> @@ -2217,7 +2219,8 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>   			} else {
>   				dev_err(dev, "failed to get reset GPIO: %ld\n",
>   					PTR_ERR(rp->reset_gpio));
> -				return PTR_ERR(rp->reset_gpio);
> +				err = PTR_ERR(rp->reset_gpio);
> +				goto err_node_put;
>   			}
>   		}
>   
> 

Re: [PATCH 1/3] PCI: tegra: Fix OF node reference leak

[Lorenzo Pieralisi]

On Tue, 4 May 2021 19:17:42 +0200, Christophe JAILLET wrote:
> Commit 9e38e690ace3 ("PCI: tegra: Fix OF node reference leak") has fixed
> some node reference leaks in this function but missed some of them.
> 
> In fact, having 'port' referenced in the 'rp' structure is not enough to
> prevent the leak, until 'rp' is actually added in the 'pcie->ports' list.
> 
> Add the missing 'goto err_node_put' accordingly.

Applied to pci/tegra, thanks!

[1/3] PCI: tegra: Fix OF node reference leak
      https://git.kernel.org/lpieralisi/pci/c/eff21f5da3
[2/3] PCI: tegra: Use 'seq_puts' instead of 'seq_printf'
      https://git.kernel.org/lpieralisi/pci/c/804b2b6f2a
[3/3] PCI: tegra: make const array err_msg static
      https://git.kernel.org/lpieralisi/pci/c/fd44e8efcc

Thanks,
Lorenzo