Re: “Mouse battery low” notification appears even when all notifications disabled

2018-10-16 Thread Greg KH
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

2018-10-16 Thread Zengtao (B)
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

2018-10-16 Thread Peter Chen
 
> 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

2018-10-16 Thread Wolfram Sang

> 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

2018-10-16 Thread Cristian
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

2018-10-16 Thread Colin King
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

2018-10-16 Thread Peter Rosin
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"

2018-10-16 Thread Julian Xhokaxhiu
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

2018-10-16 Thread Fabio Estevam
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

2018-10-16 Thread Heikki Krogerus
+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

2018-10-16 Thread Alan Stern
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

2018-10-16 Thread Alan Stern
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

2018-10-16 Thread Mathias Nyman

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

2018-10-16 Thread Alan Stern
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

2018-10-16 Thread Oliver Neukum
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"

2018-10-16 Thread Oliver Neukum
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

2018-10-16 Thread Mattias Jacobsson
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

2018-10-16 Thread Mathias Nyman

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

2018-10-16 Thread Jan Kara
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

2018-10-16 Thread Greg Kroah-Hartman
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

2018-10-16 Thread Felipe Balbi


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

2018-10-16 Thread Mathias Nyman
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

2018-10-16 Thread Mathias Nyman
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

2018-10-16 Thread Mathias Nyman
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

2018-10-16 Thread Mayuresh Kulkarni
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

2018-10-16 Thread Gustavo A. R. Silva
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

2018-10-16 Thread Mayuresh Kulkarni
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

2018-10-16 Thread Oliver Neukum
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"

2018-10-16 Thread Julian Xhokaxhiu
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"

2018-10-16 Thread Oliver Neukum
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)

2018-10-16 Thread Oliver Neukum
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