Re: [PATCH] clk: tegra: use pll_ref as the pll_e parent
On Wed, Oct 30, 2013 at 01:41:29AM +0100, Peter De Schrijver wrote: Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. Signed-off-by: Peter De Schrijver pdeschrij...@nvidia.com I will squash this into the next tegra-clk-next as the previous pull request has never made it. Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: tegra: add FUSE clock device
On Thu, Nov 21, 2013 at 03:38:10AM +0100, Alexandre Courbot wrote:
This clock is needed to ensure the FUSE registers can be accessed
without freezing the system.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
Acked-by: Peter De Schrijver pdeschrij...@nvidia.com
drivers/clk/tegra/clk-tegra114.c | 1 +
drivers/clk/tegra/clk-tegra124.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 1 +
drivers/clk/tegra/clk-tegra30.c | 2 +-
4 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/tegra/clk-tegra114.c
b/drivers/clk/tegra/clk-tegra114.c
index dbab27f773b5..b29d31d94d2e 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -925,6 +925,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = sclk, .dt_id = TEGRA114_CLK_SCLK },
{ .con_id = hclk, .dt_id = TEGRA114_CLK_HCLK },
{ .con_id = pclk, .dt_id = TEGRA114_CLK_PCLK },
+ { .con_id = fuse, .dt_id = TEGRA114_CLK_FUSE },
{ .dev_id = rtc-tegra, .dt_id = TEGRA114_CLK_RTC },
{ .dev_id = timer, .dt_id = TEGRA114_CLK_TIMER },
};
diff --git a/drivers/clk/tegra/clk-tegra124.c
b/drivers/clk/tegra/clk-tegra124.c
index 266e80b51d38..e123fd218983 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -1007,6 +1007,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = sclk, .dt_id = TEGRA124_CLK_SCLK },
{ .con_id = hclk, .dt_id = TEGRA124_CLK_HCLK },
{ .con_id = pclk, .dt_id = TEGRA124_CLK_PCLK },
+ { .con_id = fuse, .dt_id = TEGRA124_CLK_FUSE },
{ .dev_id = rtc-tegra, .dt_id = TEGRA124_CLK_RTC },
{ .dev_id = timer, .dt_id = TEGRA124_CLK_TIMER },
};
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 58faac5f509e..9dd264886621 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -452,6 +452,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = sclk, .dt_id = TEGRA20_CLK_SCLK },
{ .con_id = hclk, .dt_id = TEGRA20_CLK_HCLK },
{ .con_id = pclk, .dt_id = TEGRA20_CLK_PCLK },
+ { .con_id = fuse, .dt_id = TEGRA20_CLK_FUSE },
{ .con_id = twd, .dt_id = TEGRA20_CLK_TWD },
{ .con_id = audio, .dt_id = TEGRA20_CLK_AUDIO },
{ .con_id = audio_2x, .dt_id = TEGRA20_CLK_AUDIO_2X },
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 51c093c96657..6d96b307d48c 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -659,7 +659,7 @@ static struct tegra_devclk devclks[] __initdata = {
{ .con_id = pcie, .dev_id = tegra-pcie, .dt_id = TEGRA30_CLK_PCIE },
{ .con_id = afi, .dev_id = tegra-pcie, .dt_id = TEGRA30_CLK_AFI },
{ .con_id = pciex, .dev_id = tegra-pcie, .dt_id = TEGRA30_CLK_PCIEX
},
- { .con_id = fuse, .dev_id = fuse-tegra, .dt_id = TEGRA30_CLK_FUSE },
+ { .con_id = fuse, .dt_id = TEGRA30_CLK_FUSE },
{ .con_id = fuse_burn, .dev_id = fuse-tegra, .dt_id =
TEGRA30_CLK_FUSE_BURN },
{ .con_id = apbif, .dev_id = tegra30-ahub, .dt_id =
TEGRA30_CLK_APBIF },
{ .con_id = hda2hdmi, .dev_id = tegra30-hda, .dt_id =
TEGRA30_CLK_HDA2HDMI },
--
1.8.4.2
--
To unsubscribe from this list: send the line unsubscribe linux-tegra in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] clk: tegra: fix blink clock rate
On Thu, Nov 21, 2013 at 12:47:18AM +0100, Stephen Warren wrote: From: Stephen Warren swar...@nvidia.com The blink clock rate needs to be configured, or it will run at ~1Hz rather than the desired 32KHz. If it runs at the wrong rate, e.g. the SDIO WiFi on Seaboard and Cardhu will fail to be detected. Signed-off-by: Stephen Warren swar...@nvidia.com --- This probably needs to be squashed into commit 32721a734a3d clk: tegra: move PMC, fixed clocks to common files. --- Acked-by: Peter De Schrijver pdeschrij...@nvidia.com I will squash this into the mentioned commit. Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: tegra: add FUSE clock device
On 11/22/2013 06:41 AM, Peter De Schrijver wrote: On Thu, Nov 21, 2013 at 03:38:10AM +0100, Alexandre Courbot wrote: This clock is needed to ensure the FUSE registers can be accessed without freezing the system. Signed-off-by: Alexandre Courbot acour...@nvidia.com Acked-by: Peter De Schrijver pdeschrij...@nvidia.com Aren't you taking this patch through the clock tree? -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/21/2013 11:54 PM, Dan Williams wrote:
On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 11/20/2013 08:22 PM, Dan Williams wrote:
On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 11/20/2013 01:23 PM, Williams, Dan J wrote:
...
Why do the drivers that call dma_request_channel need to convert it to
an ERR value? i.e. what's problematic about the below (not compile
tested)?
...
diff --git a/arch/arm/plat-samsung/dma-ops.c
b/arch/arm/plat-samsung/dma-ops.c
...
@@ -22,16 +22,20 @@ static unsigned samsung_dmadev_request(enum dma_ch
dma_ch,
struct samsung_dma_req *param,
struct device *dev, char *ch_name)
...
+ if (dev-of_node) {
+ chan = dma_request_slave_channel(dev, ch_name);
+ return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
+ } else {
return (unsigned)dma_request_channel(mask, pl330_filter,
(void *)dma_ch);
+ }
The argument is that if a function returns errors encoded as an ERR
pointer, then callers must assume that any non-IS_ERR value that the
function returns is valid. NULL is one of those values. As such, callers
can no longer check the value against NULL, but must use IS_ERR().
Converting any IS_ERR() returns to NULL theoretically is the act of
converting one valid return value to some other completely random return
value.
You describe how IS_ERR() works, but you didn't address the patch.
There's nothing random about the changes to samsung_dmadev_request().
It still returns NULL or a valid channel just as before.
I was addressing the patch. I guess I should have explained as follows.
First, the following code is technically buggy:
No, it's not, but I think we have different implementations in mind.
+ chan = dma_request_slave_channel(dev, ch_name);
+ return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
... since it assumes that dma_request_slave_channel() never returns NULL
as a valid non-error value. This is specifically prohibited by the fact
that dma_request_slave_channel() returns either an ERR value or a valid
value; in that case, NULL is not an ERR value, and hence must be
considered valid.
Let's stop there and be clear we are talking about the same proposal.
The proposal is dma_request_slave_channel only returns errors or valid
pointers, never NULL.
OK, so if you make that assumption, I guess it's safe. However, I
believe that's a new class of return value. To date, we have had two
classes:
a) Returns a valid value (which could include NULL), or an ERR value.
b) Returns a valid value (which doesn't include ERR values), or NULL.
You're talking about adding a third class:
c) Returns a valid value (which doesn't include NULL or ERR values), or
an ERR value.
Russell at least has argued in the past that APIs that return ERR values
for errors by definition can return NULL as a valid return value.
However, if we go against that and explicitly define the API the way you
propose, and nobody objects to defining it that way, then yes, that
would work out OK.
--
To unsubscribe from this list: send the line unsubscribe linux-tegra in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs
On 11/22/2013 12:41 AM, Grant Likely wrote: On Thu, 21 Nov 2013 12:04:18 -0700, Stephen Warren swar...@wwwdotorg.org wrote: On 11/21/2013 06:15 AM, Grant Likely wrote: On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu hd...@nvidia.com wrote: IOMMU devices on the bus need to be poplulated first, then iommu master devices are done later. With CONFIG_OF_IOMMU, iommus= DT binding would be used to identify whether a device can be an iommu msater or not. If a device can, we'll defer to populate that device till an iommu device is populated. Once an iommu device is populated, dev-bus-iommu_ops is set in the bus. Then, those defered iommu master devices are populated and configured for IOMMU with help of the already populated iommu device via iommu_ops-add_device(). Multiple IOMMUs can be listed on this iommus binding so that a device can have multiple IOMMUs attached. Signed-off-by: Hiroshi Doyu hd...@nvidia.com --- v5: Use iommus= binding instread of arm,smmu's #stream-id-cells. v4: This is newly added, and the successor of the following RFC: [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach() http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html --- drivers/base/dd.c| 5 + drivers/iommu/of_iommu.c | 22 ++ include/linux/of_iommu.h | 7 +++ 3 files changed, 34 insertions(+) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6e892d4 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -25,6 +25,7 @@ #include linux/async.h #include linux/pm_runtime.h #include linux/pinctrl/devinfo.h +#include linux/of_iommu.h #include base.h #include power/power.h @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) dev-driver = drv; + ret = of_iommu_attach(dev); + if (ret) + goto probe_failed; + /* If using pinctrl, bind pins now before probing */ ret = pinctrl_bind_pins(dev); if (ret) I'm very concerned about this approach. Hooking into the core probe path for things like this is not going to scale well. I'm not thrilled with the pinctrl hook being here either, but that is already merged. :-( Also, hooking in here is going affect every single device device driver probe path, and a large number of them are never, ever, going to have iommu interactions. There needs to be a less invasive way of doing what you want. I still feel like the individual device drivers themselves need to be aware that they might be hooking through an IOMMU. Or, if they are hooking through a bus like PCIe, then have the PCIe bus defer creating child devices until the IOMMU is configured and in place. I general though, couldn't any MMIO on-SoC device potentially be affected by an IOMMU? The point of putting the call to of_iommu_attach() here rather than inside individual driver's probe() is to avoid requiring every driver having to paste more boiler-plate into probe(). It seems more that IOMMU attachment is closer to being a property of the bus rather than a property of the device itself. In that context it would make more sense for the bus device to hold off child device registration or probing until the IOMMU is available. That keeps the logic out of both the core code and the individual device drivers. The bus structure that DT and Linux know about is the register bus. There's no reason that devices have to emit their master transactions onto that same bus, or onto only that same bus. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 11:04 AM, Dan Williams wrote:
On Fri, Nov 22, 2013 at 9:34 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 11/21/2013 11:54 PM, Dan Williams wrote:
On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 11/20/2013 08:22 PM, Dan Williams wrote:
On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 11/20/2013 01:23 PM, Williams, Dan J wrote:
...
Why do the drivers that call dma_request_channel need to convert it to
an ERR value? i.e. what's problematic about the below (not compile
tested)?
...
diff --git a/arch/arm/plat-samsung/dma-ops.c
b/arch/arm/plat-samsung/dma-ops.c
...
@@ -22,16 +22,20 @@ static unsigned samsung_dmadev_request(enum dma_ch
dma_ch,
struct samsung_dma_req *param,
struct device *dev, char *ch_name)
...
+ if (dev-of_node) {
+ chan = dma_request_slave_channel(dev, ch_name);
+ return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
+ } else {
return (unsigned)dma_request_channel(mask, pl330_filter,
(void *)dma_ch);
+ }
The argument is that if a function returns errors encoded as an ERR
pointer, then callers must assume that any non-IS_ERR value that the
function returns is valid. NULL is one of those values. As such, callers
can no longer check the value against NULL, but must use IS_ERR().
Converting any IS_ERR() returns to NULL theoretically is the act of
converting one valid return value to some other completely random return
value.
You describe how IS_ERR() works, but you didn't address the patch.
There's nothing random about the changes to samsung_dmadev_request().
It still returns NULL or a valid channel just as before.
I was addressing the patch. I guess I should have explained as follows.
First, the following code is technically buggy:
No, it's not, but I think we have different implementations in mind.
+ chan = dma_request_slave_channel(dev, ch_name);
+ return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
... since it assumes that dma_request_slave_channel() never returns NULL
as a valid non-error value. This is specifically prohibited by the fact
that dma_request_slave_channel() returns either an ERR value or a valid
value; in that case, NULL is not an ERR value, and hence must be
considered valid.
Let's stop there and be clear we are talking about the same proposal.
The proposal is dma_request_slave_channel only returns errors or valid
pointers, never NULL.
OK, so if you make that assumption, I guess it's safe.
I made that assumption because that is what your original patch proposed:
+/**
+ * dma_request_slave_channel_or_err - try to allocate an exclusive
slave channel
+ * @dev: pointer to client device structure
+ * @name: slave channel name
+ *
+ * Returns pointer to appropriate dma channel on success or an error pointer.
+ */
What's the benefit of leaking NULL values to callers? If they already
need to check for err, why force them to check for NULL too?
Returns pointer to appropriate dma channel on success or an error
pointer. means that callers only have to check for an ERR value. If the
function returns NULL, then other DMA-related functions must treat that
as a valid channel ID. This is case (a) in my previous email.
--
To unsubscribe from this list: send the line unsubscribe linux-tegra in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. Make the caller's life easier and just turn success or failure like before. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 12:49 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. No, callers should just follow the documentation. If all error cases are indicated by an ERR pointer, then there is no need to check for NULL. In fact, client must not check anything beyond whether the value is an ERR value or not. So, there's no need to use IS_ERR_OR_NULL. It's up to the API to make sure that it returns values that are valid for other calls to related APIs. If that doesn't include NULL, it won't return NULL. If it does, it might. But, that's an internal implementation detail of the API (and associated APIs), not something that clients should know about. One situation where a NULL might be valid is where the return value isn't really a pointer, but an integer index or ID cast to a pointer. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: tegra: Provide dummy powergate implementation
In order to support increased build test coverage for drivers, implement
dummies for the powergate implementation. This will allow the drivers to
be built without requiring support for Tegra to be selected.
Signed-off-by: Thierry Reding tred...@nvidia.com
---
include/linux/tegra-powergate.h | 27 +++
1 file changed, 27 insertions(+)
diff --git a/include/linux/tegra-powergate.h b/include/linux/tegra-powergate.h
index c98cfa406952..fd4498329c7c 100644
--- a/include/linux/tegra-powergate.h
+++ b/include/linux/tegra-powergate.h
@@ -45,6 +45,7 @@ struct clk;
#define TEGRA_POWERGATE_3D0TEGRA_POWERGATE_3D
+#ifdef CONFIG_ARCH_TEGRA
int tegra_powergate_is_powered(int id);
int tegra_powergate_power_on(int id);
int tegra_powergate_power_off(int id);
@@ -52,5 +53,31 @@ int tegra_powergate_remove_clamping(int id);
/* Must be called with clk disabled, and returns with clk enabled */
int tegra_powergate_sequence_power_up(int id, struct clk *clk);
+#else
+static inline int tegra_powergate_is_powered(int id)
+{
+ return -ENOSYS;
+}
+
+static inline int tegra_powergate_power_on(int id)
+{
+ return -ENOSYS;
+}
+
+static inline int tegra_powergate_power_off(int id)
+{
+ return -ENOSYS;
+}
+
+static inline int tegra_powergate_remove_clamping(int id)
+{
+ return -ENOSYS;
+}
+
+static inline int tegra_powergate_sequence_power_up(int id, struct clk *clk)
+{
+ return -ENOSYS;
+}
+#endif
#endif /* _MACH_TEGRA_POWERGATE_H_ */
--
1.8.4.2
--
To unsubscribe from this list: send the line unsubscribe linux-tegra in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 12:49 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. No, callers should just follow the documentation. If all error cases are indicated by an ERR pointer, then there is no need to check for NULL. In fact, client must not check anything beyond whether the value is an ERR value or not. So, there's no need to use IS_ERR_OR_NULL. It's up to the API to make sure that it returns values that are valid for other calls to related APIs. If that doesn't include NULL, it won't return NULL. If it does, it might. But, that's an internal implementation detail of the API (and associated APIs), not something that clients should know about. One situation where a NULL might be valid is where the return value isn't really a pointer, but an integer index or ID cast to a pointer. Ok that's the piece I am missing, and maybe explains why samsung_dmadev_request() looks so broken. Are there really implementations out there that somehow know that the return value from dma_request_slave channel is not a (struct dma_chan *)?? At that point just change the prototype of dma_request_slave_channel to: MAGIC_t dma_request_slave_channel(struct device *dev, const char *name) Those clients need to be killed or fixed, otherwise how do you guarantee that the 'integer index or ID' does not collide with the ERR_PTR() number space? -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
мир впору постигать без очков
Ваши глаза вполне могут замечать превосходно http://glazmen.ru/ -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 01:46 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 12:49 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. No, callers should just follow the documentation. If all error cases are indicated by an ERR pointer, then there is no need to check for NULL. In fact, client must not check anything beyond whether the value is an ERR value or not. So, there's no need to use IS_ERR_OR_NULL. It's up to the API to make sure that it returns values that are valid for other calls to related APIs. If that doesn't include NULL, it won't return NULL. If it does, it might. But, that's an internal implementation detail of the API (and associated APIs), not something that clients should know about. One situation where a NULL might be valid is where the return value isn't really a pointer, but an integer index or ID cast to a pointer. Ok that's the piece I am missing, and maybe explains why samsung_dmadev_request() looks so broken. Are there really implementations out there that somehow know that the return value from dma_request_slave channel is not a (struct dma_chan *)?? No client of the API should know that; it'd be more like an agreement between multiple functions in the subsystem: handle = subsystemx_allocate_something(); ... subsystemx_use_handle(handle); Where subsystemx_allocate_something() casts from ID to pointer, and subsystemx_use_handle() casts back from pointer to ID. The callers would have no idea this was happening. I'm not actually aware of any specific cases where that actually happens right now, it's just that given the way subsystemx_allocate_something() is documented (valid handle/cookie return or ERR value) it's legal for subsystemx to work that way if it wants, and it should be able to change between this cast-a-handle style and actual pointer returns without clients being affected. At that point just change the prototype of dma_request_slave_channel to: MAGIC_t dma_request_slave_channel(struct device *dev, const char *name) Those clients need to be killed or fixed, otherwise how do you guarantee that the 'integer index or ID' does not collide with the ERR_PTR() number space? subsystemx_allocate_something() would have to ensure that. Probably just by imposing a maximum limit on the handle/ID values. Anyway, your proposal can certainly /work/. I simply wanted to point out that it was different to the two currently accepted styles of return value. If you're sure e.g. Russell isn't going to shout at me or you for introducing an API that works as you describe, we certainly could go ahead with it. Should we explicitly ping him to confirm that? -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Fri, Nov 22, 2013 at 1:50 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 01:46 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 12:49 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. No, callers should just follow the documentation. If all error cases are indicated by an ERR pointer, then there is no need to check for NULL. In fact, client must not check anything beyond whether the value is an ERR value or not. So, there's no need to use IS_ERR_OR_NULL. It's up to the API to make sure that it returns values that are valid for other calls to related APIs. If that doesn't include NULL, it won't return NULL. If it does, it might. But, that's an internal implementation detail of the API (and associated APIs), not something that clients should know about. One situation where a NULL might be valid is where the return value isn't really a pointer, but an integer index or ID cast to a pointer. Ok that's the piece I am missing, and maybe explains why samsung_dmadev_request() looks so broken. Are there really implementations out there that somehow know that the return value from dma_request_slave channel is not a (struct dma_chan *)?? No client of the API should know that; it'd be more like an agreement between multiple functions in the subsystem: handle = subsystemx_allocate_something(); ... subsystemx_use_handle(handle); Where subsystemx_allocate_something() casts from ID to pointer, and subsystemx_use_handle() casts back from pointer to ID. The callers would have no idea this was happening. That's a bug not a feature. That's someone abusing an api and breaking type safety to pass arbitrary data. But since we're talking in abstract 'buggy_subsytemx' terms why worry? I'm not actually aware of any specific cases where that actually happens right now, it's just that given the way subsystemx_allocate_something() is documented (valid handle/cookie return or ERR value) it's legal for subsystemx to work that way if it wants, and it should be able to change between this cast-a-handle style and actual pointer returns without clients being affected. Wait, this busted way of doing things is documented? At that point just change the prototype of dma_request_slave_channel to: MAGIC_t dma_request_slave_channel(struct device *dev, const char *name) Those clients need to be killed or fixed, otherwise how do you guarantee that the 'integer index or ID' does not collide with the ERR_PTR() number space? subsystemx_allocate_something() would have to ensure that. Probably just by imposing a maximum limit on the handle/ID values. Anyway, your proposal can certainly /work/. I simply wanted to point out that it was different to the two currently accepted styles of return value. If you're sure e.g. Russell isn't going to shout at me or you for introducing an API that works as you describe, we certainly could go ahead with it. Should we explicitly ping him to confirm that? He already has in other threads IS_ERR_OR_NULL() must die, this proposal is not that. Let me go back to your note about case 2 and clarify. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/20/2013 08:22 PM, Dan Williams wrote: 2) /* These first 3 lines are part of your patch */ chan = dma_request_slave_channel(dev, ch_name); if (IS_ERR(chan) chan = NULL; if (!chan) // This test and the above are IS_ERR_OR_NULL attempt allocation some other way; No it isn't. IS_ERR_OR_NULL means the api returns 3 states (channel, null, error-pointer). The client converting error-pointer to NULL after the fact is explicit way to say that the client does not care about the error value. The client is simply throwing away the error code and converting the result back into a pass fail. /* * This is code elsewhere in a driver where DMA is optional; * that code must act differently based on whether a DMA * channel was acquired or not. So, it tests chan against * NULL. */ if (!chan) // This test and the above IS_ERR are IS_ERR_OR_NULL return -ESOMETHING; It's not, because at this point chan will never be an error pointer. Sure you could do follow on cleanups to allow this driver to propagate the dma_request_slave_channel error code up and change this to if (IS_ERR(chan)) return PTR_ERR(chan), but that's incremental to the initial conversion. In case (2) above, if the driver /only/ calls a modified dma_request_slave_channel(), all the checks could just be if (IS_ERR(chan)) instead - then problem solved. It's not solved, you would need to audit the rest of the driver to make sure that everywhere it checks a channel is NULL it checks for IS_ERR instead. That's a deeper / unnecessary rework for driver's that don't care about the reason they did not get a channel. However, if the driver mixes the new dma_request_slave_channel() and the unconverted dma_request_channel(), it has to either (a) convert an ERR return from dma_request_slave_channel() to match dma_request_channel()'s NULL error Yes, better to live with this situation and convert existing drivers vs have a subset of drivers call a new dma_request_slave_channel_or_err() API and then *still* need to convert it to NULL. return, or (b) convert a NULL return from dma_request_channel() to match dma_request_slave_channel()'s ERR return. Without the conversion, all tests would have to use the deprecated IS_ERR_OR_NULL. Either of those conversion options converts an error value from 1 API into a theoretically valid return value from the other API, which is a bug. Getting an error from dma_request_slave_channel() and converting that value to NULL is a bug because dma_request_channel() would also return NULL if it did not get a channel? That's normalization, not a bug. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 04:13 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 1:50 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 01:46 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/22/2013 12:49 PM, Dan Williams wrote: On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: The proposal is dma_request_slave_channel only returns errors or valid pointers, never NULL. OK, so if you make that assumption, I guess it's safe. I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. How can a channel be valid and NULL at the same time? Without the guarantee that dma_request_channel always returns a non-null-channel pointer or an error pointer you're forcing clients to use or open-code IS_ERR_OR_NULL. No, callers should just follow the documentation. If all error cases are indicated by an ERR pointer, then there is no need to check for NULL. In fact, client must not check anything beyond whether the value is an ERR value or not. So, there's no need to use IS_ERR_OR_NULL. It's up to the API to make sure that it returns values that are valid for other calls to related APIs. If that doesn't include NULL, it won't return NULL. If it does, it might. But, that's an internal implementation detail of the API (and associated APIs), not something that clients should know about. One situation where a NULL might be valid is where the return value isn't really a pointer, but an integer index or ID cast to a pointer. Ok that's the piece I am missing, and maybe explains why samsung_dmadev_request() looks so broken. Are there really implementations out there that somehow know that the return value from dma_request_slave channel is not a (struct dma_chan *)?? No client of the API should know that; it'd be more like an agreement between multiple functions in the subsystem: handle = subsystemx_allocate_something(); ... subsystemx_use_handle(handle); Where subsystemx_allocate_something() casts from ID to pointer, and subsystemx_use_handle() casts back from pointer to ID. The callers would have no idea this was happening. That's a bug not a feature. That's someone abusing an api and breaking type safety to pass arbitrary data. But since we're talking in abstract 'buggy_subsytemx' terms why worry? I'm not actually aware of any specific cases where that actually happens right now, it's just that given the way subsystemx_allocate_something() is documented (valid handle/cookie return or ERR value) it's legal for subsystemx to work that way if it wants, and it should be able to change between this cast-a-handle style and actual pointer returns without clients being affected. Wait, this busted way of doing things is documented? I should have said: s/is documented/would be documented/. Or perhaps s/documented/discussed/. IIRC, in previous discussions of IS_ERR_OR_NULL, this came up as a specific (perhaps hypothetical) thing that APIs could legitimately do that made it important the API clients didn't impose restrictions on return values beyond what APIs documented. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
A question about the patch: On Fri, Nov 15, 2013 at 12:54 PM, Stephen Warren swar...@wwwdotorg.org wrote: From: Stephen Warren swar...@nvidia.com [..] diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 0b88dd3d05f4..928141f6f21b 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -181,11 +181,13 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, of_node_put(dma_spec.np); + if (!ofdma defer) + return ERR_PTR(-EPROBE_DEFER); if (chan) return chan; } Why do we need to make this conditional on the value of 'defer'? If the client cares it will propagate the error if it does not care then nothing is gained by converting this to -ENODEV. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 04:50 PM, Dan Williams wrote: A question about the patch: On Fri, Nov 15, 2013 at 12:54 PM, Stephen Warren swar...@wwwdotorg.org wrote: From: Stephen Warren swar...@nvidia.com [..] diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 0b88dd3d05f4..928141f6f21b 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -181,11 +181,13 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, of_node_put(dma_spec.np); + if (!ofdma defer) + return ERR_PTR(-EPROBE_DEFER); if (chan) return chan; } Why do we need to make this conditional on the value of 'defer'? If the client cares it will propagate the error if it does not care then nothing is gained by converting this to -ENODEV. The function ends up being called from two code-paths. One of which wants the new behaviour of deferring probe if a valid DMA specifier is found but there's not registered driver for it, and the other (compatibility) path wants exactly the old behaviour. The flag is passed down from dma_request_slave_channel() (old behaviour) or dma_request_slave_channel_or_err() (new behaviour). -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On 11/22/2013 04:45 PM, Dan Williams wrote: On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/20/2013 08:22 PM, Dan Williams wrote: 2) /* These first 3 lines are part of your patch */ chan = dma_request_slave_channel(dev, ch_name); if (IS_ERR(chan) chan = NULL; if (!chan) // This test and the above are IS_ERR_OR_NULL attempt allocation some other way; No it isn't. IS_ERR_OR_NULL means the api returns 3 states (channel, null, error-pointer). The client converting error-pointer to NULL after the fact is explicit way to say that the client does not care about the error value. The client is simply throwing away the error code and converting the result back into a pass fail. The client can only translate an ERR value to NULL if it knows that the API will never return NULL. If the API could return NULL, then this translation uses a valid return value to represent the error case. Functions that return an ERR value or a valid value do NOT in general say anything either way about a return value of NULL. While this translation isn't *exactly* the same as an API returning 3 states, it is almost identical; the translation is only possible if the set of numerically possible return values from the function are segmented into 3 sets: a) Valid b) An ERR value c) NULL (which is never returned) I believe the important point in Russell's argument against IS_ERR_OR_NULL is that the segmentation should be limited to two sets (a, b) or (a, c) and not 3. I don't /think/ it was relevant whether the function segmented the return values into 3 sets just so it could guarantee that it didn't return one of those sets. If that's not the case, the following would indeed solve the problem easily: /** * dma_request_slave_channel - try to allocate an exclusive slave channel ... * Returns pointer to appropriate DMA channel on success or an error pointer. * In order to ease compatibility with other DMA request APIs, this function * guarantees NEVER to return NULL. */ /* * This is code elsewhere in a driver where DMA is optional; * that code must act differently based on whether a DMA * channel was acquired or not. So, it tests chan against * NULL. */ if (!chan) // This test and the above IS_ERR are IS_ERR_OR_NULL return -ESOMETHING; It's not, because at this point chan will never be an error pointer. It's the combination of the two. IS_ERR_OR_NULL is two separate tests in one macro. Simply separating the two tests into separate lines of code doesn't change what the code is doing. Sure you could do follow on cleanups to allow this driver to propagate the dma_request_slave_channel error code up and change this to if (IS_ERR(chan)) return PTR_ERR(chan), but that's incremental to the initial conversion. In case (2) above, if the driver /only/ calls a modified dma_request_slave_channel(), all the checks could just be if (IS_ERR(chan)) instead - then problem solved. It's not solved, you would need to audit the rest of the driver to make sure that everywhere it checks a channel is NULL it checks for IS_ERR instead. That's a deeper / unnecessary rework for driver's that don't care about the reason they did not get a channel. I was of course assuming that the auditing would be done, and indeed when I first started work on this conversion, it's exactly what I was doing. That's why I said *all* checks, not just the first check. However, if the driver mixes the new dma_request_slave_channel() and the unconverted dma_request_channel(), it has to either (a) convert an ERR return from dma_request_slave_channel() to match dma_request_channel()'s NULL error Yes, better to live with this situation and convert existing drivers vs have a subset of drivers call a new dma_request_slave_channel_or_err() API and then *still* need to convert it to NULL. return, or (b) convert a NULL return from dma_request_channel() to match dma_request_slave_channel()'s ERR return. Without the conversion, all tests would have to use the deprecated IS_ERR_OR_NULL. Either of those conversion options converts an error value from 1 API into a theoretically valid return value from the other API, which is a bug. Getting an error from dma_request_slave_channel() and converting that value to NULL is a bug because dma_request_channel() would also return NULL if it did not get a channel? That's normalization, not a bug. No, it's a bug because if dma_request_slave_channel() is documented to return valid-or-ERR, then assuming that it can never return NULL is inconsistent with that. The translation is only possible if it's documented to return valid-or-ERR-but-never-NULL. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Fri, Nov 22, 2013 at 11:10:01AM -0700, Stephen Warren wrote: On 11/22/2013 11:04 AM, Dan Williams wrote: I made that assumption because that is what your original patch proposed: +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ What's the benefit of leaking NULL values to callers? If they already need to check for err, why force them to check for NULL too? Returns pointer to appropriate dma channel on success or an error pointer. means that callers only have to check for an ERR value. If the function returns NULL, then other DMA-related functions must treat that as a valid channel ID. This is case (a) in my previous email. Stephen, Dan, My point (which you quoted) is a fine one - read the definition above. Returns pointer to appropriate DMA channel on success or an error pointer. This defines the range of values which are considered successful, and those which are considered to be errors. Error pointers are defined by IS_ERR(ptr) being true (not by IS_ERR_OR_NULL(ptr) being true. Conversely, it defines what are considered to be non-errors. Therefore, users of such a function _must_ check the return value using IS_ERR() and not the IS_ERR_OR_NULL() abomination. The question about NULL is unanswered, but with nothing specified, users must assume that if a subsystem returns NULL, it's fine to pass that back to the subsystem. If the subsystem didn't intend for NULL to be valid, it shouldn't be returning NULL from such a defined function. It's not up to the user of the interface to dream up an error code if the subsystem happens to return NULL, or do other crap stuff like this: if (IS_ERR_OR_NULL(ptr)) return PTR_ERR(ptr); which we already see cropping up from time to time. So, my argument is that if you define an API to be pointers on success, or error pointers then your API better handle any cookie you return which satisfies IS_ERR(ptr) = false - which by definition includes NULL. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/31] dma: add channel request API that supports deferred probe
On Fri, Nov 22, 2013 at 4:17 PM, Stephen Warren swar...@wwwdotorg.org wrote: No, it's a bug because if dma_request_slave_channel() is documented to return valid-or-ERR, then assuming that it can never return NULL is inconsistent with that. The translation is only possible if it's documented to return valid-or-ERR-but-never-NULL. Yes! We are in violent agreement about this point. Make dma_request_slave_channel return valid-or-ERR-but-never-NULL, and show me the compat user that would care if it got an EPROBE_DEFER instead of ENODEV. -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
