From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752152AbaIJQh6 (ORCPT ); Wed, 10 Sep 2014 12:37:58 -0400 Received: from service87.mimecast.com ([91.220.42.44]:33398 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751689AbaIJQh4 convert rfc822-to-8bit (ORCPT ); Wed, 10 Sep 2014 12:37:56 -0400 Date: Wed, 10 Sep 2014 17:37:47 +0100 From: Lorenzo Pieralisi To: Liviu Dudau Cc: Bjorn Helgaas , Arnd Bergmann , Rob Herring , Jason Gunthorpe , Benjamin Herrenschmidt , Catalin Marinas , Will Deacon , Russell King , linux-pci , Linus Walleij , Tanmay Inamdar , Grant Likely , Sinan Kaya , Jingoo Han , Kukjin Kim , Suravee Suthikulanit , linux-arch , LKML , Device Tree ML , LAKML , "grant.likely@linaro.org" Subject: Re: [PATCH v10 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT Message-ID: <20140910163746.GB19662@e102568-lin.cambridge.arm.com> References: <1410184472-17630-1-git-send-email-Liviu.Dudau@arm.com> <1410184472-17630-9-git-send-email-Liviu.Dudau@arm.com> <20140909133546.GB2636@e102568-lin.cambridge.arm.com> <20140910142241.GD27864@e106497-lin.cambridge.arm.com> <20140910151026.GB11640@e102568-lin.cambridge.arm.com> <20140910153233.GG27864@e106497-lin.cambridge.arm.com> MIME-Version: 1.0 In-Reply-To: <20140910153233.GG27864@e106497-lin.cambridge.arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-OriginalArrivalTime: 10 Sep 2014 16:37:49.0136 (UTC) FILETIME=[8ED4ED00:01CFCD15] X-MC-Unique: 114091017375323301 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 10, 2014 at 04:32:33PM +0100, Liviu Dudau wrote: [...] > > > > > + /* > > > > > + * If we failed translation or got a zero-sized region > > > > > + * then skip this range > > > > > + */ > > > > > + if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) > > > > > + continue; > > > > > + > > > > > + res = kzalloc(sizeof(struct resource), GFP_KERNEL); > > > > > + if (!res) { > > > > > + err = -ENOMEM; > > > > > + goto parse_failed; > > > > > + } > > > > > + > > > > > + err = of_pci_range_to_resource(&range, dev, res); > > > > > + if (err) { > > > > > + kfree(res); > > > > > > > > You might want to add a label to free res to make things more uniform. > > Sorry, not following you. How would a label help here? It was just a suggestion so ignore it if you do not think it is cleaner. It is to make code more uniform and undo operations in one place instead of doing it piecemeal (you kfree the res here and jump to complete the clean-up, whereas you might want to add a different label and a different goto destination and carry out the kfree there). I do not mind either, it is just what I noticed. > > > > > + goto parse_failed; > > > > > + } > > > > > + > > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > > + if (*io_base) > > > > > > > > You do not zero io_base in the first place so you should ask the API > > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > > to something else to detect multiple IO resources. > > No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm > hopying that no one is crazy enough to map PCI address space at CPU address zero. > Thanks for spotting the lack of initialisation though, I need to fix it. Mmm...wasn't a trick question sorry :D PCI host bridge /pci ranges: IO 0x00000000..0x0000ffff -> 0x00000000 More than one I/O resource converted. CPU offset for old range lost! MEM 0x41000000..0x7fffffff -> 0x41000000 pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bu-01] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff] From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Wed, 10 Sep 2014 17:37:47 +0100 Subject: [PATCH v10 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT In-Reply-To: <20140910153233.GG27864@e106497-lin.cambridge.arm.com> References: <1410184472-17630-1-git-send-email-Liviu.Dudau@arm.com> <1410184472-17630-9-git-send-email-Liviu.Dudau@arm.com> <20140909133546.GB2636@e102568-lin.cambridge.arm.com> <20140910142241.GD27864@e106497-lin.cambridge.arm.com> <20140910151026.GB11640@e102568-lin.cambridge.arm.com> <20140910153233.GG27864@e106497-lin.cambridge.arm.com> Message-ID: <20140910163746.GB19662@e102568-lin.cambridge.arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Sep 10, 2014 at 04:32:33PM +0100, Liviu Dudau wrote: [...] > > > > > + /* > > > > > + * If we failed translation or got a zero-sized region > > > > > + * then skip this range > > > > > + */ > > > > > + if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) > > > > > + continue; > > > > > + > > > > > + res = kzalloc(sizeof(struct resource), GFP_KERNEL); > > > > > + if (!res) { > > > > > + err = -ENOMEM; > > > > > + goto parse_failed; > > > > > + } > > > > > + > > > > > + err = of_pci_range_to_resource(&range, dev, res); > > > > > + if (err) { > > > > > + kfree(res); > > > > > > > > You might want to add a label to free res to make things more uniform. > > Sorry, not following you. How would a label help here? It was just a suggestion so ignore it if you do not think it is cleaner. It is to make code more uniform and undo operations in one place instead of doing it piecemeal (you kfree the res here and jump to complete the clean-up, whereas you might want to add a different label and a different goto destination and carry out the kfree there). I do not mind either, it is just what I noticed. > > > > > + goto parse_failed; > > > > > + } > > > > > + > > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > > + if (*io_base) > > > > > > > > You do not zero io_base in the first place so you should ask the API > > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > > to something else to detect multiple IO resources. > > No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm > hopying that no one is crazy enough to map PCI address space at CPU address zero. > Thanks for spotting the lack of initialisation though, I need to fix it. Mmm...wasn't a trick question sorry :D PCI host bridge /pci ranges: IO 0x00000000..0x0000ffff -> 0x00000000 More than one I/O resource converted. CPU offset for old range lost! MEM 0x41000000..0x7fffffff -> 0x41000000 pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bu-01] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff]