[PATCH] usb: typec: Add bus type for plug alt modes

[PATCH] usb: typec: Add bus type for plug alt modes

[Prashant Malani]

Add the Type C bus for plug alternate modes which are being
registered via the Type C connector class. This ensures that udev events
get generated when plug alternate modes are registered (and not just for
partner/port alternate modes), even though the Type C bus doesn't link
plug alternate mode devices to alternate mode drivers.

Update the Type C bus documentation to mention that there
are alternate mode devices for plugs as well.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/driver-api/usb/typec_bus.rst | 6 +++---
 drivers/usb/typec/class.c                  | 8 ++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/usb/typec_bus.rst b/Documentation/driver-api/usb/typec_bus.rst
index 21c890ae17e5..7874d2f37d9f 100644
--- a/Documentation/driver-api/usb/typec_bus.rst
+++ b/Documentation/driver-api/usb/typec_bus.rst
@@ -15,9 +15,9 @@ modes by using the SVID and the mode number.
 
 :ref:`USB Type-C Connector Class <typec>` provides a device for every alternate
 mode a port supports, and separate device for every alternate mode the partner
-supports. The drivers for the alternate modes are bound to the partner alternate
-mode devices, and the port alternate mode devices must be handled by the port
-drivers.
+or cable plug supports. The drivers for the alternate modes are bound to the
+partner alternate mode devices, and the port alternate mode devices must be
+handled by the port drivers.
 
 When a new partner alternate mode device is registered, it is linked to the
 alternate mode device of the port that the partner is attached to, that has
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec707cb51..74061a699f16 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -478,8 +478,12 @@ typec_register_altmode(struct device *parent,
 	if (!is_port)
 		typec_altmode_set_partner(alt);
 
-	/* The partners are bind to drivers */
-	if (is_typec_partner(parent))
+	/*
+	 * The partners are bind to drivers.
+	 * Also set the bus field for plug alt modes so that the udev event occurs on device
+	 * registration.
+	 */
+	if (is_typec_partner(parent) || is_typec_plug(parent))
 		alt->adev.dev.bus = &typec_bus;
 
 	ret = device_register(&alt->adev.dev);
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Heikki Krogerus]

On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> Add the Type C bus for plug alternate modes which are being
> registered via the Type C connector class. This ensures that udev events
> get generated when plug alternate modes are registered (and not just for
> partner/port alternate modes), even though the Type C bus doesn't link
> plug alternate mode devices to alternate mode drivers.

I still don't understand how is the uevent related to the bus? If you
check the device_add() function, on line 2917, kobject_uevent() is
called unconditionally. The device does not need a bus for that event
to be generated.

Also, I don't understand how are the cable plug alt modes now
prevented from being bind to the alt mode drivers?

> Update the Type C bus documentation to mention that there
> are alternate mode devices for plugs as well.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  Documentation/driver-api/usb/typec_bus.rst | 6 +++---
>  drivers/usb/typec/class.c                  | 8 ++++++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/driver-api/usb/typec_bus.rst b/Documentation/driver-api/usb/typec_bus.rst
> index 21c890ae17e5..7874d2f37d9f 100644
> --- a/Documentation/driver-api/usb/typec_bus.rst
> +++ b/Documentation/driver-api/usb/typec_bus.rst
> @@ -15,9 +15,9 @@ modes by using the SVID and the mode number.
>  
>  :ref:`USB Type-C Connector Class <typec>` provides a device for every alternate
>  mode a port supports, and separate device for every alternate mode the partner
> -supports. The drivers for the alternate modes are bound to the partner alternate
> -mode devices, and the port alternate mode devices must be handled by the port
> -drivers.
> +or cable plug supports. The drivers for the alternate modes are bound to the
> +partner alternate mode devices, and the port alternate mode devices must be
> +handled by the port drivers.
>  
>  When a new partner alternate mode device is registered, it is linked to the
>  alternate mode device of the port that the partner is attached to, that has
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb51..74061a699f16 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -478,8 +478,12 @@ typec_register_altmode(struct device *parent,
>  	if (!is_port)
>  		typec_altmode_set_partner(alt);
>  
> -	/* The partners are bind to drivers */
> -	if (is_typec_partner(parent))
> +	/*
> +	 * The partners are bind to drivers.
> +	 * Also set the bus field for plug alt modes so that the udev event occurs on device
> +	 * registration.
> +	 */
> +	if (is_typec_partner(parent) || is_typec_plug(parent))
>  		alt->adev.dev.bus = &typec_bus;
>  
>  	ret = device_register(&alt->adev.dev);
> -- 
> 2.29.2.454.gaff20da3a2-goog

thanks,

-- 
heikki

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Prashant Malani]

