All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: rafael@kernel.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, lukasz.luba@arm.com
Subject: Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system
Date: Sun, 28 Mar 2021 08:50:47 +0200	[thread overview]
Message-ID: <YGAnRx8SiZHFPpY6@kroah.com> (raw)
In-Reply-To: <433ec4ac-a7a9-ecf9-f1c1-f658d279a2df@linaro.org>

On Sat, Mar 27, 2021 at 08:41:24PM +0100, Daniel Lezcano wrote:
> On 27/03/2021 13:50, Greg KH wrote:
> > On Fri, Mar 12, 2021 at 02:04:08PM +0100, Daniel Lezcano wrote:
> >> A SoC can be differently structured depending on the platform and the
> >> kernel can not be aware of all the combinations, as well as the
> >> specific tweaks for a particular board.
> >>
> >> The creation of the hierarchy must be delegated to userspace.
> >>
> >> These changes provide a registering mechanism where the different
> >> subsystems will initialize their dtpm backends and register with a
> >> name the dtpm node in a list.
> >>
> >> The next changes will provide an userspace interface to create
> >> hierarchically the different nodes. Those will be created by name and
> >> found via the list filled by the different subsystem.
> >>
> >> If a specified name is not found in the list, it is assumed to be a
> >> virtual node which will have children and the default is to allocate
> >> such node.
> >>
> >> When the node register in the list, the function will be dtpm_register
> >> where the previous semantic was to create the node. Thus, the
> >> functions are renamed to reflect their purpose.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> >> ---
> 
> [ ... ]
> 
> >> +static void dtpm_release(struct kref *kref)
> >> +{
> >> +	struct dtpm *dtpm = container_of(kref, struct dtpm, kref);
> >> +
> >> +	kfree(dtpm);
> >> +}
> >> +
> >> +/**
> >> + * dtpm_put - Release a reference on a dtpm device
> >> + * @dtpm: a pointer to a dtpm structure
> >> + *
> >> + * Release the reference on the specified dtpm device. The last
> >> + * reference leads to a memory release.
> >> + */
> >> +void dtpm_put(struct dtpm *dtpm)
> >> +{
> >> +	kref_put(&dtpm->kref, dtpm_release);
> > 
> > You forgot to also grab the dtpm_lock before calling this, right?  What
> > is preventing a get and put from being called at the same time?
> > 
> > You protect things at get time, but not put from what I can see :(
> 
> Thanks for spotting this, I will send a fix for that.
> 
> [ ... ]
> 
> >> +	list_add(&node->node, &dtpm_list);
> >> +
> >> +	pr_info("Registered %s\n", name);
> > 
> > When kernel code works properly, it is quiet.  This is debugging code a
> > the most, never something that everyone should be seeing all the time,
> > please remove.
> 
> Initially, a comment asked for debug traces in the code. There are more
> traces in the code at the pr_debug level.
> 
> Is your suggestion to remove the pr_info as well as other debug traces
> or convert those pr_info to pr_debug ?

pr_info() should not be used for "debug traces".

Use real tracepoints for debug traces if you still need them, hopefully
the code is all now debugged properly, right?

Again, when code is working, it is quiet.  Do not leave debugging stuff
like this in a working system.

> 
> [ ... ]
> 
> > And any reason why you are not using "real" struct devices in this
> > subsystem?  You seem to be rolling your own infrastructure for no good
> > reason.  I imagine you want sysfs support next, right?
> 
> Actually, the framework is on top of powercap, so it has de facto the
> sysfs support. On the other side, the dtpm backends are tied with the
> device they manage.

So why are they not a "real" device in the driver model?  It looks like
you almost are wanting all of that functionality and are having to
implement it "by hand" instead.

thanks,

greg k-h

  reply	other threads:[~2021-03-28  6:51 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-12 13:04 [PATCH v4 1/5] powercap/drivers/dtpm: Encapsulate even more the code Daniel Lezcano
2021-03-12 13:04 ` [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system Daniel Lezcano
2021-03-27 12:50   ` Greg KH
2021-03-27 19:41     ` Daniel Lezcano
2021-03-28  6:50       ` Greg KH [this message]
2021-03-28 11:11         ` Daniel Lezcano
2021-03-28 11:24           ` Greg KH
2021-03-28 16:07             ` Daniel Lezcano
2021-03-28 17:26               ` Greg KH
2021-03-28 18:01                 ` Daniel Lezcano
2021-03-12 13:04 ` [PATCH v4 3/5] powercap/drivers/dtpm: Simplify the dtpm table Daniel Lezcano
2021-03-12 13:04 ` [PATCH v4 4/5] powercap/drivers/dtpm: Use container_of instead of a private data field Daniel Lezcano
2021-03-12 13:04 ` [PATCH v4 5/5] powercap/drivers/dtpm: Scale the power with the load Daniel Lezcano

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=YGAnRx8SiZHFPpY6@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=rafael@kernel.org \
    --subject='Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.