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

[Dmitry Torokhov <dtor@insightbb.com>]

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