Hi Heikki,

Thanks a lot for looking at the patch.

On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > Add the Type C bus for plug alternate modes which are being
> > registered via the Type C connector class. This ensures that udev events
> > get generated when plug alternate modes are registered (and not just for
> > partner/port alternate modes), even though the Type C bus doesn't link
> > plug alternate mode devices to alternate mode drivers.
>
> I still don't understand how is the uevent related to the bus? If you
> check the device_add() function, on line 2917, kobject_uevent() is
> called unconditionally. The device does not need a bus for that event
> to be generated.

My initial thought process was to see what is the difference in the adev device
initialization between partner altmode and plug altmode (the only difference I saw in
typec_register_altmode() was regarding the bus field).

Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
so we don't know if it succeeded or not.

In the case of cable plug altmode, I see it fail with the following error[1]:

[  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!

I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]

static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
{
	struct kobj_type *ktype = get_ktype(kobj);

	if (ktype == &device_ktype) {
		struct device *dev = kobj_to_dev(kobj);
		if (dev->bus)
			return 1;
		if (dev->class)
			return 1;
	}
	return 0;
}

So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
so this check likely returns 1 in that case.

In case the provided fix is not right or acceptable, an alternative I can think of is:
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index c13779ea3200..ecb4c7546aae 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -517,6 +517,9 @@ typec_register_altmode(struct device *parent,
        if (is_typec_partner(parent))
                alt->adev.dev.bus = &typec_bus;
 
+       if (is_typec_plug(parent))
+               alt->adev.dev.class = typec_class;
+
        ret = device_register(&alt->adev.dev);
        if (ret) {
                dev_err(parent, "failed to register alternate mode (%d)\n",

This too ensures that the filter function returns a 1.

Kindly LMK which way (if any) would you prefer.

>
> Also, I don't understand how are the cable plug alt modes now
> prevented from being bind to the alt mode drivers?

Sorry about this; I am unable to test this out. I just based the observation on the line in Documentation/driver-api/usb/typec_bus.rst
(Cable Plug Alternate Modes) : "The alternate mode drivers are not bound to cable plug alternate mode devices,
only to the partner alternate mode devices" . I don't completely understand the bus.c code yet, so assumed that the code
there checked for the partner type during bind attempts.

Of course, based on what the eventual solution is, this statement may no longer be required and I can remove it from the commit message <or>
I can amend the Documentation to specify that cable plug alt modes can bind to alt mode drivers.

Thanks,

-Prashant

[1] https://elixir.bootlin.com/linux/v5.10-rc7/source/lib/kobject_uevent.c#L516
[2] https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/core.c#L1840

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Heikki Krogerus]

On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> Thanks a lot for looking at the patch.
> 
> On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > Add the Type C bus for plug alternate modes which are being
> > > registered via the Type C connector class. This ensures that udev events
> > > get generated when plug alternate modes are registered (and not just for
> > > partner/port alternate modes), even though the Type C bus doesn't link
> > > plug alternate mode devices to alternate mode drivers.
> >
> > I still don't understand how is the uevent related to the bus? If you
> > check the device_add() function, on line 2917, kobject_uevent() is
> > called unconditionally. The device does not need a bus for that event
> > to be generated.
> 
> My initial thought process was to see what is the difference in the adev device
> initialization between partner altmode and plug altmode (the only difference I saw in
> typec_register_altmode() was regarding the bus field).
> 
> Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> so we don't know if it succeeded or not.
> 
> In the case of cable plug altmode, I see it fail with the following error[1]:
> 
> [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> 
> I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> 
> static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> {
> 	struct kobj_type *ktype = get_ktype(kobj);
> 
> 	if (ktype == &device_ktype) {
> 		struct device *dev = kobj_to_dev(kobj);
> 		if (dev->bus)
> 			return 1;
> 		if (dev->class)
> 			return 1;
> 	}
> 	return 0;
> }
> 
> So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> so this check likely returns 1 in that case.

OK. I understand the issue now. So I would say that the proper
solution to this problem is to link the alt modes with the class
instead of the bus. That is much smaller change IMO.


thanks,

-- 
heikki

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Prashant Malani]

Hi Heikki,

On Wed, Dec 9, 2020 at 8:14 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > Thanks a lot for looking at the patch.
> >
> > On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > > Add the Type C bus for plug alternate modes which are being
> > > > registered via the Type C connector class. This ensures that udev events
> > > > get generated when plug alternate modes are registered (and not just for
> > > > partner/port alternate modes), even though the Type C bus doesn't link
> > > > plug alternate mode devices to alternate mode drivers.
> > >
> > > I still don't understand how is the uevent related to the bus? If you
> > > check the device_add() function, on line 2917, kobject_uevent() is
> > > called unconditionally. The device does not need a bus for that event
> > > to be generated.
> >
> > My initial thought process was to see what is the difference in the adev device
> > initialization between partner altmode and plug altmode (the only difference I saw in
> > typec_register_altmode() was regarding the bus field).
> >
> > Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> > so we don't know if it succeeded or not.
> >
> > In the case of cable plug altmode, I see it fail with the following error[1]:
> >
> > [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> >
> > I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> >
> > static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> > {
> >       struct kobj_type *ktype = get_ktype(kobj);
> >
> >       if (ktype == &device_ktype) {
> >               struct device *dev = kobj_to_dev(kobj);
> >               if (dev->bus)
> >                       return 1;
> >               if (dev->class)
> >                       return 1;
> >       }
> >       return 0;
> > }
> >
> > So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> > so this check likely returns 1 in that case.
>
> OK. I understand the issue now. So I would say that the proper
> solution to this problem is to link the alt modes with the class
> instead of the bus. That is much smaller change IMO.

Got it. Just to confirm that I understand correctly, do you mean:
1. Only cable plug alt modes should be linked with the class instead of the bus.

<or>

2. All alt modes (cable plug, partner, port) should be linked with the
class instead of the bus

My initial interpretation is 1.) since the bus linkage would be
necessary to match alt mode drivers to partner alt mode devices.
But, my understanding of the bus code is limited so I could be wrong;
could you kindly clarify?

Thanks,

-Prashant

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Heikki Krogerus]

Hi Prashant,

On Wed, Dec 09, 2020 at 08:22:52AM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> On Wed, Dec 9, 2020 at 8:14 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > Thanks a lot for looking at the patch.
> > >
> > > On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > > > Add the Type C bus for plug alternate modes which are being
> > > > > registered via the Type C connector class. This ensures that udev events
> > > > > get generated when plug alternate modes are registered (and not just for
> > > > > partner/port alternate modes), even though the Type C bus doesn't link
> > > > > plug alternate mode devices to alternate mode drivers.
> > > >
> > > > I still don't understand how is the uevent related to the bus? If you
> > > > check the device_add() function, on line 2917, kobject_uevent() is
> > > > called unconditionally. The device does not need a bus for that event
> > > > to be generated.
> > >
> > > My initial thought process was to see what is the difference in the adev device
> > > initialization between partner altmode and plug altmode (the only difference I saw in
> > > typec_register_altmode() was regarding the bus field).
> > >
> > > Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> > > so we don't know if it succeeded or not.
> > >
> > > In the case of cable plug altmode, I see it fail with the following error[1]:
> > >
> > > [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> > >
> > > I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> > >
> > > static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> > > {
> > >       struct kobj_type *ktype = get_ktype(kobj);
> > >
> > >       if (ktype == &device_ktype) {
> > >               struct device *dev = kobj_to_dev(kobj);
> > >               if (dev->bus)
> > >                       return 1;
> > >               if (dev->class)
> > >                       return 1;
> > >       }
> > >       return 0;
> > > }
> > >
> > > So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> > > so this check likely returns 1 in that case.
> >
> > OK. I understand the issue now. So I would say that the proper
> > solution to this problem is to link the alt modes with the class
> > instead of the bus. That is much smaller change IMO.
> 
> Got it. Just to confirm that I understand correctly, do you mean:
> 1. Only cable plug alt modes should be linked with the class instead of the bus.
> 
> <or>
> 
> 2. All alt modes (cable plug, partner, port) should be linked with the
> class instead of the bus
> 
> My initial interpretation is 1.) since the bus linkage would be
> necessary to match alt mode drivers to partner alt mode devices.
> But, my understanding of the bus code is limited so I could be wrong;
> could you kindly clarify?

We don't need to care about the bus here. A device can be part of a
bus and a class at the same time. I don't think there is any reason to
limit the class to only plug alt modes, so let's just assign it to all
of them.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Prashant Malani]

