Re: [PATCH] ath10k: fix the status check and wrong return
Tang Bin writes: > 在 2020/8/17 22:26, Kalle Valo 写道: >>> In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't >>> return NULL. Thus use IS_ERR() and PTR_ERR() to validate >>> the returned value instead of IS_ERR_OR_NULL(). >> Why? What's the benefit of this patch? Or what harm does >> IS_ERR_OR_NULL() create? > > Thanks for you reply, the benefit of this patch is simplify the code, > because in > > this function, I don't think the situation of 'devm_clk_get() return > NULL' exists. > > So please think about it, thanks. I think you missed my comment below: >> devm_clk_get() can return NULL if CONFIG_HAVE_CLK is disabled: >> >> static inline struct clk *devm_clk_get(struct device *dev, const char *id) >> { >> return NULL; >> } So I think this patch just creates a new bug and does not improve anything. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] ath10k: fix the status check and wrong return
On Mon, Aug 17, 2020 at 6:43 PM Tang Bin wrote: > > Hi Kalle: > > 在 2020/8/17 22:26, Kalle Valo 写道: > >> In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't > >> return NULL. Thus use IS_ERR() and PTR_ERR() to validate > >> the returned value instead of IS_ERR_OR_NULL(). > > Why? What's the benefit of this patch? Or what harm does > > IS_ERR_OR_NULL() create? > > Thanks for you reply, the benefit of this patch is simplify the code, > because in > > this function, I don't think the situation of 'devm_clk_get() return > NULL' exists. > I admit I'm not looking at HEAD, but at least in the two versions I've got checked out, devm_clk_get() can theoretically return NULL. This feels like a gratuitous change anyway, but in any case it's wrong and could cause wrong behavior. - Steve
Re: [PATCH] ath10k: fix the status check and wrong return
Hi Kalle: 在 2020/8/17 22:26, Kalle Valo 写道: In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't return NULL. Thus use IS_ERR() and PTR_ERR() to validate the returned value instead of IS_ERR_OR_NULL(). Why? What's the benefit of this patch? Or what harm does IS_ERR_OR_NULL() create? Thanks for you reply, the benefit of this patch is simplify the code, because in this function, I don't think the situation of 'devm_clk_get() return NULL' exists. So please think about it, thanks. Tang Bin Signed-off-by: Zhang Shengju Signed-off-by: Tang Bin --- drivers/net/wireless/ath/ath10k/ahb.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c index ed87bc00f..ea669af6a 100644 --- a/drivers/net/wireless/ath/ath10k/ahb.c +++ b/drivers/net/wireless/ath/ath10k/ahb.c @@ -87,24 +87,24 @@ static int ath10k_ahb_clock_init(struct ath10k *ar) dev = _ahb->pdev->dev; ar_ahb->cmd_clk = devm_clk_get(dev, "wifi_wcss_cmd"); - if (IS_ERR_OR_NULL(ar_ahb->cmd_clk)) { + if (IS_ERR(ar_ahb->cmd_clk)) { ath10k_err(ar, "failed to get cmd clk: %ld\n", PTR_ERR(ar_ahb->cmd_clk)); - return ar_ahb->cmd_clk ? PTR_ERR(ar_ahb->cmd_clk) : -ENODEV; + return PTR_ERR(ar_ahb->cmd_clk); } devm_clk_get() can return NULL if CONFIG_HAVE_CLK is disabled: static inline struct clk *devm_clk_get(struct device *dev, const char *id) { return NULL; }
Re: [PATCH] ath10k: fix the status check and wrong return
Tang Bin writes: > In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't > return NULL. Thus use IS_ERR() and PTR_ERR() to validate > the returned value instead of IS_ERR_OR_NULL(). Why? What's the benefit of this patch? Or what harm does IS_ERR_OR_NULL() create? > Signed-off-by: Zhang Shengju > Signed-off-by: Tang Bin > --- > drivers/net/wireless/ath/ath10k/ahb.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/ahb.c > b/drivers/net/wireless/ath/ath10k/ahb.c > index ed87bc00f..ea669af6a 100644 > --- a/drivers/net/wireless/ath/ath10k/ahb.c > +++ b/drivers/net/wireless/ath/ath10k/ahb.c > @@ -87,24 +87,24 @@ static int ath10k_ahb_clock_init(struct ath10k *ar) > dev = _ahb->pdev->dev; > > ar_ahb->cmd_clk = devm_clk_get(dev, "wifi_wcss_cmd"); > - if (IS_ERR_OR_NULL(ar_ahb->cmd_clk)) { > + if (IS_ERR(ar_ahb->cmd_clk)) { > ath10k_err(ar, "failed to get cmd clk: %ld\n", > PTR_ERR(ar_ahb->cmd_clk)); > - return ar_ahb->cmd_clk ? PTR_ERR(ar_ahb->cmd_clk) : -ENODEV; > + return PTR_ERR(ar_ahb->cmd_clk); > } devm_clk_get() can return NULL if CONFIG_HAVE_CLK is disabled: static inline struct clk *devm_clk_get(struct device *dev, const char *id) { return NULL; } -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] ath10k: fix the status check and wrong return
On 14/08/2020, Tang Bin wrote: > In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't > return NULL. Thus use IS_ERR() and PTR_ERR() to validate > the returned value instead of IS_ERR_OR_NULL(). > > Signed-off-by: Zhang Shengju > Signed-off-by: Tang Bin > --- > drivers/net/wireless/ath/ath10k/ahb.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/ahb.c > b/drivers/net/wireless/ath/ath10k/ahb.c > index ed87bc00f..ea669af6a 100644 > --- a/drivers/net/wireless/ath/ath10k/ahb.c > +++ b/drivers/net/wireless/ath/ath10k/ahb.c > @@ -87,24 +87,24 @@ static int ath10k_ahb_clock_init(struct ath10k *ar) > dev = _ahb->pdev->dev; > > ar_ahb->cmd_clk = devm_clk_get(dev, "wifi_wcss_cmd"); > - if (IS_ERR_OR_NULL(ar_ahb->cmd_clk)) { > + if (IS_ERR(ar_ahb->cmd_clk)) { > ath10k_err(ar, "failed to get cmd clk: %ld\n", > PTR_ERR(ar_ahb->cmd_clk)); > - return ar_ahb->cmd_clk ? PTR_ERR(ar_ahb->cmd_clk) : -ENODEV; > + return PTR_ERR(ar_ahb->cmd_clk); > } > > ar_ahb->ref_clk = devm_clk_get(dev, "wifi_wcss_ref"); > - if (IS_ERR_OR_NULL(ar_ahb->ref_clk)) { > + if (IS_ERR(ar_ahb->ref_clk)) { > ath10k_err(ar, "failed to get ref clk: %ld\n", > PTR_ERR(ar_ahb->ref_clk)); > - return ar_ahb->ref_clk ? PTR_ERR(ar_ahb->ref_clk) : -ENODEV; > + return PTR_ERR(ar_ahb->ref_clk); > } > > ar_ahb->rtc_clk = devm_clk_get(dev, "wifi_wcss_rtc"); > - if (IS_ERR_OR_NULL(ar_ahb->rtc_clk)) { > + if (IS_ERR(ar_ahb->rtc_clk)) { > ath10k_err(ar, "failed to get rtc clk: %ld\n", > PTR_ERR(ar_ahb->rtc_clk)); > - return ar_ahb->rtc_clk ? PTR_ERR(ar_ahb->rtc_clk) : -ENODEV; > + return PTR_ERR(ar_ahb->rtc_clk); > } > > return 0; > -- > 2.20.1.windows.1 > > > > Hi You should've include which HW/FW combination you tested this on
[PATCH] ath10k: fix the status check and wrong return
In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't return NULL. Thus use IS_ERR() and PTR_ERR() to validate the returned value instead of IS_ERR_OR_NULL(). Signed-off-by: Zhang Shengju Signed-off-by: Tang Bin --- drivers/net/wireless/ath/ath10k/ahb.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c index ed87bc00f..ea669af6a 100644 --- a/drivers/net/wireless/ath/ath10k/ahb.c +++ b/drivers/net/wireless/ath/ath10k/ahb.c @@ -87,24 +87,24 @@ static int ath10k_ahb_clock_init(struct ath10k *ar) dev = _ahb->pdev->dev; ar_ahb->cmd_clk = devm_clk_get(dev, "wifi_wcss_cmd"); - if (IS_ERR_OR_NULL(ar_ahb->cmd_clk)) { + if (IS_ERR(ar_ahb->cmd_clk)) { ath10k_err(ar, "failed to get cmd clk: %ld\n", PTR_ERR(ar_ahb->cmd_clk)); - return ar_ahb->cmd_clk ? PTR_ERR(ar_ahb->cmd_clk) : -ENODEV; + return PTR_ERR(ar_ahb->cmd_clk); } ar_ahb->ref_clk = devm_clk_get(dev, "wifi_wcss_ref"); - if (IS_ERR_OR_NULL(ar_ahb->ref_clk)) { + if (IS_ERR(ar_ahb->ref_clk)) { ath10k_err(ar, "failed to get ref clk: %ld\n", PTR_ERR(ar_ahb->ref_clk)); - return ar_ahb->ref_clk ? PTR_ERR(ar_ahb->ref_clk) : -ENODEV; + return PTR_ERR(ar_ahb->ref_clk); } ar_ahb->rtc_clk = devm_clk_get(dev, "wifi_wcss_rtc"); - if (IS_ERR_OR_NULL(ar_ahb->rtc_clk)) { + if (IS_ERR(ar_ahb->rtc_clk)) { ath10k_err(ar, "failed to get rtc clk: %ld\n", PTR_ERR(ar_ahb->rtc_clk)); - return ar_ahb->rtc_clk ? PTR_ERR(ar_ahb->rtc_clk) : -ENODEV; + return PTR_ERR(ar_ahb->rtc_clk); } return 0; -- 2.20.1.windows.1