[PATCH] clk: core: Copy connection id

[PATCH] clk: core: Copy connection id

[Leonard Crestez]

Some drivers use sprintf to build clk connection id names but the clk
core will save those strings and occasionally print them back. Duplicate
the con_id strings instead of fixing all the users.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Some examples of using sprintf for con_id include:
drivers/mfd/omap-usb-host.c
drivers/tty/serial/samsung.c
sound/soc/fsl/fsl_asrc.c

There are lots more. They are difficult to find and "fixing" them on the
consumer side requires nasty code to keep track of the allocated clkname.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0fb39fe..67201f6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2502,7 +2502,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 
 	clk->core = hw->core;
 	clk->dev_id = dev_id;
-	clk->con_id = con_id;
+	clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
 	clk->max_rate = ULONG_MAX;
 
 	clk_prepare_lock();
@@ -2518,6 +2518,7 @@ void __clk_free_clk(struct clk *clk)
 	hlist_del(&clk->clks_node);
 	clk_prepare_unlock();
 
+	kfree_const(clk->con_id);
 	kfree(clk);
 }
 
-- 
2.7.4

Re: [PATCH] clk: core: Copy connection id

[Stephen Boyd]

On 02/20, Leonard Crestez wrote:
> Some drivers use sprintf to build clk connection id names but the clk
> core will save those strings and occasionally print them back. Duplicate
> the con_id strings instead of fixing all the users.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/clk/clk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Some examples of using sprintf for con_id include:
> drivers/mfd/omap-usb-host.c
> drivers/tty/serial/samsung.c
> sound/soc/fsl/fsl_asrc.c
> 
> There are lots more. They are difficult to find and "fixing" them on the
> consumer side requires nasty code to keep track of the allocated clkname.

Good catch. What about dev_id though? That could also have the
same problem if some device is removed and we're still holding a
reference to the kobject's name. This is probably more rare than
what is happening here, but still seems possible that we might
trip over that later.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

T24gRnJpLCAyMDE3LTAyLTI0IGF0IDEyOjQ0IC0wODAwLCBTdGVwaGVuIEJveWQgd3JvdGU6DQo+
IE9uIDAyLzIwLCBMZW9uYXJkIENyZXN0ZXogd3JvdGU6DQo+ID4gU29tZSBkcml2ZXJzIHVzZSBz
cHJpbnRmIHRvIGJ1aWxkIGNsayBjb25uZWN0aW9uIGlkIG5hbWVzIGJ1dCB0aGUNCj4gPiBjbGsN
Cj4gPiBjb3JlIHdpbGwgc2F2ZSB0aG9zZSBzdHJpbmdzIGFuZCBvY2Nhc2lvbmFsbHkgcHJpbnQg
dGhlbSBiYWNrLg0KPiA+IER1cGxpY2F0ZQ0KPiA+IHRoZSBjb25faWQgc3RyaW5ncyBpbnN0ZWFk
IG9mIGZpeGluZyBhbGwgdGhlIHVzZXJzLg0KPiANCj4gR29vZCBjYXRjaC4gV2hhdCBhYm91dCBk
ZXZfaWQgdGhvdWdoPyBUaGF0IGNvdWxkIGFsc28gaGF2ZSB0aGUNCj4gc2FtZSBwcm9ibGVtIGlm
IHNvbWUgZGV2aWNlIGlzIHJlbW92ZWQgYW5kIHdlJ3JlIHN0aWxsIGhvbGRpbmcgYQ0KPiByZWZl
cmVuY2UgdG8gdGhlIGtvYmplY3QncyBuYW1lLiBUaGlzIGlzIHByb2JhYmx5IG1vcmUgcmFyZSB0
aGFuDQo+IHdoYXQgaXMgaGFwcGVuaW5nIGhlcmUsIGJ1dCBzdGlsbCBzZWVtcyBwb3NzaWJsZSB0
aGF0IHdlIG1pZ2h0DQo+IHRyaXAgb3ZlciB0aGF0IGxhdGVyLg0KDQpBIGRldmljZSBzaG91bGQg
bm9ybWFsbHkgZnJlZSB0aGUgY2xrcyBpdCB1c2VzIGJlZm9yZSBpdCBpcyBkZXN0cm95ZWQuDQpU
aGlzIG1lYW5zIHRoYXQgaWYgZGV2X2lkIGlzIHBvaW50aW5nIHRvIGZyZWVkIG1lbW9yeSB0aGVu
IHRoZSBjbGsNCml0c2VsZiB3YXMgcHJvYmFibHkgbGVha2VkLCByaWdodD8NCg0KVGhpcyBpcyBv
YnZpb3VzIG1pc3VzZSBvZiB0aGUgQVBJLCBub3QgbGlrZSBzcHJpbnRmLWluZyBhIGNvbl9pZCBp
biBhDQpjb21wbGV4IGRyaXZlci4gSSBkb24ndCByZWFsbHkgdGhpbmsgaXQncyB3b3J0aCBjb3B5
aW5nIHN0cmluZ3MgZm9yIGl0Lg0KDQotLQ0KUmVnYXJkcywNCkxlb25hcmQ=

Re: [PATCH] clk: core: Copy connection id

[sboyd]

On 02/25, Leonard Crestez wrote:
> On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> > On 02/20, Leonard Crestez wrote:
> > > Some drivers use sprintf to build clk connection id names but the
> > > clk
> > > core will save those strings and occasionally print them back.
> > > Duplicate
> > > the con_id strings instead of fixing all the users.
> > 
> > Good catch. What about dev_id though? That could also have the
> > same problem if some device is removed and we're still holding a
> > reference to the kobject's name. This is probably more rare than
> > what is happening here, but still seems possible that we might
> > trip over that later.
> 
> A device should normally free the clks it uses before it is destroyed.
> This means that if dev_id is pointing to freed memory then the clk
> itself was probably leaked, right?

Sure. clk_get_sys() could be called and then we could have
something sprintf the dev_id there. A quick grep doesn't show any
place where that happens though so it seems safe right now.

That said, it would be nice to clearly document that we don't
expect dev_id to be freed or changed during the lifetime of the
clk structure, but we do allow con_id to change. Perhaps the copy
shows that, but a comment would also be useful so we don't have
people wondering why dev_id isn't copied as well.

> 
> This is obvious misuse of the API, not like sprintf-ing a con_id in a
> complex driver. I don't really think it's worth copying strings for it.
> 

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

