Re: [PATCH] PCI: shpchp: remove unused function
Hi, > Fix the following clang warning: > > drivers/pci/hotplug/shpchp_hpc.c:177:20: warning: unused function > 'shpc_writeb' [-Wunused-function]. [...] Nice catch! Thank you. Reviewed-by: Krzysztof Wilczyński By the way, next time capitalise the subject line. Krzysztof
Re: [PATCH] PCI: dwc: remove unused function
Hi, > Fix the following clang warning: > > drivers/pci/controller/dwc/pcie-intel-gw.c:84:19: warning: unused > function 'pcie_app_rd' [-Wunused-function]. [...] Nice catch! Thank you. Reviewed-by: Krzysztof Wilczyński By the way, next time capitalise the subject line. Krzysztof
Re: [PATCH v4 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
Hi Greentime,
[...]
> + /* Wait for wait_idle */
> + ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
> + if (ret)
> + dev_err(dev, "Wait for wait_ilde state failed!\n");
> +
> + /* Clear */
> + writel_relaxed(0, phy_cr_para_wr_en);
> +
> + /* Wait for ~wait_idle */
> + ret = readl_poll_timeout(phy_cr_para_ack, val, !val, 10, 5000);
> + if (ret)
> + dev_err(dev, "Wait for !wait_ilde state failed!\n");
> +}
[...]
> +static int fu740_pcie_start_link(struct dw_pcie *pci)
> +{
> + struct device *dev = pci->dev;
> +
> + /* Start LTSSM. */
> + fu740_pcie_ltssm_enable(dev);
> + return 0;
> +}
The typos etc., are still here. See:
https://lore.kernel.org/linux-pci/YFQqpojmJyX0l6lx@rocinante/
Krzysztof
Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges
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, );
> + 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, );
> + 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
Re: [PATCH] PCI: endpoint: remove redundant initialization of pointer dev
Hi Colin, > The pointer dev is being initialized with a value that is > never read and it is being updated later with a new value. The > initialization is redundant and can be removed. [...] > - struct device *dev = epf->epc->dev.parent; > + struct device *dev; > struct pci_epf_bar *epf_bar; > struct pci_epc *epc; Thank you! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH] PCI: Disable D3cold support on Intel XMM7360
Hi,
Thank you for sending the patch over!
[...]
> +static void pci_fixup_no_d3cold(struct pci_dev *pdev)
> +{
> + pci_info(pdev, "disable D3cold\n");
Not sure how useful this message would generally be? Unless this is
useful to someone who is doing some troubleshooting, etc.
> + pci_d3cold_disable(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x7360, pci_fixup_no_d3cold);
[...]
A small suggestion: a brief comment, perhaps even linking to the
Bugzilla, might be a nice touch here, so that people would know why
D3cold is being disabled for XMM7360, etc.
Krzysztof
Re: [PATCH 4/4] PCI/sysfs: Allow userspace to query and set device reset mechanism
Hello, [...] Aside of the sysfs interface, would this new functionality also require anything to be overridden at boot time via passing some command-line arguments? Not sure how relevant such thing would be to device, but, whatnot reset, though. I am curious whether there would be a need for anything like that. Krzysztof
Re: [PATCH v3 3/3] PCI: imx: clear vreg bypass when pcie vph voltage is 3v3
Hi, > + /* > + * Regarding to the datasheet, the PCIE_VPH is suggested > + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the > + * VREG_BYPASS should be cleared to zero. > + */ [...] A small nitpick here. What about the following: Regarding the data sheet, the PCIE_VPH is suggested to be 1.8V. If the PCIE_VPH is supplied with 3.3V, the VREG_BYPASS should be cleared to zero. What do you think? Krzysztof
Re: [PATCH 6/6] PCI: keystone: Add workaround for Errata #i2037 (AM65x SR 1.0)
Hi Kishon,
A few small nitpicks.
> Errata #i2037 in AM65x/DRA80xM Processors Silicon Revision 1.0
> (SPRZ452D–July 2018–Revised December 2019 [1]) mentions when an
> inbound PCIe TLP spans more than two internal AXI 128-byte bursts,
> the bus may corrupt the packet payload and the corrupt data may
> cause associated applications or the processor to hang.
>
> The workaround for Errata #i2037 is to limit the maximum read
> request size and maximum payload size to 128 Bytes. Add workaround
> for Errata #i2037 here. The errata and workaround is applicable
> only to AM65x SR 1.0 and later versions of the silicon will have
> this fixed.
I think it would be either "128 B" or "128 bytes", there is no need to
capitalise bytes.
[...]
> + /*
> + * Memory transactions fail with PCI controller in AM654 PG1.0
> + * when MRRS is set to more than 128 Bytes. Force the MRRS to
> + * 128 Bytes in all downstream devices.
> + */
Same here, it would be "128 bytes" in the comment above.
[...]
> + if (pcie_get_readrq(dev) > 128) {
> + dev_info(>dev, "limiting MRRS to 128\n");
> + pcie_set_readrq(dev, 128);
> + }
[...]
Might be nice to add unit here, so "128 bytes".
Krzysztof
Re: [PATCH 5/6] PCI: keystone: Add PCI legacy interrupt support for AM654
Hi Kishon,
[...]
> + if (!legacy_irq_domain) {
> + dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> + return -EINVAL;
> + }
[...]
It would be "IRQ" and "IRQs" in the message above.
[...]
> - ret = ks_pcie_config_legacy_irq(ks_pcie);
> - if (ret)
> - return ret;
> + if (!ks_pcie->is_am6) {
> + pp->bridge->child_ops = _child_pcie_ops;
> + ret = ks_pcie_config_legacy_irq(ks_pcie);
> + if (ret)
> + return ret;
> + } else {
> + ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
> + if (ret)
> + return ret;
> + }
[...]
What if we change this to the following:
if (!ks_pcie->is_am6) {
pp->bridge->child_ops = _child_pcie_ops;
ret = ks_pcie_config_legacy_irq(ks_pcie);
} else {
ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
}
if (ret)
return ret;
Not sure if this is something you would prefer, but it seems that either
of the functions can set "ret", so checking immediately after would be
the same as checking in either of the branches. But, this is a matter
of style, so it would be up to you - not sure what do you prefer.
Krzysztof
Re: [PATCH 4/6] PCI: keystone: Convert to using hierarchy domain for legacy interrupts
Hi Kishon,
Thank you for sending the series over!
A few small nitpick, so feel free to ignore it.
[...]
> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, _fwspec);
> + if (ret < 0) {
> + pr_err("Failed to allocate parent irq %u: %d\n",
> +parent_fwspec.param[0], ret);
> + return ret;
[...]
Use "IRQ" in the message above.
Also, the error messages with both starting with upper- and lower- case
letter, not sure if this is because of dev_err() vs pr_err(), but if
there is no significance between these two methods, then it might be
nice to keep the style consistent.
Krzysztof
Re: [PATCH] PCI: ACPI: PM: Fix debug message in acpi_pci_set_power_state()
Hi, [...] > To address this issue, modify the debug message in question to print > the current power state of the target PCI device's ACPI companion > instead of printing the target power state which may not reflect > the real final power state of the device. [...] Thank you! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH 0/4] PCI: Correct function names in the header
Hi, [...] > PCI: acpi_pcihp: Correct acpi_pci_check_ejectable() function name in > the header > PCI/AER: Correct function names in the header > PCI/PME: Correct pcie_pme_init() function name in the header > PCI/ATS: Correct pci_max_pasids() function name in header [...] All of this has already been fixed in the following series: https://lore.kernel.org/linux-pci/20210311001724.423356-1...@linux.com/ Krzysztof
Re: [PATCH RESEND] PCI: dwc: put struct dw_pcie::{ep,pp} into a union to reduce its size
Hi Alexander,
Thank you for sending the patch over!
> A single dw_pcie entity can't be a root complex and an endpoint at
> the same time.
Nice catch!
A small nitpick: this would be Root Complex and Endpoint, as it's
customary to capitalise these.
Also, if you could capitalise the subject line - it could also perhaps
be simplified to something like, for example:
Optimize struct dw_pcie to reduce its size
Feel free to ignore both suggestions, as these are just nitpicks.
> We can use this to reduce the size of dw_pcie by 80, from 280 to 200
> bytes (on x32, guess more on x64), by putting the related embedded
> structures (struct pcie_port and struct dw_pcie_ep) into a union.
[...]
> - struct pcie_portpp;
> - struct dw_pcie_ep ep;
> + union {
> + struct pcie_portpp;
> + struct dw_pcie_ep ep;
> + };
[...]
How did you measure the difference? Often, people include pahole output
for the "before" and "after", so to speak, to showcase the difference
and/or improvement. Do you have something like that handy?
Krzysztof
Re: [PATCH] x86/pci: use true and false for bool variable
Hi, Thank you for sending the patch over! > fixed the following coccicheck: > ./arch/x86/pci/mmconfig-shared.c:464:9-10: WARNING: return of 0/1 in > function 'is_mmconf_reserved' with return type bool > ./arch/x86/pci/mmconfig-shared.c:493:5-6: WARNING: return of 0/1 in > function 'is_mmconf_reserved' with return type bool > ./arch/x86/pci/mmconfig-shared.c:501:9-10: WARNING: return of 0/1 in > function 'is_mmconf_reserved' with return type bool > ./arch/x86/pci/mmconfig-shared.c:522:5-6: WARNING: return of 0/1 in > function 'is_mmconf_reserved' with return type bool [...] Looks good, although a few small nitpicks: you should capitalise the subject line so that it matches the style used in previous commits, and the commit message could also be improved in terms of style and also explaining what and why this patch is fixing the return type (aside of just addressing report from Cocinelle). Other than that, Reviewed-by: Krzysztof Wilczyński Thank you! Krzysztof
Re: [PATCH] PCI: keystone: Let AM65 use the pci_ops defined in pcie-designware-host.c
Hi Kishon, Thank you for the fix! [...] > @@ -798,7 +798,8 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > int ret; > > pp->bridge->ops = _pcie_ops; > - pp->bridge->child_ops = _child_pcie_ops; > + if (!ks_pcie->is_am6) > + pp->bridge->child_ops = _child_pcie_ops; > > ret = ks_pcie_config_legacy_irq(ks_pcie); > if (ret) [...] Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH v2 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
Hi, [...] > > + /* Wait for wait_idle */ > > + ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000); > > + if (ret) > > + dev_err(dev, "Wait for wait_ilde state failed!\n"); > > It would be "wait_idle" rather than "wait_idle". [...] Apologies, meant to say "wait_ilde" in the "rather than" part, but went ahead and somehow used the correct spelling. :) Krzysztof
Re: [PATCH v2 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
Hi,
[...]
> +static void fu740_phyregwrite(const uint8_t phy, const uint16_t addr,
> + const uint16_t wrdata, struct fu740_pcie *afp)
> +{
> + struct device *dev = afp->pci.dev;
> + void __iomem *phy_cr_para_addr;
> + void __iomem *phy_cr_para_wr_data;
> + void __iomem *phy_cr_para_wr_en;
> + void __iomem *phy_cr_para_ack;
> + int ret, val;
> +
> + /* Setup */
> + if (phy) {
> + phy_cr_para_addr = afp->mgmt_base +
> PCIEX8MGMT_PHY1_CR_PARA_ADDR;
> + phy_cr_para_wr_data = afp->mgmt_base +
> PCIEX8MGMT_PHY1_CR_PARA_WR_DATA;
> + phy_cr_para_wr_en = afp->mgmt_base +
> PCIEX8MGMT_PHY1_CR_PARA_WR_EN;
> + phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_ACK;
> + } else {
> + phy_cr_para_addr = afp->mgmt_base +
> PCIEX8MGMT_PHY0_CR_PARA_ADDR;
> + phy_cr_para_wr_data = afp->mgmt_base +
> PCIEX8MGMT_PHY0_CR_PARA_WR_DATA;
> + phy_cr_para_wr_en = afp->mgmt_base +
> PCIEX8MGMT_PHY0_CR_PARA_WR_EN;
> + phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_ACK;
> + }
> +
> + writel_relaxed(addr, phy_cr_para_addr);
> + writel_relaxed(wrdata, phy_cr_para_wr_data);
> + writel_relaxed(1, phy_cr_para_wr_en);
> +
> + /* Wait for wait_idle */
> + ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
> + if (ret)
> + dev_err(dev, "Wait for wait_ilde state failed!\n");
It would be "wait_idle" rather than "wait_idle".
[...]
> + /* Wait for ~wait_idle */
> + ret = readl_poll_timeout(phy_cr_para_ack, val, !val, 10, 5000);
> + if (ret)
> + dev_err(dev, "Wait for !wait_ilde state failed!\n");
[...]
Same as above, it would be "wait_idle" in the above.
> +static void fu740_pcie_ltssm_enable(struct device *dev)
> +{
> + struct fu740_pcie *afp = dev_get_drvdata(dev);
> +
> + /* Enable LTSSM */
> + writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
> +}
> +
> +static int fu740_pcie_start_link(struct dw_pcie *pci)
> +{
> + struct device *dev = pci->dev;
> +
> + /* Start LTSSM. */
Nitpick. No need for a dot in this comment to keep it consistent with
the comment in the function above this one.
> +static int fu740_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct fu740_pcie *afp = to_fu740_pcie(pci);
> + struct device *dev = pci->dev;
> + int ret;
> +
> + /* Power on reset */
> + fu740_pcie_drive_perstn(afp);
> +
> + /* Enable pcieauxclk */
> + ret = clk_prepare_enable(afp->pcie_aux);
> + if (ret)
> + dev_err(dev, "unable to enable pcie_aux clock\n");
> +
> + /*
> + * Assert hold_phy_rst (hold the controller LTSSM in reset after
> + * power_up_rst_n for register programming with cr_para)
> + */
> + writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +
> + /* Deassert power_up_rst_n */
> + ret = reset_control_deassert(afp->rst);
> + if (ret)
> + dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
> +
> + fu740_pcie_init_phy(afp);
> +
> + /* Disable pcieauxclk */
> + clk_disable_unprepare(afp->pcie_aux);
> + /* Clear hold_phy_rst */
> + writel_relaxed(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> + /* Enable pcieauxclk */
> + ret = clk_prepare_enable(afp->pcie_aux);
> + /* Set RC mode */
> + writel_relaxed(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
> +
> + return 0;
> +}
[...]
It seems that the error handling is somewhat broken in the above
function, especially when you look at how the "ret" variables does not
seem to be used for anything once there was an error.
Krzysztof
Re: [PATCH v2 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller
Hi, Thank you for sending the patches over! A few nitpicks. > +title: SiFive fu740 PCIe host controller > + > +description: > + SiFive fu740 PCIe host controller is based on the Synopsys DesignWare > + PCI core. It shares common features with the PCIe DesignWare core and > + inherits common properties defined in > + Documentation/devicetree/bindings/pci/designware-pcie.txt. [...] In the above title and description it would be "FU740" to keep this consistent with everything else. Also, as this is a YAML file, a multi-line description might be better expressed as "description: |" or "description: |+", of course it depends on whether you would like or not to preserve line endings. [...] > + dma-coherent: > +description: Indicates that the PCIe IP block can ensure the coherency > + > + bus-range: > +description: Range of bus numbers associated with this controller. [...] > + resets: > +description: A phandle to the PCIe power up reset line > + > + pwren-gpios: > +description: Should specify the GPIO for controlling the PCI bus device > power on > + > + perstn-gpios: > +description: Should specify the GPIO for controlling the PCI bus device > reset [...] All the above descriptions should end with a period, so that we keep things consistent throughout. Krzysztof
Re: [PATCH 0/4] Expose and manage PCI device reset
Hi Amey, [...] > Basically whole thing boils down to I'm not good at handling terminal > email clients. I'll surely keep those points mentioned by Bjorn > in my mind. [...] No worries. Thunderbird works fine with Google Mail and can send plain text e-mails too, if you get tired of Mutt etc. By the way, don't immediately send v2 quite yet. Allow people some time to review first version. Well, unless you deem that you need to do it, that is. Krzysztof
Re: [PATCH 0/4] Expose and manage PCI device reset
Hi Amey, Thank you for sending the series over! [...] > > Reviews/Acks/Sign-off-by from others (aside from Tested/Reported-by) > > really need to be explicit, IMO. This is a common issue for new > > developers, but it really needs to be more formal. I wouldn't claim to > > be able to speak for Raphael and interpret his comments so far as his > > final seal of approval. > > > > Also in the patches, all Sign-offs/Reviews/Acks need to be above the > > triple dash '---' line. Anything between that line and the beginning > > of the diff is discarded by tools. People will often use that for > > difference between version since it will be discarded on commit. > > Likewise, the cover letter is not committed, so Review-by there are > > generally not done. I generally make my Sign-off last in the chain and > > maintainers will generally add theirs after that. This makes for a > > chain where someone can read up from the bottom to see how this commit > > entered the kernel. Reviews, Acks, and whatnot will therefore usually > > be collected above the author posting the patch. > > > > Since this is a v1 patch and it's likely there will be more revisions, > > rather than send a v2 immediately with corrections, I'd probably just > > reply to the cover letter retracting Raphael's Review-by for him to > > send his own and noting that you'll fix the commit reviews formatting, > > but will wait for a bit for further comments before sending a new > > version. > > > > No big deal, nice work getting it sent out. Thanks, > > > > Alex > > > Raphael sent me the email with > Reviewed-by: Raphael Norwitz that > is why I included it. > So basically in v2 I should reorder tags such that Sign-off will be > the last. Did I get that right? Or am I missing something? [...] I am not sure about the messages outside of the mailing list between you, Alex and Raphael, as normally conversation and any reviews would happen here (on the mailing list, that is), but as long as everyone involved is on the same page, then every should be fine. In terms of how to format the patch, have a look at the following, especially before you send another version, as there are some good tips and recommendations there (including how to order things): https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com/ Krzysztof
Re: [PATCH v4 2/2] nPCI: brcmstb: Use reset/rearm instead of deassert/assert
Hi, Thank you for sending the patches over! > The Brcmstb PCIe RC uses a reset control "rescal" for certain chips. This [...] A small suggestion: it would be nicer to mention "Broadcom STB" rather than "Brcmstb" in the sentence above. [...] > +err1: > + reset_control_rearm(pcie->rescal); > +err0: > clk_disable_unprepare(pcie->clk); > return ret; [...] A small nitpick. Now that there are two labels on the error recovery path, it might be better to name both of these labels a little bit better. Some examples from the PCI tree: error_clock_unprepare err_disable_clock err_disable_clk err_clk_disable So it could be: err_reset:<-- or err_rearm or even err_reset_rearm, etc. reset_control_rearm(pcie->rescal); err_disable_clk: clk_disable_unprepare(pcie->clk); What do you think? Krzysztof
Re: [PATCH 1/3] PCI: controller: al: select CONFIG_PCI_ECAM
Hi, > Compile-testing this driver without ECAM support results in a link > failure: > > ld.lld: error: undefined symbol: pci_ecam_map_bus > >>> referenced by pcie-al.c > >>> pci/controller/dwc/pcie-al.o:(al_pcie_map_bus) in archive > >>> drivers/built-in.a > > Select CONFIG_ECAM like the other drivers do. [...] Ouch. Thank you! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH v9 1/3] PCI: portdrv: Add pcie_port_service_get_irq() function
Hi, [...] > +/* > + * pcie_port_service_get_irq - get irq of the service > + * @dev: PCI Express port the service is associated with > + * @service: For the service to find > + * > + * Get irq number associated with given service on a pci_dev > + */ > +int pcie_port_service_get_irq(struct pci_dev *dev, u32 service) [...] A small nitpick. It would be "IRQ" rather than "irq" in the above kernel-doc. Also, missing periods at the end of sentence. Krzysztof
Re: [PATCH v2 5/5] PCI: dwc: Move dw_pcie_msi_init() from each users to designware host
Hi, > Currently, dw_pcie_msi_init() allocates and maps page for msi, then > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex > may lose power during suspend-to-RAM, so when we resume, we want to > redo the latter but not the former. If designware based driver (for > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the > previous msi page will be leaked. From another side, except > pci-dra7xx.c we can move the dw_pcie_msi_init() from each users to > designware host, I.E move the msi page allocation and mapping to > dw_pcie_host_init() and move the PCIE_MSI_ADDR_* programming to > dw_pcie_setup_rc(). After this moving, we solve the msi page leakage > as well. [...] A small nitpick. All the "designware" should be "DesignWare" both in the commit message and the subject. Similarly, "msi" would be "MSI", and "I.E" would become "i.e.,". If you ever sent another version of the patch, that is. See the following for reference: https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com/ Krzysztof
Re: [RFC 1/1] s390/pci: expose a PCI device's UID as its index
Hi Niklas,
[...]
> +static ssize_t index_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
> + u32 index = ~0;
> +
> + if (zpci_unique_uid)
> + index = zdev->uid;
> +
> + return sprintf(buf, "%u\n", index);
[...]
Would it be possible to use the new sysfs_emit() rather than sprintf()
even though the zpci_attr macro and still use mio_enabled_show() still
would use sprintf(). What do you think?
See https://www.kernel.org/doc/html/latest/filesystems/sysfs.html for
the changes in the internal API.
Krzysztof
Re: [PATCH -next] pci/controller/dwc: convert comma to semicolon
Hi, [...] > I would request NXP maintainers to take this patch, rewrite it as > Bjorn requested and resend it as fast as possible, this is a very > relevant fix. [...] Looking at the state of the pci-layerscape-ep.c file in Linus' tree, this still hasn't been fixed, and it has been a while. NXP folks, are you intend to pick this up? Do let us know. Krzysztof
Re: [PATCH] PCI:tegra:Correct typo for PCIe endpoint mode in Tegra194
Hi, A small nitpick. There are spaces missing in the subject line. [...] > Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to > - work in host mode. There are two instances of PCIe controllers in > + work in endpoint mode. There are two instances of PCIe controllers in > Tegra194. This controller can work either as EP or RC. In order to > enable host-specific features PCIE_TEGRA194_HOST must be selected and > in order to enable device-specific features PCIE_TEGRA194_EP must be [...] Nice catch! Thank you! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH] PCI/IOV: Clarify error message for unbound devices
Hi Moritz,
[...]
> + /* is PF driver loaded */
> + if (!pdev->driver) {
> + pci_info(pdev, "No driver bound to device. Cannot configure
> SRIOV\n");
> + ret = -ENOENT;
> + goto exit;
> + }
[...]
Thank you!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH v2 3/3] PCI: set dma-can-stall for HiSilicon chip
Hi, [...] > Property dma-can-stall depends on patchset > https://lore.kernel.org/linux-iommu/20210108145217.2254447-1-jean-phili...@linaro.org/ [...] If you plan to post another version of this patch to include the above link into the commit message or reference to the commit itself, as Jean-Philippe's series can already be included in the mainline (since it has been a while now from when this series was originally posted), then I have a favour to ask - would you also be able to also capitalise the subject line (so that it's consistent) and change "chip" to "chips" since there are two you mention in the commit message. Thank you! Krzysztof
Re: [PATCH] PCI: endpoint: Select configfs dependency
Hi, [...] > The newly added pci-epf-ntb driver uses configfs, which > causes a link failure when that is disabled at compile-time: > > arm-linux-gnueabi-ld: drivers/pci/endpoint/functions/pci-epf-ntb.o: in > function `epf_ntb_add_cfs': > pci-epf-ntb.c:(.text+0x954): undefined reference to > `config_group_init_type_name' [...] Thank you for fixing this! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH] PCI: tegra: Constify static structs
Hi Rikard, Thank you for sending the patch over! > The only usage of them is to assign their address to the 'ops' field in > the pcie_port and the dw_pcie_ep structs, both which are pointers to > const. Make them const to allow the compiler to put them in read-only > memory. [...] Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH] PCI: acpiphp: Fixed coding style
Hi,
Thank you for sending the patch over. Few suggestions below.
There seem to be an extra space in the subject line.
> In this commit fixed coding style for braces and comments.
Where these coding style changes suggested by a tool? For example, was it
something like checkpatch.pl? If so, then it would be prudent to
mention that the script found these for future reference.
[...]
> - struct list_head funcs; /* one slot may have different
> -objects (i.e. for each function) */
> + struct list_head funcs; /* one slot may have different */
> + /* objects (i.e. for each function) */
[...]
Above would be a single line commit that has been made to with within
the line length rules, as otherwise the line would be too long.
This is not necessarily something that we ought to fix, see for example:
https://elixir.bootlin.com/linux/v5.11.3/source/include/linux/pci.h
[...]
> -struct acpiphp_attention_info
> -{
> +struct acpiphp_attention_info {
> int (*set_attn)(struct hotplug_slot *slot, u8 status);
> int (*get_attn)(struct hotplug_slot *slot, u8 *status);
> struct module *owner;
[...]
Nice catch!
Generally, you would also need to your full name when providing your
"Signed-off-by:" following the style that has been widely accepted. See
git log for how it would normally look like, and also have a look at the
following for some general guidance on how to submit patches:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
Krzysztof
Re: [PATCH v2] PCI: quirk for preventing bus reset on TI C667X
Hi Antti, A few nitpicks, so feel free to ignore these, of course. If possible, capitalise the subject line. Also, perhaps "Add quirk to prevent bus (...)" might read better. > Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS [...] It would be KeyStone in the above sentence. [...] > With this change device can be assigned to VMs with VFIO, but it will > leak state between VMs. Following-up on Bojrn's question about the state leak, see: https://lore.kernel.org/linux-pci/20210129234946.GA124037@bjorn-Precision-5520/ Would there be anything else that has to be done? Krzysztof
Re: [PATCH] PCI: iproc: Fix return value of iproc_msi_irq_domain_alloc()
Hi Pali,
> IRQ domain alloc function should return zero on success. Non-zero value
> indicates failure.
>
> Signed-off-by: Pali Rohár
> Fixes: fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
[...]
Nice catch!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH 03/44] PCI: remove synclink entries from pci_ids
Hi Jiri, > The drivers were removed in a1f714b44e34 (tty: Remove redundant synclink > driver) and 3d608a591b2b (tty: Remove redundant synclinkmp driver). > > So remove also the PCI ID entries. [...] Thank you! Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH V3 2/2] PCI: Add MCFG quirks for Tegra194 host controllers
Hi Bjorn and Vidya,
[...]
> > +}
> > +
> > +struct pci_ecam_ops tegra194_pcie_ops = {
> > + .bus_shift = 20,
>
> I think e7708f5b10e2 ("PCI: Unify ECAM constants in native PCI Express
> drivers") means you don't need this .bus_shift.
[...]
Correct. If this platform implements ECAM as per the specification,
then the .bus_shift initializer is no longer needed.
Krzysztof
Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
Hi Jianjun, [...] > Thanks for your review, Thank YOU for all the work here! [...] > > > Add suspend_noirq and resume_noirq callback functions to implement > > > PM system suspend hooks for MediaTek Gen3 PCIe controller. > > > > So, "systems suspend" and "resume" hooks, correct? > > The callback functions is suspend_noirq and resume_noirq, should I use > "systems suspend" and "resume" in the commit message? [...] What I meant was something along these lines: Add suspend_noirq and resume_noirq callback functions to implement PM system suspend and resume hooks for the MediaTek Gen3 PCIe controller. When the system suspends, trigger the PCIe link to enter the L2 state and pull down the PERST# pin, gating the clocks of the MAC layer, and then power-off the physical layer to provide power-saving. When the system resumes, the PCIe link should be re-established and the related control register values should be restored. The above is just a suggestion, thus feel tree to ignore it completely, and it's heavily based on your original commit message. Krzysztof
Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support
Hi Jianjun,
[...]
> +static struct irq_chip mtk_msi_irq_chip = {
> + .name = "MSI",
> + .irq_enable = mtk_pcie_irq_unmask,
> + .irq_disable = mtk_pcie_irq_mask,
> + .irq_ack = irq_chip_ack_parent,
> + .irq_mask = mtk_pcie_irq_mask,
> + .irq_unmask = mtk_pcie_irq_unmask,
> +};
For consistency sake, what about aligning this like the
struct mtk_msi_bottom_irq_chip has been? See immediately below.
[...]
> +static struct irq_chip mtk_msi_bottom_irq_chip = {
> + .irq_ack= mtk_msi_bottom_irq_ack,
> + .irq_mask = mtk_msi_bottom_irq_mask,
> + .irq_unmask = mtk_msi_bottom_irq_unmask,
> + .irq_compose_msi_msg= mtk_compose_msi_msg,
> + .irq_set_affinity = mtk_pcie_set_affinity,
> + .name = "MSI",
> +};
Krzysztof
Re: [v8,4/7] PCI: mediatek-gen3: Add INTx support
Hi Jianjun,
[...]
> +/**
> + * mtk_intx_eoi
> + * @data: pointer to chip specific data
> + *
> + * As an emulated level IRQ, its interrupt status will remain
> + * until the corresponding de-assert message is received; hence that
> + * the status can only be cleared when the interrupt has been serviced.
> + */
[...]
See my comment about the kernel-doc from the following:
https://lore.kernel.org/linux-pci/YDZWUGcKet%2FlNWlF@rocinante/
[...]
> + if (err) {
> + dev_err(dev, "failed to init PCIe IRQ domain\n");
> + return err;
> + }
[...]
Just a nitpick. What about using "initialize" in the above?
Krzysztof
Re: [v8,6/7] PCI: mediatek-gen3: Add system PM support
Hi Jianjun,
> Add suspend_noirq and resume_noirq callback functions to implement
> PM system suspend hooks for MediaTek Gen3 PCIe controller.
So, "systems suspend" and "resume" hooks, correct?
> When system suspend, trigger the PCIe link to L2 state and pull down
It probably would be "the system suspends".
[...]
> When system resum, the PCIe link should be re-established and the
> related control register values should be restored.
Similarly to the above: "the system resumes".
[...]
> + if (err) {
> + dev_err(port->dev, "can not enter L2 state\n");
> + return err;
> + }
Most likely you want "cannot" or "can't" in the above error message.
> + /* Pull down the PERST# pin */
> + val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> + val |= PCIE_PE_RSTB;
> + writel_relaxed(val, port->base + PCIE_RST_CTRL_REG);
> +
> + dev_dbg(port->dev, "enter L2 state success");
Just a nitpick. What about "entered L2 states successfully"?
[...]
> + if (err) {
> + dev_err(port->dev, "resume failed\n");
> + return err;
> + }
This error message does not quite convey that the mtk_pcie_startup_port()
was the function that failed, which is only a part of what you have to do
to successfully resume.
> + dev_dbg(port->dev, "resume done\n");
A nitpick. Probably not needed, as lack of error message would mean
that the device resumed successfully after being suspended.
Krzysztof
Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
Hi Jianjun,
Thank you for all the work here!
[...]
> + * struct mtk_pcie_port - PCIe port information
> + * @dev: pointer to PCIe device
> + * @base: IO mapped register base
> + * @reg_base: Physical register base
> + * @mac_reset: mac reset control
> + * @phy_reset: phy reset control
> + * @phy: PHY controller block
> + * @clks: PCIe clocks
> + * @num_clks: PCIe clocks count for this port
It would be "MAC" and "PHY" in the above.
[...]
> + * mtk_pcie_config_tlp_header
> + * @bus: PCI bus to query
> + * @devfn: device/function number
> + * @where: offset in config space
> + * @size: data size in TLP header
> + *
> + * Set byte enable field and device information in configuration TLP header.
The kernel-doc above might be missing brief function description. See
the following for more concrete example:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
[...]
> +static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
> + resource_size_t cpu_addr,
> + resource_size_t pci_addr,
> + resource_size_t size,
> + unsigned long type, int num)
> +{
> + void __iomem *table;
> + u32 val;
> +
> + if (num >= PCIE_MAX_TRANS_TABLES) {
> + dev_err(port->dev, "not enough translate table[%d] for addr:
> %#llx, limited to [%d]\n",
The wording of this error message is a little confusing.
> + num, (unsigned long long) cpu_addr,
No space between the bracket and the variable name.
[...]
> + err = phy_init(port->phy);
> + if (err) {
> + dev_err(dev, "failed to initialize PCIe phy\n");
> + goto err_phy_init;
> + }
> +
> + err = phy_power_on(port->phy);
> + if (err) {
> + dev_err(dev, "failed to power on PCIe phy\n");
> + goto err_phy_on;
> + }
[...]
It would be "PHY" in the error messages above.
[...]
> + if (err) {
> + dev_err(dev, "clock init failed\n");
> + goto err_clk_init;
> + }
[...]
A nitpick, so feel free to ignore it, of course. What about "failed to
initialize clock" to keep the style consistent.
[...]
> + err = mtk_pcie_startup_port(port);
> + if (err) {
> + dev_err(dev, "PCIe startup failed\n");
[...]
Also a nitpick. What about "failed to bring PCIe link up"?
Krzysztof
Re: [PATCH v3 1/4] dt-bindings: PCI: ti,j721e: Add binding to represent refclk to the connector
Hi Kishon, [...] >clocks: > -maxItems: 1 > -description: clock-specifier to represent input to the PCIe > +minItems: 1 > +maxItems: 2 > +description: clock-specifier to represent input to the PCIe for 1 item. > + 2nd item if present represents reference clock to the connector. [...] I am not an expert on device trees, but what do you think of making this description to be as follows: description: |+ clock-specifier to represent input to the PCIe for 1 item. 2nd item if present represents reference clock to the connector. What do you think? Krzysztof
Re: [PATCH v2 1/1] PCI/RCEC: Fix RCiEP capable devices RCEC association
[+cc Lorenzo for visiblity] Hi, [...] > Fix rcec_assoc_rciep() using the PCI_SLOT() macro and convert the value > of "rciep->devfn" to a device number to ensure that the RCiEP devices > associated with the RCEC are linked when the RCEC is enumerated. > > [ Krzysztof: Update commit message. ] [...] Thank you! I appreciate that. However, we probably should drop this from the commit message. Perhaps either Bjorn or Lorenzo could do it when applying changes. Krzysztof
Re: [PATCH v4 1/4] PCI: Introduce pcim_alloc_irq_vectors()
Hi Robert, [...] > Obiously this is meant here: > > if (!pci_is_managed(dev)) [...] A question to improve my understanding for future reference. Was the previous approach of checking for "enabled" flag from struct pci_devres was not a good choice here? Krzysztof
Re: [PATCH 1/1] PCI/RCEC: Fix failure to inject errors to some RCiEP devices
Hi Qiuxu,
[...]
> > Agree to simplify the commit message. How about the following subject and
> > commit message?
> >
> > Subject:
> > Use device number to check RCiEPBitmap of RCEC
> >
> > Commit message:
> > rcec_assoc_rciep() used the combination of device number and function
> > number 'devfn' to check whether the corresponding bit in the
> > RCiEPBimap of RCEC was set. According to [1], it only needs to use the
> > device number to check the corresponding bit in the RCiEPBitmap was
> > set. So fix it by using PCI_SLOT() to convert 'devfn' to device number
> > for rcec_assoc_rciep(). [1] PCIe r5.0, sec "7.9.10.2 Association
> > Bitmap for RCiEPs"
>
> I took your suggestion and came up with the following:
>
> Function rcec_assoc_rciep() incorrectly used "rciep->devfn" (a single
> byte encoding the device and function number) as the device number to
> check whether the corresponding bit was set in the RCiEPBitmap of the
> RCEC (Root Complex Event Collector) while enumerating over each bit of
> the RCiEPBitmap.
>
> As per the PCI Express Base Specification, Revision 5.0, Version 1.0,
> Section 7.9.10.2, "Association Bitmap for RCiEPs", p. 935, only needs to
> use a device number to check whether the corresponding bit was set in
> the RCiEPBitmap.
>
> Fix rcec_assoc_rciep() using the PCI_SLOT() macro and convert the value
> of "rciep->devfn" to a device number to ensure that the RCiEP devices
> are associated with the RCEC are linked when the RCEC is enumerated.
>
> Using either of the following as the subject:
>
> PCI/RCEC: Use device number to check RCiEPBitmap of RCEC
> PCI/RCEC: Fix RCiEP capable devices RCEC association
>
> What do you think? Also, feel free to change whatever you see fit, of
> course, as tis is only a suggestion.
We could probably add the following:
Fixes: 507b460f8144 ("PCI/ERR: Add pcie_link_rcec() to associate RCiEPs")
Since this would where the issue was originally introduced. I forgot to
mention this in the previous message, apologies.
Krzysztof
Re: [PATCH 1/1] PCI/RCEC: Fix failure to inject errors to some RCiEP devices
[+cc Bjorn as we talked about RCiEP briefly on IRC] Hello Qiuxu, [...] > Sorry, just back from Chinese New Year holiday. Welcome back! I hope you had a nice rest, and also Happy New Year! [...] > > Would this only affect error injection or would this be also a generic > > problem > > with the driver itself causing issues regardless of whether it was an error > > injection or not for this particular device? I am asking, as there is a > > lot going on > > in the commit message. > > This is also a generic problem. Good to know. Bjorn was also wondering if this is potentially a sign of a larger probed with the RCiEP support. > > I wonder if simplifying this commit message so that it clearly explains > > what was > > broken, why, and how this patch is fixing it, would perhaps be an option? > > The > > backstory of how you found the issue while doing some testing and error > > injection is nice, but not sure if needed. > > > > What do you think? > > Agree to simplify the commit message. How about the following subject and > commit message? > > Subject: > Use device number to check RCiEPBitmap of RCEC > > Commit message: > rcec_assoc_rciep() used the combination of device number and function > number 'devfn' to check whether the corresponding bit in the > RCiEPBimap of RCEC was set. According to [1], it only needs to use the > device number to check the corresponding bit in the RCiEPBitmap was > set. So fix it by using PCI_SLOT() to convert 'devfn' to device number > for rcec_assoc_rciep(). [1] PCIe r5.0, sec "7.9.10.2 Association > Bitmap for RCiEPs" I took your suggestion and came up with the following: Function rcec_assoc_rciep() incorrectly used "rciep->devfn" (a single byte encoding the device and function number) as the device number to check whether the corresponding bit was set in the RCiEPBitmap of the RCEC (Root Complex Event Collector) while enumerating over each bit of the RCiEPBitmap. As per the PCI Express Base Specification, Revision 5.0, Version 1.0, Section 7.9.10.2, "Association Bitmap for RCiEPs", p. 935, only needs to use a device number to check whether the corresponding bit was set in the RCiEPBitmap. Fix rcec_assoc_rciep() using the PCI_SLOT() macro and convert the value of "rciep->devfn" to a device number to ensure that the RCiEP devices are associated with the RCEC are linked when the RCEC is enumerated. Using either of the following as the subject: PCI/RCEC: Use device number to check RCiEPBitmap of RCEC PCI/RCEC: Fix RCiEP capable devices RCEC association What do you think? Also, feel free to change whatever you see fit, of course, as tis is only a suggestion. Krzysztof
Re: [PATCH] PCI: imx6: Limit DBI register length for imx6qp pcie
[...]
> > Refer to commit 075af61c19cd ("PCI: imx6: Limit DBI register length"),
> > i.MX6QP PCIe has the similar issue.
> > Define the length of the DBI registers and limit config space to its
> > length for i.MX6QP PCIe too.
>
> You could probably flip these two sentences around to make the commit
> message read slightly better, so what about this (a suggestion):
>
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers beyond
> that point that otherwise would lead to an abort on a i.MX 6QuadPlus.
>
> See commit 075af61c19cd ("PCI: imx6: Limit DBI register length") that
> resolves a similar issue on a i.MX 6Quad PCIe.
[...]
If you do decide to send another version, then also use "PCIe" in the
subject, rather than "pcie". I forgot to mention this in the previous
message, apologies.
Krzysztof
Re: [PATCH] PCI: imx6: Limit DBI register length for imx6qp pcie
Hi Richard,
Thank you for sending the patch over!
> Refer to commit 075af61c19cd ("PCI: imx6: Limit DBI register length"),
> i.MX6QP PCIe has the similar issue.
> Define the length of the DBI registers and limit config space to its
> length for i.MX6QP PCIe too.
You could probably flip these two sentences around to make the commit
message read slightly better, so what about this (a suggestion):
Define the length of the DBI registers and limit config space to its
length. This makes sure that the kernel does not access registers beyond
that point that otherwise would lead to an abort on a i.MX 6QuadPlus.
See commit 075af61c19cd ("PCI: imx6: Limit DBI register length") that
resolves a similar issue on a i.MX 6Quad PCIe.
> Signed-off-by: Richard Zhu
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 0cf1333c0440..853ea8e82952 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1175,6 +1175,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .variant = IMX6QP,
> .flags = IMX6_PCIE_FLAG_IMX6_PHY |
>IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> + .dbi_length = 0x200,
> },
> [IMX7D] = {
> .variant = IMX7D,
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH v3 1/4] PCI: Introduce pcim_alloc_irq_vectors()
Hi Dejin,
> Introduce pcim_alloc_irq_vectors(), a device-managed version of
> pci_alloc_irq_vectors(). Introducing this function can simplify
> the error handling path in many drivers.
>
> And use pci_free_irq_vectors() to replace some code in pcim_release(),
> they are equivalent, and no functional change. It is more explicit
> that pcim_alloc_irq_vectors() is a device-managed function.
[...]
Some suggestions about the commit message as per:
https://lore.kernel.org/linux-pci/YCwE2cf9X%2FGd6lWy@rocinante/
> +/**
> + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors()
> + * @dev: PCI device to operate on
> + * @min_vecs:minimum number of vectors required (must be >=
> 1)
> + * @max_vecs:maximum (desired) number of vectors
> + * @flags: flags or quirks for the allocation
> + *
> + * Return the number of vectors allocated, (which might be smaller than
> + * @max_vecs) if successful, or a negative error code on error. If less
> + * than @min_vecs interrupt vectors are available for @dev the function
> + * will fail with -ENOSPC.
> + *
> + * It depends on calling pcim_enable_device() to make IRQ resources
> + * manageable.
> + */
> +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> + unsigned int max_vecs, unsigned int flags)
> +{
> + struct pci_devres *dr;
> +
> + dr = find_pci_dr(dev);
> + if (!dr || !dr->enabled)
> + return -EINVAL;
> +
> + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> +}
> +EXPORT_SYMBOL(pcim_alloc_irq_vectors);
[...]
Looks good! Thank you for adding kernel-doc here! Much appreciated.
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH v3 4/4] i2c: thunderx: Use the correct name of device-managed function
Hi Dejin, > Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors, > the pcim_alloc_irq_vectors() function, an explicit device-managed version > of pci_alloc_irq_vectors(). If pcim_enable_device() has been called > before, then pci_alloc_irq_vectors() is actually a device-managed > function. It is used here as a device-managed function, So replace it > with pcim_alloc_irq_vectors(). A few suggestions about the commit message in the following reply: https://lore.kernel.org/linux-pci/YCwE2cf9X%2FGd6lWy@rocinante/ Krzysztof
Re: [PATCH v3 3/4] i2c: designware: Use the correct name of device-managed function
Hi Dejin,
Thank you for all the changes, looks good!
You could improve the subject line, as it is very vague - what is the
new function name more correct? Was the other and/or the previous one
not correct? Seems like you are correcting a typo of sorts, rather than
introducing a new function in this file.
> Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
> the pcim_alloc_irq_vectors() function, an explicit device-managed
> version of pci_alloc_irq_vectors(). If pcim_enable_device() has been
> called before, then pci_alloc_irq_vectors() is actually
> a device-managed function. It is used here as a device-managed
> function, So replace it with pcim_alloc_irq_vectors().
The commit is good, but it could use some polish, so to speak.
A few suggestions to think about:
- What are we adding and/or changing, and why
- Why is using pcim_alloc_irq_vectors(), which is part
of the managed devices framework, a better alternative
to the pci_alloc_irq_vectors()
- And finally why this change allowed us to remove the
pci_free_irq_vectors()
> At the same time, simplify the error handling path.
The change simplifies the error handling path, how? A line of two which
explains how it has been achieved might help should someone reads the
commit message in the future.
[...]
> if (controller->setup) {
> r = controller->setup(pdev, controller);
> - if (r) {
> - pci_free_irq_vectors(pdev);
> + if (r)
> return r;
> - }
> }
>
> i2c_dw_adjust_bus_speed(dev);
> @@ -246,10 +244,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> i2c_dw_acpi_configure(>dev);
>
> r = i2c_dw_validate_speed(dev);
> - if (r) {
> - pci_free_irq_vectors(pdev);
> + if (r)
> return r;
> - }
>
> i2c_dw_configure(dev);
>
> @@ -269,10 +265,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
> adap->nr = controller->bus_num;
>
> r = i2c_dw_probe(dev);
> - if (r) {
> - pci_free_irq_vectors(pdev);
> + if (r)
> return r;
> - }
>
> pm_runtime_set_autosuspend_delay(>dev, 1000);
> pm_runtime_use_autosuspend(>dev);
> @@ -292,7 +286,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
>
> i2c_del_adapter(>adapter);
> devm_free_irq(>dev, dev->irq, dev);
> - pci_free_irq_vectors(pdev);
If pcim_release() is called should the pci_driver's probe callback fail,
and I assume that this is precisely the case, then all of the above make
sense in the view of using pcim_alloc_irq_vectors().
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH v3 2/4] Documentation: devres: Add pcim_alloc_irq_vectors()
Hi Dejin, Thank you again for all the work here! > Add pcim_alloc_irq_vectors(), a device-managed version of > pci_alloc_irq_vectors(). introducing this function can simplify > the error handling path in many drivers. The second sentence should most likely start with a capital letter. Having said that, people might ask - how does it simplify the error handling path? You might have to back this with a line of two to explain how does the change achieved that, so that when someone looks at the commit message it would be clear what the benefits of the change were. Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH v1 1/4] PCI: Introduce pcim_alloc_irq_vectors()
Hi Dejin and Andy, [...] > > > Question: wouldn't you need to call pci_free_irq_vectors() somewhere, > > > possibly to pcim_release() callback? Although, I am not sure where the > > > right place would be. > > > > > > I am asking, as the documentation (see [4]) suggests that one would have > > > to release allocated IRQ vectors (relevant exceprt): > > > > It's done in pcim_release() but not explicitly. > > > > if (dev->msi_enabled) > > pci_disable_msi(dev); > > if (dev->msix_enabled) > > pci_disable_msix(dev); > > > > Maybe above can be replaced by pci_free_irq_vectors() to be sure that any > > future change to PCI IRQ allocation APIs. > > > > Yes, I have checked and currently the above code is equivalent to > > pci_free_irq_vectors(). > > > > Dejin, please update your patch accordingly. > > > Hi Andy and Krzysztof, > > I have modified it and sent patch v2. thank you very much! Thank you Andy for double-checking! Much appreciated. Moving to pci_free_irq_vectors() directly looks great as we are also removing the surplus checks that pcim_release() is doing - since both the pci_disable_msi() and the pci_disable_msix() are doing internal validation to see whether MSI/MSI-X is currently enabled. Thank you again, Dejin and Andy. :) Krzysztof
Re: [PATCH] PCI: hotplug: Remove unused function pointer typedef acpiphp_callback
Hi Chen, > Remove the 'acpiphp_callback' typedef as it is not used. [...] Good catch! This typedef was initially added in 2005, and then it stopped being used around the Kernel version 3.7 release, which is also when the sole user of this typedef called acpiphp_for_each_slot() has also been retired. Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH v1 2/4] Documentation: devres: add pcim_alloc_irq_vectors()
Hello Dejin, [...] > add pcim_alloc_irq_vectors(), a explicit device-managed version of > pci_alloc_irq_vectors(). It would be "Add" at the start of the sentence. You could also drop the "explicit" word or replace it with "an explicit", if you want to keep it. Generally, this commit message could be improved - you could explain that you are updating the documentation to reflect the addition of this new function called pcim_alloc_irq_vectors(), and also briefly explain why it was added, etc. [...] >devm_pci_remap_cfg_resource() : ioremap PCI configuration space > resource > + pcim_alloc_irq_vectors() : managed irq vectors allocation [...] It would be "IRQ" here. Krzysztof
Re: [PATCH v1 3/4] i2c: designware: Use the correct name of device-managed function
Hi Dejin, See my comments regarding a very similar patch: https://lore.kernel.org/linux-pci/YCrnn+L42SR4N1PA@rocinante/ Krzysztof
Re: [PATCH v1 4/4] i2c: thunderx: Use the correct name of device-managed function
Hi Dejin, > Use the correct name of device-managed function to alloc irq vectors, > the pcim_alloc_irq_vectors() function, a explicit device-managed version > of pci_alloc_irq_vectors(). [...] It would be "IRQ" in the sentence above. Perhaps the "Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors ..." would read a little better - generally, the above sentence could be improved. You might also want to add a more details about why to use this new function instead of the previous one, etc. Krzysztof
Re: [PATCH v1 0/4] Introduce pcim_alloc_irq_vectors()
Hi Dejin, Thank you for working on this series! Do you have a link to the conversation that prompted addition of this new function? If so, then it would be nice to include a reference to it here (as a link to http://lore.kernel.org/) in the cover letter for reference, if possible, of course. Generally, it would also be nice to expand on things a little bit and explain why do you want to add this new function, and what problems does it solve. [...] > Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of > pci_alloc_irq_vectors(). and use the correct name of device-managed > function to alloc irq vectors in i2c drivers. Did you want to use a comma, instead of a period, in the sentence above? You could also probably drop the word "explicit". Krzysztof
Re: [PATCH v1 1/4] PCI: Introduce pcim_alloc_irq_vectors()
Hi Dejin,
Thank you for all the work here!
The subject and the commit message could be improved to include a little
more details about why do you want to do it, and what problems does it
aims to solve.
> Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
> pci_alloc_irq_vectors().
You can probably drop the "explicit" word from the sentence above.
> +/**
> + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors()
> + *
> + * It depends on calling pcim_enable_device() to make irq resources
> manageable.
> + */
It would be "IRQ" in the sentence above. Also see [1] for more details
about how to make a patch ready to be accepted.
Also, this comment looks like it's intended to be compliant with the
kernel-doc format, and if so, then you should describe each argument as
the bare minimum, so that the entire comment would become this function
documentation making it also a little more useful. See [2] for an
example of how to use kernel-doc.
> +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> + unsigned int max_vecs, unsigned int flags)
> +{
> + struct pci_devres *dr;
> +
> + /*Ensure that the pcim_enable_device() function has been called*/
The comment above has to be correctly aligned and it's also missing
spaces around the sentence to be properly formatted, see [3].
> + dr = find_pci_dr(dev);
> + if (!dr || !dr->enabled)
> + return -EINVAL;
> +
> + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> +}
Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
possibly to pcim_release() callback? Although, I am not sure where the
right place would be.
I am asking, as the documentation (see [4]) suggests that one would have
to release allocated IRQ vectors (relevant exceprt):
>> To automatically use MSI or MSI-X interrupt vectors, use the following
>> function:
>>
>> int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>> unsigned int max_vecs, unsigned int flags);
>>
>> which allocates up to max_vecs interrupt vectors for a PCI device.
>>
>> (...)
>>
>> Any allocated resources should be freed before removing the device using
>> the following function:
>>
>> void pci_free_irq_vectors(struct pci_dev *dev);
What do you think?
1.
https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com/
2. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
3. https://www.kernel.org/doc/html/latest/process/coding-style.html
4. https://www.kernel.org/doc/html/latest/PCI/msi-howto.html
Krzysztof
Re: [PATCH v5 15/15] dmaengine: dw-edma: Add pcim_iomap_table return check
Hi Gustavo,
> Detected by CoverityScan CID 16555 ("Dereference null return")
[...]
We can use the "Addresses-Coverity:" here, as it seems to become
a canonical thing for Coverity reports. It would also be nice to
briefly mention what the issues was, if possible.
Krzysztof
Re: [PATCH v5 14/15] dmaengine: dw-edma: Revert fix scatter-gather address calculation
Hi Gustavo, > Reverting the applied patch because it caused a regression on > ARC700 platform (32 bits). [...] Do you have the previous commit handy to reference here? Krzysztof
Re: [PATCH v5 09/15] dmaengine: dw-edma: Improve the linked list and data blocks definition
Hi Gustavo, > In the previous implementation the driver assumes that only existed 2 > memory spaces that would be equal distributed amount the write/read > channels. What do you think about: In the previous implementation the driver assumed that there existed only two memory spaces that would equally distribute the amount of read/write channels. > This might not be the case on some other implementations, therefore this > patches changes this requirement so that each write/read channel has [...] Probably "patch" here. Krzysztof
Re: [PATCH v5 05/15] dmaengine: dw-edma: Add PCIe VSEC data retrieval support
Hi Gustavo, > + /* > + * Tries to find if exists a PCIe Vendor-Specific Extended Capability > + * for the DMA, if exists one, then reconfigures with the new data [...] What about "if one exists" and "then reconfigures it". Missing period at the end of the sentence. Krzysztof
Re: [PATCH v5 04/15] PCI: Add pci_find_vsec_capability() to find a specific VSEC
Hi Gustavo, [...] > + * Typically this function will be called by the pci driver, which passes It would be "PCI" here. > + * through argument the 'struct pci_dev *' already pointing for the device > + * config space that is associated with the vendor and device ID which will > + * know which ID to search and what to do with it, however, it might be Probably "there might be". > + * cases that this function could be called outside of this scope and > + * therefore is the caller responsibility to check the vendor and/or [...] A suggestion. This commit message is a little hard to read and could be improved. It might just be me (by and large, and I am not a native English speaker), but it's actually easier to figure out what the function does after reading the implementation that from the comment. :) Krzysztof
Re: [PATCH v5 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig
Hi Gustavo, [...] > +config DW_XDATA_PCIE > + depends on PCI > + tristate "Synopsys DesignWare xData PCIe driver" > + help > + This driver allows controlling Synopsys DesignWare PCIe traffic > + generator IP also known as xData, present in Synopsys Designware > + PCIe Endpoint prototype. [...] To be consistent. It would be "DesignWare" in the sentence above. Krzysztof
Re: [PATCH 1/2] PCI: xilinx-nwl: Enable coherent PCIe DMA traffic using CCI
Hi Bharat,
Thank you for sending the patches over!
> Add support for routing PCIe DMA traffic coherently when
> Cache Coherent Interconnect (CCI) is enabled in the system.
> The "dma-coherent" property is used to determine if CCI is enabled
> or not.
> Refer https://developer.arm.com/documentation/ddi0470/k/preface
> for CCI specification.
[...]
A small nitpick, so feel free to ignore, of course.
Perhaps "Refer to" and "for the CCI", etc.
[...]
> + /* This routes the PCIe DMA traffic to go through CCI path */
> + if (of_dma_is_coherent(dev->of_node)) {
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX1) |
> + CFG_PCIE_CACHE, BRCFG_PCIE_RX1);
> + }
[...]
A suggestion.
You can drop the curly brackets here if you want to keep the style used
in the kernel, especially for when there is a single statement inside
the code block.
Krzysztof
Re: [PATCH 1/1] PCI/RCEC: Fix failure to inject errors to some RCiEP devices
Hi Qiuxu, Nice catch! Thank you for sending the fix over! [...] > On a Sapphire Rapids server, it failed to inject correctable errors > to the RCiEP device e8:02.0 which was associated with the RCEC device > e8:00.4. See the following error log before applying the patch: > > aer-inject -s e8:02.0 examples/correctable > Error: Failed to write, No such device > > This was because rcec_assoc_rciep() mistakenly used "rciep->devfn" as > device number to check whether the corresponding bit was set in > the RCiEPBitmap of the RCEC. So that the RCiEP device e8:02.0 wasn't > linked to the RCEC and resulted in the above error. > > Fix it by using PCI_SLOT() to convert rciep->devfn to device number. > Ensure that the RCiEP devices associated with the RCEC are linked to > the RCEC as the RCEC is enumerated. After applying the patch, correctable > errors can be injected to the RCiEP successfully. Would this only affect error injection or would this be also a generic problem with the driver itself causing issues regardless of whether it was an error injection or not for this particular device? I am asking, as there is a lot going on in the commit message. I wonder if simplifying this commit message so that it clearly explains what was broken, why, and how this patch is fixing it, would perhaps be an option? The backstory of how you found the issue while doing some testing and error injection is nice, but not sure if needed. What do you think? Krzysztof
Re: [PATCH] PCI: Use subdir-ccflags-* to inherit debug flag
Hi Bjorn,
Thank you! This looks great!
[...]
> commit e8e9aababe60 ("PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci
> hierarchy")
> Author: Junhao He
> Date: Thu Feb 4 19:30:15 2021 +0800
>
> PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy
>
> CONFIG_PCI_DEBUG=y adds -DDEBUG to CFLAGS, which enables things like
> pr_debug() and dev_dbg() (and hence pci_dbg()). Previously we added
> -DDEBUG for files in drivers/pci/, but not files in subdirectories of
> drivers/pci/.
>
> Add -DDEBUG to CFLAGS for all files below drivers/pci/ so CONFIG_PCI_DEBUG
> applies to the entire hierarchy.
>
> [bhelgaas: commit log]
> Link:
> https://lore.kernel.org/r/1612438215-33105-1-git-send-email-yangyic...@hisilicon.com
> Signed-off-by: Junhao He
> Signed-off-by: Yicong Yang
> Signed-off-by: Bjorn Helgaas
> Reviewed-by: Krzysztof Wilczyński
>
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc79411e2d..d62c4ac4ae1b 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
> obj-y+= controller/
> obj-y+= switch/
>
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
And thank you again, Yicong, for fixing this. Much appreciated.
Krzysztof
Re: [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return checker
Hi Gustavo, [...] > > That's true, there are a lot of drivers that don't verify that pointer. > > What do you suggest? > > 1) To remove the verification so that is aligned with the other drivers > > 2) Leave it as is. Or even to add this verification to the other drivers? > > > > Either way, I will add the pcim_iomap_table(pdev) before this > > instruction. > [...] > > A lot of the drivers consume the value from pcim_iomap_table() at > a given BAR index directly as-is, some check if the pointer they got > back is not NULL, a very few also check if the address at a given index > is not NULL. > > Given that the memory allocation for the table can fail, we ought to > check for a NULL pointer. It's a bit worrying that people decided to > consume the value it returns directly without any verification. > > I only found two drivers that perform this additional verification of > checking whether the address at a given index is valid, as per: > > https://lore.kernel.org/linux-pci/YCLFTjZQ2bCfGC+J@rocinante/ > > Personally, I would opt for (2), and then like you suggested send > a separate series to update other drivers so that they also include the > this NULL pointer check. > > But let's wait for Bjorn's take on this, though. As per Bjorn's reply: https://lore.kernel.org/linux-pci/20210209185246.GA494880@bjorn-Precision-5520/ These extra checks I proposed would be definitely too much, especially since almost everyone who uses pcim_iomap_table() also calls either pcim_iomap_regions() or pcim_iomap_regions_request_all() before accessing the table. There probably is also an opportunity to simplify some of the other drivers in the future, especially if do some API changes as per what Bjorn suggested. Sorry for taking your time, and thank you again! Krzysztof
Re: [PATCH v4 15/15] dmaengine: dw-edma: Add pcim_iomap_table return checker
Hi Gustavo, [...] > > This "pcim_iomap_table(dev)[n]" pattern is extremely common. There > > are over 100 calls of pcim_iomap_table(), and > > > > $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l > > > > says about 75 of them are of this form, where we dereference the > > result before testing it. > > That's true, there are a lot of drivers that don't verify that pointer. > What do you suggest? > 1) To remove the verification so that is aligned with the other drivers > 2) Leave it as is. Or even to add this verification to the other drivers? > > Either way, I will add the pcim_iomap_table(pdev) before this > instruction. [...] A lot of the drivers consume the value from pcim_iomap_table() at a given BAR index directly as-is, some check if the pointer they got back is not NULL, a very few also check if the address at a given index is not NULL. Given that the memory allocation for the table can fail, we ought to check for a NULL pointer. It's a bit worrying that people decided to consume the value it returns directly without any verification. I only found two drivers that perform this additional verification of checking whether the address at a given index is valid, as per: https://lore.kernel.org/linux-pci/YCLFTjZQ2bCfGC+J@rocinante/ Personally, I would opt for (2), and then like you suggested send a separate series to update other drivers so that they also include the this NULL pointer check. But let's wait for Bjorn's take on this, though. Krzysztof
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
Hi Gustavo,
[...]
> > The code in question would be (exceprt from the patch):
> >
> > [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *pid)
> > +{
> > + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > + struct dw_xdata *dw;
> > [...]
> > + dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > + if (!dw->rg_region.vaddr)
> > + return -ENOMEM;
> > [...]
> >
> > Perhaps something like the following would would?
> >
> > void __iomem * const *iomap_table;
> >
> > iomap_table = pcim_iomap_table(pdev);
> > if (!iomap_table)
> > return -ENOMEM;
> >
> > dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> > if (!dw->rg_region.vaddr)
> > return -ENOMEM;
> >
> > With sensible error messages added, of course. What do you think?
>
> I think all the improvements are welcome. I will do that.
> My only doubt is if Bjorn recommends removing the
> iomap_table[pdata->rg_bar] check, after adding the verification on the
> pcim_iomap_table, because all other drivers doesn't do that.
Good point. I only found two drivers that do this extra check:
https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/crypto/ccp/sp-pci.c#L203
https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/net/ethernet/amd/xgbe/xgbe-pci.c#L252
Bjorn, do you think that there is some likelihood that the table might
be missing a mapped address for a given BAR?
I don't think that is the case, but should it be the case, then perhaps
adding a small wrapper that would take a BAR and do all the verification
internally might be a good idea.
Krzysztof
Re: [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
Hi Gustavo, [...] > > When I do the following: > > > > # echo 1 > /sys/kernel/dw-xdata-pcie/write > > # echo 1 > /sys/kernel/dw-xdata-pcie/stop > > # cat /sys/kernel/dw-xdata-pcie/write > > > > Would output from cat above simply show "0 MB/s" then? I wonder how > > someone using this new driver could tell whether "write" or "read" > > traffic generation has been enabled aside of reading the sysfs files, > > would adding "/sys/kernel/dw-xdata-pcie/active" be an overkill here? > > > > What do you think? > > Yes, it would display 0 MB/s. This driver is to be used mainly by the > Synopsys DesignWare HW prototyping team. I don't think the general public > will be interested or can use this driver, because requires a special HW > block available only for this prototype. Got it. > I tried to reduce to the minimal the interfaces, to avoid possible > confusion. For instance, even the /sys/kernel/dw-xdata-pcie/stop > interface could be avoided, because on the driver unloading or changing > the between write or read it calls the stop procedure. Oh, OK. > Due to the nature of the HW block, it only can allow the write or the > read at some given moment, therefore based on the last command enable > write or read, we know which mode is this driving working. > This driver will be used by the testing team on their automation scripts, > thus they will know exactly the sequence input. > > Anyway, thanks for your feedback. Thank you for taking the time to add more details for me. Much appreciated. Krzysztof
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
[+cc Bjorn]
Hi Gustavo,
[...]
> Thanks for your review. I will wait for a couple of days, before sending
> a new version of this patch series based on your feedback.
Thank you!
There might be one more change, and improvement, to be done as per
Bjorn's feedback, see:
https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/
The code in question would be (exceprt from the patch):
[...]
+static int dw_xdata_pcie_probe(struct pci_dev *pdev,
+ const struct pci_device_id *pid)
+{
+ const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
+ struct dw_xdata *dw;
[...]
+ dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
+ if (!dw->rg_region.vaddr)
+ return -ENOMEM;
[...]
Perhaps something like the following would would?
void __iomem * const *iomap_table;
iomap_table = pcim_iomap_table(pdev);
if (!iomap_table)
return -ENOMEM;
dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
if (!dw->rg_region.vaddr)
return -ENOMEM;
With sensible error messages added, of course. What do you think?
Krzysztof
Re: [PATCHv2] PCI: Add Silicom Denmark vendor ID
Hi Martin, Thank you! > Update pci_ids.h with the vendor ID for Silicom Denmark. The define is > going to be referenced in driver(s) for FPGA accelerated smart NICs. > > Signed-off-by: Martin Hundebøll > --- > > Changes since v1: > * Align commit message/shortlog with similar changes to pci_ids.h > > include/linux/pci_ids.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index f968fcda338e..c119f0eb41b6 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2589,6 +2589,8 @@ > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > +#define PCI_VENDOR_ID_SILICOM_DENMARK0x1c2c > + > #define PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS 0x1c36 > > #define PCI_VENDOR_ID_CIRCUITCO 0x1cc8 > -- > 2.29.2 Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: [PATCH] pci: add Silicom Denmark vendor id
Hi Martin, Thank you for sending the patch over! A few suggestions. Try to keep the patch style aligned with previous submissions, see the following for to format the subject line, etc., a few examples: $ git log --oneline include/linux/pci_ids.h | grep -E 'Add.*ID' | head -n 10 8b7beaf9f185 PCI: Add Intel QuickAssist device IDs a4e91825d7e1 x86/amd_nb: Add AMD family 17h model 60h PCI IDs 3375590623e4 PCI: Add Zhaoxin Vendor ID 9acb9fe18d86 PCI: Add Loongson vendor ID b3f79ae45904 x86/amd_nb: Add Family 19h PCI IDs 4a36a60c34f4 PCI: Add Amazon's Annapurna Labs vendor ID 4460d68f0b2f PCI: Add Genesys Logic, Inc. Vendor ID af4e1c5eca95 x86/amd_nb: Add PCI device IDs for family 17h, model 70h 1f418f46503d PCI: Add Synopsys endpoint EDDA Device ID b8580e9de48b PCI: Add HXT vendor ID If you have a moment, see [1] which you can use as a guide for patches submissions against the PCI sub-system. [...] > Update pci_ids.h with the vendor id for Silicom Denmark. It would be "ID" here. [... > +#define PCI_VENDOR_ID_SILICOM_DENMARK0x1c2c Are they any drivers for this new device already? 1. https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com/ Krzysztof
Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
Hi Gustavo,
Thank you for all the work here!
A few suggestions.
[...]
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> + u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> +
> + if (burst & BIT(31)) {
> + burst &= ~(u32)BIT(31);
> + writel(burst, &(__dw_regs(dw)->burst_cnt));
> + }
> +}
Would it be possible to add a define for this "BIT(31)", similarly to
the "XPERF_CONTROL_ENABLE", for example:
#define XPERF_CONTROL_ENABLE BIT(5)
#define XPERF_CONTROL_DISABLE BIT(31)
What do you think?
> +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> +{
> + u32 control, status;
> +
> + /* Stop first if xfer in progress */
> + dw_xdata_stop(dw);
> +
> + /* Clear status register */
> + writel(0x0, &(__dw_regs(dw)->status));
> +
> + /* Burst count register set for continuous until stopped */
> + writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
Would you mind adding a define (possibly with a comment, if it makes
sense, of course) rather than open coding it here.
> + /* Pattern register */
> + writel(0x0, &(__dw_regs(dw)->pattern));
> +
> + /* Control register */
> + control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> + if (write) {
> + control |= CONTROL_IS_WRITE;
> + control |= CONTROL_LENGTH(dw->max_wr_len);
> + } else {
> + control |= CONTROL_LENGTH(dw->max_rd_len);
> + }
> + writel(control, &(__dw_regs(dw)->control));
> +
> + usleep_range(100, 150);
[...]
Why sleep here?
Do you just add some delay that changes were reflected, or is it
a requirement of sorts? What do you think about documenting why the
sleep where? Would it make sense?
[...]
> +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> +{
> + u64 data[2], time[2], diff;
> +
> + /* First measurement */
> + writel(0x0, &(__dw_regs(dw)->perf_control));
> + dw_xdata_perf_meas(dw, [0], write);
> + time[0] = jiffies;
> + writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> +
> + /* Delay 100ms */
> + mdelay(100);
[...]
The mdelay() is self-explanatory, so a comment that explains why to take
two measurements that are 100 ms apart and how rate is calculated might
be more useful (unless it would be an overkill here).
If this is an arbitrary delay without any special meaning, then probably
no comment is needed here.
What do you think?
[...]
> + /* Calculations */
What sort of calculations precisely? :)
[...]
> +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> +const struct pci_device_id *pid)
> +{
> + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> + struct dw_xdata *dw;
> + u64 addr;
> + int err;
> +
> + /* Enable PCI device */
> + err = pcim_enable_device(pdev);
This comment might not be needed.
[...]
> + /* Mapping PCI BAR regions */
> + err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
This comment might also not be needed.
[...]
> + /* Allocate memory */
And so this comment.
[...]
> + /* Data structure initialization */
[...]
> + /* Saving data structure reference */
[...]
> + /* Sysfs */
[...]
And possibly few of these are also not needed.
Krzysztof
Re: [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
Hi Gustavo, [...] > +The interaction with this driver is done through the module parameter and > +can be changed in runtime. The driver outputs the requested command state > +information to /var/log/kern.log or dmesg. The driver does not seem to offer any parameters (aside of using sysfs for runtime settings), and it also seem to only print what it's doing when debug level is enabled - unless I am missing something? [...] > +Request to stop any current TLP transfer: > +- Command: > + echo 1 > /sys/kernel/dw-xdata-pcie/stop [...] When I do the following: # echo 1 > /sys/kernel/dw-xdata-pcie/write # echo 1 > /sys/kernel/dw-xdata-pcie/stop # cat /sys/kernel/dw-xdata-pcie/write Would output from cat above simply show "0 MB/s" then? I wonder how someone using this new driver could tell whether "write" or "read" traffic generation has been enabled aside of reading the sysfs files, would adding "/sys/kernel/dw-xdata-pcie/active" be an overkill here? What do you think? Krzysztof
Re: [PATCH 1/1] PCI/AER: Change to use helper pcie_aer_is_native() in some places
Hi Tan,
This is very nice! Thank you for this.
[...]
> if (dev->aer_cap && pci_aer_available() &&
> - (pcie_ports_native || host->native_aer)) {
> + pcie_aer_is_native(dev)) {
> services |= PCIE_PORT_SERVICE_AER;
[...]
A suggestion. You could improve this even further, for example:
if (pci_aer_available() && pcie_aer_is_native(dev)) {
This is because the pcie_aer_is_native() function performs the
dev->aer_cap check internally, so we could rely on it, and avoid
checking the same thing twice.
What do you think?
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Re: [PATCH v2 09/11] PCI: dwc: pcie-kirin: allow using multiple reset GPIOs
Hi Mauro,
Thank you for working on this!
> @@ -151,8 +152,10 @@ struct kirin_pcie {
> struct clk *phy_ref_clk;
> struct clk *pcie_aclk;
> struct clk *pcie_aux_clk;
> - int gpio_id_reset[4];
> + int n_gpio_resets;
> int gpio_id_clkreq[3];
> + int gpio_id_reset[MAX_GPIO_RESETS];
> + const char *reset_names[MAX_GPIO_RESETS];
> u32 eye_param[5];
> };
[...]
A small nit, so feel free to ignore, of course.
The "n_gpio_resets" variable might be better as "gpio_resets_num" or
"gpio_resets_count" - both are popular name suffixes for that type of
variables. To add, other variables also start with "gpio_", thus it
would also follow the naming pattern.
[...]
> + kirin_pcie->n_gpio_resets = of_gpio_named_count(np, "reset-gpios");
[...]
This would then become (for example):
kirin_pcie->gpio_resets_count = of_gpio_named_count(np, "reset-gpios");
Krzysztof
Re: [PATCH] PCI/ASPM: Disable ASPM when save/restore PCI state
Hi Victor, Thank you for working on this! [...] > i = pci_save_pcie_state(dev); > if (i != 0) > - return i; > + goto Exit; > > i = pci_save_pcix_state(dev); > if (i != 0) > - return i; > + goto Exit; [...] > +Exit: > + pcie_restore_aspm_control(dev); > + return i; > } [...] A silly thing, but the goto labels are customary lower-case. Nonetheless, this is probably something that can be corrected when applying, so that you don't need to unnecessarily send a new version (unless you will eventually following other reviews, then don't forget about it). Krzysztof
Re: [PATCH v2] PCI: dwc: fix error return code in dw_pcie_host_init()
Hi Hai, Thank you for taking care about this. On 20-11-17 14:41:42, Wang Hai wrote: I would have to ask you to capitalise the first letter in the subject line as it has been done for other patches. Check Git history to see what it normally would look like. > Fix to return a negative error code from the error handling > case instead of 0, as done elsewhere in this function. The above commit message was taken from the first patch, and might not be accurate any more. As now you are passing an error code from the dma_mapping_error() function rather than just setting the ret variable. Also, the ret variable might have either undetermined value or some other value from previous assignment, not necessarily 0 there. Krzysztof
Re: [PATCH] PCI: Decode PCIe 64 GT/s link speed
Hi Gustavo, > PCIe r6.0, sec 7.5.3.18, defines a new 64.0 GT/s bit in the Supported > Link Speeds Vector of Link Capabilities 2. 64 GT/s already, nice. > This does not affect the speed of the link, which should be negotiated > automatically by the hardware; it only adds decoding when showing the > speed to the user. > > This patch adds the decoding of this new speed, previously, reading the > speed of a link operating at this speed showed "Unknown speed" instead > of "64.0 GT/s". Looks good! Thank you for taking care about this. Reviewed-by: Krzysztof Wilczyński Krzysztof
Re: PCI: Race condition in pci_create_sysfs_dev_files
Hello Pali! Sincere apologies for taking a long time to get back to you. On 20-11-04 17:29:31, Pali Rohár wrote: [...] > > Krzysztof, as Bjorn wrote, do you want to take this issue? > [...] Yes. I already talked to Bjorn about this briefly, and thus I am more than happy to take care about this. Most definitely. Krzysztof
Re: [PATCH] PCI: Add sysfs attribute for PCI device power state
Hi Maximilian,
On 20-11-02 15:15:20, Maximilian Luz wrote:
> While most PCI power-states can be queried from user-space via lspci,
> this has some limits. Specifically, lspci fails to provide an accurate
> value when the device is in D3cold as it has to resume the device before
> it can access its power state via the configuration space, leading to it
> reporting D0 or another on-state. Thus lspci can, for example, not be
> used to diagnose power-consumption issues for devices that can enter
> D3cold or to ensure that devices properly enter D3cold at all.
>
> To alleviate this issue, introduce a new sysfs device attribute for the
> PCI power state, showing the current power state as seen by the kernel.
Very nice! Thank you for adding this.
[...]
> +/* PCI power state */
> +static ssize_t power_state_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + pci_power_t state = READ_ONCE(pci_dev->current_state);
> +
> + return sprintf(buf, "%s\n", pci_power_name(state));
> +}
> +static DEVICE_ATTR_RO(power_state);
[...]
Curious, why did you decide to use the READ_ONCE() macro here? Some
other drivers exposing data through sysfs use, but certainly not all.
Krzysztof
[PATCH] PCI: Convert enum pci_bus_flags to bit fields in struct pci_bus
All the flags defined in the enum pci_bus_flags are used to determine
whether a particular feature of a PCI bus is available or not - features
are also often disabled via architecture or device-specific quirk.
These flags are tightly coupled with a PCI buses and PCI bridges and
primarily used in simple binary on/off manner to check whether something
is enabled or disabled, and have almost no other users (with an
exception of the x86 architecture-specific quirk) outside of the PCI
device drivers space.
Therefore, convert enum pci_bus_flags into a set of bit fields in the
struct pci_bus, and then drop said enum and the typedef pci_bus_flags_t.
This will keep PCI device-specific features as part of the struct
pci_dev and make the code that used to use flags simpler.
Related:
https://patchwork.kernel.org/patch/11772809
Suggested-by: Bjorn Helgaas
Signed-off-by: Krzysztof Wilczyński
---
arch/x86/pci/fixup.c| 6 +++---
drivers/pci/msi.c | 8
drivers/pci/pci-sysfs.c | 14 ++
drivers/pci/pci.c | 2 +-
drivers/pci/pcie/aer.c | 5 ++---
drivers/pci/probe.c | 13 +
drivers/pci/quirks.c| 16
include/linux/pci.h | 20 ++--
8 files changed, 43 insertions(+), 41 deletions(-)
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index b8c9a5b87f37..50ff8aa542b8 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -641,14 +641,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10,
quirk_apple_mbp_poweroff);
* ID, the AER driver should traverse the child device tree, reading
* AER registers to find the faulting device.
*/
-static void quirk_no_aersid(struct pci_dev *pdev)
+static void quirk_no_aer_sid(struct pci_dev *pdev)
{
/* VMD Domain */
if (is_vmd(pdev->bus) && pci_is_root_bus(pdev->bus))
- pdev->bus->bus_flags |= PCI_BUS_FLAGS_NO_AERSID;
+ pdev->bus->no_aer_sid = 1;
}
DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
- PCI_CLASS_BRIDGE_PCI, 8, quirk_no_aersid);
+ PCI_CLASS_BRIDGE_PCI, 8, quirk_no_aer_sid);
static void quirk_intel_th_dnv(struct pci_dev *dev)
{
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 30ae4ffda5c1..01e4bdbc830e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -875,13 +875,13 @@ static int pci_msi_supported(struct pci_dev *dev, int
nvec)
/*
* Any bridge which does NOT route MSI transactions from its
-* secondary bus to its primary bus must set NO_MSI flag on
-* the secondary pci_bus.
+* secondary bus to its primary bus must enable "no_msi" on
+* the secondary bus (pci_bus).
* We expect only arch-specific PCI host bus controller driver
-* or quirks for specific PCI bridges to be setting NO_MSI.
+* or quirks for specific PCI bridges to enable "no_msi".
*/
for (bus = dev->bus; bus; bus = bus->parent)
- if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+ if (bus->no_msi)
return 0;
return 1;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6d78df981d41..eca214e45418 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -357,9 +357,7 @@ static ssize_t msi_bus_show(struct device *dev, struct
device_attribute *attr,
struct pci_dev *pdev = to_pci_dev(dev);
struct pci_bus *subordinate = pdev->subordinate;
- return sprintf(buf, "%u\n", subordinate ?
- !(subordinate->bus_flags & PCI_BUS_FLAGS_NO_MSI)
- : !pdev->no_msi);
+ return sprintf(buf, "%u\n", subordinate ? !subordinate->no_msi :
!pdev->no_msi);
}
static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
@@ -376,9 +374,9 @@ static ssize_t msi_bus_store(struct device *dev, struct
device_attribute *attr,
return -EPERM;
/*
-* "no_msi" and "bus_flags" only affect what happens when a driver
-* requests MSI or MSI-X. They don't affect any drivers that have
-* already requested MSI or MSI-X.
+* "no_msi" enabled for device and bus only affect what happens
+* when a driver requests MSI or MSI-X. They don't affect any
+* drivers that have already requested MSI or MSI-X.
*/
if (!subordinate) {
pdev->no_msi = !val;
@@ -388,9 +386,9 @@ static ssize_t msi_bus_store(struct device *dev, struct
device_attribute *attr,
}
if (val)
- subordinate->bus_flags &= ~PCI_BUS_FLAGS_NO_MSI;
+ subordinate->no_msi = 0;
else
- subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+ subordinate->no
