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 <[EMAIL PROTECTED]>
>> - * Copyright (c) 2005 Michael Haboustak <[EMAIL PROTECTED]> 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!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
