From: "Krzysztof Wilczyński" <kw@linux.com> To: "Pali Rohár" <pali@kernel.org> Cc: "Bjorn Helgaas" <bhelgaas@google.com>, "Kalle Valo" <kvalo@codeaurora.org>, "Toke Høiland-Jørgensen" <toke@redhat.com>, "Marek Behún" <kabel@kernel.org>, vtolkm@gmail.com, "Rob Herring" <robh@kernel.org>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>, "Jason Cooper" <jason@lakedaemon.net>, linux-pci@vger.kernel.org, ath10k@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Date: Sat, 27 Mar 2021 01:14:10 +0100 [thread overview] Message-ID: <YF540gjh156QIirA@rocinante> (raw) In-Reply-To: <20210326124326.21163-1-pali@kernel.org> Hi Pali, Thank you for sending the patch over! [...] > +static int pcie_change_tls_to_gen1(struct pci_dev *parent) Just a nitpick, so feel free to ignore it. I would just call the variable "dev" as we pass a pointer to a particular device, but it does not matter as much, so I am leaving this to you. [...] > + if (ret == 0) { You prefer this style over "if (!ret)"? Just asking in the view of the style that seem to be preferred in the code base at the moment. > + /* Verify that new value was really set */ > + pcie_capability_read_word(parent, PCI_EXP_LNKCTL2, ®16); > + if ((reg16 & PCI_EXP_LNKCTL2_TLS) != PCI_EXP_LNKCTL2_TLS_2_5GT) > + ret = -EINVAL; I am wondering about this verification - did you have a case where the device would not properly set its capability, or accept the write and do nothing? > + if (ret != 0) I think "if (ret)" would be fine to use here, unless you prefer being more explicit. See my question about style above. > static bool pcie_retrain_link(struct pcie_link_state *link) > { > struct pci_dev *parent = link->pdev; > unsigned long end_jiffies; > u16 reg16; > + u32 reg32; > + > + /* Check if link is capable of higher speed than 2.5 GT/s and needs quirk */ > + pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, ®32); > + if ((reg32 & PCI_EXP_LNKCAP_SLS) > PCI_EXP_LNKCAP_SLS_2_5GB) { I wonder if moving this check to pcie_change_tls_to_gen1() would make more sense? It would then make this function a little cleaner. What do you think? [...] > +static void quirk_no_bus_reset_and_no_retrain_link(struct pci_dev *dev) > +{ > + dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET | PCI_DEV_FLAGS_NO_RETRAIN_LINK_WHEN_NOT_GEN1; > +} [...] I know that the style has been changed to allow 100 characters width and that checkpatch.pl now also does not warn about line length, as per commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), but I think Bjorn still prefers 80 characters, thus this line above might have to be aligned. Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: "Krzysztof Wilczyński" <kw@linux.com> To: "Pali Rohár" <pali@kernel.org> Cc: "Bjorn Helgaas" <bhelgaas@google.com>, "Kalle Valo" <kvalo@codeaurora.org>, "Toke Høiland-Jørgensen" <toke@redhat.com>, "Marek Behún" <kabel@kernel.org>, vtolkm@gmail.com, "Rob Herring" <robh@kernel.org>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>, "Jason Cooper" <jason@lakedaemon.net>, linux-pci@vger.kernel.org, ath10k@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Date: Sat, 27 Mar 2021 01:14:10 +0100 [thread overview] Message-ID: <YF540gjh156QIirA@rocinante> (raw) In-Reply-To: <20210326124326.21163-1-pali@kernel.org> Hi Pali, Thank you for sending the patch over! [...] > +static int pcie_change_tls_to_gen1(struct pci_dev *parent) Just a nitpick, so feel free to ignore it. I would just call the variable "dev" as we pass a pointer to a particular device, but it does not matter as much, so I am leaving this to you. [...] > + if (ret == 0) { You prefer this style over "if (!ret)"? Just asking in the view of the style that seem to be preferred in the code base at the moment. > + /* Verify that new value was really set */ > + pcie_capability_read_word(parent, PCI_EXP_LNKCTL2, ®16); > + if ((reg16 & PCI_EXP_LNKCTL2_TLS) != PCI_EXP_LNKCTL2_TLS_2_5GT) > + ret = -EINVAL; I am wondering about this verification - did you have a case where the device would not properly set its capability, or accept the write and do nothing? > + if (ret != 0) I think "if (ret)" would be fine to use here, unless you prefer being more explicit. See my question about style above. > static bool pcie_retrain_link(struct pcie_link_state *link) > { > struct pci_dev *parent = link->pdev; > unsigned long end_jiffies; > u16 reg16; > + u32 reg32; > + > + /* Check if link is capable of higher speed than 2.5 GT/s and needs quirk */ > + pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, ®32); > + if ((reg32 & PCI_EXP_LNKCAP_SLS) > PCI_EXP_LNKCAP_SLS_2_5GB) { I wonder if moving this check to pcie_change_tls_to_gen1() would make more sense? It would then make this function a little cleaner. What do you think? [...] > +static void quirk_no_bus_reset_and_no_retrain_link(struct pci_dev *dev) > +{ > + dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET | PCI_DEV_FLAGS_NO_RETRAIN_LINK_WHEN_NOT_GEN1; > +} [...] I know that the style has been changed to allow 100 characters width and that checkpatch.pl now also does not warn about line length, as per commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"), but I think Bjorn still prefers 80 characters, thus this line above might have to be aligned. Krzysztof _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2021-03-27 0:19 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-26 12:43 [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Pali Rohár 2021-03-26 12:43 ` Pali Rohár 2021-03-26 18:13 ` Toke Høiland-Jørgensen 2021-03-26 18:13 ` Toke Høiland-Jørgensen 2021-03-27 0:14 ` Krzysztof Wilczyński [this message] 2021-03-27 0:14 ` Krzysztof Wilczyński 2021-03-27 13:29 ` Pali Rohár 2021-03-27 13:29 ` Pali Rohár 2021-03-27 14:42 ` Marek Behún 2021-03-27 14:42 ` Marek Behún 2021-03-27 17:33 ` Pali Rohár 2021-03-27 17:33 ` Pali Rohár 2021-04-27 10:55 ` [PATCH v2] PCI: Disallow retraining link for Atheros " Pali Rohár 2021-04-27 10:55 ` Pali Rohár 2021-04-30 11:41 ` Pali Rohár 2021-04-30 11:41 ` Pali Rohár 2021-05-05 16:33 ` [PATCH v3] " Pali Rohár 2021-05-05 16:33 ` Pali Rohár 2021-05-11 20:39 ` Pali Rohár 2021-05-11 20:39 ` Pali Rohár 2021-05-28 0:08 ` Pali Rohár 2021-05-28 0:08 ` Pali Rohár 2021-06-01 11:28 ` Krzysztof Wilczyński 2021-06-01 11:28 ` Krzysztof Wilczyński 2021-06-01 20:05 ` Bjorn Helgaas 2021-06-01 20:05 ` Bjorn Helgaas 2021-06-01 21:18 ` Pali Rohár 2021-06-01 21:18 ` Pali Rohár 2021-06-02 0:00 ` Bjorn Helgaas 2021-06-02 0:00 ` Bjorn Helgaas 2021-06-02 12:08 ` Pali Rohár 2021-06-02 12:08 ` Pali Rohár 2021-06-02 15:55 ` Bjorn Helgaas 2021-06-02 15:55 ` Bjorn Helgaas 2021-06-02 19:03 ` Pali Rohár 2021-06-02 19:03 ` Pali Rohár 2021-06-16 21:38 ` Bjorn Helgaas 2021-06-16 21:38 ` Bjorn Helgaas 2021-06-21 14:28 ` Pali Rohár 2021-06-21 14:28 ` Pali Rohár 2021-06-25 20:19 ` Bjorn Helgaas 2021-06-25 20:19 ` Bjorn Helgaas 2021-06-26 14:38 ` Pali Rohár 2021-06-26 14:38 ` Pali Rohár 2021-06-21 14:39 ` Pali Rohár 2021-06-21 14:39 ` Pali Rohár 2021-10-05 19:43 ` Jannis 2021-10-05 19:43 ` Jannis
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YF540gjh156QIirA@rocinante \ --to=kw@linux.com \ --cc=ath10k@lists.infradead.org \ --cc=bhelgaas@google.com \ --cc=ilias.apalodimas@linaro.org \ --cc=jason@lakedaemon.net \ --cc=kabel@kernel.org \ --cc=kvalo@codeaurora.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=pali@kernel.org \ --cc=robh@kernel.org \ --cc=thomas.petazzoni@bootlin.com \ --cc=toke@redhat.com \ --cc=vtolkm@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.