From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name Date: Fri, 27 Jul 2018 17:29:52 +0200 Message-ID: References: <20180716144206.30985-1-miquel.raynal@bootlin.com> <20180716144206.30985-2-miquel.raynal@bootlin.com> <84f97022-a470-f314-ee75-e5afb733bea5@linaro.org> <20180727135230.3c7102c7@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180727135230.3c7102c7@xps13> Content-Language: en-US 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: Miquel Raynal Cc: Mark Rutland , Andrew Lunn , Jason Cooper , Nadav Haklai , devicetree@vger.kernel.org, Antoine Tenart , Catalin Marinas , Gregory Clement , linux-pm@vger.kernel.org, Will Deacon , Maxime Chevallier , Eduardo Valentin , David Sniatkiwicz , Rob Herring , Thomas Petazzoni , Zhang Rui , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: devicetree@vger.kernel.org T24gMjcvMDcvMjAxOCAxMzo1MiwgTWlxdWVsIFJheW5hbCB3cm90ZToKPiBIaSBEYW5pZWwsCj4g Cj4gRGFuaWVsIExlemNhbm8gPGRhbmllbC5sZXpjYW5vQGxpbmFyby5vcmc+IHdyb3RlIG9uIEZy aSwgMjcgSnVsIDIwMTgKPiAxMzozNDoxOSArMDIwMDoKPiAKPj4gT24gMTYvMDcvMjAxOCAxNjo0 MSwgTWlxdWVsIFJheW5hbCB3cm90ZToKPj4+IFRoZXJtYWwgem9uZSBuYW1lcyBtdXN0IGZvbGxv dyBjZXJ0YWluIHJ1bGVzIGltcG9zZWQgYnkgdGhlIGZyYW1ld29yay4KPj4+IFRoZXkgYXJlIGxp bWl0ZWQgaW4gbGVuZ3RoIGFuZCBzaGFsbCBub3QgaGF2ZSBhbnkgaHlwaGVuICctJy4KPj4+Cj4+ PiBUaGlzIGlzIGRvbmUgaW4gYSBzZXBhcmF0ZSBmdW5jdGlvbiBmb3IgZnV0dXJlIHVzZSBpbiBh bm90aGVyIGxvY2F0aW9uLgo+Pj4KPj4+IFNpZ25lZC1vZmYtYnk6IE1pcXVlbCBSYXluYWwgPG1p cXVlbC5yYXluYWxAYm9vdGxpbi5jb20+ICAKPj4KPj4gV2h5IGRvIHlvdSBoYXZlIHRvIHByb3Zp ZGUgYSBmdW5jdGlvbiB0byB0ZXN0IHRoYXQ/Cj4+Cj4+IExvZ2ljYWxseSwgdGhlIG9uZSB3aG8g ZGlkIHRoZSBjaGFuZ2UgdG8gYWRkIGEgdGhlcm1hbCBuYW1lLCBzaG91bGQKPj4gY2hlY2sgaXRz IGNvZGUgd29ya3MuIFdpdGhvdXQgYSBwcm9wZXIgbmFtZSB0aGF0IHdvbid0IHdvcmsuCj4gCj4g V2hhdCBkbyB5b3UgbWVhbiAidGhlIG9uZSB3aG8gZGlkIHRoZSBjaGFuZ2UiPwo+IEkgdGhpbmsg dGhlIHRoZXJtYWwgY29yZSBzaG91bGQgbm90IGNhcmUgdGhhdCBtdWNoIHRvIHdoYXQgaXMgZ2l2 ZW4gYXMKPiBuYW1lIGFuZCBzaG91bGQgcHJvYmFibHkgbm90IGJlIHNvIHN0cmljdC4KPiAKPiBB bHNvLCBJIGRvbid0IGNob29zZSB3aGF0IGRldl9uYW1lKCkgcmV0dXJucywgaXQncyBpbiB0aGUg ZGV2aWNlIHRyZWUKPiBhbmQgdGhlIGRldmljZSB0cmVlIGRvIG5vdCBjYXJlIGFib3V0IHRoZSBp bXBsZW1lbnRhdGlvbiwgaXQncyBqdXN0IGEKPiBkZXNjcmlwdGl2ZSBmaWxlLgo+IAo+Pgo+PiBT byB0aGlzIGZ1bmN0aW9uIGlzIHRlc3Rpbmcgc29tZXRoaW5nIHdoaWNoIHNob3VsZCBiZSBhbHJl YWR5IHRlc3RlZCwgbm8/Cj4gCj4gSSBkb24ndCB0aGluayBpdCBpcy4gV2l0aG91dCB0aGlzIGZ1 bmN0aW9uIHRoZSBwcm9iZSB3aWxsIHNpbXBseSBmYWlsLgo+IAo+IFRoZSBleHBsYW5hdGlvbiBv ZiB3aGF0IGZhaWxzIGlzIGluIHRoZSBjb2RlOgo+IAo+Pj4gKwkJLyoKPj4+ICsJCSAqIFdoZW4g aW5zaWRlIGEgc3lzdGVtIGNvbnRyb2xsZXIsIHRoZSBkZXZpY2UgbmFtZSBoYXMgdGhlCj4+PiAr CQkgKiBmb3JtOiBmMDZmODAwMC5zeXN0ZW0tY29udHJvbGxlcjphcC10aGVybWFsIHNvIHN0cmlw cGluZwo+Pj4gKwkJICogYWZ0ZXIgdGhlICc6JyBzaG91bGQgZ2l2ZSB1cyBhIHNob3J0ZXIgYnV0 IG1lYW5pbmdmdWwgbmFtZS4KPj4+ICsJCSAqLwo+Pj4gKwkJbmFtZSA9IHN0cnJjaHIobmFtZSwg JzonKTsKPj4+ICsJCWlmICghbmFtZSkKPj4+ICsJCQluYW1lID0gImFybWFkYV90aGVybWFsIjsK Pj4+ICsJCWVsc2UKPj4+ICsJCQluYW1lKys7Cj4gCj4gWy4uLl0KCkknbSBub3QgaW4gZmF2b3Ig b2YgdGhpcywgcG90ZW50aWFsbHkgaXQgY2FuIGludHJvZHVjZSBhIGJyZWFrIHdoaWNoIGNhbgpi ZSBleHBsb2l0ZWQgbGF0ZXIuCgpZb3UgYXJlIGNyZWF0aW5nIHRoZSB0aGVybWFsIHpvbmVzIG1h bnVhbGx5IGZyb20gdGhlIGRyaXZlciBidXQgd2FudCB0bwpyZWx5IG9uIHRoZSB0ZW1wZXJhdHVy ZSBjb250cm9sbGVyIG5hbWUgdG8gZ2l2ZSBhIG5hbWUgdG8gdGhlIHRoZXJtYWwgem9uZS4KCldo eSBub3QgY3JlYXRlIHRoZSB0aGVybWFsIHpvbmUgaW4gdGhlIERUIHdpdGggdGhlIGNvcnJlY3Qg bmFtZSBhbmQgdXNlCmRldm1fdGhlcm1hbF96b25lX29mX3NlbnNvcl9yZWdpc3RlciA/CgpPciBh bHRlcm5hdGl2ZWx5IGhhcmRjb2RlIHRoZSB0aGVybWFsIHpvbmUgbmFtZSA/CgoKCgoKCgotLSAK IDxodHRwOi8vd3d3LmxpbmFyby5vcmcvPiBMaW5hcm8ub3JnIOKUgiBPcGVuIHNvdXJjZSBzb2Z0 d2FyZSBmb3IgQVJNIFNvQ3MKCkZvbGxvdyBMaW5hcm86ICA8aHR0cDovL3d3dy5mYWNlYm9vay5j b20vcGFnZXMvTGluYXJvPiBGYWNlYm9vayB8CjxodHRwOi8vdHdpdHRlci5jb20vIyEvbGluYXJv b3JnPiBUd2l0dGVyIHwKPGh0dHA6Ly93d3cubGluYXJvLm9yZy9saW5hcm8tYmxvZy8+IEJsb2cK CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1h cm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5v cmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0t a2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Fri, 27 Jul 2018 17:29:52 +0200 Subject: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name In-Reply-To: <20180727135230.3c7102c7@xps13> References: <20180716144206.30985-1-miquel.raynal@bootlin.com> <20180716144206.30985-2-miquel.raynal@bootlin.com> <84f97022-a470-f314-ee75-e5afb733bea5@linaro.org> <20180727135230.3c7102c7@xps13> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 27/07/2018 13:52, Miquel Raynal wrote: > Hi Daniel, > > Daniel Lezcano wrote on Fri, 27 Jul 2018 > 13:34:19 +0200: > >> On 16/07/2018 16:41, Miquel Raynal wrote: >>> Thermal zone names must follow certain rules imposed by the framework. >>> They are limited in length and shall not have any hyphen '-'. >>> >>> This is done in a separate function for future use in another location. >>> >>> Signed-off-by: Miquel Raynal >> >> Why do you have to provide a function to test that? >> >> Logically, the one who did the change to add a thermal name, should >> check its code works. Without a proper name that won't work. > > What do you mean "the one who did the change"? > I think the thermal core should not care that much to what is given as > name and should probably not be so strict. > > Also, I don't choose what dev_name() returns, it's in the device tree > and the device tree do not care about the implementation, it's just a > descriptive file. > >> >> So this function is testing something which should be already tested, no? > > I don't think it is. Without this function the probe will simply fail. > > The explanation of what fails is in the code: > >>> + /* >>> + * When inside a system controller, the device name has the >>> + * form: f06f8000.system-controller:ap-thermal so stripping >>> + * after the ':' should give us a shorter but meaningful name. >>> + */ >>> + name = strrchr(name, ':'); >>> + if (!name) >>> + name = "armada_thermal"; >>> + else >>> + name++; > > [...] I'm not in favor of this, potentially it can introduce a break which can be exploited later. You are creating the thermal zones manually from the driver but want to rely on the temperature controller name to give a name to the thermal zone. Why not create the thermal zone in the DT with the correct name and use devm_thermal_zone_of_sensor_register ? Or alternatively hardcode the thermal zone name ? -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog