Re: [linux-usb-devel] [RFC] HID bus design overview.

Dmitry Torokhov
2007-04-05T03:09:20Z

On Wednesday 04 April 2007 21:25, Li Yu wrote:
> Jiri Kosina wrote:
> > BTW as soon as you have some presentable code, could you please send
> > it so
> > that we could see what aproach you have taken? Debating over code is
> > usualy more efficient than just ranting random ideas :)
> >
> >
> There is a "presentable patch" in the attachment ;)

Some random notes without reading it all carefully...

> +static int hid_bus_match(struct device *dev, struct device_driver
> *drv) +{
> + struct hid_driver *hid_drv;
> + struct hid_device *hid_dev;
> +
> + hid_drv = to_hid_driver(drv);
> + hid_dev = to_hid_device(dev);
> +
> + if (is_hid_driver_sticky(hid_drv))
> + /* the sticky driver match device do not pass here. */
> + return 0;
> + if (hid_dev->bus != hid_drv->bus)
> + return 0;

How can this happen?

> + if (!hid_drv->match || hid_drv->match(hid_drv, hid_dev)) {
> + hid_dev->driver = hid_drv;

This usually done in bus->probe() function, when we know for sure that
driver binds to to the device.

> +static void hid_bus_release(struct device *dev)
> +{
> +}
> +
> +struct device hid_bus = {
> + .bus_id = "hidbus0",
> + .release = hid_bus_release
> +};
> +
> +static void hid_dev_release(struct device *dev)
> +{
> +}
> +

That will for sure raise Greg KH's blood pressure ;)

> + for (i=0; hid_dev->attrs && hid_dev->attrs[i]; ++i) {
> + ret = device_create_file(&hid_dev->device, hid_dev->attrs[i]);
> + if (ret)
> + break;
> +

That should be handled via bus's device attributes and not open coded...

> - * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
> - * Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
> + * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@@suse.cz>
> + * Copyright (c) 2005 Michael Haboustak <mike-@@cinci.rr.com> for Concept2, Inc
> * Copyright (c) 2006 Jiri Kosina

Any particular reason for mangling addresses?

> + if (interrupt)
> + local_irq_save(flags);
> + spin_lock(&hid_lock);
> + list_for_each_entry(driver, &hid_sticky_drivers, sticky_link) {
> + hook = driver->hook;
> + if (hook && hook->raw_event) {
> + ret = hook->raw_event(hid, type, data, size, interrupt);
> + if (!ret)
> + break;
> + }
> + }
> + spin_unlock(&hid_lock);
> + if (interrupt)
> + local_irq_restore(flags);
> +

This is scary. spin_lock_irqsave() and be done with it.

> +int hid_open(struct hid_device *hid)
> +{
> + struct hid_transport *tl;
> + int ret;
> +
> + if (hid->driver->open)
> + return hid->driver->open(hid);
> + ret = 0;
> + spin_lock(&hid_lock);
> + tl = hid_transports[hid->bus];
> + if (tl->open)
> + ret = tl->open(hid);
> + spin_unlock(&hid_lock);
> + return ret;
> +}

Spinlock is not the best choise here, I'd expect most ->open()
implementation wait on some IO.

--
Dmitry