[PATCH] HID: don't grab devices with no input

[PATCH] HID: don't grab devices with no input

[Jiri Slaby]

Alan Stern wrote:
> This suggests that a lot of the work in usbhid_start should be 
> performed earlier, before calling hid_add_device.  After all, why 
> bother registering a USB device on the input bus if usbhid isn't going 
> to be able to drive it?

None of the code can be moved to the usbhid probe function, because all
of it depends on the driver's (potential) report_fixup.

However I suggest moving this test to the probe which ensures performing
the test early enough.

Andi, could you test the attached patch?

--

Some devices have no input interrupt endpoint. These won't be handled
by usbhid, but currently they are not refused and reside on hid bus.

Perform this checking earlier so that we refuse to control such
a device early enough (and not pass it to the hid bus at all).

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 drivers/hid/usbhid/hid-core.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index f7e0d71..2c5cb10 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -845,12 +845,6 @@ static int usbhid_start(struct hid_device *hid)
 		}
 	}
 
-	if (!usbhid->urbin) {
-		err_hid("couldn't find an input interrupt endpoint");
-		ret = -ENODEV;
-		goto fail;
-	}
-
 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
@@ -947,15 +941,26 @@ static struct hid_ll_driver usb_hid_driver = {
 
 static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
+	struct usb_host_interface *interface = intf->cur_altsetting;
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct usbhid_device *usbhid;
 	struct hid_device *hid;
+	unsigned int n, has_in = 0;
 	size_t len;
 	int ret;
 
 	dbg_hid("HID probe called for ifnum %d\n",
 			intf->altsetting->desc.bInterfaceNumber);
 
+	for (n = 0; n < interface->desc.bNumEndpoints; n++)
+		if (usb_endpoint_dir_in(&interface->endpoint[n].desc))
+			has_in++;
+	if (!has_in) {
+		dev_err(&intf->dev, "couldn't find an input interrupt "
+				"endpoint");
+		return -ENODEV;
+	}
+
 	hid = hid_allocate_device();
 	if (IS_ERR(hid))
 		return PTR_ERR(hid);
-- 
1.6.0.3

Re: [PATCH] HID: don't grab devices with no input

[Alan Stern]

On Thu, 13 Nov 2008, Jiri Slaby wrote:

> Alan Stern wrote:
> > This suggests that a lot of the work in usbhid_start should be 
> > performed earlier, before calling hid_add_device.  After all, why 
> > bother registering a USB device on the input bus if usbhid isn't going 
> > to be able to drive it?
> 
> None of the code can be moved to the usbhid probe function, because all
> of it depends on the driver's (potential) report_fixup.
> 
> However I suggest moving this test to the probe which ensures performing
> the test early enough.

That makes sense.  It's the only failure mode in usbhid_start which
isn't a simple out-of-memory error.

> Andi, could you test the attached patch?
> 
> --
> 
> Some devices have no input interrupt endpoint. These won't be handled
> by usbhid, but currently they are not refused and reside on hid bus.
> 
> Perform this checking earlier so that we refuse to control such
> a device early enough (and not pass it to the hid bus at all).
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

> +	for (n = 0; n < interface->desc.bNumEndpoints; n++)
> +		if (usb_endpoint_dir_in(&interface->endpoint[n].desc))
> +			has_in++;
> +	if (!has_in) {
> +		dev_err(&intf->dev, "couldn't find an input interrupt "
> +				"endpoint");
> +		return -ENODEV;
> +	}
> +

Do you want to use usb_endpoint_is_int_in() instead?  It matches the 
error message more closely.

Alan Stern

Re: [PATCH] HID: don't grab devices with no input

[Jiri Slaby]

On 11/13/2008 10:37 PM, Alan Stern wrote:
>> +	for (n = 0; n < interface->desc.bNumEndpoints; n++)
>> +		if (usb_endpoint_dir_in(&interface->endpoint[n].desc))
>> +			has_in++;
>> +	if (!has_in) {
>> +		dev_err(&intf->dev, "couldn't find an input interrupt "
>> +				"endpoint");
>> +		return -ENODEV;
>> +	}
>> +
> 
> Do you want to use usb_endpoint_is_int_in() instead?  It matches the 
> error message more closely.

Yes, good catch, thanks.

[PATCH 1/1 v2] DRM: fix radeon suspend/resume oops

[Jiri Slaby]

me wrote:
> As the accesses to the mmio member are not protected by anything, they
> seem to be racy with the open/clsoe anyways, setting this down there
> too.

On the second though it should be protected. Updated patch below...

+ added Rafael to cc list

--

When the driver is bound to a device and nobody opens the device node,
it will oops on suspend and resume, since it's not mapped and
dev_priv->mmio is NULL.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/gpu/drm/radeon/radeon_drv.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 71af746..7672310 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -56,6 +56,9 @@ static int radeon_suspend(struct drm_device *dev, pm_message_t state)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Disable *all* interrupts */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, 0);
@@ -67,6 +70,9 @@ static int radeon_resume(struct drm_device *dev)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Restore interrupt registers */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, dev_priv->r500_disp_irq_reg);
-- 
1.6.0.3

Re: [PATCH 1/1 v2] DRM: fix radeon suspend/resume oops

[Jiri Slaby]

Sorry, wrong patch, wrong destination.

On 11/13/2008 11:09 PM, Jiri Slaby wrote:
> me wrote:
>> As the accesses to the mmio member are not protected by anything, they
>> seem to be racy with the open/clsoe anyways, setting this down there
>> too.
> 
> On the second though it should be protected. Updated patch below...

[PATCH 1/1 v2] HID: don't grab devices with no input

[Jiri Slaby]

Some devices have no input interrupt endpoint. These won't be handled
by usbhid, but currently they are not refused and reside on hid bus.

Perform this checking earlier so that we refuse to control such
a device early enough (and not pass it to the hid bus at all).

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 drivers/hid/usbhid/hid-core.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 05402e9..9c9bd4c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -850,12 +850,6 @@ static int usbhid_start(struct hid_device *hid)
 		}
 	}
 
-	if (!usbhid->urbin) {
-		err_hid("couldn't find an input interrupt endpoint");
-		ret = -ENODEV;
-		goto fail;
-	}
-
 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
@@ -958,15 +952,26 @@ static struct hid_ll_driver usb_hid_driver = {
 
 static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
+	struct usb_host_interface *interface = intf->cur_altsetting;
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct usbhid_device *usbhid;
 	struct hid_device *hid;
+	unsigned int n, has_in = 0;
 	size_t len;
 	int ret;
 
 	dbg_hid("HID probe called for ifnum %d\n",
 			intf->altsetting->desc.bInterfaceNumber);
 
+	for (n = 0; n < interface->desc.bNumEndpoints; n++)
+		if (usb_endpoint_is_int_in(&interface->endpoint[n].desc))
+			has_in++;
+	if (!has_in) {
+		dev_err(&intf->dev, "couldn't find an input interrupt "
+				"endpoint");
+		return -ENODEV;
+	}
+
 	hid = hid_allocate_device();
 	if (IS_ERR(hid))
 		return PTR_ERR(hid);
-- 
1.6.0.4

Re: [PATCH 1/1 v2] HID: don't grab devices with no input

[Jiri Kosina]

On Thu, 13 Nov 2008, Jiri Slaby wrote:

> Some devices have no input interrupt endpoint. These won't be handled
> by usbhid, but currently they are not refused and reside on hid bus.
> Perform this checking earlier so that we refuse to control such
> a device early enough (and not pass it to the hid bus at all).
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

As the patch is obviously correct(tm) :) I have queued it in my tree, but 
Andi, please still report back whether it fixes the issue you are seeing.

Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/1 v2] HID: don't grab devices with no input

[Andi Kleen]

On Fri, Nov 14, 2008 at 12:02:27PM +0100, Jiri Kosina wrote:
> On Thu, 13 Nov 2008, Jiri Slaby wrote:
> 
> > Some devices have no input interrupt endpoint. These won't be handled
> > by usbhid, but currently they are not refused and reside on hid bus.
> > Perform this checking earlier so that we refuse to control such
> > a device early enough (and not pass it to the hid bus at all).
> > Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> As the patch is obviously correct(tm) :) I have queued it in my tree, but 
> Andi, please still report back whether it fixes the issue you are seeing.

sispm works again with that patch. Thanks.

The only nit I noticed is that the error message seems to miss an \n

-Andi

Re: [PATCH 1/1 v2] HID: don't grab devices with no input

[Jiri Kosina]

On Fri, 14 Nov 2008, Andi Kleen wrote:

