Hi Suman,
On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <[email protected]> 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" ?
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
- 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 ?
Thanks,
Ohad.
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 <[email protected]> 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
Hi Suman,
On Wed, Nov 12, 2014 at 9:32 PM, Suman Anna <[email protected]> wrote:
>> 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;
> + }
I'm not entirely convinced that this justifies adding the proposed
linked list. Can't we can get the base_id and number of locks by
traversing the DT?
> That said, it is also needed to provide the support for deferred probing
> without changing the return code conventions on the existing API.
Here too, adding a data structure maintaining memory objects and
taking care of it life cycle seems a bit overkill for anything to do
with return values.
> 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.
Validation is great but I'm still not convinced it can't be done
without maintaining another data structure.
Please show me specific technical issues that can't be resolved
without adding the proposed linked list.
Thanks,
Ohad.
Hi Ohad,
On 11/13/2014 04:03 AM, Ohad Ben-Cohen wrote:
> Hi Suman,
>
> On Wed, Nov 12, 2014 at 9:32 PM, Suman Anna <[email protected]> wrote:
>>> 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;
>> + }
>
> I'm not entirely convinced that this justifies adding the proposed
> linked list. Can't we can get the base_id and number of locks by
> traversing the DT?
No, not always, because, either of them can be optional between
different platform implementations. For example, on OMAP, the number of
locks is read from an IP register, and not coded in DT. Similarly,
base_id can be optional on SoCs that don't have multiple IP instances.
The only place the hwspinlock core knows both of them for sure is at the
device registration time, but the core only stores the locks and not the
devices at the moment. Any operation on the device is not possible
without knowing the exact global lock we are dealing with, and this API
is about returning that exact global lock id.
>
>> That said, it is also needed to provide the support for deferred probing
>> without changing the return code conventions on the existing API.
>
> Here too, adding a data structure maintaining memory objects and
> taking care of it life cycle seems a bit overkill for anything to do
> with return values.
IMHO, this life cycle management is not that complicated, it is managed
alongside the addition/removal of the locks during the device
registration/unregistration time.
>
>> 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.
>
> Validation is great but I'm still not convinced it can't be done
> without maintaining another data structure.
>
> Please show me specific technical issues that can't be resolved
> without adding the proposed linked list.
Same as above.
regards
Suman
Hi Suman,
On Thu, Nov 13, 2014 at 7:38 PM, Suman Anna <[email protected]> wrote:
> No, not always, because, either of them can be optional between
> different platform implementations. For example, on OMAP, the number of
> locks is read from an IP register, and not coded in DT. Similarly,
> base_id can be optional on SoCs that don't have multiple IP instances.
It can, but base_id isn't optional today --- it must be provided via
platform_data --- and I'm not sure we should implicitly change this
semantics while moving to DT. We never guessed the base_id before,
even if there was only a single IP instance, and I'm not convinced we
should start doing it now.
About the number of locks - why do we even need it at this point? the
global lock id should just be base_id + the lock index.
> IMHO, this life cycle management is not that complicated
It's always very easy to add code, really. It is never complicated.
But then gradually the code becomes harder to maintain, debug, and
change.
On the contrary, achieving the same functionality with less code is
always harder. But when we get there, the results are pretty
satisfying.
In this case I still feel the extra linked list is redundant.
Please try it: ditch the "hwspinlock/core: maintain a list of
registered hwspinlock banks" patch, and replace of_hwspin_lock_get_id
with a slim method that just returns the base_id + the lock index. Let
me know if it works for you.
Thanks,
Ohad.
Hi Ohad,
On 11/13/2014 01:45 PM, Ohad Ben-Cohen wrote:
> Hi Suman,
>
> On Thu, Nov 13, 2014 at 7:38 PM, Suman Anna <[email protected]> wrote:
>> No, not always, because, either of them can be optional between
>> different platform implementations. For example, on OMAP, the number of
>> locks is read from an IP register, and not coded in DT. Similarly,
>> base_id can be optional on SoCs that don't have multiple IP instances.
>
> It can, but base_id isn't optional today --- it must be provided via
> platform_data ---
That was the case before DT, right. As it is, the hwspinlock core has no
knowledge of platform_data, it was used by the individual implementation
drivers to supply the base_id in the registration API.
The platform_data will/should become obsolete with DT devices.
and I'm not sure we should implicitly change this
> semantics while moving to DT. We never guessed the base_id before,
> even if there was only a single IP instance, and I'm not convinced we
> should start doing it now.
OK, if we make hwlock-base-id mandatory in DT, we will get past 1
problem (that of looking up base-id for a specific device).
>
> About the number of locks - why do we even need it at this point? the
> global lock id should just be base_id + the lock index.
The number of locks would still be needed for validation purposes.
OK, lets take an example. I have say 2 device instances, say
hwlock1: hwlock@0 {
hwlock-num-locks = <32>
hwlock-base-id = <0>;
#hwlock-cells = <1>;
};
hwlock2: hwlock@0 {
hwlock-num-locks = <32>
hwlock-base-id = <32>;
#hwlock-cells = <1>;
};
some-client {
hwlocks = <&hwlock1 32>, <&hwlock2 0>;
};
The first args value is incorrect, and I expect the API to return an
error. I don't think the API can make assumptions that all passed in
values will be correct. What do you suggest that the API do here?
The above locks are equivalent if we forgo checking.
>
>> IMHO, this life cycle management is not that complicated
>
> It's always very easy to add code, really. It is never complicated.
> But then gradually the code becomes harder to maintain, debug, and
> change.
>
> On the contrary, achieving the same functionality with less code is
> always harder. But when we get there, the results are pretty
> satisfying.
>
> In this case I still feel the extra linked list is redundant.
>
> Please try it: ditch the "hwspinlock/core: maintain a list of
> registered hwspinlock banks" patch, and replace of_hwspin_lock_get_id
> with a slim method that just returns the base_id + the lock index. Let
> me know if it works for you.
Yeah, it will work (provided base-id is mandatory) and we drop the
support for
1. phandle args-specifier validation
2. Deferred probing
One solution to handle #1 is to also make the hwlock-num-locks property
also mandatory (even though it could have been read from a register and
I wonder what implementations should do if there is a mismatch between
DT provided value and the value read from IP register). I am not sure
what the DT maintainers' stance is on this, as we are forcing that to
solve an implementation detail in the hwspinlock core.
And, how do you propose we solve the problem of deferred probing? This
was the solution that is resolving both the problems without changing
return codes on existing API (that was my first attempt, but you
preferred not to change API return convention).
Kumar, Mark,
Any comments here?
regards
Suman
Hi Suman,
On Thu, Nov 13, 2014 at 11:02 PM, Suman Anna <[email protected]> wrote:
> OK, lets take an example. I have say 2 device instances, say
> hwlock1: hwlock@0 {
> hwlock-num-locks = <32>
> hwlock-base-id = <0>;
> #hwlock-cells = <1>;
> };
> hwlock2: hwlock@0 {
> hwlock-num-locks = <32>
> hwlock-base-id = <32>;
> #hwlock-cells = <1>;
> };
>
> some-client {
> hwlocks = <&hwlock1 32>, <&hwlock2 0>;
> };
>
> The first args value is incorrect, and I expect the API to return an
> error. I don't think the API can make assumptions that all passed in
> values will be correct. What do you suggest that the API do here?
I think this is just one example of many potential mistakes the DT
developer can have, and that there's nothing really special about it.
What if the '5' digit below is a typo, and the actual hardware
assignment was supposed to be '4' ?
some-client {
hwlocks = <&hwlock1 5>, <&hwlock2 5>;
};
I don't see how much different this mistake is from the one you
mentioned above. There's a limit to how much we can really catch DT
mistakes in the code, just like we couldn't really catch past mistakes
in the platform data structures.
If we can easily add some validations then great, but if we have to go
to great lengths just to gain very limited validations, then I'd vote
against doing so.
> One solution to handle #1 is to also make the hwlock-num-locks property
> also mandatory (even though it could have been read from a register
Yeah, this is awkward. We shouldn't impose this on implementations
like OMAP which don't need it.
Again I think for the very limited validation this buys us - it's not worth it.
> And, how do you propose we solve the problem of deferred probing?
It seems to me that hwspin_lock_request_specific failures should be
used by clients to defer their probing. Why wouldn't such a simple
solution work?
Thanks,
Ohad.
Hi Ohad,
On 11/14/2014 01:11 AM, Ohad Ben-Cohen wrote:
> Hi Suman,
>
> On Thu, Nov 13, 2014 at 11:02 PM, Suman Anna <[email protected]> wrote:
>> OK, lets take an example. I have say 2 device instances, say
>> hwlock1: hwlock@0 {
>> hwlock-num-locks = <32>
>> hwlock-base-id = <0>;
>> #hwlock-cells = <1>;
>> };
>> hwlock2: hwlock@0 {
>> hwlock-num-locks = <32>
>> hwlock-base-id = <32>;
>> #hwlock-cells = <1>;
>> };
>>
>> some-client {
>> hwlocks = <&hwlock1 32>, <&hwlock2 0>;
>> };
>>
>> The first args value is incorrect, and I expect the API to return an
>> error. I don't think the API can make assumptions that all passed in
>> values will be correct. What do you suggest that the API do here?
>
> I think this is just one example of many potential mistakes the DT
> developer can have, and that there's nothing really special about it.
>
> What if the '5' digit below is a typo, and the actual hardware
> assignment was supposed to be '4' ?
>
> some-client {
> hwlocks = <&hwlock1 5>, <&hwlock2 5>;
> };
>
> I don't see how much different this mistake is from the one you
> mentioned above. There's a limit to how much we can really catch DT
> mistakes in the code, just like we couldn't really catch past mistakes
> in the platform data structures.
>
> If we can easily add some validations then great, but if we have to go
> to great lengths just to gain very limited validations, then I'd vote
> against doing so.
OK, personally, I am not too comfortable to not perform any validation
on an API.
>
>> One solution to handle #1 is to also make the hwlock-num-locks property
>> also mandatory (even though it could have been read from a register
>
> Yeah, this is awkward. We shouldn't impose this on implementations
> like OMAP which don't need it.
>
> Again I think for the very limited validation this buys us - it's not worth it.
>
>> And, how do you propose we solve the problem of deferred probing?
>
> It seems to me that hwspin_lock_request_specific failures should be
> used by clients to defer their probing. Why wouldn't such a simple
> solution work?
Because the API always returns NULL on failures and there is no way for
the clients to figure out if the lock id passed is invalid or the bank
containing the lock is not registered. This was an old discussion on v4
[1], and you insisted that we don't change the return code convention on
the API. I had couple of patches on v5 that dealt with this, but even
that requires the list of registered devices.
regards
Suman
[1] https://patchwork.kernel.org/patch/3481331/
On Fri, Nov 14, 2014 at 7:09 PM, Suman Anna <[email protected]> wrote:
>> It seems to me that hwspin_lock_request_specific failures should be
>> used by clients to defer their probing. Why wouldn't such a simple
>> solution work?
> Because the API always returns NULL on failures and there is no way for
> the clients to figure out if the lock id passed is invalid or the bank
> containing the lock is not registered.
It seems to me this may always be the case - lock ids may be wrong and
there's no way to fully validate them.
Let's start with the simpler approach where
hwspin_lock_request_specific failures are used by clients to defer
their probing.
If a real use case will require changes we can always do that later,
though it seems to me the only gain by changing this API is to catch a
small subset of fatal DT mistakes which will anyway be caught very
early in the development.
Thanks,
Ohad.