LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] powerpc/vio: drop bus_type from parent device
@ 2020-04-06 15:57 Thadeu Lima de Souza Cascardo
  2020-07-30  1:28 ` Michael Ellerman
  0 siblings, 1 reply; 5+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2020-04-06 15:57 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: cascardo

Commit df44b479654f62b478c18ee4d8bc4e9f897a9844 ("kobject: return error
code if writing /sys/.../uevent fails") started returning failure when
writing to /sys/devices/vio/uevent.

This causes an early udevadm trigger to fail. On some installer versions of
Ubuntu, this will cause init to exit, thus panicing the system very early
during boot.

Removing the bus_type from the parent device will remove some of the extra
empty files from /sys/devices/vio/, but will keep the rest of the layout
for vio devices, keeping them under /sys/devices/vio/.

It has been tested that uevents for vio devices don't change after this
fix, they still contain MODALIAS.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Fixes: df44b479654f ("kobject: return error code if writing /sys/.../uevent fails")
---
 arch/powerpc/platforms/pseries/vio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
index 37f1f25ba804..a94dab3972a0 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -36,7 +36,6 @@ static struct vio_dev vio_bus_device  = { /* fake "parent" device */
 	.name = "vio",
 	.type = "",
 	.dev.init_name = "vio",
-	.dev.bus = &vio_bus_type,
 };
 
 #ifdef CONFIG_PPC_SMLPAR
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] powerpc/vio: drop bus_type from parent device
  2020-04-06 15:57 [PATCH v2] powerpc/vio: drop bus_type from parent device Thadeu Lima de Souza Cascardo
@ 2020-07-30  1:28 ` Michael Ellerman
  2020-07-30  5:37   ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Ellerman @ 2020-07-30  1:28 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo, linuxppc-dev
  Cc: cascardo, Peter Rajnoha, gregkh

[ Added Peter & Greg to Cc ]

Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> Commit df44b479654f62b478c18ee4d8bc4e9f897a9844 ("kobject: return error
> code if writing /sys/.../uevent fails") started returning failure when
> writing to /sys/devices/vio/uevent.
>
> This causes an early udevadm trigger to fail. On some installer versions of
> Ubuntu, this will cause init to exit, thus panicing the system very early
> during boot.
>
> Removing the bus_type from the parent device will remove some of the extra
> empty files from /sys/devices/vio/, but will keep the rest of the layout
> for vio devices, keeping them under /sys/devices/vio/.

What exactly does it change?

I'm finding it hard to evaluate if this change is going to cause a
regression somehow.

I'm also not clear on why removing the bus type is correct, apart from
whether it fixes the bug you're seeing.

> It has been tested that uevents for vio devices don't change after this
> fix, they still contain MODALIAS.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Fixes: df44b479654f ("kobject: return error code if writing /sys/.../uevent fails")

AFAICS there haven't been any other fixes for that commit. Do we know
why it is only vio that was affected? (possibly because it's a fake bus
to begin with?)

cheers

> diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
> index 37f1f25ba804..a94dab3972a0 100644
> --- a/arch/powerpc/platforms/pseries/vio.c
> +++ b/arch/powerpc/platforms/pseries/vio.c
> @@ -36,7 +36,6 @@ static struct vio_dev vio_bus_device  = { /* fake "parent" device */
>  	.name = "vio",
>  	.type = "",
>  	.dev.init_name = "vio",
> -	.dev.bus = &vio_bus_type,
>  };
>  
>  #ifdef CONFIG_PPC_SMLPAR
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] powerpc/vio: drop bus_type from parent device
  2020-07-30  1:28 ` Michael Ellerman
@ 2020-07-30  5:37   ` Greg KH
  2020-07-30 15:35     ` Thadeu Lima de Souza Cascardo
  2020-07-31  0:53     ` Michael Ellerman
  0 siblings, 2 replies; 5+ messages in thread
From: Greg KH @ 2020-07-30  5:37 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Thadeu Lima de Souza Cascardo, Peter Rajnoha, linuxppc-dev