> > > Some devices have no input interrupt endpoint. These won't be 
> > > handled by usbhid, but currently they are not refused and reside on 
> > > hid bus. Perform this checking earlier so that we refuse to control 
> > > such a device early enough (and not pass it to the hid bus at all). 
> > > Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> > As the patch is obviously correct(tm) :) I have queued it in my tree, but 
> > Andi, please still report back whether it fixes the issue you are seeing.
> sispm works again with that patch. Thanks.

Thanks a lot for testing.

> The only nit I noticed is that the error message seems to miss an \n

That I have already fixed in my tree.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: don't grab devices with no input

[Andi Kleen]

On Thu, Nov 13, 2008 at 11:05:09PM +0100, Jiri Slaby wrote:
> On 11/13/2008 10:37 PM, Alan Stern wrote:
> >> +	for (n = 0; n < interface->desc.bNumEndpoints; n++)
> >> +		if (usb_endpoint_dir_in(&interface->endpoint[n].desc))
> >> +			has_in++;
> >> +	if (!has_in) {
> >> +		dev_err(&intf->dev, "couldn't find an input interrupt "
> >> +				"endpoint");
> >> +		return -ENODEV;
> >> +	}
> >> +
> > 
> > Do you want to use usb_endpoint_is_int_in() instead?  It matches the 
> > error message more closely.
> 
> Yes, good catch, thanks.
Should I test it with that change or the original version?

-Andi

-- 
ak@linux.intel.com

Re: [PATCH] HID: don't grab devices with no input

[Jiri Slaby]

On 11/13/2008 11:22 PM, Andi Kleen wrote:
> On Thu, Nov 13, 2008 at 11:05:09PM +0100, Jiri Slaby wrote:
>> On 11/13/2008 10:37 PM, Alan Stern wrote:
>>> Do you want to use usb_endpoint_is_int_in() instead?  It matches the 
>>> error message more closely.
>> Yes, good catch, thanks.
> Should I test it with that change or the original version?

The updated one, please.

[PATCH 1/1] DRM: fix radeon suspend/resume oops

[Jiri Slaby]

Hi,

I've sent a bugreport twice with no reply, so coming with a patch.
Andrew please apply, if no comments or a better patch from drm
fellows comes.

As the accesses to the mmio member are not protected by anything, they
seem to be racy with the open/clsoe anyways, setting this down there
too.

--
When the driver is bound to a device and nobody opens the device node,
it will oops on suspend and resume, since it's not mapped and
dev_priv->mmio is NULL.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/radeon/radeon_drv.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 71af746..2e74a98 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -52,10 +52,14 @@ static int dri_library_name(struct drm_device *dev, char *buf)
 		        "r300"));
 }
 
+/* FIXME all this suspend/resume races with open/close? */
 static int radeon_suspend(struct drm_device *dev, pm_message_t state)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Disable *all* interrupts */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, 0);
@@ -67,6 +71,9 @@ static int radeon_resume(struct drm_device *dev)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Restore interrupt registers */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, dev_priv->r500_disp_irq_reg);
-- 
1.6.0.3

[PATCH 1/1 v2] DRM: fix radeon suspend/resume oops

[Jiri Slaby]

me wrote:
> As the accesses to the mmio member are not protected by anything, they
> seem to be racy with the open/clsoe anyways, setting this down there
> too.

On the second though it should be protected. Updated patch below...

+ added Rafael to cc list

--

When the driver is bound to a device and nobody opens the device node,
it will oops on suspend and resume, since it's not mapped and
dev_priv->mmio is NULL.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/gpu/drm/radeon/radeon_drv.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 71af746..7672310 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -56,6 +56,9 @@ static int radeon_suspend(struct drm_device *dev, pm_message_t state)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Disable *all* interrupts */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, 0);
@@ -67,6 +70,9 @@ static int radeon_resume(struct drm_device *dev)
 {
 	drm_radeon_private_t *dev_priv = dev->dev_private;
 
+	if (!dev_priv->mmio)
+		return 0;
+
 	/* Restore interrupt registers */
 	if ((dev_priv->flags & RADEON_FAMILY_MASK) >= CHIP_RS690)
 		RADEON_WRITE(R500_DxMODE_INT_MASK, dev_priv->r500_disp_irq_reg);
-- 
1.6.0.3