| Message ID | 5f3f35296b944b94546cc7d1e9cc6186484620d8.1620148539.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/3] PCI: tegra: Fix OF node reference leak | expand |
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", >
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
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 >
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
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",
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(-)