From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() Date: Tue, 20 Jan 2015 10:49:22 +0000 Message-ID: <20150120104922.GF5398@red-moon> References: <1420644571-18928-1-git-send-email-lorenzo.pieralisi@arm.com> <1420644571-18928-2-git-send-email-lorenzo.pieralisi@arm.com> <20150119183228.GE10762@e106497-lin.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20150119183228.GE10762@e106497-lin.cambridge.arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Liviu Dudau Cc: "devicetree@vger.kernel.org" , Arnd Bergmann , "linux-pci@vger.kernel.org" , Jingoo Han , Rob Herring , Mohit Kumar , Bjorn Helgaas , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org T24gTW9uLCBKYW4gMTksIDIwMTUgYXQgMDY6MzI6MjlQTSArMDAwMCwgTGl2aXUgRHVkYXUgd3Jv dGU6CgpbLi4uXQoKPiA+IEBAIC0xNDYsNiArMTQ2LDcgQEAgaW50IG9mX3BjaV9nZXRfaG9zdF9i cmlkZ2VfcmVzb3VyY2VzKHN0cnVjdCBkZXZpY2Vfbm9kZSAqZGV2LAo+ID4gIAlzdHJ1Y3Qgb2Zf cGNpX3JhbmdlX3BhcnNlciBwYXJzZXI7Cj4gPiAgCWNoYXIgcmFuZ2VfdHlwZVs0XTsKPiA+ICAJ aW50IGVycjsKPiA+ICsJc3RydWN0IHBjaV9ob3N0X2JyaWRnZV93aW5kb3cgKndpbmRvdzsKPiA+ ICAKPiA+ICAJaWYgKGlvX2Jhc2UpCj4gPiAgCQkqaW9fYmFzZSA9IChyZXNvdXJjZV9zaXplX3Qp T0ZfQkFEX0FERFI7Cj4gPiBAQCAtMjI1LDcgKzIyNiwxMCBAQCBpbnQgb2ZfcGNpX2dldF9ob3N0 X2JyaWRnZV9yZXNvdXJjZXMoc3RydWN0IGRldmljZV9ub2RlICpkZXYsCj4gPiAgY29udmVyc2lv bl9mYWlsZWQ6Cj4gPiAgCWtmcmVlKHJlcyk7Cj4gPiAgcGFyc2VfZmFpbGVkOgo+ID4gKwlsaXN0 X2Zvcl9lYWNoX2VudHJ5KHdpbmRvdywgcmVzb3VyY2VzLCBsaXN0KQo+ID4gKwkJa2ZyZWUod2lu ZG93LT5yZXMpOwo+ID4gIAlwY2lfZnJlZV9yZXNvdXJjZV9saXN0KHJlc291cmNlcyk7Cj4gPiAr CWtmcmVlKGJ1c19yYW5nZSk7Cj4gPiAgCXJldHVybiBlcnI7Cj4gPiAgfQo+ID4gIEVYUE9SVF9T WU1CT0xfR1BMKG9mX3BjaV9nZXRfaG9zdF9icmlkZ2VfcmVzb3VyY2VzKTsKPiAKPiBIaSBMb3Jl bnpvIGV0IGFsbCwKPiAKPiBIZXJlIGlzIG15IHBlcnNvbmFsIHZpZXcgYW5kIEkgYW0gaGFwcHkg dG8gaGVhciBmcm9tIG90aGVycyBvbiB0aGUgZGVzaXJlZAo+IGJlaGF2aW91cjoKPiAKPiBXaGVu IEkgd3JvdGUgdGhpcyBmdW5jdGlvbiB3aGF0IEkgaGFkIGluIG1pbmQgd2FzIHRoYXQgaXQgd2ls bCBwYXJzZSBhcwo+IG11Y2ggYXMgcG9zc2libGUgZnJvbSB0aGUgZGV2aWNlIHRyZWUgYW5kIHJl dHVybiBhIGxpc3Qgb2YgcmVzb3VyY2VzIHRoYXQKPiBjb3VsZCBiZSBzdWNjZXNzZnVsbHkgY29u dmVydGVkLiBJZiB0aGUgZW50aXJlIGxpc3Qgb2YgcmFuZ2VzIGNvdWxkIG5vdAo+IGJlIGNvbnZl cnRlZCB0aGVuIGFuIGVycm9yIGNvZGUgd2lsbCBiZSByZXR1cm5lZCwgYnV0IHRoZSBjYWxsZXIg c3RpbGwKPiBoYWQgdGhlIGxpc3QgYXMgY29uc3RydWN0ZWQgdXAgdG8gdGhlIGVycm9yLiBJdCB3 YXMgdGhlIGpvYiBvZiB0aGUgY2FsbGVyCj4gdG8gZnJlZSB0aGUgbGlzdCBpbiBlaXRoZXIgY2Fz ZXMsIGFzIHN0YXRlZCBpbiB0aGUgY29tbWVudC4KClRoYXQncyB3aGF0IEkgYW0gcXVlc3Rpb25p bmcuIElmIHRoZSBmdW5jdGlvbiB0YWtlcyBhbiBlcnJvciBwYXRoLCB0aGUKd2luZG93cyBsaXN0 IGlzIGZyZWVkLCBzbyB0aGUgcmVzb3VyY2UgcG9pbnRlcnMgYXJlIGdvbmUuIFRoZXJlIGlzIG5v CndheSB0aGUgY2FsbGVyIGNhbiBncmFiIHRob3NlIHJlc291cmNlIHBvaW50ZXJzIGFuZCBmcmVl IHRoZW0uCgpTbyBlaXRoZXIgd2F5LCB0aGUgZnVuY3Rpb24gbmVlZHMgcGF0Y2hpbmcuIEVpdGhl ciB3ZSBkbyBub3QgZnJlZSB0aGUKd2luZG93cyBsaXN0ICh3ZSByZW1vdmUgcGNpX2ZyZWVfcmVz b3VyY2VfbGlzdCkgb3Igd2UgYXBwbHkgbXkgZml4IChvcgp3ZSByZWZhY3RvciB0aGUgQVBJIHdo aWNoIGlzIGxpa2VseSB0byBiZSB3aGF0IEkgd2lsbCBkbykuCgpMb3JlbnpvCgo+IAo+IFRoZSBo aXN0b3JpY2FsIHJlYXNvbiB3aHkgdGhlIGZ1bmN0aW9uIHdhcyB3cml0dGVuIHRoYXQgd2F5IHdh cyBiZWNhdXNlIGF0Cj4gc29tZSBtb21lbnQgYWZ0ZXIgcGFyc2luZyBJJ3ZlIGhhZCBhbiBhZGRp dGlvbmFsIHN0ZXAgd2hlcmUgYXJjaGVzIGNvdWxkCj4gY2xlYW51cCAvIHZldG8gdGhlIGxpc3Qg YW5kIHRoZXkgY291bGQgcmV0dXJuIGFuIGVycm9yIHZhbHVlIHRvIHNpZ25hbAo+IHRoZWlyIGRp c2NvbnRlbnQuIEFsc28gSSB3YXMgKGFtKSBub3Qgc3VyZSBob3cgbGVuaWVudCB3ZSBjb3VsZCBi ZSB3aXRoCj4gdGhlIGRldmljZSB0cmVlIG5vdCBiZWluZyBzYW5lIChhdCBsZWFzdCBvbmUgaG9z dCBicmlkZ2UgYmluZGluZyBsaXN0cyB0aGUKPiBjb25maWcgc3BhY2UgYXMgYSByYW5nZSwgd2hp Y2ggd2FzIGFjY2VwdGVkIGFzIGJyb2tlbikuCj4gCj4gU28sIGZyb20gdGhhdCBwb2ludCBvZiB2 aWV3LCBJIHdvdWxkIE5BSyB0aGlzIHBhdGNoLCBhcyB0aGUgZnVuY3Rpb24gd29ya3MKPiBhcyBp bnRlbmRlZC4gSWYgb3RoZXJzIGZpbmQgdGhpcyBtb2RlIG9mIG9wZXJhdGlvbiB0b28gY29udm9s dXRlZCwgdGhlbgo+IHRoZSBwYXRjaCBzaG91bGQgcHJvYmFibHkgbWFrZSBjbGVhciB0aGF0IGNs ZWFudXAgb25seSBuZWVkcyB0byBiZSBkb25lIG9uCj4gZnVuY3Rpb24gcmV0dXJuaW5nIHN1Y2Nl c3MuCj4gCj4gQmVzdCByZWdhcmRzLAo+IAo+ID4gLS0gCj4gPiAyLjIuMQo+ID4gCj4gPiAKPiAK PiAtLSAKPiA9PT09PT09PT09PT09PT09PT09PQo+IHwgSSB3b3VsZCBsaWtlIHRvIHwKPiB8IGZp eCB0aGUgd29ybGQsICB8Cj4gfCBidXQgdGhleSdyZSBub3QgfAo+IHwgZ2l2aW5nIG1lIHRoZSAg IHwKPiAgXCBzb3VyY2UgY29kZSEgIC8KPiAgIC0tLS0tLS0tLS0tLS0tLQo+ICAgICDCr1xfKOOD hClfL8KvCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwps aW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJh ZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51 eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:38856 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754056AbbATKta (ORCPT ); Tue, 20 Jan 2015 05:49:30 -0500 Date: Tue, 20 Jan 2015 10:49:22 +0000 From: Lorenzo Pieralisi To: Liviu Dudau Cc: "linux-pci@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" , Arnd Bergmann , Mohit Kumar , Jingoo Han , Bjorn Helgaas , Rob Herring Subject: Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() Message-ID: <20150120104922.GF5398@red-moon> References: <1420644571-18928-1-git-send-email-lorenzo.pieralisi@arm.com> <1420644571-18928-2-git-send-email-lorenzo.pieralisi@arm.com> <20150119183228.GE10762@e106497-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <20150119183228.GE10762@e106497-lin.cambridge.arm.com> Sender: linux-pci-owner@vger.kernel.org List-ID: On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Tue, 20 Jan 2015 10:49:22 +0000 Subject: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() In-Reply-To: <20150119183228.GE10762@e106497-lin.cambridge.arm.com> References: <1420644571-18928-1-git-send-email-lorenzo.pieralisi@arm.com> <1420644571-18928-2-git-send-email-lorenzo.pieralisi@arm.com> <20150119183228.GE10762@e106497-lin.cambridge.arm.com> Message-ID: <20150120104922.GF5398@red-moon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ?\_(?)_/?