T24gVHVlLCAyMDE3LTAyLTI4IGF0IDAwOjA1IC0wODAwLCBzYm95ZEBjb2RlYXVyb3JhLm9yZyB3
cm90ZToNCj4gT24gMDIvMjUsIExlb25hcmQgQ3Jlc3RleiB3cm90ZToNCj4gPiANCj4gPiBPbiBG
cmksIDIwMTctMDItMjQgYXQgMTI6NDQgLTA4MDAsIFN0ZXBoZW4gQm95ZCB3cm90ZToNCj4gPiA+
IA0KPiA+ID4gT24gMDIvMjAsIExlb25hcmQgQ3Jlc3RleiB3cm90ZToNCj4gPiA+ID4gDQo+ID4g
PiA+IFNvbWUgZHJpdmVycyB1c2Ugc3ByaW50ZiB0byBidWlsZCBjbGsgY29ubmVjdGlvbiBpZCBu
YW1lcyBidXQNCj4gPiA+ID4gdGhlDQo+ID4gPiA+IGNsaw0KPiA+ID4gPiBjb3JlIHdpbGwgc2F2
ZSB0aG9zZSBzdHJpbmdzIGFuZCBvY2Nhc2lvbmFsbHkgcHJpbnQgdGhlbSBiYWNrLg0KPiA+ID4g
PiBEdXBsaWNhdGUNCj4gPiA+ID4gdGhlIGNvbl9pZCBzdHJpbmdzIGluc3RlYWQgb2YgZml4aW5n
IGFsbCB0aGUgdXNlcnMuDQo+ID4gPiBHb29kIGNhdGNoLiBXaGF0IGFib3V0IGRldl9pZCB0aG91
Z2g/IFRoYXQgY291bGQgYWxzbyBoYXZlIHRoZQ0KPiA+ID4gc2FtZSBwcm9ibGVtIGlmIHNvbWUg
ZGV2aWNlIGlzIHJlbW92ZWQgYW5kIHdlJ3JlIHN0aWxsIGhvbGRpbmcgYQ0KPiA+ID4gcmVmZXJl
bmNlIHRvIHRoZSBrb2JqZWN0J3MgbmFtZS4gVGhpcyBpcyBwcm9iYWJseSBtb3JlIHJhcmUgdGhh
bg0KPiA+ID4gd2hhdCBpcyBoYXBwZW5pbmcgaGVyZSwgYnV0IHN0aWxsIHNlZW1zIHBvc3NpYmxl
IHRoYXQgd2UgbWlnaHQNCj4gPiA+IHRyaXAgb3ZlciB0aGF0IGxhdGVyLg0KPiA+IEEgZGV2aWNl
IHNob3VsZCBub3JtYWxseSBmcmVlIHRoZSBjbGtzIGl0IHVzZXMgYmVmb3JlIGl0IGlzDQo+ID4g
ZGVzdHJveWVkLg0KPiA+IFRoaXMgbWVhbnMgdGhhdCBpZiBkZXZfaWQgaXMgcG9pbnRpbmcgdG8g
ZnJlZWQgbWVtb3J5IHRoZW4gdGhlIGNsaw0KPiA+IGl0c2VsZiB3YXMgcHJvYmFibHkgbGVha2Vk
LCByaWdodD8NCj4gU3VyZS4gY2xrX2dldF9zeXMoKSBjb3VsZCBiZSBjYWxsZWQgYW5kIHRoZW4g
d2UgY291bGQgaGF2ZQ0KPiBzb21ldGhpbmcgc3ByaW50ZiB0aGUgZGV2X2lkIHRoZXJlLiBBIHF1
aWNrIGdyZXAgZG9lc24ndCBzaG93IGFueQ0KPiBwbGFjZSB3aGVyZSB0aGF0IGhhcHBlbnMgdGhv
dWdoIHNvIGl0IHNlZW1zIHNhZmUgcmlnaHQgbm93Lg0KPiANCj4gVGhhdCBzYWlkLCBpdCB3b3Vs
ZCBiZSBuaWNlIHRvIGNsZWFybHkgZG9jdW1lbnQgdGhhdCB3ZSBkb24ndA0KPiBleHBlY3QgZGV2
X2lkIHRvIGJlIGZyZWVkIG9yIGNoYW5nZWQgZHVyaW5nIHRoZSBsaWZldGltZSBvZiB0aGUNCj4g
Y2xrIHN0cnVjdHVyZSwgYnV0IHdlIGRvIGFsbG93IGNvbl9pZCB0byBjaGFuZ2UuIFBlcmhhcHMg
dGhlIGNvcHkNCj4gc2hvd3MgdGhhdCwgYnV0IGEgY29tbWVudCB3b3VsZCBhbHNvIGJlIHVzZWZ1
bCBzbyB3ZSBkb24ndCBoYXZlDQo+IHBlb3BsZSB3b25kZXJpbmcgd2h5IGRldl9pZCBpc24ndCBj
b3BpZWQgYXMgd2VsbC4NCg0KVGhpcyBzaG91bGQgYmUgbWVudGlvbmVkIG9uIHRoZSBwdWJsaWMg
ZG9jdW1lbnRhdGlvbiBmb3IgY2xrX2dldF9zeXMsDQpjbGtfZ2V0IGFuZCBkZXZtX2Nsa19nZXQs
IHJpZ2h0PyBUaGVzZSBzZWVtIHRvIGJlIHRoZSBwdWJsaWMgZW50cnkNCnBvaW50cyB0byB0aGUg
Y2xrIHN1YnN5c3RlbSBhbmQgdXNlcnMgYXJlIGV4cGVjdGVkIHRvIHJlYWQgdGhlaXIgZG9jcy4N
Cg0KRG8geW91IHdhbnQgbWUgdG8gcmVzZW5kIHRoZSBwYXRjaCB3aXRoIHRoZXNlIG5vdGVzPw0K
DQpJJ20gbm90IGNvbWZvcnRhYmxlIGFkZGluZyB0byBkb2N1bWVudGF0aW9uIHdoZW4gSSBkb24n
dCBmdWxseQ0KdW5kZXJzdGFuZCB0aGUgc3lzdGVtIG15c2VsZi4gSSBvbmx5IGRpc2NvdmVyZWQg
dGhpcyB3aGlsZSBsb29raW5nIGludG8NCnVucmVsYXRlZCBkcml2ZXIgaXNzdWVzLg==

Re: [PATCH] clk: core: Copy connection id

[sboyd]

On 03/02, Leonard Crestez wrote:
> On Tue, 2017-02-28 at 00:05 -0800, sboyd@codeaurora.org wrote:
> > Sure. clk_get_sys() could be called and then we could have
> > something sprintf the dev_id there. A quick grep doesn't show any
> > place where that happens though so it seems safe right now.
> > 
> > That said, it would be nice to clearly document that we don't
> > expect dev_id to be freed or changed during the lifetime of the
> > clk structure, but we do allow con_id to change. Perhaps the copy
> > shows that, but a comment would also be useful so we don't have
> > people wondering why dev_id isn't copied as well.
> 
> This should be mentioned on the public documentation for clk_get_sys,
> clk_get and devm_clk_get, right? These seem to be the public entry
> points to the clk subsystem and users are expected to read their docs.

Sure. Except those are not just implemented for the common clk
framework, so the wording will need to be generic. Also we have
some more entry points like of_clk_get() too that would need a
note.

> 
> Do you want me to resend the patch with these notes?

No. I've applied this current patch to clk-fixes.

> 
> I'm not comfortable adding to documentation when I don't fully
> understand the system myself. I only discovered this while looking into
> unrelated driver issues.

Ok. I can take care of sending the patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project