mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Leonard Crestez <leonard.crestez@nxp.com>
To: "sboyd@codeaurora.org" <sboyd@codeaurora.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: Re: [PATCH] clk: core: Copy connection id
Date: Thu, 2 Mar 2017 12:45:16 +0000	[thread overview]
Message-ID: <1488458715.12287.18.camel@nxp.com> (raw)
In-Reply-To: <20170228080558.GL25384@codeaurora.org>

On Tue, 2017-02-28 at 00:05 -0800, sboyd@codeaurora.org wrote:
> 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 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.

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

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.

  reply	other threads:[~2017-03-02 17:27 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-20 13:20 Leonard Crestez
2017-02-24 20:44 ` Stephen Boyd
2017-02-25  9:20   ` Leonard Crestez
2017-02-28  8:05     ` sboyd
2017-03-02 12:45       ` Leonard Crestez [this message]
2017-03-07 13:53         ` sboyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1488458715.12287.18.camel@nxp.com \
    --to=leonard.crestez@nxp.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome