Re: [PATCH] fbdev: udlfb: Use usb_control_msg_send()

[Alan Stern]

On Fri, May 19, 2023 at 11:16:25PM +0200, Helge Deller wrote:
> Use the newly introduced usb_control_msg_send() instead of usb_control_msg()
> when selecting the channel.
> 
> Signed-off-by: Helge Deller 
> ---
>  drivers/video/fbdev/udlfb.c | 14 +++---
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
> index 256d9b61f4ea..dabc30a09f96 100644
> --- a/drivers/video/fbdev/udlfb.c
> +++ b/drivers/video/fbdev/udlfb.c
> @@ -1543,24 +1543,16 @@ static const struct device_attribute 
> fb_device_attrs[] = {
>  static int dlfb_select_std_channel(struct dlfb_data *dlfb)
>  {
>   int ret;
> - void *buf;
>   static const u8 set_def_chn[] = {
>   0x57, 0xCD, 0xDC, 0xA7,
>   0x1C, 0x88, 0x5E, 0x15,
>   0x60, 0xFE, 0xC6, 0x97,
>   0x16, 0x3D, 0x47, 0xF2  };
> 
> - buf = kmemdup(set_def_chn, sizeof(set_def_chn), GFP_KERNEL);
> -
> - if (!buf)
> - return -ENOMEM;
> -
> - ret = usb_control_msg(dlfb->udev, usb_sndctrlpipe(dlfb->udev, 0),
> - NR_USB_REQUEST_CHANNEL,
> + ret = usb_control_msg_send(dlfb->udev, 0, NR_USB_REQUEST_CHANNEL,
>   (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
> - buf, sizeof(set_def_chn), USB_CTRL_SET_TIMEOUT);
> -
> - kfree(buf);
> + _def_chn, sizeof(set_def_chn), USB_CTRL_SET_TIMEOUT,
> + GFP_KERNEL);
> 
>   return ret;
>  }

Reviewed-by: Alan Stern 

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Fri, May 19, 2023 at 12:38:15PM +0200, Helge Deller wrote:
> Patch looks good and survived the test.
> 
> Will you send a proper patch to the fbdev mailing list, so that I can
> include it?

Will do.

While you're working on this driver, here's a suggestion for another 
improvement you can make.  The temporary buffer allocations and calls to 
usb_control_msg() in dlfb_get_edid() and dlfb_select_std_channel() can 
be replaced with calls to usb_control_msg_recv() and 
usb_control_msg_send() respectively.

Alan Stern

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
> * Alan Stern :
> > On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> > > On 5/18/23 15:54, Alan Stern wrote:
> > > > In this case it looks like dlfb_usb_probe() or one of the routines it
> > > > calls is wrong; it assumes that an endpoint has the expected type
> > > > without checking.  More precisely, it thinks an endpoint is BULK when
> > > > actually it is INTERRUPT.  That's what needs to be fixed.
> > >
> > > Maybe usb_submit_urb() should return an error so that drivers can
> > > react on it, instead of adding the same kind of checks to all drivers?
> >
> > Feel free to submit a patch doing this.
> 
> As you wrote above, this may break other drivers too, so I'd leave that
> discussion & decision to the USB maintainers (like you).
> 
> > But the checks should be added
> > in any case; without them the drivers are simply wrong.
> 
> I pushed the hackish patch below through the syz tests which gives this log:
> (see https://syzkaller.appspot.com/text?tag=CrashLog=160b750928)
> [   77.559566][T9] usb 1-1: Unable to get valid EDID from device/display
> [   77.587021][T9] WARNING: BOGUS urb xfer, pipe 3 != type 1 (fix driver 
> to choose correct endpoint)
> [   77.596448][T9] usb 1-1: dlfb_urb_completion - nonzero write bulk 
> status received: -115
> [   77.605308][T9] usb 1-1: submit urb error: -22
> [   77.613225][T9] udlfb: probe of 1-1:0.52 failed with error -22
> 
> So, basically there is no urgent fix needed for the dlfb fbdev driver,
> as it will gracefully fail as is (which is correct).
> 
> What do you suggest we should do with this syzkaller-bug ?
> I'd rate it as false-alarm, but it will continue to complain because of
> the dev_WARN() in urb.c

Let's try this patch instead.  It might contain a stupid error because I 
haven't even tried to compile it, but it ought to fix the real problem.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
a4422ff22142

Index: usb-devel/drivers/video/fbdev/udlfb.c
===
--- usb-devel.orig/drivers/video/fbdev/udlfb.c
+++ usb-devel/drivers/video/fbdev/udlfb.c
@@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_int
struct fb_info *info;
int retval;
struct usb_device *usbdev = interface_to_usbdev(intf);
-   struct usb_endpoint_descriptor *out;
+   static u8 out_ep[] = {1 + USB_DIR_OUT, 0};
 
/* usb initialization */
dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
@@ -1666,9 +1666,9 @@ static int dlfb_usb_probe(struct usb_int
dlfb->udev = usb_get_dev(usbdev);
usb_set_intfdata(intf, dlfb);
 
-   retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, , 
NULL, NULL);
-   if (retval) {
-   dev_err(>dev, "Device should have at lease 1 bulk 
endpoint!\n");
+   if (!usb_check_bulk_endpoints(intf, out_ep)) {
+   dev_err(>dev, "Invalid DisplayLink device!\n");
+   retval = -EINVAL;
goto error;
}
 

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> On 5/18/23 15:54, Alan Stern wrote:
> > On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
> > > I think this is an informational warning from the USB stack,
> > 
> > It is not informational.  It is a warning that the caller has a bug.
> 
> I'm not a USB expert, so I searched for such bug reports, and it seems
> people sometimes faced this warning with different USB devices.

Yes.

> > You can't fix a bug by changing the line that reports it from dev_WARN
> > to printk!
> 
> Of course this patch wasn't intended as "fix".
> It was intended to see how the udlfb driver behaves in this situation, e.g.
> if the driver then crashes afterwards.
> 
> Furthermore, why does usb_submit_urb() prints this WARNING and then continues?
> If it's a real bug, why doesn't it returns an error instead?
> So, in principle I still think this warning is kind of informational,
> which of course points to some kind of problem which should be fixed.

Depending on the situation, the bug may or may not lead to an error.  At 
the time the dev_WARN was added, we were less careful about these sorts 
of checks; I did not want to cause previously working devices to stop 
working by failing the URB submission.

> > In this case it looks like dlfb_usb_probe() or one of the routines it
> > calls is wrong; it assumes that an endpoint has the expected type
> > without checking.  More precisely, it thinks an endpoint is BULK when
> > actually it is INTERRUPT.  That's what needs to be fixed.
> 
> Maybe usb_submit_urb() should return an error so that drivers can
> react on it, instead of adding the same kind of checks to all drivers?

Feel free to submit a patch doing this.  But the checks should be added 
in any case; without them the drivers are simply wrong.

Alan Stern

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

s/usb/core/hub.c:2575
> >  hub_port_connect drivers/usb/core/hub.c:5407 [inline]
> >  hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
> >  port_event drivers/usb/core/hub.c:5711 [inline]
> >  hub_event+0x2e3d/0x4ed0 drivers/usb/core/hub.c:5793
> >  process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
> >  worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
> >  kthread+0x344/0x440 kernel/kthread.c:379
> >  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
> 
> I think this is an informational warning from the USB stack,

It is not informational.  It is a warning that the caller has a bug.

> since the syzbot usb device doesn't behave as expected.
> 
> What happens with this patch applied?
> 
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
> usb-testing
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 9f3c54032556..dd77b9e757da 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -501,7 +501,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> 
>   /* Check that the pipe's type matches the endpoint's type */
>   if (usb_pipe_type_check(urb->dev, urb->pipe))
> - dev_WARN(>dev, "BOGUS urb xfer, pipe %x != type %x\n",
> + printk("BOGUS urb xfer, pipe %x != type %x (hardware 
> misbehaviour?)\n",
>   usb_pipetype(urb->pipe), pipetypes[xfertype]);
> 
>   /* Check against a simple/standard policy */

You can't fix a bug by changing the line that reports it from dev_WARN 
to printk!

In this case it looks like dlfb_usb_probe() or one of the routines it 
calls is wrong; it assumes that an endpoint has the expected type 
without checking.  More precisely, it thinks an endpoint is BULK when 
actually it is INTERRUPT.  That's what needs to be fixed.

Alan Stern

How to fix screen resolution detection?

[Alan Stern]

The screen resolution on my laptop is not reported accurately.  Here's 
an extract from the output of xdpyinfo:

screen #0:
  dimensions:3200x1800 pixels (847x476 millimeters)
  resolution:96x96 dots per inch

The number of pixels is correct, but the size and resolution values 
smack of a bogus default.  The actual width of the screen (determined 
with a tape measure) is about 11.5 inches (291 mm), which yields a 
resolution of 280 dots per inch (11 dots per mm), approximately.  
Most definitely _not_ 96 dpi.

Presumably X gets the size/resolution information from Wayland, which 
gets it from the kernel, which gets it from the firmware.  So the kernel 
driver is the logical place to start in figuring where things are going 
wrong.  The laptop uses i915; here are the relevant lines from the 
kernel log:

[0.00] Linux version 5.14.9-200.fc34.x86_64 
(mockbu...@bkernel02.iad2.fedoraproject.org) (gcc (GCC) 11.2.1 20210728 (Red 
Hat 11.2.1-1), GNU ld version 2.35.2-5.fc34) #1 SMP Thu Sep 30 11:55:35 UTC 2021

[0.463895] efifb: probing for efifb
[0.463913] efifb: framebuffer at 0xe000, using 22500k, total 22500k
[0.463916] efifb: mode is 3200x1800x32, linelength=12800, pages=1
[0.463919] efifb: scrolling: redraw
[0.463920] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0
[0.464028] Console: switching to colour frame buffer device 400x112
[0.474894] fb0: EFI VGA frame buffer device

[2.58] fb0: switching to inteldrmfb from EFI VGA
[2.891260] Console: switching to colour dummy device 80x25
[2.891318] i915 :00:02.0: vgaarb: deactivate vga console
[2.902665] i915 :00:02.0: vgaarb: changed VGA decodes: 
olddecodes=io+mem,decodes=io+mem:owns=io+mem
[2.904833] i915 :00:02.0: [drm] Finished loading DMC firmware 
i915/skl_dmc_ver1_27.bin (v1.27)
[2.947359] [drm] Initialized i915 1.6.0 20201103 for :00:02.0 on minor 0
[2.949468] ACPI: video: Video Device [GFX0] (multi-head: yes  rom: no  
post: no)
[2.949803] input: Video Bus as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input9
[2.964371] fbcon: i915 (fb0) is primary device
[2.979854] Console: switching to colour frame buffer device 400x112
[3.012355] i915 :00:02.0: [drm] fb0: i915 frame buffer device

Now, I know nothing about the kernel's graphics subsystems.  How can I 
find out what size/resolution information i915 is getting and passing to 
Wayland?  If it's wrong, how can I fix it?

Thanks,

Alan Stern

Re: [PATCH v5] drm: Use USB controller's DMA mask when importing dmabufs

[Alan Stern]

On Fri, Feb 26, 2021 at 10:26:47AM +0100, Thomas Zimmermann wrote:
> USB devices cannot perform DMA and hence have no dma_mask set in their
> device structure. Therefore importing dmabuf into a USB-based driver
> fails, which breaks joining and mirroring of display in X11.
> 
> For USB devices, pick the associated USB controller as attachment device.
> This allows the DRM import helpers to perform the DMA setup. If the DMA
> controller does not support DMA transfers, we're out of luck and cannot
> import. Our current USB-based DRM drivers don't use DMA, so the actual
> DMA device is not important.
> 
> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> instance of struct drm_driver.
> 
> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
> 
> v5:
>   * provide a helper for USB interfaces (Alan)
>   * add FIXME item to documentation and TODO list (Daniel)

> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -748,6 +748,37 @@ void usb_put_intf(struct usb_interface *intf)
>  }
>  EXPORT_SYMBOL_GPL(usb_put_intf);
>  
> +/**
> + * usb_get_dma_device - acquire a reference on the usb device's DMA endpoint
> + * @udev: usb device
> + *
> + * While a USB device cannot perform DMA operations by itself, many USB
> + * controllers can. A call to usb_get_dma_device() returns the DMA endpoint
> + * for the given USB device, if any. The returned device structure should be
> + * released with put_device().
> + *
> + * See also usb_intf_get_dma_device().
> + *
> + * Returns: A reference to the usb device's DMA endpoint; or NULL if none
> + *  exists.
> + */
> +struct device *usb_get_dma_device(struct usb_device *udev)
> +{
> + struct device *dmadev;
> +
> + if (!udev->bus)
> + return NULL;
> +
> + dmadev = get_device(udev->bus->sysdev);
> + if (!dmadev || !dmadev->dma_mask) {
> + put_device(dmadev);
> + return NULL;
> + }
> +
> + return dmadev;
> +}
> +EXPORT_SYMBOL_GPL(usb_get_dma_device);

There's no point making this a separate function, since it has no
callers of its own.  Just make usb_intf_get_dma_device the only new
function.

> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -711,6 +711,7 @@ struct usb_device {
>   unsigned use_generic_driver:1;
>  };
>  #define  to_usb_device(d) container_of(d, struct usb_device, dev)
> +#define dev_is_usb(d)((d)->bus == _bus_type)
>  
>  static inline struct usb_device *interface_to_usbdev(struct usb_interface 
> *intf)
>  {
> @@ -746,6 +747,29 @@ extern int usb_lock_device_for_reset(struct usb_device 
> *udev,
>  extern int usb_reset_device(struct usb_device *dev);
>  extern void usb_queue_reset_device(struct usb_interface *dev);
>  
> +extern struct device *usb_get_dma_device(struct usb_device *udev);
> +
> +/**
> + * usb_intf_get_dma_device - acquire a reference on the usb interface's DMA 
> endpoint
> + * @intf: the usb interface
> + *
> + * While a USB device cannot perform DMA operations by itself, many USB
> + * controllers can. A call to usb_intf_get_dma_device() returns the DMA 
> endpoint
> + * for the given USB interface, if any. The returned device structure should 
> be
> + * released with put_device().
> + *
> + * See also usb_get_dma_device().
> + *
> + * Returns: A reference to the usb interface's DMA endpoint; or NULL if none
> + *  exists.
> + */
> +static inline struct device *usb_intf_get_dma_device(struct usb_interface 
> *intf)
> +{
> + if (!intf)
> + return NULL;

Why would intf ever be NULL?

> + return usb_get_dma_device(interface_to_usbdev(intf));
> +}

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm: Use USB controller's DMA mask when importing dmabufs

[Alan Stern]

On Thu, Feb 25, 2021 at 09:23:05AM +0100, Takashi Iwai wrote:
> On Thu, 25 Feb 2021 08:57:14 +0100,
> Thomas Zimmermann wrote:
> > 
> > Hi
> > 
> > Am 24.02.21 um 16:21 schrieb Alan Stern:
> > > On Wed, Feb 24, 2021 at 10:23:04AM +0100, Thomas Zimmermann wrote:
> > >> USB devices cannot perform DMA and hence have no dma_mask set in their
> > >> device structure. Therefore importing dmabuf into a USB-based driver
> > >> fails, which breaks joining and mirroring of display in X11.
> > >>
> > >> For USB devices, pick the associated USB controller as attachment device.
> > >> This allows the DRM import helpers to perform the DMA setup. If the DMA
> > >> controller does not support DMA transfers, we're out of luck and cannot
> > >> import. Our current USB-based DRM drivers don't use DMA, so the actual
> > >> DMA device is not important.
> > >>
> > >> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> > >> instance of struct drm_driver.
> > >>
> > >> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
> > >>
> > >> v4:
> > >>  * implement workaround with USB helper functions (Greg)
> > >>  * use struct usb_device->bus->sysdev as DMA device (Takashi)
> > >> v3:
> > >>  * drop gem_create_object
> > >>  * use DMA mask of USB controller, if any (Daniel, Christian, Noralf)
> > >> v2:
> > >>  * move fix to importer side (Christian, Daniel)
> > >>  * update SHMEM and CMA helpers for new PRIME callbacks
> > >>
> > >> Signed-off-by: Thomas Zimmermann 
> > >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB 
> > >> devices")
> > >> Cc: Christoph Hellwig 
> > >> Cc: Greg Kroah-Hartman 
> > >> Cc:  # v5.10+
> > >> ---
> > >
> > >> +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> > >> +struct dma_buf *dma_buf)
> > >> +{
> > >> +struct usb_device *udev;
> > >> +struct device *dmadev;
> > >> +struct drm_gem_object *obj;
> > >> +
> > >> +if (!dev_is_usb(dev->dev))
> > >> +return ERR_PTR(-ENODEV);
> > >> +udev = interface_to_usbdev(to_usb_interface(dev->dev));
> > >> +
> > >> +dmadev = usb_get_dma_device(udev);
> > >
> > > You can do it this way if you want, but I think usb_get_dma_device would
> > > be easier to use if its argument was a pointer to struct usb_interface
> > > or (even better) a pointer to a usb_interface's embedded struct device.
> > > Then you wouldn't need to compute udev, and the same would be true for
> > > other callers.
> > 
> > It seemed natural to me to use usb_device, because it contains the bus
> > pointer. But maybe a little wrapper for usb_interface in the header
> > file makes things easier to read. I'll wait a bit for other reviews to
> > come in.
> 
> I agree with Thomas, as most of users referring to the sysdev do
> access in a pattern like udev->bus->sysdev, AFAIK.

Apart from the USB core and host/gadget controller drivers, there 
appears to be only one reference to sysdev for a USB device: the one in 
usb-storage (and that one really should be dmadev).

In general, I expect callers of the new routine would be drivers that 
bind to a USB interface (like usb-storage), not to a USB device.  So 
they would naturally have the interface pointer handy.

But the routine could be written in a different way.  If it took a 
pointer to struct device as its argument, it could easily tell whether 
that structure was embedded in a usb_device or a usb_interface 
struct, and do the right thing either way.

Or there could be two routines: one taking a usb_device pointer and one 
taking a usb_interface pointer.

The idea here is to make the routine as easy as possible for callers.  
If this means making the routine a little longer, that's okay -- there's 
only one copy of the routine but there could be lots of callers.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm: Use USB controller's DMA mask when importing dmabufs

[Alan Stern]

On Wed, Feb 24, 2021 at 10:23:04AM +0100, Thomas Zimmermann wrote:
> USB devices cannot perform DMA and hence have no dma_mask set in their
> device structure. Therefore importing dmabuf into a USB-based driver
> fails, which breaks joining and mirroring of display in X11.
> 
> For USB devices, pick the associated USB controller as attachment device.
> This allows the DRM import helpers to perform the DMA setup. If the DMA
> controller does not support DMA transfers, we're out of luck and cannot
> import. Our current USB-based DRM drivers don't use DMA, so the actual
> DMA device is not important.
> 
> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> instance of struct drm_driver.
> 
> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
> 
> v4:
>   * implement workaround with USB helper functions (Greg)
>   * use struct usb_device->bus->sysdev as DMA device (Takashi)
> v3:
>   * drop gem_create_object
>   * use DMA mask of USB controller, if any (Daniel, Christian, Noralf)
> v2:
>   * move fix to importer side (Christian, Daniel)
>   * update SHMEM and CMA helpers for new PRIME callbacks
> 
> Signed-off-by: Thomas Zimmermann 
> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> Cc: Christoph Hellwig 
> Cc: Greg Kroah-Hartman 
> Cc:  # v5.10+
> ---

> +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> + struct dma_buf *dma_buf)
> +{
> + struct usb_device *udev;
> + struct device *dmadev;
> + struct drm_gem_object *obj;
> +
> + if (!dev_is_usb(dev->dev))
> + return ERR_PTR(-ENODEV);
> + udev = interface_to_usbdev(to_usb_interface(dev->dev));
> +
> + dmadev = usb_get_dma_device(udev);

You can do it this way if you want, but I think usb_get_dma_device would 
be easier to use if its argument was a pointer to struct usb_interface 
or (even better) a pointer to a usb_interface's embedded struct device.  
Then you wouldn't need to compute udev, and the same would be true for 
other callers.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs

[Alan Stern]

On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.02.21 um 14:44 schrieb Takashi Iwai:

> > Aside from the discussion whether this "workaround" is needed, the use
> > of udev->bus->controller here looks a bit suspicious.  As the old USB
> > code (before the commit 6eb0233ec2d0) indicated, it was rather
> > usb->bus->sysdev that was used for the DMA mask, and it's also the one
> > most of USB core code refers to.  A similar question came up while
> > fixing the same kind of bug in the media subsystem, and we concluded
> > that bus->sysdev is a better choice.
> 
> Good to hear that we're not the only ones affected by this. Wrt the original
> code, using sysdev makes even more sense.

Hmmm, I had forgotten about this.  So DMA masks aren't inherited after 
all, thanks to commit 6eb0233ec2d0.  That leas me to wonder how well 
usb-storage is really working these days...

The impression I get is that Greg would like the USB core to export a 
function which takes struct usb_interface * as argument and returns the 
appropriate DMA mask value.  Then instead of messing around with USB 
internals, drm_gem_prime_import_usb could just call this new function.

Adding such a utility function would be a sufficiently small change that 
it could go into the -stable kernels with no trouble.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs

[Alan Stern]

On Tue, Feb 23, 2021 at 12:19:56PM +0100, Greg KH wrote:
> On Tue, Feb 23, 2021 at 11:58:42AM +0100, Thomas Zimmermann wrote:

> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > 
> >  #include 
> >  #include 
> > @@ -1055,3 +1056,38 @@ void drm_prime_gem_destroy(struct drm_gem_object 
> > *obj, struct sg_table *sg)
> > dma_buf_put(dma_buf);
> >  }
> >  EXPORT_SYMBOL(drm_prime_gem_destroy);
> > +
> > +/**
> > + * drm_gem_prime_import_usb - helper library implementation of the import 
> > callback for USB devices
> > + * @dev: drm_device to import into
> > + * @dma_buf: dma-buf object to import
> > + *
> > + * This is an implementation of drm_gem_prime_import() for USB-based 
> > devices.
> > + * USB devices cannot perform DMA directly. This function selects the USB 
> > host
> > + * controller as DMA device instead. Drivers can use this as their
> > + * _driver.gem_prime_import implementation.
> > + *
> > + * See also drm_gem_prime_import().
> > + */
> > +#ifdef CONFIG_USB
> > +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> > +   struct dma_buf *dma_buf)
> > +{
> > +   struct usb_device *udev;
> > +   struct device *usbhost;
> > +
> > +   if (dev->dev->bus != _bus_type)
> > +   return ERR_PTR(-ENODEV);
> > +
> > +   udev = interface_to_usbdev(to_usb_interface(dev->dev));
> > +   if (!udev->bus)
> > +   return ERR_PTR(-ENODEV);
> > +
> > +   usbhost = udev->bus->controller;
> > +   if (!usbhost || !usbhost->dma_mask)
> > +   return ERR_PTR(-ENODEV);

Thomas, I doubt that you have to go through all of this.  The 
usb-storage driver, for instance, just uses the equivalent of 
dev->dev->dma_mask.  Even though USB devices don't do DMA themselves, 
the DMA mask value is inherited from the parent host controller's device 
struct.

Have you tried doing this?

> If individual USB drivers need access to this type of thing, shouldn't
> that be done in the USB core itself?
> 
> {hint, yes}
> 
> There shouldn't be anything "special" about a DRM driver that needs this
> vs. any other driver that might want to know about DMA things related to
> a specific USB device.  Why isn't this an issue with the existing
> storage or v4l USB devices?

If Thomas finds that the approach I outlined above works, then the rest 
of this email thread becomes moot.  :-)

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Alan Stern]

On Fri, Feb 19, 2021 at 04:56:16PM +0100, Christian König wrote:
> 
> 
> Am 19.02.21 um 16:53 schrieb Alan Stern:
> > On Fri, Feb 19, 2021 at 02:45:54PM +0100, Christian König wrote:
> > > Well as far as I can see this is a relative clear NAK.
> > > 
> > > When a device can't do DMA and has no DMA mask then why it is requesting 
> > > an
> > > sg-table in the first place?
> > This may not be important for your discussion, but I'd like to give an
> > answer to the question -- at least, for the case of USB.
> > 
> > A USB device cannot do DMA and has no DMA mask.  Nevertheless, if you
> > want to send large amounts of bulk data to/from a USB device then using
> > an SG table is often a good way to do it.  The reason is simple: All
> > communication with a USB device has to go through a USB host controller,
> > and many (though not all) host controllers _can_ do DMA and _do_ have a
> > DMA mask.
> > 
> > The USB mass-storage and uas drivers in particular make heavy use of
> > this mechanism.
> 
> Yeah, I was assuming something like that would work.
> 
> But in this case the USB device should give the host controllers device
> structure to the dma_buf_attach function so that the sg_table can be filled
> in with DMA addresses properly.

Indeed.  Although in the contexts I'm familiar with, the host controller 
device is actually passed to routines like dma_pool_create, 
dma_alloc_coherent, dma_map_sg, or dma_map_single.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Alan Stern]

On Fri, Feb 19, 2021 at 02:45:54PM +0100, Christian König wrote:
> Well as far as I can see this is a relative clear NAK.
> 
> When a device can't do DMA and has no DMA mask then why it is requesting an
> sg-table in the first place?

This may not be important for your discussion, but I'd like to give an 
answer to the question -- at least, for the case of USB.

A USB device cannot do DMA and has no DMA mask.  Nevertheless, if you 
want to send large amounts of bulk data to/from a USB device then using 
an SG table is often a good way to do it.  The reason is simple: All 
communication with a USB device has to go through a USB host controller, 
and many (though not all) host controllers _can_ do DMA and _do_ have a 
DMA mask.

The USB mass-storage and uas drivers in particular make heavy use of 
this mechanism.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 21/30] usb: host: ehci-tegra: Support OPP and SoC core voltage scaling

[Alan Stern]

On Thu, Nov 05, 2020 at 02:44:18AM +0300, Dmitry Osipenko wrote:
> Add initial OPP and SoC core voltage scaling support to the Tegra EHCI
> driver. This is required for enabling system-wide DVFS on older Tegra
> SoCs.
> 
> Tested-by: Peter Geis 
> Tested-by: Nicolas Chauvet 
> Signed-off-by: Dmitry Osipenko 
> ---

I'm no expert on OPP stuff, but some of what you have done here looks 
peculiar.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 869d9c4de5fc..0976577f54b4 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -364,6 +365,79 @@ static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd 
> *hcd, struct urb *urb)
>   free_dma_aligned_buffer(urb);
>  }
>  
> +static void tegra_ehci_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_tegra_ehci_init_opp_table(struct device *dev)
> +{
> + unsigned long rate = ULONG_MAX;
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + struct dev_pm_opp *opp;
> + int err;
> +
> + /* legacy device-trees don't have OPP table */
> + if (!device_property_present(dev, "operating-points-v2"))
> + return 0;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, , 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> +  "failed to prepare OPP table\n");
> +
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> +
> + /* find suitable OPP for the maximum clock rate */
> + opp = dev_pm_opp_find_freq_floor(dev, );
> + err = PTR_ERR_OR_ZERO(opp);
> + if (err) {
> + dev_err(dev, "failed to get OPP: %d\n", err);
> + goto remove_table;
> + }
> +
> + dev_pm_opp_put(opp);
> +
> + /*
> +  * First dummy rate-set initializes voltage vote by setting voltage
> +  * in accordance to the clock rate.
> +  */
> + err = dev_pm_opp_set_rate(dev, rate);
> + if (err) {
> + dev_err(dev, "failed to initialize OPP clock: %d\n", err);
> + goto remove_table;
> + }
> +
> + err = devm_add_action(dev, tegra_ehci_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);

