| Message ID | 20240323084737.12986-1-lidong.zhong@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/pseries: remove returning ENODEV when uevent is triggered | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Hi Michael, Could you share your opinion about this patch please? Thanks, Lidong On Sat, Mar 23, 2024 at 4:47 PM Lidong Zhong <lidong.zhong@suse.com> wrote: > We have noticed the following nuisance messages during boot > > [ 7.120610][ T1060] vio vio: uevent: failed to send synthetic uevent > [ 7.122281][ T1060] vio 4000: uevent: failed to send synthetic uevent > [ 7.122304][ T1060] vio 4001: uevent: failed to send synthetic uevent > [ 7.122324][ T1060] vio 4002: uevent: failed to send synthetic uevent > [ 7.122345][ T1060] vio 4004: uevent: failed to send synthetic uevent > > It's caused by either vio_register_device_node() failed to set > dev->of_node or > the missing "compatible" property. Try return as much information as > possible > instead of a failure. The above annoying errors can also be removed > after the patch applied. > > Signed-off-by: Lidong Zhong <lidong.zhong@suse.com> > --- > arch/powerpc/platforms/pseries/vio.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/vio.c > b/arch/powerpc/platforms/pseries/vio.c > index 90ff85c879bf..62961715ca24 100644 > --- a/arch/powerpc/platforms/pseries/vio.c > +++ b/arch/powerpc/platforms/pseries/vio.c > @@ -1593,12 +1593,13 @@ static int vio_hotplug(const struct device *dev, > struct kobj_uevent_env *env) > > dn = dev->of_node; > if (!dn) > - return -ENODEV; > + goto out; > cp = of_get_property(dn, "compatible", NULL); > if (!cp) > - return -ENODEV; > - > - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); > + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); > + else > + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, > cp); > +out: > return 0; > } > > -- > 2.35.3 > >
Hi Lidong, Thanks for the patch. I'm not an expert on udev etc. so apologies if any of these questions are stupid. Lidong Zhong <lidong.zhong@suse.com> writes: > We have noticed the following nuisance messages during boot > > [ 7.120610][ T1060] vio vio: uevent: failed to send synthetic uevent > [ 7.122281][ T1060] vio 4000: uevent: failed to send synthetic uevent > [ 7.122304][ T1060] vio 4001: uevent: failed to send synthetic uevent > [ 7.122324][ T1060] vio 4002: uevent: failed to send synthetic uevent > [ 7.122345][ T1060] vio 4004: uevent: failed to send synthetic uevent > > It's caused by either vio_register_device_node() failed to set dev->of_node or > the missing "compatible" property. Try return as much information as possible > instead of a failure. Does udev etc. cope with that? Can we just change the content of the MODALIAS value like that? With this patch we'll start emitting uevents for devices we previously didn't. I guess that's OK because nothing is expecting them? Can you include a log of udev showing the event firing and that nothing breaks. On my system here I see nothing matches the devices except for libvpd, which seems to match lots of things. > diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c > index 90ff85c879bf..62961715ca24 100644 > --- a/arch/powerpc/platforms/pseries/vio.c > +++ b/arch/powerpc/platforms/pseries/vio.c > @@ -1593,12 +1593,13 @@ static int vio_hotplug(const struct device *dev, struct kobj_uevent_env *env) > > dn = dev->of_node; > if (!dn) > - return -ENODEV; > + goto out; > cp = of_get_property(dn, "compatible", NULL); > if (!cp) > - return -ENODEV; > - > - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); > + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); If it's OK to skip the compatible property then we don't need the of_node at all, and we could always emit this, even when of_node is not available. > + else > + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); > +out: > return 0; > } I think we also should update the vio modalias_show() to follow the same logic, otherwise the uevent MODALIAS value and the modalias file won't match which is confusing. Preferably vio_hotplug() and modalias_show() would just call a common helper. cheers
Hi Michael, Thanks for your reply. On Tue, Apr 9, 2024 at 4:46 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > Hi Lidong, > > Thanks for the patch. > > I'm not an expert on udev etc. so apologies if any of these questions > are stupid. > > Lidong Zhong <lidong.zhong@suse.com> writes: > > We have noticed the following nuisance messages during boot > > > > [ 7.120610][ T1060] vio vio: uevent: failed to send synthetic uevent > > [ 7.122281][ T1060] vio 4000: uevent: failed to send synthetic uevent > > [ 7.122304][ T1060] vio 4001: uevent: failed to send synthetic uevent > > [ 7.122324][ T1060] vio 4002: uevent: failed to send synthetic uevent > > [ 7.122345][ T1060] vio 4004: uevent: failed to send synthetic uevent > > > > It's caused by either vio_register_device_node() failed to set > dev->of_node or > > the missing "compatible" property. Try return as much information as > possible > > instead of a failure. > > Does udev etc. cope with that? Can we just change the content of the > MODALIAS value like that? > > With this patch we'll start emitting uevents for devices we previously > didn't. I guess that's OK because nothing is expecting them? > > Can you include a log of udev showing the event firing and that nothing > breaks. > > On my system here I see nothing matches the devices except for libvpd, > which seems to match lots of things. > It's an issue reported by our customer. I am sorry I can't provide more information because I don't have the environment to reproduce this issue. The only related log I got is shown below: Feb 07 14:08:03 rb3i0060 udevadm[623]: vio: Failed to write 'add' to '/sys/devices/vio/uevent', ignoring: No such device Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio: failed to send uevent Feb 07 14:08:03 rb3i0060 kernel: vio vio: uevent: failed to send synthetic uevent Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4000: failed to send uevent Feb 07 14:08:03 rb3i0060 kernel: vio 4000: uevent: failed to send synthetic uevent Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4001: failed to send uevent Feb 07 14:08:03 rb3i0060 kernel: vio 4001: uevent: failed to send synthetic uevent Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4002: failed to send uevent Feb 07 14:08:03 rb3i0060 kernel: vio 4002: uevent: failed to send synthetic uevent Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4004: failed to send uevent Feb 07 14:08:03 rb3i0060 kernel: vio 4004: uevent: failed to send synthetic uevent Feb 07 14:08:03 rb3i0060 udevadm[623]: 4000: Failed to write 'add' to '/sys/devices/vio/4000/uevent', ignoring: No such device Feb 07 14:08:03 rb3i0060 udevadm[623]: 4001: Failed to write 'add' to '/sys/devices/vio/4001/uevent', ignoring: No such device Feb 07 14:08:03 rb3i0060 udevadm[623]: 4002: Failed to write 'add' to '/sys/devices/vio/4002/uevent', ignoring: No such device Feb 07 14:08:03 rb3i0060 udevadm[623]: 4004: Failed to write 'add' to '/sys/devices/vio/4004/uevent', ignoring: No such device systemd-udev-trigger service calls 'udevadm trigger --type=devices --action=add' and kernel returns -ENODEV because either dev->of_node is NULL or 'compatible' property is not present. Similar cases were already reported after some search, for example https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1827162 https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1845319 I don't think it causes real problems but confusion to users. > > diff --git a/arch/powerpc/platforms/pseries/vio.c > b/arch/powerpc/platforms/pseries/vio.c > > index 90ff85c879bf..62961715ca24 100644 > > --- a/arch/powerpc/platforms/pseries/vio.c > > +++ b/arch/powerpc/platforms/pseries/vio.c > > @@ -1593,12 +1593,13 @@ static int vio_hotplug(const struct device *dev, > struct kobj_uevent_env *env) > > > > dn = dev->of_node; > > if (!dn) > > - return -ENODEV; > > + goto out; > > cp = of_get_property(dn, "compatible", NULL); > > if (!cp) > > - return -ENODEV; > > - > > - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); > > + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); > > If it's OK to skip the compatible property then we don't need the > of_node at all, and we could always emit this, even when of_node is not > available. > You mean something like this? @@ -1592,13 +1592,10 @@ static int vio_hotplug(const struct device *dev, struct kobj_uevent_env *env) const char *cp; dn = dev->of_node; - if (!dn) - return -ENODEV; - cp = of_get_property(dn, "compatible", NULL); - if (!cp) - return -ENODEV; - - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); + if (dn && (cp = of_get_property(dn, "compatible", NULL)) + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); + else + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); return 0; > > > + else > > + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, > cp); > > +out: > > return 0; > > } > > I think we also should update the vio modalias_show() to follow the same > logic, otherwise the uevent MODALIAS value and the modalias file won't > match which is confusing. > > Preferably vio_hotplug() and modalias_show() would just call a common > helper. > > cheers > Thanks for the suggestion. I'll send a v2 patch.
Hi Michael, After checking the definition of modalias in modalias_show(), I think it's better to keep the same logic in vio_hotplug(), that's removing the else part in my original patch shown below. + if (dn && (cp = of_get_property(dn, "compatible", NULL)) + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); + else + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); I think we can avoid some possible regression then. I'll make the change in my v2 patch. -- Regards, Lidong Zhong On Wed, Apr 10, 2024 at 9:25 AM Lidong Zhong <lidong.zhong@suse.com> wrote: > Hi Michael, > > Thanks for your reply. > > On Tue, Apr 9, 2024 at 4:46 PM Michael Ellerman <mpe@ellerman.id.au> > wrote: > >> Hi Lidong, >> >> Thanks for the patch. >> >> I'm not an expert on udev etc. so apologies if any of these questions >> are stupid. >> >> Lidong Zhong <lidong.zhong@suse.com> writes: >> > We have noticed the following nuisance messages during boot >> > >> > [ 7.120610][ T1060] vio vio: uevent: failed to send synthetic uevent >> > [ 7.122281][ T1060] vio 4000: uevent: failed to send synthetic uevent >> > [ 7.122304][ T1060] vio 4001: uevent: failed to send synthetic uevent >> > [ 7.122324][ T1060] vio 4002: uevent: failed to send synthetic uevent >> > [ 7.122345][ T1060] vio 4004: uevent: failed to send synthetic uevent >> > >> > It's caused by either vio_register_device_node() failed to set >> dev->of_node or >> > the missing "compatible" property. Try return as much information as >> possible >> > instead of a failure. >> >> Does udev etc. cope with that? Can we just change the content of the >> MODALIAS value like that? >> >> With this patch we'll start emitting uevents for devices we previously >> didn't. I guess that's OK because nothing is expecting them? >> >> Can you include a log of udev showing the event firing and that nothing >> breaks. >> >> On my system here I see nothing matches the devices except for libvpd, >> which seems to match lots of things. >> > > It's an issue reported by our customer. I am sorry I can't provide more > information because I don't have the environment > to reproduce this issue. The only related log I got is shown below: > > Feb 07 14:08:03 rb3i0060 udevadm[623]: vio: Failed to write 'add' to > '/sys/devices/vio/uevent', ignoring: No such device > > Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio: failed to > send uevent > > Feb 07 14:08:03 rb3i0060 kernel: vio vio: uevent: failed to send synthetic > uevent > > Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4000: failed > to send uevent > > Feb 07 14:08:03 rb3i0060 kernel: vio 4000: uevent: failed to send > synthetic uevent > > Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4001: failed > to send uevent > > Feb 07 14:08:03 rb3i0060 kernel: vio 4001: uevent: failed to send > synthetic uevent > > Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4002: failed > to send uevent > > Feb 07 14:08:03 rb3i0060 kernel: vio 4002: uevent: failed to send > synthetic uevent > > Feb 07 14:08:03 rb3i0060 kernel: synth uevent: /devices/vio/4004: failed > to send uevent > > Feb 07 14:08:03 rb3i0060 kernel: vio 4004: uevent: failed to send > synthetic uevent > > Feb 07 14:08:03 rb3i0060 udevadm[623]: 4000: Failed to write 'add' to > '/sys/devices/vio/4000/uevent', ignoring: No such device > > Feb 07 14:08:03 rb3i0060 udevadm[623]: 4001: Failed to write 'add' to > '/sys/devices/vio/4001/uevent', ignoring: No such device > > Feb 07 14:08:03 rb3i0060 udevadm[623]: 4002: Failed to write 'add' to > '/sys/devices/vio/4002/uevent', ignoring: No such device > > Feb 07 14:08:03 rb3i0060 udevadm[623]: 4004: Failed to write 'add' to > '/sys/devices/vio/4004/uevent', ignoring: No such device > > systemd-udev-trigger service calls 'udevadm trigger --type=devices > --action=add' and kernel returns -ENODEV because either > dev->of_node is NULL or 'compatible' property is not present. Similar > cases were already reported after some search, for example > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1827162 > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1845319 > I don't think it causes real problems but confusion to users. > > >> > diff --git a/arch/powerpc/platforms/pseries/vio.c >> b/arch/powerpc/platforms/pseries/vio.c >> > index 90ff85c879bf..62961715ca24 100644 >> > --- a/arch/powerpc/platforms/pseries/vio.c >> > +++ b/arch/powerpc/platforms/pseries/vio.c >> > @@ -1593,12 +1593,13 @@ static int vio_hotplug(const struct device >> *dev, struct kobj_uevent_env *env) >> > >> > dn = dev->of_node; >> > if (!dn) >> > - return -ENODEV; >> > + goto out; >> > cp = of_get_property(dn, "compatible", NULL); >> > if (!cp) >> > - return -ENODEV; >> > - >> > - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); >> > + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); >> >> If it's OK to skip the compatible property then we don't need the >> of_node at all, and we could always emit this, even when of_node is not >> available. >> > > You mean something like this? > @@ -1592,13 +1592,10 @@ static int vio_hotplug(const struct device *dev, > struct kobj_uevent_env *env) > const char *cp; > > dn = dev->of_node; > - if (!dn) > - return -ENODEV; > - cp = of_get_property(dn, "compatible", NULL); > - if (!cp) > - return -ENODEV; > - > - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); > + if (dn && (cp = of_get_property(dn, "compatible", NULL)) > + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, > cp); > + else > + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); > return 0; > > >> >> > + else >> > + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, >> cp); >> > +out: >> > return 0; >> > } >> >> I think we also should update the vio modalias_show() to follow the same >> logic, otherwise the uevent MODALIAS value and the modalias file won't >> match which is confusing. >> >> Preferably vio_hotplug() and modalias_show() would just call a common >> helper. >> >> cheers >> > > Thanks for the suggestion. I'll send a v2 patch. > > > -- > Regards, > Lidong Zhong >
diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c index 90ff85c879bf..62961715ca24 100644 --- a/arch/powerpc/platforms/pseries/vio.c +++ b/arch/powerpc/platforms/pseries/vio.c @@ -1593,12 +1593,13 @@ static int vio_hotplug(const struct device *dev, struct kobj_uevent_env *env) dn = dev->of_node; if (!dn) - return -ENODEV; + goto out; cp = of_get_property(dn, "compatible", NULL); if (!cp) - return -ENODEV; - - add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); + add_uevent_var(env, "MODALIAS=vio:T%s", vio_dev->type); + else + add_uevent_var(env, "MODALIAS=vio:T%sS%s", vio_dev->type, cp); +out: return 0; }
We have noticed the following nuisance messages during boot [ 7.120610][ T1060] vio vio: uevent: failed to send synthetic uevent [ 7.122281][ T1060] vio 4000: uevent: failed to send synthetic uevent [ 7.122304][ T1060] vio 4001: uevent: failed to send synthetic uevent [ 7.122324][ T1060] vio 4002: uevent: failed to send synthetic uevent [ 7.122345][ T1060] vio 4004: uevent: failed to send synthetic uevent It's caused by either vio_register_device_node() failed to set dev->of_node or the missing "compatible" property. Try return as much information as possible instead of a failure. The above annoying errors can also be removed after the patch applied. Signed-off-by: Lidong Zhong <lidong.zhong@suse.com> --- arch/powerpc/platforms/pseries/vio.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)