From: Lars Poeschel <[email protected]>
This removes an assignment in device_add. It assigned the parent
kobject to the kobject of the new device. This is not necessary,
because the call to kobject_add a few lines later also does this same
assignment.
Signed-off-by: Lars Poeschel <[email protected]>
---
drivers/base/core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index bb5806a2bd4c..03b5396cd192 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2847,8 +2847,6 @@ int device_add(struct device *dev)
error = PTR_ERR(kobj);
goto parent_error;
}
- if (kobj)
- dev->kobj.parent = kobj;
/* use parent numa_node */
if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
@@ -2856,7 +2854,7 @@ int device_add(struct device *dev)
/* first, register with generic layer. */
/* we require the name to be set before, and pass NULL */
- error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
+ error = kobject_add(&dev->kobj, kobj, NULL);
if (error) {
glue_dir = get_glue_dir(dev);
goto Error;
--
2.28.0
On Tue, Sep 29, 2020 at 01:58:08PM +0200, [email protected] wrote:
> From: Lars Poeschel <[email protected]>
>
> This removes an assignment in device_add. It assigned the parent
> kobject to the kobject of the new device. This is not necessary,
> because the call to kobject_add a few lines later also does this same
> assignment.
>
> Signed-off-by: Lars Poeschel <[email protected]>
> ---
> drivers/base/core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bb5806a2bd4c..03b5396cd192 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2847,8 +2847,6 @@ int device_add(struct device *dev)
> error = PTR_ERR(kobj);
> goto parent_error;
> }
> - if (kobj)
> - dev->kobj.parent = kobj;
>
> /* use parent numa_node */
> if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> @@ -2856,7 +2854,7 @@ int device_add(struct device *dev)
>
> /* first, register with generic layer. */
> /* we require the name to be set before, and pass NULL */
> - error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> + error = kobject_add(&dev->kobj, kobj, NULL);
That's very subtle, and might not really be correct for all users, have
you checked?
Anyway, I'd rather leave this as-is if possible, as we know this works
correctly, and it is not going to save any time/energy to remove that
assignment, right?
thanks,
greg k-h
On Tue, Sep 29, 2020 at 02:25:33PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 29, 2020 at 01:58:08PM +0200, [email protected] wrote:
> > From: Lars Poeschel <[email protected]>
> >
> > This removes an assignment in device_add. It assigned the parent
> > kobject to the kobject of the new device. This is not necessary,
> > because the call to kobject_add a few lines later also does this same
> > assignment.
> >
> > Signed-off-by: Lars Poeschel <[email protected]>
> > ---
> > drivers/base/core.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index bb5806a2bd4c..03b5396cd192 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2847,8 +2847,6 @@ int device_add(struct device *dev)
> > error = PTR_ERR(kobj);
> > goto parent_error;
> > }
> > - if (kobj)
> > - dev->kobj.parent = kobj;
> >
> > /* use parent numa_node */
> > if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> > @@ -2856,7 +2854,7 @@ int device_add(struct device *dev)
> >
> > /* first, register with generic layer. */
> > /* we require the name to be set before, and pass NULL */
> > - error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> > + error = kobject_add(&dev->kobj, kobj, NULL);
>
> That's very subtle, and might not really be correct for all users, have
> you checked?
Of course I have not checked for all users ;-), but I have checked this
for my system and I did not notice any difference. My system is an arm
based board that does several hundreds of calls to the device_add
function per kernel bootup.
> Anyway, I'd rather leave this as-is if possible, as we know this works
> correctly, and it is not going to save any time/energy to remove that
> assignment, right?
Of course it's up to you to leave this as is.
Pure binary size drops from 0x784 to 0x778 (12 bytes) with this patch
for the device_add function on arm with gcc 10.2.0.
So this saves a tiny amount of size and energy. If it's worth that, I
don't know.
And not to mention the time/energy you save when some time some random
guy again stubles upon this, sends you a patch and then you have to
reply. ;-)
Ok, as said:Taking this is up to you. I can also live without this.
Regards,
Lars
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 9c28a17954f000e148b7863d4c5efa8fd3d80f4f ("[PATCH] driver core: Remove double assignment")
url: https://github.com/0day-ci/linux/commits/poeschel-lemonage-de/driver-core-Remove-double-assignment/20200929-195945
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git e5e5fcef600e94d83c6542cdcca3ab6dada95946
in testcase: suspend-stress
version:
with following parameters:
mode: mem
iterations: 10
on test machine: 4 threads Ivy Bridge with 4G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
kern :warn : [ 5.897345] WARNING: CPU: 3 PID: 1 at kernel/events/core.c:12993 perf_event_sysfs_init+0x5d/0x81
kern :warn : [ 5.897666] Modules linked in:
kern :warn : [ 5.897825] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-00023-g9c28a17954f000 #1
kern :warn : [ 5.898127] Hardware name: TOSHIBA PORTEGE Z830/PORTEGE Z830, BIOS Version 2.30 07/12/2012
kern :warn : [ 5.898441] RIP: 0010:perf_event_sysfs_init+0x5d/0x81
kern :warn : [ 5.898637] Code: 83 7b 30 00 74 26 83 7b 38 00 78 20 48 89 df e8 d7 9c 52 fe 89 c2 85 c0 74 12 48 8b 73 30 48 c7 c7 98 96 35 82 e8 83 c3 3a fe <0f> 0b 48 8b 1b eb c5 c7 05 24 1f 3c 00 01 00 00 00 48 c7 c7 40 ac
kern :warn : [ 5.899196] RSP: 0000:ffffc90000033e18 EFLAGS: 00010286
kern :warn : [ 5.899399] RAX: 0000000000000000 RBX: ffffffff82617020 RCX: 000000000000054a
kern :warn : [ 5.899634] RDX: 0000000000000001 RSI: 0000000000000086 RDI: ffffffff82fb882c
kern :warn : [ 5.899870] RBP: ffffffff82d335c2 R08: 000000000000054a R09: 000000000000ffa0
kern :warn : [ 5.900106] R10: 0000000000000737 R11: ffffc90000033cc5 R12: 0000000000000000
kern :warn : [ 5.900343] R13: ffff888107462800 R14: 0000000000000000 R15: 0000000000000000
kern :warn : [ 5.900583] FS: 0000000000000000(0000) GS:ffff88811e6c0000(0000) knlGS:0000000000000000
kern :warn : [ 5.900886] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kern :warn : [ 5.901094] CR2: 0000000000000000 CR3: 000000000260a001 CR4: 00000000001706e0
kern :warn : [ 5.901330] Call Trace:
kern :warn : [ 5.901481] do_one_initcall+0x46/0x204
kern :warn : [ 5.901655] kernel_init_freeable+0x1da/0x23c
kern :warn : [ 5.901839] ? rest_init+0xc6/0xc6
kern :warn : [ 5.902004] kernel_init+0xa/0x11a
kern :warn : [ 5.902168] ret_from_fork+0x22/0x30
kern :warn : [ 5.902338] ---[ end trace cba6bbf77ba0c364 ]---
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
Thanks,
lkp