Do you really want to use the same error unwinding for opp_table values 
obtained from dev_pm_opp_set_regulators() as from 
dev_pm_opp_get_opp_table()?

> +
> + return err;
> +}
> +
>  static const struct tegra_ehci_soc_config tegra30_soc_config = {
>   .has_hostpc = true,
>  };
> @@ -431,6 +505,11 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>   goto cleanup_hcd_create;
>   }
>  
> + err = devm_tegra_ehci_init_opp_table(>dev);
> + if (err)
> + return dev_err_probe(>dev, err,
> +  "Failed to initialize OPP\n");

Why log a second error message?  Just return err.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Alan Stern]

On Tue, Jun 02, 2020 at 05:21:50AM +, Peter Stuge wrote:
> > The USB protocol forbids a device from sending a STALL response to a
> > SETUP packet.  The only valid response is ACK.  Thus, there is no way
> > to prevent the host from sending its DATA packet for a control-OUT
> > transfer.
> 
> Right; a STALL handshake only after a DATA packet, but a udc could silently
> drop the first DATA packet if instructed to STALL during SETUP processing.
> I don't know how common that is for the udc:s supported by gadget, but some
> MCU:s behave like that.

It happens from time to time, such as when the host sends a SETUP packet 
that the gadget driver doesn't understand.

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.
> 
> How can it do that for OUT, and IN if possible there too?

