Messages in this thread | | | Subject | Re: [PATCH v4 1/2] clk: hisilicon: Use correct return value about hisi_reset_init() | From | Tiezhu Yang <> | Date | Fri, 29 May 2020 14:44:20 +0800 |
| |
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
| |