Re: [PATCH] powerpc/interrupt: Put braces around empty body in an 'if' statement
On 6/18/22 20:11, Souptick Joarder wrote:
> From: "Souptick Joarder (HPE)"
>
> Kernel test robot throws warning ->
>
> arch/powerpc/kernel/interrupt.c:
> In function 'interrupt_exit_kernel_prepare':
>
>>> arch/powerpc/kernel/interrupt.c:542:55: warning: suggest
> braces around empty body in an 'if' statement [-Wempty-body]
> 542 | CT_WARN_ON(ct_state() == CONTEXT_USER);
That must be when CONFIG_CONTEXT_TRACKING_USER is not set/enabled.
Can you confirm that?
Then the preferable fix would be in :
change
#define CT_WARN_ON(cond)
to either an empty do-while loop or a static inline function.
(adding Frederic to Cc:)
>
> Fix it by adding braces.
>
> Reported-by: Kernel test robot
> Signed-off-by: Souptick Joarder (HPE)
> ---
> arch/powerpc/kernel/interrupt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 784ea3289c84..b8a918bab48f 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -538,8 +538,9 @@ notrace unsigned long
> interrupt_exit_kernel_prepare(struct pt_regs *regs)
>* CT_WARN_ON comes here via program_check_exception,
>* so avoid recursion.
>*/
> - if (TRAP(regs) != INTERRUPT_PROGRAM)
> + if (TRAP(regs) != INTERRUPT_PROGRAM) {
> CT_WARN_ON(ct_state() == CONTEXT_USER);
> + }
>
> kuap = kuap_get_and_assert_locked();
>
--
~Randy
[PATCH] powerpc/prom_init: Add memset as valid external symbol if CONFIG_KASAN=y
If CONFIG_KASAN=y, powerpc:allmodconfig fails to build with the following
error.
Error: External symbol 'memset' referenced from prom_init.c
The problem was introduced with commit 41b7a347bf14 ("powerpc: Book3S
64-bit outline-only KASAN support"). So far, with CONFIG_KASAN=y, only
__memset was accepted as valid external symbol in prom_init_check.sh.
Add memset as well to fix the problem.
Fixes: 41b7a347bf14 ("powerpc: Book3S 64-bit outline-only KASAN support")
Cc: Michael Ellerman
Cc: Daniel Axtens
Signed-off-by: Guenter Roeck
---
arch/powerpc/kernel/prom_init_check.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init_check.sh
b/arch/powerpc/kernel/prom_init_check.sh
index b183ab9c5107..787142b5dd26 100644
--- a/arch/powerpc/kernel/prom_init_check.sh
+++ b/arch/powerpc/kernel/prom_init_check.sh
@@ -16,7 +16,7 @@
grep "^CONFIG_KASAN=y$" .config >/dev/null
if [ $? -eq 0 ]
then
- MEM_FUNCS="__memcpy __memset"
+ MEM_FUNCS="__memcpy __memset memset"
else
MEM_FUNCS="memcpy memset"
fi
--
2.35.1
[PATCH] powerpc/interrupt: Put braces around empty body in an 'if' statement
From: "Souptick Joarder (HPE)"
Kernel test robot throws warning ->
arch/powerpc/kernel/interrupt.c:
In function 'interrupt_exit_kernel_prepare':
>> arch/powerpc/kernel/interrupt.c:542:55: warning: suggest
braces around empty body in an 'if' statement [-Wempty-body]
542 | CT_WARN_ON(ct_state() == CONTEXT_USER);
Fix it by adding braces.
Reported-by: Kernel test robot
Signed-off-by: Souptick Joarder (HPE)
---
arch/powerpc/kernel/interrupt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index 784ea3289c84..b8a918bab48f 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -538,8 +538,9 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct
pt_regs *regs)
* CT_WARN_ON comes here via program_check_exception,
* so avoid recursion.
*/
- if (TRAP(regs) != INTERRUPT_PROGRAM)
+ if (TRAP(regs) != INTERRUPT_PROGRAM) {
CT_WARN_ON(ct_state() == CONTEXT_USER);
+ }
kuap = kuap_get_and_assert_locked();
--
2.25.1
[powerpc:fixes-test] BUILD SUCCESS 7bc08056a6dabc3a1442216daf527edf61ac24b6
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes-test branch HEAD: 7bc08056a6dabc3a1442216daf527edf61ac24b6 powerpc/rtas: Allow ibm,platform-dump RTAS call with null buffer address elapsed time: 723m configs tested: 114 configs skipped: 94 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm64 defconfig arm64allyesconfig arm allmodconfig arm defconfig arm allyesconfig i386 randconfig-c001 s390 zfcpdump_defconfig sh se7751_defconfig shmigor_defconfig mipsgpr_defconfig arm axm55xx_defconfig powerpc storcenter_defconfig mips xway_defconfig openrisc or1klitex_defconfig powerpc ps3_defconfig sh r7780mp_defconfig sh sh2007_defconfig arm assabet_defconfig alphaalldefconfig arm pxa910_defconfig armmps2_defconfig pariscgeneric-64bit_defconfig powerpc wii_defconfig sh ul2_defconfig nios2alldefconfig arm sunxi_defconfig sh r7785rp_defconfig mips tb0226_defconfig armqcom_defconfig arm jornada720_defconfig m68kmvme147_defconfig sh rts7751r2dplus_defconfig powerpc maple_defconfig mips decstation_defconfig armtrizeps4_defconfig powerpc iss476-smp_defconfig xtensa cadence_csp_defconfig powerpcwarp_defconfig xtensa audio_kc705_defconfig m68k sun3_defconfig sh alldefconfig openriscdefconfig arc axs103_smp_defconfig arcnsimosci_defconfig sh shx3_defconfig csky alldefconfig shsh7763rdp_defconfig m68k m5249evb_defconfig m68k alldefconfig ia64zx1_defconfig ia64defconfig riscv allnoconfig m68k allyesconfig m68k allmodconfig m68kdefconfig cskydefconfig nios2allyesconfig alpha defconfig alphaallyesconfig xtensa allyesconfig arc defconfig sh allmodconfig s390defconfig s390 allmodconfig parisc defconfig parisc64defconfig parisc allyesconfig s390 allyesconfig mips allyesconfig mips allmodconfig powerpc allyesconfig powerpc allnoconfig powerpc allmodconfig i386 randconfig-a012 i386 randconfig-a014 i386 randconfig-a016 x86_64randconfig-a006 x86_64randconfig-a004 x86_64randconfig-a002 riscv defconfig riscvnommu_virt_defconfig riscv rv32_defconfig riscvnommu_k210_defconfig riscvallmodconfig riscvallyesconfig x86_64rhel-8.3-kselftests x86_64 kexec x86_64 defconfig x86_64 allyesconfig x86_64 rhel-8.3 x86_64 rhel-8.3-func x86_64 rhel-8.3-syz x86_64 rhel-8.3-kunit clang tested configs: arm pcm027_defconfig powerpcgamecube_defconfig arm spear13xx_defconfig powerpc tqm8560_defconfig mips rbtx49xx_defconfig powerpc mpc8272_ads_defconfig arm
Re: [PATCH] tty: serial: Fix refcount leak bug in ucc_uart.c
On Sat, Jun 18, 2022 at 1:09 AM Liang He wrote: > > In soc_info(), of_find_node_by_type() will return a node pointer > with refcount incremented. We should use of_node_put() when it is > not used anymore. > > Signed-off-by: Liang He Acked-by: Timur Tabi
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
At 2022-06-18 16:48:26, "Christophe Leroy" wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>>
>>
>>
>>
>>
>> 在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>>>
>>>
>>> Le 17/06/2022 à 13:26, Liang He a écrit :
In add_pcspkr(), it is better to call of_node_put() after the
'if(!np)' check.
>>>
>>> Why is it better ?
>>>
>>>
>>>
>>> /**
>>> * of_node_put() - Decrement refcount of a node
>>> * @node: Node to dec refcount, NULL is supported to simplify writing of
>>> * callers
>>> */
>>> void of_node_put(struct device_node *node)
>>> {
>>> if (node)
>>> kobject_put(>kobj);
>>> }
>>> EXPORT_SYMBOL(of_node_put);
>>>
>>>
>>>
>>> Christophe
>>
>> Hi, Christophe.
>>
>> Thanks for your reply and I want to have a discussion.
>>
>> In my thought, xxx_put(pointer)'s semantic usually means
>> this reference has been used done and will not be used
>> anymore. Is this semantic more reasonable, right?
>>
>> Besides, if the np is NULL, we can just return and save a cpu
>> time for the xxx_put() call.
>>
>> Otherwise, I prefer to call it 'use(check)-after-put'.
>>
>> In fact, I have meet many other 'use(check)-after-put' instances
>> after I send this patch-commit, so I am waiting for this
>> discussion.
>>
>> This is just my thought, it may be wrong.
>>
>> Anyway, thanks for your reply.
>
>Well in principle you are right, in an ideal world it should be like
>that. However, you have to wonder if it is worth the churn. The CPU
>cycle argument is valid only if that function is used in a hot path. But
>as we are talking about error handling, it can't be a hot path.
>
Thanks very much for this valuable lesson.
>Taking into account the comment associated of of_node_put : "NULL is
>supported to simplify writing of callers", it means that usage is valid,
>just like it is with function kfree() after a kmalloc().
>
>So in a new developpement, or when doing real modifications to a driver,
>that kind of change can be done ideally. However for drivers that have
>been there for years without any change, ask yourself if it is worth the
>churn. You spend time on it, you require other people to spend time on
>it for reviewing and applying your patches and during that time they
>don't do other things that could have been more usefull.
>
Thanks for you advice, I will keep it in my mind before I send a new patch.
>So unless this change is part of a more global patch, I think it is not
>worth the effort.
>
>By the way, also for all your other patches, I think you should start
>doing all the changes locally on your side, and when you are finished
>try to group things together in bigger patches per area instead of
>sending one by one. I see you have already started doing that for
>opal/powernv for instance, but there are still individual powernv/opal
>in the queue. I think you should group all together in a single patch.
>And same for other areas, please try to minimise the number of patches.
>We don't link huge bombs that modify all the kernel at once, but you can
>group things together, one patch for powerpc core parts, one patch for
>each platform in arch/powerpc/platforms/ etc ...
>
You are right and I will follow this principle in future patching work.
While It is too exciting for me to begin the patching work , I should have
grouped my patches.
>
>Christophe
Thanks again, Christophe.
Liang
Re: [PATCH -next v5 6/8] arm64: add support for machine check error safe
On Sat, Jun 18, 2022 at 05:18:55PM +0800, Tong Tiangen wrote:
> 在 2022/6/17 16:55, Mark Rutland 写道:
> > On Sat, May 28, 2022 at 06:50:54AM +, Tong Tiangen wrote:
> > > +static bool arm64_do_kernel_sea(unsigned long addr, unsigned int esr,
> > > + struct pt_regs *regs, int sig, int code)
> > > +{
> > > + if (!IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC))
> > > + return false;
> > > +
> > > + if (user_mode(regs) || !current->mm)
> > > + return false;
> >
> > What's the `!current->mm` check for?
>
> At first, I considered that only user processes have the opportunity to
> recover when they trigger memory error.
>
> But it seems that this restriction is unreasonable. When the kernel thread
> triggers memory error, it can also be recovered. for instance:
>
> https://lore.kernel.org/linux-mm/20220527190731.322722-1-jiaqi...@google.com/
>
> And i think if(!current->mm) shoud be added below:
>
> if(!current->mm) {
> set_thread_esr(0, esr);
> arm64_force_sig_fault(...);
> }
> return true;
Why does 'current->mm' have anything to do with this, though?
There can be kernel threads with `current->mm` set in unusual circumstances
(and there's a lot of kernel code out there which handles that wrong), so if
you want to treat user tasks differently, we should be doing something like
checking PF_KTHREAD, or adding something like an is_user_task() helper.
[...]
> > > +
> > > + if (apei_claim_sea(regs) < 0)
> > > + return false;
> > > +
> > > + if (!fixup_exception_mc(regs))
> > > + return false;
> >
> > I thought we still wanted to signal the task in this case? Or do you expect
> > to
> > add that into `fixup_exception_mc()` ?
>
> Yeah, here return false and will signal to task in do_sea() ->
> arm64_notify_die().
I mean when we do the fixup.
I thought the idea was to apply the fixup (to stop the kernel from crashing),
but still to deliver a fatal signal to the user task since we can't do what the
user task asked us to.
> > > +
> > > + set_thread_esr(0, esr);
> >
> > Why are we not setting the address? Is that deliberate, or an oversight?
>
> Here set fault_address to 0, i refer to the logic of arm64_notify_die().
>
> void arm64_notify_die(...)
> {
> if (user_mode(regs)) {
> WARN_ON(regs != current_pt_regs());
> current->thread.fault_address = 0;
> current->thread.fault_code = err;
>
> arm64_force_sig_fault(signo, sicode, far, str);
> } else {
> die(str, regs, err);
> }
> }
>
> I don't know exactly why and do you know why arm64_notify_die() did this? :)
To be honest, I don't know, and that looks equally suspicious to me.
Looking at the git history, that was added in commit:
9141300a5884b57c ("arm64: Provide read/write fault information in compat
signal handlers")
... so maybe Catalin recalls why.
Perhaps the assumption is just that this will be fatal and so unimportant? ...
but in that case the same logic would apply to the ESR value, so it's not clear
to me.
Mark.
Re: [PATCH -next v5 2/8] arm64: extable: make uaaccess helper use extable type EX_TYPE_UACCESS_ERR_ZERO
On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: > > > > diff --git a/arch/arm64/include/asm/asm-extable.h > > > > b/arch/arm64/include/asm/asm-extable.h > > > > index 56ebe183e78b..9c94ac1f082c 100644 > > > > --- a/arch/arm64/include/asm/asm-extable.h > > > > +++ b/arch/arm64/include/asm/asm-extable.h > > > > @@ -28,6 +28,14 @@ > > > > __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) > > > > .endm > > > > +/* > > > > + * Create an exception table entry for uaccess `insn`, which > > > > will branch to `fixup` > > > > + * when an unhandled fault is taken. > > > > + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). > > > > + */ > > > > + .macro _asm_extable_uaccess, insn, fixup > > > > + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) > > > > + .endm > > > > > > I'm not too keen on using `~0` here, since that also sets other bits > > > in the > > > data field, and its somewhat opaque. > > > > > > How painful is it to generate the data fields as with the C version > > > of this > > > macro, so that we can pass in wzr explciitly for the two sub-fields? > > > > > > Other than that, this looks good to me. > > > > > > Thanks, > > > Mark. > > > > ok, will fix next version. > > > > Thanks, > > Tong. > > I tried to using data filelds as with C version, but here assembly code we > can not using operator such as << and |, if we use lsl and orr instructions, > the gpr will be occupied. > > So how about using 0x3ff directly here? it means err register and zero > register both set to x31. I had a go at implementing this, and it seems simple enough. Please see: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess Mark.
Re: [PATCH -next v5 7/8] arm64: add uaccess to machine check safe
On Sat, Jun 18, 2022 at 05:27:45PM +0800, Tong Tiangen wrote:
>
>
> 在 2022/6/17 17:06, Mark Rutland 写道:
> > On Sat, May 28, 2022 at 06:50:55AM +, Tong Tiangen wrote:
> > > If user access fail due to hardware memory error, only the relevant
> > > processes are affected, so killing the user process and isolate the
> > > error page with hardware memory errors is a more reasonable choice
> > > than kernel panic.
> > >
> > > Signed-off-by: Tong Tiangen
> >
> > > ---
> > > arch/arm64/lib/copy_from_user.S | 8
> > > arch/arm64/lib/copy_to_user.S | 8
> >
> > All of these changes are to the *kernel* accesses performed as part of copy
> > to/from user, and have nothing to do with userspace, so it does not make
> > sense
> > to mark these as UACCESS.
>
> You have a point. so there is no need to modify copy_from/to_user.S in this
> patch set.
Cool, thanks. If this patch just has the extable change, that's fine by me.
> > Do we *actually* need to recover from failues on these accesses? Looking at
> > _copy_from_user(), the kernel will immediately follow this up with a
> > memset()
> > to the same address which will be fatal anyway, so this is only punting the
> > failure for a few instructions.
>
> If recovery success, The task will be killed and there will be no subsequent
> memset().
I don't think that's true.
IIUC per the last patch, in the exception handler we'll apply the fixup then
force a signal. That doesn't kill the task immediately, and we'll return from
the exception handler back into the original context (with the fixup applied).
The structure of copy_from_user() is
copy_from_user(to, from, n) {
_copy_from_user(to, from, n) {
res = n;
res = raw_copy_from_user(to, from, n);
if (res)
memset(to + (n - res), 0, res);
}
}
So when the fixup is applied and res indicates that the copy terminated early,
there is an unconditinal memset() before the fatal signal is handled in the
return to userspace path.
> > If we really need to recover from certain accesses to kernel memory we
> > should
> > add a new EX_TYPE_KACCESS_ERR_ZERO_MC or similar, but we need a strong
> > rationale as to why that's useful. As things stand I do not beleive it makes
> > sense for copy to/from user specifically.
[...]
> > > diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
> > > index c301dcf6335f..8ca8d9639f9f 100644
> > > --- a/arch/arm64/mm/extable.c
> > > +++ b/arch/arm64/mm/extable.c
> > > @@ -86,10 +86,10 @@ bool fixup_exception_mc(struct pt_regs *regs)
> > > if (!ex)
> > > return false;
> > > - /*
> > > - * This is not complete, More Machine check safe extable type can
> > > - * be processed here.
> > > - */
> > > + switch (ex->type) {
> > > + case EX_TYPE_UACCESS_ERR_ZERO:
> > > + return ex_handler_uaccess_err_zero(ex, regs);
> > > + }
> >
> > This addition specifically makes sense to me, so can you split this into a
> > separate patch?
>
> According to my understanding of the above, only the modification of
> extable.c is retained.
>
> So what do you mean which part is made into a separate patch?
As above, if you just retain the extable.c changes, that's fine by me.
Thanks,
Mark.
Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
Le 18/06/2022 à 10:03, Liang He a écrit :
>
>
>
>
>
> 在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>>
>>
>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>> In add_pcspkr(), it is better to call of_node_put() after the
>>> 'if(!np)' check.
>>
>> Why is it better ?
>>
>>
>>
>> /**
>> * of_node_put() - Decrement refcount of a node
>> * @node: Node to dec refcount, NULL is supported to simplify writing of
>> * callers
>> */
>> void of_node_put(struct device_node *node)
>> {
>> if (node)
>> kobject_put(>kobj);
>> }
>> EXPORT_SYMBOL(of_node_put);
>>
>>
>>
>> Christophe
>
> Hi, Christophe.
>
> Thanks for your reply and I want to have a discussion.
>
> In my thought, xxx_put(pointer)'s semantic usually means
> this reference has been used done and will not be used
> anymore. Is this semantic more reasonable, right?
>
> Besides, if the np is NULL, we can just return and save a cpu
> time for the xxx_put() call.
>
> Otherwise, I prefer to call it 'use(check)-after-put'.
>
> In fact, I have meet many other 'use(check)-after-put' instances
> after I send this patch-commit, so I am waiting for this
> discussion.
>
> This is just my thought, it may be wrong.
>
> Anyway, thanks for your reply.
Well in principle you are right, in an ideal world it should be like
that. However, you have to wonder if it is worth the churn. The CPU
cycle argument is valid only if that function is used in a hot path. But
as we are talking about error handling, it can't be a hot path.
Taking into account the comment associated of of_node_put : "NULL is
supported to simplify writing of callers", it means that usage is valid,
just like it is with function kfree() after a kmalloc().
So in a new developpement, or when doing real modifications to a driver,
that kind of change can be done ideally. However for drivers that have
been there for years without any change, ask yourself if it is worth the
churn. You spend time on it, you require other people to spend time on
it for reviewing and applying your patches and during that time they
don't do other things that could have been more usefull.
So unless this change is part of a more global patch, I think it is not
worth the effort.
By the way, also for all your other patches, I think you should start
doing all the changes locally on your side, and when you are finished
try to group things together in bigger patches per area instead of
sending one by one. I see you have already started doing that for
opal/powernv for instance, but there are still individual powernv/opal
in the queue. I think you should group all together in a single patch.
And same for other areas, please try to minimise the number of patches.
We don't link huge bombs that modify all the kernel at once, but you can
group things together, one patch for powerpc core parts, one patch for
each platform in arch/powerpc/platforms/ etc ...
Christophe
Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
在 2022-06-18 15:13:13，"Christophe Leroy" 写道：
>
>
>Le 17/06/2022 à 13:26, Liang He a écrit :
>> In add_pcspkr(), it is better to call of_node_put() after the
>> 'if(!np)' check.
>
>Why is it better ?
>
>
>
>/**
> * of_node_put() - Decrement refcount of a node
> * @node: Node to dec refcount, NULL is supported to simplify writing of
> *callers
> */
>void of_node_put(struct device_node *node)
>{
> if (node)
> kobject_put(>kobj);
>}
>EXPORT_SYMBOL(of_node_put);
>
>
>
>Christophe
Hi, Christophe.
Thanks for your reply and I want to have a discussion.
In my thought, xxx_put(pointer)'s semantic usually means
this reference has been used done and will not be used
anymore. Is this semantic more reasonable, right?
Besides, if the np is NULL, we can just return and save a cpu
time for the xxx_put() call.
Otherwise, I prefer to call it 'use(check)-after-put'.
In fact, I have meet many other 'use(check)-after-put' instances
after I send this patch-commit, so I am waiting for this
discussion.
This is just my thought, it may be wrong.
Anyway, thanks for your reply.
Liang
>
>
>>
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/kernel/setup-common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/setup-common.c
>> b/arch/powerpc/kernel/setup-common.c
>> index eb0077b302e2..761817d1f4db 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
>> int ret;
>>
>> np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
>> -of_node_put(np);
>> if (!np)
>> return -ENODEV;
>> +of_node_put(np);
>>
>> pd = platform_device_alloc("pcspkr", -1);
>> if (!pd)
Re: [PATCH] powerpc: powernv: Fix refcount leak bug in opal-powercap
Le 17/06/2022 à 07:42, Liang He a écrit :
At 2022-06-17 13:01:27, "Christophe JAILLET"
wrote:
Le 17/06/2022 à 06:20, Liang He a écrit :
In opal_powercap_init(), of_find_compatible_node() will return
a node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Besides, for_each_child_of_node() will automatically *inc* and *dec*
refcount during iteration. However, we should add the of_node_put()
if there is a break.
Hi,
I'm not sure that your patch is right here. Because of this *inc* and
*dec* things, do we still need to of_node_put(powercap) once we have
entered for_each_child_of_node?
I think that this reference will be released on the first iteration of
the loop.
Hi, CJ,
Thanks for your reply and I want have a discuss.
Based on my review on the src of 'of_get_next_child', there is only
*inc* for next and *dec* for pre as follow.
(|node| == powercap)
==__of_get_next_child( |node|, prev)==
...
next = prev? prev->sibling:|node|->child;
of_node_get(next);
of_node_put(prev);
...
=
However, there is no any code to release the |node| (i.e., *powercap*).
Am I right? If I am wrong, please correct me, thanks.
You are right.
I mis-read __of_get_next_child(().
CJ
Maybe of_node_put(powercap) should be duplicated everywhere it is
relevant and removed from the error handling path?
Or an additional reference should be taken before the loop?
Or adding a new label with "powercap = NULL" and branching there when
needed?
CJ
If my understanding is right, I think current patch is right.
Otherwise, I will make a new patch to handle that, Thanks.
Liang
Signed-off-by: Liang He
---
arch/powerpc/platforms/powernv/opal-powercap.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c
b/arch/powerpc/platforms/powernv/opal-powercap.c
index 64506b46e77b..b102477d3f95 100644
--- a/arch/powerpc/platforms/powernv/opal-powercap.c
+++ b/arch/powerpc/platforms/powernv/opal-powercap.c
@@ -153,7 +153,7 @@ void __init opal_powercap_init(void)
pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps),
GFP_KERNEL);
if (!pcaps)
- return;
+ goto out_powercap;
powercap_kobj = kobject_create_and_add("powercap", opal_kobj);
if (!powercap_kobj) {
@@ -236,6 +236,9 @@ void __init opal_powercap_init(void)
kfree(pcaps[i].pg.name);
}
kobject_put(powercap_kobj);
+ of_node_put(node);
out_pcaps:
kfree(pcaps);
+out_powercap:
+ of_node_put(powercap);
}
[PATCH] powerpc: perf: Fix refcount leak bug in imc-pmu.c
In update_events_in_group(), of_find_node_by_phandle() will return
a node pointer with refcount incremented. We should use of_node_put()
in fail path or when it is not used anymore.
Signed-off-by: Liang He
---
arch/powerpc/perf/imc-pmu.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index d7976ab40d38..d517aba94d1b 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -240,8 +240,10 @@ static int update_events_in_group(struct device_node
*node, struct imc_pmu *pmu)
ct = of_get_child_count(pmu_events);
/* Get the event prefix */
- if (of_property_read_string(node, "events-prefix", ))
+ if (of_property_read_string(node, "events-prefix", )) {
+ of_node_put(pmu_events);
return 0;
+ }
/* Get a global unit and scale data if available */
if (of_property_read_string(node, "scale", _scale))
@@ -255,8 +257,10 @@ static int update_events_in_group(struct device_node
*node, struct imc_pmu *pmu)
/* Allocate memory for the events */
pmu->events = kcalloc(ct, sizeof(struct imc_events), GFP_KERNEL);
- if (!pmu->events)
+ if (!pmu->events) {
+ of_node_put(pmu_events);
return -ENOMEM;
+ }
ct = 0;
/* Parse the events and update the struct */
@@ -266,6 +270,8 @@ static int update_events_in_group(struct device_node *node,
struct imc_pmu *pmu)
ct++;
}
+ of_node_put(pmu_events);
+
/* Allocate memory for attribute group */
attr_group = kzalloc(sizeof(*attr_group), GFP_KERNEL);
if (!attr_group) {
--
2.25.1
Re: [PATCH] powerpc: kernel: Change the order of of_node_put()
Le 17/06/2022 à 13:26, Liang He a écrit :
> In add_pcspkr(), it is better to call of_node_put() after the
> 'if(!np)' check.
Why is it better ?
/**
* of_node_put() - Decrement refcount of a node
* @node: Node to dec refcount, NULL is supported to simplify writing of
* callers
*/
void of_node_put(struct device_node *node)
{
if (node)
kobject_put(>kobj);
}
EXPORT_SYMBOL(of_node_put);
Christophe
>
> Signed-off-by: Liang He
> ---
> arch/powerpc/kernel/setup-common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/setup-common.c
> b/arch/powerpc/kernel/setup-common.c
> index eb0077b302e2..761817d1f4db 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
> int ret;
>
> np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
> - of_node_put(np);
> if (!np)
> return -ENODEV;
> + of_node_put(np);
>
> pd = platform_device_alloc("pcspkr", -1);
> if (!pd)
[PATCH] tty: serial: Fix refcount leak bug in ucc_uart.c
In soc_info(), of_find_node_by_type() will return a node pointer with refcount incremented. We should use of_node_put() when it is not used anymore. Signed-off-by: Liang He --- drivers/tty/serial/ucc_uart.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c index 6000853973c1..3cc9ef08455c 100644 --- a/drivers/tty/serial/ucc_uart.c +++ b/drivers/tty/serial/ucc_uart.c @@ -1137,6 +1137,8 @@ static unsigned int soc_info(unsigned int *rev_h, unsigned int *rev_l) /* No compatible property, so try the name. */ soc_string = np->name; + of_node_put(np); + /* Extract the SOC number from the "PowerPC," string */ if ((sscanf(soc_string, "PowerPC,%u", ) != 1) || !soc) return 0; -- 2.25.1
