| 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: <SN7PR12MB720174840144F13B58F1385B8B6A9@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Thu, 27 Apr 2023 05:55:59 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Rob Herring <robh@...nel.org>
CC: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"michals@...inx.com" <michals@...inx.com>,
"Yeleswarapu, Nagaradhesh" <nagaradhesh.yeleswarapu@....com>,
"Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>,
"lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>
Subject: RE: [PATCH 2/2] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver
Hi Rob Herring,
Thanks for reviewing, please find my inline response to your comments.
Regards,
Thippeswamy H
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
>
> I don't think you need either of these.
- Agreed and will fix this in next patch.
>
> > +#define XILINX_PCIE_DMA_REG_MSI_LOW_MASK 0x00000178
> > +#define XILINX_PCIE_DMA_REG_MSI_HI_MASK 0x0000017c
> > +
> > +/* Interrupt registers definitions */
> > +#define XILINX_PCIE_DMA_INTR_LINK_DOWN 0
> > +#define XILINX_PCIE_DMA_INTR_HOT_RESET 3
> > +#define XILINX_PCIE_DMA_INTR_CFG_TIMEOUT 8
> > +#define XILINX_PCIE_DMA_INTR_CORRECTABLE 9
> > +#define XILINX_PCIE_DMA_INTR_NONFATAL 10
> > +#define XILINX_PCIE_DMA_INTR_FATAL 11
> > +#define XILINX_PCIE_DMA_INTR_INTX 16
> > +#define XILINX_PCIE_DMA_INTR_MSI 17
> > +#define XILINX_PCIE_DMA_INTR_SLV_UNSUPP 20
> > +#define XILINX_PCIE_DMA_INTR_SLV_UNEXP 21
> > +#define XILINX_PCIE_DMA_INTR_SLV_COMPL 22
> > +#define XILINX_PCIE_DMA_INTR_SLV_ERRP 23
> > +#define XILINX_PCIE_DMA_INTR_SLV_CMPABT 24
> > +#define XILINX_PCIE_DMA_INTR_SLV_ILLBUR 25
> > +#define XILINX_PCIE_DMA_INTR_MST_DECERR 26
> > +#define XILINX_PCIE_DMA_INTR_MST_SLVERR 27
>
> Looks like a superset of the pcie-xilinx-cpm.c registers. You can't share some
> code here? Like all the interrupt handling code which does nothing more
> than print debug messages...
- Agreed, will add above macro's to common header file and fix it next patch
>
> > + * struct xilinx_pcie_dma - PCIe port information
> > + * @reg_base: IO Mapped Register Base
> > + * @irq: Interrupt number
> > + * @cfg: Holds mappings of config space window
> > + * @root_busno: Root Bus number
> > + * @dev: Device pointer
> > + * @phys_reg_base: Physical address of reg base
> > + * @leg_domain: Legacy IRQ domain pointer
> > + * @pldma_domain: PL DMA IRQ domain pointer
> > + * @resources: Bus Resources
> > + * @msi: MSI information
> > + * @irq_misc: Legacy and error interrupt number
> > + * @intx_irq: legacy interrupt number
> > + * @lock: lock protecting shared register access */ struct
> > +xilinx_pcie_dma {
> > + void __iomem *reg_base;
> > + u32 irq;
> > + struct pci_config_window *cfg;
> > + u8 root_busno;
>
> No need to store this. You can use pci_is_root_bus().
- Agreed and fix it in next patch
> > + struct device *dev;
> > + phys_addr_t phys_reg_base;
> > +*port) {
> > + return (pcie_read(port, XILINX_PCIE_DMA_REG_PSCR) &
> > + XILINX_PCIE_DMA_REG_PSCR_LNKUP) ? 1 : 0; }
> > +
> > +/**
> > + * xilinx_pcie_dma_clear_err_interrupts - Clear Error Interrupts
> > + * @port: PCIe port information
>
> You don't really need kerneldoc comments on private functions.
- Agreed, will fix it in next patch
> > + */
> > +static void xilinx_pcie_dma_clear_err_interrupts(struct
> > +xilinx_pcie_dma *port) {
> > + unsigned long val = pcie_read(port, XILINX_PCIE_DMA_REG_RPEFR);
> > +
> > + if (val & XILINX_PCIE_DMA_RPEFR_ERR_VALID) {
> > + dev_dbg(port->dev, "Requester ID %lu\n",
> > + val & XILINX_PCIE_DMA_RPEFR_REQ_ID);
> > + pcie_write(port, XILINX_PCIE_DMA_RPEFR_ALL_MASK,
> > + XILINX_PCIE_DMA_REG_RPEFR);
> > + }
> > +}
> > + *
> > + * Return: 'true' on success and 'false' if invalid device is found
> > +*/ static bool xilinx_pcie_dma_valid_device(struct pci_bus *bus,
> > +unsigned int devfn) {
> > + struct xilinx_pcie_dma *port = bus->sysdata;
> > +
> > + /* Check if link is up when trying to access downstream ports */
> > + if (bus->number != port->root_busno)
>
> Use pci_is_root_bus()
- Agreed,fix it in next patch
> > + if (!xilinx_pcie_dma_linkup(port))
> > + return false;
>
> The link went down right here after you checked. What happens next?
- for above mentioned case, for the transactions which are sent but not completed, when link goes down, the bridge responds with SLVERR for these requests.
> > +
> > + /* Only one device down on each root port */
> > + if (bus->number == port->root_busno && devfn > 0)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * Return: '0' on success and error value on failure */ static int
> > +xilinx_pcie_dma_init_irq_domain(struct xilinx_pcie_dma *port) {
> > + struct device *dev = port->dev;
> > + struct device_node *node = dev->of_node;
> > + struct device_node *pcie_intc_node;
> > + int ret;
> > +
> > + /* Setup INTx */
> > + pcie_intc_node = of_get_next_child(node, NULL);
>
> This breaks if someone puts the PCI devices into DT (which is perfectly valid
> to do on any PCI host bridge). It also assumes some order of child nodes
> which is undefined. Be specific about what child node you want.
--> Agreed, fix it in next patch
> > + if (!pcie_intc_node) {
> > + dev_err(dev, "No PCIe Intc node found\n");
> > + return PTR_ERR(pcie_intc_node);
> > + }
> > +
> > + port->pldma_domain = irq_domain_add_linear(pcie_intc_node, 32,
> > + &event_domain_ops, port);
> > + if (!port->pldma_domain)
> > + goto out;
> > +
> > + irq_domain_update_bus_token(port->pldma_domain,
> DOMAIN_BUS_NEXUS);
> > +
> > + port->leg_domain = irq_domain_add_linear(pcie_intc_node,
> PCI_NUM_INTX,
> > + &intx_domain_ops,
> > + struct resource regs;
> > + int err;
> > +
> > + err = of_address_to_resource(node, 0, ®s);
Use platform_get_resource() instead.
--> Agreed, fix it in next patch
> > + if (err) {
> > + dev_err(dev, "missing \"reg\" property\n");
> > + return err;
> &xilinx_pcie_dma_ops);
> > + if (IS_ERR(port->cfg))
> > + return -1;
>
> Return the original error value. -1 should never be used.
--> Agreed, fix in next patch
> > +
> > + port->reg_base = port->cfg->win;
> > +
Powered by blists - more mailing lists