From: Suman Anna <s-anna@ti.com>
To: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
Kumar Gala <galak@codeaurora.org>,
Tony Lindgren <tony@atomide.com>,
Josh Cartwright <joshc@codeaurora.org>,
Bjorn Andersson <bjorn@kryo.se>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
linux-arm <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
Date: Wed, 12 Nov 2014 13:32:06 -0600 [thread overview]
Message-ID: <5463B5B6.8040709@ti.com> (raw)
In-Reply-To: <CAK=WgbamdqAufjC4Xb5moERRBSB-cO6c1RPTE=f+dp75F+Eh7A@mail.gmail.com>
Hi Ohad,
Thanks for the review.
On 11/12/2014 01:08 PM, Ohad Ben-Cohen wrote:
> Hi Suman,
>
> On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>> +int of_hwspin_lock_get_id(struct device_node *np, int index)
>> +{
>> + struct hwspinlock_device *bank;
>> + struct of_phandle_args args;
>> + int id;
>> + int ret;
>> +
>> + ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
>> + &args);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&hwspinlock_tree_lock);
>> + list_for_each_entry(bank, &hwspinlock_devices, list)
>> + if (bank->dev->of_node == args.np)
>> + break;
>> + mutex_unlock(&hwspinlock_tree_lock);
>> + if (&bank->list == &hwspinlock_devices) {
>> + ret = -EPROBE_DEFER;
>> + goto out;
>> + }
>
> Is this the validation you mentioned which requires the existence of
> "hwspinlock/core: maintain a list of registered hwspinlock banks" ?
Well, not exactly. The validation is on the following segment,
+ id = of_hwspin_lock_simple_xlate(bank, &args);
+ if (id < 0 || id >= bank->num_locks) {
+ ret = -EINVAL;
+ goto out;
+ }
That said, it is also needed to provide the support for deferred probing
without changing the return code conventions on the existing API.
>
> I'm not convinced this is needed for several reasons:
> - the user isn't using the lock yet at this point, and may only need
> the id in order to communicate it to a remote processor
Yes, and wouldn't that require that the id is validated? It just cannot
return any return value, and expect it will be verified somewhere else
or in a following API call. Not doing the validation unnecessarily
complicates the system usage of a lock as you are sharing an invalid
lock to a remote processor and then you have two validation failure
paths on different processors.
> - if the user will try to use the lock prematurely,
> hwspin_lock_request_specific should stop her
> - moreover, hwspin_lock_request_specific must be the one who validates
> the id, since in heterogeneous systems the user may get the id from a
> remote processor and not via of_hwspin_lock_get_id
>
> "hwspinlock/core: maintain a list of registered hwspinlock banks"
> adds complexity which must be very strongly justified.
>
> If we're not sure there is a strong justification for it, we better
> not merge it.
>
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
> ...
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
>
> Do we really must expose these two helpers globally?
>
> Can we instead make these "static inline" methods and embed them in
> hwspinlock_internal.h ?
Actually, not a bad idea, I will move it, thanks. All the client drivers
would need it, and they already have to include the internal header.
regards
Suman
next prev parent reply other threads:[~2014-11-12 19:32 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-12 20:24 [PATCHv6 0/5] hwspinlock core/omap dt support Suman Anna
2014-09-12 20:24 ` [PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock Suman Anna
2014-11-12 15:14 ` Ohad Ben-Cohen
2014-11-12 17:08 ` Suman Anna
2014-12-08 17:21 ` Bjorn Andersson
2014-09-12 20:24 ` [PATCHv6 2/5] Documentation: dt: add the omap hwspinlock bindings document Suman Anna
2014-11-12 15:16 ` Ohad Ben-Cohen
2014-09-12 20:24 ` [PATCHv6 3/5] hwspinlock/core: maintain a list of registered hwspinlock banks Suman Anna
2014-09-12 20:24 ` [PATCHv6 4/5] hwspinlock/core: add common OF helpers Suman Anna
2014-11-12 19:08 ` Ohad Ben-Cohen
2014-11-12 19:32 ` Suman Anna [this message]
2014-11-13 10:03 ` Ohad Ben-Cohen
2014-11-13 17:38 ` Suman Anna
2014-11-13 19:45 ` Ohad Ben-Cohen
2014-11-13 21:02 ` Suman Anna
2014-11-14 7:11 ` Ohad Ben-Cohen
2014-11-14 17:09 ` Suman Anna
2014-11-14 20:05 ` Ohad Ben-Cohen
2014-09-12 20:24 ` [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes Suman Anna
2014-11-12 19:14 ` Ohad Ben-Cohen
2014-11-12 19:50 ` Suman Anna
2014-11-13 9:04 ` Ohad Ben-Cohen
2014-11-20 0:43 ` Bjorn Andersson
2014-11-20 6:36 ` Ohad Ben-Cohen
2014-09-30 16:25 ` [PATCHv6 0/5] hwspinlock core/omap dt support Suman Anna
2014-09-30 20:54 ` Bjorn Andersson
2014-09-30 21:27 ` Suman Anna
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=5463B5B6.8040709@ti.com \
--to=s-anna@ti.com \
--cc=bjorn@kryo.se \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=joshc@codeaurora.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=ohad@wizery.com \
--cc=tony@atomide.com \
/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;
as well as URLs for NNTP newsgroup(s).