Re: “Mouse battery low” notification appears even when all notifications disabled
On Tue, Oct 16, 2018 at 04:15:34PM -0300, Cristian wrote: > Hello, > > Open bug in launchpad.net > https://bugs.launchpad.net/bugs/1798166 > > "I have a wireless mouse powered by non-rechargeable battery. The > mouse works absolutely fine even with low battery level. Problem is, > Ubuntu gives notifications about the battery level all the time. > > I have had this issue for almost two months now, and mouse is working > perfectly. I had to click away the notification almost every time I > moved the mouse. I was then able to make the notifications appear less > frequently, but the notification still always appear after some > interval, and always after I log in. > > Here are some specific things I have tried, they have not helped. > > https://askubuntu.com/questions/1071406/how-to-disable-mouse-battery-low-notification-in-ubuntu-18-04"; This is not a kernel issue, please work with your distro to resolve this. Best of luck! greg k-h
RE: [PATCH] xhci: remove the unused sw_lpm_support
Hi Mathias: >-Original Message- >From: Mathias Nyman [mailto:mathias.ny...@linux.intel.com] >Sent: Tuesday, October 16, 2018 8:34 PM >To: Zengtao (B) ; mathias.ny...@intel.com; >gre...@linuxfoundation.org >Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org >Subject: Re: [PATCH] xhci: remove the unused sw_lpm_support > >On 16.10.2018 14:37, Zeng Tao wrote: >> It is introduced for the pre-0.96 xHC controllers, and the driver only >> support HW LPM for 1.0 and later controllers.It's not actually used >> now and is thought not to be used in the future any more, so just >remove it. >> >> Acked-by: Mathias Nyman > >Please don't add my Acked-by to patches I haven't seen. > > From Documentation/process/submitting-patches.rst: > Thanks for pointing out the wrong tag, I meant to add suggested-by, ^_^ >"Acked-by: is not as formal as Signed-off-by:. It is a record that the acker >has at least reviewed the patch and has indicated acceptance." > >> Signed-off-by: Zeng Tao > >Otherwise the patch looks good, I'll queue it, and remove the extra >Acked-by > It's ok for me, thank you. Regards Zengtao
RE: [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
> On Tue, Oct 16, 2018 at 2:02 AM Peter Chen wrote: > > > > For USB HSIC, the data and strobe pin needs to be pulled down at > > default, we consider it as "idle" state. When the USB host is ready to > > be used, the strobe pin needs to be pulled up, we consider it as > > "active" state. > > > > Signed-off-by: Peter Chen > > --- > > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > index 529e51879fb2..10c8d793ea49 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > @@ -81,6 +81,7 @@ Optional properties: > >mux state of 1 indicates host mode. > > - mux-control-names: Shall be "usb_switch" if mux-controls is specified. > > - pinctrl-names: Names for optional pin modes in "default", "host", > > "device" > > + Or names for HSIC "idle" and "active" pin modes. > > I don't think this description is clear enough. > > Could you please add a real dts snippet for the HSIC case instead? Ok, I will add example like below at next version. usb@02184000 { /* USB OTG */ { ... pinctrl-names = "idle", "active"; pinctrl-0 = <&pinctrl_usbh2_1>; pinctrl-1 = <&pinctrl_usbh2_2>; ... }; Peter
Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
> I don't think SMBus is an option in this case since the intended client > (Cypress > something in patch 2/2) does xfers that would need 16-bit commands and I think > they are always 8-bit in SMBus, no? Yes. Command is 8 bit, data can be 16. Thanks for the heads up! signature.asc Description: PGP signature
“Mouse battery low” notification appears even when all notifications disabled
Hello, Open bug in launchpad.net https://bugs.launchpad.net/bugs/1798166 "I have a wireless mouse powered by non-rechargeable battery. The mouse works absolutely fine even with low battery level. Problem is, Ubuntu gives notifications about the battery level all the time. I have had this issue for almost two months now, and mouse is working perfectly. I had to click away the notification almost every time I moved the mouse. I was then able to make the notifications appear less frequently, but the notification still always appear after some interval, and always after I log in. Here are some specific things I have tried, they have not helped. https://askubuntu.com/questions/1071406/how-to-disable-mouse-battery-low-notification-in-ubuntu-18-04"; Best regards, -- Cristian Aravena Romero (caravena)
[PATCH] usbip: tools: fix atoi() on non-null terminated string
From: Colin Ian King Currently the call to atoi is being passed a single char string that is not null terminated, so there is a potential read overrun along the stack when parsing for an integer value. Fix this by instead using a 2 char string that is initialized to all zeros to ensure that a 1 char read into the string is always terminated with a \0. Detected by cppcheck: "Invalid atoi() argument nr 1. A nul-terminated string is required." Fixes: 3391ba0e2792 ("usbip: tools: Extract generic code to be shared with vudc backend") Signed-off-by: Colin Ian King --- tools/usb/usbip/libsrc/usbip_host_common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c b/tools/usb/usbip/libsrc/usbip_host_common.c index dc93fadbee96..d79c7581b175 100644 --- a/tools/usb/usbip/libsrc/usbip_host_common.c +++ b/tools/usb/usbip/libsrc/usbip_host_common.c @@ -43,7 +43,7 @@ static int32_t read_attr_usbip_status(struct usbip_usb_device *udev) int size; int fd; int length; - char status; + char status[2] = { 0 }; int value = 0; size = snprintf(status_attr_path, sizeof(status_attr_path), @@ -61,14 +61,14 @@ static int32_t read_attr_usbip_status(struct usbip_usb_device *udev) return -1; } - length = read(fd, &status, 1); + length = read(fd, status, 1); if (length < 0) { err("error reading attribute %s", status_attr_path); close(fd); return -1; } - value = atoi(&status); + value = atoi(status); return value; } -- 2.19.1
Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
On 2018-10-02 09:27, Wolfram Sang wrote: > Hi, > We got confirmation from HW team about 4 byte read limitation. There has to be a STOP after every single read cycle. One read cycle supports maximum of 4 byte burst. I will update the patches with a comment on this. >>> >>> Could it be that this is more an SMBus controller than an I2C controller? >>> I haven't looked at the specs but maybe populating smbus_xfer instead of >>> master_xfer is an option here? >> I think its more of i2c controller and we do mention " .max_read_len = 4" in >> " struct i2c_adapter_quirks". Do you still see something missing here? > > Well, if there has to be STOP after a read, then you can't do a transfer > containing "write-read-write", or? So, I wondered if this controller is > of the type which can mainly do "write-then-read" transfers only (check > I2C_AQ_COMB* quirk flags). And for some of those controller types, it > might be even easier to drop generic I2C transfers and only handle the > SMBUS calls. > > I didn't check this driver closely yet, so I can't tell if/what it needs > from the above. I wanted to give this input already, though. I don't think SMBus is an option in this case since the intended client (Cypress something in patch 2/2) does xfers that would need 16-bit commands and I think they are always 8-bit in SMBus, no? Cheers, Peter
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
Hi Oliver, to my surprise I was starting to attempt the patch of the code and the rebuilt, 30 mins ago, when I noticed a new version was released in Arch ( 4.18.14 ) so I decided to update and reboot, to see if actually it changed something and...to my surprise it did! $ uname -a Linux anarchy 4.18.14-arch1-1-ARCH #1 SMP PREEMPT Sat Oct 13 13:42:37 UTC 2018 x86_64 GNU/Linux $ lsusb -t /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 5000M |__ Port 1: Dev 2, If 0, Class=Mass Storage, Driver=uas, 5000M $ lsusb Bus 002 Device 002: ID 152d:1561 JMicron Technology Corp. / JMicron USA Technology Corp. JMS561U two ports SATA 6Gb/s bridge I am honestly not sure what is changed between 4.18.14 and .12 ( I will investigate ) but to my surprise I got UAS enabled which is what I was requiring to test. Thank you very much for the support anyway! Best regards, Julian Xhokaxhiu Full Stack Developer, IT Practised (ISCED 4) https://julianxhokaxhiu.com/ On Tue, Oct 16, 2018 at 2:40 PM Oliver Neukum wrote: > > On Di, 2018-10-16 at 10:41 +0200, Julian Xhokaxhiu wrote: > > Good morning Oliver, > > > > thank you very much for the reply. I will try to build a test kernel > > hopefully later this evening and I'll get back to you. > > > > Meanwhile you can see in the attachment in my first email the output > > of `$ lsusb -v -d 152d:1561` > > That is the problem. I just cannot find a quirk for that device. > And the generic ASMedia code should not match. > > > Is there a way to test UAS without recompiling the Kernel? Like a > > Kernel argument or modprobe option? > > Unfortunately the existing option allow setting US_FL_IGNORE_UAS > but not clearing it. You need to recompile. > > Regards > Oliver >
Re: [PATCH 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
Hi Peter, On Tue, Oct 16, 2018 at 2:02 AM Peter Chen wrote: > > For USB HSIC, the data and strobe pin needs to be pulled down > at default, we consider it as "idle" state. When the USB host > is ready to be used, the strobe pin needs to be pulled up, > we consider it as "active" state. > > Signed-off-by: Peter Chen > --- > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 529e51879fb2..10c8d793ea49 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -81,6 +81,7 @@ Optional properties: >mux state of 1 indicates host mode. > - mux-control-names: Shall be "usb_switch" if mux-controls is specified. > - pinctrl-names: Names for optional pin modes in "default", "host", "device" > + Or names for HSIC "idle" and "active" pin modes. I don't think this description is clear enough. Could you please add a real dts snippet for the HSIC case instead?
Re: [PATCH v13 0/2] Add support for USB Type-C interface on latest NVIDIA GPU
+Andy These have changed a bit since Andy gave his review. On Fri, Oct 12, 2018 at 06:00:50PM +, Ajay Gupta wrote: > Hi Heikki and Wolfram > Do you have any comments on these changes? Let me take one more look at the UCSI driver, just in case. Nothing's probable going to happen for a while in any case as the merge window is about to open. thanks, -- heikki
Re: Query on usb/core/devio.c
On Tue, 16 Oct 2018, Oliver Neukum wrote: > On Mo, 2018-10-15 at 09:50 -0400, Alan Stern wrote: > > > > It seems that a better approach would be to have an ioctl which would: > > > > fail if there are any active user URBs; > > > > drop the usbfs PM reference so the device can suspend; > > > > block interruptibly until the device resumes; > > Thus we would require user space to have a thread for each device > it wants to allow suspend for. True. I suppose instead of waiting on the usbfs device file, we could use select() on some sysfs file (something like power/runtime_status). Unlike the usbfs device file, keeping a sysfs file open won't interfere with runtime PM. And it might be useful for other user programs to have a way of being notified when a device (not necessarily USB!) goes into or out of suspend. > [..] > > What difference does it make if the URBs are killed by the user instead > > of the kernel? > > We stay within the limits of the timing (as well as we can) and the > case of a failed suspend is much easier to handle. > Else we have an unlimited time between cessation of IO and going > into suspend. The time could be limited be a timeout. In any case, we must recognize that userspace device drivers will never be as powerful as kernel drivers. The difficulty of handling suspend and resume is one example of this. Alan Stern
Re: Query on usb/core/devio.c
On Tue, 16 Oct 2018, Mayuresh Kulkarni wrote: > > It seems that a better approach would be to have an ioctl which would: > > > > fail if there are any active user URBs; > > > > drop the usbfs PM reference so the device can suspend; > > > > block interruptibly until the device resumes; > > > > reacquire the PM reference (forcing the device to resume) > > if the ioctl call is interrupted. > > > > Please correct me if wrong, but as I understand, with this new ioctl > proposal, below should be possible - > 1. User space decides when it is time to suspend the device and calls > this ioctl. The implmentation takes care to ensure, no URB is in > flight from this caller to this device. Then proceed with suspend. Well, I did not imagine the ioctl would try to ensure that no URBs are in flight. The user would be responsible for that. Also, proceeding with the suspend is difficult, as Oliver has pointed out. There is no guarantee that the device will go into suspend, merely because the userspace driver isn't accessing it. (In fact, there's no guarantee that the device will _ever_ go into suspend.) The ioctl would probably have to include some sort of timeout; it would return early if the timeout expired before the device was suspended. > 2. In an another thread, the user-space calls poll(USB-device-fd). > When poll() returns, that means the device is active now. User space > can then request an URB to read out the async notification, if any. No; no other thread or polling is needed. The ioctl call returns when the device resumes. At that point the user program can check whether there is an async notification pending. > 3. Compatibility is maintained with current user-space implementation as only > "newer" user-space will call this new ioctl. Correct. > The USB device can suspend between (1) and (2). > > If this is correct interpretation, what will happen when USB device > sends remote-wake to host, but the reason is not async notification > but something other than that (e.g.: something standard UAC or HID)? The reason for the wakeup won't make any difference; the behavior would be the same regardless. > Here, the URB will be queued forcing the link to be active (and we > probably land into same issue). > In such a case, is it expected from user-space to dequeue the URB > after a while and follow (1) and (2) above? Of course. Isn't that what you would do if the reason for the wakeup _was_ an async notification? So you should do the same thing even if the reason is something else. > Apologies if this is implied but I am just trying to get my head > around with the proposal, hence being verbose. Perfectly okay. As Oliver mentioned, it's foolish to design an API that doesn't do what the user wants. Alan Stern
Re: USB hotplug on HP 255 G6 laptop
On 16.10.2018 14:38, Jan Kara wrote: On Wed 03-10-18 16:46:05, Jan Kara wrote: On Wed 03-10-18 17:19:33, Mathias Nyman wrote: On 02.10.2018 17:53, Jan Kara wrote: On Tue 02-10-18 17:01:54, Mathias Nyman wrote: On 02.10.2018 16:06, Jan Kara wrote: Hello, my wife has HP 255 G6 laptop. When it is attached to AC, everything works as expected however when it is running on battery, USB hotplug stops working - newly plugged devices do not appear to be visible to the kernel. Only when the AC is plugged back in, the kernel suddently wakes up and detects all newly attached devices. Maybe it is related to some power management? Any idea how to debug this? Dmesg from the system is attached. The kernels I've tested with (both behave the same way) are 4.18.11 kernel and also openSUSE Leap 15 kernel which is 4.12-based. Thanks in advance for any help. Are you running laptop mode tools or similar that would enable runtime suspend D3 state for xhci controller? It is openSUSE Leap 15 installation with xfce4 desktop so I assume there is some power-management going on. Not sure what I should look for but there's xfce4-power-manager running, also upowerd is running. what does lspci -vv say? Attached. check the content of the following files while running on battery: cat /sys/bus/pci/devices/:00:10.0/firmware_node/power_state D0 cat /sys/bus/pci/devices/:00:10.0/power/control auto - when on battery, on - when on AC. try: echo on > /sys/bus/pci/devices/:00:10.0/power/control before pluggin in a usb device, does it help? Yes, USB device gets recognized after this. To me this looks like xhci is runtime suspended, but controller is still in a PCI D0 state. Normally the xHC should be put to PCI D3 in runtime suspend, and woken up by a PCI PME# when a device is plugged in, but PME# is not enabled in D0 so in this case nothing wakes up the xHC. The xhci runtime suspend code already stopped the controller, so probably you're not getting an interrupt either. PCI code looks at firmware ACPI tables to choose the suspend D state, if something is off in the tables it's possible the wrong state is chosen. worth checking. Could you dump the DSDT ACPI table form /sys/firmware/acpi/tables/DSDT Sure. Attached. As a quick temporary workaround you could find and prevent the powersaving program from enabling runtime suspend. (make sure it won't write "auto" to /sys/bus/pci/devices/:00:10.0/power/control) OK, thanks for the hint. Any news here? Are really ACPI tables wrong on this laptop? Can't say for sure There might be differences how Windows and Linux parse ACPI tables. If _PR0 or _PS0 is present for a device in ACPI tables then Linux will assume the device is ACPI power manageable, and select PCI D states from ACPI tables. _S3W should return the highest D-value (deepest sleep state) device can wake up from S3 (system suspend) _S0W should return the highest D-value (deepest sleep state) device can wake up from S0 (Runtime suspend) Your ACPI DSDT table shows: Scope (_SB.PCI0.XHC0) { Name (_PR0, Package (0x01) // _PR0: Power Resources for D0 { P0U3 }) Name (_PR3, Package (0x01) // _PR3: Power Resources for D3hot { P3U3 }) Method (_S0W, 0, NotSerialized) // _S0W: S0 Device Wake State { If ((XHCD == One)) { Return (0x04) } Else { Return (Zero) } } Method (_S3W, 0, NotSerialized) // _S3W: S3 Device Wake State { Return (0x04) } Those with better APCI and PCI knowledge might know more details, or better debugging for this, but it at least would be possible to check which D state is chosen by adding the below code, and recompiling the kernel: diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index c2ab577..c9cb93a 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -501,6 +501,12 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) else d_max = ACPI_STATE_D3_COLD; acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL, d_max); + + if (pdev->device == 0x31a8) { + dev_err(&pdev->dev, "HACK: device 0x%x: ACPI selected D%d state, Force D3\n", + pdev->device, acpi_state); + acpi_state = ACPI_STATE_D3_HOT; + } if (acpi_state < 0) return PCI_POWER_ERROR; You need to change the "0x31a8" to whatever PCI ID your xhci controller has. lspci -nn is your friend. -Mathias
Re: Query on usb/core/devio.c
On Tue, 16 Oct 2018, Mayuresh Kulkarni wrote: > > How about instead having a mechanism whereby your usrspace driver can > > tell when the device does a remote wakeup? At that point it could > > submit an URB via usbfs and receive the async notification. > > > > Alan Stern > > Unfortunately, that is not possible in our use-case. It is the composite USB > device that needs to detect an activity and convey it to host for taking > appropriate action. Sorry, I don't understand. Isn't that exactly the same as what I wrote above? Alan Stern > But since the activity depends on end-user, it can happen at anytime. We do > not want to keep the link active, just to detect this, which is not good from > battery life point of view.
Re: Query on usb/core/devio.c
On Di, 2018-10-16 at 12:10 +0100, Mayuresh Kulkarni wrote: > On Mon, 15 Oct 2018 09:50:46 -0400 > Alan Stern wrote: > > > It seems that a better approach would be to have an ioctl which would: > > > > fail if there are any active user URBs; > > > > drop the usbfs PM reference so the device can suspend; > > > > block interruptibly until the device resumes; > > > > reacquire the PM reference (forcing the device to resume) > > if the ioctl call is interrupted. > > > > Please correct me if wrong, but as I understand, with this new ioctl > proposal, below should be possible - > 1. User space decides when it is time to suspend the device and calls this > ioctl. The implmentation takes care to ensure, no URB is in flight from this > caller to this device. Then proceed with suspend. > The exact semantics this ioctl() should have is what we are currently discussing. > 2. In an another thread, the user-space calls poll(USB-device-fd). When > poll() returns, that means the device is active now. User space can then > request an URB to read out the async notification, if any. That is one of the designs we are looking at. > 3. Compatibility is maintained with current user-space implementation as only > "newer" user-space will call this new ioctl. Yes. > The USB device can suspend between (1) and (2). Yes. > If this is correct interpretation, what will happen when USB device sends > remote-wake to host, but the reason is not async notification but something > other than that (e.g.: something standard UAC or HID)? Here, the URB will be > queued forcing the link to be active (and we probably land into same issue). Remote wakeup does not carry information with it. In fact a wake up from the host can race with remote wake up. Any wake up would require a user space driver to do IO with the device and query it for a reason to wake up. > In such a case, is it expected from user-space to dequeue the URB after a > while and follow (1) and (2) above? Yes. > Apologies if this is implied but I am just trying to get my head around with > the proposal, hence being verbose. Please feel free and encouraged to state your requirements as clearly and verbosely as you like. It would suck to introduce an API only to find that it cannot do the job. Regards Oliver
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
On Di, 2018-10-16 at 10:41 +0200, Julian Xhokaxhiu wrote: > Good morning Oliver, > > thank you very much for the reply. I will try to build a test kernel > hopefully later this evening and I'll get back to you. > > Meanwhile you can see in the attachment in my first email the output > of `$ lsusb -v -d 152d:1561` That is the problem. I just cannot find a quirk for that device. And the generic ASMedia code should not match. > Is there a way to test UAS without recompiling the Kernel? Like a > Kernel argument or modprobe option? Unfortunately the existing option allow setting US_FL_IGNORE_UAS but not clearing it. You need to recompile. Regards Oliver
[PATCH] USB: misc: appledisplay: fix backlight update_status return code
Upon success the update_status handler returns a positive number corresponding to the number of bytes transferred by usb_control_msg. However the return code of the update_status handler should indicate if an error occurred(negative) or how many bytes of the user's input to sysfs that was consumed. Return code zero indicates all bytes were consumed. The bug can for example result in the update_status handler being called twice, the second time with only the "unconsumed" part of the user's input to sysfs. Effectively setting an incorrect brightness. Change the update_status handler to return zero for all successful transactions and forward usb_control_msg's error code upon failure. Signed-off-by: Mattias Jacobsson <2...@mok.nu> --- I've not found any documentation regarding the return code from the update_status handler. The information above is based on looking at other driver and how the return code is used for by the caller of the update_status handler. Please let me know if it is incorrect. --- drivers/usb/misc/appledisplay.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c index d746c26a8055..bd539f3058bc 100644 --- a/drivers/usb/misc/appledisplay.c +++ b/drivers/usb/misc/appledisplay.c @@ -146,8 +146,11 @@ static int appledisplay_bl_update_status(struct backlight_device *bd) pdata->msgdata, 2, ACD_USB_TIMEOUT); mutex_unlock(&pdata->sysfslock); - - return retval; + + if (retval < 0) + return retval; + else + return 0; } static int appledisplay_bl_get_brightness(struct backlight_device *bd) -- 2.19.1
Re: [PATCH] xhci: remove the unused sw_lpm_support
On 16.10.2018 14:37, Zeng Tao wrote: It is introduced for the pre-0.96 xHC controllers, and the driver only support HW LPM for 1.0 and later controllers.It's not actually used now and is thought not to be used in the future any more, so just remove it. Acked-by: Mathias Nyman Please don't add my Acked-by to patches I haven't seen. From Documentation/process/submitting-patches.rst: "Acked-by: is not as formal as Signed-off-by:. It is a record that the acker has at least reviewed the patch and has indicated acceptance." Signed-off-by: Zeng Tao Otherwise the patch looks good, I'll queue it, and remove the extra Acked-by Thanks -Mathias
Re: USB hotplug on HP 255 G6 laptop
On Wed 03-10-18 16:46:05, Jan Kara wrote: > On Wed 03-10-18 17:19:33, Mathias Nyman wrote: > > On 02.10.2018 17:53, Jan Kara wrote: > > > On Tue 02-10-18 17:01:54, Mathias Nyman wrote: > > > > On 02.10.2018 16:06, Jan Kara wrote: > > > > > Hello, > > > > > > > > > > my wife has HP 255 G6 laptop. When it is attached to AC, everything > > > > > works > > > > > as expected however when it is running on battery, USB hotplug stops > > > > > working - newly plugged devices do not appear to be visible to the > > > > > kernel. > > > > > Only when the AC is plugged back in, the kernel suddently wakes up and > > > > > detects all newly attached devices. Maybe it is related to some power > > > > > management? Any idea how to debug this? Dmesg from the system is > > > > > attached. > > > > > The kernels I've tested with (both behave the same way) are 4.18.11 > > > > > kernel > > > > > and also openSUSE Leap 15 kernel which is 4.12-based. Thanks in > > > > > advance for > > > > > any help. > > > > > > > > > > > > > Are you running laptop mode tools or similar that would enable runtime > > > > suspend > > > > D3 state for xhci controller? > > > > > > It is openSUSE Leap 15 installation with xfce4 desktop so I assume there > > > is > > > some power-management going on. Not sure what I should look for but > > > there's > > > xfce4-power-manager running, also upowerd is running. > > > > > > > what does lspci -vv say? > > > > > > Attached. > > > > > > > check the content of the following files while running on battery: > > > > > > > > cat /sys/bus/pci/devices/:00:10.0/firmware_node/power_state > > > > > > D0 > > > > > > > cat /sys/bus/pci/devices/:00:10.0/power/control > > > > > > auto - when on battery, on - when on AC. > > > > > > > try: echo on > /sys/bus/pci/devices/:00:10.0/power/control > > > > before pluggin in a usb device, does it help? > > > > > > Yes, USB device gets recognized after this. > > > > > > > To me this looks like xhci is runtime suspended, but controller is still > > in a PCI D0 state. > > > > Normally the xHC should be put to PCI D3 in runtime suspend, and woken > > up by a PCI PME# when a device is plugged in, but PME# is not enabled in > > D0 so in this case nothing wakes up the xHC. > > > > The xhci runtime suspend code already stopped the controller, so probably > > you're not getting an interrupt either. > > > > PCI code looks at firmware ACPI tables to choose the suspend D state, if > > something is off in the tables it's possible the wrong state is chosen. > > worth checking. > > > > Could you dump the DSDT ACPI table form /sys/firmware/acpi/tables/DSDT > > Sure. Attached. > > > As a quick temporary workaround you could find and prevent the > > powersaving program from enabling runtime suspend. (make sure it won't > > write "auto" to /sys/bus/pci/devices/:00:10.0/power/control) > > OK, thanks for the hint. Any news here? Are really ACPI tables wrong on this laptop? Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH] usb: gadget: storage: Fix Spectre v1 vulnerability
On Tue, Oct 16, 2018 at 02:28:19PM +0300, Felipe Balbi wrote: > > Hi Greg, > > "Gustavo A. R. Silva" writes: > > > num can be indirectly controlled by user-space, hence leading to > > a potential exploitation of the Spectre variant 1 vulnerability. > > > > This issue was detected with the help of Smatch: > > > > drivers/usb/gadget/function/f_mass_storage.c:3177 fsg_lun_make() warn: > > potential spectre issue 'fsg_opts->common->luns' [r] (local cap) > > > > Fix this by sanitizing num before using it to index > > fsg_opts->common->luns > > > > Notice that given that speculation windows are large, the policy is > > to kill the speculation on the first load and not worry if it can be > > completed with a dependent load/store [1]. > > > > [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2 > > > > Cc: sta...@vger.kernel.org > > Signed-off-by: Gustavo A. R. Silva > > Can you still take this as an urgent fix? > > Acked-by: Felipe Balbi Yes, will do so, thanks. greg k-h > > -- > balbi
Re: [PATCH] usb: gadget: storage: Fix Spectre v1 vulnerability
Hi Greg, "Gustavo A. R. Silva" writes: > num can be indirectly controlled by user-space, hence leading to > a potential exploitation of the Spectre variant 1 vulnerability. > > This issue was detected with the help of Smatch: > > drivers/usb/gadget/function/f_mass_storage.c:3177 fsg_lun_make() warn: > potential spectre issue 'fsg_opts->common->luns' [r] (local cap) > > Fix this by sanitizing num before using it to index > fsg_opts->common->luns > > Notice that given that speculation windows are large, the policy is > to kill the speculation on the first load and not worry if it can be > completed with a dependent load/store [1]. > > [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2 > > Cc: sta...@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva Can you still take this as an urgent fix? Acked-by: Felipe Balbi -- balbi
[PATCH 1/2] usb: xhci: tegra: Power-off power-domains on removal
From: Jon Hunter Currently the XUSB power domains used by the Tegra xHCI controller are never powered off on the removal of the driver, however, they will be powered off on probe failure. Update the removal code to be consistent with the probe failure path to power off the XUSB power domains. Signed-off-by: Jon Hunter Acked-by: Thierry Reding Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci-tegra.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index 4ee510a..920a50a 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -1249,6 +1249,11 @@ static int tegra_xusb_remove(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); + if (!pdev->dev.pm_domain) { + tegra_powergate_power_off(TEGRA_POWERGATE_XUSBC); + tegra_powergate_power_off(TEGRA_POWERGATE_XUSBA); + } + tegra_xusb_padctl_put(tegra->padctl); return 0; -- 2.7.4
[PATCH 0/2] xhci features for usb-next
Hi Greg These two patches are part 2/5 and 3/5 of the Tegra xhci genpd support series. It's how Jon Hunter proposed them to be merged: " This series is making changes to more than one subsystem and at first glance may look like a maintainers nightmare. However, my proposal is this, once reviewed and people are happy ... 1. Patches 1-3 should be merged first. Patches 1 can go via the Tegra tree and 2-3 via the USB tree. 2. Once patches 1-3 are accepted, then 4 and 5 can be merged via the Tegra tree." -Mathias Jon Hunter (2): usb: xhci: tegra: Power-off power-domains on removal usb: xhci: tegra: Add genpd support drivers/usb/host/xhci-tegra.c | 92 +-- 1 file changed, 81 insertions(+), 11 deletions(-) -- 2.7.4
[PATCH 2/2] usb: xhci: tegra: Add genpd support
From: Jon Hunter The generic power-domain framework has been updated to allow devices that require more than one power-domain to create a new device for each power-domain required and then link these new power-domain devices to the consumer device. Update the Tegra xHCI driver to use the new APIs provided by the generic power-domain framework so we can use the generic power-domain framework for managing the xHCI controllers power-domains. Please note that to maintain backward compatibility with older device-tree blobs these new generic power-domain APIs are only used if the 'power-domains' property is present and otherwise we fall back to using the legacy Tegra APIs for managing the power-domains. Signed-off-by: Jon Hunter Acked-by: Thierry Reding Reviewed-by: Ulf Hansson Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci-tegra.c | 89 +-- 1 file changed, 77 insertions(+), 12 deletions(-) diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index 920a50a..6b5db34 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -194,6 +195,11 @@ struct tegra_xusb { struct reset_control *host_rst; struct reset_control *ss_rst; + struct device *genpd_dev_host; + struct device *genpd_dev_ss; + struct device_link *genpd_dl_host; + struct device_link *genpd_dl_ss; + struct phy **phys; unsigned int num_phys; @@ -928,6 +934,57 @@ static int tegra_xusb_load_firmware(struct tegra_xusb *tegra) return 0; } +static void tegra_xusb_powerdomain_remove(struct device *dev, + struct tegra_xusb *tegra) +{ + if (tegra->genpd_dl_ss) + device_link_del(tegra->genpd_dl_ss); + if (tegra->genpd_dl_host) + device_link_del(tegra->genpd_dl_host); + if (tegra->genpd_dev_ss) + dev_pm_domain_detach(tegra->genpd_dev_ss, true); + if (tegra->genpd_dev_host) + dev_pm_domain_detach(tegra->genpd_dev_host, true); +} + +static int tegra_xusb_powerdomain_init(struct device *dev, + struct tegra_xusb *tegra) +{ + int err; + + tegra->genpd_dev_host = dev_pm_domain_attach_by_name(dev, "xusb_host"); + if (IS_ERR(tegra->genpd_dev_host)) { + err = PTR_ERR(tegra->genpd_dev_host); + dev_err(dev, "failed to get host pm-domain: %d\n", err); + return err; + } + + tegra->genpd_dev_ss = dev_pm_domain_attach_by_name(dev, "xusb_ss"); + if (IS_ERR(tegra->genpd_dev_ss)) { + err = PTR_ERR(tegra->genpd_dev_ss); + dev_err(dev, "failed to get superspeed pm-domain: %d\n", err); + return err; + } + + tegra->genpd_dl_host = device_link_add(dev, tegra->genpd_dev_host, + DL_FLAG_PM_RUNTIME | + DL_FLAG_STATELESS); + if (!tegra->genpd_dl_host) { + dev_err(dev, "adding host device link failed!\n"); + return -ENODEV; + } + + tegra->genpd_dl_ss = device_link_add(dev, tegra->genpd_dev_ss, +DL_FLAG_PM_RUNTIME | +DL_FLAG_STATELESS); + if (!tegra->genpd_dl_ss) { + dev_err(dev, "adding superspeed device link failed!\n"); + return -ENODEV; + } + + return 0; +} + static int tegra_xusb_probe(struct platform_device *pdev) { struct tegra_xusb_mbox_msg msg; @@ -1038,7 +1095,7 @@ static int tegra_xusb_probe(struct platform_device *pdev) goto put_padctl; } - if (!pdev->dev.pm_domain) { + if (!of_property_read_bool(pdev->dev.of_node, "power-domains")) { tegra->host_rst = devm_reset_control_get(&pdev->dev, "xusb_host"); if (IS_ERR(tegra->host_rst)) { @@ -1069,17 +1126,22 @@ static int tegra_xusb_probe(struct platform_device *pdev) tegra->host_clk, tegra->host_rst); if (err) { + tegra_powergate_power_off(TEGRA_POWERGATE_XUSBA); dev_err(&pdev->dev, "failed to enable XUSBC domain: %d\n", err); - goto disable_xusba; + goto put_padctl; } + } else { + err = tegra_xusb_powerdomain_init(&pdev->dev, tegra); + if (err) + goto put_powerdomains; } tegra->supplies = devm_kcalloc(&pdev->dev, tegra->soc->num_supplies,
Re: Query on usb/core/devio.c
On Mon, 15 Oct 2018 09:50:46 -0400 Alan Stern wrote: > On Mon, 15 Oct 2018, Oliver Neukum wrote: > > > On Do, 2018-10-11 at 14:29 -0400, Alan Stern wrote: > > > On Thu, 11 Oct 2018, Mayuresh Kulkarni wrote: > > [..] > > > > We are looking into closing the device instance during normal operation > > > > i.e.: only open/close device when needed. > > > > > > > > However, we still have one particular use-case where our USB device > > > > sends async notification to USB-host via the above interface. For that, > > > > as of now, we queue a URB from user-space. But because, the link never > > > > enters suspend (L2), we receive this async notification. > > > > > > > > I am not sure, how this use-case will work, if we open-close device > > > > only when needed. > > > > > > > > Assuming we have PM calls moved from open/close to ioctl or some other > > > > appropriate method, is the following sequence possible - > > > > * Queue an URB -> suspend (L2) > > > > * Device does remote wake & sends async notification > > > > * URB above returns with that notification > > > > > > It won't work like that. When a device goes into suspend there can't > > > be any outstanding URBs. > > > > Exactly. Yet in case the device is active the URB must be kept running. > > > > > How about instead having a mechanism whereby your usrspace driver can > > > tell when the device does a remote wakeup? At that point it could > > > > select() and trigger a wake up from resume()? > > Something like that, although usbfs already has a usbdev_poll() routine > with fixed semantics. I'm not sure we can change it. > > Another difficulty is that select() applies to open files, and usbfs > currently doesn't allow the device to suspend while the device file is > open. > > It seems that a better approach would be to have an ioctl which would: > > fail if there are any active user URBs; > > drop the usbfs PM reference so the device can suspend; > > block interruptibly until the device resumes; > > reacquire the PM reference (forcing the device to resume) > if the ioctl call is interrupted. > Please correct me if wrong, but as I understand, with this new ioctl proposal, below should be possible - 1. User space decides when it is time to suspend the device and calls this ioctl. The implmentation takes care to ensure, no URB is in flight from this caller to this device. Then proceed with suspend. 2. In an another thread, the user-space calls poll(USB-device-fd). When poll() returns, that means the device is active now. User space can then request an URB to read out the async notification, if any. 3. Compatibility is maintained with current user-space implementation as only "newer" user-space will call this new ioctl. The USB device can suspend between (1) and (2). If this is correct interpretation, what will happen when USB device sends remote-wake to host, but the reason is not async notification but something other than that (e.g.: something standard UAC or HID)? Here, the URB will be queued forcing the link to be active (and we probably land into same issue). In such a case, is it expected from user-space to dequeue the URB after a while and follow (1) and (2) above? Apologies if this is implied but I am just trying to get my head around with the proposal, hence being verbose. > > > submit an URB via usbfs and receive the async notification. > > > > I am afraid that design includes an inevitable race condition. > > It works fine for resume(). It fails for suspend(). If you > > require user space to cancel periodic transfers before the device > > can suspend, you will introduce a window of at least two seconds > > between the cancellation and the eventual suspend() during which > > the device will not work. In fact events may be outright lost. > > That "two seconds" value can be changed by the user, all the way down > to 0 if necessary. However, we cannot prevent other processes from > keeping the device out of suspend or waking it up. > > > If you want to suspend with user space drivers active, the kernel will > > have to kill all active URBs originating from user space drivers. > > What difference does it make if the URBs are killed by the user instead > of the kernel? > > > And notify user space when the device is resumed. IMHO select() > > is the syscall designed for that. > > Apart from the difficulties mentioned above. > > Alan Stern
[PATCH] usb: gadget: storage: Fix Spectre v1 vulnerability
num can be indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability. This issue was detected with the help of Smatch: drivers/usb/gadget/function/f_mass_storage.c:3177 fsg_lun_make() warn: potential spectre issue 'fsg_opts->common->luns' [r] (local cap) Fix this by sanitizing num before using it to index fsg_opts->common->luns Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1]. [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2 Cc: sta...@vger.kernel.org Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/function/f_mass_storage.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index cb402e7a..043f97a 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -221,6 +221,8 @@ #include #include +#include + #include "configfs.h" @@ -3152,6 +3154,7 @@ static struct config_group *fsg_lun_make(struct config_group *group, fsg_opts = to_fsg_opts(&group->cg_item); if (num >= FSG_MAX_LUNS) return ERR_PTR(-ERANGE); + num = array_index_nospec(num, FSG_MAX_LUNS); mutex_lock(&fsg_opts->lock); if (fsg_opts->refcnt || fsg_opts->common->luns[num]) { -- 2.7.4
Re: Query on usb/core/devio.c
On Thu, 11 Oct 2018 14:29:54 -0400 Alan Stern wrote: > On Thu, 11 Oct 2018, Mayuresh Kulkarni wrote: > > > On Tue, 9 Oct 2018 13:27:02 +0200 > > Greg KH wrote: > > > > > On Tue, Oct 09, 2018 at 10:51:53AM +0100, Mayuresh Kulkarni wrote: > > > > With all due respect, one of the possible reason for this could be, > > > > power saving on mobile/tablet platforms (running Android). These > > > > platforms usually have a single USB port. Specifically from our point > > > > of view, these platforms are removing 3.5 mm jack and hence the only > > > > interface available for headsets is USB. > > > > > > But the USB audio interface uses the in-kernel driver, which should > > > handle the power management issues automatically. There is no need to > > > use usbfs for hardware like this. > > > > > > So what real-world example are you having that requires this that does > > > not use a kernel driver? And why can you not just close the device? > > > > > > > (As you probably might have guessed), we have a composite USB device with > > USB audio and USB HID. However, to support our specific features (USP) that > > does not fit into either USB-audio or USB-HID spec, we have another > > interface. Some of the controls exposed by it indirectly affect the audio > > pipeline. > > It is this part of system which is in user-space and uses USB-FS driver to > > talk to interface. > > > > We are looking into closing the device instance during normal operation > > i.e.: only open/close device when needed. > > > > However, we still have one particular use-case where our USB device sends > > async notification to USB-host via the above interface. For that, as of > > now, we queue a URB from user-space. But because, the link never enters > > suspend (L2), we receive this async notification. > > > > I am not sure, how this use-case will work, if we open-close device only > > when needed. > > > > Assuming we have PM calls moved from open/close to ioctl or some other > > appropriate method, is the following sequence possible - > > * Queue an URB -> suspend (L2) > > * Device does remote wake & sends async notification > > * URB above returns with that notification > > It won't work like that. When a device goes into suspend there can't > be any outstanding URBs. > > How about instead having a mechanism whereby your usrspace driver can > tell when the device does a remote wakeup? At that point it could > submit an URB via usbfs and receive the async notification. > > Alan Stern Unfortunately, that is not possible in our use-case. It is the composite USB device that needs to detect an activity and convey it to host for taking appropriate action. But since the activity depends on end-user, it can happen at anytime. We do not want to keep the link active, just to detect this, which is not good from battery life point of view.
Re: Query on usb/core/devio.c
On Mo, 2018-10-15 at 09:50 -0400, Alan Stern wrote: > > It seems that a better approach would be to have an ioctl which would: > > fail if there are any active user URBs; > > drop the usbfs PM reference so the device can suspend; > > block interruptibly until the device resumes; Thus we would require user space to have a thread for each device it wants to allow suspend for. [..] > What difference does it make if the URBs are killed by the user instead > of the kernel? We stay within the limits of the timing (as well as we can) and the case of a failed suspend is much easier to handle. Else we have an unlimited time between cessation of IO and going into suspend. > > And notify user space when the device is resumed. IMHO select() > > is the syscall designed for that. > > Apart from the difficulties mentioned above. Yes, I see. Regards Oliver
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
Good morning Oliver, thank you very much for the reply. I will try to build a test kernel hopefully later this evening and I'll get back to you. Meanwhile you can see in the attachment in my first email the output of `$ lsusb -v -d 152d:1561` Is there a way to test UAS without recompiling the Kernel? Like a Kernel argument or modprobe option? Best regards, Julian Xhokaxhiu Full Stack Developer, IT Practised (ISCED 4) https://julianxhokaxhiu.com/ On Tue, Oct 16, 2018 at 10:14 AM Oliver Neukum wrote: > > On Mo, 2018-10-15 at 14:12 +0200, Julian Xhokaxhiu wrote: > > Hi Oliver, > > > > I'm currently using the latest 4.18.12 mainline ( on Arch > > https://www.archlinux.org/packages/core/x86_64/linux/ ), and yes > > you're right I am NOT using UAS at the moment. The link I left is > > because I noticed those errors on dmesg, and I thought I had it > > enabled. This is why I am writing to you right now :) > > > > I personally think the SSD sometimes hangs because TRIM commands are > > not sent properly to the adapter. Although I'm not 100% sure, this is > > why I would like to test UAS. > > > > Can you please point me to the right instructions for trying UAS on my > > adapter? So I can report back the outcome. > > > > Also I noticed that is blacklisted because it's detected as ASMedia ( > > although it's JMicron ) and falls in one of the four cases that checks > > for speed ( or something like that, I can't find the link to the > > source code again otherwise I would have pointed to the code line ). > > If it is detected as ASMedia, you can make a test kernel that removes > this code: > > if (le16_to_cpu(udev->descriptor.idVendor) == 0x174c && > (le16_to_cpu(udev->descriptor.idProduct) == 0x5106 || > le16_to_cpu(udev->descriptor.idProduct) == 0x55aa)) { > if (udev->actconfig->desc.bMaxPower == 0) { > /* ASM1153, do nothing */ > } else if (udev->speed < USB_SPEED_SUPER) { > /* No streams info, assume ASM1051 */ > flags |= US_FL_IGNORE_UAS; > } else if (usb_ss_max_streams(&eps[1]->ss_ep_comp) == 32) { > /* Possibly an ASM1051, disable uas */ > flags |= US_FL_IGNORE_UAS; > } else { > /* ASM1053, these have issues with large transfers */ > flags |= US_FL_MAX_SECTORS_240; > } > } > > from uas-detect.h > > Yet it seems not to match the log you posted. Could you post the output of > "lsusb -v" for your device? > > A problem with TRIM is certainly worth investigating. > > Regards > Oliver >
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
On Mo, 2018-10-15 at 14:12 +0200, Julian Xhokaxhiu wrote: > Hi Oliver, > > I'm currently using the latest 4.18.12 mainline ( on Arch > https://www.archlinux.org/packages/core/x86_64/linux/ ), and yes > you're right I am NOT using UAS at the moment. The link I left is > because I noticed those errors on dmesg, and I thought I had it > enabled. This is why I am writing to you right now :) > > I personally think the SSD sometimes hangs because TRIM commands are > not sent properly to the adapter. Although I'm not 100% sure, this is > why I would like to test UAS. > > Can you please point me to the right instructions for trying UAS on my > adapter? So I can report back the outcome. > > Also I noticed that is blacklisted because it's detected as ASMedia ( > although it's JMicron ) and falls in one of the four cases that checks > for speed ( or something like that, I can't find the link to the > source code again otherwise I would have pointed to the code line ). If it is detected as ASMedia, you can make a test kernel that removes this code: if (le16_to_cpu(udev->descriptor.idVendor) == 0x174c && (le16_to_cpu(udev->descriptor.idProduct) == 0x5106 || le16_to_cpu(udev->descriptor.idProduct) == 0x55aa)) { if (udev->actconfig->desc.bMaxPower == 0) { /* ASM1153, do nothing */ } else if (udev->speed < USB_SPEED_SUPER) { /* No streams info, assume ASM1051 */ flags |= US_FL_IGNORE_UAS; } else if (usb_ss_max_streams(&eps[1]->ss_ep_comp) == 32) { /* Possibly an ASM1051, disable uas */ flags |= US_FL_IGNORE_UAS; } else { /* ASM1053, these have issues with large transfers */ flags |= US_FL_MAX_SECTORS_240; } } from uas-detect.h Yet it seems not to match the log you posted. Could you post the output of "lsusb -v" for your device? A problem with TRIM is certainly worth investigating. Regards Oliver
Re: WARNING in usb_submit_urb (3)
On Mo, 2018-10-15 at 13:12 -0400, Alan Stern wrote: > On Mon, 15 Oct 2018, Andrey Konovalov wrote: > > Ah, I see the problem. In fact it is the same issue, but the commit > mentioned above contains an error (is_in gets tested too soon). The > fix is below; can you check it? > > Alan Stern > Thanks for the catch. Regards Oliver