Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753700AbbFIMe3 (ORCPT ); Tue, 9 Jun 2015 08:34:29 -0400 Received: from mail-la0-f41.google.com ([209.85.215.41]:35210 "EHLO mail-la0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932341AbbFIMeU (ORCPT ); Tue, 9 Jun 2015 08:34:20 -0400 MIME-Version: 1.0 In-Reply-To: <20150609111320.F299FC40580@trevor.secretlab.ca> References: <1433501478-15164-1-git-send-email-ricardo.ribalda@gmail.com> <20150608182310.1F13FC406AA@trevor.secretlab.ca> <20150609111320.F299FC40580@trevor.secretlab.ca> From: Ricardo Ribalda Delgado Date: Tue, 9 Jun 2015 14:33:57 +0200 Message-ID: Subject: Re: [PATCH 1/2] kernel/resource: Add new flag IORESOURCE_SHARED To: Grant Likely Cc: Rob Herring , Rob Herring , Andrew Morton , Bjorn Helgaas , Vivek Goyal , Jakub Sitnicki , Mike Travis , Jiang Liu , Thierry Reding , "devicetree@vger.kernel.org" , LKML , Greg Kroah-Hartman , Tejun Heo , Cliff Wickman Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4931 Lines: 160 Hello Grant On Tue, Jun 9, 2015 at 1:13 PM, Grant Likely wrote: > > On Mon, 8 Jun 2015 22:02:06 +0200 > , Ricardo Ribalda Delgado > wrote: > > Hello Grant > > > > On Mon, Jun 8, 2015 at 8:23 PM, Grant Likely wrote: > > > On Fri, 5 Jun 2015 12:51:17 +0200 > > > , Ricardo Ribalda Delgado > > > wrote: > > >> Some device tree platforms have not defined correctly their memory > > >> resources (i.e. Overlapping or duplication of resources). > > >> To avoid this issue we have historically avoided to add their resources to > > >> the resource tree. This leads to code duplication and oops when trying to > > >> unload dynamically a device tree (feature introduced recently). > > >> > > >> This new flag tells the resource system that a resource can be shared by > > >> multiple owners, so we can support device trees with problems at the > > >> same time that we do not duplicate code or crash when unloading the > > >> device tree. > > >> > > >> Signed-off-by: Ricardo Ribalda Delgado > > >> --- > > > > > > I'm really not comfortable with this change. The resource tree code is > > > complicated enough as is. Adding this exception case quite probably adds > > > corner cases that aren't property dealt with. If two regions overlay, > > > and then request_region is called? Which region does it become a child > > > of? And that's just off the top of my head. I don't want to hack in > > > changes to the resource code for what is a corner case. > > > > I see your concern, perhaps you could provide a testcase and we can > > find out if it fails or not. So far I have tested a device tree with > > two devices on the same memory region, each device managed by a > > driver. > > Actually, you need to provide the test case. You need to show that > you've thought through all the implications and corner cases on the > resource code. This is a non-trivial change to the how the resource code > works, and you need to demonstrate that your really understand the > implications of what you are doing. On non broken platforms: it will work exactly as it works today. On broken platforms: I have tried with duplicated devices: Both requesting the region via devm_ioremap_resource a0000000-dfffffff : PCI Bus 0000:00 b0000000-cfffffff : PCI Bus 0000:01 b0030000-b003ffff : /axi1/pcie_bridge_1 b0030000-b003ffff : /axi1/pcie_bridge b0030000-b003ffff : /axi1/pcie_bridge b0030000-b003ffff : /axi1/pcie_bridge_1 pcie_bridge_0: pcie_bridge { #address-cells = <1>; #size-cells = <1>; compatible = "xlnx,axi-pcie-1.00.a"; reg = < 0x30030000 0x10000 >; interrupt-parent = <&xps_intc_0>; interrupts = < 0 2 >; qtec,apertures =< 0x80000000 0x20000000 0xa0000000 0x20000000 0xc0000000 0x20000000 0xe0000000 0x20000000 >; }; pcie_bridge_1: pcie_bridge_1 { #address-cells = <1>; #size-cells = <1>; compatible = "xlnx,axi-pcie-1.00.a"; reg = < 0x30030000 0x10000 >; interrupt-parent = <&xps_intc_0>; interrupts = < 31 2 >; qtec,apertures =< 0x80000000 0x20000000 0xa0000000 0x20000000 0xc0000000 0x20000000 0xe0000000 0x20000000 >; }; And for two devices requesting the same region via devm_ioremap_resource() a0000000-dfffffff : PCI Bus 0000:00 b0000000-cfffffff : PCI Bus 0000:01 b0030000-b003ffff : /axi1/pcie_bridge b0030000-b003ffff : /axi1/pcie_bridge b0030000-b003ffff : /axi1/pcie_bridge pcie_bridge_0: pcie_bridge { #address-cells = <1>; #size-cells = <1>; compatible = "xlnx,axi-pcie-1.00.a"; reg = < 0x30030000 0x10000 >; interrupt-parent = <&xps_intc_0>; interrupts = < 0 2 >; qtec,apertures =< 0x80000000 0x20000000 0xa0000000 0x20000000 0xc0000000 0x20000000 0xe0000000 0x20000000 >; }; packer_0: packer_0{ #address-cells = <1>; #size-cells = <1>; compatible = "qtec,axi_matrix_packer-1.00.a"; reg = < 0x30060000 0x10000 >; qtec,pcie_bridge= <&pcie_bridge_0>; }; If you can think of any other corner case, please let me know, and I will try it, now I have a setup for this. > > > Start with the example I pointed out. When a driver does a > request_mem_region(), which resource does it end up being a parent of if > the regions overlap? Can you write a unittest that demonstrates the code > has the correct behaviour? Will a driver end up getting the wrong > device's resource structure as the parent? (hint: yes it will) > > > I can load and unload the device tree perfectly. > > Merely making it work for your use-case isn't the issue. It's whether or > not making this change will break the core behavour of the resource > code. > > g. -- Ricardo Ribalda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/