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

[Li Yu <raise.sail@gmail.com>]

Dmitry Torokhov wrote:
>> +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?
>
>   
Our HID driver just are some logical drivers, they are not attach some
physical devices directly. However, The HID core may include more than
one physical bus devices, I think HID drivers only take care of these
devices at one physical bus. So, compare here can avoid call 
hid_drv->match() across bus.
>> +	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.
>
>   
Yes, this may have a bit of hack, but this make hid_drv->probe() more
easier. And, as you seen, the hid_drv_probe() will check the actual
probe process is whether OK. If not so, the hid_drv_probe() will clean
this member.
>> +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 ;)
>
>   
Er, if not so, we will get some dump stack information in dmesg when
remove this module ......
>> +	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...
>
>   
I agree, this is another TODO.
>> - *  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?
>
>   
I also want to who changed them! ~_~

>> +	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.
>
>   
May be, I really do not want to increase interrupt disabling time in our
action.

>> +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.
>
>   
Yes, I agree! Also, there have another code access hid_transports[]
without spin it!