flash fail with mediatek device
Hello, I would like to use "SP Flash Tool" to flash Android MediaTek. Process ends with error S_FT_DA_NO_RESPONSE and I have no more ideas how to proceed. Internet has many clips on non-linux system that shows working "SP Flash Tool" with specific CDC driver. Unfortunately is not clear configuration for working Linux version of program. Question is how to get it working. kernel: 4.4.176. Remark: does not work with previous 4.4.* kernels. modules: cdc_acm loaded in advance. program: SP_Flash_Tool v5.1824 (Linux) . Remark: fail with previous as well. udev rule that stops ModemManaget exist (system) - ID_MM_DEVICE_IGNORE is set udev rule that stops MTP probe added (host) - MTP_NO_PROBE is set Device: lsusb ... Bus 002 Device 004: ID 0e8d:2000 MediaTek Inc. MT65xx Preloader ... When the program "SP Flash Tool" is in "download" mode, i.e. it has to and device is plugged following is visible: On terminal: == Connecting to BROM... Scanning USB port... Search usb, timeout set as 360 ms add@/devices/pci:00/:00:13.2/usb4/4-3 add@/devices/pci:00/:00:13.2/usb4/4-3/4-3:1.0 add@/devices/pci:00/:00:13.2/usb4/4-3/4-3:1.1 add@/devices/pci:00/:00:13.2/usb4/4-3/4-3:1.1/tty/ttyACM0 vid is 0e8d device vid = 0e8d pid is 2000 device pid = 2000 com portName is: /dev/ttyACM0 Total wait time = -1556345943.00 USB port is obtained. path name(/dev/ttyACM0), port name(/dev/ttyACM0) USB port detected: /dev/ttyACM0 BROM connected Downloading & Connecting to DA... connect DA end stage: 2, enable DRAM in 1st DA: 0 COM port is open. Trying to sync with the target... Failed to Connect DA: S_FT_DA_NO_RESPONSE Disconnect! BROM Exception! ( ERROR : S_FT_DA_NO_RESPONSE (4001) DA didn't send response data to FlashTool! == System log == ... .. kernel: [...] usb 4-3: new high-speed USB device number 3 using ehci-pci .. kernel: [...] usb 4-3: New USB device found, idVendor=0e8d, idProduct=2000 .. kernel: [...] usb 4-3: New USB device strings: Mfr=1, Product=2, SerialNumber=0 .. kernel: [...] usb 4-3: Product: MT65xx Preloader .. kernel: [...] usb 4-3: Manufacturer: MediaTek .. kernel: [...] cdc_acm 4-3:1.1: ttyACM0: USB ACM device ... == Remark: if "SP Flash Tool" is not in "download" mode device disconnects immediately. Regards, Roumen Petrov P.S. verbose data for USB device. a) lsusb -v -s 002:004 (stderr): can't get device qualifier: Resource temporarily unavailable can't get debug descriptor: Resource temporarily unavailable cannot read device status, Resource temporarily unavailable (11) b) lsusb -v -s 002:004 (stdout): Bus 002 Device 004: ID 0e8d:2000 MediaTek Inc. MT65xx Preloader Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 2 Communications bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 64 idVendor 0x0e8d MediaTek Inc. idProduct 0x2000 MT65xx Preloader bcdDevice 1.00 iManufacturer 1 (error) iProduct 2 (error) iSerial 0 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 70 bNumInterfaces 2 bConfigurationValue 1 iConfiguration 3 (error) bmAttributes 0xc0 Self Powered MaxPower 500mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 10 CDC Data bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 4 (error) Endpoint Descriptor: bLength 8 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Endpoint Descriptor: bLength 8 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 0 bNumEndpoints 1 bInterfaceCl
Re: [PATCH 0/5] USB: fix tty unthrottle races
On Thu, Apr 25, 2019 at 06:05:35PM +0200, Johan Hovold wrote: > This series fixes a couple of long-standing issues in USB serial and > cdc-acm which essentially share the same implementation. > > As noted by Oliver a few years back, read-urb completion can race with > unthrottle() running on another CPU and this can potentially lead to > memory corruption. This particular bug in cdc-acm was unfortunately > reintroduced a year later. > > There's also a second race due to missing memory barriers which could > theoretically lead to the port staying throttled until reopened on > weakly ordered systems. A second set of memory barriers should address > that. > Note that the cdc-acm patches have so far only been compile tested. I've tested also the cdc-acm changes now. So unless anyone complains, I'll apply the USB-serial ones in a few days, and maybe Greg can pick up the cdc-acm patches. Johan
Re: flash fail with mediatek device
On Mon, Apr 29, 2019 at 11:13:12AM +0300, Румен Петров wrote: > Hello, > > I would like to use "SP Flash Tool" to flash Android MediaTek. That's great, but there is nothing that we can do to help out here, please contact MediaTek about this as this is their specific userspace tool, and you are using a specific MediaTek kernel, which only they can support. Good luck! greg k-h
Re: [PATCH 1/5] USB: serial: fix unthrottle races
On Do, 2019-04-25 at 18:05 +0200, Johan Hovold wrote: > @@ -484,6 +503,12 @@ void usb_serial_generic_unthrottle(struct tty_struct > *tty) > port->throttled = port->throttle_req = 0; > spin_unlock_irq(&port->lock); > > + /* > +* Matches the smp_mb__after_atomic() in > +* usb_serial_generic_read_bulk_callback(). > +*/ > + smp_mb(); > + > if (was_throttled) > usb_serial_generic_submit_read_urbs(port, GFP_KERNEL); Doesn't the spin_unlock_irq() imply smp_mb()? Otherwise it looks correct to me. Regards Oliver
Re: [PATCH 1/5] USB: serial: fix unthrottle races
On Mon, Apr 29, 2019 at 11:50:58AM +0200, Oliver Neukum wrote: > On Do, 2019-04-25 at 18:05 +0200, Johan Hovold wrote: > > @@ -484,6 +503,12 @@ void usb_serial_generic_unthrottle(struct tty_struct > > *tty) > > port->throttled = port->throttle_req = 0; > > spin_unlock_irq(&port->lock); > > > > + /* > > +* Matches the smp_mb__after_atomic() in > > +* usb_serial_generic_read_bulk_callback(). > > +*/ > > + smp_mb(); > > + > > if (was_throttled) > > usb_serial_generic_submit_read_urbs(port, GFP_KERNEL); > > > Doesn't the spin_unlock_irq() imply smp_mb()? > Otherwise it looks correct to me. No, spin_unlock_irq() is only a one-way barrier, and doesn't prevent later accesses from "moving" into the locked section. Johan
Re: [PATCH 5/5] USB: cdc-acm: clean up throttle handling
On Do, 2019-04-25 at 18:05 +0200, Johan Hovold wrote: > Clean up the throttle implementation by dropping the redundant > throttle_req flag which was a remnant from back when USB serial had only > a single read URB, something which was later carried over to cdc-acm. > > Also convert the throttled flag to an atomic bit flag. > > Signed-off-by: Johan Hovold Acked-by: Oliver Neukum
Re: [PATCH 4/5] USB: cdc-acm: fix unthrottle races
On Do, 2019-04-25 at 18:05 +0200, Johan Hovold wrote:
> Fix two long-standing bugs which could potentially lead to memory
> corruption or leave the port throttled until it is reopened (on weakly
> ordered systems), respectively, when read-URB completion races with
> unthrottle().
>
> First, the URB must not be marked as free before processing is complete
> to prevent it from being submitted by unthrottle() on another CPU.
>
> CPU 1 CPU 2
>
> complete() unthrottle()
> process_urb();
> smp_mb__before_atomic();
> set_bit(i, free); if (test_and_clear_bit(i, free))
> submit_urb();
>
> Second, the URB must be marked as free before checking the throttled
> flag to prevent unthrottle() on another CPU from failing to observe that
> the URB needs to be submitted if complete() sees that the throttled flag
> is set.
>
> CPU 1 CPU 2
>
> complete() unthrottle()
> set_bit(i, free); throttled = 0;
> smp_mb__after_atomic(); smp_mb();
> if (throttled) if (test_and_clear_bit(i, free))
> return; submit_urb();
>
> Note that test_and_clear_bit() only implies barriers when the test is
> successful. To handle the case where the URB is still in use an explicit
> barrier needs to be added to unthrottle() for the second race condition.
>
> Also note that the first race was fixed by 36e59e0d70d6 ("cdc-acm: fix
> race between callback and unthrottle") back in 2015, but the bug was
> reintroduced a year later.
>
> Fixes: 1aba579f3cf5 ("cdc-acm: handle read pipe errors")
> Fixes: 088c64f81284 ("USB: cdc-acm: re-write read processing")
> Signed-off-by: Johan Hovold
Acked-by: Oliver Neukum
Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.
On 4/27/2019 00:43, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> wrote:
>>
>> - Added backup of DCFG register.
>> - Added Set the Power-On Programming done bit.
>>
>> Signed-off-by: Artur Petrosyan
>> ---
>> drivers/usb/dwc2/gadget.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 6812a8a3a98b..dcb0fbb8bc42 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct dwc2_hsotg
>> *hsotg, int remote_wakeup)
>> {
>> struct dwc2_dregs_backup *dr;
>> int i;
>> + u32 dctl;
>>
>> dev_dbg(hsotg->dev, "%s\n", __func__);
>>
>> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct dwc2_hsotg
>> *hsotg, int remote_wakeup)
>> if (!remote_wakeup)
>> dwc2_writel(hsotg, dr->dctl, DCTL);
>>
>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>> + dwc2_writel(hsotg, dr->dcfg, DCFG);
>> +
>> + /* Set the Power-On Programming done bit */
>> + dctl = dwc2_readl(hsotg, DCTL);
>> + dctl |= DCTL_PWRONPRGDONE;
>> + dwc2_writel(hsotg, dctl, DCTL);
>> + }
>
> I can't vouch one way or the other for the correctness of this change,
> but I will say that it's frustrating how asymmetric hibernate and
> partial power down are. It makes things really hard to reason about.
> Is there any way you could restructure this so it happens in the same
> way between hibernate and partial power down?
>
> Specifically looking at the similar sequence in
> dwc2_gadget_exit_hibernation() (which calls this function), I see:
>
> 1. There are some extra delays. Are those needed for partial power down?
Do you mean delays in dwc2_gadget_exit_hibernation() ? If yes they are
needed for hibernation flow. What relates to delays in hibernation
needed for partial power down. Anything that is implemented in the
hibernation delays or other things are part of hibernation and are not
used in partial power down because they are two different flows of power
saving.
>
> 2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
> happens if "not remote wakeup". Is it truly on purpose that you don't
> do that?
Currently partial power down doesn't support remote wakeup flow.
>
> 3. I see that dctl gets "DCTL_PWRONPRGDONE" set as part of the
> sequence in the "not remote wakeup" case before calling this function.
> ...but then part of the function (that you didn't change) clobbers it,
> I think.
>
Setting device programming done bit in dwc2_gadget_exit_hibernation()
comes from older code and from debugging I noticed that if it is not
done at that point then the flow brakes.
So in partial power down flow we need to set that bit wile restoring
device registers. That is why the implementation is such.
>
> I have no idea if any of that is a problem but the fact that the
> hibernate and partial power down code runs through such different
> paths makes it really hard to know / reason about. Many of those
> differences exist before your patch, but you're adding a new
> difference rather than trying to unify and that worries me.
>
>
So to make it easy to reason about I think we should debug it. Please
point out where it fails. Have you tested this flow and did you see any
wrong behavior of hibernation or partial power down? if yes please
provide the debug logs so that they can be investigated.
All of these patches have been tested on HAPS-DX and and Linaro HiKey
960 board. These patches fix Hibernation and Partial Power down issues.
>
> -Doug
>
--
Regards,
Artur
Re: [PATCH v1 03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.
Hi,
On 4/27/2019 00:44, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> wrote:
>>
>> @@ -426,8 +438,6 @@ static void dwc2_handle_wakeup_detected_intr(struct
>> dwc2_hsotg *hsotg)
>> /* Change to L0 state */
>> hsotg->lx_state = DWC2_L0;
>> } else {
>> - if (hsotg->params.power_down)
>> - return;
>>
>
> nit: you leave an odd blank line here. Please delete it.
>
> -Doug
>
Agree with this thank you for the review.
--
Regards,
Artur
Re: [PATCH v1 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.
Hi,
On 4/27/2019 00:45, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> wrote:
>>
>> - In dwc2_port_suspend() function added waiting for the
>>HPRT0.PrtSusp register field to be set.
>>
>> - In _dwc2_hcd_suspend() function added checking of
>>"hsotg->flags.b.port_connect_status" port connection
>>status if port connection status is 0 then skipping
>>power saving (entering partial power down mode).
>>Because if there is no device connected there would
>>be no need to enter partial power down mode.
>>
>> - Added "hsotg->bus_suspended = true" beceuse after
>
> s/beceuse/because
>
>>entering partial power down in host mode the
>>bus_suspended flag must be set.
>
> The above statement sounds to me like trying to win an argument by
> saying "I'm right because I'm right." Can you give more details about
> why "bus_suspended" must be set? See below also.
>
There is no intention of winning any argument.
Are you familiar with "bus_suspended" flag ? have you looked at what is
it used for ?
* @bus_suspended: True if bus is suspended
So when entering to hibernation is performed bus is suspended. To
indicate that "hsotg->bus_suspended" is assigned to true.
>
>> Signed-off-by: Artur Petrosyan
>> ---
>> drivers/usb/dwc2/hcd.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index dd82fa516f3f..1d18258b5ff8 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg
>> *hsotg, u16 windex)
>> hprt0 |= HPRT0_SUSP;
>> dwc2_writel(hsotg, hprt0, HPRT0);
>>
>> + /* Wait for the HPRT0.PrtSusp register field to be set */
>> + if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
>> + dev_warn(hsotg->dev, "Suspend wasn't generated\n");
>> +
>> hsotg->bus_suspended = true;
>>
>> /*
>> @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>> goto unlock;
>>
>> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>> + if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
>> + hsotg->flags.b.port_connect_status == 0)
>> goto skip_power_saving;
>>
>> /*
>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>> goto skip_power_saving;
>> }
>>
>> + hsotg->bus_suspended = true;
>> +
>
> I'm a bit unsure about this. Specifically I note that
> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
> Does that now become dead code?
No it doesn't became a dead code.
>
> ...I talk about this a bit more in my review of ("usb: dwc2: Add
> enter/exit hibernation from system issued suspend/resume")
>
>
> -Doug
>
--
Regards,
Artur
Re: [PATCH v1 08/14] usb: dwc2: Add default param to control power optimization.
Hi,
On 4/27/2019 00:46, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> wrote:
>>
>> - Added a default param "power_saving" to enable or
>>disable hibernation or partial power down features.
>>
>> - Printed hibernation param in hw_params_show and
>>power_saving param in params_show.
>>
>> Signed-off-by: Artur Petrosyan
>> Signed-off-by: Minas Harutyunyan
>> ---
>> drivers/usb/dwc2/core.h| 3 +++
>> drivers/usb/dwc2/debugfs.c | 2 ++
>> drivers/usb/dwc2/params.c | 19 +--
>> 3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 30bab8463c96..9221933ab64e 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -373,6 +373,8 @@ enum dwc2_ep0_state {
>>* case.
>>* 0 - No (default)
>>* 1 - Yes
>> + * @power_saving: Specifies if power saving is enabled or not. If it is
>> + * enabled power_down functionality will be enabled.
>>* @power_down: Specifies whether the controller support
>> power_down.
>>* If power_down is enabled, the controller will enter
>>* power_down in both peripheral and host mode when
>
> Why are you adding a new parameter? power_saving should be exactly
> the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that
> anywhere you need it.
Customers should have a parameter using which they will disable entire
power saving hibernation and Partial Power Down support.
power_down is used to see which power saving mode we got
(hibernation/partial power down).
>
> Having two parameters like you're doing is just asking for them to get
> out of sync. ...and, in fact, I think they will get out of sync. On
> rk3288, for instance:
>
> -> dwc2_set_default_params()
> ---> power_saving = true
> ---> dwc2_set_param_power_down()
> -> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
> -> set_params(), which is actually dwc2_set_rk_params()
> ---> power_down = 0
Setting power_down = 0 is a wrong and old option of disabling power
saving feature because if we set power_down = 0 then it shows that there
is no support for any power saving mode. That is why this patch is
introduced to provide an easier way of disabling power saving modes.
>
>
> ...so at the end of dwc2_init_params() you will have power_saving =
> true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems
> bad. ...and, in fact:
>
> # grep '^power' /sys/kernel/debug/*.usb/params
> /sys/kernel/debug/ff54.usb/params:power_saving : 1
> /sys/kernel/debug/ff54.usb/params:power_down: 0
> /sys/kernel/debug/ff58.usb/params:power_saving : 1
> /sys/kernel/debug/ff58.usb/params:power_down: 0
>
>
> ...so while you could fix all of the various set_params() functions,
> it seems better to just drop this patch since I don't think it buys
> anything.
I don't think we should drop this patch. Because, it is introducing the
correct way of disabling power saving (hibernation/partial power down
modes). Explanation is listed above.
>
> -Doug
>
--
Regards,
Artur
Re: flash fail with mediatek device
Greg KH wrote: On Mon, Apr 29, 2019 at 11:13:12AM +0300, Румен Петров wrote: Hello, I would like to use "SP Flash Tool" to flash Android MediaTek. That's great, but there is nothing that we can do to help out here, please contact MediaTek about this as this is their specific userspace tool, and you are using a specific MediaTek kernel, which only they can support. Quite interesting. Many guides that claim use of SP Flash Tool" mention Ubuntu 14.04 (kernel 3.13), 16.04 (kernel 4.4) and 16.10 (kernel 4.8). That's way I wonder how all those people use flash tool. Good luck! greg k-h Roumen
[PATCH] usb: dwc2: Set actual frame number for completed ISOC transfer for none DDMA
On ISOC OUT transfer completion, in none DDMA mode, set actual frame number returning to function driver in usb_request. Signed-off-by: Minas Harutyunyan --- drivers/usb/dwc2/gadget.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 6812a8a3a98b..608e0b1331c5 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2402,6 +2402,10 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg *hsotg, int epnum) dwc2_gadget_incr_frame_num(hs_ep); } + /* Set actual frame number for completed transfers */ + if (!using_desc_dma(hsotg) && hs_ep->isochronous) + req->frame_number = hsotg->frame_number; + dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, result); } -- 2.11.0
Re: [PATCH v1 09/14] usb: dwc2: Update dwc2_handle_usb_suspend_intr function.
On 4/27/2019 00:52, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> wrote:
>>
>> To avoid working in two modes (partial power down
>> and hibernation) changed conditions for entering
>> partial power down or hibernation.
>>
>> Instead of checking hw_params.power_optimized and
>> hw_params.hibernation now checking power_down
>> param which already set to one of the options
>> (Hibernation or Partial Power Down) based on
>> OTG_EN_PWROPT.
>>
>> Signed-off-by: Artur Petrosyan
>> Signed-off-by: Minas Harutyunyan
>> ---
>> drivers/usb/dwc2/core_intr.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> In general I'm in support of this patch--it's cleaner and gets rid of
> a needless goto \o/
>
> ...but you don't go far enough. You can fully get rid of all of the
> "-ENOTSUPP" stuff. I've actually picked my patches and yours atop
> Felipe's "testing/next" tree and you can find it here:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190426-2Ddwc2-2Dstuff&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=WBAPjgkB_xB8UlcsYvQdxyxg2a3wC70A-jrd4IucYKw&s=6HkWJc-CszWClXSA1Ja9AupZVe7Qb4VTMofH8yTmj0o&e=
>
> ...as part of that I've included a patch ("usb: dwc2: Get rid of
> useless error checks for hibernation/partial power down"), AKA:
Have you tested the patch? on which platform?
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2B_0c924f736e2f7c1bb02531aa33c04a3ae5f4fc4c&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=WBAPjgkB_xB8UlcsYvQdxyxg2a3wC70A-jrd4IucYKw&s=pm6jeDE--3WsqgUui0ZU15vcvHZRQ05jA8mvP1LohS0&e=
>
> Feel free to squash that into your patch or add it to your series if
> you like it. Note that patch points out that there's are still some
> instances where calling dwc2_exit_partial_power_down() might still
> happen in a case where it's not obvious if we were in partial power
> down mode and made me wonder if there might be some bugs there.
I will test it too. In case it is fully ok and has no issues. I will let
you know.
>
> -Doug
>
--
Regards,
Artur
Re: [PATCH v1 14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume
Hi,
On 4/27/2019 01:01, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> wrote:
>>
>> Added a new flow of entering and exiting hibernation when PC is
>> hibernated or suspended.
>>
>> Signed-off-by: Artur Petrosyan
>> ---
>> drivers/usb/dwc2/hcd.c | 128
>> +++--
>> 1 file changed, 81 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 45d4a3e1ebd2..f1e92a287cb1 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>> goto unlock;
>>
>> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
>> hsotg->flags.b.port_connect_status == 0)
>> goto skip_power_saving;
>>
>> - /*
>> -* Drive USB suspend and disable port Power
>> -* if usb bus is not suspended.
>> -*/
>> - if (!hsotg->bus_suspended) {
>> - hprt0 = dwc2_read_hprt0(hsotg);
>> - hprt0 |= HPRT0_SUSP;
>> - hprt0 &= ~HPRT0_PWR;
>> - dwc2_writel(hsotg, hprt0, HPRT0);
>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>> - dwc2_vbus_supply_exit(hsotg);
>> - spin_lock_irqsave(&hsotg->lock, flags);
>> - }
>> + switch (hsotg->params.power_down) {
>> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
>> + /*
>> +* Drive USB suspend and disable port Power
>> +* if usb bus is not suspended.
>> +*/
>> + if (!hsotg->bus_suspended) {
>> + hprt0 = dwc2_read_hprt0(hsotg);
>> + hprt0 |= HPRT0_SUSP;
>> + hprt0 &= ~HPRT0_PWR;
>> + dwc2_writel(hsotg, hprt0, HPRT0);
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>> + dwc2_vbus_supply_exit(hsotg);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>> + }
>>
>> - /* Enter partial_power_down */
>> - ret = dwc2_enter_partial_power_down(hsotg);
>> - if (ret) {
>> - if (ret != -ENOTSUPP)
>> - dev_err(hsotg->dev,
>> - "enter partial_power_down failed\n");
>> + /* Enter partial_power_down */
>> + ret = dwc2_enter_partial_power_down(hsotg);
>> + if (ret) {
>> + if (ret != -ENOTSUPP)
>> + dev_err(hsotg->dev,
>> + "enter partial_power_down failed\n");
>> + goto skip_power_saving;
>> + }
>> + hsotg->bus_suspended = true;
>> + break;
>> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
>> + if (!hsotg->bus_suspended) {
>
> Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
> call dwc2_enter_partial_power_down() even if bus_suspended is true,
> but for hibernate you don't call dwc2_enter_hibernation()?
For Hibernation I do call dwc2_enter_hibernation().
>
>
>> + /* Enter hibernation */
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>> + ret = dwc2_enter_hibernation(hsotg, 1);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>> + if (ret && ret != -ENOTSUPP)
>> + dev_err(hsotg->dev,
>> + "%s: enter hibernation failed\n",
>> + __func__);
>
> nit: no __func__ in dev_xxx() error messages. The device plus the
> message should be enough. Only resort to __func__ if you're forced to
> do an error message without a "struct device *".
This code comes form previous implementations I have not touched it not
to back anything.
>
> nit: as per my comments in an earlier patch, remove special case for -ENOTSUPP
>
> Also, presumably you want to match the error handling in
> DWC2_POWER_DOWN_PARAM_PARTIAL and do a "goto skip_power_saving" when
> you see an error?
When there is an error power_saving should be skipped.
>
>
>> + } else {
>> + goto skip_power_saving;
>> + }
>> + break;
>> + default:
>> goto skip_power_saving;
>> }
>>
>> - hsotg->bus_suspended = true;
>> -
>
> It's a bit weird to remove this, but I guess it just got moved to the
> partial power down case? ...and in the hibernate case you're relying
> on the hibernate function to set this? Yet another frustratingly
> asymmetric code structure...Enter hibernation implement
[PATCH] [PATCH v2] USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)
Prolific has developed a new USB to UART chip: PL2303HXN
PL2303HXN : PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE/PL2303GB
The Vendor request used by the PL2303HXN (TYPE_HXN) is different from
the existing PL2303 series (TYPE_HX & TYPE_01).
Therefore, different Vendor requests are used to issue related commands.
1. Added a new TYPE_HXN type in pl2303_type_data, and then executes
new Vendor request,new flow control and other related instructions
if TYPE_HXN is recognized.
2. Because the new PL2303HXN only accept the new Vendor request,
the old Vendor request cannot be accepted (the error message
will be returned)
So first determine the TYPE_HX or TYPE_HXN through
PL2303_READ_TYPE_HX_STATUS in pl2303_startup.
2.1 If the return message is "1", then the PL2303 is the existing
TYPE_HX/ TYPE_01 series.
The other settings in pl2303_startup are to continue execution.
2.2 If the return message is "not 1", then the PL2303 is the new
TYPE_HXN series.
The other settings in pl2303_startup are ignored.
(PL2303HXN will directly use the default value in the hardware,
no need to add additional settings through the software)
3. In pl2303_open: Because TYPE_HXN is different from the instruction of reset
down/up stream used by TYPE_HX.
Therefore, we will also execute different instructions here.
4. In pl2303_set_termios: The UART flow control instructions used by
TYPE_HXN/TYPE_HX/TYPE_01 are different.
Therefore, we will also execute different instructions here.
5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different
from the vendor request instruction used by TYPE_HX/TYPE_01,
it will also execute different instructions here.
Signed-off-by: Charles Yeh
---
drivers/usb/serial/pl2303.c | 107 +---
drivers/usb/serial/pl2303.h | 6 ++
2 files changed, 92 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bb3f9aa4a909..d938091ba4cc 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GC) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GB) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GT) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GL) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GE) },
+ { USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_GS) },
{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
{ USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),
@@ -129,9 +135,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define VENDOR_WRITE_REQUEST_TYPE 0x40
#define VENDOR_WRITE_REQUEST 0x01
+#define VENDOR_WRITE_NREQUEST 0x80
#define VENDOR_READ_REQUEST_TYPE 0xc0
#define VENDOR_READ_REQUEST0x01
+#define VENDOR_READ_NREQUEST 0x81
#define UART_STATE_INDEX 8
#define UART_STATE_MSR_MASK0x8b
@@ -145,11 +153,19 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define UART_OVERRUN_ERROR 0x40
#define UART_CTS 0x80
+#define PL2303_READ_TYPE_HX_STATUS 0x8080
+#define PL2303_TYPE_HXN_FLOW_CTRL 0x0A
+#define PL2303_TYPE_HXN_CTRL_RTS_CTS 0xFA
+#define PL2303_TYPE_HXN_CTRL_XON_XOFF 0xEE
+#define PL2303_TYPE_HXN_NONE_FLOW 0xFF
+#define PL2303_TYPE_HXN_RESET_DOWN_UPSTREAM0x07
+
static void pl2303_set_break(struct usb_serial_port *port, bool enable);
enum pl2303_type {
TYPE_01,/* Type 0 and 1 (difference unknown) */
TYPE_HX,/* HX version of the pl2303 chip */
+ TYPE_HXN, /* HXN version of the pl2303 chip */
TYPE_COUNT
};
@@ -179,16 +195,26 @@ static const struct pl2303_type_data
pl2303_type_data[TYPE_COUNT] = {
[TYPE_HX] = {
.max_baud_rate =1200,
},
+ [TYPE_HXN] = {
+ .max_baud_rate =1200,
+ },
};
static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
unsigned char buf[1])
{
struct device *dev = &serial->interface->dev;
+ struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
int res;
+ u8 request;
+
+ if (spriv->type == &pl2303_type_data[TYPE_HXN])
+ request = VENDOR_READ_NREQUEST;
+ else
+ request = VENDOR_READ_REQUEST;
res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
- VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYP
[PATCH -next] usb: typec: ucsi: ccg: fix missing unlock on error in ccg_cmd_write_flash_row()
Add the missing unlock before return from function ccg_cmd_write_flash_row()
in the error handling case.
Fixes: 5c9ae5a87573 ("usb: typec: ucsi: ccg: add firmware flashing support")
Signed-off-by: Wei Yongjun
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 4632b91a04a6..9d46aa9e4e35 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -631,6 +631,7 @@ ccg_cmd_write_flash_row(struct ucsi_ccg *uc, u16 row,
ret = i2c_master_send(client, buf, CCG4_ROW_SIZE + 2);
if (ret != CCG4_ROW_SIZE + 2) {
dev_err(uc->dev, "REG_FLASH_RW_MEM write fail %d\n", ret);
+ mutex_unlock(&uc->lock);
return ret < 0 ? ret : -EIO;
}
Re: [PATCH -next] usb: typec: ucsi: ccg: fix missing unlock on error in ccg_cmd_write_flash_row()
On Mon, Apr 29, 2019 at 12:26:30PM +, Wei Yongjun wrote:
> Add the missing unlock before return from function ccg_cmd_write_flash_row()
> in the error handling case.
>
> Fixes: 5c9ae5a87573 ("usb: typec: ucsi: ccg: add firmware flashing support")
> Signed-off-by: Wei Yongjun
Acked-by: Heikki Krogerus
> ---
> drivers/usb/typec/ucsi/ucsi_ccg.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 4632b91a04a6..9d46aa9e4e35 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -631,6 +631,7 @@ ccg_cmd_write_flash_row(struct ucsi_ccg *uc, u16 row,
> ret = i2c_master_send(client, buf, CCG4_ROW_SIZE + 2);
> if (ret != CCG4_ROW_SIZE + 2) {
> dev_err(uc->dev, "REG_FLASH_RW_MEM write fail %d\n", ret);
> + mutex_unlock(&uc->lock);
> return ret < 0 ? ret : -EIO;
> }
>
>
thanks,
--
heikki
[PATCH] UAS: fix alignment of scatter/gather segments
This is the UAS version of
747668dbc061b3e62bc1982767a3a1f9815fcf0e
usb-storage: Set virt_boundary_mask to avoid SG overflows
We are not as likely to be vulnerable as storage, as it is unlikelier
that UAS is run over a controller without native support for SG,
but the issue exists.
Signed-off-by: Oliver Neukum
---
drivers/usb/storage/uas.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6d71b8fff9df..ec9c1c7bb156 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
{
struct uas_dev_info *devinfo =
(struct uas_dev_info *)sdev->host->hostdata;
+ int maxp;
sdev->hostdata = devinfo;
/*
-* USB has unusual DMA-alignment requirements: Although the
-* starting address of each scatter-gather element doesn't matter,
-* the length of each element except the last must be divisible
-* by the Bulk maxpacket value. There's currently no way to
-* express this by block-layer constraints, so we'll cop out
-* and simply require addresses to be aligned at 512-byte
-* boundaries. This is okay since most block I/O involves
-* hardware sectors that are multiples of 512 bytes in length,
-* and since host controllers up through USB 2.0 have maxpacket
-* values no larger than 512.
-*
-* But it doesn't suffice for Wireless USB, where Bulk maxpacket
-* values can be as large as 2048. To make that work properly
-* will require changes to the block layer.
+* USB has unusual scatter-gather requirements: the length of each
+* scatterlist element except the last must be divisible by the
+* Bulk maxpacket value. Fortunately this value is always a
+* power of 2. Inform the block layer about this requirement.
+*/
+
+ maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
+ blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
+
+ /*
+* This one is for the controllers. We assume 512 is always good.
*/
blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
--
2.16.4
RE: [PATCH] UAS: fix alignment of scatter/gather segments
From: Oliver Neukum
> Sent: 29 April 2019 13:20
> This is the UAS version of
>
> 747668dbc061b3e62bc1982767a3a1f9815fcf0e
> usb-storage: Set virt_boundary_mask to avoid SG overflows
>
> We are not as likely to be vulnerable as storage, as it is unlikelier
> that UAS is run over a controller without native support for SG,
> but the issue exists.
>
> Signed-off-by: Oliver Neukum
> ---
> drivers/usb/storage/uas.c | 26 --
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 6d71b8fff9df..ec9c1c7bb156 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -789,24 +789,22 @@ static int uas_slave_alloc(struct scsi_device *sdev)
> {
> struct uas_dev_info *devinfo =
> (struct uas_dev_info *)sdev->host->hostdata;
> + int maxp;
>
> sdev->hostdata = devinfo;
>
> /*
> - * USB has unusual DMA-alignment requirements: Although the
> - * starting address of each scatter-gather element doesn't matter,
> - * the length of each element except the last must be divisible
> - * by the Bulk maxpacket value. There's currently no way to
> - * express this by block-layer constraints, so we'll cop out
> - * and simply require addresses to be aligned at 512-byte
> - * boundaries. This is okay since most block I/O involves
> - * hardware sectors that are multiples of 512 bytes in length,
> - * and since host controllers up through USB 2.0 have maxpacket
> - * values no larger than 512.
> - *
> - * But it doesn't suffice for Wireless USB, where Bulk maxpacket
> - * values can be as large as 2048. To make that work properly
> - * will require changes to the block layer.
> + * USB has unusual scatter-gather requirements: the length of each
> + * scatterlist element except the last must be divisible by the
> + * Bulk maxpacket value. Fortunately this value is always a
> + * power of 2. Inform the block layer about this requirement.
> + */
That isn't the correct restriction for XHCI.
It has its own perverse restrictions.
I think they are all handled within the xhci driver.
David
> +
> + maxp = usb_maxpacket(devinfo->udev, devinfo->data_in_pipe, 0);
> + blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
> +
> + /*
> + * This one is for the controllers. We assume 512 is always good.
>*/
> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>
> --
> 2.16.4
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mo, 2019-04-29 at 13:31 +, David Laight wrote: > From: Oliver Neukum > > > > +* USB has unusual scatter-gather requirements: the length of each > > +* scatterlist element except the last must be divisible by the > > +* Bulk maxpacket value. Fortunately this value is always a > > +* power of 2. Inform the block layer about this requirement. > > +*/ > > That isn't the correct restriction for XHCI. > It has its own perverse restrictions. > I think they are all handled within the xhci driver. Yes, but that does not matter. You just cannot assume that only XHCI will be used with UAS. In particular virtual drivers will be used. Regards Oliver
[PATCH] This patch fixes endian issue in xHCI for scratchpad buffer.
Scratchpad buffer is an array of pointers every of which must be
in little endian format.
Signed-off-by: Aleksey Kuleshov
---
drivers/usb/host/xhci-mem.c | 6 +++---
drivers/usb/host/xhci.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index cf5e179..ffc3236 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1677,7 +1677,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t
flags)
if (!buf)
goto fail_sp4;
- xhci->scratchpad->sp_array[i] = dma;
+ xhci->scratchpad->sp_array[i] = cpu_to_le64(dma);
xhci->scratchpad->sp_buffers[i] = buf;
}
@@ -1687,7 +1687,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t
flags)
for (i = i - 1; i >= 0; i--) {
dma_free_coherent(dev, xhci->page_size,
xhci->scratchpad->sp_buffers[i],
- xhci->scratchpad->sp_array[i]);
+ le64_to_cpu(xhci->scratchpad->sp_array[i]));
}
kfree(xhci->scratchpad->sp_buffers);
@@ -1719,7 +1719,7 @@ static void scratchpad_free(struct xhci_hcd *xhci)
for (i = 0; i < num_sp; i++) {
dma_free_coherent(dev, xhci->page_size,
xhci->scratchpad->sp_buffers[i],
- xhci->scratchpad->sp_array[i]);
+ le64_to_cpu(xhci->scratchpad->sp_array[i]));
}
kfree(xhci->scratchpad->sp_buffers);
dma_free_coherent(dev, num_sp * sizeof(u64),
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9334cde..385fa70 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1621,7 +1621,7 @@ struct xhci_erst {
};
struct xhci_scratchpad {
- u64 *sp_array;
+ __le64 *sp_array;
dma_addr_t sp_dma;
void **sp_buffers;
};
--
1.8.3.1
RE: [PATCH] UAS: fix alignment of scatter/gather segments
From: Oliver Neukum > Sent: 29 April 2019 14:38 > On Mo, 2019-04-29 at 13:31 +, David Laight wrote: > > From: Oliver Neukum > > > > > > + * USB has unusual scatter-gather requirements: the length of each > > > + * scatterlist element except the last must be divisible by the > > > + * Bulk maxpacket value. Fortunately this value is always a > > > + * power of 2. Inform the block layer about this requirement. > > > + */ > > > > That isn't the correct restriction for XHCI. > > It has its own perverse restrictions. > > I think they are all handled within the xhci driver. > > Yes, but that does not matter. You just cannot assume that only > XHCI will be used with UAS. In particular virtual drivers will > be used. True, but there is no need to enforce a 2k (IIRC) alignment for XHCI. Perhaps you need a different property from the controller. Even if you decide the code is 'good enough' (I don't know what the cost is of enforcing a 2k alignment instead of 512 bytes) the comment is just plain wrong. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mo, 2019-04-29 at 14:19 +, David Laight wrote: > From: Oliver Neukum > > Sent: 29 April 2019 14:38 > > On Mo, 2019-04-29 at 13:31 +, David Laight wrote: > > > From: Oliver Neukum > > > > > > > > +* USB has unusual scatter-gather requirements: the length of > > > > each > > > > +* scatterlist element except the last must be divisible by the > > > > +* Bulk maxpacket value. Fortunately this value is always a > > > > +* power of 2. Inform the block layer about this requirement. > > > > +*/ > > > > > > That isn't the correct restriction for XHCI. > > > It has its own perverse restrictions. > > > I think they are all handled within the xhci driver. > > > > Yes, but that does not matter. You just cannot assume that only > > XHCI will be used with UAS. In particular virtual drivers will > > be used. > > True, but there is no need to enforce a 2k (IIRC) alignment for XHCI. > Perhaps you need a different property from the controller. AFAICT controllers do not export that property. > Even if you decide the code is 'good enough' (I don't know what the > cost is of enforcing a 2k alignment instead of 512 bytes) > the comment is just plain wrong. Usually block IO will be pages. They are 4K aligned. In terms of performance this code is unlikely to matter. But it is needed for correctness. What would you want for the comment? Regards Oliver
RE: [PATCH] UAS: fix alignment of scatter/gather segments
From: Oliver Neukum > On Mo, 2019-04-29 at 14:19 +, David Laight wrote: > > From: Oliver Neukum > > > Sent: 29 April 2019 14:38 > > > On Mo, 2019-04-29 at 13:31 +, David Laight wrote: > > > > From: Oliver Neukum > > > > > > > > > > + * USB has unusual scatter-gather requirements: the length of > > > > > each > > > > > + * scatterlist element except the last must be divisible by the > > > > > + * Bulk maxpacket value. Fortunately this value is always a > > > > > + * power of 2. Inform the block layer about this requirement. > > > > > + */ > > > > > > > > That isn't the correct restriction for XHCI. > > > > It has its own perverse restrictions. > > > > I think they are all handled within the xhci driver. > > > > > > Yes, but that does not matter. You just cannot assume that only > > > XHCI will be used with UAS. In particular virtual drivers will > > > be used. > > > > True, but there is no need to enforce a 2k (IIRC) alignment for XHCI. > > Perhaps you need a different property from the controller. > > AFAICT controllers do not export that property. Perhaps they need to > > Even if you decide the code is 'good enough' (I don't know what the > > cost is of enforcing a 2k alignment instead of 512 bytes) > > the comment is just plain wrong. > > Usually block IO will be pages. They are 4K aligned. > In terms of performance this code is unlikely to matter. Presumably there are some cases where the buffer isn't 4k aligned. I'm guessing things like 'dd' of raw disks? If the buffer has the wrong alignment then I presume a bounce buffer has to be allocated? The USB controller drivers are likely to need to be able to do that anyway because buffers from the network stack can have almost arbitrary alignment (I remember that code being horrid, I don't remember a data copy in the TX path). > But it is needed for correctness. If you are doing it for 'correctness' then you need the correct size. If you are doing it because you've seen too small an alignment you should be mentioning the failing system. > What would you want for the comment? You need to be more specific about the alignment requirements than the old comment, not far less specific. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > From: Oliver Neukum > > On Mo, 2019-04-29 at 14:19 +, David Laight wrote: > > AFAICT controllers do not export that property. > > Perhaps they need to Feel free to make a patch. > > > Even if you decide the code is 'good enough' (I don't know what the > > > cost is of enforcing a 2k alignment instead of 512 bytes) > > > the comment is just plain wrong. > > > > Usually block IO will be pages. They are 4K aligned. > > In terms of performance this code is unlikely to matter. > > Presumably there are some cases where the buffer isn't 4k aligned. > I'm guessing things like 'dd' of raw disks? Possibly. > If the buffer has the wrong alignment then I presume a bounce buffer > has to be allocated? > The USB controller drivers are likely to need to be able to do that > anyway because buffers from the network stack can have almost > arbitrary alignment (I remember that code being horrid, I don't > remember a data copy in the TX path). You can ask the network layer to undo scatter/gather. I don't see an issue. > > But it is needed for correctness. > > If you are doing it for 'correctness' then you need the correct size. Why? Any larger size will do. > If you are doing it because you've seen too small an alignment you > should be mentioning the failing system. Why? > > What would you want for the comment? > > You need to be more specific about the alignment requirements than > the old comment, not far less specific. But the statement the old comment made are no longer correct. Regards Oliver
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mon, 29 Apr 2019, Oliver Neukum wrote: > On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > > From: Oliver Neukum > > > On Mo, 2019-04-29 at 14:19 +, David Laight wrote: > > > > AFAICT controllers do not export that property. > > > > Perhaps they need to > > Feel free to make a patch. > > > > > Even if you decide the code is 'good enough' (I don't know what the > > > > cost is of enforcing a 2k alignment instead of 512 bytes) > > > > the comment is just plain wrong. > > > > > > Usually block IO will be pages. They are 4K aligned. > > > In terms of performance this code is unlikely to matter. > > > > Presumably there are some cases where the buffer isn't 4k aligned. > > I'm guessing things like 'dd' of raw disks? > > Possibly. > > > If the buffer has the wrong alignment then I presume a bounce buffer > > has to be allocated? > > The USB controller drivers are likely to need to be able to do that > > anyway because buffers from the network stack can have almost > > arbitrary alignment (I remember that code being horrid, I don't > > remember a data copy in the TX path). > > You can ask the network layer to undo scatter/gather. > I don't see an issue. > > > > But it is needed for correctness. > > > > If you are doing it for 'correctness' then you need the correct size. > > Why? Any larger size will do. > > > If you are doing it because you've seen too small an alignment you > > should be mentioning the failing system. > > Why? > > > > What would you want for the comment? > > > > You need to be more specific about the alignment requirements than > > the old comment, not far less specific. > > But the statement the old comment made are no longer correct. Perhaps David would be satisfied if the comment were changed to say that _some_ USB controller drivers have this unusual alignment requirement. Alan Stern
RE: [PATCH -next] usb: typec: ucsi: ccg: fix missing unlock on error in ccg_cmd_write_flash_row()
Hi Wei
> -Original Message-
> From: Wei Yongjun
> Sent: Monday, April 29, 2019 5:27 AM
> To: Heikki Krogerus ; Greg Kroah-Hartman
> ; Ajay Gupta ; Wolfram Sang
>
> Cc: Wei Yongjun ; linux-usb@vger.kernel.org;
> kernel-janit...@vger.kernel.org
> Subject: [PATCH -next] usb: typec: ucsi: ccg: fix missing unlock on error in
> ccg_cmd_write_flash_row()
>
> Add the missing unlock before return from function ccg_cmd_write_flash_row()
> in the error handling case.
Thanks for fixing this. The change looks good.
> nvpublic
>
> Fixes: 5c9ae5a87573 ("usb: typec: ucsi: ccg: add firmware flashing support")
> Signed-off-by: Wei Yongjun
> ---
> drivers/usb/typec/ucsi/ucsi_ccg.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 4632b91a04a6..9d46aa9e4e35 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -631,6 +631,7 @@ ccg_cmd_write_flash_row(struct ucsi_ccg *uc, u16 row,
> ret = i2c_master_send(client, buf, CCG4_ROW_SIZE + 2);
> if (ret != CCG4_ROW_SIZE + 2) {
> dev_err(uc->dev, "REG_FLASH_RW_MEM write fail %d\n", ret);
> + mutex_unlock(&uc->lock);
> return ret < 0 ? ret : -EIO;
> }
>
>
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote: > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > > > But the statement the old comment made are no longer correct. > > Perhaps David would be satisfied if the comment were changed to say > that _some_ USB controller drivers have this unusual alignment > requirement. It would seem to me that every controller that does not do scatter/gather has this requirement. In other words, this is the true requirement of USB. It does not come from the controller. It comes from the protocol's need to not send a short package. The second, old, comment is about controllers. Regards Oliver
Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.
Hi,
On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
wrote:
>
> On 4/27/2019 00:43, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> > wrote:
> >>
> >> - Added backup of DCFG register.
> >> - Added Set the Power-On Programming done bit.
> >>
> >> Signed-off-by: Artur Petrosyan
> >> ---
> >> drivers/usb/dwc2/gadget.c | 10 ++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >> index 6812a8a3a98b..dcb0fbb8bc42 100644
> >> --- a/drivers/usb/dwc2/gadget.c
> >> +++ b/drivers/usb/dwc2/gadget.c
> >> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct dwc2_hsotg
> >> *hsotg, int remote_wakeup)
> >> {
> >> struct dwc2_dregs_backup *dr;
> >> int i;
> >> + u32 dctl;
> >>
> >> dev_dbg(hsotg->dev, "%s\n", __func__);
> >>
> >> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct dwc2_hsotg
> >> *hsotg, int remote_wakeup)
> >> if (!remote_wakeup)
> >> dwc2_writel(hsotg, dr->dctl, DCTL);
> >>
> >> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> >> + dwc2_writel(hsotg, dr->dcfg, DCFG);
> >> +
> >> + /* Set the Power-On Programming done bit */
> >> + dctl = dwc2_readl(hsotg, DCTL);
> >> + dctl |= DCTL_PWRONPRGDONE;
> >> + dwc2_writel(hsotg, dctl, DCTL);
> >> + }
> >
> > I can't vouch one way or the other for the correctness of this change,
> > but I will say that it's frustrating how asymmetric hibernate and
> > partial power down are. It makes things really hard to reason about.
> > Is there any way you could restructure this so it happens in the same
> > way between hibernate and partial power down?
> >
>
> > Specifically looking at the similar sequence in
> > dwc2_gadget_exit_hibernation() (which calls this function), I see:
> >
> > 1. There are some extra delays. Are those needed for partial power down?
> Do you mean delays in dwc2_gadget_exit_hibernation() ? If yes they are
> needed for hibernation flow. What relates to delays in hibernation
> needed for partial power down. Anything that is implemented in the
> hibernation delays or other things are part of hibernation and are not
> used in partial power down because they are two different flows of power
> saving.
OK, if they aren't needed for partial power down then that's fine.
When I see two functions doing nearly the same sets of writes but one
has delays and the other doesn't it makes me wonder if that was on
purpose or not.
> > 2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
> > happens if "not remote wakeup". Is it truly on purpose that you don't
> > do that?
> Currently partial power down doesn't support remote wakeup flow.
Oh. What happens if you have partial power down enabled and try to
enable remote wakeup? Does it give an error?
> > 3. I see that dctl gets "DCTL_PWRONPRGDONE" set as part of the
> > sequence in the "not remote wakeup" case before calling this function.
> > ...but then part of the function (that you didn't change) clobbers it,
> > I think.
> >
> Setting device programming done bit in dwc2_gadget_exit_hibernation()
> comes from older code and from debugging I noticed that if it is not
> done at that point then the flow brakes.
>
> So in partial power down flow we need to set that bit wile restoring
> device registers. That is why the implementation is such.
>
> >
> > I have no idea if any of that is a problem but the fact that the
> > hibernate and partial power down code runs through such different
> > paths makes it really hard to know / reason about. Many of those
> > differences exist before your patch, but you're adding a new
> > difference rather than trying to unify and that worries me.
> >
> >
>
> So to make it easy to reason about I think we should debug it. Please
> point out where it fails. Have you tested this flow and did you see any
> wrong behavior of hibernation or partial power down? if yes please
> provide the debug logs so that they can be investigated.
>
> All of these patches have been tested on HAPS-DX and and Linaro HiKey
> 960 board. These patches fix Hibernation and Partial Power down issues.
I have no real way to test this code. I'm only setup to use dwc2 as a
USB host since my target device is a laptop with type A ports on it.
Although one of the ports could be made a gadget and I could force the
mode and use an A-to-A cable, I don't have any use cases here nor do I
really have any experience using dwc2 as a gadget.
...so if you and others are happy with the code I won't stand in the
way--I was just reviewing the rest of the series so I figured I'd do a
cursory pass on this patch too.
-Doug
Re: [PATCH v1 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.
Hi, On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan wrote: > > Hi, > > On 4/27/2019 00:45, Doug Anderson wrote: > > Hi, > > > > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan > > wrote: > >> > >> - In dwc2_port_suspend() function added waiting for the > >>HPRT0.PrtSusp register field to be set. > >> > >> - In _dwc2_hcd_suspend() function added checking of > >>"hsotg->flags.b.port_connect_status" port connection > >>status if port connection status is 0 then skipping > >>power saving (entering partial power down mode). > >>Because if there is no device connected there would > >>be no need to enter partial power down mode. > >> > >> - Added "hsotg->bus_suspended = true" beceuse after > > > > s/beceuse/because > > > >>entering partial power down in host mode the > >>bus_suspended flag must be set. > > > > The above statement sounds to me like trying to win an argument by > > saying "I'm right because I'm right." Can you give more details about > > why "bus_suspended" must be set? See below also. > > > There is no intention of winning any argument. I was trying to say that your statement does not convey any information about the "why". It just says: "I now set this variable because it needs to be set". This tells me nothing useful because presumably if it did't need to be set then you wouldn't have set it. I want to know more information about how the code was broken before you did this. What specifically requires this variable to be set? > Are you familiar with "bus_suspended" flag ? have you looked at what is > it used for ? > > * @bus_suspended: True if bus is suspended > > So when entering to hibernation is performed bus is suspended. To > indicate that "hsotg->bus_suspended" is assigned to true. Perhaps you can help me understand what the difference is between "port suspend" and "bus suspend" on dwc2. I think this is where my confusion lies since there are functions for both and they do subtly different things. ...but both functions set bus_suspended. > >> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > >> goto skip_power_saving; > >> } > >> > >> + hsotg->bus_suspended = true; > >> + > > > > I'm a bit unsure about this. Specifically I note that > > _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)". > > Does that now become dead code? > No it doesn't became a dead code. Can you explain when it gets triggered, then? -Doug
Re: [PATCH v1 08/14] usb: dwc2: Add default param to control power optimization.
Hi,
On Mon, Apr 29, 2019 at 4:30 AM Artur Petrosyan
wrote:
>
> Hi,
>
> On 4/27/2019 00:46, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> > wrote:
> >>
> >> - Added a default param "power_saving" to enable or
> >>disable hibernation or partial power down features.
> >>
> >> - Printed hibernation param in hw_params_show and
> >>power_saving param in params_show.
> >>
> >> Signed-off-by: Artur Petrosyan
> >> Signed-off-by: Minas Harutyunyan
> >> ---
> >> drivers/usb/dwc2/core.h| 3 +++
> >> drivers/usb/dwc2/debugfs.c | 2 ++
> >> drivers/usb/dwc2/params.c | 19 +--
> >> 3 files changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >> index 30bab8463c96..9221933ab64e 100644
> >> --- a/drivers/usb/dwc2/core.h
> >> +++ b/drivers/usb/dwc2/core.h
> >> @@ -373,6 +373,8 @@ enum dwc2_ep0_state {
> >>* case.
> >>* 0 - No (default)
> >>* 1 - Yes
> >> + * @power_saving: Specifies if power saving is enabled or not. If it
> >> is
> >> + * enabled power_down functionality will be enabled.
> >>* @power_down: Specifies whether the controller support
> >> power_down.
> >>* If power_down is enabled, the controller will
> >> enter
> >>* power_down in both peripheral and host mode when
> >
> > Why are you adding a new parameter? power_saving should be exactly
> > the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that
> > anywhere you need it.
> Customers should have a parameter using which they will disable entire
> power saving hibernation and Partial Power Down support.
>
> power_down is used to see which power saving mode we got
> (hibernation/partial power down).
>
> >
> > Having two parameters like you're doing is just asking for them to get
> > out of sync. ...and, in fact, I think they will get out of sync. On
> > rk3288, for instance:
> >
> > -> dwc2_set_default_params()
> > ---> power_saving = true
> > ---> dwc2_set_param_power_down()
> > -> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
> > -> set_params(), which is actually dwc2_set_rk_params()
> > ---> power_down = 0
> Setting power_down = 0 is a wrong and old option of disabling power
> saving feature because if we set power_down = 0 then it shows that there
> is no support for any power saving mode. That is why this patch is
> introduced to provide an easier way of disabling power saving modes.
If setting "power_down = 0" is wrong then please update your patch to
remove all the mainline code that sets power_down to 0. Presumably
this means you'd want to convert that code over to using "power_saving
= False". Perhaps then I can see your vision of how this works more
clearly.
NOTE: I'm curious how you envision what someone would do if they had a
core that supported hibernation but they only wanted to enable partial
power down. I guess then they'd have to set "power_saving = True" and
then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your
vision of the world is:
// Example 1: Core supports power savings but we want disabled
// (no code since this is the default)
// Example 2: Pick the best power saving available
params->power_saving = True
// Example 3: Supports hibernation, but we only want partial:
params->power_saving = True
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
My vision of the world is:
// Example 1: Core supports power savings but we want disabled
params->power_down = DWC2_POWER_DOWN_PARAM_NONE
// Example 2: Pick the best power saving available
// (no code since this is the default)
// Example 3: Supports hibernation, but we only want partial:
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
I like that in my vision of the world "pick the best" is the default
(though I suppose we need to fix the driver so it actually works) and
that there's only one variable so you don't have extra confusion.
> > ...so at the end of dwc2_init_params() you will have power_saving =
> > true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems
> > bad. ...and, in fact:
> >
> > # grep '^power' /sys/kernel/debug/*.usb/params
> > /sys/kernel/debug/ff54.usb/params:power_saving : 1
> > /sys/kernel/debug/ff54.usb/params:power_down: 0
> > /sys/kernel/debug/ff58.usb/params:power_saving : 1
> > /sys/kernel/debug/ff58.usb/params:power_down: 0
> >
> >
> > ...so while you could fix all of the various set_params() functions,
> > it seems better to just drop this patch since I don't think it buys
> > anything.
> I don't think we should drop this patch. Because, it is introducing the
> correct way of disabling power saving (hibernation/partial power down
> modes). Explanation is listed above.
I personally see no benefit still. It's
Re: [PATCH v1 14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume
Hi,
On Mon, Apr 29, 2019 at 5:01 AM Artur Petrosyan
wrote:
>
> Hi,
>
> On 4/27/2019 01:01, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> > wrote:
> >>
> >> Added a new flow of entering and exiting hibernation when PC is
> >> hibernated or suspended.
> >>
> >> Signed-off-by: Artur Petrosyan
> >> ---
> >> drivers/usb/dwc2/hcd.c | 128
> >> +++--
> >> 1 file changed, 81 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> >> index 45d4a3e1ebd2..f1e92a287cb1 100644
> >> --- a/drivers/usb/dwc2/hcd.c
> >> +++ b/drivers/usb/dwc2/hcd.c
> >> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> >> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> >> goto unlock;
> >>
> >> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> >> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
> >> hsotg->flags.b.port_connect_status == 0)
> >> goto skip_power_saving;
> >>
> >> - /*
> >> -* Drive USB suspend and disable port Power
> >> -* if usb bus is not suspended.
> >> -*/
> >> - if (!hsotg->bus_suspended) {
> >> - hprt0 = dwc2_read_hprt0(hsotg);
> >> - hprt0 |= HPRT0_SUSP;
> >> - hprt0 &= ~HPRT0_PWR;
> >> - dwc2_writel(hsotg, hprt0, HPRT0);
> >> - spin_unlock_irqrestore(&hsotg->lock, flags);
> >> - dwc2_vbus_supply_exit(hsotg);
> >> - spin_lock_irqsave(&hsotg->lock, flags);
> >> - }
> >> + switch (hsotg->params.power_down) {
> >> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
> >> + /*
> >> +* Drive USB suspend and disable port Power
> >> +* if usb bus is not suspended.
> >> +*/
> >> + if (!hsotg->bus_suspended) {
> >> + hprt0 = dwc2_read_hprt0(hsotg);
> >> + hprt0 |= HPRT0_SUSP;
> >> + hprt0 &= ~HPRT0_PWR;
> >> + dwc2_writel(hsotg, hprt0, HPRT0);
> >> + spin_unlock_irqrestore(&hsotg->lock, flags);
> >> + dwc2_vbus_supply_exit(hsotg);
> >> + spin_lock_irqsave(&hsotg->lock, flags);
> >> + }
> >>
> >> - /* Enter partial_power_down */
> >> - ret = dwc2_enter_partial_power_down(hsotg);
> >> - if (ret) {
> >> - if (ret != -ENOTSUPP)
> >> - dev_err(hsotg->dev,
> >> - "enter partial_power_down failed\n");
> >> + /* Enter partial_power_down */
> >> + ret = dwc2_enter_partial_power_down(hsotg);
> >> + if (ret) {
> >> + if (ret != -ENOTSUPP)
> >> + dev_err(hsotg->dev,
> >> + "enter partial_power_down
> >> failed\n");
> >> + goto skip_power_saving;
> >> + }
> >> + hsotg->bus_suspended = true;
> >> + break;
> >> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> >> + if (!hsotg->bus_suspended) {
> >
> > Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
> > call dwc2_enter_partial_power_down() even if bus_suspended is true,
> > but for hibernate you don't call dwc2_enter_hibernation()?
> For Hibernation I do call dwc2_enter_hibernation().
Maybe you didn't understand the question. I'll be clearer.
Imagine _dwc2_hcd_suspend() is called but "bus_suspended" is already
true at the start of the function.
If we're in DWC2_POWER_DOWN_PARAM_PARTIAL, _dwc2_hcd_suspend() _will_
call dwc2_enter_partial_power_down()
If we're in DWC2_POWER_DOWN_PARAM_HIBERNATION, _dwc2_hcd_suspend()
_will NOT_ call dwc2_enter_partial_power_down()
This is all part of the whole asymmetry between PARTIAL and
HIBERNATION that makes it hard to understand.
> >> + /* Enter hibernation */
> >> + spin_unlock_irqrestore(&hsotg->lock, flags);
> >> + ret = dwc2_enter_hibernation(hsotg, 1);
> >> + spin_lock_irqsave(&hsotg->lock, flags);
> >> + if (ret && ret != -ENOTSUPP)
> >> + dev_err(hsotg->dev,
> >> + "%s: enter hibernation failed\n",
> >> + __func__);
> >
> > nit: no __func__ in dev_xxx() error messages. The device plus the
> > message should be enough. Only resort to __func__ if you're forced to
> > do an error message without a "struct device *".
> This code comes form previous implementations I have not touched it not
> to back anything.
Please fix. Even if you had internal code that did this it still
needs to
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mon, 29 Apr 2019, Oliver Neukum wrote: > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote: > > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > > > On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > > > > > But the statement the old comment made are no longer correct. > > > > Perhaps David would be satisfied if the comment were changed to say > > that _some_ USB controller drivers have this unusual alignment > > requirement. > > It would seem to me that every controller that does not do > scatter/gather has this requirement. In other words, this is > the true requirement of USB. It does not come from the > controller. It comes from the protocol's need to not > send a short package. Are you sure that xHCI has this requirement? I haven't checked the spec. I know that UHCI, OHCI, and EHCI do need this alignment (and OHCI and EHCI do in fact have hardware support for scatter-gather). More precisely, what matters is whether the controller is able to merge two different DMA segments into a single packet. UHCI can't. OHCI and EHCI can, but only if the first segment ends at a page boundary and the second begins at a page boundary -- it's easier just to say that the segments have to be maxpacket-aligned. > The second, old, comment is about controllers. Well, if the drivers would use bounce buffers to work around the controllers' issues then they wouldn't have this special requirement. So it really is a combination of what the hardware can do and what the driver can do. Alan Stern
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote: > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote: > > > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > > > > > On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > > > > > > > But the statement the old comment made are no longer correct. > > > > > > Perhaps David would be satisfied if the comment were changed to say > > > that _some_ USB controller drivers have this unusual alignment > > > requirement. > > > > It would seem to me that every controller that does not do > > scatter/gather has this requirement. In other words, this is > > the true requirement of USB. It does not come from the > > controller. It comes from the protocol's need to not > > send a short package. > > Are you sure that xHCI has this requirement? I haven't checked the I am sure that it has not. UAS would never have worked. Like in the case of storage this patch is necessary for virtual controllers. > spec. I know that UHCI, OHCI, and EHCI do need this alignment (and > OHCI and EHCI do in fact have hardware support for scatter-gather). > > More precisely, what matters is whether the controller is able to merge > two different DMA segments into a single packet. UHCI can't. OHCI and Correct. However, we cannot blindly assume in a class driver that certain controllers will be used. > EHCI can, but only if the first segment ends at a page boundary and the > second begins at a page boundary -- it's easier just to say that the > segments have to be maxpacket-aligned. > > > The second, old, comment is about controllers. > > Well, if the drivers would use bounce buffers to work around the > controllers' issues then they wouldn't have this special requirement. > So it really is a combination of what the hardware can do and what the > driver can do. Yes, but the point of using an API to specify restrictions to the upper layer is to avoid using bounce buffers. Besides, bounce buffers in block IO is interesting in terms of VM implications. Regards Oliver
Re: [PATCH] UAS: fix alignment of scatter/gather segments
On Mon, 29 Apr 2019, Oliver Neukum wrote: > On Mo, 2019-04-29 at 13:55 -0400, Alan Stern wrote: > > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > > > On Mo, 2019-04-29 at 12:08 -0400, Alan Stern wrote: > > > > On Mon, 29 Apr 2019, Oliver Neukum wrote: > > > > > > > > > On Mo, 2019-04-29 at 15:06 +, David Laight wrote: > > > > > > > > > But the statement the old comment made are no longer correct. > > > > > > > > Perhaps David would be satisfied if the comment were changed to say > > > > that _some_ USB controller drivers have this unusual alignment > > > > requirement. > > > > > > It would seem to me that every controller that does not do > > > scatter/gather has this requirement. In other words, this is > > > the true requirement of USB. It does not come from the > > > controller. It comes from the protocol's need to not > > > send a short package. > > > > Are you sure that xHCI has this requirement? I haven't checked the > > I am sure that it has not. UAS would never have worked. > Like in the case of storage this patch is necessary > for virtual controllers. Okay, yes, I agree with what you say. With the addition that some controllers which _do_ support scatter-gather also have this requirement. In fact, xhci-hcd may be the only driver that doesn't need this special alignment. Alan Stern > > spec. I know that UHCI, OHCI, and EHCI do need this alignment (and > > OHCI and EHCI do in fact have hardware support for scatter-gather). > > > > More precisely, what matters is whether the controller is able to merge > > two different DMA segments into a single packet. UHCI can't. OHCI and > > Correct. However, we cannot blindly assume in a class driver that > certain controllers will be used. > > > EHCI can, but only if the first segment ends at a page boundary and the > > second begins at a page boundary -- it's easier just to say that the > > segments have to be maxpacket-aligned. > > > > > The second, old, comment is about controllers. > > > > Well, if the drivers would use bounce buffers to work around the > > controllers' issues then they wouldn't have this special requirement. > > So it really is a combination of what the hardware can do and what the > > driver can do. > > Yes, but the point of using an API to specify restrictions to the > upper layer is to avoid using bounce buffers. Besides, bounce > buffers in block IO is interesting in terms of VM implications. > > Regards > Oliver > > >
Re: [PATCH] usb: gadget: dwc2: fix zlp handling
Hi Felipe,
On 4/1/2019 3:33 PM, Minas Harutyunyan wrote:
On 4/1/2019 2:51 PM, Andrzej Pietrasiewicz wrote:
The patch 10209abe87f5ebfd482a00323f5236d6094d0865
usb: dwc2: gadget: Add scatter-gather mode
avoided a NULL pointer dereference (hs_ep->req == NULL) by
calling dwc2_gadget_fill_nonisoc_xfer_dma_one() directly instead of through
the dwc2_gadget_config_nonisoc_xfer_ddma() wrapper, which unconditionally
dereferenced the said pointer.
However, this was based on an incorrect assumption that in the context of
dwc2_hsotg_program_zlp() the pointer is always NULL, which is not the case.
The result were SB CV MSC tests failing starting from Test Case 6.
Instead, this patch reverts to calling the wrapper and adds a check for
the pointer being NULL inside the wrapper.
Fixes: 10209abe87f5 (usb: dwc2: gadget: Add scatter-gather mode)
Signed-off-by: Andrzej Pietrasiewicz
Acked-by: Minas Harutyunyan
---
drivers/usb/dwc2/gadget.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6812a8a3a98b..e76b2e040b4c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -835,19 +835,22 @@ static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct
dwc2_hsotg_ep *hs_ep,
* with corresponding information based on transfer data.
*/
static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
-struct usb_request *ureq,
-unsigned int offset,
+dma_addr_t dma_buff,
unsigned int len)
{
+ struct usb_request *ureq = NULL;
struct dwc2_dma_desc *desc = hs_ep->desc_list;
struct scatterlist *sg;
int i;
u8 desc_count = 0;
+ if (hs_ep->req)
+ ureq = &hs_ep->req->req;
+
/* non-DMA sg buffer */
- if (!ureq->num_sgs) {
+ if (!ureq || !ureq->num_sgs) {
dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc,
- ureq->dma + offset, len, true);
+ dma_buff, len, true);
return;
}
@@ -1135,7 +1138,7 @@ static void dwc2_hsotg_start_req(struct dwc2_hsotg *hsotg,
offset = ureq->actual;
/* Fill DDMA chain entries */
- dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq, offset,
+ dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset,
length);
/* write descriptor chain address to control register */
@@ -2028,12 +2031,13 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg
*hsotg,
dev_dbg(hsotg->dev, "Receiving zero-length packet on ep%d\n",
index);
if (using_desc_dma(hsotg)) {
+ /* Not specific buffer needed for ep0 ZLP */
+ dma_addr_t dma = hs_ep->desc_list_dma;
+
if (!index)
dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep);
- /* Not specific buffer needed for ep0 ZLP */
- dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &hs_ep->desc_list,
- hs_ep->desc_list_dma, 0, true);
+ dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0);
} else {
dwc2_writel(hsotg, DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) |
DXEPTSIZ_XFERSIZE(0),
This patch is fix for "usb: dwc2: gadget: Add scatter-gather mode" patch
to allow pass USB CV.
Could you please merge to your testing/next this patch also.
Thanks,
Minas
