[PATCH 0/4] usb: Use notifier for linking Type C ports.

[PATCH 0/4] usb: Use notifier for linking Type C ports.

[Prashant Malani]

This series resolves the cyclic dependency error which was introduced by
commit 63cd78617350 ("usb: Link the ports to the connectors they are
attached to") which lead to it being reverted. The approach here is to
use a notifier to link a new Type C port to pre-existing USB ports
instead of calling an iterator of usb ports from the Type C connector
class. This allows commit 63cd78617350 ("usb: Link the ports to the
connectors they are attached to") to then be submitted without any
depmod cyclic dependency error.

The final patch removes the usb port iterator since it is no longer
needed.

Heikki Krogerus (1):
  usb: Link the ports to the connectors they are attached to

Prashant Malani (3):
  usb: typec: Add port registration notifier
  usb: Use notifier to link Type C ports
  Revert "usb: Iterator for ports"

 Documentation/ABI/testing/sysfs-bus-usb |  9 +++++
 drivers/usb/core/hub.h                  |  3 ++
 drivers/usb/core/port.c                 | 20 +++++++++++
 drivers/usb/core/usb.c                  | 46 -------------------------
 drivers/usb/typec/class.c               | 33 ++++++++++++++++--
 drivers/usb/typec/class.h               |  1 -
 drivers/usb/typec/port-mapper.c         | 41 ----------------------
 include/linux/usb.h                     |  9 -----
 include/linux/usb/typec.h               | 13 +++++++
 9 files changed, 75 insertions(+), 100 deletions(-)

-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 1/4] usb: typec: Add port registration notifier

[Prashant Malani]

Introduce a blocking notifier to be called when a new Type C port gets
registered with the connector class framework.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
NOTE: typec_port_registration_register_notify() is a bit long,
so please let me know if you have any shorter suggestions for naming
this function.

 drivers/usb/typec/class.c | 30 ++++++++++++++++++++++++++++++
 include/linux/usb/typec.h | 13 +++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index aeef453aa658..14b82109b0f5 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -16,6 +16,8 @@
 #include "bus.h"
 #include "class.h"
 
+static BLOCKING_NOTIFIER_HEAD(typec_port_registration_notifier);
+
 static DEFINE_IDA(typec_index_ida);
 
 struct class typec_class = {
@@ -1979,6 +1981,32 @@ void typec_port_register_altmodes(struct typec_port *port,
 }
 EXPORT_SYMBOL_GPL(typec_port_register_altmodes);
 
+/**
+ *  typec_port_registration_register_notify - Register a notifier for Type C port registration.
+ *  @nb: notifier block to signal
+ *
+ *  This function allows callers to get a notification when a Type C port is registered with
+ *  the connector class.
+ */
+int typec_port_registration_register_notify(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&typec_port_registration_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(typec_port_registration_register_notify);
+
+/**
+ *  typec_port_registration_unregister_notify - Unregister a notifier for Type C port registration.
+ *  @nb: notifier block to unregister
+ *
+ *  This function allows callers to unregister notifiers which were previously registered using
+ *  typec_port_registration_register_notify().
+ */
+int typec_port_registration_unregister_notify(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&typec_port_registration_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(typec_port_registration_unregister_notify);
+
 /**
  * typec_register_port - Register a USB Type-C Port
  * @parent: Parent device
@@ -2086,6 +2114,8 @@ struct typec_port *typec_register_port(struct device *parent,
 	if (ret)
 		dev_warn(&port->dev, "failed to create symlinks (%d)\n", ret);
 
+	blocking_notifier_call_chain(&typec_port_registration_notifier, 0, port);
+
 	return port;
 }
 EXPORT_SYMBOL_GPL(typec_register_port);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index e2e44bb1dad8..398317835f24 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -3,6 +3,7 @@
 #ifndef __LINUX_USB_TYPEC_H
 #define __LINUX_USB_TYPEC_H
 
+#include <linux/notifier.h>
 #include <linux/types.h>
 
 /* USB Type-C Specification releases */
@@ -308,6 +309,8 @@ int typec_get_negotiated_svdm_version(struct typec_port *port);
 #if IS_REACHABLE(CONFIG_TYPEC)
 int typec_link_port(struct device *port);
 void typec_unlink_port(struct device *port);
+int typec_port_registration_register_notify(struct notifier_block *nb);
+int typec_port_registration_unregister_notify(struct notifier_block *nb);
 #else
 static inline int typec_link_port(struct device *port)
 {
@@ -315,6 +318,16 @@ static inline int typec_link_port(struct device *port)
 }
 
 static inline void typec_unlink_port(struct device *port) { }
+
+int typec_port_registration_register_notify(struct notifier_block *nb)
+{
+	return 0;
+}
+
+int typec_port_registration_unregister_notify(struct notifier_block *nb)
+{
+	return 0;
+}
 #endif
 
 #endif /* __LINUX_USB_TYPEC_H */
-- 
2.34.0.rc2.393.gf8c9666880-goog

Re: [PATCH 1/4] usb: typec: Add port registration notifier

[Prashant Malani]

On Wed, Nov 24, 2021 at 3:11 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Introduce a blocking notifier to be called when a new Type C port gets
> registered with the connector class framework.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> NOTE: typec_port_registration_register_notify() is a bit long,
> so please let me know if you have any shorter suggestions for naming
> this function.
>
>  drivers/usb/typec/class.c | 30 ++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h | 13 +++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index aeef453aa658..14b82109b0f5 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -16,6 +16,8 @@
>  #include "bus.h"
>  #include "class.h"
>
> +static BLOCKING_NOTIFIER_HEAD(typec_port_registration_notifier);
> +
>  static DEFINE_IDA(typec_index_ida);
>
>  struct class typec_class = {
> @@ -1979,6 +1981,32 @@ void typec_port_register_altmodes(struct typec_port *port,
>  }
>  EXPORT_SYMBOL_GPL(typec_port_register_altmodes);
>
> +/**
> + *  typec_port_registration_register_notify - Register a notifier for Type C port registration.
> + *  @nb: notifier block to signal
> + *
> + *  This function allows callers to get a notification when a Type C port is registered with
> + *  the connector class.
> + */
> +int typec_port_registration_register_notify(struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_register(&typec_port_registration_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(typec_port_registration_register_notify);
> +
> +/**
> + *  typec_port_registration_unregister_notify - Unregister a notifier for Type C port registration.
> + *  @nb: notifier block to unregister
> + *
> + *  This function allows callers to unregister notifiers which were previously registered using
> + *  typec_port_registration_register_notify().
> + */
> +int typec_port_registration_unregister_notify(struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_unregister(&typec_port_registration_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(typec_port_registration_unregister_notify);
> +
>  /**
>   * typec_register_port - Register a USB Type-C Port
>   * @parent: Parent device
> @@ -2086,6 +2114,8 @@ struct typec_port *typec_register_port(struct device *parent,
>         if (ret)
>                 dev_warn(&port->dev, "failed to create symlinks (%d)\n", ret);
>
> +       blocking_notifier_call_chain(&typec_port_registration_notifier, 0, port);
> +
>         return port;
>  }
>  EXPORT_SYMBOL_GPL(typec_register_port);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index e2e44bb1dad8..398317835f24 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -3,6 +3,7 @@
>  #ifndef __LINUX_USB_TYPEC_H
>  #define __LINUX_USB_TYPEC_H
>
> +#include <linux/notifier.h>
>  #include <linux/types.h>
>
>  /* USB Type-C Specification releases */
> @@ -308,6 +309,8 @@ int typec_get_negotiated_svdm_version(struct typec_port *port);
>  #if IS_REACHABLE(CONFIG_TYPEC)
>  int typec_link_port(struct device *port);
>  void typec_unlink_port(struct device *port);
> +int typec_port_registration_register_notify(struct notifier_block *nb);
> +int typec_port_registration_unregister_notify(struct notifier_block *nb);
>  #else
>  static inline int typec_link_port(struct device *port)
>  {
> @@ -315,6 +318,16 @@ static inline int typec_link_port(struct device *port)
>  }
>
>  static inline void typec_unlink_port(struct device *port) { }
> +
> +int typec_port_registration_register_notify(struct notifier_block *nb)
> +{
> +       return 0;
> +}
> +
> +int typec_port_registration_unregister_notify(struct notifier_block *nb)
> +{
> +       return 0;
> +}
>  #endif

Oops, looks like I forgot to static inline these....

Also, probably better to return -ENODEV for these calls?

Sorry about that; I'll fix it up and send another version.

Thanks,

[PATCH 2/4] usb: Use notifier to link Type C ports

[Prashant Malani]

Instead of using a helper function to register pre-existing USB ports to
a new type C port, have each port register a notifier with the Type C
connector class, and use that to perform the linking when a new Type C
port is registered.

This avoid a cyclic dependency from a later change which is needed to
link a new USB port to a pre-existing Type C port. Some context on this
is in here [1]; in summary, typec_link_ports() introduces a dependency
from typec -> usbcore. However, commit 63cd78617350 ("usb: Link the
ports to the connectors they are attached to") then introduces a
dependency from usbcore -> typec. In order to allow that commit without
creating the cyclic dependency, we use a notifier here instead of
typec_link_ports().

Since we don't need the helper typec_link_ports() function anymore,
remove it and its related functions from port-mapper code.

[1]:
https://lore.kernel.org/all/20210412213655.3776e15e@canb.auug.org.au/

Cc: Benson Leung <bleung@chromium.org>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/usb/core/hub.h          |  3 +++
 drivers/usb/core/port.c         | 18 +++++++++++++++
 drivers/usb/typec/class.c       |  5 +---
 drivers/usb/typec/class.h       |  1 -
 drivers/usb/typec/port-mapper.c | 41 ---------------------------------
 5 files changed, 22 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 22ea1f4f2d66..d09a8c9c1b4e 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -11,6 +11,7 @@
  *  move struct usb_hub to this file.
  */
 
+#include <linux/notifier.h>
 #include <linux/usb.h>
 #include <linux/usb/ch11.h>
 #include <linux/usb/hcd.h>
@@ -89,6 +90,7 @@ struct usb_hub {
  * @is_superspeed cache super-speed status
  * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted.
  * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted.
+ * @typec_nb: notifier called when a Type C port is registered.
  */
 struct usb_port {
 	struct usb_device *child;
@@ -105,6 +107,7 @@ struct usb_port {
 	unsigned int is_superspeed:1;
 	unsigned int usb3_lpm_u1_permit:1;
 	unsigned int usb3_lpm_u2_permit:1;
+	struct notifier_block typec_nb;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index dfcca9c876c7..53a64ce76183 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -9,6 +9,7 @@
 
 #include <linux/slab.h>
 #include <linux/pm_qos.h>
+#include <linux/usb/typec.h>
 
 #include "hub.h"
 
@@ -528,6 +529,14 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
 		link_peers_report(port_dev, peer);
 }
 
+static int usb_port_link_typec_port(struct notifier_block *nb, unsigned long event, void *ptr)
+{
+	struct usb_port *port_dev = container_of(nb, struct usb_port, typec_nb);
+
+	typec_link_port(&port_dev->dev);
+	return NOTIFY_OK;
+}
+
 int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 {
 	struct usb_port *port_dev;
@@ -577,6 +586,14 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 
 	find_and_link_peer(hub, port1);
 
+	/*
+	 * In some cases, the Type C port gets registered later, so
+	 * register a Type C notifier so that we can link the ports
+	 * later too.
+	 */
+	port_dev->typec_nb.notifier_call = usb_port_link_typec_port;
+	typec_port_registration_register_notify(&port_dev->typec_nb);
+
 	/*
 	 * Enable runtime pm and hold a refernce that hub_configure()
 	 * will drop once the PM_QOS_NO_POWER_OFF flag state has been set
@@ -616,6 +633,7 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
 	struct usb_port *port_dev = hub->ports[port1 - 1];
 	struct usb_port *peer;
 
+	typec_port_registration_unregister_notify(&port_dev->typec_nb);
 	peer = port_dev->peer;
 	if (peer)
 		unlink_peers(port_dev, peer);
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 14b82109b0f5..92d03eb65f12 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -2110,10 +2110,7 @@ struct typec_port *typec_register_port(struct device *parent,
 		return ERR_PTR(ret);
 	}
 
-	ret = typec_link_ports(port);
-	if (ret)
-		dev_warn(&port->dev, "failed to create symlinks (%d)\n", ret);
-
+	/* Used to allow USB ports to register to this Type C port */
 	blocking_notifier_call_chain(&typec_port_registration_notifier, 0, port);
 
 	return port;
diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
index aef03eb7e152..517236935a55 100644
--- a/drivers/usb/typec/class.h
+++ b/drivers/usb/typec/class.h
@@ -79,7 +79,6 @@ extern const struct device_type typec_port_dev_type;
 extern struct class typec_mux_class;
 extern struct class typec_class;
 
-int typec_link_ports(struct typec_port *connector);
 void typec_unlink_ports(struct typec_port *connector);
 
 #endif /* __USB_TYPEC_CLASS__ */
diff --git a/drivers/usb/typec/port-mapper.c b/drivers/usb/typec/port-mapper.c
index 9b0991bdf391..5597291bd769 100644
--- a/drivers/usb/typec/port-mapper.c
+++ b/drivers/usb/typec/port-mapper.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/usb.h>
 #include <linux/usb/typec.h>
 
 #include "class.h"
@@ -220,46 +219,6 @@ void typec_unlink_port(struct device *port)
 }
 EXPORT_SYMBOL_GPL(typec_unlink_port);
 
-static int each_port(struct device *port, void *connector)
-{
-	struct port_node *node;
-	int ret;
-
-	node = create_port_node(port);
-	if (IS_ERR(node))
-		return PTR_ERR(node);
-
-	if (!connector_match(connector, node)) {
-		remove_port_node(node);
-		return 0;
-	}
-
-	ret = link_port(to_typec_port(connector), node);
-	if (ret) {
-		remove_port_node(node->pld);
-		return ret;
-	}
-
-	get_device(connector);
-
-	return 0;
-}
-
-int typec_link_ports(struct typec_port *con)
-{
-	int ret = 0;
-
-	con->pld = get_pld(&con->dev);
-	if (!con->pld)
-		return 0;
-
-	ret = usb_for_each_port(&con->dev, each_port);
-	if (ret)
-		typec_unlink_ports(con);
-
-	return ret;
-}
-
 void typec_unlink_ports(struct typec_port *con)
 {
 	struct port_node *node;
-- 
2.34.0.rc2.393.gf8c9666880-goog

Re: [PATCH 2/4] usb: Use notifier to link Type C ports

[kernel test robot]

Hi Prashant,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linux/master linus/master peter-chen-usb/for-usb-next v5.16-rc2 next-20211124]
[cannot apply to balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Prashant-Malani/usb-Use-notifier-for-linking-Type-C-ports/20211125-071439
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-defconfig (https://download.01.org/0day-ci/archive/20211125/202111251010.fxed9VtQ-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/d56a1c2271ef9c1877e9400fb1adc5adbb278e51
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Prashant-Malani/usb-Use-notifier-for-linking-Type-C-ports/20211125-071439
        git checkout d56a1c2271ef9c1877e9400fb1adc5adbb278e51
        # save the config file to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/usb/core/port.c:12:
>> include/linux/usb/typec.h:322:5: warning: no previous prototype for 'typec_port_registration_register_notify' [-Wmissing-prototypes]
     322 | int typec_port_registration_register_notify(struct notifier_block *nb)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/usb/typec.h:327:5: warning: no previous prototype for 'typec_port_registration_unregister_notify' [-Wmissing-prototypes]
     327 | int typec_port_registration_unregister_notify(struct notifier_block *nb)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/typec_port_registration_register_notify +322 include/linux/usb/typec.h

ed296d8d0a92758 Prashant Malani 2021-11-24  321  
ed296d8d0a92758 Prashant Malani 2021-11-24 @322  int typec_port_registration_register_notify(struct notifier_block *nb)
ed296d8d0a92758 Prashant Malani 2021-11-24  323  {
ed296d8d0a92758 Prashant Malani 2021-11-24  324  	return 0;
ed296d8d0a92758 Prashant Malani 2021-11-24  325  }
ed296d8d0a92758 Prashant Malani 2021-11-24  326  
ed296d8d0a92758 Prashant Malani 2021-11-24 @327  int typec_port_registration_unregister_notify(struct notifier_block *nb)
ed296d8d0a92758 Prashant Malani 2021-11-24  328  {
ed296d8d0a92758 Prashant Malani 2021-11-24  329  	return 0;
ed296d8d0a92758 Prashant Malani 2021-11-24  330  }
ae196ddb0d3186b Heikki Krogerus 2021-04-07  331  #endif
ae196ddb0d3186b Heikki Krogerus 2021-04-07  332  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 2/4] usb: Use notifier to link Type C ports

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3036 bytes --]

Hi Prashant,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linux/master linus/master peter-chen-usb/for-usb-next v5.16-rc2 next-20211124]
[cannot apply to balbi-usb/testing/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Prashant-Malani/usb-Use-notifier-for-linking-Type-C-ports/20211125-071439
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-defconfig (https://download.01.org/0day-ci/archive/20211125/202111251010.fxed9VtQ-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/d56a1c2271ef9c1877e9400fb1adc5adbb278e51
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Prashant-Malani/usb-Use-notifier-for-linking-Type-C-ports/20211125-071439
        git checkout d56a1c2271ef9c1877e9400fb1adc5adbb278e51
        # save the config file to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/usb/core/port.c:12:
>> include/linux/usb/typec.h:322:5: warning: no previous prototype for 'typec_port_registration_register_notify' [-Wmissing-prototypes]
     322 | int typec_port_registration_register_notify(struct notifier_block *nb)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/usb/typec.h:327:5: warning: no previous prototype for 'typec_port_registration_unregister_notify' [-Wmissing-prototypes]
     327 | int typec_port_registration_unregister_notify(struct notifier_block *nb)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/typec_port_registration_register_notify +322 include/linux/usb/typec.h

ed296d8d0a92758 Prashant Malani 2021-11-24  321  
ed296d8d0a92758 Prashant Malani 2021-11-24 @322  int typec_port_registration_register_notify(struct notifier_block *nb)
ed296d8d0a92758 Prashant Malani 2021-11-24  323  {
ed296d8d0a92758 Prashant Malani 2021-11-24  324  	return 0;
ed296d8d0a92758 Prashant Malani 2021-11-24  325  }
ed296d8d0a92758 Prashant Malani 2021-11-24  326  
ed296d8d0a92758 Prashant Malani 2021-11-24 @327  int typec_port_registration_unregister_notify(struct notifier_block *nb)
ed296d8d0a92758 Prashant Malani 2021-11-24  328  {
ed296d8d0a92758 Prashant Malani 2021-11-24  329  	return 0;
ed296d8d0a92758 Prashant Malani 2021-11-24  330  }
ae196ddb0d3186b Heikki Krogerus 2021-04-07  331  #endif
ae196ddb0d3186b Heikki Krogerus 2021-04-07  332  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH 3/4] usb: Link the ports to the connectors they are attached to

[Prashant Malani]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Creating link to the USB Type-C connector for every new port
that is added when possible.

Originally submitted as commit 63cd78617350 ("usb: Link the ports to the
connectors they are attached to") but later reverted in
commit 5bdb080f9603 ("Revert "usb: Link the ports to the connectors they
are attached to"") due to a cyclic module dependency, which has since
been resolved.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++
 drivers/usb/core/port.c                 | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 2ebe5708b4bc..a017cf610d9b 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -244,6 +244,15 @@ Description:
 		is permitted, "u2" if only u2 is permitted, "u1_u2" if both u1 and
 		u2 are permitted.
 
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/connector
+Date:		April 2021
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Link to the USB Type-C connector when available. This link is
+		only created when USB Type-C Connector Class is enabled, and
+		only if the system firmware is capable of describing the
+		connection between a port and its connector.
+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <mathias.nyman@linux.intel.com>
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 53a64ce76183..83f5eac6691c 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -585,6 +585,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	}
 
 	find_and_link_peer(hub, port1);
+	typec_link_port(&port_dev->dev);
 
 	/*
 	 * In some cases, the Type C port gets registered later, so
@@ -637,5 +638,6 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
 	peer = port_dev->peer;
 	if (peer)
 		unlink_peers(port_dev, peer);
+	typec_unlink_port(&port_dev->dev);
 	device_unregister(&port_dev->dev);
 }
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 4/4] Revert "usb: Iterator for ports"

[Prashant Malani]

This reverts commit b433c4c789d612cf58739a772bbddbd949bafd20.

We no longer use typec_link_ports() to link a newly registered Type C
port to pre-existing USB ports, so we don't need this iterator anymore.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/usb/core/usb.c | 46 ------------------------------------------
 include/linux/usb.h    |  9 ---------
 2 files changed, 55 deletions(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 62368c4ed37a..2ce3667ec6fa 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -398,52 +398,6 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *))
 }
 EXPORT_SYMBOL_GPL(usb_for_each_dev);
 
-struct each_hub_arg {
-	void *data;
-	int (*fn)(struct device *, void *);
-};
-
-static int __each_hub(struct usb_device *hdev, void *data)
-{
-	struct each_hub_arg *arg = (struct each_hub_arg *)data;
-	struct usb_hub *hub;
-	int ret = 0;
-	int i;
-
-	hub = usb_hub_to_struct_hub(hdev);
-	if (!hub)
-		return 0;
-
-	mutex_lock(&usb_port_peer_mutex);
-
-	for (i = 0; i < hdev->maxchild; i++) {
-		ret = arg->fn(&hub->ports[i]->dev, arg->data);
-		if (ret)
-			break;
-	}
-
-	mutex_unlock(&usb_port_peer_mutex);
-
-	return ret;
-}
-
-/**
- * usb_for_each_port - interate over all USB ports in the system
- * @data: data pointer that will be handed to the callback function
- * @fn: callback function to be called for each USB port
- *
- * Iterate over all USB ports and call @fn for each, passing it @data. If it
- * returns anything other than 0, we break the iteration prematurely and return
- * that value.
- */
-int usb_for_each_port(void *data, int (*fn)(struct device *, void *))
-{
-	struct each_hub_arg arg = {data, fn};
-
-	return usb_for_each_dev(&arg, __each_hub);
-}
-EXPORT_SYMBOL_GPL(usb_for_each_port);
-
 /**
  * usb_release_dev - free a usb device structure when all users of it are finished.
  * @dev: device that's been disconnected
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7ccaa76a9a96..200b7b79acb5 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -875,15 +875,6 @@ extern struct usb_host_interface *usb_find_alt_setting(
 		unsigned int iface_num,
 		unsigned int alt_num);
 
-#if IS_REACHABLE(CONFIG_USB)
-int usb_for_each_port(void *data, int (*fn)(struct device *, void *));
-#else
-static inline int usb_for_each_port(void *data, int (*fn)(struct device *, void *))
-{
-	return 0;
-}
-#endif
-
 /* port claiming functions */
 int usb_hub_claim_port(struct usb_device *hdev, unsigned port1,
 		struct usb_dev_state *owner);
-- 
2.34.0.rc2.393.gf8c9666880-goog

Re: [PATCH 0/4] usb: Use notifier for linking Type C ports.

[Heikki Krogerus]

On Wed, Nov 24, 2021 at 03:10:06PM -0800, Prashant Malani wrote:
> This series resolves the cyclic dependency error which was introduced by
> commit 63cd78617350 ("usb: Link the ports to the connectors they are
> attached to") which lead to it being reverted. The approach here is to
> use a notifier to link a new Type C port to pre-existing USB ports
> instead of calling an iterator of usb ports from the Type C connector
> class. This allows commit 63cd78617350 ("usb: Link the ports to the
> connectors they are attached to") to then be submitted without any
> depmod cyclic dependency error.
> 
> The final patch removes the usb port iterator since it is no longer
> needed.

This is not enough. Build the Type-C Class as a module and the USB bus
statically, and the links will not get created.

I'm not sure you actually achieve much with this series, and I'm not
sure this approach will ever fully solve the problem. As long as we
have to declare API, we will have the circular dependency issue on our
hands. But there are ways to avoid that.

There is for example the component framework (drivers/base/component.c)
that I've been thinking about using here. In this case it would work
so that you declare the USB Type-C part as your aggregate driver, and
everything that is connected to it (so USB ports, DisplayPorts, TBT,
etc.) would then just declare themselves as general components. Could
you take a look at that?

thanks,

-- 
heikki

Re: [PATCH 0/4] usb: Use notifier for linking Type C ports.

[Heikki Krogerus]

Hi Prashant,

On Fri, Nov 26, 2021 at 11:40:49AM +0200, Heikki Krogerus wrote:
> On Wed, Nov 24, 2021 at 03:10:06PM -0800, Prashant Malani wrote:
> > This series resolves the cyclic dependency error which was introduced by
> > commit 63cd78617350 ("usb: Link the ports to the connectors they are
> > attached to") which lead to it being reverted. The approach here is to
> > use a notifier to link a new Type C port to pre-existing USB ports
> > instead of calling an iterator of usb ports from the Type C connector
> > class. This allows commit 63cd78617350 ("usb: Link the ports to the
> > connectors they are attached to") to then be submitted without any
> > depmod cyclic dependency error.
> > 
> > The final patch removes the usb port iterator since it is no longer
> > needed.
> 
> This is not enough. Build the Type-C Class as a module and the USB bus
> statically, and the links will not get created.
> 
> I'm not sure you actually achieve much with this series, and I'm not
> sure this approach will ever fully solve the problem. As long as we
> have to declare API, we will have the circular dependency issue on our
> hands. But there are ways to avoid that.
> 
> There is for example the component framework (drivers/base/component.c)
> that I've been thinking about using here. In this case it would work
> so that you declare the USB Type-C part as your aggregate driver, and
> everything that is connected to it (so USB ports, DisplayPorts, TBT,
> etc.) would then just declare themselves as general components. Could
> you take a look at that?

I'm preparing a patch where I store all _PLDs in the ACPI tables, and
create list of devices that share it. I can convert port-mapper.c to
it and the component framework while at it.


Br,

-- 
heikki

Re: [PATCH 0/4] usb: Use notifier for linking Type C ports.

[Prashant Malani]

Hi Heikki,

Thanks for taking a look at the series.

On Tue, Nov 30, 2021 at 3:03 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Prashant,
>
> On Fri, Nov 26, 2021 at 11:40:49AM +0200, Heikki Krogerus wrote:
> > On Wed, Nov 24, 2021 at 03:10:06PM -0800, Prashant Malani wrote:
> > > This series resolves the cyclic dependency error which was introduced by
> > > commit 63cd78617350 ("usb: Link the ports to the connectors they are
> > > attached to") which lead to it being reverted. The approach here is to
> > > use a notifier to link a new Type C port to pre-existing USB ports
> > > instead of calling an iterator of usb ports from the Type C connector
> > > class. This allows commit 63cd78617350 ("usb: Link the ports to the
> > > connectors they are attached to") to then be submitted without any
> > > depmod cyclic dependency error.
> > >
> > > The final patch removes the usb port iterator since it is no longer
> > > needed.
> >
> > This is not enough. Build the Type-C Class as a module and the USB bus
> > statically, and the links will not get created.
> >

I see. I suppose it is academic now (given your follow up email about converting
port-mapper to component framework), but would reversing where the
notifier block is i.e,
have usbcore expose the notifier registration API instead of
typec-class, resolve
the issue? That would mean the dependency is the same as what it is right now
in the code, right (typec -> usbcore)

> > I'm not sure you actually achieve much with this series, and I'm not
> > sure this approach will ever fully solve the problem. As long as we
> > have to declare API, we will have the circular dependency issue on our
> > hands. But there are ways to avoid that.
> >
> > There is for example the component framework (drivers/base/component.c)
> > that I've been thinking about using here. In this case it would work
> > so that you declare the USB Type-C part as your aggregate driver, and
> > everything that is connected to it (so USB ports, DisplayPorts, TBT,
> > etc.) would then just declare themselves as general components. Could
> > you take a look at that?
>
> I'm preparing a patch where I store all _PLDs in the ACPI tables, and
> create list of devices that share it. I can convert port-mapper.c to
> it and the component framework while at it.

Great, thanks. We can help with testing once you have a patch series
to share.

Best regards,

-Prashant

Re: [PATCH 0/4] usb: Use notifier for linking Type C ports.

[Heikki Krogerus]

On Tue, Nov 30, 2021 at 11:27:12AM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> Thanks for taking a look at the series.
> 
> On Tue, Nov 30, 2021 at 3:03 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Prashant,
> >
> > On Fri, Nov 26, 2021 at 11:40:49AM +0200, Heikki Krogerus wrote:
> > > On Wed, Nov 24, 2021 at 03:10:06PM -0800, Prashant Malani wrote:
> > > > This series resolves the cyclic dependency error which was introduced by
> > > > commit 63cd78617350 ("usb: Link the ports to the connectors they are
> > > > attached to") which lead to it being reverted. The approach here is to
> > > > use a notifier to link a new Type C port to pre-existing USB ports
> > > > instead of calling an iterator of usb ports from the Type C connector
> > > > class. This allows commit 63cd78617350 ("usb: Link the ports to the
> > > > connectors they are attached to") to then be submitted without any
> > > > depmod cyclic dependency error.
> > > >
> > > > The final patch removes the usb port iterator since it is no longer
> > > > needed.
> > >
> > > This is not enough. Build the Type-C Class as a module and the USB bus
> > > statically, and the links will not get created.
> > >
> 
> I see. I suppose it is academic now (given your follow up email about converting
> port-mapper to component framework), but would reversing where the
> notifier block is i.e,
> have usbcore expose the notifier registration API instead of
> typec-class, resolve
> the issue? That would mean the dependency is the same as what it is right now
> in the code, right (typec -> usbcore)

Well, then you would have the same issue if you build the Type-C class
statically and USB as a module, no?

I'm sure that if we though about this hard enough, we would find a way
to make the notifiers work, most likely by handling every possible
scenario separately, but it would still not remove the core problem.
There is the dependency between these components/drivers. The proper
solution does not create that dependency.

Although I'm not sure that the component framework is the best (it is
in the end just a workaround as well, but at least it's there
available for everybody), by taking advantage of the _PLD we can
create a solution where both components can live completely
independently - the order in which they are registered becomes
irrelevant as well as are they build as modules or not.

> > > I'm not sure you actually achieve much with this series, and I'm not
> > > sure this approach will ever fully solve the problem. As long as we
> > > have to declare API, we will have the circular dependency issue on our
> > > hands. But there are ways to avoid that.
> > >
> > > There is for example the component framework (drivers/base/component.c)
> > > that I've been thinking about using here. In this case it would work
> > > so that you declare the USB Type-C part as your aggregate driver, and
> > > everything that is connected to it (so USB ports, DisplayPorts, TBT,
> > > etc.) would then just declare themselves as general components. Could
> > > you take a look at that?
> >
> > I'm preparing a patch where I store all _PLDs in the ACPI tables, and
> > create list of devices that share it. I can convert port-mapper.c to
> > it and the component framework while at it.
> 
> Great, thanks. We can help with testing once you have a patch series
> to share.

OK, cool.

thanks,

-- 
heikki