Hi Heikki,

On Wed, Dec 9, 2020 at 9:15 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Prashant,
>
> On Wed, Dec 09, 2020 at 08:22:52AM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > On Wed, Dec 9, 2020 at 8:14 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks a lot for looking at the patch.
> > > >
> > > > On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > > > > Add the Type C bus for plug alternate modes which are being
> > > > > > registered via the Type C connector class. This ensures that udev events
> > > > > > get generated when plug alternate modes are registered (and not just for
> > > > > > partner/port alternate modes), even though the Type C bus doesn't link
> > > > > > plug alternate mode devices to alternate mode drivers.
> > > > >
> > > > > I still don't understand how is the uevent related to the bus? If you
> > > > > check the device_add() function, on line 2917, kobject_uevent() is
> > > > > called unconditionally. The device does not need a bus for that event
> > > > > to be generated.
> > > >
> > > > My initial thought process was to see what is the difference in the adev device
> > > > initialization between partner altmode and plug altmode (the only difference I saw in
> > > > typec_register_altmode() was regarding the bus field).
> > > >
> > > > Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> > > > so we don't know if it succeeded or not.
> > > >
> > > > In the case of cable plug altmode, I see it fail with the following error[1]:
> > > >
> > > > [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> > > >
> > > > I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> > > >
> > > > static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> > > > {
> > > >       struct kobj_type *ktype = get_ktype(kobj);
> > > >
> > > >       if (ktype == &device_ktype) {
> > > >               struct device *dev = kobj_to_dev(kobj);
> > > >               if (dev->bus)
> > > >                       return 1;
> > > >               if (dev->class)
> > > >                       return 1;
> > > >       }
> > > >       return 0;
> > > > }
> > > >
> > > > So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> > > > so this check likely returns 1 in that case.
> > >
> > > OK. I understand the issue now. So I would say that the proper
> > > solution to this problem is to link the alt modes with the class
> > > instead of the bus. That is much smaller change IMO.
> >
> > Got it. Just to confirm that I understand correctly, do you mean:
> > 1. Only cable plug alt modes should be linked with the class instead of the bus.
> >
> > <or>
> >
> > 2. All alt modes (cable plug, partner, port) should be linked with the
> > class instead of the bus
> >
> > My initial interpretation is 1.) since the bus linkage would be
> > necessary to match alt mode drivers to partner alt mode devices.
> > But, my understanding of the bus code is limited so I could be wrong;
> > could you kindly clarify?
>
> We don't need to care about the bus here. A device can be part of a
> bus and a class at the same time. I don't think there is any reason to
> limit the class to only plug alt modes, so let's just assign it to all
> of them.

