| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <acf6834f-6851-4daf-85da-076e6ca142db@foss.st.com>
Date: Mon, 25 Nov 2024 16:28:45 +0100
From: Christian Bruel <christian.bruel@...s.st.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <lpieralisi@...nel.org>, <kw@...ux.com>,
<manivannan.sadhasivam@...aro.org>, <robh@...nel.org>,
<bhelgaas@...gle.com>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<mcoquelin.stm32@...il.com>, <alexandre.torgue@...s.st.com>,
<p.zabel@...gutronix.de>, <cassel@...nel.org>,
<quic_schintav@...cinc.com>, <fabrice.gasnier@...s.st.com>,
<linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5] PCI: stm32: Add PCIe endpoint support for STM32MP25
On 11/12/24 21:38, Bjorn Helgaas wrote:
> On Tue, Nov 12, 2024 at 05:19:24PM +0100, Christian Bruel wrote:
>> Add driver to configure the STM32MP25 SoC PCIe Gen2 controller based on the
>> DesignWare PCIe core in endpoint mode.
>> Uses the common reference clock provided by the host.
>
>> +++ b/drivers/pci/controller/dwc/Kconfig
>
>> +config PCIE_STM32_EP
>> + tristate "STMicroelectronics STM32MP25 PCIe Controller (endpoint mode)"
>> + depends on ARCH_STM32 || COMPILE_TEST
>> + depends on PCI_ENDPOINT
>> + select PCIE_DW_EP
>> + help
>> + Enables endpoint support for DesignWare core based PCIe controller in found
>> + in STM32MP25 SoC.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called pcie-stm32-ep.
>
> Move as for the host mode entry.
>
>> +++ b/drivers/pci/controller/dwc/pcie-stm32-ep.c
>
>> +static const struct of_device_id stm32_pcie_ep_of_match[] = {
>> + { .compatible = "st,stm32mp25-pcie-ep" },
>> + {},
>> +};
>
> Move next to stm32_pcie_ep_driver.
>
>> +static void stm32_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> + struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> + enum pci_barno bar;
>> +
>> + for (bar = BAR_0; bar <= PCI_STD_NUM_BARS; bar++)
>
> Most users just use "bar = 0". BAR_0 is 0, but there's no real
> connection with PCI_STD_NUM_BARS, so I think 0 is probably better.
>
> Looks like this should be "bar < PCI_STD_NUM_BARS"?
oops, thanks
>
>> + dw_pcie_ep_reset_bar(pci, bar);
>> +
>> + /* Defer Completion Requests until link started */
>
> Not sure what a Completion Request is. Is this some internal STM or
> DWC thing? Or is this related to Request Retry Status completions for
> config requests?
this is sysconf bit maps to the app_req_retry_en Synopsys controller
signal. "When app_req_retry_en is asserted, the controller completes
incoming configuration requess with a CRS"
>
>> + regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> + STM32MP25_PCIECR_REQ_RETRY_EN,
>> + STM32MP25_PCIECR_REQ_RETRY_EN);
>> +}
>
>> +static int stm32_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> + unsigned int type, u16 interrupt_num)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> + switch (type) {
>> + case PCI_IRQ_INTX:
>> + return dw_pcie_ep_raise_intx_irq(ep, func_no);
>> + case PCI_IRQ_MSI:
>> + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> + default:
>> + dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>
> Is the compiler not smart enough to notice that this is unreachable?
>
>> +static void stm32_pcie_perst_deassert(struct dw_pcie *pci)
>> +{
>> + struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> + struct device *dev = pci->dev;
>> + struct dw_pcie_ep *ep = &pci->ep;
>> + int ret;
>> +
>> + if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_ENABLED) {
>> + dev_dbg(pci->dev, "Link is already enabled\n");
>> + return;
>> + }
>> +
>> + dev_dbg(dev, "PERST de-asserted by host. Starting link training\n");
>> +
>> + ret = pm_runtime_resume_and_get(dev);
>> + if (ret < 0) {
>> + dev_err(dev, "pm runtime resume failed: %d\n", ret);
>> + return;
>> + }
>> +
>> + ret = stm32_pcie_enable_resources(stm32_pcie);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable resources: %d\n", ret);
>> + pm_runtime_put_sync(dev);
>> + return;
>> + }
>> +
>> + ret = dw_pcie_ep_init_registers(ep);
>> + if (ret) {
>> + dev_err(dev, "Failed to complete initialization: %d\n", ret);
>> + stm32_pcie_disable_resources(stm32_pcie);
>> + pm_runtime_put_sync(dev);
>> + return;
>> + }
>> +
>> + pci_epc_init_notify(ep->epc);
>> +
>> + ret = stm32_pcie_enable_link(pci);
>> + if (ret) {
>> + dev_err(dev, "PCIe Cannot establish link: %d\n", ret);
>> + stm32_pcie_disable_resources(stm32_pcie);
>> + pm_runtime_put_sync(dev);
>> + return;
>> + }
>> +
>> + stm32_pcie->link_status = STM32_PCIE_EP_LINK_ENABLED;
>> +}
>
>> +static int stm32_add_pcie_ep(struct stm32_pcie *stm32_pcie,
>> + struct platform_device *pdev)
>> +{
>> + struct dw_pcie *pci = stm32_pcie->pci;
>> + struct dw_pcie_ep *ep = &pci->ep;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> + STM32MP25_PCIECR_TYPE_MASK,
>> + STM32MP25_PCIECR_EP);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pm_runtime_resume_and_get(dev);
>> + if (ret < 0) {
>> + dev_err(dev, "pm runtime resume failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + reset_control_assert(stm32_pcie->rst);
>> + reset_control_deassert(stm32_pcie->rst);
>> +
>> + ep->ops = &stm32_pcie_ep_ops;
>> +
>> + ret = dw_pcie_ep_init(ep);
>> + if (ret) {
>> + dev_err(dev, "failed to initialize ep: %d\n", ret);
>> + pm_runtime_put_sync(dev);
>> + return ret;
>> + }
>> +
>> + ret = stm32_pcie_enable_resources(stm32_pcie);
>> + if (ret) {
>> + dev_err(dev, "failed to enable resources: %d\n", ret);
>> + dw_pcie_ep_deinit(ep);
>> + pm_runtime_put_sync(dev);
>> + return ret;
>> + }
>> +
>> + ret = dw_pcie_ep_init_registers(ep);
>> + if (ret) {
>> + dev_err(dev, "Failed to initialize DWC endpoint registers\n");
>> + stm32_pcie_disable_resources(stm32_pcie);
>> + dw_pcie_ep_deinit(ep);
>> + pm_runtime_put_sync(dev);
>> + return ret;
>> + }
>
> Consider gotos for the error cases with a cleanup block at the end.
> There's a fair bit of repetition there as more things get initialized,
> and it's error-prone to extend this in the future.
>
> Same applies in stm32_pcie_perst_deassert().
>
>> + pci_epc_init_notify(ep->epc);
>> +
>> + return 0;
>> +}
>> +
>> +static int stm32_pcie_probe(struct platform_device *pdev)
>> +{
>> + struct stm32_pcie *stm32_pcie;
>> + struct dw_pcie *dw;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
>> + if (!stm32_pcie)
>> + return -ENOMEM;
>> +
>> + dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
>> + if (!dw)
>> + return -ENOMEM;
>
> Add blank line here.
>
>> + stm32_pcie->pci = dw;
>
>> +static struct platform_driver stm32_pcie_ep_driver = {
>> + .probe = stm32_pcie_probe,
>> + .remove_new = stm32_pcie_remove,
>
> .remove().
>
>> + .driver = {
>> + .name = "stm32-ep-pcie",
>> + .of_match_table = stm32_pcie_ep_of_match,
>> + },
>> +};
Powered by blists - more mailing lists