In the way described just above: The gadget driver's SETUP handler tells 
the UDC to stall the data packet.

> Is it related to f->setup() returning < 0 ?

Yes; the composite core interprets such a value as meaning to STALL 
endpoint 0.

> The spec also allows NAK, but the gadget code seems to not (need to?)
> explicitly support that. Can you comment on this as well?

If the gadget driver doesn't submit a usb_request then the UDC will 
reply with NAK.

> > Once the driver knows what the data packet contains, the gadget API
> > doesn't provide any way to STALL the status stage.
> 
> Thanks. I think this particular gadget driver doesn't need to decide late.
> 
> Ideally the control transfers can even be avoided.


On Tue, Jun 02, 2020 at 01:46:38PM +0200, Noralf Trønnes wrote:

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.  Once the driver knows what
> > the data packet contains, the gadget API doesn't provide any way to
> > STALL the status stage.  There has been a proposal to change the API
> > to make this possible, but so far it hasn't gone forward.
> > 
> 
> This confirms what I have seen in the kernel and the reason I added a
> status request so I can know the result of the operation the device has
> performed.

Does this status request use ep0 or some other endpoint?

> I have a problem that I've encountered with this status request.
> In my first version the gadget would usb_ep_queue() the status value
> when the operation was done and as long as this happended within the
> host timeout (5s) everything worked fine.
> 
> Then I hit a 10s timeout in the gadget when performing a display modeset
> operation (wait on missing vblank). The result of this was that the host
> timed out and moved on. The gadget however didn't know that the host
> gave up, so it queued up the status value. The result of this was that
> all further requests from the host would time out.
> Do you know a solution to this?
> My work around is to just poll on the status request, which returns a
> value immediately, until there's a result. The udc driver I use is dwc2.

It's hard to give a precise answer without knowing the details of how 
your driver works.

There are two reasonable approaches you could use.  One is to have a 
vendor-specific control request to get the result of the preceding 
operation.  This works but it has high overhead -- which may not matter 
if it happens infrequently and isn't sensitive to latency.

The other approach is to send the status data over a bulk-IN endpoint.  
It would have to be formatted in such a way that the host could 
recognize it as a status packet and not some other sort of data packet.  
That way, if the host received a stale status value, it could ignore it 
and move on.

You also may want to give some thought to a "resynchronization" 
protocol, for use in situations where the host times out waiting for a 
response from the device while the device is waiting for something else 
(the host, a vblank, or whatever).  This could be a special control 
request, or you could rely on the host doing a complete USB reset.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Alan Stern]

On Tue, Jun 02, 2020 at 12:12:07AM +, Peter Stuge wrote:

...

> The way I read composite_setup() after try_fun_setup: it calls f->setup()
> when available, and that can return < 0 to stall.
> 
> I expect that composite_setup() and thus f->setup() run when the
> SETUP packet has arrived, thus before the data packet arrives, and if
> composite_setup() stalls then the device/function should never see the
> data packet.
> 
> For an OUT transaction I think the host controller might still send
> the DATA packet, but the device controllers that I know don't make it
> visible to the application in that case.

...

Are you guys interested in comments from other people who know more
about the kernel and how it works with USB?  Your messages have been
far too long to go into in any detail, but I will address this one issue.

The USB protocol forbids a device from sending a STALL response to a
SETUP packet.  The only valid response is ACK.  Thus, there is no way
to prevent the host from sending its DATA packet for a control-OUT
transfer.

A gadget driver can STALL in response to a control-OUT data packet,
but only before it has seen the packet.  Once the driver knows what
the data packet contains, the gadget API doesn't provide any way to
STALL the status stage.  There has been a proposal to change the API
to make this possible, but so far it hasn't gone forward.

Alan Stern
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/9] mfd: Add driver for Multifunction USB Device

[Alan Stern]

On Sat, 29 Feb 2020, Noralf Trønnes wrote:

> >> +static void mud_irq_urb_completion(struct urb *urb)
> >> +{
> >> +  struct device *dev = >dev->dev;
> >> +  int ret;
> >> +
> >> +  mdebug("%s: actual_length=%u\n", __func__, urb->actual_length);
> >> +
> >> +  switch (urb->status) {
> >> +  case 0:
> >> +  mud_irq_queue(urb);
> >> +  break;
> >> +  case -EPROTO:   /* FIXME: verify: dwc2 reports this on disconnect */
> > 
> > What does this mean?  Why can't you fix it now?
> 
> I don't know if this is a dwc2 driver problem or if EPROTO is a valid
> disconnect error. I haven't seen it in other gadget drivers, so I need

Note: This is not a gadget driver.  You should be looking in device 
drivers.

> to look more into this or even better if someone from USB can answer this.

See Documentation/driver-api/usb/error-codes.rst.  In short, -EPROTO is
one of several status codes you may get when an URB fails because the
device was disconnected.

> >> +  case -ECONNRESET:
> >> +  case -ENOENT:
> >> +  case -ESHUTDOWN:
> >> +  dev_dbg(dev, "irq urb shutting down with status: %d\n", 
> >> urb->status);
> > 
> > s/irq/IRQ/ in all comments and prints.
> > 
> > Same with URB?
> > 
> >> +  return;
> >> +  default:
> >> +  dev_dbg(dev, "irq urb failure with status: %d\n", urb->status);
> >> +  break;
> > 
> > So it's failed, but you're going to attempt to submit it anyway?
> 
> Yes, I don't know the reason why it failed, it might succeed the next
> time. But this is also something that someone with real life experience
> with USB failures could weigh in on. Maybe I should send a reset request
> so the device can reset its state machine, I don't know.

USB connections are usually pretty reliable.  Sometimes there are
transient errors, but they are relatively rare.  No one would criticize
a driver for giving up the first time it gets an error (especially if
there was an easy way to reset it) -- but people will get annoyed if a
ton of error messages shows up on the console whenever they unplug the
device.

In general, the overall design of the driver seems to be reasonable.  
I can't judge the interfaces with other subsystems or the other aspects
of their design, but the USB part is okay.  (I haven't gone through it
in detail.)

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/11] docs: Fix some broken references

[Alan Stern]

On Wed, 9 May 2018, Mauro Carvalho Chehab wrote:

> Em Wed, 9 May 2018 19:15:01 +0200
> Andrea Parri <parri.and...@gmail.com> escreveu:

> > >  tools/memory-model/README | 10 +-  
> > 
> > As mentioned in the previous thread, I am for keeping the current
> > references: the REAMDE is listing the doc files, as well as other
> > files in tools/memory-model/, relatively to that directory.
> 
> Yeah, at least this hunk deserves some rework, as now some
> references are Documentation/.../foo, while others are just
> bar.
> 
> As on (almost) all other places (except for tools/memory-model/README),
> the references are always from the main directory, I would make all
> patches there also relative to main dir. If you're afraid of
> not being too clearer, we could prefix all of them with something
> like:
> 
>   ${LINUX}/tools/memory-model/...
> 
> just like some DT binding files do:
> 
>   Documentation/devicetree/bindings/sound/audio-graph-card.txt:see 
> ${LINUX}/Documentation/devicetree/bindings/graph.txt
> 
> A bonus of doing that is that the broken reference detect script can
> keep parsing it without changes (well, it wouldn't be hard to make
> it also accept a relative file, but doing that just due to 
> tools/memory-model/README seems overkill).
> 
> Another advantage is that it would allow to easily add references
> there from the main kernel Documentation, if needed in the future,
> without messing with local x non-local relative namespace.

How about changing the relative references so that something like
Documentation/recipes.txt becomes ./Documentation/recipes.txt?

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[GIT PULL] On-demand device probing

[Alan Stern]

On Tue, 20 Oct 2015, Tomeu Vizoso wrote:

> On 20 October 2015 at 18:04, Alan Stern  wrote:
> > On Tue, 20 Oct 2015, Mark Brown wrote:
> >
> >> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
> >>
> >> > Furthermore, that applies only to devices that use synchronous suspend.
> >> > Async suspend is becoming common, and there the only restrictions are
> >> > parent-child relations plus whatever explicit requirements that drivers
> >> > impose by calling device_pm_wait_for_dev().
> >>
> >> Hrm, this is the first I'd noticed that feature though I see the initial
> >> commit dates from January.
> >
> > Async suspend and device_pm_wait_for_dev() were added in January 2010,
> > not 2015!
> >
> >>  It looks like most of the users are PCs at
> >> the minute but we should be using it more widely for embedded things,
> >> there's definitely some cases I'm aware of where it will allow us to
> >> remove some open coding.
> >>
> >> It does seem like we want to be feeding dependency information we
> >> discover for probing way into the suspend dependencies...
> >
> > Rafael has been thinking about a way to do this systematically.
> > Nothing concrete has emerged yet.
> 
> This iteration of the series would make this quite easy, as
> dependencies are calculated before probes are attempted:
> 
> https://lkml.org/lkml/2015/6/17/311

But what Rafael is proposing is quite general; it would apply to _all_
dependencies as opposed to just those present in DT drivers or those 
affecting platform_devices.

Alan Stern

[GIT PULL] On-demand device probing

[Alan Stern]

On Tue, 20 Oct 2015, Mark Brown wrote:

> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
> 
> > Furthermore, that applies only to devices that use synchronous suspend.  
> > Async suspend is becoming common, and there the only restrictions are 
> > parent-child relations plus whatever explicit requirements that drivers 
> > impose by calling device_pm_wait_for_dev().
> 
> Hrm, this is the first I'd noticed that feature though I see the initial
> commit dates from January.

Async suspend and device_pm_wait_for_dev() were added in January 2010, 
not 2015!

>  It looks like most of the users are PCs at
> the minute but we should be using it more widely for embedded things,
> there's definitely some cases I'm aware of where it will allow us to
> remove some open coding.
> 
> It does seem like we want to be feeding dependency information we
> discover for probing way into the suspend dependencies...

Rafael has been thinking about a way to do this systematically.  
Nothing concrete has emerged yet.

Alan Stern

[GIT PULL] On-demand device probing

[Alan Stern]

On Tue, 20 Oct 2015, Rob Herring wrote:

> > The probe ordering is not the entire picture, though.
> >
> > Even if you get the probe ordering right, the problem is going to show up in
> > multiple other places: system suspend/resume, runtime PM, system shutdown,
> > unbinding of drivers.  In all of those cases it is necessary to handle 
> > things
> > in a specific order if there is a dependency.
> 
> My understanding was with deferred probe that it also solves suspend
> ordering problems because things are suspended in reverse order of
> probing.

Devices are suspended in reverse order of _registration_.  Not of 
probing.

Furthermore, that applies only to devices that use synchronous suspend.  
Async suspend is becoming common, and there the only restrictions are 
parent-child relations plus whatever explicit requirements that drivers 
impose by calling device_pm_wait_for_dev().

Alan Stern

gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt respo

[Alan Stern]

On Wed, 20 Mar 2013, Jiri Kosina wrote:

> > I don't know of any way.  In fact, I have been thinking of writing a 
> > test driver module, with a module parameter telling it which IRQ number 
> > to register for.  It seems like the sort of thing that would be useful 
> > to have, from time to time.
> 
> Ok, so how about this?
> Daniel, is it enough to make the problem appear on your system (by 
> building this into the kernel and booting with dummy-irq.irq=16)?
> 
> Thanks.
> 
> 
> 
> 
> 
> From: Jiri Kosina 
> Subject: [PATCH] dummy-irq: introduce a dummy IRQ handler driver
> 
> This module accepts a single 'irq' parameter, which it should register for.
> 
> Its sole purpose is to help with debugging of IRQ sharing problems, by
> force-enabling IRQ that would otherwise be disabled.
> 
> Suggested-by: Alan Stern 
> Signed-off-by: Jiri Kosina 

This is just what I was thinking of.  Three extremely minor 
suggestions...

> +static irqreturn_t dummy_interrupt(int irq, void *dev_id)
> +{
> + static int count = 0;
> +
> + if (count == 0) {
> + printk("dummy-irq: interrupt occured on IRQ %d\n", irq);

You probably should put a severity level here.  KERN_INFO?

> + count++;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +static int __init dummy_irq_init(void)
> +{
> + if (request_irq(irq, _interrupt, IRQF_SHARED, "dummy_irq", )) 
> {
> + printk(KERN_ERR "dummy-irq: cannot register IRQ %d\n", irq);
> + return -EIO;
> + }
> + printk(KERN_INFO "dummy-irq: registered for IRQ %d\n", irq);
> + return 0;
> +}
> +
> +static void __exit dummy_irq_exit(void)
> +{
> + printk(KERN_INFO "dummy-irq unloaded\n");
> + free_irq(irq, );
> + return;

A return statement isn't needed here.

> +}
> +
> +module_init(dummy_irq_init);
> +module_exit(dummy_irq_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jiri Kosina");
> +module_param_named(irq, irq, uint, 0444);

module_param is good enough when the parameter's name is the same as 
the variable's name.

> +MODULE_PARM_DESC(irq, "The IRQ to register for");

Alan Stern

Re: gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt respo

[Alan Stern]

On Wed, 20 Mar 2013, Jiri Kosina wrote:

  I don't know of any way.  In fact, I have been thinking of writing a 
  test driver module, with a module parameter telling it which IRQ number 
  to register for.  It seems like the sort of thing that would be useful 
  to have, from time to time.
 
 Ok, so how about this?
 Daniel, is it enough to make the problem appear on your system (by 
 building this into the kernel and booting with dummy-irq.irq=16)?
 
 Thanks.
 
 
 
 
 
 From: Jiri Kosina jkos...@suse.cz
 Subject: [PATCH] dummy-irq: introduce a dummy IRQ handler driver
 
 This module accepts a single 'irq' parameter, which it should register for.
 
 Its sole purpose is to help with debugging of IRQ sharing problems, by
 force-enabling IRQ that would otherwise be disabled.
 
 Suggested-by: Alan Stern st...@rowland.harvard.edu
 Signed-off-by: Jiri Kosina jkos...@suse.cz

This is just what I was thinking of.  Three extremely minor 
suggestions...

 +static irqreturn_t dummy_interrupt(int irq, void *dev_id)
 +{
 + static int count = 0;
 +
 + if (count == 0) {
 + printk(dummy-irq: interrupt occured on IRQ %d\n, irq);

You probably should put a severity level here.  KERN_INFO?

 + count++;
 + }
 +
 + return IRQ_NONE;
 +}
 +
 +static int __init dummy_irq_init(void)
 +{
 + if (request_irq(irq, dummy_interrupt, IRQF_SHARED, dummy_irq, irq)) 
 {
 + printk(KERN_ERR dummy-irq: cannot register IRQ %d\n, irq);
 + return -EIO;
 + }
 + printk(KERN_INFO dummy-irq: registered for IRQ %d\n, irq);
 + return 0;
 +}
 +
 +static void __exit dummy_irq_exit(void)
 +{
 + printk(KERN_INFO dummy-irq unloaded\n);
 + free_irq(irq, irq);
 + return;

A return statement isn't needed here.

 +}
 +
 +module_init(dummy_irq_init);
 +module_exit(dummy_irq_exit);
 +
 +MODULE_LICENSE(GPL);
 +MODULE_AUTHOR(Jiri Kosina);
 +module_param_named(irq, irq, uint, 0444);

module_param is good enough when the parameter's name is the same as 
the variable's name.

 +MODULE_PARM_DESC(irq, The IRQ to register for);

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt respo

[Alan Stern]

On Tue, 19 Mar 2013, Daniel Vetter wrote:

> > That might be misleading.  It's possible that the erroneous IRQs _are_
> > being issued but you're simply not aware of them.  If the kernel thinks
> > that no device is using IRQ 16 then it will leave that IRQ disabled.
> 
> I guess I should have phrased it more precisely, but that's exactly
> what I expect is happening on my machine: I don't have anything on
> irq16 (i.e. in non-msi mode the gfx interrupt isn't shared) and hence
> the irq is completely disabled. Which obviously makes it impossible
> for me to reproduce the issue. To test that theory, is there a quick
> way to force-enable a given interrupt, short of just hacking up a 2nd
> dummy irq handler in my driver?

I don't know of any way.  In fact, I have been thinking of writing a 
test driver module, with a module parameter telling it which IRQ number 
to register for.  It seems like the sort of thing that would be useful 
to have, from time to time.

Alan Stern

gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses

[Alan Stern]

On Tue, 19 Mar 2013, Daniel Vetter wrote:

> For reference below the updated commit message.
> 
> Cheers, Daniel
> 
> Author: Jiri Kosina 
> Date:   Tue Mar 19 09:56:57 2013 +0100

> 
> drm/i915: stop using GMBUS IRQs on Gen4 chips
> 
> Commit 28c70f162 ("drm/i915: use the gmbus irq for waits") switched to
> using GMBUS irqs instead of GPIO bit-banging for chipset generations 4
> and above.
> 
> It turns out though that on many systems this leads to spurious interrupts
> being generated, long after the register write to disable the IRQs has 
> been
> issued.
> 
> Typically this results in the spurious interrupt source getting
> disabled:
> 
> [9.636345] irq 16: nobody cared (try booting with the "irqpoll" 
> option)
> [9.637915] Pid: 4157, comm: ifup Tainted: GF
> 3.9.0-rc2-00341-g0863702 #422
> [9.639484] Call Trace:
> [9.640731][] __report_bad_irq+0x1d/0xc7
> [9.640731]  [] note_interrupt+0x15b/0x1e8
> [9.640731]  [] handle_irq_event_percpu+0x1bf/0x214
> [9.640731]  [] handle_irq_event+0x3c/0x5c
> [9.640731]  [] handle_fasteoi_irq+0x7a/0xb0
> [9.640731]  [] handle_irq+0x1a/0x24
> [9.640731]  [] do_IRQ+0x48/0xaf
> [9.640731]  [] common_interrupt+0x6a/0x6a
> [9.640731][] ? 
> system_call_fastpath+0x16/0x1b
> [9.640731] handlers:
> [9.640731] [] usb_hcd_irq [usbcore]
> [9.640731] [] yenta_interrupt [yenta_socket]
> [9.640731] Disabling IRQ #16
> 
> The really curious thing is now that irq 16 is _not_ the interrupt for
> the i915 driver when using MSI, but it _is_ the interrupt when not
> using MSI. So by all indications it seems like gmbus is able to
> generate a legacy (shared) interrupt in MSI mode on some
> configurations. I've tried to reproduce this and the differentiating
> thing seems to be that on unaffected systems no other device uses irq
> 16 (which seems to be the non-MSI intel gfx interrupt on all gm45).

That might be misleading.  It's possible that the erroneous IRQs _are_
being issued but you're simply not aware of them.  If the kernel thinks
that no device is using IRQ 16 then it will leave that IRQ disabled.

Alan Stern

Re: gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt respo

[Alan Stern]

On Tue, 19 Mar 2013, Daniel Vetter wrote:

 For reference below the updated commit message.
 
 Cheers, Daniel
 
 Author: Jiri Kosina jkos...@suse.cz
 Date:   Tue Mar 19 09:56:57 2013 +0100

 
 drm/i915: stop using GMBUS IRQs on Gen4 chips
 
 Commit 28c70f162 (drm/i915: use the gmbus irq for waits) switched to
 using GMBUS irqs instead of GPIO bit-banging for chipset generations 4
 and above.
 
 It turns out though that on many systems this leads to spurious interrupts
 being generated, long after the register write to disable the IRQs has 
 been
 issued.
 
 Typically this results in the spurious interrupt source getting
 disabled:
 
 [9.636345] irq 16: nobody cared (try booting with the irqpoll 
 option)
 [9.637915] Pid: 4157, comm: ifup Tainted: GF
 3.9.0-rc2-00341-g0863702 #422
 [9.639484] Call Trace:
 [9.640731]  IRQ  [8109b40d] __report_bad_irq+0x1d/0xc7
 [9.640731]  [8109b7db] note_interrupt+0x15b/0x1e8
 [9.640731]  [810999f7] handle_irq_event_percpu+0x1bf/0x214
 [9.640731]  [81099a88] handle_irq_event+0x3c/0x5c
 [9.640731]  [8109c139] handle_fasteoi_irq+0x7a/0xb0
 [9.640731]  [8100400e] handle_irq+0x1a/0x24
 [9.640731]  [81003d17] do_IRQ+0x48/0xaf
 [9.640731]  [8142f1ea] common_interrupt+0x6a/0x6a
 [9.640731]  EOI  [8142f952] ? 
 system_call_fastpath+0x16/0x1b
 [9.640731] handlers:
 [9.640731] [a000d771] usb_hcd_irq [usbcore]
 [9.640731] [a0306189] yenta_interrupt [yenta_socket]
 [9.640731] Disabling IRQ #16
 
 The really curious thing is now that irq 16 is _not_ the interrupt for
 the i915 driver when using MSI, but it _is_ the interrupt when not
 using MSI. So by all indications it seems like gmbus is able to
 generate a legacy (shared) interrupt in MSI mode on some
 configurations. I've tried to reproduce this and the differentiating
 thing seems to be that on unaffected systems no other device uses irq
 16 (which seems to be the non-MSI intel gfx interrupt on all gm45).

That might be misleading.  It's possible that the erroneous IRQs _are_
being issued but you're simply not aware of them.  If the kernel thinks
that no device is using IRQ 16 then it will leave that IRQ disabled.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: gm45 intel gfx can generate non-MSI irq# in MSI mode (was Re: [PATCH] drm/i915: stop using GMBUS IRQs on Gen4 chips (was Re: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt respo

[Alan Stern]

On Tue, 19 Mar 2013, Daniel Vetter wrote:

  That might be misleading.  It's possible that the erroneous IRQs _are_
  being issued but you're simply not aware of them.  If the kernel thinks
  that no device is using IRQ 16 then it will leave that IRQ disabled.
 
 I guess I should have phrased it more precisely, but that's exactly
 what I expect is happening on my machine: I don't have anything on
 irq16 (i.e. in non-msi mode the gfx interrupt isn't shared) and hence
 the irq is completely disabled. Which obviously makes it impossible
 for me to reproduce the issue. To test that theory, is there a quick
 way to force-enable a given interrupt, short of just hacking up a 2nd
 dummy irq handler in my driver?

I don't know of any way.  In fact, I have been thinking of writing a 
test driver module, with a module parameter telling it which IRQ number 
to register for.  It seems like the sort of thing that would be useful 
to have, from time to time.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

runtime PM and special power switches

[Alan Stern]

On Tue, 11 Sep 2012, Rafael J. Wysocki wrote:

> Hi,
> 
> On Tuesday, September 11, 2012, Dave Airlie wrote:
> > Hi Rafael,
> > 
> > I've been investigating runtime PM support for some use-cases on GPUs.
> > 
> > In some laptops we have a secondary GPU (optimus) that can be powered
> > up for certain 3D tasks and then turned off when finished with. Now I
> > did an initial pass on supporting it without using the kernel runtime
> > PM stuff, but Alan said I should take a look so here I am.
> 
> Alan Stern or Alan Cox? :-)
> 
> > While I've started to get a handle on things, we have a bit of an
> > extra that I'm not sure we cater for.
> > 
> > Currently we get called from the PCI layer which after we are finished
> > with our runtime suspend callback, will go put the device into the
> > correct state etc, however on these optimus/powerxpress laptops we
> > have a separate ACPI or platform driver controlled power switch that
> > we need to call once the PCI layer is finished the job. This switch
> > effectively turns the power to the card completely off leaving it
> > drawing no power.
> > 
> > No we can't hit the switch from the driver callback as the PCI layer
> > will get lost, so I'm wondering how you'd envisage we could plug this
> > in.
> 
> Hmm.  In principle we might modify pci_pm_runtime_suspend() so that it
> doesn't call pci_finish_runtime_suspend() if pci_dev->state_saved is
> set.  That would actually make it work in analogy with pci_pm_suspend_noirq(),
> so perhaps it's not even too dangerous.

This sounds more like a job for a power domain.  Unless the power
switch is already in the device hierarchy as a parent to the PCI
device.

Alan Stern

Re: runtime PM and special power switches

[Alan Stern]

On Tue, 11 Sep 2012, Rafael J. Wysocki wrote:

 Hi,
 
 On Tuesday, September 11, 2012, Dave Airlie wrote:
  Hi Rafael,
  
  I've been investigating runtime PM support for some use-cases on GPUs.
  
  In some laptops we have a secondary GPU (optimus) that can be powered
  up for certain 3D tasks and then turned off when finished with. Now I
  did an initial pass on supporting it without using the kernel runtime
  PM stuff, but Alan said I should take a look so here I am.
 
 Alan Stern or Alan Cox? :-)
 
  While I've started to get a handle on things, we have a bit of an
  extra that I'm not sure we cater for.
  
  Currently we get called from the PCI layer which after we are finished
  with our runtime suspend callback, will go put the device into the
  correct state etc, however on these optimus/powerxpress laptops we
  have a separate ACPI or platform driver controlled power switch that
  we need to call once the PCI layer is finished the job. This switch
  effectively turns the power to the card completely off leaving it
  drawing no power.
  
  No we can't hit the switch from the driver callback as the PCI layer
  will get lost, so I'm wondering how you'd envisage we could plug this
  in.
 
 Hmm.  In principle we might modify pci_pm_runtime_suspend() so that it
 doesn't call pci_finish_runtime_suspend() if pci_dev-state_saved is
 set.  That would actually make it work in analogy with pci_pm_suspend_noirq(),
 so perhaps it's not even too dangerous.

This sounds more like a job for a power domain.  Unless the power
switch is already in the device hierarchy as a parent to the PCI
device.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Block: use a freezable workqueue for disk-event polling

[Alan Stern]

This patch (as1519) fixes a bug in the block layer's disk-events
polling.  The polling is done by a work routine queued on the
system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
polling continues even in the middle of a system sleep transition.

Obviously, polling a suspended drive for media changes and such isn't
a good thing to do; in the case of USB mass-storage devices it can
lead to real problems requiring device resets and even re-enumeration.

The patch fixes things by creating a new system-wide, non-reentrant,
freezable workqueue and using it for disk-events polling.

Signed-off-by: Alan Stern 
CC: Tejun Heo 
CC: 

---

I'm not sure who to send this patch to, since it is relevant to both
the block and PM subsystems.  Jens, is it okay if Rafael takes it?



 block/genhd.c |   10 +-
 include/linux/workqueue.h |4 
 kernel/workqueue.c|7 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(>dwork.timer, intv / 4);
if (check_now)
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
else if (intv)
-   queue_delayed_work(system_nrt_wq, >dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);
 out_unlock:
spin_unlock_irqrestore(>lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev->clearing |= mask;
if (!ev->block) {
cancel_delayed_work(>dwork);
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
}
spin_unlock_irq(>lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge

/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
flush_delayed_work(>dwork);
__disk_unblock_events(disk, false);

@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo

intv = disk_events_poll_jiffies(disk);
if (!ev->block && intv)
-   queue_delayed_work(system_nrt_wq, >dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);

spin_unlock_irq(>lock);

Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;

 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);

 #define CREATE_TRACE_POINTS
 #include 
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue("events_freezable",
  WQ_FREEZABLE, 0);
+   system_nrt_freezable_wq = alloc_workqueue("events_nrt_freezable",
+   WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-  !system_unbound_wq || !system_freezable_wq);
+  !system_unbound_wq || !system_freezable_wq ||
+   !system_nrt_freezable_wq);
return 0;
 }
 early_initcall(init_workqueues);

[PATCH] Block: use a freezable workqueue for disk-event polling

[Alan Stern]

This patch (as1519) fixes a bug in the block layer's disk-events
polling.  The polling is done by a work routine queued on the
system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
polling continues even in the middle of a system sleep transition.

Obviously, polling a suspended drive for media changes and such isn't
a good thing to do; in the case of USB mass-storage devices it can
lead to real problems requiring device resets and even re-enumeration.

The patch fixes things by creating a new system-wide, non-reentrant,
freezable workqueue and using it for disk-events polling.

Signed-off-by: Alan Stern st...@rowland.harvard.edu
CC: Tejun Heo t...@kernel.org
CC: sta...@kernel.org

---

I'm not sure who to send this patch to, since it is relevant to both
the block and PM subsystems.  Jens, is it okay if Rafael takes it?



 block/genhd.c |   10 +-
 include/linux/workqueue.h |4 
 kernel/workqueue.c|7 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(ev-dwork.timer, intv / 4);
if (check_now)
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
else if (intv)
-   queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
 out_unlock:
spin_unlock_irqrestore(ev-lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev-clearing |= mask;
if (!ev-block) {
cancel_delayed_work(ev-dwork);
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
}
spin_unlock_irq(ev-lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
 
/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
flush_delayed_work(ev-dwork);
__disk_unblock_events(disk, false);
 
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
 
intv = disk_events_poll_jiffies(disk);
if (!ev-block  intv)
-   queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
 
spin_unlock_irq(ev-lock);
 
Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
 
 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
 
 #define CREATE_TRACE_POINTS
 #include trace/events/workqueue.h
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue(events_freezable,
  WQ_FREEZABLE, 0);
+   system_nrt_freezable_wq = alloc_workqueue(events_nrt_freezable,
+   WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-  !system_unbound_wq || !system_freezable_wq);
+  !system_unbound_wq || !system_freezable_wq ||
+   !system_nrt_freezable_wq

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello,
> 
> On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote:
> > Um.  I don't think I can audit all the calls in the kernel that submit
> > block requests and determine which ones need to be allowed while a
> > system sleep is in progress.
> 
> ??? we need to do that anyway and the ones which should go through are
> much smaller than the ones which shouldn't go through.

Agreed.  I'm just saying that it's too big a job for me to handle by
myself.  And all the marking has to be done before you plug the
unmarked requests; otherwise you could break hibernation.


> > Well, there are some dedicated threads that exist for no other purpose
> > than to do I/O to devices and to handle hotplug/unplug events.  I don't
> > see any reason not allow such threads to be freezable.  It's a quick, 
> > convenient method for getting them out of the way.
> 
> Well, it's convenient to use incorrectly.  If you look at most of
> freezable kthreads, they're sadly broken.  I mean, a lot of kthread
> users don't even get kthread_should_stop() right.  With freezable()
> thrown into the mix, it seems hopeless.  With wq, it's better as
> freezing is handled by wq proper.  Even then, I don't know.  It just
> seems to lead people to think "ooh, I marked it freezable so I don't
> have to think about synchronization across PM events.  Freezer will
> magically solve this for me!".  :(

Certainly there are issues which need to be considered carefully.  That 
doesn't mean it should never be used.

Alan Stern

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello,
> 
> On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
> > These should  all be freezable and we might even be able to get away
> > with WQ_UNBOUND for some of these.
> 
> In general, I would recommend specifying as few special attribute as
> possible.  If WQ_UNBOUND is necessary (large amount of CPU cycles
> consumed, extremely high concurrency), sure, but I think we're
> generally better off using as default attributes as possible.  It just
> makes things much easier later when we need to implement new features
> or update the implementation.
> 
> > I think we put most of these in system_nrt_wq because Tejun put an
> > earlier job into that queue when he converted it from slow_work and we
> > just cargo-cult copied that...
> > 
> > I'll spend some time looking at this in the next day or two, but I
> > suspect that the right answer is to just move these off of the "public"
> > workqueues altogether.
> 
> If freezing & nrt is everything necessary, just create
> system_nrt_freezable_wq and use that.

Here's my proposed patch.  If nobody objects, I'll submit it to Rafael
with an appropriate patch description.  Then anybody who wants can
convert over to the new workqueue.

Alan Stern



 block/genhd.c |   10 +-
 include/linux/workqueue.h |4 
 kernel/workqueue.c|7 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(>dwork.timer, intv / 4);
if (check_now)
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
else if (intv)
-   queue_delayed_work(system_nrt_wq, >dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);
 out_unlock:
spin_unlock_irqrestore(>lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev->clearing |= mask;
if (!ev->block) {
cancel_delayed_work(>dwork);
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
}
spin_unlock_irq(>lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge

/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
-   queue_delayed_work(system_nrt_wq, >dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
flush_delayed_work(>dwork);
__disk_unblock_events(disk, false);

@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo

intv = disk_events_poll_jiffies(disk);
if (!ev->block && intv)
-   queue_delayed_work(system_nrt_wq, >dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);

spin_unlock_irq(>lock);

Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;

 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);

 #define CREATE_TRACE_POINTS
 #include 
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello, (cc'ing Rafael and Jens)
> 
> On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote:
> > My question to all of you: Should system_nrt_wq be made freezable, or 
> > should I create a new workqueue that is both freezable and 
> > non-reentrant?  And if I do, which of the usages above should be 
> > converted to the new workqueue?
> 
> Let's make it explicit that the wq is freezable.  I'm a bit
> uncomfortable with the way it's headed.  What we should be doing is
> implementing plugging of request queue for all regular requests while
> suspend is in progress and then annotate the ones which should go
> through.  We're trying to do it the other way around.

Um.  I don't think I can audit all the calls in the kernel that submit
block requests and determine which ones need to be allowed while a
system sleep is in progress.

> Also, in general, I don't think using freezing widely for kernel
> threads / wqs is a good idea.  Plugging device access at subsystem
> layer should cover most cases and we have notifiers to implement such
> support and to handle special cases.  There are even code paths which
> try to determine whether system went through PM operation by looking
> at whether %current went through the freezer.  IMHO, we'll be better
> off with removing freezer support for kthreads.  :(

Well, there are some dedicated threads that exist for no other purpose
than to do I/O to devices and to handle hotplug/unplug events.  I don't
see any reason not allow such threads to be freezable.  It's a quick, 
convenient method for getting them out of the way.

More general-purpose threads, like the async_schedule reservoir and 
workqueue threads, are a different story.

Alan Stern

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

Folks:

I recently uncovered a bug in the block layer.  It uses a workqueue to
periodically probe removable drives for media or other state changes,
and the workqueue it uses is system_nrt_wq.

The bug is that system_nrt_wq is not freezable, so it keeps on running
even while the system is in the process of suspending or hibernating.  
Doing I/O to a suspended drive doesn't work well and in some cases
causes nasty problems.  Obviously these polls need to stop during a 
suspend transition.

A search through the kernel shows that system_nrt_wq is also used in a
few other subsystems:

./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, >work);
./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, >work);
./fs/cifs/misc.c:   queue_work(system_nrt_wq,
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, >echo, 
SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, _ses->echo, 
SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, 
_sb->prune_tlinks,
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, 
_sb->prune_tlinks,

./drivers/mmc/core/host.c:  queue_work(system_nrt_wq, 
>clk_gate_work);

./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, 
DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, 0);

./security/keys/gc.c:   queue_work(system_nrt_wq, _gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, _gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, _gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, _gc_work);
./security/keys/key.c:  queue_work(system_nrt_wq, _gc_work);

My question to all of you: Should system_nrt_wq be made freezable, or 
should I create a new workqueue that is both freezable and 
non-reentrant?  And if I do, which of the usages above should be 
converted to the new workqueue?

Thanks,

Alan Stern

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

Folks:

I recently uncovered a bug in the block layer.  It uses a workqueue to
periodically probe removable drives for media or other state changes,
and the workqueue it uses is system_nrt_wq.

The bug is that system_nrt_wq is not freezable, so it keeps on running
even while the system is in the process of suspending or hibernating.  
Doing I/O to a suspended drive doesn't work well and in some cases
causes nasty problems.  Obviously these polls need to stop during a 
suspend transition.

A search through the kernel shows that system_nrt_wq is also used in a
few other subsystems:

./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, rdata-work);
./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, wdata-work);
./fs/cifs/misc.c:   queue_work(system_nrt_wq,
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, server-echo, 
SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, tcp_ses-echo, 
SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, 
cifs_sb-prune_tlinks,
./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, 
cifs_sb-prune_tlinks,

./drivers/mmc/core/host.c:  queue_work(system_nrt_wq, 
host-clk_gate_work);

./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, dev-mode_config.output_poll_work, 
DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:
queue_delayed_work(system_nrt_wq, dev-mode_config.output_poll_work, 0);

./security/keys/gc.c:   queue_work(system_nrt_wq, key_gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, key_gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, key_gc_work);
./security/keys/gc.c:   queue_work(system_nrt_wq, key_gc_work);
./security/keys/key.c:  queue_work(system_nrt_wq, key_gc_work);

My question to all of you: Should system_nrt_wq be made freezable, or 
should I create a new workqueue that is both freezable and 
non-reentrant?  And if I do, which of the usages above should be 
converted to the new workqueue?

Thanks,

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

 Hello, (cc'ing Rafael and Jens)
 
 On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote:
  My question to all of you: Should system_nrt_wq be made freezable, or 
  should I create a new workqueue that is both freezable and 
  non-reentrant?  And if I do, which of the usages above should be 
  converted to the new workqueue?
 
 Let's make it explicit that the wq is freezable.  I'm a bit
 uncomfortable with the way it's headed.  What we should be doing is
 implementing plugging of request queue for all regular requests while
 suspend is in progress and then annotate the ones which should go
 through.  We're trying to do it the other way around.

Um.  I don't think I can audit all the calls in the kernel that submit
block requests and determine which ones need to be allowed while a
system sleep is in progress.

 Also, in general, I don't think using freezing widely for kernel
 threads / wqs is a good idea.  Plugging device access at subsystem
 layer should cover most cases and we have notifiers to implement such
 support and to handle special cases.  There are even code paths which
 try to determine whether system went through PM operation by looking
 at whether %current went through the freezer.  IMHO, we'll be better
 off with removing freezer support for kthreads.  :(

Well, there are some dedicated threads that exist for no other purpose
than to do I/O to devices and to handle hotplug/unplug events.  I don't
see any reason not allow such threads to be freezable.  It's a quick, 
convenient method for getting them out of the way.

More general-purpose threads, like the async_schedule reservoir and 
workqueue threads, are a different story.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

 Hello,
 
 On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
  These should  all be freezable and we might even be able to get away
  with WQ_UNBOUND for some of these.
 
 In general, I would recommend specifying as few special attribute as
 possible.  If WQ_UNBOUND is necessary (large amount of CPU cycles
 consumed, extremely high concurrency), sure, but I think we're
 generally better off using as default attributes as possible.  It just
 makes things much easier later when we need to implement new features
 or update the implementation.
 
  I think we put most of these in system_nrt_wq because Tejun put an
  earlier job into that queue when he converted it from slow_work and we
  just cargo-cult copied that...
  
  I'll spend some time looking at this in the next day or two, but I
  suspect that the right answer is to just move these off of the public
  workqueues altogether.
 
 If freezing  nrt is everything necessary, just create
 system_nrt_freezable_wq and use that.

Here's my proposed patch.  If nobody objects, I'll submit it to Rafael
with an appropriate patch description.  Then anybody who wants can
convert over to the new workqueue.

Alan Stern



 block/genhd.c |   10 +-
 include/linux/workqueue.h |4 
 kernel/workqueue.c|7 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(ev-dwork.timer, intv / 4);
if (check_now)
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
else if (intv)
-   queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
 out_unlock:
spin_unlock_irqrestore(ev-lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev-clearing |= mask;
if (!ev-block) {
cancel_delayed_work(ev-dwork);
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
}
spin_unlock_irq(ev-lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
 
/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
-   queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
flush_delayed_work(ev-dwork);
__disk_unblock_events(disk, false);
 
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
 
intv = disk_events_poll_jiffies(disk);
if (!ev-block  intv)
-   queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+   queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
 
spin_unlock_irq(ev-lock);
 
Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
 
 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
 
 #define CREATE_TRACE_POINTS
 #include trace/events/workqueue.h
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue(events_freezable

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

 Hello,
 
 On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote:
  Um.  I don't think I can audit all the calls in the kernel that submit
  block requests and determine which ones need to be allowed while a
  system sleep is in progress.
 
 ??? we need to do that anyway and the ones which should go through are
 much smaller than the ones which shouldn't go through.

Agreed.  I'm just saying that it's too big a job for me to handle by
myself.  And all the marking has to be done before you plug the
unmarked requests; otherwise you could break hibernation.


  Well, there are some dedicated threads that exist for no other purpose
  than to do I/O to devices and to handle hotplug/unplug events.  I don't
  see any reason not allow such threads to be freezable.  It's a quick, 
  convenient method for getting them out of the way.
 
 Well, it's convenient to use incorrectly.  If you look at most of
 freezable kthreads, they're sadly broken.  I mean, a lot of kthread
 users don't even get kthread_should_stop() right.  With freezable()
 thrown into the mix, it seems hopeless.  With wq, it's better as
 freezing is handled by wq proper.  Even then, I don't know.  It just
 seems to lead people to think ooh, I marked it freezable so I don't
 have to think about synchronization across PM events.  Freezer will
 magically solve this for me!.  :(

Certainly there are issues which need to be considered carefully.  That 
doesn't mean it should never be used.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Oops in i915 intel_init_clock_gating

[Alan Stern]

On Thu, 23 Jun 2011, Florian Mickler wrote:

> On Wed, 22 Jun 2011 15:04:56 -0400 (EDT)
> Alan Stern  wrote:
> 
> > On Wed, 22 Jun 2011, Keith Packard wrote:
> > 
> > > On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes  > > virtuousgeek.org> wrote:
> > > 
> > > I haven't seen any comments as to whether this patch needs to be merged
> > > into the kernel. Has anyone tested this?
> > 
> > I have tested it, and it worked well on my system:
> > 
> > http://marc.info/?l=linux-kernel=130817292323254=2
> > 
> > Alan Stern
> > 
> Some clock-gating patch was also proposed here:
> https://bugzilla.kernel.org/show_bug.cgi?id=37252

That was a different fix for a different problem, although the symptoms 
were the same.

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Thu, 23 Jun 2011, Florian Mickler wrote:

 On Wed, 22 Jun 2011 15:04:56 -0400 (EDT)
 Alan Stern st...@rowland.harvard.edu wrote:
 
  On Wed, 22 Jun 2011, Keith Packard wrote:
  
   On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes 
   jbar...@virtuousgeek.org wrote:
   
   I haven't seen any comments as to whether this patch needs to be merged
   into the kernel. Has anyone tested this?
  
  I have tested it, and it worked well on my system:
  
  http://marc.info/?l=linux-kernelm=130817292323254w=2
  
  Alan Stern
  
 Some clock-gating patch was also proposed here:
 https://bugzilla.kernel.org/show_bug.cgi?id=37252

That was a different fix for a different problem, although the symptoms 
were the same.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 22 Jun 2011, Keith Packard wrote:

> On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes  virtuousgeek.org> wrote:
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 0defd42..a1a28fb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -429,6 +429,9 @@ static int i915_drm_thaw(struct drm_device *dev)
> > /* KMS EnterVT equivalent */
> > if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > mutex_lock(>struct_mutex);
> > +
> > +   intel_init_clock_gating(dev);
> > +
> > dev_priv->mm.suspended = 0;
> >  
> > error = i915_gem_init_ringbuffer(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c 
> > b/drivers/gpu/drm/i915/i915_suspend.c
> > index 60a94d2..b478d16 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -863,8 +863,6 @@ int i915_restore_state(struct drm_device *dev)
> > I915_WRITE(IMR, dev_priv->saveIMR);
> > }
> >  
> > -   intel_init_clock_gating(dev);
> > -
> > if (IS_IRONLAKE_M(dev)) {
> > ironlake_enable_drps(dev);
> > intel_init_emon(dev);
> > 
> 
> I haven't seen any comments as to whether this patch needs to be merged
> into the kernel. Has anyone tested this?

I have tested it, and it worked well on my system:

http://marc.info/?l=linux-kernel=130817292323254=2

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 22 Jun 2011, Keith Packard wrote:

 On Wed, 15 Jun 2011 13:32:56 -0700, Jesse Barnes jbar...@virtuousgeek.org 
 wrote:
 
  diff --git a/drivers/gpu/drm/i915/i915_drv.c 
  b/drivers/gpu/drm/i915/i915_drv.c
  index 0defd42..a1a28fb 100644
  --- a/drivers/gpu/drm/i915/i915_drv.c
  +++ b/drivers/gpu/drm/i915/i915_drv.c
  @@ -429,6 +429,9 @@ static int i915_drm_thaw(struct drm_device *dev)
  /* KMS EnterVT equivalent */
  if (drm_core_check_feature(dev, DRIVER_MODESET)) {
  mutex_lock(dev-struct_mutex);
  +
  +   intel_init_clock_gating(dev);
  +
  dev_priv-mm.suspended = 0;
   
  error = i915_gem_init_ringbuffer(dev);
  diff --git a/drivers/gpu/drm/i915/i915_suspend.c 
  b/drivers/gpu/drm/i915/i915_suspend.c
  index 60a94d2..b478d16 100644
  --- a/drivers/gpu/drm/i915/i915_suspend.c
  +++ b/drivers/gpu/drm/i915/i915_suspend.c
  @@ -863,8 +863,6 @@ int i915_restore_state(struct drm_device *dev)
  I915_WRITE(IMR, dev_priv-saveIMR);
  }
   
  -   intel_init_clock_gating(dev);
  -
  if (IS_IRONLAKE_M(dev)) {
  ironlake_enable_drps(dev);
  intel_init_emon(dev);
  
 
 I haven't seen any comments as to whether this patch needs to be merged
 into the kernel. Has anyone tested this?

I have tested it, and it worked well on my system:

http://marc.info/?l=linux-kernelm=130817292323254w=2

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 15 Jun 2011, Jesse Barnes wrote:

> On Wed, 15 Jun 2011 16:08:51 -0400 (EDT)
> Alan Stern  wrote:
> 
> > The problem of dev_priv->display.init_clock_gating not getting set is 
> > still present in 3.0-rc3.  On my system this happens because 
> > intel_init_display() never gets called in the first place.
> > 
> > AFAICT, the normal calling sequence during driver initialization is:
> > 
> > i915_driver_load() -> i915_load_modeset_init() ->
> > intel_modeset_init() -> intel_init_display().
> > 
> > But in my case the call to i915_load_modeset_init() doesn't occur 
> > because drm_core_check_feature(dev, DRIVER_MODESET) is False.
> 
> Ouch, a non-KMS config.  Any reason you can't use KMS?

Normally I do use it.  This was a special testing config I've been 
nursing along for years, since well before KMS existed.  Either I never 
enabled KMS in the config, or else at some point it caused trouble so I 
removed it and never added it back.  Can't remember which -- all the 
testing I do with this config is at a VT, never under X.

> This patch should help at any rate.

I confirm that the patch fixes the problem.  Thanks.

On a different but related note, "rmmod i915" incites a lockdep 
notification:

[   54.316439] INFO: trying to register non-static key.
[   54.316589] the code is fine but needs lockdep annotation.
[   54.316729] turning off the locking correctness validator.
[   54.316871] Pid: 1683, comm: rmmod Not tainted 3.0.0-rc3 #2
[   54.317011] Call Trace:
[   54.317153]  [] ? printk+0xf/0x11
[   54.317296]  [] register_lock_class+0x58/0x2d7
[   54.317438]  [] ? get_parent_ip+0xb/0x31
[   54.317579]  [] ? is_module_text_address+0x37/0x45
[   54.317722]  [] ? __kernel_text_address+0x1c/0x3e
[   54.317864]  [] __lock_acquire+0xa3/0xc5a
[   54.318005]  [] ? dump_trace+0x7f/0xa5
[   54.318146]  [] ? __lock_acquire+0xc4b/0xc5a
[   54.318287]  [] lock_acquire+0x5e/0x75
[   54.318427]  [] ? work_on_cpu+0x96/0x96
[   54.318567]  [] wait_on_work+0x3c/0x133
[   54.318707]  [] ? work_on_cpu+0x96/0x96
[   54.318848]  [] ? lock_timer_base.clone.23+0x20/0x3e
[   54.318991]  [] ? _raw_spin_unlock_irqrestore+0x36/0x5b
[   54.319134]  [] ? get_parent_ip+0xb/0x31
[   54.319275]  [] ? sub_preempt_count+0x7c/0x89
[   54.319417]  [] __cancel_work_timer+0xa0/0xde
[   54.319559]  [] cancel_work_sync+0xa/0xc
[   54.319714]  [] i915_driver_unload+0x136/0x224 [i915]
[   54.319874]  [] drm_put_dev+0xa9/0x170 [drm]
[   54.320029]  [] drm_pci_exit+0x49/0x63 [drm]
[   54.320045]  [] i915_exit+0x12/0x742 [i915]
[   54.320045]  [] sys_delete_module+0x175/0x1c1
[   54.320045]  [] ? remove_vma+0x52/0x58
[   54.320045]  [] ? restore_all+0xf/0xf
[   54.320045]  [] sysenter_do_call+0x12/0x36
[   54.336786] [drm] Module unloaded

Is this a known problem?

Alan Stern

Oops in i915 intel_init_clock_gating

[Alan Stern]

The problem of dev_priv->display.init_clock_gating not getting set is 
still present in 3.0-rc3.  On my system this happens because 
intel_init_display() never gets called in the first place.

AFAICT, the normal calling sequence during driver initialization is:

i915_driver_load() -> i915_load_modeset_init() ->
intel_modeset_init() -> intel_init_display().

But in my case the call to i915_load_modeset_init() doesn't occur 
because drm_core_check_feature(dev, DRIVER_MODESET) is False.

I tested with the following patch:


Index: usb-3.0/drivers/gpu/drm/i915/intel_display.c
===
--- usb-3.0.orig/drivers/gpu/drm/i915/intel_display.c
+++ usb-3.0/drivers/gpu/drm/i915/intel_display.c
@@ -7511,6 +7511,10 @@ void intel_init_clock_gating(struct drm_
 {
struct drm_i915_private *dev_priv = dev->dev_private;

+   if (!dev_priv->display.init_clock_gating) {
+   printk(KERN_WARNING "init_clock_gating not set!\n");
+   WARN_ON(1);
+   } else
dev_priv->display.init_clock_gating(dev);

if (dev_priv->display.init_pch_clock_gating)
Index: usb-3.0/drivers/gpu/drm/i915/i915_dma.c
===
--- usb-3.0.orig/drivers/gpu/drm/i915/i915_dma.c
+++ usb-3.0/drivers/gpu/drm/i915/i915_dma.c
@@ -2078,7 +2078,9 @@ int i915_driver_load(struct drm_device *

intel_detect_pch(dev);

+printk(KERN_INFO "Testing drm_core_check_feature DRIVER_MODESET\n");
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+printk(KERN_INFO "Calling i915_load_modeset_init\n");
ret = i915_load_modeset_init(dev);
if (ret < 0) {
DRM_ERROR("failed to init modeset\n");


Here is the dmesg log showing what happens during "insmod i915.ko":

[  908.129497] pci :00:02.0: setting latency timer to 64
[  908.179865] [drm] Supports vblank timestamp caching Rev 1 (10.10.2010).
[  908.180633] [drm] No driver support for vblank timestamp query.
[  908.180804] Testing drm_core_check_feature DRIVER_MODESET
[  908.181120] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0

The first debugging message is printed but not the second.


Here is what happens during a resume from system suspend:

[  943.013656] init_clock_gating not set!
[  943.013790] [ cut here ]
[  943.013954] WARNING: at drivers/gpu/drm/i915/intel_display.c:7516 
intel_init_clock_gating+0x30/0x4a [i915]()
[  943.014193] Hardware name: HP dx2000 MT (EE004AA)
[  943.014330] Modules linked in: i915 fbcon font bitblit softcursor 
drm_kms_helper drm fb fbdev i2c_algo_bit cfbcopyarea i2c_core video backlight 
cfbimgblt cfbfillrect e100 ohci_hcd mii pcspkr evdev uhci_hcd ehci_hcd fan 
processor thermal_sys button usbcore [last unloaded: i915]
[  943.015825] Pid: 1678, comm: bash Not tainted 3.0.0-rc3 #2
[  943.015966] Call Trace:
[  943.016179]  [] warn_slowpath_common+0x65/0x7a
[  943.016342]  [] ? intel_init_clock_gating+0x30/0x4a [i915]
[  943.016487]  [] warn_slowpath_null+0xf/0x13
[  943.016644]  [] intel_init_clock_gating+0x30/0x4a [i915]
[  943.016801]  [] i915_restore_state+0xf4/0x1bf [i915]
[  943.016989]  [] i915_drm_thaw+0x41/0xc1 [i915]
[  943.017141]  [] i915_resume+0x38/0x4b [i915]
[  943.017301]  [] drm_class_resume+0x39/0x3b [drm]
[  943.017447]  [] legacy_resume+0x1e/0x46
[  943.017599]  [] ? drm_class_suspend+0x3d/0x3d [drm]
[  943.017742]  [] device_resume+0x83/0xa0
[  943.017881]  [] dpm_resume+0xdc/0x156
[  943.018020]  [] dpm_resume_end+0xb/0x15
[  943.018162]  [] suspend_devices_and_enter+0x165/0x192
[  943.018330]  [] enter_state+0xd2/0x123
[  943.018471]  [] state_store+0x95/0xa1
[  943.018610]  [] ? pm_async_store+0x33/0x33
[  943.018752]  [] kobj_attr_store+0x16/0x22
[  943.018894]  [] sysfs_write_file+0xb3/0xec
[  943.019034]  [] ? sysfs_open_file+0x1c2/0x1c2
[  943.019176]  [] vfs_write+0x76/0xa2
[  943.019315]  [] sys_write+0x3b/0x5d
[  943.019456]  [] sysenter_do_call+0x12/0x36
[  943.019625] ---[ end trace dc74bd86a8bff7da ]---


What's the right way to fix this?

Alan Stern

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

The problem of dev_priv-display.init_clock_gating not getting set is 
still present in 3.0-rc3.  On my system this happens because 
intel_init_display() never gets called in the first place.

AFAICT, the normal calling sequence during driver initialization is:

i915_driver_load() - i915_load_modeset_init() -
intel_modeset_init() - intel_init_display().

But in my case the call to i915_load_modeset_init() doesn't occur 
because drm_core_check_feature(dev, DRIVER_MODESET) is False.

I tested with the following patch:


Index: usb-3.0/drivers/gpu/drm/i915/intel_display.c
===
--- usb-3.0.orig/drivers/gpu/drm/i915/intel_display.c
+++ usb-3.0/drivers/gpu/drm/i915/intel_display.c
@@ -7511,6 +7511,10 @@ void intel_init_clock_gating(struct drm_
 {
struct drm_i915_private *dev_priv = dev-dev_private;
 
+   if (!dev_priv-display.init_clock_gating) {
+   printk(KERN_WARNING init_clock_gating not set!\n);
+   WARN_ON(1);
+   } else
dev_priv-display.init_clock_gating(dev);
 
if (dev_priv-display.init_pch_clock_gating)
Index: usb-3.0/drivers/gpu/drm/i915/i915_dma.c
===
--- usb-3.0.orig/drivers/gpu/drm/i915/i915_dma.c
+++ usb-3.0/drivers/gpu/drm/i915/i915_dma.c
@@ -2078,7 +2078,9 @@ int i915_driver_load(struct drm_device *
 
intel_detect_pch(dev);
 
+printk(KERN_INFO Testing drm_core_check_feature DRIVER_MODESET\n);
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+printk(KERN_INFO Calling i915_load_modeset_init\n);
ret = i915_load_modeset_init(dev);
if (ret  0) {
DRM_ERROR(failed to init modeset\n);


Here is the dmesg log showing what happens during insmod i915.ko:

[  908.129497] pci :00:02.0: setting latency timer to 64
[  908.179865] [drm] Supports vblank timestamp caching Rev 1 (10.10.2010).
[  908.180633] [drm] No driver support for vblank timestamp query.
[  908.180804] Testing drm_core_check_feature DRIVER_MODESET
[  908.181120] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0

The first debugging message is printed but not the second.


Here is what happens during a resume from system suspend:

[  943.013656] init_clock_gating not set!
[  943.013790] [ cut here ]
[  943.013954] WARNING: at drivers/gpu/drm/i915/intel_display.c:7516 
intel_init_clock_gating+0x30/0x4a [i915]()
[  943.014193] Hardware name: HP dx2000 MT (EE004AA)
[  943.014330] Modules linked in: i915 fbcon font bitblit softcursor 
drm_kms_helper drm fb fbdev i2c_algo_bit cfbcopyarea i2c_core video backlight 
cfbimgblt cfbfillrect e100 ohci_hcd mii pcspkr evdev uhci_hcd ehci_hcd fan 
processor thermal_sys button usbcore [last unloaded: i915]
[  943.015825] Pid: 1678, comm: bash Not tainted 3.0.0-rc3 #2
[  943.015966] Call Trace:
[  943.016179]  [c1027315] warn_slowpath_common+0x65/0x7a
[  943.016342]  [f013e716] ? intel_init_clock_gating+0x30/0x4a [i915]
[  943.016487]  [c1027339] warn_slowpath_null+0xf/0x13
[  943.016644]  [f013e716] intel_init_clock_gating+0x30/0x4a [i915]
[  943.016801]  [f012f1f9] i915_restore_state+0xf4/0x1bf [i915]
[  943.016989]  [f0124100] i915_drm_thaw+0x41/0xc1 [i915]
[  943.017141]  [f01242c0] i915_resume+0x38/0x4b [i915]
[  943.017301]  [f00b0472] drm_class_resume+0x39/0x3b [drm]
[  943.017447]  [c116b520] legacy_resume+0x1e/0x46
[  943.017599]  [f00b0439] ? drm_class_suspend+0x3d/0x3d [drm]
[  943.017742]  [c116b7de] device_resume+0x83/0xa0
[  943.017881]  [c116bdd8] dpm_resume+0xdc/0x156
[  943.018020]  [c116bf68] dpm_resume_end+0xb/0x15
[  943.018162]  [c1053994] suspend_devices_and_enter+0x165/0x192
[  943.018330]  [c1053a93] enter_state+0xd2/0x123
[  943.018471]  [c105329f] state_store+0x95/0xa1
[  943.018610]  [c105320a] ? pm_async_store+0x33/0x33
[  943.018752]  [c1103c15] kobj_attr_store+0x16/0x22
[  943.018894]  [c10c7e5b] sysfs_write_file+0xb3/0xec
[  943.019034]  [c10c7da8] ? sysfs_open_file+0x1c2/0x1c2
[  943.019176]  [c108d793] vfs_write+0x76/0xa2
[  943.019315]  [c108d8f6] sys_write+0x3b/0x5d
[  943.019456]  [c11fb610] sysenter_do_call+0x12/0x36
[  943.019625] ---[ end trace dc74bd86a8bff7da ]---


What's the right way to fix this?

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Oops in i915 intel_init_clock_gating

[Alan Stern]

On Wed, 15 Jun 2011, Jesse Barnes wrote:

 On Wed, 15 Jun 2011 16:08:51 -0400 (EDT)
 Alan Stern st...@rowland.harvard.edu wrote:
 
  The problem of dev_priv-display.init_clock_gating not getting set is 
  still present in 3.0-rc3.  On my system this happens because 
  intel_init_display() never gets called in the first place.
  
  AFAICT, the normal calling sequence during driver initialization is:
  
  i915_driver_load() - i915_load_modeset_init() -
  intel_modeset_init() - intel_init_display().
  
  But in my case the call to i915_load_modeset_init() doesn't occur 
  because drm_core_check_feature(dev, DRIVER_MODESET) is False.
 
 Ouch, a non-KMS config.  Any reason you can't use KMS?

Normally I do use it.  This was a special testing config I've been 
nursing along for years, since well before KMS existed.  Either I never 
enabled KMS in the config, or else at some point it caused trouble so I 
removed it and never added it back.  Can't remember which -- all the 
testing I do with this config is at a VT, never under X.

 This patch should help at any rate.

I confirm that the patch fixes the problem.  Thanks.

On a different but related note, rmmod i915 incites a lockdep 
notification:

[   54.316439] INFO: trying to register non-static key.
[   54.316589] the code is fine but needs lockdep annotation.
[   54.316729] turning off the locking correctness validator.
[   54.316871] Pid: 1683, comm: rmmod Not tainted 3.0.0-rc3 #2
[   54.317011] Call Trace:
[   54.317153]  [c11f582a] ? printk+0xf/0x11
[   54.317296]  [c1049b3f] register_lock_class+0x58/0x2d7
[   54.317438]  [c102176e] ? get_parent_ip+0xb/0x31
[   54.317579]  [c105295a] ? is_module_text_address+0x37/0x45
[   54.317722]  [c1038917] ? __kernel_text_address+0x1c/0x3e
[   54.317864]  [c1049e61] __lock_acquire+0xa3/0xc5a
[   54.318005]  [c1003833] ? dump_trace+0x7f/0xa5
[   54.318146]  [c104aa09] ? __lock_acquire+0xc4b/0xc5a
[   54.318287]  [c104adf7] lock_acquire+0x5e/0x75
[   54.318427]  [c10364f4] ? work_on_cpu+0x96/0x96
[   54.318567]  [c1036530] wait_on_work+0x3c/0x133
[   54.318707]  [c10364f4] ? work_on_cpu+0x96/0x96
[   54.318848]  [c102fe0d] ? lock_timer_base.clone.23+0x20/0x3e
[   54.318991]  [c11f7892] ? _raw_spin_unlock_irqrestore+0x36/0x5b
[   54.319134]  [c102176e] ? get_parent_ip+0xb/0x31
[   54.319275]  [c11f9f99] ? sub_preempt_count+0x7c/0x89
[   54.319417]  [c1036cb2] __cancel_work_timer+0xa0/0xde
[   54.319559]  [c1036d07] cancel_work_sync+0xa/0xc
[   54.319714]  [f0128105] i915_driver_unload+0x136/0x224 [i915]
[   54.319874]  [f00af39d] drm_put_dev+0xa9/0x170 [drm]
[   54.320029]  [f00b086d] drm_pci_exit+0x49/0x63 [drm]
[   54.320045]  [f01508d0] i915_exit+0x12/0x742 [i915]
[   54.320045]  [c1050da5] sys_delete_module+0x175/0x1c1
[   54.320045]  [c107efb2] ? remove_vma+0x52/0x58
[   54.320045]  [c11f7ce0] ? restore_all+0xf/0xf
[   54.320045]  [c11fb610] sysenter_do_call+0x12/0x36
[   54.336786] [drm] Module unloaded

Is this a known problem?

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

s2ram slow (radeon) / failing (usb)

[Alan Stern]

On Sun, 2 May 2010, Bruno [UTF-8] Pr??mont wrote:

> > There's no way to fix the USB problem without knowing what goes wrong.  
> > Let's see how far you get before the system freezes on a kernel with 
> > CONFIG_USB_DEBUG enabled.
> 
> Am I missing something?
> 
> I've enabled CONFIG_USB_DEBUG but don't see any additional module parameter
> nor anything extra to toggle and I don't get more output than without it.

Depends what you mean by "output".  The kernel generates more log 
messages, but they may not get sent to your console.  You need to make 
sure the console's log level is set high enough to see debugging 
messages.  For example:

echo 9 >/proc/sys/kernel/printk

or type Alt-SysRq-9.

Alan Stern

s2ram slow (radeon) / failing (usb)

[Alan Stern]

On Sun, 2 May 2010, Bruno [UTF-8] Pr??mont wrote:

> Hi,
> 
> On a IEI Kino 690S1 I'm having a hard time to get s2ram running.
> It freezes during device suspend (unless I rmmod everything USB
> related) - usb fails even in pm_test case 'devices'.
> 
> When the system is able to suspend it takes an eternity (more than 3
> minutes to wake-up, the radeon apparently being responsible for quite
> a big share of that slowness.
> 
> 
> During resume early it looks like every PCI access needs about a second,
> and there are a few cases where during lots of seconds nothing seems to
> happen and the first event following is related to radeon.
> 
> The kernel used is todays Linus's tree at commit 
> be1066bbcd443a65df312fdecea7e4959adedb45
> with Dave's drm-linus and drm-radeon-testing applied on top.
> 
> Note, I've not been able to suspend to RAM properly recently (last one
> that worked correctly but resumed without graphics was some-when during
> 2.6.2x, before KMS)
> Since then the system would either fail suspend or resume.
> 
> Manual changes I applied in order to find out some context information:
> - add a few debugging printk's to ata/ahci as that was the last entry
>   on serial console for freezing suspends (that one succeeded but
>   following step never completed, from suspend_prepare that would have
>   been USB => unload usb before suspend)
> - strip "if EMBEDED" from CONFIG_SERIAL_8250_PNP and disabled it so serial
>   console would continue working as long as possible and output suspend
>   progress (resume output happens only very late)
> 
> Is there some additional information I could gather in order do help
> improving s2ram on this system?
> - get it to suspend with usb loaded (ohci + ehci)
> - get it to resume a reasonable speed

There's no way to fix the USB problem without knowing what goes wrong.  
Let's see how far you get before the system freezes on a kernel with 
CONFIG_USB_DEBUG enabled.

Alan Stern

Re: s2ram slow (radeon) / failing (usb)

[Alan Stern]

On Sun, 2 May 2010, Bruno [UTF-8] Prémont wrote:

  There's no way to fix the USB problem without knowing what goes wrong.  
  Let's see how far you get before the system freezes on a kernel with 
  CONFIG_USB_DEBUG enabled.
 
 Am I missing something?
 
 I've enabled CONFIG_USB_DEBUG but don't see any additional module parameter
 nor anything extra to toggle and I don't get more output than without it.

Depends what you mean by output.  The kernel generates more log 
messages, but they may not get sent to your console.  You need to make 
sure the console's log level is set high enough to see debugging 
messages.  For example:

echo 9 /proc/sys/kernel/printk

or type Alt-SysRq-9.

Alan Stern

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel