| Message ID | 20170425125547.865FB508DA7@solo.franken.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote: > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(), > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent() > and clk_get_sys() are wrong. According to spec these functions > should either return a pointer to a struct clk or a valid IS_ERR > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes > more sense. That's wrong. When the clk API is disabled, the expected behaviour is that drivers will not fail. Returning ERR_PTR(-ENODEV) will cause them to fail, so will break platforms. NAK. > Without this change serial console on SNI RM400 machines (MIPS arch) > is broken, because sccnxp driver doesn't get a valid clock rate. So the driver needs to depend on HAVE_CLOCK.
On Tue, Apr 25, 2017 at 05:31:37PM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote: > > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(), > > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent() > > and clk_get_sys() are wrong. According to spec these functions > > should either return a pointer to a struct clk or a valid IS_ERR > > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes > > more sense. > > That's wrong. When the clk API is disabled, the expected behaviour is > that drivers will not fail. Returning ERR_PTR(-ENODEV) will cause them > to fail, so will break platforms. > > NAK. > then we have to go through all drivers, which could work with and without HAVE_CLK and replace the IS_ERR() checks with something like IS_ERR_OR_NULL(). Easy for the part I'm interested in the first place. > > Without this change serial console on SNI RM400 machines (MIPS arch) > > is broken, because sccnxp driver doesn't get a valid clock rate. > > So the driver needs to depeond on HAVE_CLOCK. the driver worked without HAVE_CLOCK before just fine, and while it got invaded by CLK API it got broken. No problem to fix that, but just looking at include/linux/clk.h: /** * devm_clk_get - lookup and obtain a managed reference to a clock producer. * @dev: device for clock "consumer" * @id: clock consumer ID * * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. The implementation I would expect either no replacement for that, if !HAVE_CLOCK or simple a sane result code... and NULL isn't sane at least with the description above... But still it's your call. Thomas.
On Tue, Apr 25, 2017 at 10:54:35PM +0200, Tom Bogendoerfer wrote: > On Tue, Apr 25, 2017 at 05:31:37PM +0100, Russell King - ARM Linux wrote: > > On Tue, Apr 25, 2017 at 02:30:07PM +0200, Thomas Bogendoerfer wrote: > > > If CONFIG_HAVE_CLOCK is not set, return values of clk_get(), > > > devm_clk_get(), devm_get_clk_from_child(), clk_get_parent() > > > and clk_get_sys() are wrong. According to spec these functions > > > should either return a pointer to a struct clk or a valid IS_ERR > > > condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes > > > more sense. > > > > That's wrong. When the clk API is disabled, the expected behaviour is > > that drivers will not fail. Returning ERR_PTR(-ENODEV) will cause them > > to fail, so will break platforms. > > > > NAK. > > > > then we have to go through all drivers, which could work with and > without HAVE_CLK and replace the IS_ERR() checks with something > like IS_ERR_OR_NULL(). Easy for the part I'm interested in the > first place. What you describe is exactly what you're proposing to happen if your patch gets merged - we go from a situation where drivers that do this today: clk = devm_clk_get(); if (IS_ERR(clk)) return PTR_ERR(clk); start failing to probe, whereas the current situation is that they work. It's well established that the NULL clk means that there is no clock available (eg, because the architecture doesn't support the clk API) and this allows drivers to work across different architectures today. provided they aren't reliant on obtaining the current clock rate. > > > Without this change serial console on SNI RM400 machines (MIPS arch) > > > is broken, because sccnxp driver doesn't get a valid clock rate. > > > > So the driver needs to depeond on HAVE_CLOCK. > > the driver worked without HAVE_CLOCK before just fine, and while it > got invaded by CLK API it got broken. > > No problem to fix that, but just looking at include/linux/clk.h: > > /** > * devm_clk_get - lookup and obtain a managed reference to a clock producer. > * @dev: device for clock "consumer" > * @id: clock consumer ID > * > * Returns a struct clk corresponding to the clock producer, or > * valid IS_ERR() condition containing errno. The implementation > > I would expect either no replacement for that, if !HAVE_CLOCK > or simple a sane result code... and NULL isn't sane at least > with the description above... As far as drivers are concerned, any value returned that IS_ERR() tests false must not be assumed to be a failure. However, looking at commit 90efa75f7ab0, there's one point that's definitely wrong, and another that can be improved to avoid your problem. 1. clk_get_rate() on a disabled clock: * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. The clock is expected to be enabled before clk_get_rate() is called, and the driver is not doing that. 2. if uartclk is zero after enabling, then there's clearly no clock rate available, and the driver really ought to fallback to using the old method. So, I'd suggest that the driver should be coded: clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(clk)) { ret = PTR_ERR(clk); if (ret == -EPROBE_DEFER) goto err_out; uartclk = 0; } else { uartclk = clk_get_rate(clk); } if (!uartclk) { dev_notice(&pdev->dev, "Using default clock frequency\n"); uartclk = s->freq_std; } /* Check input frequency */ ...
diff --git a/include/linux/clk.h b/include/linux/clk.h index e9d36b3..b844a65 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -442,18 +442,18 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id); static inline struct clk *clk_get(struct device *dev, const char *id) { - return NULL; + return ERR_PTR(-ENODEV); } static inline struct clk *devm_clk_get(struct device *dev, const char *id) { - return NULL; + return ERR_PTR(-ENODEV); } static inline struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id) { - return NULL; + return ERR_PTR(-ENODEV); } static inline void clk_put(struct clk *clk) {} @@ -494,12 +494,12 @@ static inline int clk_set_parent(struct clk *clk, struct clk *parent) static inline struct clk *clk_get_parent(struct clk *clk) { - return NULL; + return ERR_PTR(-ENODEV); } static inline struct clk *clk_get_sys(const char *dev_id, const char *con_id) { - return NULL; + return ERR_PTR(-ENODEV); } #endif
If CONFIG_HAVE_CLOCK is not set, return values of clk_get(), devm_clk_get(), devm_get_clk_from_child(), clk_get_parent() and clk_get_sys() are wrong. According to spec these functions should either return a pointer to a struct clk or a valid IS_ERR condition. NULL is neither, so returning ERR_PTR(-ENODEV) makes more sense. Without this change serial console on SNI RM400 machines (MIPS arch) is broken, because sccnxp driver doesn't get a valid clock rate. Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> --- include/linux/clk.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)