I had actually tried this earlier, but here we run into errors.
If we always set the class, then "partner" altmode device creation
fails ("port" altmode creation will likely also fail, but I haven't
verified that)

The issue is that if we set both "class" and "bus", the device_add()
[1] code tries to create the "subsystem" symlink in the altmode
device's sysfs entry twice.

The first creation is in the call to device_add_class_symlinks()[2]
which creates a "subsystem" file [3]. Note that if "class" is not set,
this code doesn't execute.
Next is the call to bus_add_device() [4] which again tries to create
the "subsystem" symlink [5] and fails since it already exists; this
leads to failure.

There are 2 solutions I can see:
1. Only set class for cable plug alt modes (which won't have a bus
set). This will avoid the double "subsystem" sysfs file creation.
2. Change the bus_add_device() code to:
    a. use the _nowarn() option of the symlink create function which
prevents the warn stack traces on -EEXIST error, and
    b. check for -EEXIST return value and don't fail if so.

2.) Sounds good to me, but I'm not sure if it's alright to continue if
a "subsystem" symlink already exists.

If 2.) is OK with you, I will upload a patch to implement it and make
a series with that patch and this one.

Best regards,

-Prashant

[1]: https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/core.c#L2884
[2]: https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/core.c#L2955
[3]: https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/core.c#L2722
[4]: https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/core.c#L2961
[5]: https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/base/bus.c#L471

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Prashant Malani]

