| Message ID | 20211124231028.696982-1-pmalani@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: Use notifier for linking Type C ports. | expand |
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,
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,
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
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,
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(-)