Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937348AbdLSGzv (ORCPT ); Tue, 19 Dec 2017 01:55:51 -0500 Received: from mail-ot0-f173.google.com ([74.125.82.173]:35099 "EHLO mail-ot0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932857AbdLSGzs (ORCPT ); Tue, 19 Dec 2017 01:55:48 -0500 X-Google-Smtp-Source: ACJfBoulCj33bhERsTTnotNx9YdyPNXM5MhPzwQlbpzwuB6aO47dNYIQrKjDkS1TuL8nj5AyKBYFOHJO91JTB//iDVs= MIME-Version: 1.0 In-Reply-To: References: <87ec50c846bbc7afc09ba0855aba1cdea6473308.1512048582.git.baolin.wang@linaro.org> <20171215104204.p7i27sen77ohbmzi@dell> From: Baolin Wang Date: Tue, 19 Dec 2017 14:55:47 +0800 Message-ID: Subject: Re: [PATCH v6] mfd: syscon: Add hardware spinlock support To: Arnd Bergmann Cc: Lee Jones , Rob Herring , Mark Rutland , Mark Brown , Linux Kernel Mailing List , DTML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3149 Lines: 68 On 18 December 2017 at 20:44, Arnd Bergmann wrote: > On Mon, Dec 18, 2017 at 7:54 AM, Baolin Wang wrote: >> On 15 December 2017 at 21:13, Arnd Bergmann wrote: >>> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones 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