Re: [PATCH 3/5] usb: xhci: dbc: Provide sysfs option to configure dbc descriptors

[Greg KH <greg@kroah.com>]

On Fri, Jun 07, 2019 at 12:03:04PM +0530, Prabhat Chand Pandey wrote:
> From: "K V, Abhilash" <abhilash.k.v@intel.com>
> 
> Show the active dbc function and dbc descriptors, allowing
> user space to dynamically modify the descriptors
> 
> The DBC specific sysfs attributes are separated into two groups,
> in the first group there are dbc & dbc_function sysfs attributes and in
> second group all other DBC descriptor specific sysfs attributes.
> 
> First group of attributes will be populated at the time of dbc_init and
> second group of attributes will only be populated when "dbc" attribute
> value is set to "enable".
> 
> Whenever "dbc" attribute value will be "disable" then second group
> of attributes will be removed.
> 
> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> Signed-off-by: Gururaj K <gururaj.k@intel.com>
> Signed-off-by: K V, Abhilash <abhilash.k.v@intel.com>
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 112 ++++++
>  drivers/usb/host/xhci-dbgcap.c                | 339 ++++++++++++++++++
>  2 files changed, 451 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> index 0088aba4caa8..b46b6afc6c4a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> @@ -23,3 +23,115 @@ Description:
>  		Reading this attribute gives the state of the DbC. It
>  		can be one of the following states: disabled, enabled,
>  		initialized, connected, configured and stalled.
> +
> +What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_function

You are putting xhci-specific files in a pci device directory.  why not
put it in your host controller device instead?  That's the best place
for this, and what about xhci drivers that are NOT pci?

Don't abuse the pci device for things that are not pci.

> +static struct attribute *dbc_descriptor_attributes[] = {
> +	&dev_attr_dbc_manufacturer.attr,
> +	&dev_attr_dbc_product.attr,
> +	&dev_attr_dbc_serial.attr,
> +	&dev_attr_dbc_protocol.attr,
> +	&dev_attr_dbc_vid.attr,
> +	&dev_attr_dbc_pid.attr,
> +	&dev_attr_dbc_device_rev.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dbc_descriptor_attrib_grp = {
> +	.attrs = dbc_descriptor_attributes,
> +};

ATTRIBUTE_GROUPS()?

> +
>  static ssize_t dbc_store(struct device *dev,
>  			 struct device_attribute *attr,
>  			 const char *buf, size_t count)
> @@ -938,6 +1270,10 @@ static ssize_t dbc_store(struct device *dev,
>  			goto err;
>  		}
>  		xhci_dbc_start(xhci);
> +		ret = sysfs_create_group(&dev->kobj,
> +					 &dbc_descriptor_attrib_grp);

If you EVER have to make a sysfs call within a driver, that is a HUGE
red flag that you are doing something wrong.

You are doing something wrong here, you just raced with userspace and
created a bunch of new files that userspace will not see.

Please do this correctly by setting the default device group for the
driver.

> +		if (ret)
> +			goto err;
>  	} else if (!strncmp(buf, "disable", 7) && dbc->state != DS_DISABLED) {
>  		if (!dbc_registered_func)
>  			return -EINVAL;
> @@ -945,6 +1281,7 @@ static ssize_t dbc_store(struct device *dev,
>  		if (dbc_registered_func->stop)
>  			dbc_registered_func->stop(dbc);
>  		module_put(dbc_registered_func->owner);
> +		sysfs_remove_group(&dev->kobj, &dbc_descriptor_attrib_grp);

Same here.

And does this really remove the files for when the PCI device is removed
from the system?  Or the driver from the device?  It doesn't seem like
it, but I might be missing a codepath here...

thanks,

greg k-h