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 <[email protected]>
---
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
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 <[email protected]>
> + 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
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.
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 <[email protected]>
---
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
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 <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
---
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
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...
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
--
[email protected]
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.
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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 <[email protected]>
> > 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