[0/6] usb: Linking ports to their Type-C connectors

Hi,

Adding a simple function typec_link_port() that can be used to create
a symlink "connector" that points to the USB Type-C connector of a
port. It is used with USB ports initially, but hopefully later also
with other things like DisplayPorts.

Being able to see which connector is connected to a port is important
in general, but it is really important when for example the data or
power role of a device needs to swapped. The user probable wants to
know which USB device is disconnected if role swap on a USB Type-C
connector is executed.

Hope these are OK.

thanks,

Heikki Krogerus (6):
  usb: Iterator for ports
  usb: typec: Organize the private headers properly
  usb: typec: Declare the typec_class static
  usb: typec: Port mapping utility
  usb: Link the ports to the connectors they are attached to
  usb: typec: Link all ports during connector registration

 Documentation/ABI/testing/sysfs-bus-usb |   9 +
 drivers/usb/core/port.c                 |   3 +
 drivers/usb/core/usb.c                  |  43 ++++
 drivers/usb/typec/Makefile              |   1 +
 drivers/usb/typec/bus.c                 |   2 +
 drivers/usb/typec/bus.h                 |  19 +-
 drivers/usb/typec/class.c               | 101 +++------
 drivers/usb/typec/class.h               |  94 ++++++++
 drivers/usb/typec/mux.c                 |   4 +-
 drivers/usb/typec/mux.h                 |  21 ++
 drivers/usb/typec/port-mapper.c         | 283 ++++++++++++++++++++++++
 include/linux/usb.h                     |   1 +
 include/linux/usb/typec.h               |  13 ++
 13 files changed, 499 insertions(+), 95 deletions(-)
 create mode 100644 drivers/usb/typec/class.h
 create mode 100644 drivers/usb/typec/mux.h
 create mode 100644 drivers/usb/typec/port-mapper.c

On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> Introducing usb_for_each_port(). It works the same way as
> usb_for_each_dev(), but instead of going through every USB
> device in the system, it walks through the USB ports in the
> system.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This has a couple of nasty errors.

> ---
>  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h    |  1 +
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 2ce3667ec6fae..6d49db9a1b208 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -398,6 +398,49 @@ 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 device *dev, void *data)
> +{
> +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> +	struct usb_device *hdev = to_usb_device(dev);

to_usb_device() won't work properly if the struct device isn't embedded 
in an actual usb_device structure.  And that will happen, since the USB 
bus type holds usb_interface structures as well as usb_devices.

In fact, you should use usb_for_each_dev here; it already does what you 
want.

> +	struct usb_hub *hub;
> +	int ret;
> +	int i;
> +
> +	hub = usb_hub_to_struct_hub(hdev);
> +	if (!hub)
> +		return 0;
> +
> +	for (i = 0; i < hdev->maxchild; i++) {
> +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Don't you need some sort of locking or refcounting here?  What would 
happen if this hub got removed while the routine was running?

Alan Stern

On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > Introducing usb_for_each_port(). It works the same way as
> > usb_for_each_dev(), but instead of going through every USB
> > device in the system, it walks through the USB ports in the
> > system.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This has a couple of nasty errors.
> 
> > ---
> >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb.h    |  1 +
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 2ce3667ec6fae..6d49db9a1b208 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -398,6 +398,49 @@ 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 device *dev, void *data)
> > +{
> > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > +	struct usb_device *hdev = to_usb_device(dev);
> 
> to_usb_device() won't work properly if the struct device isn't embedded 
> in an actual usb_device structure.  And that will happen, since the USB 
> bus type holds usb_interface structures as well as usb_devices.

OK, so I need to filter them out.

> In fact, you should use usb_for_each_dev here; it already does what you 
> want.

Unfortunately I can't use usb_for_each_dev here, because it deals with
struct usb_device while I need to deal with struct device in the
callback.

> > +	struct usb_hub *hub;
> > +	int ret;
> > +	int i;
> > +
> > +	hub = usb_hub_to_struct_hub(hdev);
> > +	if (!hub)
> > +		return 0;
> > +
> > +	for (i = 0; i < hdev->maxchild; i++) {
> > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Don't you need some sort of locking or refcounting here?  What would 
> happen if this hub got removed while the routine was running?

I'll use a lock then.

thanks,

On Thu, Mar 25, 2021 at 05:14:45PM +0200, Heikki Krogerus wrote:
> On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > Introducing usb_for_each_port(). It works the same way as
> > > usb_for_each_dev(), but instead of going through every USB
> > > device in the system, it walks through the USB ports in the
> > > system.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > This has a couple of nasty errors.
> > 
> > > ---
> > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/usb.h    |  1 +
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -398,6 +398,49 @@ 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 device *dev, void *data)
> > > +{
> > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > +	struct usb_device *hdev = to_usb_device(dev);
> > 
> > to_usb_device() won't work properly if the struct device isn't embedded 
> > in an actual usb_device structure.  And that will happen, since the USB 
> > bus type holds usb_interface structures as well as usb_devices.
> 
> OK, so I need to filter them out.
> 
> > In fact, you should use usb_for_each_dev here; it already does what you 
> > want.
> 
> Unfortunately I can't use usb_for_each_dev here, because it deals with
> struct usb_device while I need to deal with struct device in the
> callback.

Ah, I can use it instead of bus_for_each_dev() in usb_for_each_port().
I'll fix these in v2.

For the lock I guess I can just use the peer lock (usb_port_peer_mutex)?

thanks,

On Thu, Mar 25, 2021 at 04:20:15PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote:
> > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote:
> > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote:
> > > > Introducing usb_for_each_port(). It works the same way as
> > > > usb_for_each_dev(), but instead of going through every USB
> > > > device in the system, it walks through the USB ports in the
> > > > system.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > This has a couple of nasty errors.
> > > 
> > > > ---
> > > >  drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/usb.h    |  1 +
> > > >  2 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > > index 2ce3667ec6fae..6d49db9a1b208 100644
> > > > --- a/drivers/usb/core/usb.c
> > > > +++ b/drivers/usb/core/usb.c
> > > > @@ -398,6 +398,49 @@ 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 device *dev, void *data)
> > > > +{
> > > > +	struct each_hub_arg *arg = (struct each_hub_arg *)data;
> > > > +	struct usb_device *hdev = to_usb_device(dev);
> > > 
> > > to_usb_device() won't work properly if the struct device isn't embedded 
> > > in an actual usb_device structure.  And that will happen, since the USB 
> > > bus type holds usb_interface structures as well as usb_devices.
> > 
> > OK, so I need to filter them out.
> > 
> > > In fact, you should use usb_for_each_dev here; it already does what you 
> > > want.
> > 
> > Unfortunately I can't use usb_for_each_dev here, because it deals with
> > struct usb_device while I need to deal with struct device in the
> > callback.
> 
> Why do you need 'struct device' in the callback?  All you really want is
> the hub devices, right?

I need the ports, not the hubs.

> > > > +	struct usb_hub *hub;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	hub = usb_hub_to_struct_hub(hdev);
> > > > +	if (!hub)
> > > > +		return 0;
> > > > +
> > > > +	for (i = 0; i < hdev->maxchild; i++) {
> > > > +		ret = arg->fn(&hub->ports[i]->dev, arg->data);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > Don't you need some sort of locking or refcounting here?  What would 
> > > happen if this hub got removed while the routine was running?
> > 
> > I'll use a lock then.
> 
> That's not going to work to be held over a callback.  Just properly
> increment the reference count.

I though we have done that already. Does bus_for_each_dev() do that
for the device that it passes to the callback until that callback
returns?

thanks,