Re: [PATCH v6] mfd: syscon: Add hardware spinlock support

[Baolin Wang]

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