| Message ID | 20221121062106.2569297-1-pmalani@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: pd: Add symlink to linked device | expand |
On Mon, Nov 21, 2022 at 06:20:39AM +0000, Prashant Malani wrote: > There exists a symlink from a device to its USB Power Delivery object, > but not the other way around. Add a symlink from the USB PD object to > the device which it's associated with, and call it "device". > > This is helpful to identify said device (a Type-C peripheral for > example) during uevents, since during USB PD object > creation/destruction, a uevent is generated for the PD object, > but not the device linked to it. > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Benson Leung <bleung@chromium.org> > Signed-off-by: Prashant Malani <pmalani@chromium.org> > --- > .../ABI/testing/sysfs-class-usb_power_delivery | 6 ++++++ > drivers/usb/typec/pd.c | 12 ++++++++++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-usb_power_delivery b/Documentation/ABI/testing/sysfs-class-usb_power_delivery > index ce2b1b563cb3..e7d19193875f 100644 > --- a/Documentation/ABI/testing/sysfs-class-usb_power_delivery > +++ b/Documentation/ABI/testing/sysfs-class-usb_power_delivery > @@ -4,6 +4,12 @@ Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Description: > Directory for USB Power Delivery devices. > > +What: /sys/class/usb_power_delivery/.../device > +Date: November 2022 > +Contact: Prashant Malani <pmalani@chromium.org> > +Description: > + Symbolic link to the directory of the device to which the USB PD object is linked. Line is way too long. But wait, a "device" has a specific meaning in a sysfs file, and that is not what is happening here. Please don't make fake "device" symlinks when these are not really using a device here. Either fix it up to properly use the device structures in the code, or call this something else. What in userspace wants to see this symlink? thanks, greg k-h
Hi Greg, Thanks for reviewing the patch. On Nov 21 07:35, Greg Kroah-Hartman wrote: > On Mon, Nov 21, 2022 at 06:20:39AM +0000, Prashant Malani wrote: > > > > +What: /sys/class/usb_power_delivery/.../device > > +Date: November 2022 > > +Contact: Prashant Malani <pmalani@chromium.org> > > +Description: > > + Symbolic link to the directory of the device to which the USB PD object is linked. > > Line is way too long. Sorry about that. I'll try to make it more concise in the next version. > > But wait, a "device" has a specific meaning in a sysfs file, and that is > not what is happening here. > > Please don't make fake "device" symlinks when these are not really using > a device here. Either fix it up to properly use the device structures > in the code, or call this something else. Got it, sorry about using that specific term. How about: 1. "linked_device" 2. "base_device" ? I prefer 1.) but happy to use the other, or any other suggestion. > > What in userspace wants to see this symlink? ChromeOS's userspace Type-C daemon. It listens for Type-C partner uevents, but the PD objects can be created later (and destroyed and then recreated again, for PD chargers which dynamically change their source caps), and this helps determine which partner does those events apply to. Thanks, -Prashant
On Mon, Nov 21, 2022 at 07:16:03AM +0000, Prashant Malani wrote: > Hi Greg, > > Thanks for reviewing the patch. > > On Nov 21 07:35, Greg Kroah-Hartman wrote: > > On Mon, Nov 21, 2022 at 06:20:39AM +0000, Prashant Malani wrote: > > > > > > +What: /sys/class/usb_power_delivery/.../device > > > +Date: November 2022 > > > +Contact: Prashant Malani <pmalani@chromium.org> > > > +Description: > > > + Symbolic link to the directory of the device to which the USB PD object is linked. > > > > Line is way too long. > > Sorry about that. I'll try to make it more concise in the next version. That's not the issue, the problem is that it needs to be properly line-wrapped. > > But wait, a "device" has a specific meaning in a sysfs file, and that is > > not what is happening here. > > > > Please don't make fake "device" symlinks when these are not really using > > a device here. Either fix it up to properly use the device structures > > in the code, or call this something else. > > Got it, sorry about using that specific term. How about: > 1. "linked_device" > 2. "base_device" ? Why either? What exactly is this "device" that you are trying to link to? And why not just use the real device symlink instead? > I prefer 1.) but happy to use the other, or any other suggestion. > > > > > What in userspace wants to see this symlink? > > ChromeOS's userspace Type-C daemon. It listens for Type-C partner uevents, but the PD objects > can be created later (and destroyed and then recreated again, for PD chargers which dynamically > change their source caps), and this helps determine which partner does those events apply to. Then set up the proper device symlink like the driver model supports, then you don't have to create a new one, and you don't have to add the documentation entry as it's implied. thanks, greg k-h
Hi Greg, On Nov 21 08:36, Greg Kroah-Hartman wrote: > On Mon, Nov 21, 2022 at 07:16:03AM +0000, Prashant Malani wrote: > > On Nov 21 07:35, Greg Kroah-Hartman wrote: > > > On Mon, Nov 21, 2022 at 06:20:39AM +0000, Prashant Malani wrote: > > Why either? What exactly is this "device" that you are trying to link > to? And why not just use the real device symlink instead? It's the Type-C port partner device, created here [1] > > > > > > > > What in userspace wants to see this symlink? > > > > ChromeOS's userspace Type-C daemon. It listens for Type-C partner uevents, but the PD objects > > can be created later (and destroyed and then recreated again, for PD chargers which dynamically > > change their source caps), and this helps determine which partner does those events apply to. > > Then set up the proper device symlink like the driver model supports, > then you don't have to create a new one, and you don't have to add the > documentation entry as it's implied. Sounds good. I'll submit a new series (or single patch) which follows this approach. Thanks again! -Prashant [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/typec/class.c?h=usb-next#n885
diff --git a/Documentation/ABI/testing/sysfs-class-usb_power_delivery b/Documentation/ABI/testing/sysfs-class-usb_power_delivery index ce2b1b563cb3..e7d19193875f 100644 --- a/Documentation/ABI/testing/sysfs-class-usb_power_delivery +++ b/Documentation/ABI/testing/sysfs-class-usb_power_delivery @@ -4,6 +4,12 @@ Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> Description: Directory for USB Power Delivery devices. +What: /sys/class/usb_power_delivery/.../device +Date: November 2022 +Contact: Prashant Malani <pmalani@chromium.org> +Description: + Symbolic link to the directory of the device to which the USB PD object is linked. + What: /sys/class/usb_power_delivery/.../revision Date: May 2022 Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c index dc72005d68db..ed073d922655 100644 --- a/drivers/usb/typec/pd.c +++ b/drivers/usb/typec/pd.c @@ -656,7 +656,8 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_unregister); * @dev: The device. * * This function can be used to create a symlink named "usb_power_delivery" for - * @dev that points to @pd. + * @dev that points to @pd. It also creates another symlink named "device" which + * points from @pd to @dev. */ int usb_power_delivery_link_device(struct usb_power_delivery *pd, struct device *dev) { @@ -669,6 +670,12 @@ int usb_power_delivery_link_device(struct usb_power_delivery *pd, struct device if (ret) return ret; + ret = sysfs_create_link(&pd->dev.kobj, &dev->kobj, "device"); + if (ret) { + sysfs_remove_link(&dev->kobj, "usb_power_delivery"); + return ret; + } + get_device(&pd->dev); get_device(dev); @@ -681,13 +688,14 @@ EXPORT_SYMBOL_GPL(usb_power_delivery_link_device); * @pd: The USB PD instance. * @dev: The device. * - * Remove the symlink that was previously created with pd_link_device(). + * Remove the symlinks that were previously created with pd_link_device(). */ void usb_power_delivery_unlink_device(struct usb_power_delivery *pd, struct device *dev) { if (IS_ERR_OR_NULL(pd) || !dev) return; + sysfs_remove_link(&pd->dev.kobj, "device"); sysfs_remove_link(&dev->kobj, "usb_power_delivery"); put_device(&pd->dev); put_device(dev);
There exists a symlink from a device to its USB Power Delivery object, but not the other way around. Add a symlink from the USB PD object to the device which it's associated with, and call it "device". This is helpful to identify said device (a Type-C peripheral for example) during uevents, since during USB PD object creation/destruction, a uevent is generated for the PD object, but not the device linked to it. Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Benson Leung <bleung@chromium.org> Signed-off-by: Prashant Malani <pmalani@chromium.org> --- .../ABI/testing/sysfs-class-usb_power_delivery | 6 ++++++ drivers/usb/typec/pd.c | 12 ++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)