Re: [PATCH v5 2/4] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

Hi Greg,

On Thu, Feb 11, 2021 at 08:03:01AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 10, 2021 at 09:10:37AM -0800, Matthias Kaehlcke wrote:
> > +static int onboard_hub_add_usbdev(struct onboard_hub *hub, struct usb_device *udev)
> > +{
> > +	struct udev_node *node;
> > +	char link_name[64];
> > +	int ret = 0;
> > +
> > +	mutex_lock(&hub->lock);
> > +
> > +	if (hub->going_away) {
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	node = devm_kzalloc(hub->dev, sizeof(*node), GFP_KERNEL);
> > +	if (!node) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	node->udev = udev;
> > +
> > +	list_add(&node->list, &hub->udev_list);
> > +
> > +	snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev));
> > +	WARN_ON(sysfs_create_link(&hub->dev->kobj, &udev->dev.kobj, link_name));
> 
> Never use WARN_ON() unless you want the machine to reboot if it triggers
> (panic on warn).

Ah, thanks, I wasn't aware of that. Will change to some type of log if the
sysfs attributes stick around.

> But the larger question is what is this sysfs link for?  It's not
> documented anywhere, and so, shouldn't be allowed.  Who is going to use
> it and why is it needed?

Alan asked to add them: https://lore.kernel.org/patchwork/patch/1313000/#1514338

I'm fine with either way, let's just agree on something :)

> > +
> > +unlock:
> > +	mutex_unlock(&hub->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static void onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev)
> > +{
> > +	struct udev_node *node;
> > +	char link_name[64];
> > +
> > +	snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev));
> > +	sysfs_remove_link(&hub->dev->kobj, link_name);
> > +
> > +	mutex_lock(&hub->lock);
> > +
> > +	list_for_each_entry(node, &hub->udev_list, list) {
> > +		if (node->udev == udev) {
> > +			list_del(&node->list);
> > +			break;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&hub->lock);
> > +}
> > +
> > +static ssize_t always_powered_in_suspend_show(struct device *dev, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	struct onboard_hub *hub = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", hub->always_powered_in_suspend);
> 
> sysfs_emit()?

ok

> And you forgot the Documentation/ABI/ entries for this driver, so it
> really can not be reviewed...

I'll add it in the next version.

> > +}
> > +
> > +static ssize_t always_powered_in_suspend_store(struct device *dev, struct device_attribute *attr,
> > +			    const char *buf, size_t count)
> > +{
> > +	struct onboard_hub *hub = dev_get_drvdata(dev);
> > +	bool val;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	hub->always_powered_in_suspend = val;
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(always_powered_in_suspend);
> > +
> > +static struct attribute *onboard_hub_sysfs_entries[] = {
> > +	&dev_attr_always_powered_in_suspend.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group onboard_hub_sysfs_group = {
> > +	.attrs = onboard_hub_sysfs_entries,
> > +};
> > +
> > +static int onboard_hub_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct onboard_hub *hub;
> > +	int err;
> > +
> > +	hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> > +	if (!hub)
> > +		return -ENOMEM;
> > +
> > +	hub->vdd = devm_regulator_get(dev, "vdd");
> > +	if (IS_ERR(hub->vdd))
> > +		return PTR_ERR(hub->vdd);
> > +
> > +	hub->dev = dev;
> > +	mutex_init(&hub->lock);
> > +	INIT_LIST_HEAD(&hub->udev_list);
> > +
> > +	dev_set_drvdata(dev, hub);
> > +
> > +	err = devm_device_add_group(dev, &onboard_hub_sysfs_group);
> 
> You just raced userspace and lost :(
> 
> Please use the correct api to add sysfs attributes to the device
> automatically by the driver core.

ok

> But the larger question is why do you need them at all?  What do they
> do that we can't already do with existing apis that you feel a one-off
> api for this driver is required?

The 'always_powered_in_suspend' attribute allows the admin to configure whether
the hub should always been kept on in suspend. I'm know about existing APIs that
would be suitable for that, but that might be just my ignorance. If you have any
suggestions please let me know.

> > +	if (err) {
> > +		dev_err(dev, "failed to create sysfs entries: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	err = onboard_hub_power_on(hub);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * The USB driver might have been detached from the USB devices by
> > +	 * onboard_hub_remove() make sure to re-attach it if needed.
> > +	 */
> > +	(void)driver_attach(&onboard_hub_usbdev_driver.drvwrap.driver);
> 
> (void)????
> 
> Please no, do it right.

Ok, driver_attach() does not return an error when the driver is already
attached, so it should be fine to change this to an error check.

> But, why is a driver calling this function anyway?  That feels really
> really wrong...

I found during testing that this is needed to make sure the driver is attached
again after it was released in onboard_hub_remove(). Alan requested to release
the driver to avoid dangling references:
https://lore.kernel.org/patchwork/patch/1310889/#1506598

Thanks

Matthias