From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F2D4FEB64DD for ; Fri, 30 Jun 2023 23:19:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=eoQo0Uej8maDXITbNdUGaZ/Bh8sj5luZFsQAeVUavIs=; b=D1Pfs/mZYSyuf0 9y7UDG2wWDLycqlibNwWLEoN3a2JwJftlIjoKXc8aoYeFhN97uMds6YiXJTX0YCtICDXABfY0xlA4 tocE9nzNPwlKtoQbGBZcpsQYOTULMkfaqmE4MNCVHiLPE0yVfiFeFuNhircJ+NazqiaKsslmiXz50 N9CSEZSuyTnXTievKXkTQdVydLCN2O6OkSBwfv2qI5VeImx7SYxQ+UBSwL0qfQ9LeaEL/lo7W30ke REVhiCP+R3WdgbfdQW/1bvamwOjKaT197J+gLpp3a22dd5eKIv49IgjUlocE6n4hWtw7LKGRz4Zb4 2Jifx8UcXNyDj74B4MJQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qFNNx-004kYJ-2S; Fri, 30 Jun 2023 23:18:41 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qFNNt-004kXa-14 for linux-arm-kernel@lists.infradead.org; Fri, 30 Jun 2023 23:18:38 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B8CD161516; Fri, 30 Jun 2023 23:18:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D9A84C433C8; Fri, 30 Jun 2023 23:18:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1688167115; bh=JKd3pcacA54AZ0fE+QcV56KctfOkUbfebcxzki2Xzw4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=ZMi4zhYmfvWJMdkvedanUYstuVE7V++h8+4bGqe8iRsO64USW/4d3yNZcbJWF4dFb iqTvVQKxd/hcW4amlQdMvCalBxakXpwsJTYx//KfXK+Ocq3HhdqzLAEEYyA/WYIIO9 m3cUTjAbAJXwWJTR0npi8sxQCqDafIpDyT5YR4y6LnkTsWJGTejQg1k828QAQJx+xF YLnwfYb5OtgQJjbOcQXgbr+hQQtFhmaqz89WhtJPqc0oVV2rfVNBe+d6Ybj7rH73xa 7quq7fo9yvPZswXaoXZA2zNn/lkZUWFjoQZtXWIaOEyaGkBTpKhMpv4g35BMDBquFB c2GoPvpi27qaQ== Date: Fri, 30 Jun 2023 18:18:32 -0500 From: Bjorn Helgaas To: Thippeswamy Havalige Cc: krzysztof.kozlowski@linaro.org, devicetree@vger.kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, bhelgaas@google.com, lorenzo.pieralisi@arm.com, linux-arm-kernel@lists.infradead.org, bharat.kumar.gogada@amd.com, michals@amd.com Subject: Re: [PATCH V5 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver Message-ID: <20230630231832.GA496495@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230628092812.1592644-4-thippeswamy.havalige@amd.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230630_161837_464857_66540671 X-CRM114-Status: GOOD ( 25.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jun 28, 2023 at 02:58:12PM +0530, Thippeswamy Havalige wrote: > Add support for Xilinx XDMA Soft IP core as Root Port. > ... > |Reported-by: kernel test robot > |Reported-by: Dan Carpenter > |Closes: https://lore.kernel.org/r/202305261250.2cs1phTS-lkp@intel.com/ Remove these. I mentioned this before: https://lore.kernel.org/r/ZHd/7AaLaGyr1jNA@bhelgaas > + * struct pl_dma_pcie - PCIe port information > + * @dev: Device pointer > + * @reg_base: IO Mapped Register Base > + * @irq: Interrupt number > + * @cfg: Holds mappings of config space window > + * @phys_reg_base: Physical address of reg base > + * @intx_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 Capitalize the intx_irq and lock descriptions so they match the others. "Legacy and error interrupt number" and "legacy interrupt number" sound like they overlap -- "legacy interrupt number" is part of both. Is that an error? > +static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus, unsigned int devfn) > +{ > + struct pl_dma_pcie *port = bus->sysdata; > + > + /* Check if link is up when trying to access downstream ports */ > + if (!pci_is_root_bus(bus)) { > + /* > + * If the link goes down after we check for link-up, we have a problem: > + * if a PIO request is initiated while link-down, the whole controller > + * hangs, and even after link comes up again, previous PIO requests > + * won't work, and a reset of the whole PCIe controller is needed. > + * Henceforth we need link-up check here to avoid sending PIO request > + * when link is down. Wrap this comment so it fits in 80 columns like the rest of the file. I think the comment was added because I pointed out that this is racy. Obviously the comment doesn't *fix* the race, and it actually doesn't even describe the race. Even with the xilinx_pl_dma_pcie_link_up() check, this is racy because xilinx_pl_dma_pcie_link_up() may tell you the link is up, but the link may go down before the driver attempts the config transaction. THAT is the race. If the controller hangs in that situation, that's a hardware defect, and from your comment, it sounds like it's unrecoverable. > + */ > + if (!xilinx_pl_dma_pcie_link_up(port)) > + return false; > +static int xilinx_pl_dma_pcie_intx_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) Wrap to fit in 80 columns like the rest of the file. > +static struct irq_chip xilinx_msi_irq_chip = { > + .name = "pl_dma_pciepcie:msi", Why does this name have two copies of "pcie" in it? This driver has four irq_chip structs; maybe the names could be more similar? xilinx_leg_irq_chip INTx xilinx_msi_irq_chip pl_dma_pciepcie:msi xilinx_irq_chip Xilinx MSI xilinx_pl_dma_pcie_event_irq_chip RC-Event > + /* Plug the INTx chained handler */ > + irq_set_chained_handler_and_data(port->intx_irq, > + xilinx_pl_dma_pcie_intx_flow, port); > + > + /* Plug the main event chained handler */ > + irq_set_chained_handler_and_data(port->irq, > + xilinx_pl_dma_pcie_event_flow, port); What's the reason for using chained IRQs? Can this be done without them? I don't claim to understand all the issues here, but it seems better to avoid chained IRQ handlers when possible: https://lore.kernel.org/all/877csohcll.ffs@tglx/ > + /*set the Bridge enable bit */ Space before "Set". I mentioned this before at https://lore.kernel.org/r/ZHd/7AaLaGyr1jNA@bhelgaas > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "missing \"reg\" property\n"); All your other error messages are capitalized. Make this one match. > + bridge->ops = (struct pci_ops *)&xilinx_pl_dma_pcie_ops.pci_ops; I don't think this cast is needed. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel