Am Mittwoch, den 24.07.2019, 17:02 -0400 schrieb Alan Stern:
> On Wed, 24 Jul 2019, Oliver Neukum wrote:
>
> > drivers/hid/usbhid/hid-core.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index c7bc9db5b192..98b996ecf4d3 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1229,6 +1229,17 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> > struct usbhid_device *usbhid = hid->driver_data;
> > int r = 0;
> >
> > + spin_lock_irq(&usbhid->lock);
> > + if (test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
> > + r = -ENODEV;
> > + spin_unlock_irq(&usbhid->lock);
> > + goto bail_out;
> > + } else {
> > + /* protect against disconnect */
> > + usb_get_dev(interface_to_usbdev(usbhid->intf));
> > + spin_unlock_irq(&usbhid->lock);
> > + }
> > +
> > switch (lvl) {
> > case PM_HINT_FULLON:
> > r = usb_autopm_get_interface(usbhid->intf);
> > @@ -1238,7 +1249,9 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> > usb_autopm_put_interface(usbhid->intf);
> > break;
> > }
> > + usb_put_dev(interface_to_usbdev(usbhid->intf));
> >
> > +bail_out:
> > return r;
> > }
>
> Isn't this treating the symptom instead of the cause?
Sort of. Holding a reference for the whole time would have merit,
but I doubt it is strictly necessary.
> Shouldn't the hid_device hold a reference to usbhid->intf throughout
> its lifetime? That way this sort of problem wouldn't arise in any
> routine, not just usbhid_power().
Unfortunately the semantics would still be wrong without the check
in corner cases. In case disconnect() is called without a physical
unplug, we must not touch the power state.
I am indeed afraid that in that case my putative fix is still racy.
But I don't to just introduce a mutex just for this. Any ideas?
Regards
Oliver
