Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Ohad, On 10/06/2014 04:44 AM, Ohad Ben-Cohen wrote: On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna s-a...@ti.com wrote: I am yet to receive any comments on v6, but that series should address both your need for a probe deferral and Ohad's request to not change any return types. Please give it a try and let me know if you have any comments. Guys, Just to let you know we have several holidays around here these days, and I intend to review all pending patches immediately soon after. Ping on this. Can you review the latest series v6 [1] and pick it up for 3.19? The MSM spinlock driver is also blocked/dependent on that series. regards Suman [1] http://marc.info/?l=linux-arm-kernelm=141055365513902w=2 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Nov 6, 2014 at 8:24 PM, Suman Anna s-a...@ti.com wrote: Ping on this. Can you review the latest series v6 [1] and pick it up for 3.19? The MSM spinlock driver is also blocked/dependent on that series. Sure, it's on my mind, I hope I'll get to it next week. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna s-a...@ti.com wrote: I am yet to receive any comments on v6, but that series should address both your need for a probe deferral and Ohad's request to not change any return types. Please give it a try and let me know if you have any comments. Guys, Just to let you know we have several holidays around here these days, and I intend to review all pending patches immediately soon after. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote: This patch adds three new OF helper functions to use/request locks from a hwspinlock device instantiated through a device-tree blob. Hi Ohad, Suman I'm about to send out some patches that depends on this functionality, how do we move forward? I still think it's wrong to not return -EPROBE_DEFER, but I much rather have the code returning NULL than not having it in the tree (we can always argue about it later...). @Suman, do you remember if there was any other comments on the patch? @Ohad, do you object merging Suman's patch in it's current form? I think it should still apply cleanly. Regards, Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Bjorn, On 09/26/2014 09:40 AM, Bjorn Andersson wrote: On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote: This patch adds three new OF helper functions to use/request locks from a hwspinlock device instantiated through a device-tree blob. Hi Ohad, Suman I'm about to send out some patches that depends on this functionality, how do we move forward? I still think it's wrong to not return -EPROBE_DEFER, but I much rather have the code returning NULL than not having it in the tree (we can always argue about it later...). @Suman, do you remember if there was any other comments on the patch? I have posted two further revisions of this series, the latest is v6 [1]. I added additional patches in v5 that added the concept of reserved locks, and I have posted them as a separate RFC [2] for v6 so as to not block the core DT support. In anycase, the latest v6 version does not define the of_hwspin_lock_request_specific() function anymore, and it is replaced with of_hwspin_lock_get_id() function, based on Ohad's review comments on v5, and I did add the support for -EPROBE_DEFER in this API, without changing any of the existing return code conventions. I am yet to receive any comments on v6, but that series should address both your need for a probe deferral and Ohad's request to not change any return types. Please give it a try and let me know if you have any comments. regards Suman [1] http://marc.info/?l=linux-arm-kernelm=141055365513902w=2 [2] http://marc.info/?l=linux-arm-kernelm=14104214657w=2 @Ohad, do you object merging Suman's patch in it's current form? I think it should still apply cleanly. Regards, Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Fri, Mar 14, 2014 at 5:23 PM, Josh Cartwright jo...@codeaurora.org wrote: So, are you suggesting that because fatal errors should be extremely rare, a consuming driver should just assume that if NULL is returned from a hwspin_lock_request*() function that it was the device not yet probed case that was hit? No - it's not the scarcity, it's the severity. The error path that will be optimized here is an invalid id. If this happens, the consumer will crash and burn, and I'm not sure that slightly optimizing his death is very interesting? BTW the hwspinlock core once did use ERR_PTR and friends, and it was changed due to convincing arguments against that methodology on this mailing list. We can change it back but we need a strong(er) case. Note that having the consumer/hwspinlock device relationship modeled in devicetree introduces more potential failure cases... Yeah. Even the error above, presumed to be EPROBE_DEFER, may be a symptom of some other fatal error that occurred, and we can't be sure that a future request will surely be satisfied. So before we bloat our code, I suggest that we wait for consumers to show up and see if there's real benefit. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Suman, On Tue, Mar 4, 2014 at 7:38 PM, Suman Anna s-a...@ti.com wrote: Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. Can you confirm the changes you want me to make, so that I can refresh and post a v5 for 3.15? Sorry, I missed your replies for some reason. I prefer we stick with the current error handling code because I find the alternative inferior (as long as it's not strictly needed). Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson bj...@kryo.se wrote: When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. I'm not convinced. The only advantage this brings is to avoid retrying in case a fatal error has occurred. Such fatal errors are extremely rare, and when they show up - extremely painful, and I suspect that optimizing them wouldn't be a big win. OTOH, keeping the code easier to read and less error prone is a big win. I prefer we keep it simple for now. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Fri, Mar 14, 2014 at 03:12:26PM +0200, Ohad Ben-Cohen wrote: On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson bj...@kryo.se wrote: When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. I'm not convinced. The only advantage this brings is to avoid retrying in case a fatal error has occurred. Such fatal errors are extremely rare, and when they show up - extremely painful, and I suspect that optimizing them wouldn't be a big win. So, are you suggesting that because fatal errors should be extremely rare, a consuming driver should just assume that if NULL is returned from a hwspin_lock_request*() function that it was the device not yet probed case that was hit? Note that having the consumer/hwspinlock device relationship modeled in devicetree introduces more potential failure cases... -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Tue, Mar 04, 2014 at 11:38:23AM -0600, Suman Anna wrote: Hi Ohad, On 03/02/2014 02:19 PM, Bjorn Andersson wrote: On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com wrote: On 02/07/2014 04:49 PM, Bjorn Andersson wrote: Ohad, Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. Can you confirm the changes you want me to make, so that I can refresh and post a v5 for 3.15? What's the status on this? I'm assuming this is going to miss 3.15? Having DT support in the core will be useful to move the Qualcomm hwspinlock driver forward as well. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Ohad, On 03/02/2014 02:19 PM, Bjorn Andersson wrote: On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com wrote: On 02/07/2014 04:49 PM, Bjorn Andersson wrote: It seems to be standard practice to pass the error value back to the consumer, so you should return ERR_PTR(ret); here instead of the NULL... I have modelled the return values in this function based on the return values in the existing hwspin_lock_request interfaces. I would need to change those functions as well. Ohad, Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. Can you confirm the changes you want me to make, so that I can refresh and post a v5 for 3.15? regards Suman -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Ohad, On 03/02/2014 02:19 PM, Bjorn Andersson wrote: On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com wrote: On 02/07/2014 04:49 PM, Bjorn Andersson wrote: It seems to be standard practice to pass the error value back to the consumer, so you should return ERR_PTR(ret); here instead of the NULL... I have modelled the return values in this function based on the return values in the existing hwspin_lock_request interfaces. I would need to change those functions as well. Ohad, Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. From a current user/client perspectives, I didn't find any clients of hwspinlock within the kernel. So, this is probably the right time to change the return code convention. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. The usage on the clients will also not become too complicated. The only change on the clients is mostly the base error check change from if (!hwlock) to if (IS_ERR(hwlock)). regards Suman When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. Regards, Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com wrote: On 02/07/2014 04:49 PM, Bjorn Andersson wrote: It seems to be standard practice to pass the error value back to the consumer, so you should return ERR_PTR(ret); here instead of the NULL... I have modelled the return values in this function based on the return values in the existing hwspin_lock_request interfaces. I would need to change those functions as well. Ohad, Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of initialization failed or device not yet probed; so that the client knows if it should fail or retry later. Regards, Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com wrote: On 02/07/2014 04:49 PM, Bjorn Andersson wrote: It seems to be standard practice to pass the error value back to the consumer, so you should return ERR_PTR(ret); here instead of the NULL... I have modelled the return values in this function based on the return values in the existing hwspin_lock_request interfaces. I would need to change those functions as well. Ohad, Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Bjorn,
On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote:
This patch adds three new OF helper functions to use/request
locks from a hwspinlock device instantiated through a
device-tree blob.
Nice, I ran in to the problem of needing a probe deferral on a
hwspinlock earlier this week so I implemented this yesterday...then I
got a pointer to your series.
[snip]
/**
+ * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock
+ * @np: device node from which to request the specific hwlock
+ * @propname: property name containing hwlock specifier(s)
+ * @index: index of the hwlock
+ *
+ * This function is the OF equivalent of hwspin_lock_request_specific(). This
+ * function provides a means for users of the hwspinlock module to request a
+ * specific hwspinlock using the phandle of the hwspinlock device. The
requested
+ * lock number is indexed relative to the hwspinlock device, unlike the
+ * hwspin_lock_request_specific() which is an absolute lock number.
+ *
+ * Returns the address of the assigned hwspinlock, or NULL on error
+ */
+struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
+ const char *propname, int index)
+{
+ struct hwspinlock_device *bank;
+ struct of_phandle_args args;
+ int id;
+ int ret;
+
+ ret = of_parse_phandle_with_args(np, propname, #hwlock-cells, index,
+args);
+ if (ret) {
+ pr_warn(%s: can't parse hwlocks property of node '%s[%d]' ret =
%d\n,
+ __func__, np-full_name, index, ret);
+ return NULL;
+ }
of_parse_phandle_with_args() already does pr_err if it can't find the
phandle and on some of the issues related to arguments. So please
remove this pr_warn().
Yes, I will clean this up.
It seems to be standard practice to pass the error value back to the
consumer, so you should
return ERR_PTR(ret); here instead of the NULL...
I have modelled the return values in this function based on the return
values in the existing hwspin_lock_request interfaces. I would need to
change those functions as well.
Ohad,
Do you have any objections to the return code convention change? I agree
with Bjorn on the changes. If you are ok, then I will add a separate
patch for the existing functions and revise this patch as well.
+
+ 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) {
+ pr_warn(%s: requested hwspinlock device %s is not
registered\n,
+ __func__, args.np-full_name);
+ return NULL;
...especially since you want the consumer to have the ability to
identify this error. Here you should
return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
lock is not _yet_ registered, but will be in the future.
You should remove this pr_warn as well. The standard use of this
function would be in a probe() and just returning this error value
from that probe will give you a line in the log indicating that this
was in fact the issue.
OK.
+ }
+
+ id = bank-ops-of_xlate(bank, args);
+ if (id 0 || id = bank-num_locks) {
+ pr_warn(%s: requested lock %d is either out of range [0, %d] or
failed translation\n,
+ __func__, id, bank-num_locks - 1);
+ return NULL;
Please return ERR_PTR(-EINVAL); here.
OK, will change this based on Ohad's ack/nack.
Looking forward to your next spin, as I will actually use this interface :)
Thanks for your comments. I will wait to see if there are any additional
comments before I refresh the series later this week.
regards
Suman
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote:
This patch adds three new OF helper functions to use/request
locks from a hwspinlock device instantiated through a
device-tree blob.
Nice, I ran in to the problem of needing a probe deferral on a
hwspinlock earlier this week so I implemented this yesterday...then I
got a pointer to your series.
[snip]
/**
+ * of_hwspin_lock_request_specific() - request a OF phandle-based specific
lock
+ * @np: device node from which to request the specific hwlock
+ * @propname: property name containing hwlock specifier(s)
+ * @index: index of the hwlock
+ *
+ * This function is the OF equivalent of hwspin_lock_request_specific(). This
+ * function provides a means for users of the hwspinlock module to request a
+ * specific hwspinlock using the phandle of the hwspinlock device. The
requested
+ * lock number is indexed relative to the hwspinlock device, unlike the
+ * hwspin_lock_request_specific() which is an absolute lock number.
+ *
+ * Returns the address of the assigned hwspinlock, or NULL on error
+ */
+struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
+ const char *propname, int index)
+{
+ struct hwspinlock_device *bank;
+ struct of_phandle_args args;
+ int id;
+ int ret;
+
+ ret = of_parse_phandle_with_args(np, propname, #hwlock-cells, index,
+args);
+ if (ret) {
+ pr_warn(%s: can't parse hwlocks property of node '%s[%d]'
ret = %d\n,
+ __func__, np-full_name, index, ret);
+ return NULL;
+ }
of_parse_phandle_with_args() already does pr_err if it can't find the
phandle and on some of the issues related to arguments. So please
remove this pr_warn().
It seems to be standard practice to pass the error value back to the
consumer, so you should
return ERR_PTR(ret); here instead of the NULL...
+
+ 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) {
+ pr_warn(%s: requested hwspinlock device %s is not
registered\n,
+ __func__, args.np-full_name);
+ return NULL;
...especially since you want the consumer to have the ability to
identify this error. Here you should
return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
lock is not _yet_ registered, but will be in the future.
You should remove this pr_warn as well. The standard use of this
function would be in a probe() and just returning this error value
from that probe will give you a line in the log indicating that this
was in fact the issue.
+ }
+
+ id = bank-ops-of_xlate(bank, args);
+ if (id 0 || id = bank-num_locks) {
+ pr_warn(%s: requested lock %d is either out of range [0, %d]
or failed translation\n,
+ __func__, id, bank-num_locks - 1);
+ return NULL;
Please return ERR_PTR(-EINVAL); here.
Looking forward to your next spin, as I will actually use this interface :)
Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
