Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 8 Dec 2020 15:45:19 -0800
From:   Prashant Malani <pmalani@chromium.org>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     "open list:USB NETWORKING DRIVERS" <linux-usb@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Benson Leung <bleung@chromium.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: typec: Add bus type for plug alt modes
Message-ID: <CACeCKaehg=HTuQNLtQaJZWvTnOFYM9b1BWfM+WX_ebiZ-_i8JQ@mail.gmail.com>
References: <20201203030846.51669-1-pmalani@chromium.org>
 <20201208093734.GD680328@kuha.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201208093734.GD680328@kuha.fi.intel.com>
Precedence: bulk

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