On Thu, Jul 30, 2020 at 11:28:38AM +1000, Michael Ellerman wrote:
> [ Added Peter & Greg to Cc ]
> 
> Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> > Commit df44b479654f62b478c18ee4d8bc4e9f897a9844 ("kobject: return error
> > code if writing /sys/.../uevent fails") started returning failure when
> > writing to /sys/devices/vio/uevent.
> >
> > This causes an early udevadm trigger to fail. On some installer versions of
> > Ubuntu, this will cause init to exit, thus panicing the system very early
> > during boot.
> >
> > Removing the bus_type from the parent device will remove some of the extra
> > empty files from /sys/devices/vio/, but will keep the rest of the layout
> > for vio devices, keeping them under /sys/devices/vio/.
> 
> What exactly does it change?
> 
> I'm finding it hard to evaluate if this change is going to cause a
> regression somehow.
> 
> I'm also not clear on why removing the bus type is correct, apart from
> whether it fixes the bug you're seeing.
> 
> > It has been tested that uevents for vio devices don't change after this
> > fix, they still contain MODALIAS.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > Fixes: df44b479654f ("kobject: return error code if writing /sys/.../uevent fails")
> 
> AFAICS there haven't been any other fixes for that commit. Do we know
> why it is only vio that was affected? (possibly because it's a fake bus
> to begin with?)

So there was an error previously, the core was ignoring it, and now it
isn't and to fix that you want to remove describing what bus a device is
on?

Huh???

> 
> cheers
> 
> > diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
> > index 37f1f25ba804..a94dab3972a0 100644
> > --- a/arch/powerpc/platforms/pseries/vio.c
> > +++ b/arch/powerpc/platforms/pseries/vio.c
> > @@ -36,7 +36,6 @@ static struct vio_dev vio_bus_device  = { /* fake "parent" device */
> >  	.name = "vio",
> >  	.type = "",
> >  	.dev.init_name = "vio",
> > -	.dev.bus = &vio_bus_type,
> >  };

Wait, a static 'struct device'?  You all are playing with fire there.
That's a reference counted object, and should never be declared like
that at all.

I see you register it, but never unregister it, why?  Why is it even
needed?

And if you remove the bus type of it, it will show up in a different
part of sysfs, so I think this patch will show a user-visable change,
right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] powerpc/vio: drop bus_type from parent device
  2020-07-30  5:37   ` Greg KH
@ 2020-07-30 15:35     ` Thadeu Lima de Souza Cascardo
  2020-07-31  0:53     ` Michael Ellerman
  1 sibling, 0 replies; 5+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2020-07-30 15:35 UTC (permalink / raw)
  To: Greg KH; +Cc: Peter Rajnoha, linuxppc-dev

On Thu, Jul 30, 2020 at 07:37:16AM +0200, Greg KH wrote:
> On Thu, Jul 30, 2020 at 11:28:38AM +1000, Michael Ellerman wrote:
> > [ Added Peter & Greg to Cc ]
> > 
> > Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
> > > Commit df44b479654f62b478c18ee4d8bc4e9f897a9844 ("kobject: return error
> > > code if writing /sys/.../uevent fails") started returning failure when
> > > writing to /sys/devices/vio/uevent.
> > >
> > > This causes an early udevadm trigger to fail. On some installer versions of
> > > Ubuntu, this will cause init to exit, thus panicing the system very early
> > > during boot.
> > >
> > > Removing the bus_type from the parent device will remove some of the extra
> > > empty files from /sys/devices/vio/, but will keep the rest of the layout
> > > for vio devices, keeping them under /sys/devices/vio/.
> > 
> > What exactly does it change?
> > 
> > I'm finding it hard to evaluate if this change is going to cause a
> > regression somehow.
> > 
> > I'm also not clear on why removing the bus type is correct, apart from
> > whether it fixes the bug you're seeing.
> > 
> > > It has been tested that uevents for vio devices don't change after this
> > > fix, they still contain MODALIAS.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > Fixes: df44b479654f ("kobject: return error code if writing /sys/.../uevent fails")
> > 
> > AFAICS there haven't been any other fixes for that commit. Do we know
> > why it is only vio that was affected? (possibly because it's a fake bus
> > to begin with?)
> 
> So there was an error previously, the core was ignoring it, and now it
> isn't and to fix that you want to remove describing what bus a device is
> on?
> 
> Huh???
> 
> > 
> > cheers
> > 
> > > diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
> > > index 37f1f25ba804..a94dab3972a0 100644
> > > --- a/arch/powerpc/platforms/pseries/vio.c
> > > +++ b/arch/powerpc/platforms/pseries/vio.c
> > > @@ -36,7 +36,6 @@ static struct vio_dev vio_bus_device  = { /* fake "parent" device */
> > >  	.name = "vio",
> > >  	.type = "",
> > >  	.dev.init_name = "vio",
> > > -	.dev.bus = &vio_bus_type,
> > >  };
> 
> Wait, a static 'struct device'?  You all are playing with fire there.
> That's a reference counted object, and should never be declared like
> that at all.
> 
> I see you register it, but never unregister it, why?  Why is it even
> needed?
> 
> And if you remove the bus type of it, it will show up in a different
> part of sysfs, so I think this patch will show a user-visable change,
> right?
> 
> thanks,
> 
> greg k-h

As the comment says, it's a "fake parent device". There is a user-visible
change, which is removing some attributes from the object, but it's still
showing up on the same path.

Returning an error code like df44b479654f does is also a user visible change
and it breaks installer images that panic early on boot.

I could investigate an alternative here, which would be not fail when writing
to uevent for this specific fake device.

Cascardo.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] powerpc/vio: drop bus_type from parent device
  2020-07-30  5:37   ` Greg KH
  2020-07-30 15:35     ` Thadeu Lima de Souza Cascardo
@ 2020-07-31  0:53     ` Michael Ellerman
  1 sibling, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2020-07-31  0:53 UTC (permalink / raw)
  To: Greg KH
  Cc: Stephen Rothwell, Thadeu Lima de Souza Cascardo, Peter Rajnoha,
	linuxppc-dev