On Wed, Dec 9, 2020 at 2:59 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Heikki,
>
> On Wed, Dec 9, 2020 at 9:15 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Prashant,
> >
> > On Wed, Dec 09, 2020 at 08:22:52AM -0800, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > On Wed, Dec 9, 2020 at 8:14 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks a lot for looking at the patch.
> > > > >
> > > > > On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > > > >
> > > > > > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > > > > > Add the Type C bus for plug alternate modes which are being
> > > > > > > registered via the Type C connector class. This ensures that udev events
> > > > > > > get generated when plug alternate modes are registered (and not just for
> > > > > > > partner/port alternate modes), even though the Type C bus doesn't link
> > > > > > > plug alternate mode devices to alternate mode drivers.
> > > > > >
> > > > > > I still don't understand how is the uevent related to the bus? If you
> > > > > > check the device_add() function, on line 2917, kobject_uevent() is
> > > > > > called unconditionally. The device does not need a bus for that event
> > > > > > to be generated.
> > > > >
> > > > > My initial thought process was to see what is the difference in the adev device
> > > > > initialization between partner altmode and plug altmode (the only difference I saw in
> > > > > typec_register_altmode() was regarding the bus field).
> > > > >
> > > > > Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> > > > > so we don't know if it succeeded or not.
> > > > >
> > > > > In the case of cable plug altmode, I see it fail with the following error[1]:
> > > > >
> > > > > [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> > > > >
> > > > > I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> > > > >
> > > > > static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> > > > > {
> > > > >       struct kobj_type *ktype = get_ktype(kobj);
> > > > >
> > > > >       if (ktype == &device_ktype) {
> > > > >               struct device *dev = kobj_to_dev(kobj);
> > > > >               if (dev->bus)
> > > > >                       return 1;
> > > > >               if (dev->class)
> > > > >                       return 1;
> > > > >       }
> > > > >       return 0;
> > > > > }
> > > > >
> > > > > So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> > > > > so this check likely returns 1 in that case.
> > > >
> > > > OK. I understand the issue now. So I would say that the proper
> > > > solution to this problem is to link the alt modes with the class
> > > > instead of the bus. That is much smaller change IMO.
> > >
> > > Got it. Just to confirm that I understand correctly, do you mean:
> > > 1. Only cable plug alt modes should be linked with the class instead of the bus.
> > >
> > > <or>
> > >
> > > 2. All alt modes (cable plug, partner, port) should be linked with the
> > > class instead of the bus
> > >
> > > My initial interpretation is 1.) since the bus linkage would be
> > > necessary to match alt mode drivers to partner alt mode devices.
> > > But, my understanding of the bus code is limited so I could be wrong;
> > > could you kindly clarify?
> >
> > We don't need to care about the bus here. A device can be part of a
> > bus and a class at the same time. I don't think there is any reason to
> > limit the class to only plug alt modes, so let's just assign it to all
> > of them.
>
> I had actually tried this earlier, but here we run into errors.
> If we always set the class, then "partner" altmode device creation
> fails ("port" altmode creation will likely also fail, but I haven't
> verified that)
>
> The issue is that if we set both "class" and "bus", the device_add()
> [1] code tries to create the "subsystem" symlink in the altmode
> device's sysfs entry twice.
>
> The first creation is in the call to device_add_class_symlinks()[2]
> which creates a "subsystem" file [3]. Note that if "class" is not set,
> this code doesn't execute.
> Next is the call to bus_add_device() [4] which again tries to create
> the "subsystem" symlink [5] and fails since it already exists; this
> leads to failure.
>
> There are 2 solutions I can see:
> 1. Only set class for cable plug alt modes (which won't have a bus
> set). This will avoid the double "subsystem" sysfs file creation.
> 2. Change the bus_add_device() code to:
>     a. use the _nowarn() option of the symlink create function which
> prevents the warn stack traces on -EEXIST error, and
>     b. check for -EEXIST return value and don't fail if so.
>
> 2.) Sounds good to me, but I'm not sure if it's alright to continue if
> a "subsystem" symlink already exists.

It looks like the "subsystem" name depends on the bus_type.name  and
class.name (for bus and class respectively).
So it is possible the two symlinks will not point to the same location
(For example, the class for typec is "typec_mux"
but the bus is simply "typec").

Given this, it sounds like option 1.) might be better, but I'll defer
to your suggestions.


Best regards,

-Prashant

Re: [PATCH] usb: typec: Add bus type for plug alt modes

[Heikki Krogerus]

On Wed, Dec 09, 2020 at 03:47:17PM -0800, Prashant Malani wrote:
> On Wed, Dec 9, 2020 at 2:59 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Heikki,
> >
> > On Wed, Dec 9, 2020 at 9:15 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > On Wed, Dec 09, 2020 at 08:22:52AM -0800, Prashant Malani wrote:
> > > > Hi Heikki,
> > > >
> > > > On Wed, Dec 9, 2020 at 8:14 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Tue, Dec 08, 2020 at 03:45:19PM -0800, Prashant Malani wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks a lot for looking at the patch.
> > > > > >
> > > > > > On Tue, Dec 8, 2020 at 1:37 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Wed, Dec 02, 2020 at 07:08:47PM -0800, Prashant Malani wrote:
> > > > > > > > Add the Type C bus for plug alternate modes which are being
> > > > > > > > registered via the Type C connector class. This ensures that udev events
> > > > > > > > get generated when plug alternate modes are registered (and not just for
> > > > > > > > partner/port alternate modes), even though the Type C bus doesn't link
> > > > > > > > plug alternate mode devices to alternate mode drivers.
> > > > > > >
> > > > > > > I still don't understand how is the uevent related to the bus? If you
> > > > > > > check the device_add() function, on line 2917, kobject_uevent() is
> > > > > > > called unconditionally. The device does not need a bus for that event
> > > > > > > to be generated.
> > > > > >
> > > > > > My initial thought process was to see what is the difference in the adev device
> > > > > > initialization between partner altmode and plug altmode (the only difference I saw in
> > > > > > typec_register_altmode() was regarding the bus field).
> > > > > >
> > > > > > Yes, kobject_uevent() is called unconditionally, but it's return value isn't checked,
> > > > > > so we don't know if it succeeded or not.
> > > > > >
> > > > > > In the case of cable plug altmode, I see it fail with the following error[1]:
> > > > > >
> > > > > > [  114.431409] kobject: 'port1-plug0.0' (000000004ad42956): kobject_uevent_env: filter function caused the event to drop!
> > > > > >
> > > > > > I think the filter function which is called is this one: drivers/base/core.c: dev_uevent_filter() [2]
> > > > > >
> > > > > > static int dev_uevent_filter(struct kset *kset, struct kobject *kobj)
> > > > > > {
> > > > > >       struct kobj_type *ktype = get_ktype(kobj);
> > > > > >
> > > > > >       if (ktype == &device_ktype) {
> > > > > >               struct device *dev = kobj_to_dev(kobj);
> > > > > >               if (dev->bus)
> > > > > >                       return 1;
> > > > > >               if (dev->class)
> > > > > >                       return 1;
> > > > > >       }
> > > > > >       return 0;
> > > > > > }
> > > > > >
> > > > > > So, both the "if (dev->bus)" and "if (dev->class)" checks are failing here. In the case of partner alt modes, bus is set by the class.c code
> > > > > > so this check likely returns 1 in that case.
> > > > >
> > > > > OK. I understand the issue now. So I would say that the proper
> > > > > solution to this problem is to link the alt modes with the class
> > > > > instead of the bus. That is much smaller change IMO.
> > > >
> > > > Got it. Just to confirm that I understand correctly, do you mean:
> > > > 1. Only cable plug alt modes should be linked with the class instead of the bus.
> > > >
> > > > <or>
> > > >
> > > > 2. All alt modes (cable plug, partner, port) should be linked with the
> > > > class instead of the bus
> > > >
> > > > My initial interpretation is 1.) since the bus linkage would be
> > > > necessary to match alt mode drivers to partner alt mode devices.
> > > > But, my understanding of the bus code is limited so I could be wrong;
> > > > could you kindly clarify?
> > >
> > > We don't need to care about the bus here. A device can be part of a
> > > bus and a class at the same time. I don't think there is any reason to
> > > limit the class to only plug alt modes, so let's just assign it to all
> > > of them.
> >
> > I had actually tried this earlier, but here we run into errors.
> > If we always set the class, then "partner" altmode device creation
> > fails ("port" altmode creation will likely also fail, but I haven't
> > verified that)
> >
> > The issue is that if we set both "class" and "bus", the device_add()
> > [1] code tries to create the "subsystem" symlink in the altmode
> > device's sysfs entry twice.
> >
> > The first creation is in the call to device_add_class_symlinks()[2]
> > which creates a "subsystem" file [3]. Note that if "class" is not set,
> > this code doesn't execute.
> > Next is the call to bus_add_device() [4] which again tries to create
> > the "subsystem" symlink [5] and fails since it already exists; this
> > leads to failure.
> >
> > There are 2 solutions I can see:
> > 1. Only set class for cable plug alt modes (which won't have a bus
> > set). This will avoid the double "subsystem" sysfs file creation.
> > 2. Change the bus_add_device() code to:
> >     a. use the _nowarn() option of the symlink create function which
> > prevents the warn stack traces on -EEXIST error, and
> >     b. check for -EEXIST return value and don't fail if so.
> >
> > 2.) Sounds good to me, but I'm not sure if it's alright to continue if
> > a "subsystem" symlink already exists.
> 
> It looks like the "subsystem" name depends on the bus_type.name  and
> class.name (for bus and class respectively).
> So it is possible the two symlinks will not point to the same location
> (For example, the class for typec is "typec_mux"
> but the bus is simply "typec").
> 
> Given this, it sounds like option 1.) might be better, but I'll defer
> to your suggestions.

OK. Let's go with that option.

thanks,

-- 
heikki