From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ig0-f182.google.com ([209.85.213.182]:33203 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751353AbbESXPB (ORCPT ); Tue, 19 May 2015 19:15:01 -0400 Received: by igbpi8 with SMTP id pi8so119930848igb.0 for ; Tue, 19 May 2015 16:15:00 -0700 (PDT) Date: Tue, 19 May 2015 18:14:56 -0500 From: Bjorn Helgaas To: Hauke Mehrtens Cc: Ray Jui , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Arnd Bergmann , Linux PCI , Scott Branden , "linux-arm-kernel@lists.infradead.org" , bcm-kernel-feedback-list Subject: Re: [PATCH 2/2] PCI: iproc: add bcma pcie driver Message-ID: <20150519231456.GX31666@google.com> References: <1431465781-10753-1-git-send-email-hauke@hauke-m.de> <1431465781-10753-3-git-send-email-hauke@hauke-m.de> <5553742E.7070204@broadcom.com> <55537C43.4050606@broadcom.com> <55539B63.2060203@hauke-m.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <55539B63.2060203@hauke-m.de> Sender: linux-pci-owner@vger.kernel.org List-ID: On Wed, May 13, 2015 at 08:43:47PM +0200, Hauke Mehrtens wrote: > On 05/13/2015 06:30 PM, Ray Jui wrote: > > Hi Rafal, > > > > On 5/13/2015 9:19 AM, Rafał Miłecki wrote: > >> On 13 May 2015 at 17:56, Ray Jui wrote: > >>> On 5/12/2015 11:27 PM, Rafał Miłecki wrote: > >>>> On 12 May 2015 at 23:23, Hauke Mehrtens wrote: > >>>>> This driver adds support for the PCIe 2.0 controller found on the bcma > >>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X / > >>>>> BCM5301X ARM SoCs. > >>>>> > >>>>> The driver found in the Broadcom SDK does some more stuff, like setting > >>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some > >>>>> PHY changes like "improving" the PCIe jitter and doing some special > >>>>> initializations for the 3rd PCIe port. > >>>>> > >>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards > >>>>> connected to them. > >>>>> > >>>>> PCI_DOMAINS is needed by this driver, because normally there is more > >>>>> than one PCIe controller and without PCI_DOMAINS only the first > >>>>> controller gets registered. > >>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events. > >>>>> > >>>>> Signed-off-by: Hauke Mehrtens > >>>> > >>>> Acked-by: Rafał Miłecki > >>>> > >>>> > >>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev) > >>>>> +{ > >>>>> + struct iproc_pcie *pcie; > >>>>> + LIST_HEAD(res); > >>>>> + struct resource res_mem; > >>>>> + int ret; > >>>>> + > >>>>> + pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL); > >>>>> + if (!pcie) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> + pcie->dev = &bdev->dev; > >>>>> + bcma_set_drvdata(bdev, pcie); > >>>>> + > >>>>> + pcie->base = bdev->io_addr; > >>>>> + > >>>>> + res_mem.start = bdev->addr_s[0]; > >>>>> + res_mem.end = bdev->addr_s[0] + SZ_128M - 1; > >>>>> + res_mem.name = "PCIe MEM space"; > >>>>> + res_mem.flags = IORESOURCE_MEM; > >>>>> + pci_add_resource(&res, &res_mem); > >>>>> + > >>>>> + pcie->resources = &res; > >>>>> + > >>>>> + pcie->map_irq = iproc_pcie_bcma_map_irq; > >>>>> + > >>>>> + ret = iproc_pcie_setup(pcie); > >>>> > >>>> I think I don't like this part of iproc design. It lefts > >>>> pcie->resources pointing to some random memory after the setup/probe > >>>> are done. Guess it should be a separated parameter or sth. > >>>> > >>>> The patch is still OK, I just refer to generic iproc possible issue. > >>>> > >>> Sorry Rafal, but could you please be more specific on this? > >> > >> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res" > >> variable (each its own). They do: > >> pcie->resources = &res; > >> and then they call > >> iproc_pcie_setup(pcie); > >> > >> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the > >> pointer pcie->resources is not valid anymore, yet pcie struct is still > >> in use. Of course pcie->resources isn't used anymore, but still, it's > >> some in-struct pointer (to the random memory since local variable is > >> not accessible anymore). > >> > >> I think you should drop > >> struct list_head *resources; > >> from the struct iproc_pcie and use > >> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources) > >> > > > > Okay thanks. That makes sense. > > > > Or I should keep a copy of the resources under pcie->resources. In the > > current pcie-iproc.c, the resource is not used anywhere else except when > > creating the root bus under iproc_pcie_setup. But in the future, I might > > need to add more code to explicitly program the outbound/inbound windows > > so this driver can work with some other iProc SoCs where the desired > > windows do not match power-on-reset values. > > > > I plan to change this along with the window programming patch in the > > future. I also think Hauke's current patch is fine. > > > > Thanks, > > > > Ray > > > I think parts of this resources are allocated and never freed. > > pci_add_resource() allocates some bytes, but I do not see where they are > freed, this also applies to the platform driver. Where are we on this patch? I see Ray's ack for [1/2], and a "I think the current patch is fine"; is that an ack for [2/2] as well? Bjorn From mboxrd@z Thu Jan 1 00:00:00 1970 From: bhelgaas@google.com (Bjorn Helgaas) Date: Tue, 19 May 2015 18:14:56 -0500 Subject: [PATCH 2/2] PCI: iproc: add bcma pcie driver In-Reply-To: <55539B63.2060203@hauke-m.de> References: <1431465781-10753-1-git-send-email-hauke@hauke-m.de> <1431465781-10753-3-git-send-email-hauke@hauke-m.de> <5553742E.7070204@broadcom.com> <55537C43.4050606@broadcom.com> <55539B63.2060203@hauke-m.de> Message-ID: <20150519231456.GX31666@google.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, May 13, 2015 at 08:43:47PM +0200, Hauke Mehrtens wrote: > On 05/13/2015 06:30 PM, Ray Jui wrote: > > Hi Rafal, > > > > On 5/13/2015 9:19 AM, Rafa? Mi?ecki wrote: > >> On 13 May 2015 at 17:56, Ray Jui wrote: > >>> On 5/12/2015 11:27 PM, Rafa? Mi?ecki wrote: > >>>> On 12 May 2015 at 23:23, Hauke Mehrtens wrote: > >>>>> This driver adds support for the PCIe 2.0 controller found on the bcma > >>>>> bus. This controller can be found on (mostly) all Broadcom BCM470X / > >>>>> BCM5301X ARM SoCs. > >>>>> > >>>>> The driver found in the Broadcom SDK does some more stuff, like setting > >>>>> up some DMA memory areas, chaining MPS and MRRS to 512 and also some > >>>>> PHY changes like "improving" the PCIe jitter and doing some special > >>>>> initializations for the 3rd PCIe port. > >>>>> > >>>>> This was tested on a bcm4708 board with 2 PCIe ports and wireless cards > >>>>> connected to them. > >>>>> > >>>>> PCI_DOMAINS is needed by this driver, because normally there is more > >>>>> than one PCIe controller and without PCI_DOMAINS only the first > >>>>> controller gets registered. > >>>>> This controller gets 6 IRQs, the last one is trigged by all IRQ events. > >>>>> > >>>>> Signed-off-by: Hauke Mehrtens > >>>> > >>>> Acked-by: Rafa? Mi?ecki > >>>> > >>>> > >>>>> +static int iproc_pcie_bcma_probe(struct bcma_device *bdev) > >>>>> +{ > >>>>> + struct iproc_pcie *pcie; > >>>>> + LIST_HEAD(res); > >>>>> + struct resource res_mem; > >>>>> + int ret; > >>>>> + > >>>>> + pcie = devm_kzalloc(&bdev->dev, sizeof(*pcie), GFP_KERNEL); > >>>>> + if (!pcie) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> + pcie->dev = &bdev->dev; > >>>>> + bcma_set_drvdata(bdev, pcie); > >>>>> + > >>>>> + pcie->base = bdev->io_addr; > >>>>> + > >>>>> + res_mem.start = bdev->addr_s[0]; > >>>>> + res_mem.end = bdev->addr_s[0] + SZ_128M - 1; > >>>>> + res_mem.name = "PCIe MEM space"; > >>>>> + res_mem.flags = IORESOURCE_MEM; > >>>>> + pci_add_resource(&res, &res_mem); > >>>>> + > >>>>> + pcie->resources = &res; > >>>>> + > >>>>> + pcie->map_irq = iproc_pcie_bcma_map_irq; > >>>>> + > >>>>> + ret = iproc_pcie_setup(pcie); > >>>> > >>>> I think I don't like this part of iproc design. It lefts > >>>> pcie->resources pointing to some random memory after the setup/probe > >>>> are done. Guess it should be a separated parameter or sth. > >>>> > >>>> The patch is still OK, I just refer to generic iproc possible issue. > >>>> > >>> Sorry Rafal, but could you please be more specific on this? > >> > >> iproc_pcie_pltfm_probe (and iproc_pcie_bcma_probe) have local "res" > >> variable (each its own). They do: > >> pcie->resources = &res; > >> and then they call > >> iproc_pcie_setup(pcie); > >> > >> After iproc_pcie_pltfm_probe / iproc_pcie_bcma_probe returns, the > >> pointer pcie->resources is not valid anymore, yet pcie struct is still > >> in use. Of course pcie->resources isn't used anymore, but still, it's > >> some in-struct pointer (to the random memory since local variable is > >> not accessible anymore). > >> > >> I think you should drop > >> struct list_head *resources; > >> from the struct iproc_pcie and use > >> iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *resources) > >> > > > > Okay thanks. That makes sense. > > > > Or I should keep a copy of the resources under pcie->resources. In the > > current pcie-iproc.c, the resource is not used anywhere else except when > > creating the root bus under iproc_pcie_setup. But in the future, I might > > need to add more code to explicitly program the outbound/inbound windows > > so this driver can work with some other iProc SoCs where the desired > > windows do not match power-on-reset values. > > > > I plan to change this along with the window programming patch in the > > future. I also think Hauke's current patch is fine. > > > > Thanks, > > > > Ray > > > I think parts of this resources are allocated and never freed. > > pci_add_resource() allocates some bytes, but I do not see where they are > freed, this also applies to the platform driver. Where are we on this patch? I see Ray's ack for [1/2], and a "I think the current patch is fine"; is that an ack for [2/2] as well? Bjorn