Greg KH <gregkh@linuxfoundation.org> writes:
> On Thu, Jul 30, 2020 at 11:28:38AM +1000, Michael Ellerman wrote:
>> [ Added Peter & Greg to Cc ]
>> 
>> Thadeu Lima de Souza Cascardo <cascardo@canonical.com> writes:
>> > Commit df44b479654f62b478c18ee4d8bc4e9f897a9844 ("kobject: return error
>> > code if writing /sys/.../uevent fails") started returning failure when
>> > writing to /sys/devices/vio/uevent.
>> >
>> > This causes an early udevadm trigger to fail. On some installer versions of
>> > Ubuntu, this will cause init to exit, thus panicing the system very early
>> > during boot.
>> >
>> > Removing the bus_type from the parent device will remove some of the extra
>> > empty files from /sys/devices/vio/, but will keep the rest of the layout
>> > for vio devices, keeping them under /sys/devices/vio/.
>> 
>> What exactly does it change?
>> 
>> I'm finding it hard to evaluate if this change is going to cause a
>> regression somehow.
>> 
>> I'm also not clear on why removing the bus type is correct, apart from
>> whether it fixes the bug you're seeing.
>> 
>> > It has been tested that uevents for vio devices don't change after this
>> > fix, they still contain MODALIAS.
>> >
>> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> > Fixes: df44b479654f ("kobject: return error code if writing /sys/.../uevent fails")
>> 
>> AFAICS there haven't been any other fixes for that commit. Do we know
>> why it is only vio that was affected? (possibly because it's a fake bus
>> to begin with?)
>
> So there was an error previously, the core was ignoring it, and now it
> isn't and to fix that you want to remove describing what bus a device is
> on?
>
> Huh???

Right.

Not to mention there are existing unfixed kernels out there, so whatever
userspace is crashing will need to be fixed for those anyway.

>> > diff --git a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
>> > index 37f1f25ba804..a94dab3972a0 100644
>> > --- a/arch/powerpc/platforms/pseries/vio.c
>> > +++ b/arch/powerpc/platforms/pseries/vio.c
>> > @@ -36,7 +36,6 @@ static struct vio_dev vio_bus_device  = { /* fake "parent" device */
>> >  	.name = "vio",
>> >  	.type = "",
>> >  	.dev.init_name = "vio",
>> > -	.dev.bus = &vio_bus_type,
>> >  };
>
> Wait, a static 'struct device'?  You all are playing with fire there.
> That's a reference counted object, and should never be declared like
> that at all.

Since 2005 :)

AC33c9bcf1 ("[PATCH] ppc64: tidy up vio devices fake parent")


> I see you register it, but never unregister it, why?  Why is it even
> needed?

I don't remember, if I ever knew.

The code says:

	/*
	 * The fake parent of all vio devices, just to give us
	 * a nice directory
	 */
	err = device_register(&vio_bus_device.dev);


But I suspect that may no longer be true.

ie. the devices show up in /sys/bus/vio/devices because they have
dev.bus = vio_bus_type, the fake parent doesn't seem to determine the
location.

> And if you remove the bus type of it, it will show up in a different
> part of sysfs, so I think this patch will show a user-visable change,
> right?

Yes I think so. But because it's a fake device to begin with that's
possibly OK.

I think we really need to get to the bottom of whether we need that
device at all, it seems like it might be left over cruft from the
ancient past.

I'll try and find time to work it out.

cheers

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-06 15:57 [PATCH v2] powerpc/vio: drop bus_type from parent device Thadeu Lima de Souza Cascardo
2020-07-30  1:28 ` Michael Ellerman
2020-07-30  5:37   ` Greg KH
2020-07-30 15:35     ` Thadeu Lima de Souza Cascardo
2020-07-31  0:53     ` Michael Ellerman

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git