* [v3,2/2] usb: dwc3: pci: Intel Merrifield can be host
From: Andy Shevchenko @ 2018-07-26 11:48 UTC (permalink / raw)
To: Felipe Balbi, linux-usb, Greg Kroah-Hartman; +Cc: Andy Shevchenko
On Intel Edison board the OTG function is enabled, thus,
USB can switch to the host mode.
Allow that by changing dr_mode property to "otg" for Intel Merrifield.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/usb/dwc3/dwc3-pci.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index cc4db9a06505..88b800ab6419 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -107,6 +107,12 @@ static const struct property_entry dwc3_pci_intel_properties[] = {
{}
};
+static const struct property_entry dwc3_pci_mrfld_properties[] = {
+ PROPERTY_ENTRY_STRING("dr_mode", "otg"),
+ PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
+ {}
+};
+
static const struct property_entry dwc3_pci_amd_properties[] = {
PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
PROPERTY_ENTRY_U8("snps,lpm-nyet-threshold", 0xf),
@@ -293,7 +299,7 @@ static void dwc3_pci_remove(struct pci_dev *pci)
static const struct pci_device_id dwc3_pci_id_table[] = {
DWC3_PCI_DEV(INTEL, INTEL_BSW, dwc3_pci_intel_properties),
DWC3_PCI_DEV(INTEL, INTEL_BYT, dwc3_pci_intel_properties),
- DWC3_PCI_DEV(INTEL, INTEL_MRFLD, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_MRFLD, dwc3_pci_mrfld_properties),
DWC3_PCI_DEV(INTEL, INTEL_SPTLP, dwc3_pci_intel_properties),
DWC3_PCI_DEV(INTEL, INTEL_SPTH, dwc3_pci_intel_properties),
DWC3_PCI_DEV(INTEL, INTEL_BXT, dwc3_pci_intel_properties),
^ permalink raw reply related
* [v3,1/2] usb: dwc3: pci: Supply device properties via driver data
From: Andy Shevchenko @ 2018-07-26 11:48 UTC (permalink / raw)
To: Felipe Balbi, linux-usb, Greg Kroah-Hartman; +Cc: Andy Shevchenko
For now all PCI enumerated dwc3 devices require some properties
to be present. This allows us to unconditionally append them and supply
via driver_data.
No functional change intended.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/usb/dwc3/dwc3-pci.c | 107 +++++++++++++++++-------------------
1 file changed, 50 insertions(+), 57 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index b29f031dfc23..cc4db9a06505 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -101,52 +101,37 @@ static int dwc3_byt_enable_ulpi_refclock(struct pci_dev *pci)
return 0;
}
+static const struct property_entry dwc3_pci_intel_properties[] = {
+ PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
+ PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
+ {}
+};
+
+static const struct property_entry dwc3_pci_amd_properties[] = {
+ PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
+ PROPERTY_ENTRY_U8("snps,lpm-nyet-threshold", 0xf),
+ PROPERTY_ENTRY_BOOL("snps,u2exit_lfps_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,u2ss_inp3_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,req_p1p2p3_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,del_p1p2p3_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,del_phy_power_chg_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,lfps_filter_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,rx_detect_poll_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,tx_de_emphasis_quirk"),
+ PROPERTY_ENTRY_U8("snps,tx_de_emphasis", 1),
+ /* FIXME these quirks should be removed when AMD NL tapes out */
+ PROPERTY_ENTRY_BOOL("snps,disable_scramble_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
+ PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
+ PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
+ {}
+};
+
static int dwc3_pci_quirks(struct dwc3_pci *dwc)
{
- struct platform_device *dwc3 = dwc->dwc3;
struct pci_dev *pdev = dwc->pci;
- if (pdev->vendor == PCI_VENDOR_ID_AMD &&
- pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
- struct property_entry properties[] = {
- PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
- PROPERTY_ENTRY_U8("snps,lpm-nyet-threshold", 0xf),
- PROPERTY_ENTRY_BOOL("snps,u2exit_lfps_quirk"),
- PROPERTY_ENTRY_BOOL("snps,u2ss_inp3_quirk"),
- PROPERTY_ENTRY_BOOL("snps,req_p1p2p3_quirk"),
- PROPERTY_ENTRY_BOOL("snps,del_p1p2p3_quirk"),
- PROPERTY_ENTRY_BOOL("snps,del_phy_power_chg_quirk"),
- PROPERTY_ENTRY_BOOL("snps,lfps_filter_quirk"),
- PROPERTY_ENTRY_BOOL("snps,rx_detect_poll_quirk"),
- PROPERTY_ENTRY_BOOL("snps,tx_de_emphasis_quirk"),
- PROPERTY_ENTRY_U8("snps,tx_de_emphasis", 1),
- /*
- * FIXME these quirks should be removed when AMD NL
- * tapes out
- */
- PROPERTY_ENTRY_BOOL("snps,disable_scramble_quirk"),
- PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
- PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
- PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
- { },
- };
-
- return platform_device_add_properties(dwc3, properties);
- }
-
if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
- int ret;
-
- struct property_entry properties[] = {
- PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
- PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
- { }
- };
-
- ret = platform_device_add_properties(dwc3, properties);
- if (ret < 0)
- return ret;
-
if (pdev->device == PCI_DEVICE_ID_INTEL_BXT ||
pdev->device == PCI_DEVICE_ID_INTEL_BXT_M) {
guid_parse(PCI_INTEL_BXT_DSM_GUID, &dwc->guid);
@@ -155,6 +140,7 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
if (pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
struct gpio_desc *gpio;
+ int ret;
/* On BYT the FW does not always enable the refclock */
ret = dwc3_byt_enable_ulpi_refclock(pdev);
@@ -216,9 +202,9 @@ static void dwc3_pci_resume_work(struct work_struct *work)
}
#endif
-static int dwc3_pci_probe(struct pci_dev *pci,
- const struct pci_device_id *id)
+static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
{
+ struct property_entry *p = (struct property_entry *)id->driver_data;
struct dwc3_pci *dwc;
struct resource res[2];
int ret;
@@ -261,6 +247,10 @@ static int dwc3_pci_probe(struct pci_dev *pci,
dwc->dwc3->dev.parent = dev;
ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
+ ret = platform_device_add_properties(dwc->dwc3, p);
+ if (ret < 0)
+ return ret;
+
ret = dwc3_pci_quirks(dwc);
if (ret)
goto err;
@@ -297,21 +287,24 @@ static void dwc3_pci_remove(struct pci_dev *pci)
platform_device_unregister(dwc->dwc3);
}
+#define DWC3_PCI_DEV(v, d, data) \
+ { PCI_VDEVICE(v, PCI_DEVICE_ID_##d), (kernel_ulong_t)&data }
+
static const struct pci_device_id dwc3_pci_id_table[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BSW), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BYT), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_MRFLD), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SPTLP), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SPTH), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BXT), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BXT_M), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_KBP), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_GLK), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CNPLP), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CNPH), },
- { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICLLP), },
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_NL_USB), },
+ DWC3_PCI_DEV(INTEL, INTEL_BSW, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_BYT, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_MRFLD, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_SPTLP, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_SPTH, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_BXT, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_BXT_M, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_APL, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_KBP, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_GLK, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_CNPLP, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_CNPH, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(INTEL, INTEL_ICLLP, dwc3_pci_intel_properties),
+ DWC3_PCI_DEV(AMD, AMD_NL_USB, dwc3_pci_amd_properties),
{ } /* Terminating Entry */
};
MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
^ permalink raw reply related
* usb: dwc2: gadget: ISOC's starting flow improvement
From: Felipe Balbi @ 2018-07-26 11:26 UTC (permalink / raw)
To: Minas Harutyunyan; +Cc: John Youn
No top posting ;)
Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
> Hi Felipe,
>
> This patch should be applied after follow patch from Artur Petrosyan:
>
> "[PATCH] usb: dwc2: Change reading of current frame number flow."
Hasn't this been part of mainline for a while already?
commit c7c24e7a047652c558e7aa4b0f54aae3a61aacc4
Author: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
Date: Sat May 5 09:46:26 2018 -0400
usb: dwc2: Change reading of current frame number flow.
The current frame_number is read from core for both
device and host modes. Reading of the current frame
number needs to be performed ASAP due to IRQ latency's.
This is why, it is moved to common interrupt handler.
Accordingly updated dwc2_gadget_target_frame_elapsed()
function which uses stored frame_number instead of
reading frame number.
In cases when target frame value is incremented
the frame_number is required to read again.
Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
^ permalink raw reply
* usb: dwc2: gadget: ISOC's starting flow improvement
From: Minas Harutyunyan @ 2018-07-26 11:25 UTC (permalink / raw)
To: Felipe Balbi, Minas Harutyunyan, Greg Kroah-Hartman,
linux-usb@vger.kernel.org
Cc: John Youn
Hi Felipe,
This patch should be applied after follow patch from Artur Petrosyan:
"[PATCH] usb: dwc2: Change reading of current frame number flow."
Thanks,
Minas
On 7/26/2018 2:45 PM, Felipe Balbi wrote:
> Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
>
>> To start ISOC transfers in handlers dwc2_gadget_handle_nak() and
>> dwc2_gadget_handle_out_token_ep_disabled() driver reads current frame
>> number, based on which, set target frame number to start first ISOC
>> transfer.
>>
>> In case if system's high IRQ latency and multiple EP's asserted
>> interrupt in same frame, there are high probability that when reading
>> current frame number in EP's handlers, actual frame number can be
>> increased. As result for bInterval > 1, starting target frame
>> will be set wrongly and all ISOC packets will be dropped.
>>
>> In patch "usb: dwc2: Change reading of current frame number flow"
>> reading of current frame number done ASAP in common interrupt handler.
>> This frame number stored in frame_number variable which used as
>> starting frame number for ISOC EP's in above mentioned handlers.
>>
>> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
>
> please rebase on top of testing/next after a couple hours:
>
> checking file drivers/usb/dwc2/gadget.c
> Hunk #1 FAILED at 2749.
> Hunk #2 succeeded at 2772 (offset -1 lines).
> Hunk #3 FAILED at 2811.
> 2 out of 3 hunks FAILED
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* usb: dwc3: core: Add quirk for enabling AutoRetry feature in the controller
From: Felipe Balbi @ 2018-07-26 11:14 UTC (permalink / raw)
To: Anurag Kumar Vulisha, Rob Herring
Cc: gregkh@linuxfoundation.org, mark.rutland@arm.com,
v.anuragkumar@gmail.com, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
hi,
Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
> Hi Rob,
>
> Thanks for your comments, please see my comments inline
>
>>-----Original Message-----
>>From: Rob Herring [mailto:robh@kernel.org]
>>Sent: Thursday, July 26, 2018 1:33 AM
>>To: Anurag Kumar Vulisha <anuragku@xilinx.com>
>>Cc: gregkh@linuxfoundation.org; mark.rutland@arm.com; balbi@kernel.org;
>>v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; devicetree@vger.kernel.org;
>>linux-kernel@vger.kernel.org
>>Subject: Re: [PATCH] usb: dwc3: core: Add quirk for enabling AutoRetry feature in the
>>controller
>>
>>On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote:
>>> By default when core sees any transaction error(CRC or overflow)
>>> it replies with terminating retry ACK (Retry=1 and Nump == 0).
>>> Enabling this Auto Retry feature in controller, on seeing any
>>> transaction errors makes the core to send an non-terminating ACK
>>> transaction packet (that is, ACK TP with Retry=1 and Nump != 0).
>>> Doing so will give controller a chance to recover from the error
>>> condition.
>>>
>>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>>> ---
>>> Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++
>>> drivers/usb/dwc3/core.c | 16 ++++++++++++++++
>>> drivers/usb/dwc3/core.h | 6 ++++++
>>> 3 files changed, 27 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 7f13ebe..2ba2bc2 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -94,6 +94,11 @@ Optional properties:
>>> this and tx-thr-num-pkt-prd to a valid, non-zero value
>>> 1-16 (DWC_usb31 programming guide section 1.2.3) to
>>> enable periodic ESS TX threshold.
>>> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the
>>
>>s/_/-/
> Sorry, I am not able to understand what needs to fixed here. Please help me in
> understanding, so that I can fix it in v2.
replace _ with -
>>> + controller operating in Host mode on seeing transaction
>>> + errors(CRC errors or internal overrun scenerios) on IN
>>> + transfers to reply to the device with a non-terminating
>>> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0)
>>
>>Seems like the property is unnecessary. When would you not want this
>>retry behavior? Why not just enable unconditionally in the driver?
>>
> There is no harm in adding this fix always but I think this Retry
> feature should be added depending on the user and type of the
> application. For example, applying this feature in a streaming
> application where isochronous transfers are used might end up in
read the docs. Auto Retry in only for non-isochronous IN endpoints. You
don't need any quirk for this. Just enable it unconditionally. Actually,
you wanna make sure the core can run in Host mode before setting this,
but that's it. No need for a quirk.
^ permalink raw reply
* [v6,4/4] arm: arm64: dts: add property snps incr burst type adjustment
From: Felipe Balbi @ 2018-07-26 11:10 UTC (permalink / raw)
To: Pengbo Mu, Rob Herring, Mark Rutland, Catalin Marinas,
Will Deacon, Greg Kroah-Hartman
Cc: linux-kernel, linux-usb, devicetree, linux-arm-kernel, ran.wang_1
Pengbo Mu <pengbo.mu@nxp.com> writes:
> Property "snps,incr-burst-type-adjustment = <x>, <y>..." for USB3.0
> DWC3.When only one value means INCRx mode with fix burst type.
> When more than one value, means undefined length burst mode, USB
> controller can use the length less than or equal to the largest
> enabled burst length.
>
> While enabling undefined length INCR burst type and INCR16 burst type,
> get better write performance on NXP Layerscape platforms: around 3%
> improvement (from 364MB/s to 375MB/s).
>
> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> Signed-off-by: Pengbo Mu <pengbo.mu@nxp.com>
will this go through arm64 tree or do you need me to pick it up?
^ permalink raw reply
* [v2,3/3] dwc3: Intel Merrifield can be host
From: Felipe Balbi @ 2018-07-26 11:08 UTC (permalink / raw)
To: Andy Shevchenko, linux-usb, Greg Kroah-Hartman, Ferry Toth
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Intel Edison board the OTG function is enabled, thus,
> USB can switch to the host mode.
>
> Allow that by changing dr_mode property to "otg" for Intel Merrifield.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
this will need rebasing too
^ permalink raw reply
* [1/1] usb:gadget:function:fix memory leak
From: Felipe Balbi @ 2018-07-26 11:08 UTC (permalink / raw)
To: Xidong Wang, Greg Kroah-Hartman, Johan Hovold, Michal Nazarewicz,
Vincent Pelletier
Cc: linux-usb, linux-kernel
hi,
Xidong Wang <wangxidong_97@163.com> writes:
> In function f_audio_set_alt(), the memory allocated by
> usb_ep_alloc_request() is not released on the error path
> that req->buf, which holds the return value of kzalloc(),
> is NULL. This will result in a memory leak bug.
>
> Signed-off-by: Xidong Wang <wangxidong_97@163.com>
> ---
> drivers/usb/gadget/function/f_uac1_legacy.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 24c086b..2fcdade 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -630,8 +630,11 @@ static int f_audio_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> ERROR(cdev,
> "%s queue req: %d\n",
> out_ep->name, err);
> - } else
> + } else {
> + usb_ep_free_request(
> + out_ep, req);
> err = -ENOMEM;
> + }
I feel like this hunk has been ping ponging between having
usb_ep_free_request() and not having it because completion callback will
call usb_ep_free_request() or something along those lines.
Can we get a final solution that solves all cases and doesn't introduce
other bugs?
^ permalink raw reply
* [v1,2/2] dwc3: Supply device properties via driver data
From: Felipe Balbi @ 2018-07-26 11:06 UTC (permalink / raw)
To: Andy Shevchenko, linux-usb, Greg Kroah-Hartman
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> For now all PCI enumerated dwc3 devices require some properties
> to be present. This allows us to unconditionally append them and supply
> via driver_data.
>
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Doesn't apply. Please rebase on testing/next in a couple hours after I
push it.
checking file drivers/usb/dwc3/dwc3-pci.c
Hunk #1 succeeded at 101 with fuzz 2 (offset 33 lines).
Hunk #2 succeeded at 148 with fuzz 2 (offset 33 lines).
Hunk #3 FAILED at 146.
Hunk #4 succeeded at 210 (offset 30 lines).
Hunk #5 succeeded at 255 (offset 30 lines).
Hunk #6 FAILED at 264.
2 out of 6 hunks FAILED
^ permalink raw reply
* [v6] usb: gadget: udc: renesas_usb3: Add register of usb role switch
From: Felipe Balbi @ 2018-07-26 10:58 UTC (permalink / raw)
To: Yoshihiro Shimoda, robh+dt, mark.rutland
Cc: gregkh, heikki.krogerus, hdegoede, andy.shevchenko, linux-usb,
linux-renesas-soc, devicetree
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> writes:
> This patch adds role switch support for R-Car SoCs into the USB 3.0
> peripheral driver. Some R-Car SoCs (e.g. R-Car H3) have USB 3.0
> dual-role device controller which has the USB 3.0 xHCI host and
> Renesas USB 3.0 peripheral.
>
> Unfortunately, the mode change register (DRD_CON) contains
> the USB 3.0 peripheral controller side only. So, this renesas_usb3
> driver manages the DRD_CON now. However, in peripheral mode, the host
> should stop. Also the host hardware needs to reinitialize its own
> registers when the mode changes from peripheral to host mode.
> Otherwise, the host cannot work correctly (e.g. detect a device
> as high-speed).
>
> To achieve this reinitialization by a driver, this driver also
> registers a role switch driver to manage the DRD_CON and get
> a device pointer of usb 3.0 host from "companion" property of OF.
> Then, when the usb role is changed, renesas_usb3_role_switch_set()
> will attach/release the xhci-plat driver to reinitialize the host
> hardware.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
unfortunately doesn't apply:
checking file drivers/usb/gadget/udc/Kconfig
Hunk #1 FAILED at 193.
1 out of 1 hunk FAILED
checking file drivers/usb/gadget/udc/renesas_usb3.c
Hunk #7 succeeded at 2465 (offset -12 lines).
Hunk #8 succeeded at 2624 (offset -11 lines).
Hunk #9 succeeded at 2715 (offset -11 lines).
^ permalink raw reply
* Driver for MaxLinear/Exar USB (UART) Serial Adapters
From: Greg Kroah-Hartman @ 2018-07-26 10:57 UTC (permalink / raw)
To: Patong Yang; +Cc: linux-usb, johan, pyang
On Tue, Jul 24, 2018 at 03:36:36PM -0700, Patong Yang wrote:
> The original driver/patch was submitted on April 4, 2018. This is the
> second version based on the feedback received on the original patch.
>
> v2: Removed custom IOCTLs, as suggested by Greg KH
> Using standard Linux GPIO APIs, as suggested by Greg KH
> Removed file reads/writes as suggested by Greg KH
>
> Signed-off-by: Patong Yang <patong.mxl@gmail.com>
> ---
> drivers/usb/serial/xrusb_serial.c | 2380 +++++++++++++++++++++++++++++
> drivers/usb/serial/xrusb_serial.h | 234 +++
Why do you need a .h file for a single driver? Please just put it all
into one file.
But there is a bigger problem here:
> + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS);
> + if (!xrusb_tty_driver)
> + return -ENOMEM;
Why are you not using the usb serial core here? You need to do that,
not try to provide your own custom tty driver. That way userspace
programs will "just work" with your new device, no changes needed as
your major/minor number and device name would be custom only for your
device, which is not acceptable.
By doing that, your code will also be much smaller, always a good
benefit as well.
thanks,
greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [v3,1/3] usb: dwc3: pci: Add GPIO lookup table on platforms without ACPI GPIO resources
From: Felipe Balbi @ 2018-07-26 10:54 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman; +Cc: Andy Shevchenko, linux-usb
Hans de Goede <hdegoede@redhat.com> writes:
> Hi,
>
> On 10-06-18 16:01, Hans de Goede wrote:
>> Bay Trail / BYT SoCs do not have a builtin device-mode phy, instead
>> they require an external ULPI phy for device-mode.
>>
>> Only some BYT devices have an external phy, but even on those devices
>> device-mode is not working because the dwc3 does not see the phy.
>>
>> The problem is that the ACPI fwnode for the dwc3 does not contain the
>> expected GPIO resources for the GPIOs connected to the chip-select and
>> reset pins of the phy.
>>
>> I've found the workaround which some Android x86 kernels use for this:
>> https://github.com/BORETS24/Kernel-for-Asus-Zenfone-2/blob/master/arch/x86/platform/intel-mid/device_libs/pci/platform_usb_otg.c
>> Which boils down to hardcoding the GPIOs for these devices.
>>
>> The good news it that all boards (*) use the same GPIOs.
>>
>> This commit fixes the ULPI phy not woring by adding a gpiod_lookup_table
>> call which adds a hardcoded mapping for BYT devices. Note that the mapping
>> added by gpiod_add_lookup_table is a fallback mapping, so boards which
>> properly provide GPIO resources in the ACPI firmware-node resources
>> will not use this.
>>
>> *) Except for the first revision of the evalulation-kit, which normal users
>> don't have
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> -Add the mapping with gpiod_add_lookup_table() unconditionally on BYT
>> devices, as they are only used after checking for GPIO resources in ACPI
>
> Ping? AFAIK this series is ready for merging now and it is necessary to
> make gadget mode work on all Bay Trail devices which support gadget
> mode.
now queued for v4.19
^ permalink raw reply
* [v4,1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Felipe Balbi @ 2018-07-26 10:53 UTC (permalink / raw)
To: Marcus Folkesson, Greg Kroah-Hartman, Jonathan Corbet, davem,
Mauro Carvalho Chehab, Andrew Morton, Randy Dunlap,
Ruslan Bilovol, Thomas Gleixner, Kate Stewart
Cc: linux-usb, linux-doc, linux-kernel
Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> Chip Card Interface Device (CCID) protocol is a USB protocol that
> allows a smartcard device to be connected to a computer via a card
> reader using a standard USB interface, without the need for each manufacturer
> of smartcards to provide its own reader or protocol.
>
> This gadget driver makes Linux show up as a CCID device to the host and let a
> userspace daemon act as the smartcard.
>
> This is useful when the Linux gadget itself should act as a cryptographic
> device or forward APDUs to an embedded smartcard device.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
I still don't see the benefit of having this in the kernel when
functionfs would do just fine.
^ permalink raw reply
* [6/7] usb: gadget: f_uac2: disable IN/OUT ep if unused
From: Felipe Balbi @ 2018-07-26 10:51 UTC (permalink / raw)
To: Eugeniu Rosca, Ruslan Bilovol, Jassi Brar
Cc: Andrzej Pietrasiewicz, Daniel Mack, Robert Baldyga, Peter Chen,
Christoph Hellwig, Eugeniu Rosca, Vladimir Zapolskiy,
Joshua Frkuska, linux-usb, Andreas Pape
Eugeniu Rosca <erosca@de.adit-jv.com> writes:
> From: Andreas Pape <apape@de.adit-jv.com>
>
> Via p_chmask/c_chmask the user can define whether uac2 shall support
> playback and/or capture. This has only effect on the created ALSA device,
> but not on the USB descriptor. This patch adds playback/capture descriptors
> dependent on that parameter.
>
> Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
unfortunately doesn't apply:
checking file drivers/usb/gadget/function/f_uac2.c
Hunk #13 FAILED at 678.
1 out of 14 hunks FAILED
Please rebase on testing/next after a couple hours
^ permalink raw reply
* [v7] usb: gadget: uvc: configfs: Add bFrameIndex attributes
From: Felipe Balbi @ 2018-07-26 10:49 UTC (permalink / raw)
To: Laurent Pinchart, Joel Pepper; +Cc: Paul Elder, linux-usb
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> From: Joel Pepper <joel.pepper@rwth-aachen.de>
>
> - Add bFrameIndex as a UVCG_FRAME_ATTR_RO for each frame size.
> - Automatically assign ascending bFrameIndex to each frame in a format.
>
> Before all "bFrameindex" attributes were set to "1" with no way to
> configure the gadget otherwise. This resulted in the host always
> negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
> After the negotiation the host driver will set the user or application
> selected framesize, while the gadget is actually set to the first
> framesize.
>
> Now, when the containing format is linked into the streaming header,
> iterate over all child frame descriptors and assign ascending indices.
> The automatically assigned indices can be read from the new read only
> bFrameIndex configsfs attribute in each frame descriptor item.
>
> Signed-off-by: Joel Pepper <joel.pepper@rwth-aachen.de>
> [Simplified documentation, renamed function, blank space update]
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
please rebase on testing/next, but give me a couple hours to go through
the rest of my inbox first ;)
checking file Documentation/ABI/testing/configfs-usb-gadget-uvc
Hunk #1 succeeded at 177 (offset -4 lines).
Hunk #2 succeeded at 228 (offset -8 lines).
checking file drivers/usb/gadget/function/uvc_configfs.c
Hunk #1 succeeded at 720 (offset -121 lines).
Hunk #2 succeeded at 757 (offset -133 lines).
Hunk #3 succeeded at 996 (offset -137 lines).
Hunk #4 succeeded at 1179 (offset -137 lines).
Hunk #5 FAILED at 1395.
1 out of 5 hunks FAILED
^ permalink raw reply
* [1/3] usb: gadget: f_fs: Only return delayed status when len is 0
From: Felipe Balbi @ 2018-07-26 10:45 UTC (permalink / raw)
To: Jerry Zhang, Greg Kroah-Hartman, linux-usb
Cc: Michal Nazarewicz, Krzysztof Opasiak, Badhri Jagan Sridharan,
Andrzej Pietrasiewicz, Lars-Peter Clausen, felixhaedicke
Hi,
Jerry Zhang <zhangjerry@google.com> writes:
> Hi Felipe,
>
> I noticed this wasn't queued up for 4.18. Do you think there is
> anything I need to do to get this patch set into 4.19? Also, can we at
> least add just this patch ('usb: gadget: f_fs: Only return delayed
> status when len is 0') to 4.18 as functionfs control requests won't
> work without it?
patch 1 is now queued for next merge window. What should I do with
patches 2 and 3?
^ permalink raw reply
* usb: dwc2: gadget: ISOC's starting flow improvement
From: Felipe Balbi @ 2018-07-26 10:41 UTC (permalink / raw)
To: Minas Harutyunyan, Greg Kroah-Hartman; +Cc: John Youn
Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
> To start ISOC transfers in handlers dwc2_gadget_handle_nak() and
> dwc2_gadget_handle_out_token_ep_disabled() driver reads current frame
> number, based on which, set target frame number to start first ISOC
> transfer.
>
> In case if system's high IRQ latency and multiple EP's asserted
> interrupt in same frame, there are high probability that when reading
> current frame number in EP's handlers, actual frame number can be
> increased. As result for bInterval > 1, starting target frame
> will be set wrongly and all ISOC packets will be dropped.
>
> In patch "usb: dwc2: Change reading of current frame number flow"
> reading of current frame number done ASAP in common interrupt handler.
> This frame number stored in frame_number variable which used as
> starting frame number for ISOC EP's in above mentioned handlers.
>
> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
please rebase on top of testing/next after a couple hours:
checking file drivers/usb/dwc2/gadget.c
Hunk #1 FAILED at 2749.
Hunk #2 succeeded at 2772 (offset -1 lines).
Hunk #3 FAILED at 2811.
2 out of 3 hunks FAILED
^ permalink raw reply
* [v2,2/4] usb: dwc2: Modify dwc2_readl/writel functions prototype
From: Felipe Balbi @ 2018-07-26 10:39 UTC (permalink / raw)
To: Gevorg Sahakyan, Greg Kroah-Hartman, Minas Harutyunyan, linux-usb
Cc: John Youn
Gevorg Sahakyan <Gevorg.Sahakyan@synopsys.com> writes:
> Added hsotg argument to dwc2_readl/writel function prototype,
> and also instead of address pass offset of register.
> hsotg will contain flag field for endianness.
>
> Also customized dwc2_set_bit and dwc2_clear_bit function for
> dwc2_readl/writel functions.
>
> Signed-off-by: Gevorg Sahakyan <sahakyan@synopsys.com>
sorry for the delay. I've been super busy with an internal task.
Anyway, this doesn't apply to testing/next. Give me a couple hours for
me to go through the other patches, then I'll push testing/next so you
can rebase.
checking file drivers/usb/dwc2/core.c
checking file drivers/usb/dwc2/core.h
Hunk #1 succeeded at 1172 (offset 3 lines).
Hunk #2 succeeded at 1184 (offset 3 lines).
Hunk #3 succeeded at 1195 (offset 3 lines).
Hunk #4 succeeded at 1321 (offset 3 lines).
checking file drivers/usb/dwc2/core_intr.c
checking file drivers/usb/dwc2/debugfs.c
checking file drivers/usb/dwc2/gadget.c
Hunk #11 succeeded at 916 (offset 6 lines).
Hunk #12 succeeded at 968 (offset 6 lines).
Hunk #13 succeeded at 1065 (offset 6 lines).
Hunk #14 succeeded at 1079 (offset 6 lines).
Hunk #15 succeeded at 1105 (offset 6 lines).
Hunk #16 succeeded at 1128 (offset 6 lines).
Hunk #17 succeeded at 1467 (offset 6 lines).
Hunk #18 succeeded at 1481 (offset 6 lines).
Hunk #19 succeeded at 1635 (offset 6 lines).
Hunk #20 succeeded at 1774 (offset 6 lines).
Hunk #21 succeeded at 1826 (offset 6 lines).
Hunk #22 succeeded at 1956 (offset 6 lines).
Hunk #23 succeeded at 2125 (offset 6 lines).
Hunk #24 succeeded at 2139 (offset 6 lines).
Hunk #25 succeeded at 2169 (offset 6 lines).
Hunk #26 succeeded at 2199 (offset 6 lines).
Hunk #27 succeeded at 2248 (offset 6 lines).
Hunk #28 succeeded at 2344 (offset 6 lines).
Hunk #29 succeeded at 2375 (offset 6 lines).
Hunk #30 succeeded at 2393 (offset 6 lines).
Hunk #31 succeeded at 2447 (offset 6 lines).
Hunk #32 succeeded at 2472 (offset 6 lines).
Hunk #33 succeeded at 2496 (offset 6 lines).
Hunk #34 succeeded at 2550 (offset 6 lines).
Hunk #35 succeeded at 2654 (offset 6 lines).
Hunk #36 succeeded at 2684 (offset 6 lines).
Hunk #37 succeeded at 2699 (offset 6 lines).
Hunk #38 succeeded at 2775 (offset 4 lines).
Hunk #39 succeeded at 2829 (offset 1 line).
Hunk #40 succeeded at 2866 (offset 1 line).
Hunk #41 succeeded at 2897 (offset 1 line).
Hunk #42 succeeded at 3016 (offset 1 line).
Hunk #43 succeeded at 3087 (offset 1 line).
Hunk #44 succeeded at 3115 (offset 1 line).
Hunk #45 succeeded at 3216 (offset 1 line).
Hunk #46 succeeded at 3231 (offset 1 line).
Hunk #47 succeeded at 3257 (offset 1 line).
Hunk #48 succeeded at 3277 (offset 1 line).
Hunk #49 succeeded at 3301 (offset 1 line).
Hunk #50 succeeded at 3345 (offset 1 line).
Hunk #51 succeeded at 3358 (offset 1 line).
Hunk #52 succeeded at 3387 (offset 1 line).
Hunk #53 succeeded at 3425 (offset 1 line).
Hunk #54 succeeded at 3433 (offset 1 line).
Hunk #55 succeeded at 3470 (offset 1 line).
Hunk #56 succeeded at 3479 (offset 1 line).
Hunk #57 succeeded at 3516 (offset 1 line).
Hunk #58 succeeded at 3527 (offset 1 line).
Hunk #59 succeeded at 3537 (offset 1 line).
Hunk #60 succeeded at 3623 (offset 1 line).
Hunk #61 succeeded at 3639 (offset 1 line).
Hunk #62 succeeded at 3653 (offset 1 line).
Hunk #63 succeeded at 3668 (offset 1 line).
Hunk #64 succeeded at 3708 (offset 1 line).
Hunk #65 succeeded at 3716 (offset 1 line).
Hunk #66 succeeded at 3725 (offset 1 line).
Hunk #67 succeeded at 3736 (offset 1 line).
Hunk #68 succeeded at 3744 (offset 1 line).
Hunk #69 succeeded at 3759 (offset 1 line).
Hunk #70 succeeded at 3831 (offset 1 line).
Hunk #71 succeeded at 3879 (offset 1 line).
Hunk #72 succeeded at 3920 (offset 1 line).
Hunk #73 succeeded at 3958 (offset 1 line).
Hunk #74 succeeded at 3970 (offset 1 line).
Hunk #75 succeeded at 4021 (offset 1 line).
Hunk #76 succeeded at 4031 (offset 1 line).
Hunk #77 succeeded at 4138 (offset 1 line).
Hunk #78 succeeded at 4151 (offset 1 line).
Hunk #79 succeeded at 4165 (offset 1 line).
Hunk #80 succeeded at 4213 (offset 1 line).
Hunk #81 succeeded at 4243 (offset 1 line).
Hunk #82 succeeded at 4536 (offset 1 line).
Hunk #83 succeeded at 4607 (offset 1 line).
Hunk #84 succeeded at 4632 (offset 1 line).
Hunk #85 succeeded at 4834 (offset 4 lines).
Hunk #86 succeeded at 4850 (offset 4 lines).
Hunk #87 succeeded at 4862 (offset 4 lines).
Hunk #88 succeeded at 4897 (offset 4 lines).
Hunk #89 succeeded at 4916 (offset 4 lines).
Hunk #90 succeeded at 4928 (offset 4 lines).
Hunk #91 succeeded at 4953 (offset 4 lines).
Hunk #92 succeeded at 4987 (offset 4 lines).
Hunk #93 succeeded at 5062 (offset 4 lines).
Hunk #94 succeeded at 5121 (offset 4 lines).
checking file drivers/usb/dwc2/hcd.c
Hunk #40 FAILED at 1566.
Hunk #41 succeeded at 1584 (offset 9 lines).
Hunk #42 succeeded at 1609 (offset 9 lines).
Hunk #43 succeeded at 1667 (offset 9 lines).
Hunk #44 succeeded at 1697 (offset 9 lines).
Hunk #45 succeeded at 1754 (offset 9 lines).
Hunk #46 succeeded at 1762 (offset 9 lines).
Hunk #47 succeeded at 1772 (offset 9 lines).
Hunk #48 succeeded at 1886 (offset 9 lines).
Hunk #49 succeeded at 1907 (offset 9 lines).
Hunk #50 succeeded at 1920 (offset 9 lines).
Hunk #51 succeeded at 1984 (offset 9 lines).
Hunk #52 succeeded at 1998 (offset 9 lines).
Hunk #53 succeeded at 2026 (offset 9 lines).
Hunk #54 succeeded at 2070 (offset 9 lines).
Hunk #55 succeeded at 2094 (offset 9 lines).
Hunk #56 succeeded at 2113 (offset 9 lines).
Hunk #57 succeeded at 2278 (offset 9 lines).
Hunk #58 succeeded at 2290 (offset 9 lines).
Hunk #59 succeeded at 2324 (offset 9 lines).
Hunk #60 succeeded at 2373 (offset 9 lines).
Hunk #61 succeeded at 2395 (offset 9 lines).
Hunk #62 succeeded at 2414 (offset 9 lines).
Hunk #63 succeeded at 2425 (offset 9 lines).
Hunk #64 succeeded at 2445 (offset 9 lines).
Hunk #65 succeeded at 2481 (offset 9 lines).
Hunk #66 succeeded at 3075 (offset 64 lines).
Hunk #67 succeeded at 3090 (offset 64 lines).
Hunk #68 succeeded at 3160 (offset 64 lines).
Hunk #69 succeeded at 3173 (offset 64 lines).
Hunk #70 succeeded at 3205 (offset 64 lines).
Hunk #71 succeeded at 3228 (offset 64 lines).
Hunk #72 succeeded at 3265 (offset 64 lines).
Hunk #73 succeeded at 3285 (offset 64 lines).
Hunk #74 succeeded at 3296 (offset 64 lines).
Hunk #75 succeeded at 3335 (offset 64 lines).
Hunk #76 succeeded at 3353 (offset 64 lines).
Hunk #77 succeeded at 3379 (offset 64 lines).
Hunk #78 succeeded at 3439 (offset 64 lines).
Hunk #79 succeeded at 3470 (offset 64 lines).
Hunk #80 succeeded at 3488 (offset 64 lines).
Hunk #81 succeeded at 3522 (offset 64 lines).
Hunk #82 succeeded at 3533 (offset 64 lines).
Hunk #83 succeeded at 3541 (offset 64 lines).
Hunk #84 succeeded at 3585 (offset 64 lines).
Hunk #85 succeeded at 3605 (offset 64 lines).
Hunk #86 succeeded at 3726 (offset 64 lines).
Hunk #87 succeeded at 3767 (offset 64 lines).
Hunk #88 succeeded at 3816 (offset 64 lines).
Hunk #89 succeeded at 3826 (offset 64 lines).
Hunk #90 succeeded at 3845 (offset 64 lines).
Hunk #91 succeeded at 3867 (offset 64 lines).
Hunk #92 succeeded at 3924 (offset 64 lines).
Hunk #93 succeeded at 3935 (offset 64 lines).
Hunk #94 succeeded at 4056 (offset 64 lines).
Hunk #95 succeeded at 4108 (offset 64 lines).
Hunk #96 succeeded at 4363 (offset 64 lines).
Hunk #97 succeeded at 4473 (offset 64 lines).
Hunk #98 succeeded at 4564 (offset 64 lines).
Hunk #99 succeeded at 5085 (offset 64 lines).
Hunk #100 succeeded at 5138 (offset 64 lines).
Hunk #101 succeeded at 5428 (offset 80 lines).
Hunk #102 succeeded at 5464 (offset 80 lines).
Hunk #103 succeeded at 5507 with fuzz 1 (offset 80 lines).
Hunk #104 succeeded at 5523 (offset 80 lines).
Hunk #105 succeeded at 5620 (offset 80 lines).
Hunk #106 succeeded at 5651 (offset 80 lines).
1 out of 106 hunks FAILED
checking file drivers/usb/dwc2/hcd.h
Hunk #1 succeeded at 469 (offset 8 lines).
Hunk #2 succeeded at 487 (offset 8 lines).
Hunk #3 succeeded at 690 (offset 8 lines).
checking file drivers/usb/dwc2/hcd_ddma.c
checking file drivers/usb/dwc2/hcd_intr.c
Hunk #20 succeeded at 959 (offset 7 lines).
Hunk #21 succeeded at 1185 (offset 7 lines).
Hunk #22 succeeded at 1561 (offset 7 lines).
Hunk #23 succeeded at 1776 (offset 7 lines).
Hunk #24 succeeded at 1803 (offset 7 lines).
Hunk #25 succeeded at 1863 (offset 7 lines).
Hunk #26 succeeded at 1958 (offset 7 lines).
Hunk #27 succeeded at 2031 (offset 7 lines).
Hunk #28 succeeded at 2047 (offset 7 lines).
Hunk #29 succeeded at 2182 (offset 7 lines).
Hunk #30 succeeded at 2266 (offset 7 lines).
checking file drivers/usb/dwc2/hcd_queue.c
Hunk #2 succeeded at 1747 (offset 3 lines).
Hunk #3 succeeded at 1788 (offset 3 lines).
checking file drivers/usb/dwc2/params.c
^ permalink raw reply
* usbip: Fix misuse of strncpy()
From: Ben Hutchings @ 2018-07-26 10:39 UTC (permalink / raw)
To: Shuah Khan, Valentina Manea; +Cc: linux-usb
On Tue, 2018-07-24 at 11:04 -0600, Shuah Khan wrote:
> On 07/20/2018 08:12 PM, Ben Hutchings wrote:
> > gcc 8 reports:
> >
> > usbip_device_driver.c: In function ‘read_usb_vudc_device’:
> > usbip_device_driver.c:106:2: error: ‘strncpy’ specified bound 256 equals destination size [-Werror=stringop-truncation]
> > strncpy(dev->path, path, SYSFS_PATH_MAX);
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > usbip_device_driver.c:125:2: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
> > strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > I'm not convinced it makes sense to truncate the copied strings here,
> > but since we're already doing so let's ensure they're still null-
> > terminated. We can't easily use strlcpy() here, so use snprintf().
> >
> > usbip_common.c has the same problem.
> >
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Cc: stable@vger.kernel.org
> > ---
> > tools/usb/usbip/libsrc/usbip_common.c | 4 ++--
> > tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > --- a/tools/usb/usbip/libsrc/usbip_common.c
> > +++ b/tools/usb/usbip/libsrc/usbip_common.c
> > @@ -226,8 +226,8 @@ int read_usb_device(struct udev_device *
> > path = udev_device_get_syspath(sdev);
> > name = udev_device_get_sysname(sdev);
> >
> > - strncpy(udev->path, path, SYSFS_PATH_MAX);
> > - strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
> > + snprintf(udev->path, SYSFS_PATH_MAX, "%s", path);
> > + snprintf(udev->busid, SYSFS_BUS_ID_SIZE, "%s", name);
>
> I am okay with the change to use snprintf(). Please add check for
> return to avoid GCC 7 -Wformat-overflow wanrs on snprintf instances
> that don't check return.
[...]
I've tried running:
./autogen.sh
CC=gcc-7 CFLAGS=-Wformat-overflow ./configure
make
but this didn't produce any warnings. How did you get the warning?
Ben.
^ permalink raw reply
* [2/6] module: add support for symbol namespaces.
From: Martijn Coenen @ 2018-07-26 7:44 UTC (permalink / raw)
To: Lucas De Marchi
Cc: Jessica Yu, lkml, Masahiro Yamada, Michal Marek,
Geert Uytterhoeven, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
the arch/x86 maintainers, Alan Stern, Greg KH, Oliver Neukum,
Arnd Bergmann, Stephen Boyd, Philippe Ombredanne, Kate Stewart,
Sam Ravnborg, Linux Kbuild mailing list, linux-m68k, USB list,
USB Storage list, linux-scsi, Linux-Arch, Martijn Coenen,
Sandeep Patil, Iliyan Malchev, Joel Fernandes, linux-modules
On Wed, Jul 25, 2018 at 6:48 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
> this might be because you are looking to the wrong project
> (module-init-tools) rather than kmod that replaced it some years ago?
Oh yes indeed, thanks for pointing that out :-)
>> Just scanning the depmod source code, it seems really trivial to just
>> have depmod accommodate the new symbol name format. Adding Lucas (kmod
>> maintainer) to CC.
>
> Yep, that would be easy and allow depmod to be backward compatible
> since we would do nothing if the symbol doesn't have the suffix.
> As for dependency on a new version, this seems trivial enough to be
> backported to previous releases used on distros so even if they are
> not rolling they would get a compatible depmod.
Thanks for the suggestion. I had also considered modifying the script
that generates System.map to strip out the namespace; but that seems a
bit hacky since it will then mismatch with the actual symbol table
that's in the kernel image. The situation with depmod has gotten me a
bit worried that perhaps there are more tools that look at System.map
or the kernel's symbol table that won't be able to understand the new
format. But of course those could be updated, too.
Another alternative I was considering is leaving the namespace out of
the __ksymtab symbols, and instead for the symbols that have a
namespace, have an entry in __ksymtab (without namespace) and an entry
in a new section __ksymtab_ns (with namespace). We can then discard
__ksymtab_ns when building the final executable/modules, since that
information is not needed at runtime (it's encoded in struct
kernel_symbol). What makes this a bit more complex is that modpost
takes the fully linked version of vmlinux which would already have
__ksymtab_ns stripped, so we'd need to use objcopy to copy out that
section so modpost can use it to read vmlinux's namespaced symbols.
Modules wouldn't have that problem, since modpost processes modules
before their final linking step.
This last approach is more complex than just modifying depmod, but it
does have the benefit that we won't be break userspace tools depending
on the kernel __ksymtab section symbols. On the other hand, perhaps
userspace tools would like to have the namespace information at some
point.
For now I'll keep things as is and write up a patch for depmod, but if
others prefer the __ksymtab_ns approach I described above I don't mind
making that work.
Thanks,
Martijn
>
> CC'ing linux-modules@vger.kernel.org to keep track of this there
>
>
> thanks
> Lucas De Marchi
>
>>
>> Thanks,
>>
>> Jessica
>>
>> >
>> >>
>> >> On x86_64 I saw no difference in binary size (compression), but at
>> >> runtime this will require a word of memory per export to hold the
>> >> namespace. An alternative could be to store namespaced symbols in their
>> >> own section and use a separate 'struct namespaced_kernel_symbol' for
>> >> that section, at the cost of making the module loader more complex.
>> >>
>> >> Signed-off-by: Martijn Coenen <maco@android.com>
>> >> ---
>> >> include/asm-generic/export.h | 2 +-
>> >> include/linux/export.h | 83 +++++++++++++++++++++++++++---------
>> >> include/linux/module.h | 13 ++++++
>> >> kernel/module.c | 79 ++++++++++++++++++++++++++++++++++
>> >> 4 files changed, 156 insertions(+), 21 deletions(-)
>> >>
>> >> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
>> >> index 68efb950a918..4c3d1afb702f 100644
>> >> --- a/include/asm-generic/export.h
>> >> +++ b/include/asm-generic/export.h
>> >> @@ -29,7 +29,7 @@
>> >> .section ___ksymtab\sec+\name,"a"
>> >> .balign KSYM_ALIGN
>> >> __ksymtab_\name:
>> >> - __put \val, __kstrtab_\name
>> >> + __put \val, __kstrtab_\name, 0
>> >> .previous
>> >> .section __ksymtab_strings,"a"
>> >> __kstrtab_\name:
>> >> diff --git a/include/linux/export.h b/include/linux/export.h
>> >> index ad6b8e697b27..9f6e70eeb85f 100644
>> >> --- a/include/linux/export.h
>> >> +++ b/include/linux/export.h
>> >> @@ -22,6 +22,11 @@ struct kernel_symbol
>> >> {
>> >> unsigned long value;
>> >> const char *name;
>> >> + const char *namespace;
>> >> +};
>> >> +
>> >> +struct namespace_import {
>> >> + const char *namespace;
>> >> };
>> >>
>> >> #ifdef MODULE
>> >> @@ -54,18 +59,28 @@ extern struct module __this_module;
>> >> #define __CRC_SYMBOL(sym, sec)
>> >> #endif
>> >>
>> >> +#define NS_SEPARATOR "."
>> >> +
>> >> +#define MODULE_IMPORT_NS(ns) \
>> >> + static const struct namespace_import __knsimport_##ns \
>> >> + asm("__knsimport_" #ns) \
>> >> + __attribute__((section("__knsimport"), used)) \
>> >> + = { #ns }
>> >> +
>> >> /* For every exported symbol, place a struct in the __ksymtab section */
>> >> -#define ___EXPORT_SYMBOL(sym, sec) \
>> >> +#define ___EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) \
>> >> extern typeof(sym) sym; \
>> >> __CRC_SYMBOL(sym, sec) \
>> >> - static const char __kstrtab_##sym[] \
>> >> + static const char __kstrtab_##sym##nspost[] \
>> >> __attribute__((section("__ksymtab_strings"), aligned(1))) \
>> >> = #sym; \
>> >> - static const struct kernel_symbol __ksymtab_##sym \
>> >> + static const struct kernel_symbol __ksymtab_##sym##nspost \
>> >> + asm("__ksymtab_" #sym nspost2) \
>> >> __used \
>> >> - __attribute__((section("___ksymtab" sec "+" #sym), used)) \
>> >> + __attribute__((section("___ksymtab" sec "+" #sym #nspost))) \
>> >> + __attribute__((used)) \
>> >> __attribute__((aligned(sizeof(void *)))) \
>> >> - = { (unsigned long)&sym, __kstrtab_##sym }
>> >> + = { (unsigned long)&sym, __kstrtab_##sym##nspost, ns }
>> >>
>> >> #if defined(__KSYM_DEPS__)
>> >>
>> >> @@ -76,52 +91,80 @@ extern struct module __this_module;
>> >> * system filters out from the preprocessor output (see ksym_dep_filter
>> >> * in scripts/Kbuild.include).
>> >> */
>> >> -#define __EXPORT_SYMBOL(sym, sec) === __KSYM_##sym ===
>> >> +#define __EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) === __KSYM_##sym ===
>> >>
>> >> #elif defined(CONFIG_TRIM_UNUSED_KSYMS)
>> >>
>> >> #include <generated/autoksyms.h>
>> >>
>> >> -#define __EXPORT_SYMBOL(sym, sec) \
>> >> - __cond_export_sym(sym, sec, __is_defined(__KSYM_##sym))
>> >> -#define __cond_export_sym(sym, sec, conf) \
>> >> - ___cond_export_sym(sym, sec, conf)
>> >> -#define ___cond_export_sym(sym, sec, enabled) \
>> >> - __cond_export_sym_##enabled(sym, sec)
>> >> -#define __cond_export_sym_1(sym, sec) ___EXPORT_SYMBOL(sym, sec)
>> >> -#define __cond_export_sym_0(sym, sec) /* nothing */
>> >> +#define __EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) \
>> >> + __cond_export_sym(sym, sec, ns, nspost, nspost2, \
>> >> + __is_defined(__KSYM_##sym))
>> >> +#define __cond_export_sym(sym, sec, ns, nspost, nspost2, conf) \
>> >> + ___cond_export_sym(sym, sec, ns, nspost, nspost2, conf)
>> >> +#define ___cond_export_sym(sym, sec, ns, nspost, nspost2, enabled) \
>> >> + __cond_export_sym_##enabled(sym, sec, ns, nspost, nspost2)
>> >> +#define __cond_export_sym_1(sym, sec, ns, nspost, nspost2) \
>> >> + ___EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2)
>> >> +#define __cond_export_sym_0(sym, sec, ns, nspost, nspost2) /* nothing */
>> >>
>> >> #else
>> >> #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
>> >> #endif
>> >>
>> >> #define EXPORT_SYMBOL(sym) \
>> >> - __EXPORT_SYMBOL(sym, "")
>> >> + __EXPORT_SYMBOL(sym, "", NULL, ,)
>> >>
>> >> #define EXPORT_SYMBOL_GPL(sym) \
>> >> - __EXPORT_SYMBOL(sym, "_gpl")
>> >> + __EXPORT_SYMBOL(sym, "_gpl", NULL, ,)
>> >>
>> >> #define EXPORT_SYMBOL_GPL_FUTURE(sym) \
>> >> - __EXPORT_SYMBOL(sym, "_gpl_future")
>> >> + __EXPORT_SYMBOL(sym, "_gpl_future", NULL, ,)
>> >> +
>> >> +#define EXPORT_SYMBOL_NS(sym, ns) \
>> >> + __EXPORT_SYMBOL(sym, "", #ns, __##ns, NS_SEPARATOR #ns)
>> >> +
>> >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) \
>> >> + __EXPORT_SYMBOL(sym, "_gpl", #ns, __##ns, NS_SEPARATOR #ns)
>> >>
>> >> #ifdef CONFIG_UNUSED_SYMBOLS
>> >> -#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused")
>> >> -#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl")
>> >> +#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused", , ,)
>> >> +#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl", , ,)
>> >> #else
>> >> #define EXPORT_UNUSED_SYMBOL(sym)
>> >> #define EXPORT_UNUSED_SYMBOL_GPL(sym)
>> >> #endif
>> >>
>> >> -#endif /* __GENKSYMS__ */
>> >> +#endif /* __KERNEL__ && !__GENKSYMS__ */
>> >> +
>> >> +#if defined(__GENKSYMS__)
>> >> +/*
>> >> + * When we're running genksyms, ignore the namespace and make the _NS
>> >> + * variants look like the normal ones. There are two reasons for this:
>> >> + * 1) In the normal definition of EXPORT_SYMBOL_NS, the 'ns' macro
>> >> + * argument is itself not expanded because it's always tokenized or
>> >> + * concatenated; but when running genksyms, a blank definition of the
>> >> + * macro does allow the argument to be expanded; if a namespace
>> >> + * happens to collide with a #define, this can cause issues.
>> >> + * 2) There's no need to modify genksyms to deal with the _NS variants
>> >> + */
>> >> +#define EXPORT_SYMBOL_NS(sym, ns) \
>> >> + EXPORT_SYMBOL(sym)
>> >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) \
>> >> + EXPORT_SYMBOL_GPL(sym)
>> >> +#endif
>> >>
>> >> #else /* !CONFIG_MODULES... */
>> >>
>> >> #define EXPORT_SYMBOL(sym)
>> >> +#define EXPORT_SYMBOL_NS(sym, ns)
>> >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns)
>> >> #define EXPORT_SYMBOL_GPL(sym)
>> >> #define EXPORT_SYMBOL_GPL_FUTURE(sym)
>> >> #define EXPORT_UNUSED_SYMBOL(sym)
>> >> #define EXPORT_UNUSED_SYMBOL_GPL(sym)
>> >>
>> >> +#define MODULE_IMPORT_NS(ns)
>> >> #endif /* CONFIG_MODULES */
>> >> #endif /* !__ASSEMBLY__ */
>> >>
>> >> diff --git a/include/linux/module.h b/include/linux/module.h
>> >> index d44df9b2c131..afab4e8fa188 100644
>> >> --- a/include/linux/module.h
>> >> +++ b/include/linux/module.h
>> >> @@ -268,6 +268,12 @@ void *__symbol_get(const char *symbol);
>> >> void *__symbol_get_gpl(const char *symbol);
>> >> #define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x))))
>> >>
>> >> +/* namespace dependencies of the module */
>> >> +struct module_ns_dep {
>> >> + struct list_head ns_dep;
>> >> + const char *namespace;
>> >> +};
>> >> +
>> >> /* modules using other modules: kdb wants to see this. */
>> >> struct module_use {
>> >> struct list_head source_list;
>> >> @@ -359,6 +365,13 @@ struct module {
>> >> const struct kernel_symbol *gpl_syms;
>> >> const s32 *gpl_crcs;
>> >>
>> >> + /* Namespace imports */
>> >> + unsigned int num_ns_imports;
>> >> + const struct namespace_import *ns_imports;
>> >> +
>> >> + /* Namespace dependencies */
>> >> + struct list_head ns_dependencies;
>> >> +
>> >> #ifdef CONFIG_UNUSED_SYMBOLS
>> >> /* unused exported symbols. */
>> >> const struct kernel_symbol *unused_syms;
>> >> diff --git a/kernel/module.c b/kernel/module.c
>> >> index f475f30eed8c..63de0fe849f9 100644
>> >> --- a/kernel/module.c
>> >> +++ b/kernel/module.c
>> >> @@ -1166,6 +1166,51 @@ static inline int module_unload_init(struct module *mod)
>> >> }
>> >> #endif /* CONFIG_MODULE_UNLOAD */
>> >>
>> >> +static bool module_has_ns_dependency(struct module *mod, const char *ns)
>> >> +{
>> >> + struct module_ns_dep *ns_dep;
>> >> +
>> >> + list_for_each_entry(ns_dep, &mod->ns_dependencies, ns_dep) {
>> >> + if (strcmp(ns_dep->namespace, ns) == 0)
>> >> + return true;
>> >> + }
>> >> +
>> >> + return false;
>> >> +}
>> >> +
>> >> +static int add_module_ns_dependency(struct module *mod, const char *ns)
>> >> +{
>> >> + struct module_ns_dep *ns_dep;
>> >> +
>> >> + if (module_has_ns_dependency(mod, ns))
>> >> + return 0;
>> >> +
>> >> + ns_dep = kmalloc(sizeof(*ns_dep), GFP_ATOMIC);
>> >> + if (!ns_dep)
>> >> + return -ENOMEM;
>> >> +
>> >> + ns_dep->namespace = ns;
>> >> +
>> >> + list_add(&ns_dep->ns_dep, &mod->ns_dependencies);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static bool module_imports_ns(struct module *mod, const char *ns)
>> >> +{
>> >> + size_t i;
>> >> +
>> >> + if (!ns)
>> >> + return true;
>> >> +
>> >> + for (i = 0; i < mod->num_ns_imports; ++i) {
>> >> + if (!strcmp(mod->ns_imports[i].namespace, ns))
>> >> + return true;
>> >> + }
>> >> +
>> >> + return false;
>> >> +}
>> >> +
>> >> static size_t module_flags_taint(struct module *mod, char *buf)
>> >> {
>> >> size_t l = 0;
>> >> @@ -1415,6 +1460,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
>> >> goto getname;
>> >> }
>> >>
>> >> + /*
>> >> + * We can't yet verify that the module actually imports this
>> >> + * namespace, because the imports themselves are only available
>> >> + * after processing the symbol table and doing relocation; so
>> >> + * instead just record the dependency and check later.
>> >> + */
>> >> + if (sym->namespace) {
>> >> + err = add_module_ns_dependency(mod, sym->namespace);
>> >> + if (err)
>> >> + sym = ERR_PTR(err);
>> >> + }
>> >> +
>> >> getname:
>> >> /* We must make copy under the lock if we failed to get ref. */
>> >> strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
>> >> @@ -3061,6 +3118,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>> >> sizeof(*mod->gpl_syms),
>> >> &mod->num_gpl_syms);
>> >> mod->gpl_crcs = section_addr(info, "__kcrctab_gpl");
>> >> +
>> >> + mod->ns_imports = section_objs(info, "__knsimport",
>> >> + sizeof(*mod->ns_imports),
>> >> + &mod->num_ns_imports);
>> >> +
>> >> mod->gpl_future_syms = section_objs(info,
>> >> "__ksymtab_gpl_future",
>> >> sizeof(*mod->gpl_future_syms),
>> >> @@ -3381,6 +3443,19 @@ static int post_relocation(struct module *mod, const struct load_info *info)
>> >> return module_finalize(info->hdr, info->sechdrs, mod);
>> >> }
>> >>
>> >> +static void verify_namespace_dependencies(struct module *mod)
>> >> +{
>> >> + struct module_ns_dep *ns_dep;
>> >> +
>> >> + list_for_each_entry(ns_dep, &mod->ns_dependencies, ns_dep) {
>> >> + if (!module_imports_ns(mod, ns_dep->namespace)) {
>> >> + pr_warn("%s: module uses symbols from namespace %s,"
>> >> + " but does not import it.\n",
>> >> + mod->name, ns_dep->namespace);
>> >> + }
>> >> + }
>> >> +}
>> >> +
>> >> /* Is this module of this name done loading? No locks held. */
>> >> static bool finished_loading(const char *name)
>> >> {
>> >> @@ -3682,6 +3757,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>> >> if (err)
>> >> goto free_module;
>> >>
>> >> + INIT_LIST_HEAD(&mod->ns_dependencies);
>> >> +
>> >> #ifdef CONFIG_MODULE_SIG
>> >> mod->sig_ok = info->sig_ok;
>> >> if (!mod->sig_ok) {
>> >> @@ -3730,6 +3807,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>> >> if (err < 0)
>> >> goto free_modinfo;
>> >>
>> >> + verify_namespace_dependencies(mod);
>> >> +
>> >> flush_module_icache(mod);
>> >>
>> >> /* Now copy in args */
>> >> --
>> >> 2.18.0.203.gfac676dfb9-goog
>> >>
>
>
>
> --
> Lucas De Marchi
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [4/5] memstick: rtsx_usb_ms: Support runtime power management
From: Kai-Heng Feng @ 2018-07-26 7:38 UTC (permalink / raw)
To: Alan Stern
Cc: arnd, gregkh, ulf.hansson, bauer.chen, ricky_wu, linux-kernel,
linux-usb
> On 2018Jul26, at 01:59, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 25 Jul 2018, Kai-Heng Feng wrote:
>
>> In order to let host's parent device, rtsx_usb, to use USB remote wake
>> up signaling to do card detection, it needs to be suspended. Hence it's
>> necessary to add runtime PM support for the memstick host.
>>
>> To keep memstick host stays suspended when it's not in use, convert the
>> card detection function from kthread to delayed_work, which can be
>> scheduled when the host is resumed and can be canceled when the host is
>> suspended.
>>
>> Use an idle function check if there's no card and the power mode is
>> MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> I'm not familiar enough with this driver to comment on specific
> instances, but you should carefully go through the code and make sure
> that you use pm_runtime_get_noresume() and pm_runtime_put_noidle()
> correctly.
>
> The basic idea is to use these when you know beforehand that the device
> is already at full power (for _get_noresume) and the usage count will
> not go to 0 (for _put_noidle). If you aren't certain of these
> requirements then you should call pm_runtime_get_sync() and
> pm_runtime_put().
There are two different uses for pm_runtime helpers.
The first use case is to prevent parent device, rtsx_usb, from suspending:
pm_runtime_get_noresume()
rtsx_usb_read_register()
pm_runtime_put_noidle()
The second use case, to manage memstick host itself.
memstick_detect_change() powers the host on if there’s a card in slot, and it powers host off when there’s no card.
Once memstick_detect_change() is done, we can check if the host can be suspended.
So we are sure the timing when the power is on or off.
Kai-Heng
>
> Alan Stern
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [2/5] memstick: Prevent memstick host from getting runtime suspended during card detection
From: Kai-Heng Feng @ 2018-07-26 7:01 UTC (permalink / raw)
To: Alan Stern
Cc: arnd, gregkh, ulf.hansson, bauer.chen, ricky_wu, linux-kernel,
linux-usb
> On 2018Jul26, at 01:56, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 25 Jul 2018, Kai-Heng Feng wrote:
>
>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>> helpers to let memstick host support runtime pm.
>>
>> There's a small window between memstick_detect_change() and its queued
>> work, memstick_check(). In this window the rpm count may go down to zero
>> before the memstick host powers on, so the host can be inadvertently
>> suspended.
>>
>> Increment rpm count before calling memstick_check(), and decrement rpm
>> count afterward, as now we are sure the memstick host should be
>> suspended or not.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/memstick/core/memstick.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>> index 76382c858c35..944fe0c93fa7 100644
>> --- a/drivers/memstick/core/memstick.c
>> +++ b/drivers/memstick/core/memstick.c
>> @@ -18,6 +18,7 @@
>> #include <linux/delay.h>
>> #include <linux/slab.h>
>> #include <linux/module.h>
>> +#include <linux/pm_runtime.h>
>>
>> #define DRIVER_NAME "memstick"
>>
>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>> */
>> void memstick_detect_change(struct memstick_host *host)
>> {
>> + pm_runtime_get_noresume(host->dev.parent);
>> queue_work(workqueue, &host->media_checker);
>> }
>> EXPORT_SYMBOL(memstick_detect_change);
>> @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work)
>> host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF);
>>
>> mutex_unlock(&host->lock);
>> +
>> + pm_runtime_put_noidle(host->dev.parent);
>> dev_dbg(&host->dev, "memstick_check finished\n");
>> }
>
> This should be pm_runtime_put(), not _put_noidle(). The reason is
> because if this call causes the PM runtime usage count to drop to 0,
> you do want the device to go into runtime suspend.
Will update in next version, thanks!
Kai-Heng
>
> Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* usb: dwc3: core: Add quirk for enabling AutoRetry feature in the controller
From: Anurag Kumar Vulisha @ 2018-07-26 2:35 UTC (permalink / raw)
To: Rob Herring
Cc: gregkh@linuxfoundation.org, mark.rutland@arm.com,
balbi@kernel.org, v.anuragkumar@gmail.com,
linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Hi Rob,
Thanks for your comments, please see my comments inline
>-----Original Message-----
>From: Rob Herring [mailto:robh@kernel.org]
>Sent: Thursday, July 26, 2018 1:33 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>
>Cc: gregkh@linuxfoundation.org; mark.rutland@arm.com; balbi@kernel.org;
>v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; devicetree@vger.kernel.org;
>linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] usb: dwc3: core: Add quirk for enabling AutoRetry feature in the
>controller
>
>On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote:
>> By default when core sees any transaction error(CRC or overflow)
>> it replies with terminating retry ACK (Retry=1 and Nump == 0).
>> Enabling this Auto Retry feature in controller, on seeing any
>> transaction errors makes the core to send an non-terminating ACK
>> transaction packet (that is, ACK TP with Retry=1 and Nump != 0).
>> Doing so will give controller a chance to recover from the error
>> condition.
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>> ---
>> Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++
>> drivers/usb/dwc3/core.c | 16 ++++++++++++++++
>> drivers/usb/dwc3/core.h | 6 ++++++
>> 3 files changed, 27 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 7f13ebe..2ba2bc2 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -94,6 +94,11 @@ Optional properties:
>> this and tx-thr-num-pkt-prd to a valid, non-zero value
>> 1-16 (DWC_usb31 programming guide section 1.2.3) to
>> enable periodic ESS TX threshold.
>> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the
>
>s/_/-/
Sorry, I am not able to understand what needs to fixed here. Please help me in
understanding, so that I can fix it in v2.
>
>> + controller operating in Host mode on seeing transaction
>> + errors(CRC errors or internal overrun scenerios) on IN
>> + transfers to reply to the device with a non-terminating
>> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0)
>
>Seems like the property is unnecessary. When would you not want this
>retry behavior? Why not just enable unconditionally in the driver?
>
There is no harm in adding this fix always but I think this Retry feature should be added
depending on the user and type of the application. For example, applying this feature in
a streaming application where isochronous transfers are used might end up in retrying
in failed packet until timeout happens, this might reduce the performance. And there may
be cases where the user want to know when the transfer error occurred. Enabling this feature
always will fail in both the above mentioned cases. Because of this reason added this quirk,
so that user can decide when to apply and not. Please correct me if I am wrong
Thanks,
Anurag Kumar Vulisha
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* usb: dwc3: core: Add quirk for enabling AutoRetry feature in the controller
From: Rob Herring @ 2018-07-25 20:03 UTC (permalink / raw)
To: Anurag Kumar Vulisha
Cc: gregkh, mark.rutland, balbi, v.anuragkumar, linux-usb, devicetree,
linux-kernel
On Sat, Jul 21, 2018 at 03:58:40PM +0530, Anurag Kumar Vulisha wrote:
> By default when core sees any transaction error(CRC or overflow)
> it replies with terminating retry ACK (Retry=1 and Nump == 0).
> Enabling this Auto Retry feature in controller, on seeing any
> transaction errors makes the core to send an non-terminating ACK
> transaction packet (that is, ACK TP with Retry=1 and Nump != 0).
> Doing so will give controller a chance to recover from the error
> condition.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> ---
> Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++++
> drivers/usb/dwc3/core.c | 16 ++++++++++++++++
> drivers/usb/dwc3/core.h | 6 ++++++
> 3 files changed, 27 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 7f13ebe..2ba2bc2 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -94,6 +94,11 @@ Optional properties:
> this and tx-thr-num-pkt-prd to a valid, non-zero value
> 1-16 (DWC_usb31 programming guide section 1.2.3) to
> enable periodic ESS TX threshold.
> + - snps,enable_auto_retry: Set to enable Auto retry Feature to make the
s/_/-/
> + controller operating in Host mode on seeing transaction
> + errors(CRC errors or internal overrun scenerios) on IN
> + transfers to reply to the device with a non-terminating
> + retry ACK (i.e, an ACK TP with Retry=1 & Nump != 0)
Seems like the property is unnecessary. When would you not want this
retry behavior? Why not just enable unconditionally in the driver?
Rob
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [4/5] memstick: rtsx_usb_ms: Support runtime power management
From: Alan Stern @ 2018-07-25 17:59 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: arnd, gregkh, ulf.hansson, bauer.chen, ricky_wu, linux-kernel,
linux-usb
On Wed, 25 Jul 2018, Kai-Heng Feng wrote:
> In order to let host's parent device, rtsx_usb, to use USB remote wake
> up signaling to do card detection, it needs to be suspended. Hence it's
> necessary to add runtime PM support for the memstick host.
>
> To keep memstick host stays suspended when it's not in use, convert the
> card detection function from kthread to delayed_work, which can be
> scheduled when the host is resumed and can be canceled when the host is
> suspended.
>
> Use an idle function check if there's no card and the power mode is
> MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
I'm not familiar enough with this driver to comment on specific
instances, but you should carefully go through the code and make sure
that you use pm_runtime_get_noresume() and pm_runtime_put_noidle()
correctly.
The basic idea is to use these when you know beforehand that the device
is already at full power (for _get_noresume) and the usage count will
not go to 0 (for _put_noidle). If you aren't certain of these
requirements then you should call pm_runtime_get_sync() and
pm_runtime_put().
Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox