On 18 December 2017 at 20:44, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Mon, Dec 18, 2017 at 7:54 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>> On 15 December 2017 at 21:13, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>>>
>>>>> @@ -87,6 +88,30 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>>>> if (ret)
>>>>> reg_io_width = 4;
>>>>>
>>>>> + ret = of_hwspin_lock_get_id(np, 0);
>>>>> + if (ret > 0) {
>>>>> + syscon_config.hwlock_id = ret;
>>>>> + syscon_config.hwlock_mode = HWLOCK_IRQSTATE;
>>>>> + } else {
>>>>> + switch (ret) {
>>>>> + case -ENOENT:
>>>>> + /* Ignore missing hwlock, it's optional. */
>>>>> + break;
>>>>> + case 0:
>>>>> + /* In case of the HWSPINLOCK is not enabled. */
>>>>> + if (!IS_ENABLED(CONFIG_HWSPINLOCK))
>>>>> + break;
>>>>> +
>>>>> + ret = -EINVAL;
>>>>> + /* fall-through */
>>>>> + default:
>>>>> + pr_err("Failed to retrieve valid hwlock: %d\n", ret);
>>>>> + /* fall-through */
>>>>> + case -EPROBE_DEFER:
>>>>> + goto err_regmap;
>>>>> + }
>>>
>>> The 'case 0' seems odd here, are we sure that this is always a failure?
>>> From the of_hwspin_lock_get_id() definition it looks like zero might
>>> be valid, and the !CONFIG_HWSPINLOCK implementation appears
>>> to be written so that we should consider '0' valid but unused and
>>> silently continue with that. If that is generally not the intended
>>> use, it should probably return -EINVAL or something like that.
>>
>> Yes, 0 is valid for of_hwspin_lock_get_id(), but if we pass 'hwlock id
>> = 0' to regmap, the regmap core will not regard it as a valid hwlock
>> id to request the hwlock and will use default mutex lock instead of
>> hwlock, which will cause problems. Meanwhile if we silently continue
>> with case 0, users will not realize that they set one invalid hwlock
>> id to regmap core, so here we regarded case 0 as one invalid id to
>> print error messages for users.
>
> Something else still seems wrong then: If regmap doesn't accept a zero
> lock-id, then of_hwspin_lock_get_id() should never return that as a
> valid ID, right?
Um, why regmap doesn't accept a zero lock-id, that because regmap will
reguest hwlock depending on the 'regmap_config->hwlock_id' is not
zero, if regmap regard a zero lock-id as valid which will affect other
'struct regmap_config' definition. So users should not assign the zero
lock-id to regmap.
Now of_hwspin_lock_get_id() can return 0 as valid, which depend on
what is the base id registered by hwspinlock driver. So you think we
should not regard 0 as valid from of_hwspin_lock_get_id(), I can try
to send another patch to fix.
But for this patch I still think we need regard the zero lock-id as
invalid and gave error messages to users.
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
