| Message ID | 20200814144844.1920-1-tangbin@cmss.chinamobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ath10k: fix the status check and wrong return | expand |
On 14/08/2020, Tang Bin <tangbin@cmss.chinamobile.com> 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 <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > 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 = &ar_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
Tang Bin <tangbin@cmss.chinamobile.com> 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 <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > 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 = &ar_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; }
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 <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> 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 = &ar_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; > } >
On Mon, Aug 17, 2020 at 6:43 PM Tang Bin <tangbin@cmss.chinamobile.com> 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
Tang Bin <tangbin@cmss.chinamobile.com> 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.
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 = &ar_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;