[PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

The return value about hisi_reset_init() is not correct, fix it.

Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - No changes, just add "Fixes:" tag

v3:
  - Use ERR_CAST(rstc->membase) to fix the sparse warning

v4:
  - No changes

 drivers/clk/hisilicon/clk-hi3519.c      | 4 ++--
 drivers/clk/hisilicon/crg-hi3516cv300.c | 4 ++--
 drivers/clk/hisilicon/crg-hi3798cv200.c | 4 ++--
 drivers/clk/hisilicon/reset.c           | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/hisilicon/clk-hi3519.c b/drivers/clk/hisilicon/clk-hi3519.c
index ad0c7f3..803fa66 100644
--- a/drivers/clk/hisilicon/clk-hi3519.c
+++ b/drivers/clk/hisilicon/clk-hi3519.c
@@ -149,8 +149,8 @@ static int hi3519_clk_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	crg->rstc = hisi_reset_init(pdev);
-	if (!crg->rstc)
-		return -ENOMEM;
+	if (IS_ERR(crg->rstc))
+		return PTR_ERR(crg->rstc);
 
 	crg->clk_data = hi3519_clk_register(pdev);
 	if (IS_ERR(crg->clk_data)) {
diff --git a/drivers/clk/hisilicon/crg-hi3516cv300.c b/drivers/clk/hisilicon/crg-hi3516cv300.c
index 5d4e61c..c2af03d 100644
--- a/drivers/clk/hisilicon/crg-hi3516cv300.c
+++ b/drivers/clk/hisilicon/crg-hi3516cv300.c
@@ -271,8 +271,8 @@ static int hi3516cv300_crg_probe(struct platform_device *pdev)
 		return -ENOENT;
 
 	crg->rstc = hisi_reset_init(pdev);
-	if (!crg->rstc)
-		return -ENOMEM;
+	if (IS_ERR(crg->rstc))
+		return PTR_ERR(crg->rstc);
 
 	crg->clk_data = crg->funcs->register_clks(pdev);
 	if (IS_ERR(crg->clk_data)) {
diff --git a/drivers/clk/hisilicon/crg-hi3798cv200.c b/drivers/clk/hisilicon/crg-hi3798cv200.c
index 08a19ba..66fd6a9 100644
--- a/drivers/clk/hisilicon/crg-hi3798cv200.c
+++ b/drivers/clk/hisilicon/crg-hi3798cv200.c
@@ -354,8 +354,8 @@ static int hi3798cv200_crg_probe(struct platform_device *pdev)
 		return -ENOENT;
 
 	crg->rstc = hisi_reset_init(pdev);
-	if (!crg->rstc)
-		return -ENOMEM;
+	if (IS_ERR(crg->rstc))
+		return PTR_ERR(crg->rstc);
 
 	crg->clk_data = crg->funcs->register_clks(pdev);
 	if (IS_ERR(crg->clk_data)) {
diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
index 93cee17..c733e2e 100644
--- a/drivers/clk/hisilicon/reset.c
+++ b/drivers/clk/hisilicon/reset.c
@@ -93,11 +93,11 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
 
 	rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
 	if (!rstc)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	rstc->membase = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(rstc->membase))
-		return NULL;
+		return ERR_CAST(rstc->membase);
 
 	spin_lock_init(&rstc->lock);
 	rstc->rcdev.owner = THIS_MODULE;
-- 
2.1.0

[PATCH v4 2/2] clk: Allow COMPILE_TEST for subdir hisilicon in Makefile

[Tiezhu Yang]

If CONFIG_ARCH_HISI is not set but COMPILE_TEST is set, some files
in the subdir hisilicon can not be built due to CONFIG_ARCH_HISI
check in drivers/clk/Makefile.

Since the related configs in drivers/clk/hisilicon/Kconfig depend
on ARCH_HISI, so remove CONFIG_ARCH_HISI check for subdir hisilicon
in drivers/clk/Makefile.

At the same time, we should add CONFIG_ARCH_HISI and COMPILE_TEST
(for better compile testing coverage) check for the common files
in drivers/clk/hisilicon/Makefile, otherwise there exists build
failure about undefined reference.

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---

v2:
  - Add CONFIG_ARCH_HISI check for the common files
    to fix the build failure

v3:
  - Add CONFIG_COMPILE_TEST check for the common files
    for better compile testing coverage

v4:
  - Modify the patch subject to reflect the reality

 drivers/clk/Makefile           | 2 +-
 drivers/clk/hisilicon/Makefile | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index f4169cc..81045ec 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -79,7 +79,7 @@ obj-y					+= bcm/
 obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
 obj-$(CONFIG_ARCH_DAVINCI)		+= davinci/
 obj-$(CONFIG_H8300)			+= h8300/
-obj-$(CONFIG_ARCH_HISI)			+= hisilicon/
+obj-y					+= hisilicon/
 obj-y					+= imgtec/
 obj-y					+= imx/
 obj-y					+= ingenic/
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index b2441b9..e58104d 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -3,7 +3,8 @@
 # Hisilicon Clock specific Makefile
 #
 
-obj-y	+= clk.o clkgate-separated.o clkdivider-hi6220.o clk-hisi-phase.o
+obj-$(CONFIG_ARCH_HISI)		+= clk.o clkgate-separated.o clkdivider-hi6220.o clk-hisi-phase.o
+obj-$(CONFIG_COMPILE_TEST)	+= clk.o clkgate-separated.o clkdivider-hi6220.o clk-hisi-phase.o
 
 obj-$(CONFIG_ARCH_HI3xxx)	+= clk-hi3620.o
 obj-$(CONFIG_ARCH_HIP04)	+= clk-hip04.o
-- 
2.1.0

Re: [PATCH v4 2/2] clk: Allow COMPILE_TEST for subdir hisilicon in Makefile

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5818 bytes --]

Hi Tiezhu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Tiezhu-Yang/clk-hisilicon-Use-correct-return-value-about-hisi_reset_init/20200527-233606
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: openrisc-randconfig-r021-20200531 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_fixed_rate':
>> clk.c:(.text+0x158): undefined reference to `clk_register_fixed_rate'
clk.c:(.text+0x158): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_fixed_rate'
or1k-linux-ld: clk.c:(.text+0x220): undefined reference to `clk_unregister_fixed_rate'
clk.c:(.text+0x220): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_fixed_rate'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_fixed_factor':
>> clk.c:(.text+0x2a0): undefined reference to `clk_register_fixed_factor'
clk.c:(.text+0x2a0): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_fixed_factor'
or1k-linux-ld: clk.c:(.text+0x370): undefined reference to `clk_unregister_fixed_factor'
clk.c:(.text+0x370): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_fixed_factor'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_mux':
>> clk.c:(.text+0x434): undefined reference to `clk_register_mux_table'
clk.c:(.text+0x434): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_mux_table'
or1k-linux-ld: clk.c:(.text+0x464): undefined reference to `clk_register_clkdev'
clk.c:(.text+0x464): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_clkdev'
or1k-linux-ld: clk.c:(.text+0x528): undefined reference to `clk_unregister_mux'
clk.c:(.text+0x528): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_mux'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_divider':
>> clk.c:(.text+0x6e8): undefined reference to `clk_register_divider_table'
clk.c:(.text+0x6e8): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_divider_table'
or1k-linux-ld: clk.c:(.text+0x718): undefined reference to `clk_register_clkdev'
clk.c:(.text+0x718): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_clkdev'
or1k-linux-ld: clk.c:(.text+0x7d0): undefined reference to `clk_unregister_divider'
clk.c:(.text+0x7d0): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_divider'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_gate':
>> clk.c:(.text+0x870): undefined reference to `clk_register_gate'
clk.c:(.text+0x870): additional relocation overflows omitted from the output
or1k-linux-ld: clk.c:(.text+0x8a0): undefined reference to `clk_register_clkdev'
or1k-linux-ld: clk.c:(.text+0x960): undefined reference to `clk_unregister_gate'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_gate_sep':
>> clk.c:(.text+0xa4c): undefined reference to `clk_register_clkdev'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_init':
clk.c:(.text+0xb5c): undefined reference to `of_clk_src_onecell_get'
or1k-linux-ld: clk.c:(.text+0xb6c): undefined reference to `of_clk_src_onecell_get'
or1k-linux-ld: clk.c:(.text+0xb70): undefined reference to `of_clk_add_provider'
or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hi6220_clk_register_divider':
>> clk.c:(.init.text+0xe0): undefined reference to `clk_register_clkdev'
or1k-linux-ld: drivers/clk/hisilicon/clkgate-separated.o: in function `hisi_register_clkgate_sep':
>> clkgate-separated.c:(.text+0x2b4): undefined reference to `clk_register'
or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_set_rate':
>> clkdivider-hi6220.c:(.text+0x30): undefined reference to `divider_get_val'
or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_round_rate':
>> clkdivider-hi6220.c:(.text+0x15c): undefined reference to `clk_hw_get_parent'
or1k-linux-ld: clkdivider-hi6220.c:(.text+0x180): undefined reference to `divider_round_rate_parent'
or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_recalc_rate':
>> clkdivider-hi6220.c:(.text+0x214): undefined reference to `divider_recalc_rate'
or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_register_clkdiv':
>> clkdivider-hi6220.c:(.text+0x380): undefined reference to `clk_register'
or1k-linux-ld: drivers/clk/hisilicon/clk-hisi-phase.o: in function `clk_register_hisi_phase':
>> clk-hisi-phase.c:(.text+0x2f0): undefined reference to `devm_clk_register'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37074 bytes --]

Re: [PATCH v4 2/2] clk: Allow COMPILE_TEST for subdir hisilicon in Makefile

[Tiezhu Yang]

On 06/01/2020 05:05 AM, kbuild test robot wrote:
> Hi Tiezhu,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on clk/clk-next]
> [also build test ERROR on v5.7-rc7 next-20200529]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Tiezhu-Yang/clk-hisilicon-Use-correct-return-value-about-hisi_reset_init/20200527-233606
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
> config: openrisc-randconfig-r021-20200531 (attached as .config)
> compiler: or1k-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>

Hi,

Thanks for your report, please ignore this patch.

>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_fixed_rate':
>>> clk.c:(.text+0x158): undefined reference to `clk_register_fixed_rate'
> clk.c:(.text+0x158): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_fixed_rate'
> or1k-linux-ld: clk.c:(.text+0x220): undefined reference to `clk_unregister_fixed_rate'
> clk.c:(.text+0x220): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_fixed_rate'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_fixed_factor':
>>> clk.c:(.text+0x2a0): undefined reference to `clk_register_fixed_factor'
> clk.c:(.text+0x2a0): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_fixed_factor'
> or1k-linux-ld: clk.c:(.text+0x370): undefined reference to `clk_unregister_fixed_factor'
> clk.c:(.text+0x370): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_fixed_factor'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_mux':
>>> clk.c:(.text+0x434): undefined reference to `clk_register_mux_table'
> clk.c:(.text+0x434): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_mux_table'
> or1k-linux-ld: clk.c:(.text+0x464): undefined reference to `clk_register_clkdev'
> clk.c:(.text+0x464): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_clkdev'
> or1k-linux-ld: clk.c:(.text+0x528): undefined reference to `clk_unregister_mux'
> clk.c:(.text+0x528): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_mux'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_divider':
>>> clk.c:(.text+0x6e8): undefined reference to `clk_register_divider_table'
> clk.c:(.text+0x6e8): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_divider_table'
> or1k-linux-ld: clk.c:(.text+0x718): undefined reference to `clk_register_clkdev'
> clk.c:(.text+0x718): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_register_clkdev'
> or1k-linux-ld: clk.c:(.text+0x7d0): undefined reference to `clk_unregister_divider'
> clk.c:(.text+0x7d0): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `clk_unregister_divider'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_gate':
>>> clk.c:(.text+0x870): undefined reference to `clk_register_gate'
> clk.c:(.text+0x870): additional relocation overflows omitted from the output
> or1k-linux-ld: clk.c:(.text+0x8a0): undefined reference to `clk_register_clkdev'
> or1k-linux-ld: clk.c:(.text+0x960): undefined reference to `clk_unregister_gate'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_register_gate_sep':
>>> clk.c:(.text+0xa4c): undefined reference to `clk_register_clkdev'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hisi_clk_init':
> clk.c:(.text+0xb5c): undefined reference to `of_clk_src_onecell_get'
> or1k-linux-ld: clk.c:(.text+0xb6c): undefined reference to `of_clk_src_onecell_get'
> or1k-linux-ld: clk.c:(.text+0xb70): undefined reference to `of_clk_add_provider'
> or1k-linux-ld: drivers/clk/hisilicon/clk.o: in function `hi6220_clk_register_divider':
>>> clk.c:(.init.text+0xe0): undefined reference to `clk_register_clkdev'
> or1k-linux-ld: drivers/clk/hisilicon/clkgate-separated.o: in function `hisi_register_clkgate_sep':
>>> clkgate-separated.c:(.text+0x2b4): undefined reference to `clk_register'
> or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_set_rate':
>>> clkdivider-hi6220.c:(.text+0x30): undefined reference to `divider_get_val'
> or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_round_rate':
>>> clkdivider-hi6220.c:(.text+0x15c): undefined reference to `clk_hw_get_parent'
> or1k-linux-ld: clkdivider-hi6220.c:(.text+0x180): undefined reference to `divider_round_rate_parent'
> or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_clkdiv_recalc_rate':
>>> clkdivider-hi6220.c:(.text+0x214): undefined reference to `divider_recalc_rate'
> or1k-linux-ld: drivers/clk/hisilicon/clkdivider-hi6220.o: in function `hi6220_register_clkdiv':
>>> clkdivider-hi6220.c:(.text+0x380): undefined reference to `clk_register'
> or1k-linux-ld: drivers/clk/hisilicon/clk-hisi-phase.o: in function `clk_register_hisi_phase':
>>> clk-hisi-phase.c:(.text+0x2f0): undefined reference to `devm_clk_register'
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-27 07:39:21)
> The return value about hisi_reset_init() is not correct, fix it.
> 
> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")

hisi_reset_init() returns NULL on error in that commit. This patch
doesn't make sense.

> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

On 05/28/2020 03:06 AM, Stephen Boyd wrote:
> Quoting Tiezhu Yang (2020-05-27 07:39:21)
>> The return value about hisi_reset_init() is not correct, fix it.
>>
>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
> hisi_reset_init() returns NULL on error in that commit. This patch
> doesn't make sense.

Hi Stephen,

The initial aim of this patch is to use correct return value about
hisi_reset_init(), maybe NULL is OK, but the return value in this
patch is more accurate.

In the current code, it always returns -ENOMEM when call function
hisi_reset_init() failed which is not proper, because in the function
hisi_reset_init(), devm_platform_ioremap_resource() may returns -EINVAL,
-EBUSY or -ENOMEM if failed.

devm_platform_ioremap_resource()
         devm_ioremap_resource()
                 __devm_ioremap_resource()


static void __iomem *
__devm_ioremap_resource(struct device *dev, const struct resource *res,
             enum devm_ioremap_type type)
{
     resource_size_t size;
     void __iomem *dest_ptr;

     BUG_ON(!dev);

     if (!res || resource_type(res) != IORESOURCE_MEM) {
         dev_err(dev, "invalid resource\n");
         return IOMEM_ERR_PTR(-EINVAL);
     }

     size = resource_size(res);

     if (!devm_request_mem_region(dev, res->start, size, dev_name(dev))) {
         dev_err(dev, "can't request region for resource %pR\n", res);
         return IOMEM_ERR_PTR(-EBUSY);
     }

     dest_ptr = __devm_ioremap(dev, res->start, size, type);
     if (!dest_ptr) {
         dev_err(dev, "ioremap failed for resource %pR\n", res);
         devm_release_mem_region(dev, res->start, size);
         dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
     }

     return dest_ptr;
}

Thanks,
Tiezhu Yang

>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-27 19:27:42)
> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
> > Quoting Tiezhu Yang (2020-05-27 07:39:21)
> >> The return value about hisi_reset_init() is not correct, fix it.
> >>
> >> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
> > hisi_reset_init() returns NULL on error in that commit. This patch
> > doesn't make sense.
> 
> Hi Stephen,
> 
> The initial aim of this patch is to use correct return value about
> hisi_reset_init(), maybe NULL is OK, but the return value in this
> patch is more accurate.

The implementation of hisi_reset_init() that I see is this:


	struct hisi_reset_controller *rstc;

	rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
	if (!rstc)
		return NULL;

	rstc->membase = devm_platform_ioremap_resource(pdev, 0);
	if (IS_ERR(rstc->membase))
		return NULL;

	spin_lock_init(&rstc->lock);
	rstc->rcdev.owner = THIS_MODULE;
	rstc->rcdev.ops = &hisi_reset_ops;
	rstc->rcdev.of_node = pdev->dev.of_node;
	rstc->rcdev.of_reset_n_cells = 2;
	rstc->rcdev.of_xlate = hisi_reset_of_xlate;
	reset_controller_register(&rstc->rcdev);

	return rstc;

And that returns NULL on an error and a valid pointer on success.
Changing the code to check the return value of hisi_reset_init() for an
error pointer is simply wrong without updating hisi_reset_init() to
return an error pointer on error. Where is the patch that changes
hisi_reset_init() to return an error pointer?

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

On 05/29/2020 07:15 AM, Stephen Boyd wrote:
> Quoting Tiezhu Yang (2020-05-27 19:27:42)
>> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
>>> Quoting Tiezhu Yang (2020-05-27 07:39:21)
>>>> The return value about hisi_reset_init() is not correct, fix it.
>>>>
>>>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
>>> hisi_reset_init() returns NULL on error in that commit. This patch
>>> doesn't make sense.
>> Hi Stephen,
>>
>> The initial aim of this patch is to use correct return value about
>> hisi_reset_init(), maybe NULL is OK, but the return value in this
>> patch is more accurate.
> The implementation of hisi_reset_init() that I see is this:
>
>
> 	struct hisi_reset_controller *rstc;
>
> 	rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
> 	if (!rstc)
> 		return NULL;
>
> 	rstc->membase = devm_platform_ioremap_resource(pdev, 0);
> 	if (IS_ERR(rstc->membase))
> 		return NULL;
>
> 	spin_lock_init(&rstc->lock);
> 	rstc->rcdev.owner = THIS_MODULE;
> 	rstc->rcdev.ops = &hisi_reset_ops;
> 	rstc->rcdev.of_node = pdev->dev.of_node;
> 	rstc->rcdev.of_reset_n_cells = 2;
> 	rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> 	reset_controller_register(&rstc->rcdev);
>
> 	return rstc;
>
> And that returns NULL on an error and a valid pointer on success.
> Changing the code to check the return value of hisi_reset_init() for an
> error pointer is simply wrong without updating hisi_reset_init() to
> return an error pointer on error. Where is the patch that changes
> hisi_reset_init() to return an error pointer?

Hi Stephen,

Do you mean the following changes?

diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
index 93cee17..c733e2e 100644
--- a/drivers/clk/hisilicon/reset.c
+++ b/drivers/clk/hisilicon/reset.c
@@ -93,11 +93,11 @@  struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
  
  	rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
  	if (!rstc)
- return NULL;
+ return ERR_PTR(-ENOMEM);
  
  	rstc->membase = devm_platform_ioremap_resource(pdev, 0);
  	if (IS_ERR(rstc->membase))
- return NULL;
+ return ERR_CAST(rstc->membase);
  
  	spin_lock_init(&rstc->lock);
  	rstc->rcdev.owner = THIS_MODULE;


devm_platform_ioremap_resource()
          devm_ioremap_resource()
                  __devm_ioremap_resource()

By the way, we can see the comment of devm_ioremap_resource():

Usage example:

          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
          base = devm_ioremap_resource(&pdev->dev, res);
          if (IS_ERR(base))
                  return PTR_ERR(base);

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-28 19:03:54)
> On 05/29/2020 07:15 AM, Stephen Boyd wrote:
> > Quoting Tiezhu Yang (2020-05-27 19:27:42)
> >> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
> >>> Quoting Tiezhu Yang (2020-05-27 07:39:21)
> >>>> The return value about hisi_reset_init() is not correct, fix it.
> >>>>
> >>>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
> >>> hisi_reset_init() returns NULL on error in that commit. This patch
> >>> doesn't make sense.
> >> Hi Stephen,
> >>
> >> The initial aim of this patch is to use correct return value about
> >> hisi_reset_init(), maybe NULL is OK, but the return value in this
> >> patch is more accurate.
> > The implementation of hisi_reset_init() that I see is this:
> >
> >
> >       struct hisi_reset_controller *rstc;
> >
> >       rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
> >       if (!rstc)
> >               return NULL;
> >
> >       rstc->membase = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(rstc->membase))
> >               return NULL;
> >
> >       spin_lock_init(&rstc->lock);
> >       rstc->rcdev.owner = THIS_MODULE;
> >       rstc->rcdev.ops = &hisi_reset_ops;
> >       rstc->rcdev.of_node = pdev->dev.of_node;
> >       rstc->rcdev.of_reset_n_cells = 2;
> >       rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> >       reset_controller_register(&rstc->rcdev);
> >
> >       return rstc;
> >
> > And that returns NULL on an error and a valid pointer on success.
> > Changing the code to check the return value of hisi_reset_init() for an
> > error pointer is simply wrong without updating hisi_reset_init() to
> > return an error pointer on error. Where is the patch that changes
> > hisi_reset_init() to return an error pointer?
> 
> Hi Stephen,
> 
> Do you mean the following changes?

Yes where is this change?

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

On 05/29/2020 11:58 AM, Stephen Boyd wrote:
> Quoting Tiezhu Yang (2020-05-28 19:03:54)
>> On 05/29/2020 07:15 AM, Stephen Boyd wrote:
>>> Quoting Tiezhu Yang (2020-05-27 19:27:42)
>>>> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
>>>>> Quoting Tiezhu Yang (2020-05-27 07:39:21)
>>>>>> The return value about hisi_reset_init() is not correct, fix it.
>>>>>>
>>>>>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
>>>>> hisi_reset_init() returns NULL on error in that commit. This patch
>>>>> doesn't make sense.
>>>> Hi Stephen,
>>>>
>>>> The initial aim of this patch is to use correct return value about
>>>> hisi_reset_init(), maybe NULL is OK, but the return value in this
>>>> patch is more accurate.
>>> The implementation of hisi_reset_init() that I see is this:
>>>
>>>
>>>        struct hisi_reset_controller *rstc;
>>>
>>>        rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
>>>        if (!rstc)
>>>                return NULL;
>>>
>>>        rstc->membase = devm_platform_ioremap_resource(pdev, 0);
>>>        if (IS_ERR(rstc->membase))
>>>                return NULL;
>>>
>>>        spin_lock_init(&rstc->lock);
>>>        rstc->rcdev.owner = THIS_MODULE;
>>>        rstc->rcdev.ops = &hisi_reset_ops;
>>>        rstc->rcdev.of_node = pdev->dev.of_node;
>>>        rstc->rcdev.of_reset_n_cells = 2;
>>>        rstc->rcdev.of_xlate = hisi_reset_of_xlate;
>>>        reset_controller_register(&rstc->rcdev);
>>>
>>>        return rstc;
>>>
>>> And that returns NULL on an error and a valid pointer on success.
>>> Changing the code to check the return value of hisi_reset_init() for an
>>> error pointer is simply wrong without updating hisi_reset_init() to
>>> return an error pointer on error. Where is the patch that changes
>>> hisi_reset_init() to return an error pointer?
>> Hi Stephen,
>>
>> Do you mean the following changes?
> Yes where is this change?

ERR_PTR(-ENOMEM) and ERR_CAST(rstc->membase)


  	rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
  	if (!rstc)
- return NULL;
+ return ERR_PTR(-ENOMEM);
  
  	rstc->membase = devm_platform_ioremap_resource(pdev, 0);
  	if (IS_ERR(rstc->membase))
- return NULL;
+ return ERR_CAST(rstc->membase);

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-28 21:02:05)
> On 05/29/2020 11:58 AM, Stephen Boyd wrote:
> > Quoting Tiezhu Yang (2020-05-28 19:03:54)
> >> On 05/29/2020 07:15 AM, Stephen Boyd wrote:
> >>> Quoting Tiezhu Yang (2020-05-27 19:27:42)
> >>>> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
> >>>>> Quoting Tiezhu Yang (2020-05-27 07:39:21)
> >>>>>> The return value about hisi_reset_init() is not correct, fix it.
> >>>>>>
> >>>>>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
> >>>>> hisi_reset_init() returns NULL on error in that commit. This patch
> >>>>> doesn't make sense.
> >>>> Hi Stephen,
> >>>>
> >>>> The initial aim of this patch is to use correct return value about
> >>>> hisi_reset_init(), maybe NULL is OK, but the return value in this
> >>>> patch is more accurate.
> >>> The implementation of hisi_reset_init() that I see is this:
> >>>
> >>>
> >>>        struct hisi_reset_controller *rstc;
> >>>
> >>>        rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
> >>>        if (!rstc)
> >>>                return NULL;
> >>>
> >>>        rstc->membase = devm_platform_ioremap_resource(pdev, 0);
> >>>        if (IS_ERR(rstc->membase))
> >>>                return NULL;
> >>>
> >>>        spin_lock_init(&rstc->lock);
> >>>        rstc->rcdev.owner = THIS_MODULE;
> >>>        rstc->rcdev.ops = &hisi_reset_ops;
> >>>        rstc->rcdev.of_node = pdev->dev.of_node;
> >>>        rstc->rcdev.of_reset_n_cells = 2;
> >>>        rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> >>>        reset_controller_register(&rstc->rcdev);
> >>>
> >>>        return rstc;
> >>>
> >>> And that returns NULL on an error and a valid pointer on success.
> >>> Changing the code to check the return value of hisi_reset_init() for an
> >>> error pointer is simply wrong without updating hisi_reset_init() to
> >>> return an error pointer on error. Where is the patch that changes
> >>> hisi_reset_init() to return an error pointer?
> >> Hi Stephen,
> >>
> >> Do you mean the following changes?
> > Yes where is this change?
> 
> ERR_PTR(-ENOMEM) and ERR_CAST(rstc->membase)
> 

I think you didn't understand my question. I'm asking where is this
patch applied to the kernel and what commit is it? I don't see it in the
clk tree.

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

On 05/29/2020 12:31 PM, Stephen Boyd wrote:
> Quoting Tiezhu Yang (2020-05-28 21:02:05)
>> On 05/29/2020 11:58 AM, Stephen Boyd wrote:
>>> Quoting Tiezhu Yang (2020-05-28 19:03:54)
>>>> On 05/29/2020 07:15 AM, Stephen Boyd wrote:
>>>>> Quoting Tiezhu Yang (2020-05-27 19:27:42)
>>>>>> On 05/28/2020 03:06 AM, Stephen Boyd wrote:
>>>>>>> Quoting Tiezhu Yang (2020-05-27 07:39:21)
>>>>>>>> The return value about hisi_reset_init() is not correct, fix it.
>>>>>>>>
>>>>>>>> Fixes: e9a2310fb689 ("reset: hisilicon: fix potential NULL pointer dereference")
>>>>>>> hisi_reset_init() returns NULL on error in that commit. This patch
>>>>>>> doesn't make sense.
>>>>>> Hi Stephen,
>>>>>>
>>>>>> The initial aim of this patch is to use correct return value about
>>>>>> hisi_reset_init(), maybe NULL is OK, but the return value in this
>>>>>> patch is more accurate.
>>>>> The implementation of hisi_reset_init() that I see is this:
>>>>>
>>>>>
>>>>>         struct hisi_reset_controller *rstc;
>>>>>
>>>>>         rstc = devm_kmalloc(&pdev->dev, sizeof(*rstc), GFP_KERNEL);
>>>>>         if (!rstc)
>>>>>                 return NULL;
>>>>>
>>>>>         rstc->membase = devm_platform_ioremap_resource(pdev, 0);
>>>>>         if (IS_ERR(rstc->membase))
>>>>>                 return NULL;
>>>>>
>>>>>         spin_lock_init(&rstc->lock);
>>>>>         rstc->rcdev.owner = THIS_MODULE;
>>>>>         rstc->rcdev.ops = &hisi_reset_ops;
>>>>>         rstc->rcdev.of_node = pdev->dev.of_node;
>>>>>         rstc->rcdev.of_reset_n_cells = 2;
>>>>>         rstc->rcdev.of_xlate = hisi_reset_of_xlate;
>>>>>         reset_controller_register(&rstc->rcdev);
>>>>>
>>>>>         return rstc;
>>>>>
>>>>> And that returns NULL on an error and a valid pointer on success.
>>>>> Changing the code to check the return value of hisi_reset_init() for an
>>>>> error pointer is simply wrong without updating hisi_reset_init() to
>>>>> return an error pointer on error. Where is the patch that changes
>>>>> hisi_reset_init() to return an error pointer?
>>>> Hi Stephen,
>>>>
>>>> Do you mean the following changes?
>>> Yes where is this change?
>> ERR_PTR(-ENOMEM) and ERR_CAST(rstc->membase)
>>
> I think you didn't understand my question. I'm asking where is this
> patch applied to the kernel and what commit is it? I don't see it in the
> clk tree.

Sorry for that, actually I do not quite understand what you mean.

In my opinion, after the following commit,  when devm_ioremap_resource()
is called in hisi_reset_init(), hisi_reset_init() still returns NULL and 
it only returns
-ENOMEM when call hisi_reset_init() failed, I think it may returns 
-EINVAL, -EBUSY
or -ENOMEM if failed, this is what I want to fix.

"reset: hisilicon: fix potential NULL pointer dereference"
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/drivers/clk/hisilicon/reset.c?h=clk-next&id=e9a2310fb689151166df7fd9971093362d34bd79

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-28 23:44:20)
> On 05/29/2020 12:31 PM, Stephen Boyd wrote:
> > Quoting Tiezhu Yang (2020-05-28 21:02:05)
> > I think you didn't understand my question. I'm asking where is this
> > patch applied to the kernel and what commit is it? I don't see it in the
> > clk tree.
> 
> Sorry for that, actually I do not quite understand what you mean.
> 
> In my opinion, after the following commit,  when devm_ioremap_resource()
> is called in hisi_reset_init(), hisi_reset_init() still returns NULL and 
> it only returns
> -ENOMEM when call hisi_reset_init() failed, I think it may returns 
> -EINVAL, -EBUSY
> or -ENOMEM if failed, this is what I want to fix.
> 
> "reset: hisilicon: fix potential NULL pointer dereference"
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/drivers/clk/hisilicon/reset.c?h=clk-next&id=e9a2310fb689151166df7fd9971093362d34bd79
> 

This commit doesn't change the value that is returned by
hisi_reset_init() on an error. It still returns NULL when an error
happens.

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Tiezhu Yang]

On 05/29/2020 05:36 PM, Stephen Boyd wrote:
> Quoting Tiezhu Yang (2020-05-28 23:44:20)
>> On 05/29/2020 12:31 PM, Stephen Boyd wrote:
>>> Quoting Tiezhu Yang (2020-05-28 21:02:05)
>>> I think you didn't understand my question. I'm asking where is this
>>> patch applied to the kernel and what commit is it? I don't see it in the
>>> clk tree.
>> Sorry for that, actually I do not quite understand what you mean.
>>
>> In my opinion, after the following commit,  when devm_ioremap_resource()
>> is called in hisi_reset_init(), hisi_reset_init() still returns NULL and
>> it only returns
>> -ENOMEM when call hisi_reset_init() failed, I think it may returns
>> -EINVAL, -EBUSY
>> or -ENOMEM if failed, this is what I want to fix.
>>
>> "reset: hisilicon: fix potential NULL pointer dereference"
>> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/drivers/clk/hisilicon/reset.c?h=clk-next&id=e9a2310fb689151166df7fd9971093362d34bd79
>>
> This commit doesn't change the value that is returned by
> hisi_reset_init() on an error. It still returns NULL when an error
> happens.

Yes, I agree, but after this commit e9a2310fb689 ("reset:
hisilicon: fix potential NULL pointer dereference"), the
return value of hisi_reset_init() is not so correct because
it replaces devm_ioremap() with devm_ioremap_resource().

Do you think the code of this patch is OK but the "Fixes:" commit
is not accurate? If so, can we remove the "Fixes:" tag?

Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init()

[Stephen Boyd]

Quoting Tiezhu Yang (2020-05-29 03:20:11)
> On 05/29/2020 05:36 PM, Stephen Boyd wrote:
> > Quoting Tiezhu Yang (2020-05-28 23:44:20)
> >> On 05/29/2020 12:31 PM, Stephen Boyd wrote:
> >>> Quoting Tiezhu Yang (2020-05-28 21:02:05)
> >>> I think you didn't understand my question. I'm asking where is this
> >>> patch applied to the kernel and what commit is it? I don't see it in the
> >>> clk tree.
> >> Sorry for that, actually I do not quite understand what you mean.
> >>
> >> In my opinion, after the following commit,  when devm_ioremap_resource()
> >> is called in hisi_reset_init(), hisi_reset_init() still returns NULL and
> >> it only returns
> >> -ENOMEM when call hisi_reset_init() failed, I think it may returns
> >> -EINVAL, -EBUSY
> >> or -ENOMEM if failed, this is what I want to fix.
> >>
> >> "reset: hisilicon: fix potential NULL pointer dereference"
> >> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/drivers/clk/hisilicon/reset.c?h=clk-next&id=e9a2310fb689151166df7fd9971093362d34bd79
> >>
> > This commit doesn't change the value that is returned by
> > hisi_reset_init() on an error. It still returns NULL when an error
> > happens.
> 
> Yes, I agree, but after this commit e9a2310fb689 ("reset:
> hisilicon: fix potential NULL pointer dereference"), the
> return value of hisi_reset_init() is not so correct because
> it replaces devm_ioremap() with devm_ioremap_resource().

Where does the return value of hisi_reset_init() change in that commit?
The usage of devm_ioremap_resource() vs. devm_ioremap() doesn't change
the return value of hisi_reset_init().

> 
> Do you think the code of this patch is OK but the "Fixes:" commit
> is not accurate? If so, can we remove the "Fixes:" tag?
> 

No. The patch is incorrect and the Fixes tag is incorrect.