Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Tue, 19 Dec 2017 02:04:51 +0100,
Mauro Santos wrote:
> 
> On 18-12-2017 22:48, Takashi Iwai wrote:
> > On Mon, 18 Dec 2017 22:56:07 +0100,
> > Mauro Santos wrote:
> >>
> >> On 18-12-2017 19:30, Takashi Iwai wrote:
> >>> On Mon, 18 Dec 2017 20:10:44 +0100,
> >>> Mauro Santos wrote:
> 
>  On 18-12-2017 17:50, Jaejoong Kim wrote:
> > Mauro,
> >
> > Could you please try debug patch(I also attach the patch file)?
> 
>  With the attached patch I get the following when plugging in the usb dac
>  directly to a usb3 port:
>  [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
>  [   54.514996] usb 1-2: device descriptor read/64, error -71
>  [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
>  /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
>  [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
>  v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
>  [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
>  [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
>  [   54.950429] usb 1-2: [11] SU [PCM] items = 2
>  [   54.950985] usbcore: registered new interface driver snd-usb-audio
> >>>
> >>> Hmm, the driver get the supposedly correct name string here, so I see
> >>> no flaw, so far.
> >>>
> >>> Could you put the similar debug prints after reverting the commit and
> >>> compare?  Or, at minimum, you can enable simply the kernel debug
> >>> prints like below:
> >>>
> >>>   % echo "file sound/usb/mixer.c +p" > 
> >>> /sys/kernel/debug/dynamic_debug_control
> >>>
> >>> and re-plug the device.
> >>>
> >>> Also, could you attach the output of "amixer contents" on both working
> >>> and non-working kernels?
> >>>
> >>
> >> I have compiled a new kernel where I have reverted the commit and I've
> >> added the debug output based on your last debug patch. I attach the
> >> patch that reverts the changes and adds the debug output just in case
> >> anyone wants to do a sanity check on it (don't mind the indentation I
> >> think I botched that).
> >>
> >> With the debug patches I get no extra output when echoing to the
> >> dynamic_debug/control file, I guess that's expected.
> >>
> >> I attach the dmesg and amixer outputs for the case without revert plus
> >> debug (bad) and revert plus debug (good).
> >>
> >> One change does jump out:
> >>
> >> bad:  usb 1-2: [11] SU [PCM] items = 2
> >> good: usb 1-2: [11] SU [PCM Capture Source] items = 2
> > 
> > Thanks, that explains what went wrong.  The commit dropped the brace
> > at the if-line, and it ended up with the lack of suffix unless
> > get_term_name() fails.
> > 
> > The fix patch is below.  Could you check whether it cures the issue?
> 
> This patch does seem to work fine for me, I can see all the mixer
> controls for the usb soundcard/dac.
> 
> For good measure I have also tested with a couple of usb headsets and I
> didn't see anything amiss (they don't have any "Capture Source" controls
> though).
> 
> Thank you for looking into this and fixing it.

Good to hear, I queued the fix to for-linus branch.
I'm going to send a pull request for rc5.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5

[Bjørn Mork]

"SZ Lin (林上智)"  writes:
>> Johan Hovold  writes:
>> 
>> >> +static const struct option_blacklist_info yuga_clm920_nc5_blacklist = {
>> >> + .reserved = BIT(0) | BIT(1) | BIT(4), };
>> >
>> > Do you really need to blacklist the first interface?
>> 
>> Good question. Interface #0 does look a lot like a Qualcomm DM/DIAG 
>> function, based
>> on two bulk endpoints, no additional descriptors and the fact that it is the 
>> first interface.
>> If so, then we do want a serial driver for it.  There is a basic libqcdm 
>> implementation in
>> ModemManager if you want to test it out.
>> 
>
> I have confirmed that interface #0 is QCDM/DIAG port in this module,
>and thus I will remove this from reserved list in next patch.
>Furthermore, interface #1 is ADB port. Should I also remove this from
>reserved list?

No. ADB is handled by userspace tools using libusb.  It should not be
bound to any serial driver, so you will need to blacklist it.  But you
need to keep the blacklist anyway to include the QCDM/DIAG port

I assume Johan's alternative was to match class/subclass/protocol
against ff/00/00, which would have worked if you only wanted to include
interfaces 2 and 3.

>> And I expect interface #4 is QMI/rmnet?  Feel free to confirm that 
>> assumption with a
>> patch against qmi_wwan :-)
>> 
> Yes, it is. I will send qmi_wwan patch by all means.

Thanks


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5

[林上智]

> -Original Message-
> From: Bjørn Mork [mailto:bj...@mork.no]
> Sent: Tuesday, December 19, 2017 2:44 AM
> To: SZ Lin (林上智)
> Cc: Johan Hovold; Taiyi TY Wu (吳泰毅); Greg Kroah-Hartman;
> linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5
> 
> Johan Hovold  writes:
> 
> >> +static const struct option_blacklist_info yuga_clm920_nc5_blacklist = {
> >> +  .reserved = BIT(0) | BIT(1) | BIT(4), };
> >
> > Do you really need to blacklist the first interface?
> 
> Good question. Interface #0 does look a lot like a Qualcomm DM/DIAG function, 
> based
> on two bulk endpoints, no additional descriptors and the fact that it is the 
> first interface.
> If so, then we do want a serial driver for it.  There is a basic libqcdm 
> implementation in
> ModemManager if you want to test it out.
> 

I have confirmed that interface #0 is QCDM/DIAG port in this module, and thus I 
will remove this from reserved list in next patch.
Furthermore, interface #1 is ADB port. Should I also remove this from reserved 
list?

> And I expect interface #4 is QMI/rmnet?  Feel free to confirm that assumption 
> with a
> patch against qmi_wwan :-)
> 
Yes, it is. I will send qmi_wwan patch by all means.
> 
> Bjørn

SZ

[PATCH V2] USBIP: return correct port ENABLE status

[pei . zhang]

From: Pei Zhang 

USB system will clear port's ENABLE feature for some USB devices when
vdev is already assigned port address. This cause getPortStatus reports
to system that this device is not enabled, client OS will failed to use
this usb device.

The failure devices include a SAMSUNG SSD storage, Logitech webcam C920.

V2: send again to all related maintainers.

Signed-off-by: Pei Zhang 
---
 drivers/usb/usbip/vhci_hcd.c | 63 
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 713e941..7970bab 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -430,38 +430,45 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
vhci_hcd->re_timeout = 0;
}
 
-   if ((vhci_hcd->port_status[rhport] & (1 << 
USB_PORT_FEAT_RESET)) !=
-   0 && time_after(jiffies, vhci_hcd->re_timeout)) {
-   vhci_hcd->port_status[rhport] |= (1 << 
USB_PORT_FEAT_C_RESET);
-   vhci_hcd->port_status[rhport] &= ~(1 << 
USB_PORT_FEAT_RESET);
-   vhci_hcd->re_timeout = 0;
-
-   if (vhci_hcd->vdev[rhport].ud.status ==
-   VDEV_ST_NOTASSIGNED) {
-   usbip_dbg_vhci_rh(
-   " enable rhport %d (status %u)\n",
-   rhport,
-   vhci_hcd->vdev[rhport].ud.status);
-   vhci_hcd->port_status[rhport] |=
-   USB_PORT_STAT_ENABLE;
-   }
-
-   if (hcd->speed < HCD_USB3) {
-   switch (vhci_hcd->vdev[rhport].speed) {
-   case USB_SPEED_HIGH:
-   vhci_hcd->port_status[rhport] |=
- USB_PORT_STAT_HIGH_SPEED;
-   break;
-   case USB_SPEED_LOW:
+   if ((vhci_hcd->port_status[rhport] & (1 << 
USB_PORT_FEAT_RESET))) {
+   if (time_after(jiffies, vhci_hcd->re_timeout)) {
+   vhci_hcd->port_status[rhport] |= (1 << 
USB_PORT_FEAT_C_RESET);
+   vhci_hcd->port_status[rhport] &= ~(1 << 
USB_PORT_FEAT_RESET);
+   vhci_hcd->re_timeout = 0;
+
+   if (vhci_hcd->vdev[rhport].ud.status ==
+   VDEV_ST_NOTASSIGNED) {
+   usbip_dbg_vhci_rh(
+   " enable rhport %d (status 
%u)\n",
+   rhport, 
vhci_hcd->vdev[rhport].ud.status);
vhci_hcd->port_status[rhport] |=
-   USB_PORT_STAT_LOW_SPEED;
-   break;
-   default:
-   pr_err("vhci_device speed not set\n");
-   break;
+   USB_PORT_STAT_ENABLE;
+   }
+
+   if (hcd->speed < HCD_USB3) {
+   switch (vhci_hcd->vdev[rhport].speed) {
+   case USB_SPEED_HIGH:
+   vhci_hcd->port_status[rhport] |=
+   
USB_PORT_STAT_HIGH_SPEED;
+   break;
+   case USB_SPEED_LOW:
+   vhci_hcd->port_status[rhport] |=
+   USB_PORT_STAT_LOW_SPEED;
+   break;
+   default:
+   pr_err("vhci_device speed not 
set\n");
+   break;
+   }
}
}
+   } else {
+   /* Port would be disabled by clearing FEAT_ENABLE,
+* make it enabled again here.
+*/
+   if (vhci_hcd->vdev[rhport].ud.status == VDEV_ST_USED)
+   vhci_hcd->port_status[rhport] |= 
USB_PORT_STAT_ENABLE;
}
+
((__le16 *) buf)[0] = 
cpu_to_le16(vhci_hcd->port_status[rhport]);
((__le16 *) buf)[1] =

[PATCH v2] usb: Add device quirk for Logitech HD Pro Webcam C925e

[Dmitry Fleytman]

From: Dmitry Fleytman Dmitry Fleytman 

Commit e0429362ab15
("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
introduced quirk to workaround an issue with some Logitech webcams.

There is one more model that has the same issue - C925e, so applying
the same quirk as well.

See aforementioned commit message for detailed explanation of the problem.

Signed-off-by: Dmitry Fleytman 
---
 drivers/usb/core/quirks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index a10b346b9777..848da96614e8 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -52,10 +52,11 @@ static const struct usb_device_id usb_quirk_list[] = {
/* Microsoft LifeCam-VX700 v2.0 */
{ USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
 
-   /* Logitech HD Pro Webcams C920, C920-C and C930e */
+   /* Logitech HD Pro Webcams C920, C920-C, C925e and C930e */
{ USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
{ USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
{ USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
+   { USB_DEVICE(0x046d, 0x085b), .driver_info = USB_QUIRK_DELAY_INIT },
 
/* Logitech ConferenceCam CC3000e */
{ USB_DEVICE(0x046d, 0x0847), .driver_info = USB_QUIRK_DELAY_INIT },
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add device quirk for Logitech HD Pro Webcam C925e

[Dmitry Fleytman]

> On 18 Dec 2017, at 20:07, Kai-Heng Feng  wrote:
> 
> Hi, 
> 
>> On 19 Dec 2017, at 12:04 AM, Dmitry Fleytman  
>> wrote:
>> 
>> From: Dmitry Fleytman Dmitry Fleytman 
>> 
>> Commit e0429362ab15
>> ("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
>> introduced quirk to workaround an issue with some Logitech webcams.
>> 
>> There is one more model that has the same issue - C925e, so applying
>> the same quirk as well.
>> 
>> See aforementioned commit message for detailed explanation of the problem.
>> 
>> Signed-off-by: Dmitry Fleytman 
>> ---
>> drivers/usb/core/quirks.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
>> index a10b346b9777..6d2d3b0bcc2a 100644
>> --- a/drivers/usb/core/quirks.c
>> +++ b/drivers/usb/core/quirks.c
>> @@ -52,10 +52,11 @@ static const struct usb_device_id usb_quirk_list[] = {
>>  /* Microsoft LifeCam-VX700 v2.0 */
>>  { USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
>> 
>> -/* Logitech HD Pro Webcams C920, C920-C and C930e */
>> +/* Logitech HD Pro Webcams C920, C920-C, C925e and C930e */
>>  { USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
>>  { USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
>>  { USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
>> +{ USB_DEVICE(0x046d, 0x085B), .driver_info = USB_QUIRK_DELAY_INIT },
> 
> Nitpicking here, but maybe use 0x085b instead of 0x085B?

Yes. Sending v2...

> 
> Kai-Heng
> 
>> 
>>  /* Logitech ConferenceCam CC3000e */
>>  { USB_DEVICE(0x046d, 0x0847), .driver_info = USB_QUIRK_DELAY_INIT },
>> -- 
>> 2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1] usb: chipidea: tegra: Use aligned DMA on Tegra30

[Dmitry Osipenko]

USB Ethernet gadget now works on Tegra30.

Signed-off-by: Dmitry Osipenko 
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c 
b/drivers/usb/chipidea/ci_hdrc_tegra.c
index 7b65a1040d2c..7f4d2b6af37a 100644
--- a/drivers/usb/chipidea/ci_hdrc_tegra.c
+++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
@@ -29,7 +29,7 @@ static const struct tegra_udc_soc_info tegra20_udc_soc_info = 
{
 };
 
 static const struct tegra_udc_soc_info tegra30_udc_soc_info = {
-   .flags = 0,
+   .flags = CI_HDRC_REQUIRES_ALIGNED_DMA,
 };
 
 static const struct tegra_udc_soc_info tegra114_udc_soc_info = {
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Jaejoong Kim]

2017-12-19 7:48 GMT+09:00 Takashi Iwai :
> On Mon, 18 Dec 2017 22:56:07 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 19:30, Takashi Iwai wrote:
>> > On Mon, 18 Dec 2017 20:10:44 +0100,
>> > Mauro Santos wrote:
>> >>
>> >> On 18-12-2017 17:50, Jaejoong Kim wrote:
>> >>> Mauro,
>> >>>
>> >>> Could you please try debug patch(I also attach the patch file)?
>> >>
>> >> With the attached patch I get the following when plugging in the usb dac
>> >> directly to a usb3 port:
>> >> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
>> >> [   54.514996] usb 1-2: device descriptor read/64, error -71
>> >> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
>> >> /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
>> >> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
>> >> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
>> >> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
>> >> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
>> >> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
>> >> [   54.950985] usbcore: registered new interface driver snd-usb-audio
>> >
>> > Hmm, the driver get the supposedly correct name string here, so I see
>> > no flaw, so far.
>> >
>> > Could you put the similar debug prints after reverting the commit and
>> > compare?  Or, at minimum, you can enable simply the kernel debug
>> > prints like below:
>> >
>> >   % echo "file sound/usb/mixer.c +p" > 
>> > /sys/kernel/debug/dynamic_debug_control
>> >
>> > and re-plug the device.
>> >
>> > Also, could you attach the output of "amixer contents" on both working
>> > and non-working kernels?
>> >
>>
>> I have compiled a new kernel where I have reverted the commit and I've
>> added the debug output based on your last debug patch. I attach the
>> patch that reverts the changes and adds the debug output just in case
>> anyone wants to do a sanity check on it (don't mind the indentation I
>> think I botched that).
>>
>> With the debug patches I get no extra output when echoing to the
>> dynamic_debug/control file, I guess that's expected.
>>
>> I attach the dmesg and amixer outputs for the case without revert plus
>> debug (bad) and revert plus debug (good).
>>
>> One change does jump out:
>>
>> bad:  usb 1-2: [11] SU [PCM] items = 2
>> good: usb 1-2: [11] SU [PCM Capture Source] items = 2
>
> Thanks, that explains what went wrong.  The commit dropped the brace
> at the if-line, and it ended up with the lack of suffix unless
> get_term_name() fails.
>
> The fix patch is below.  Could you check whether it cures the issue?
>
> Also, Jaejoong, could you check whether it doesn't your device,
> either?  I believe it should keep working, but just to be sure.

I don't have an USB audio DAC with selector unit descriptor.
But your patch file looks good to me. Thanks for fix the regression.

>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai 
> Subject: [PATCH] ALSA: usb-audio: Fix the missing ctl name suffix at parsing
>  SU
>
> The commit 89b89d121ffc ("ALSA: usb-audio: Add check return value for
> usb_string()") added the check of the return value from
> snd_usb_copy_string_desc(), which is correct per se, but it introduced
> a regression.  In the original code, either the "Clock Source",
> "Playback Source" or "Capture Source" suffix is added after the
> terminal string, while the commit changed it to add the suffix only
> when get_term_name() is failing.  It ended up with an incorrect ctl
> name like "PCM" instead of "PCM Capture Source".
>
> Also, even the original code has a similar bug: when the ctl name is
> generated from snd_usb_copy_string_desc() for the given iSelector, it
> also doesn't put the suffix.
>
> This patch addresses these issues: the suffix is added always when no
> static mapping is found.  Also the patch tries to put more comments
> and cleans up the if/else block for better readability in order to
> avoid the same pitfall again.
>
> Fixes: 89b89d121ffc ("ALSA: usb-audio: Add check return value for 
> usb_string()")
> Reported-by: Mauro Santos 
> Cc: 
> Signed-off-by: Takashi Iwai 
> ---
>  sound/usb/mixer.c | 27 ---
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index afc208e1c756..60ebc99ae323 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2173,20 +2173,25 @@ static int parse_audio_selector_unit(struct 
> mixer_build *state, int unitid,
> kctl->private_value = (unsigned long)namelist;
> kctl->private_free = usb_mixer_selector_elem_free;
>
> -   nameid = uac_selector_unit_iSelector(desc);
> +   /* check the static mapping table at first */
> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> -   if (len)
> -   ;
> -   else if (nameid)
> -   

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 22:48, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 22:56:07 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 19:30, Takashi Iwai wrote:
>>> On Mon, 18 Dec 2017 20:10:44 +0100,
>>> Mauro Santos wrote:

 On 18-12-2017 17:50, Jaejoong Kim wrote:
> Mauro,
>
> Could you please try debug patch(I also attach the patch file)?

 With the attached patch I get the following when plugging in the usb dac
 directly to a usb3 port:
 [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
 [   54.514996] usb 1-2: device descriptor read/64, error -71
 [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
 /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
 [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
 v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
 [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
 [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
 [   54.950429] usb 1-2: [11] SU [PCM] items = 2
 [   54.950985] usbcore: registered new interface driver snd-usb-audio
>>>
>>> Hmm, the driver get the supposedly correct name string here, so I see
>>> no flaw, so far.
>>>
>>> Could you put the similar debug prints after reverting the commit and
>>> compare?  Or, at minimum, you can enable simply the kernel debug
>>> prints like below:
>>>
>>>   % echo "file sound/usb/mixer.c +p" > 
>>> /sys/kernel/debug/dynamic_debug_control
>>>
>>> and re-plug the device.
>>>
>>> Also, could you attach the output of "amixer contents" on both working
>>> and non-working kernels?
>>>
>>
>> I have compiled a new kernel where I have reverted the commit and I've
>> added the debug output based on your last debug patch. I attach the
>> patch that reverts the changes and adds the debug output just in case
>> anyone wants to do a sanity check on it (don't mind the indentation I
>> think I botched that).
>>
>> With the debug patches I get no extra output when echoing to the
>> dynamic_debug/control file, I guess that's expected.
>>
>> I attach the dmesg and amixer outputs for the case without revert plus
>> debug (bad) and revert plus debug (good).
>>
>> One change does jump out:
>>
>> bad:  usb 1-2: [11] SU [PCM] items = 2
>> good: usb 1-2: [11] SU [PCM Capture Source] items = 2
> 
> Thanks, that explains what went wrong.  The commit dropped the brace
> at the if-line, and it ended up with the lack of suffix unless
> get_term_name() fails.
> 
> The fix patch is below.  Could you check whether it cures the issue?

This patch does seem to work fine for me, I can see all the mixer
controls for the usb soundcard/dac.

For good measure I have also tested with a couple of usb headsets and I
didn't see anything amiss (they don't have any "Capture Source" controls
though).

Thank you for looking into this and fixing it.

> Also, Jaejoong, could you check whether it doesn't your device,
> either?  I believe it should keep working, but just to be sure.
> 
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai 
> Subject: [PATCH] ALSA: usb-audio: Fix the missing ctl name suffix at parsing
>  SU
> 
> The commit 89b89d121ffc ("ALSA: usb-audio: Add check return value for
> usb_string()") added the check of the return value from
> snd_usb_copy_string_desc(), which is correct per se, but it introduced
> a regression.  In the original code, either the "Clock Source",
> "Playback Source" or "Capture Source" suffix is added after the
> terminal string, while the commit changed it to add the suffix only
> when get_term_name() is failing.  It ended up with an incorrect ctl
> name like "PCM" instead of "PCM Capture Source".
> 
> Also, even the original code has a similar bug: when the ctl name is
> generated from snd_usb_copy_string_desc() for the given iSelector, it
> also doesn't put the suffix.
> 
> This patch addresses these issues: the suffix is added always when no
> static mapping is found.  Also the patch tries to put more comments
> and cleans up the if/else block for better readability in order to
> avoid the same pitfall again.
> 
> Fixes: 89b89d121ffc ("ALSA: usb-audio: Add check return value for 
> usb_string()")
> Reported-by: Mauro Santos 
> Cc: 
> Signed-off-by: Takashi Iwai 
> ---
>  sound/usb/mixer.c | 27 ---
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index afc208e1c756..60ebc99ae323 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2173,20 +2173,25 @@ static int parse_audio_selector_unit(struct 
> mixer_build *state, int unitid,
>   kctl->private_value = (unsigned long)namelist;
>   kctl->private_free = usb_mixer_selector_elem_free;
>  
> - nameid = uac_selector_unit_iSelector(desc);
> + /* check the static mapping table at first */
>   len = check_mapped_name(map, 

Re: Storing errors in the XArray

[Randy Dunlap]

On 12/15/2017 09:10 AM, Matthew Wilcox wrote:
> On Mon, Dec 11, 2017 at 03:10:22PM -0800, Randy Dunlap wrote:
>>> +The XArray does not support storing :c:func:`IS_ERR` pointers; some
>>> +conflict with data values and others conflict with entries the XArray
>>> +uses for its own purposes.  If you need to store special values which
>>> +cannot be confused with real kernel pointers, the values 4, 8, ... 4092
>>> +are available.
>>
>> or if I know that they values are errno-range values, I can just shift them
>> left by 2 to store them and then shift them right by 2 to use them?
> 
> On further thought, I like this idea so much, it's worth writing helpers
> for this usage.  And test-suite (also doubles as a demonstration of how
> to use it).
> 
> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
> index c616e9319c7c..53aa251df57a 100644
> --- a/include/linux/xarray.h
> +++ b/include/linux/xarray.h
> @@ -232,6 +232,39 @@ static inline bool xa_is_value(const void *entry)
>   return (unsigned long)entry & 1;
>  }
>  
> +/**
> + * xa_mk_errno() - Create an XArray entry from an error number.
> + * @error: Error number to store in XArray.
> + *
> + * Return: An entry suitable for storing in the XArray.
> + */
> +static inline void *xa_mk_errno(long error)
> +{
> + return (void *)(error << 2);
> +}
> +
> +/**
> + * xa_to_errno() - Get error number stored in an XArray entry.
> + * @entry: XArray entry.
> + *
> + * Return: The error number stored in the XArray entry.
> + */
> +static inline unsigned long xa_to_errno(const void *entry)
> +{
> + return (long)entry >> 2;
> +}
> +
> +/**
> + * xa_is_errno() - Determine if an entry is an errno.
> + * @entry: XArray entry.
> + *
> + * Return: True if the entry is an errno, false if it is a pointer.
> + */
> +static inline bool xa_is_errno(const void *entry)
> +{
> + return (((unsigned long)entry & 3) == 0) && (entry > (void *)-4096);

Some named mask bits would be ^^^ preferable there.
#define MAX_ERRNO   4095 // from err.h
 && (entry >= (void 
*)-MAX_ERRNO);

> +}
> +
>  /**
>   * xa_is_internal() - Is the entry an internal entry?
>   * @entry: Entry retrieved from the XArray
> diff --git a/tools/testing/radix-tree/xarray-test.c 
> b/tools/testing/radix-tree/xarray-test.c
> index 43111786ebdd..b843cedf3988 100644
> --- a/tools/testing/radix-tree/xarray-test.c
> +++ b/tools/testing/radix-tree/xarray-test.c
> @@ -29,7 +29,13 @@ void check_xa_err(struct xarray *xa)
>   assert(xa_err(xa_store(xa, 1, xa_mk_value(0), GFP_KERNEL)) == 0);
>   assert(xa_err(xa_store(xa, 1, NULL, 0)) == 0);
>  // kills the test-suite :-(
> -// assert(xa_err(xa_store(xa, 0, xa_mk_internal(0), 0)) == -EINVAL);
> +//   assert(xa_err(xa_store(xa, 0, xa_mk_internal(0), 0)) == -EINVAL);
> +
> + assert(xa_err(xa_store(xa, 0, xa_mk_errno(-ENOMEM), GFP_KERNEL)) == 0);
> + assert(xa_err(xa_load(xa, 0)) == 0);
> + assert(xa_is_errno(xa_load(xa, 0)) == true);
> + assert(xa_to_errno(xa_load(xa, 0)) == -ENOMEM);
> + xa_erase(xa, 0);
>  }
>  
>  void check_xa_tag(struct xarray *xa)
> 

Thanks,
-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vhci: stop printing kernel pointer addresses in messages

[Shuah Khan]

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan 
---
 drivers/usb/usbip/vhci_hcd.c | 10 --
 drivers/usb/usbip/vhci_rx.c  | 23 +++
 drivers/usb/usbip/vhci_tx.c  |  3 ++-
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 390053e05dca..4dc8b42db7d9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -656,9 +656,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb 
*urb, gfp_t mem_flag
struct vhci_device *vdev;
unsigned long flags;
 
-   usbip_dbg_vhci_hc("enter, usb_hcd %p urb %p mem_flags %d\n",
- hcd, urb, mem_flags);
-
if (portnum > VHCI_HC_PORTS) {
pr_err("invalid port number %d\n", portnum);
return -ENODEV;
@@ -822,8 +819,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
struct vhci_device *vdev;
unsigned long flags;
 
-   pr_info("dequeue a urb %p\n", urb);
-
spin_lock_irqsave(>lock, flags);
 
priv = urb->hcpriv;
@@ -851,7 +846,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
/* tcp connection is closed */
spin_lock(>priv_lock);
 
-   pr_info("device %p seems to be disconnected\n", vdev);
list_del(>list);
kfree(priv);
urb->hcpriv = NULL;
@@ -863,8 +857,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 * vhci_rx will receive RET_UNLINK and give back the URB.
 * Otherwise, we give back it here.
 */
-   pr_info("gives back urb %p\n", urb);
-
usb_hcd_unlink_urb_from_ep(hcd, urb);
 
spin_unlock_irqrestore(>lock, flags);
@@ -892,8 +884,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 
unlink->unlink_seqnum = priv->seqnum;
 
-   pr_info("device %p seems to be still connected\n", vdev);
-
/* send cmd_unlink and try to cancel the pending URB in the
 * peer */
list_add_tail(>list, >unlink_tx);
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index 90577e8b2282..112ebb90d8c9 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -23,24 +23,23 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device 
*vdev, __u32 seqnum)
urb = priv->urb;
status = urb->status;
 
-   usbip_dbg_vhci_rx("find urb %p vurb %p seqnum %u\n",
-   urb, priv, seqnum);
+   usbip_dbg_vhci_rx("find urb seqnum %u\n", seqnum);
 
switch (status) {
case -ENOENT:
/* fall through */
case -ECONNRESET:
-   dev_info(>dev->dev,
-"urb %p was unlinked %ssynchronuously.\n", urb,
-status == -ENOENT ? "" : "a");
+   dev_dbg(>dev->dev,
+"urb seq# %u was unlinked %ssynchronuously\n",
+seqnum, status == -ENOENT ? "" : "a");
break;
case -EINPROGRESS:
/* no info output */
break;
default:
-   dev_info(>dev->dev,
-"urb %p may be in a error, status %d\n", urb,
-status);
+   dev_dbg(>dev->dev,
+"urb seq# %u may be in a error, status %d\n",
+seqnum, status);
}
 
list_del(>list);
@@ -67,8 +66,8 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
spin_unlock_irqrestore(>priv_lock, flags);
 
if (!urb) {
-   pr_err("cannot find a urb of seqnum %u\n", pdu->base.seqnum);
-   pr_info("max seqnum %d\n",
+   pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
+   pdu->base.seqnum,
atomic_read(_hcd->seqnum));
usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
return;
@@ -91,7 +90,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
if (usbip_dbg_flag_vhci_rx)
usbip_dump_urb(urb);
 
-   usbip_dbg_vhci_rx("now giveback urb %p\n", urb);
+   usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum);
 
spin_lock_irqsave(>lock, flags);
usb_hcd_unlink_urb_from_ep(vhci_hcd_to_hcd(vhci_hcd), urb);
@@ -158,7 +157,7 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
   

[PATCH] usbip: stub: stop printing kernel pointer addresses in messages

[Shuah Khan]

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan 
---
 drivers/usb/usbip/stub_main.c | 5 +++--
 drivers/usb/usbip/stub_rx.c   | 7 ++-
 drivers/usb/usbip/stub_tx.c   | 6 +++---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 4f48b306713f..c31c8402a0c5 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -237,11 +237,12 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
struct stub_priv *priv;
struct urb *urb;
 
-   dev_dbg(>udev->dev, "free sdev %p\n", sdev);
+   dev_dbg(>udev->dev, "Stub device cleaning up urbs\n");
 
while ((priv = stub_priv_pop(sdev))) {
urb = priv->urb;
-   dev_dbg(>udev->dev, "free urb %p\n", urb);
+   dev_dbg(>udev->dev, "free urb seqnum %lu\n",
+   priv->seqnum);
usb_kill_urb(urb);
 
kmem_cache_free(stub_priv_cache, priv);
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index b1820ab083e1..6c5a59313999 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -211,9 +211,6 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
if (priv->seqnum != pdu->u.cmd_unlink.seqnum)
continue;
 
-   dev_info(>urb->dev->dev, "unlink urb %p\n",
-priv->urb);
-
/*
 * This matched urb is not completed yet (i.e., be in
 * flight in usb hcd hardware/driver). Now we are
@@ -252,8 +249,8 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
ret = usb_unlink_urb(priv->urb);
if (ret != -EINPROGRESS)
dev_err(>urb->dev->dev,
-   "failed to unlink a urb %p, ret %d\n",
-   priv->urb, ret);
+   "failed to unlink a urb # %lu, ret %d\n",
+   priv->seqnum, ret);
 
return 0;
}
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index 53172b1f6257..f0ec41a50cbc 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -88,7 +88,7 @@ void stub_complete(struct urb *urb)
/* link a urb to the queue of tx. */
spin_lock_irqsave(>priv_lock, flags);
if (sdev->ud.tcp_socket == NULL) {
-   usbip_dbg_stub_tx("ignore urb for closed connection %p", urb);
+   usbip_dbg_stub_tx("ignore urb for closed connection\n");
/* It will be freed in stub_device_cleanup_urbs(). */
} else if (priv->unlinking) {
stub_enqueue_ret_unlink(sdev, priv->seqnum, urb->status);
@@ -190,8 +190,8 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 
/* 1. setup usbip_header */
setup_ret_submit_pdu(_header, urb);
-   usbip_dbg_stub_tx("setup txdata seqnum: %d urb: %p\n",
- pdu_header.base.seqnum, urb);
+   usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+ pdu_header.base.seqnum);
usbip_header_correct_endian(_header, 1);
 
iov[iovnum].iov_base = _header;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Naming of tag operations in the XArray

[Randy Dunlap]

On 12/15/2017 04:34 AM, Matthew Wilcox wrote:
> On Thu, Dec 14, 2017 at 08:22:14PM -0800, Matthew Wilcox wrote:
>> On Mon, Dec 11, 2017 at 03:10:22PM -0800, Randy Dunlap wrote:
 +A freshly-initialised XArray contains a ``NULL`` pointer at every index.
 +Each non-``NULL`` entry in the array has three bits associated with
 +it called tags.  Each tag may be flipped on or off independently of
 +the others.  You can search for entries with a given tag set.
>>>
>>> Only tags that are set, or search for entries with some tag(s) cleared?
>>> Or is that like a mathematical set?
>>
>> hmm ...
>>
>> "Each tag may be set or cleared independently of the others.  You can
>> search for entries which have a particular tag set."
>>
>> Doesn't completely remove the ambiguity, but I can't think of how to phrase
>> that better ...
> 
> Thinking about this some more ...
> 
> At the moment, the pieces of the API which deal with tags look like this:
> 
> bool xa_tagged(const struct xarray *, xa_tag_t)
> bool xa_get_tag(struct xarray *, unsigned long index, xa_tag_t);
> void xa_set_tag(struct xarray *, unsigned long index, xa_tag_t);
> void xa_clear_tag(struct xarray *, unsigned long index, xa_tag_t);
> int xa_get_tagged(struct xarray *, void **dst, unsigned long start,
> unsigned long max, unsigned int n, xa_tag_t);
> 
> bool xas_get_tag(const struct xa_state *, xa_tag_t);
> void xas_set_tag(const struct xa_state *, xa_tag_t);
> void xas_clear_tag(const struct xa_state *, xa_tag_t);
> void *xas_find_tag(struct xa_state *, unsigned long max, xa_tag_t);
> xas_for_each_tag(xas, entry, max, tag) { }
> 
> (at some point there will be an xa_for_each_tag too, there just hasn't
> been a user yet).
> 
> I'm always ambivalent about using the word 'get' in an API because it has
> two common meanings; (increment a refcount) and (return the state).  How

Yes, I get that.  But you usually wouldn't lock a tag AFAIK.

> would people feel about these names instead:

I think that the original names are mostly better, except I do like
xa_select() instead of xa_get_tagged().  But even that doesn't have
to change.

> bool xa_any_tagged(xa, tag);
> bool xa_is_tagged(xa, index, tag);
> void xa_tag(xa, index, tag);
> void xa_untag(xa, index, tag);
> int xa_select(xa, dst, start, max, n, tag);
> 
> bool xas_is_tagged(xas, tag);
> void xas_tag(xas, tag);
> void xas_untag(xas, tag);
> void *xas_find_tag(xas, max, tag);
> xas_for_each_tag(xas, entry, max, tag) { }
> 
> (the last two are unchanged)
> 


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 22:56:07 +0100,
Mauro Santos wrote:
> 
> On 18-12-2017 19:30, Takashi Iwai wrote:
> > On Mon, 18 Dec 2017 20:10:44 +0100,
> > Mauro Santos wrote:
> >>
> >> On 18-12-2017 17:50, Jaejoong Kim wrote:
> >>> Mauro,
> >>>
> >>> Could you please try debug patch(I also attach the patch file)?
> >>
> >> With the attached patch I get the following when plugging in the usb dac
> >> directly to a usb3 port:
> >> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
> >> [   54.514996] usb 1-2: device descriptor read/64, error -71
> >> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
> >> /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
> >> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
> >> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
> >> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
> >> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
> >> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
> >> [   54.950985] usbcore: registered new interface driver snd-usb-audio
> > 
> > Hmm, the driver get the supposedly correct name string here, so I see
> > no flaw, so far.
> > 
> > Could you put the similar debug prints after reverting the commit and
> > compare?  Or, at minimum, you can enable simply the kernel debug
> > prints like below:
> > 
> >   % echo "file sound/usb/mixer.c +p" > 
> > /sys/kernel/debug/dynamic_debug_control
> > 
> > and re-plug the device.
> > 
> > Also, could you attach the output of "amixer contents" on both working
> > and non-working kernels?
> > 
> 
> I have compiled a new kernel where I have reverted the commit and I've
> added the debug output based on your last debug patch. I attach the
> patch that reverts the changes and adds the debug output just in case
> anyone wants to do a sanity check on it (don't mind the indentation I
> think I botched that).
> 
> With the debug patches I get no extra output when echoing to the
> dynamic_debug/control file, I guess that's expected.
> 
> I attach the dmesg and amixer outputs for the case without revert plus
> debug (bad) and revert plus debug (good).
> 
> One change does jump out:
> 
> bad:  usb 1-2: [11] SU [PCM] items = 2
> good: usb 1-2: [11] SU [PCM Capture Source] items = 2

Thanks, that explains what went wrong.  The commit dropped the brace
at the if-line, and it ended up with the lack of suffix unless
get_term_name() fails.

The fix patch is below.  Could you check whether it cures the issue?

Also, Jaejoong, could you check whether it doesn't your device,
either?  I believe it should keep working, but just to be sure.


Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: usb-audio: Fix the missing ctl name suffix at parsing
 SU

The commit 89b89d121ffc ("ALSA: usb-audio: Add check return value for
usb_string()") added the check of the return value from
snd_usb_copy_string_desc(), which is correct per se, but it introduced
a regression.  In the original code, either the "Clock Source",
"Playback Source" or "Capture Source" suffix is added after the
terminal string, while the commit changed it to add the suffix only
when get_term_name() is failing.  It ended up with an incorrect ctl
name like "PCM" instead of "PCM Capture Source".

Also, even the original code has a similar bug: when the ctl name is
generated from snd_usb_copy_string_desc() for the given iSelector, it
also doesn't put the suffix.

This patch addresses these issues: the suffix is added always when no
static mapping is found.  Also the patch tries to put more comments
and cleans up the if/else block for better readability in order to
avoid the same pitfall again.

Fixes: 89b89d121ffc ("ALSA: usb-audio: Add check return value for usb_string()")
Reported-by: Mauro Santos 
Cc: 
Signed-off-by: Takashi Iwai 
---
 sound/usb/mixer.c | 27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index afc208e1c756..60ebc99ae323 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2173,20 +2173,25 @@ static int parse_audio_selector_unit(struct mixer_build 
*state, int unitid,
kctl->private_value = (unsigned long)namelist;
kctl->private_free = usb_mixer_selector_elem_free;
 
-   nameid = uac_selector_unit_iSelector(desc);
+   /* check the static mapping table at first */
len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
-   if (len)
-   ;
-   else if (nameid)
-   len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
-sizeof(kctl->id.name));
-   else
-   len = get_term_name(state, >oterm,
-   kctl->id.name, sizeof(kctl->id.name), 0);
-
if (!len) {
-   strlcpy(kctl->id.name, "USB", 

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 21:56, Mauro Santos wrote:
> On 18-12-2017 19:30, Takashi Iwai wrote:
>> On Mon, 18 Dec 2017 20:10:44 +0100,
>> Mauro Santos wrote:
>>>
>>> On 18-12-2017 17:50, Jaejoong Kim wrote:
 Mauro,

 Could you please try debug patch(I also attach the patch file)?
>>>
>>> With the attached patch I get the following when plugging in the usb dac
>>> directly to a usb3 port:
>>> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
>>> [   54.514996] usb 1-2: device descriptor read/64, error -71
>>> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
>>> /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
>>> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
>>> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
>>> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
>>> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
>>> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
>>> [   54.950985] usbcore: registered new interface driver snd-usb-audio
>>
>> Hmm, the driver get the supposedly correct name string here, so I see
>> no flaw, so far.
>>
>> Could you put the similar debug prints after reverting the commit and
>> compare?  Or, at minimum, you can enable simply the kernel debug
>> prints like below:
>>
>>   % echo "file sound/usb/mixer.c +p" > 
>> /sys/kernel/debug/dynamic_debug_control
>>
>> and re-plug the device.
>>
>> Also, could you attach the output of "amixer contents" on both working
>> and non-working kernels?
>>
> 
> I have compiled a new kernel where I have reverted the commit and I've
> added the debug output based on your last debug patch. I attach the
> patch that reverts the changes and adds the debug output just in case
> anyone wants to do a sanity check on it (don't mind the indentation I
> think I botched that).
> 
> With the debug patches I get no extra output when echoing to the
> dynamic_debug/control file, I guess that's expected.
> 
Turns out there is some output, I can't echo and plug, I need to plug,
echo, replug. Dmesg outputs are attached.

> I attach the dmesg and amixer outputs for the case without revert plus
> debug (bad) and revert plus debug (good).
> 
> One change does jump out:
> 
> bad:  usb 1-2: [11] SU [PCM] items = 2
> good: usb 1-2: [11] SU [PCM Capture Source] items = 2
> 


-- 
Mauro Santos
[   81.753703] usb 1-2: new full-speed USB device number 9 using xhci_hcd
[   81.877148] usb 1-2: device descriptor read/64, error -71
[   82.210907] input: HiFimeDIY Audio HiFimeDIY DAC as 
/devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0005/input/input22
[   82.211121] hid-generic 0003:1852:7022.0005: input,hidraw2: USB HID v1.00 
Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
[   82.231042] usb 1-2: [16] FU [PCM Playback Switch] ch = 1, val = 0/1/1
[   82.232211] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex 
= 0x1001, type = 4, data = 0x40/0x0
[   82.232616] usb 1-2: [16] FU [PCM Playback Volume] ch = 2, val = -14080/0/128
[   82.232626] usb 1-2: [14] FU [Line Capture Switch] ch = 1, val = 0/1/1
[   82.233919] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex 
= 0xe01, type = 4, data = 0x40/0x0
[   82.234301] usb 1-2: [14] FU [Line Capture Volume] ch = 2, val = 
-10240/3072/128
[   82.234310] usb 1-2: [DEBUG] nameid:0, len:0
[   82.234313] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[   82.234315] usb 1-2: [11] SU [PCM] items = 2
[   59.283418] usb 1-2: new full-speed USB device number 8 using xhci_hcd
[   59.407116] usb 1-2: device descriptor read/64, error -71
[   59.740686] input: HiFimeDIY Audio HiFimeDIY DAC as 
/devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0004/input/input21
[   59.740893] hid-generic 0003:1852:7022.0004: input,hidraw2: USB HID v1.00 
Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
[   59.760971] usb 1-2: [16] FU [PCM Playback Switch] ch = 1, val = 0/1/1
[   59.762056] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex 
= 0x1001, type = 4, data = 0x40/0x0
[   59.762418] usb 1-2: [16] FU [PCM Playback Volume] ch = 2, val = -14080/0/128
[   59.762423] usb 1-2: [14] FU [Line Capture Switch] ch = 1, val = 0/1/1
[   59.763510] usb 1-2: cannot set ctl value: req = 0x4, wValue = 0x201, wIndex 
= 0xe01, type = 4, data = 0x40/0x0
[   59.763845] usb 1-2: [14] FU [Line Capture Volume] ch = 2, val = 
-10240/3072/128
[   59.763848] usb 1-2: [DEBUG] nameid:0, len:0
[   59.763849] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[   59.763850] usb 1-2: [11] SU [PCM Capture Source] items = 2

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 19:30, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 20:10:44 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 17:50, Jaejoong Kim wrote:
>>> Mauro,
>>>
>>> Could you please try debug patch(I also attach the patch file)?
>>
>> With the attached patch I get the following when plugging in the usb dac
>> directly to a usb3 port:
>> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
>> [   54.514996] usb 1-2: device descriptor read/64, error -71
>> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
>> /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
>> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
>> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
>> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
>> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
>> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
>> [   54.950985] usbcore: registered new interface driver snd-usb-audio
> 
> Hmm, the driver get the supposedly correct name string here, so I see
> no flaw, so far.
> 
> Could you put the similar debug prints after reverting the commit and
> compare?  Or, at minimum, you can enable simply the kernel debug
> prints like below:
> 
>   % echo "file sound/usb/mixer.c +p" > /sys/kernel/debug/dynamic_debug_control
> 
> and re-plug the device.
> 
> Also, could you attach the output of "amixer contents" on both working
> and non-working kernels?
> 

I have compiled a new kernel where I have reverted the commit and I've
added the debug output based on your last debug patch. I attach the
patch that reverts the changes and adds the debug output just in case
anyone wants to do a sanity check on it (don't mind the indentation I
think I botched that).

With the debug patches I get no extra output when echoing to the
dynamic_debug/control file, I guess that's expected.

I attach the dmesg and amixer outputs for the case without revert plus
debug (bad) and revert plus debug (good).

One change does jump out:

bad:  usb 1-2: [11] SU [PCM] items = 2
good: usb 1-2: [11] SU [PCM Capture Source] items = 2

-- 
Mauro Santos
Simple mixer control 'PCM',0
  Capabilities: pvolume pswitch pswitch-joined
  Playback channels: Front Left - Front Right
  Limits: Playback 0 - 110
  Mono:
  Front Left: Playback 0 [0%] [-55.00dB] [on]
  Front Right: Playback 0 [0%] [-55.00dB] [on]
Simple mixer control 'PCM Capture Source',0
  Capabilities: enum
  Items: 'Line' 'IEC958 In'
  Item0: 'Line'
Simple mixer control 'Line',0
  Capabilities: cvolume cswitch cswitch-joined
  Capture channels: Front Left - Front Right
  Limits: Capture 0 - 104
  Front Left: Capture 0 [0%] [-40.00dB] [off]
  Front Right: Capture 0 [0%] [-40.00dB] [off]
[   63.600402] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[   63.723739] usb 1-2: device descriptor read/64, error -71
[   64.057506] input: HiFimeDIY Audio HiFimeDIY DAC as 
/devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
[   64.057780] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID v1.00 
Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
[   64.154091] usb 1-2: [DEBUG] nameid:0, len:0
[   64.154095] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[   64.154097] usb 1-2: [11] SU [PCM Capture Source] items = 2
[   64.154495] usbcore: registered new interface driver snd-usb-audio
diff -ur a/sound/usb/mixer.c b/sound/usb/mixer.c
--- a/sound/usb/mixer.c	2017-12-18 19:47:02.748776502 +
+++ b/sound/usb/mixer.c	2017-12-18 20:18:30.023770892 +
@@ -595,7 +595,7 @@
 	while (snd_ctl_find_id(mixer->chip->card, >id))
 		kctl->id.index++;
 	if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
-		usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
+		usb_audio_err(mixer->chip, "cannot add control (err = %d)\n",
 			  err);
 		return err;
 	}
@@ -656,10 +656,14 @@
 			 unsigned char *name, int maxlen, int term_only)
 {
 	struct iterm_name_combo *names;
+int len;
 
-	if (iterm->name)
-		return snd_usb_copy_string_desc(state, iterm->name,
+	if (iterm->name) {
+		len = snd_usb_copy_string_desc(state, iterm->name,
 		name, maxlen);
+usb_audio_err(state->chip, "[DEBUG] len:%d, iterm->name:%d\n", len, iterm->name);
+return len;
+}
 
 	/* virtual type - not a real terminal */
 	if (iterm->type >> 16) {
@@ -2175,17 +2179,24 @@
 
 	nameid = uac_selector_unit_iSelector(desc);
 	len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+
+usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);
+
 	if (len)
 		;
-	else if (nameid)
+	else if (nameid) {
 		len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
 	 sizeof(kctl->id.name));
-	else
+usb_audio_err(state->chip, "[DEBUG] len:%d, copy_string id.name:%s\n",
+len, (len > 0) ? kctl->id.name : " ");
+}
+	else {
 		len = get_term_name(state, >oterm,
 kctl->id.name, 

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 20:10:44 +0100,
Mauro Santos wrote:
> 
> On 18-12-2017 17:50, Jaejoong Kim wrote:
> > Mauro,
> > 
> > Could you please try debug patch(I also attach the patch file)?
> 
> With the attached patch I get the following when plugging in the usb dac
> directly to a usb3 port:
> [   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
> [   54.514996] usb 1-2: device descriptor read/64, error -71
> [   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
> /devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
> [   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
> v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
> [   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
> [   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
> [   54.950429] usb 1-2: [11] SU [PCM] items = 2
> [   54.950985] usbcore: registered new interface driver snd-usb-audio

Hmm, the driver get the supposedly correct name string here, so I see
no flaw, so far.

Could you put the similar debug prints after reverting the commit and
compare?  Or, at minimum, you can enable simply the kernel debug
prints like below:

  % echo "file sound/usb/mixer.c +p" > /sys/kernel/debug/dynamic_debug_control

and re-plug the device.

Also, could you attach the output of "amixer contents" on both working
and non-working kernels?


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 17:50, Jaejoong Kim wrote:
> Mauro,
> 
> Could you please try debug patch(I also attach the patch file)?

With the attached patch I get the following when plugging in the usb dac
directly to a usb3 port:
[   54.391539] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[   54.514996] usb 1-2: device descriptor read/64, error -71
[   54.849808] input: HiFimeDIY Audio HiFimeDIY DAC as
/devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
[   54.850168] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
[   54.950421] usb 1-2: [DEBUG] nameid:0, len:0
[   54.950426] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[   54.950429] usb 1-2: [11] SU [PCM] items = 2
[   54.950985] usbcore: registered new interface driver snd-usb-audio

> 
> And can you share your usb audio dac? Is this same device with yours?
> https://hifimediy.com/index.php?route=product/product_id=83
> 

It is similar to that but I bought mine in August of 2013.

The description of my device when I bought it was:
HifiMeDiy Sabre USB DAC. Digital to Audio Converter 96khz/24bit (incl
USB to optical converter feature).

I have opened the box and on the PCB mine says UAE23 v1.3A.
It uses a tenor te7022l usb receiver plus a sabre es9032p dac if it matters.

> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 84b9f9c..6200aa2 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -595,7 +595,7 @@ int snd_usb_mixer_add_control(struct
> usb_mixer_elem_list *list,
>   while (snd_ctl_find_id(mixer->chip->card, >id))
>   kctl->id.index++;
>   if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
> - usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
> + usb_audio_err(mixer->chip, "[DEBUG] cannot add control (err = %d)\n",
> err);
>   return err;
>   }
> @@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build
> *state, struct usb_audio_term *iterm
>   unsigned char *name, int maxlen, int term_only)
>  {
>   struct iterm_name_combo *names;
> + int len;
> 
> - if (iterm->name)
> - return snd_usb_copy_string_desc(state, iterm->name,
> + if (iterm->name) {
> + len = snd_usb_copy_string_desc(state, iterm->name,
>   name, maxlen);
> + usb_audio_err(state->chip, "[DEBUG] len:%d, iterm->name:%d\n", len,
> iterm->name);
> + return len;
> + }
> 
>   /* virtual type - not a real terminal */
>   if (iterm->type >> 16) {
> @@ -2162,14 +2166,23 @@ static int parse_audio_selector_unit(struct
> mixer_build *state, int unitid,
> 
>   nameid = uac_selector_unit_iSelector(desc);
>   len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> +
> + usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);
> +
>   if (len)
>   ;
> - else if (nameid)
> + else if (nameid) {
>   len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
>   sizeof(kctl->id.name));
> - else
> + usb_audio_err(state->chip, "[DEBUG] len:%d, copy_string id.name:%s\n",
> + len, (len > 0) ? kctl->id.name : " ");
> + }
> + else {
>   len = get_term_name(state, >oterm,
>   kctl->id.name, sizeof(kctl->id.name), 0);
> + usb_audio_err(state->chip, "[DEBUG] len:%d, get_term_name:%s\n",
> + len, (len > 0) ? kctl->id.name : " ");
> + }
> 
>   if (!len) {
>   strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
> @@ -2182,7 +2195,7 @@ static int parse_audio_selector_unit(struct
> mixer_build *state, int unitid,
>   append_ctl_name(kctl, " Playback Source");
>   }
> 
> - usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
> + usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
>   cval->head.id, kctl->id.name, desc->bNrInPins);
>   return snd_usb_mixer_add_control(>head, kctl);
>  }
> 
> 
> 2017-12-19 2:19 GMT+09:00 Jaejoong Kim :
>> 2017-12-19 2:13 GMT+09:00 Takashi Iwai :
>>> On Mon, 18 Dec 2017 18:05:18 +0100,
>>> Mauro Santos wrote:

 On 18-12-2017 15:45, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 16:30:13 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 13:53, Takashi Iwai wrote:
>>> On Mon, 18 Dec 2017 14:44:44 +0100,
>>> Greg KH wrote:

 On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> I believe this is the right place to report this problem, but if it
> isn't please point me in the right direction.

 Adding the developer of that patch, and the sound maintainer and
 developers to the thread.

> I have noticed that after the update from kernel 4.14.5 to 4.14.6
> alsamixer does not show the usual volume controls for my usb 
> soundcard.
>
> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: 
> Add
> check return value for usb_string() (from linux-stable) makes the
> controls come back again with kernel 4.14.6.
>>> (snip)

 This is odd, Takashi, I 

Re: [PATCH 2/5 v3] Modify behaviour of request_*muxed_region()

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán 
> 
> In order to make request_*muxed_region() behave more like
> mutex_lock(), a possible failure case needs to be eliminated.
> When drivers do not properly share the same I/O region, e.g.
> one is using request_region() and the other is using
> request_muxed_region(), the kernel didn't warn the user about it.
> This change modifies IORESOURCE_MUXED behaviour so it always
> goes to sleep waiting for the resuorce to be freed and the

resource

> inconsistent resource flag usage is logged with KERN_ERR.
> 
> v2: Fixed checkpatch.pl warnings and extended the comment
> about request_declared_muxed_region.
> 
> v3: Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi 
> ---
>  kernel/resource.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 05db9c4e3992..0f26f887ac33 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1143,6 +1143,7 @@ resource_size_t resource_alignment(struct resource *res)
>   *
>   * request_declared_muxed_region creates a new shared busy region
>   * described in an existing resource descriptor.
> + * It only returns if it succeeded.
>   *
>   * release_region releases a matching busy region.
>   * The region is only freed if it was allocated.
> @@ -1209,7 +1210,10 @@ struct resource *__request_declared_region(struct 
> resource *parent,
>   continue;
>   }
>   }
> - if (conflict->flags & flags & IORESOURCE_MUXED) {
> + if (flags & IORESOURCE_MUXED) {
> + if (!(conflict->flags & IORESOURCE_MUXED))
> + pr_err("Resource conflict between muxed \"%s\" 
> and non-muxed \"%s\" I/O regions!\n",
> + res->name, conflict->name);

With this, the muxed resource requestor will hang since the non-muxed
requestor will not release the resource. I understand that you are trying
to get rid of the error return, but is replacing it with a hang really
better ?

>   add_wait_queue(_resource_wait, );
>   write_unlock(_lock);
>   set_current_state(TASK_UNINTERRUPTIBLE);
> -- 
> 2.13.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5 v3] Extend the request_region() infrastructure

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:37AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán 
> 
> Add a new IORESOURCE_ALLOCATED flag that is automatically used
> when alloc_resource() is used internally in kernel/resource.c
> and free_resource() now takes this flag into account.
> 
> The core of __request_region() was factored out into a new function
> called __request_declared_region() that needs struct resource *
> instead of the (start, n, name) triplet.
> 
> These changes allow using statically declared struct resource
> data coupled with the pre-existing DEFINE_RES_IO_NAMED() static
> initializer macro. The new macro exploiting
> __request_declared_region() is request_declared_muxed_region()
> 
> v2:
> 
> Fixed checkpatch.pl warnings and errors and extended the macro
> API with request_declared_region() and release_declared_region()
> 
> Reversed the order of __request_declared_region and __request_region
> 
> Added high level description of the muxed and declared variants
> of the macros.
> 
> v3:
> 
> Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi 
> ---
>  include/linux/ioport.h | 14 ++
>  kernel/resource.c  | 40 +---
>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 93b4183cf53d..d198d6a58574 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -53,6 +53,7 @@ struct resource {
>  #define IORESOURCE_MEM_640x0010
>  #define IORESOURCE_WINDOW0x0020  /* forwarded by bridge */
>  #define IORESOURCE_MUXED 0x0040  /* Resource is software muxed */
> +#define IORESOURCE_ALLOCATED 0x0080  /* Resource was allocated */
>  
>  #define IORESOURCE_EXT_TYPE_BITS 0x0100  /* Resource extended types */
>  #define IORESOURCE_SYSRAM0x0100  /* System RAM (modifier) */
> @@ -218,7 +219,14 @@ static inline bool resource_contains(struct resource 
> *r1, struct resource *r2)
>  
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name) 
> __request_region(_resource, (start), (n), (name), 0)
> +#define request_declared_region(res) __request_region( \
> + _resource, \
> + (res), 0)
>  #define request_muxed_region(start,n,name)   
> __request_region(_resource, (start), (n), (name), IORESOURCE_MUXED)
> +#define request_declared_muxed_region(res)   __request_declared_region( \
> + _resource, \
> + (res), \
> + IORESOURCE_MUXED)
>  #define __request_mem_region(start,n,name, excl) 
> __request_region(_resource, (start), (n), (name), excl)
>  #define request_mem_region(start,n,name) __request_region(_resource, 
> (start), (n), (name), 0)
>  #define request_mem_region_exclusive(start,n,name) \
> @@ -230,8 +238,14 @@ extern struct resource * __request_region(struct 
> resource *,
>   resource_size_t n,
>   const char *name, int flags);
>  
> +extern struct resource *__request_declared_region(struct resource *parent,
> + struct resource *res, int flags);
> +
>  /* Compatibility cruft */
>  #define release_region(start,n)  __release_region(_resource, 
> (start), (n))
> +#define release_declared_region(res) __release_region(_resource, \
> + (res)->start, \
> + (res)->end - (res)->start + 1)
>  #define release_mem_region(start,n)  __release_region(_resource, 
> (start), (n))
>  
>  extern void __release_region(struct resource *, resource_size_t,
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 54ba6de3757c..05db9c4e3992 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -184,6 +184,9 @@ static void free_resource(struct resource *res)
>   if (!res)
>   return;
>  
> + if (!(res->flags & IORESOURCE_ALLOCATED))
> + return;
> +
>   if (!PageSlab(virt_to_head_page(res))) {
>   spin_lock(_resource_lock);
>   res->sibling = bootmem_resource_free;
> @@ -210,6 +213,8 @@ static struct resource *alloc_resource(gfp_t flags)
>   else
>   res = kzalloc(sizeof(struct resource), flags);
>  
> + res->flags = IORESOURCE_ALLOCATED;
> +

kzalloc() can fail, thus res can be NULL.

>   return res;
>  }
>  
> @@ -1128,8 +1133,19 @@ resource_size_t resource_alignment(struct resource 
> *res)
>   * the IO flag meanings (busy etc).
>   *
>   * request_region creates a new busy region.
> + * The resource descriptor is allocated by this function.
> + *
> + * request_declared_region creates a new busy region
> + * 

Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5

[Bjørn Mork]

Johan Hovold  writes:

>> +static const struct option_blacklist_info yuga_clm920_nc5_blacklist = {
>> +.reserved = BIT(0) | BIT(1) | BIT(4),
>> +};
>
> Do you really need to blacklist the first interface?

Good question. Interface #0 does look a lot like a Qualcomm DM/DIAG
function, based on two bulk endpoints, no additional descriptors and the
fact that it is the first interface.  If so, then we do want a serial
driver for it.  There is a basic libqcdm implementation in ModemManager
if you want to test it out.

And I expect interface #4 is QMI/rmnet?  Feel free to confirm that
assumption with a patch against qmi_wwan :-)


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 16:59, Jaejoong Kim wrote:
> Mauro,
> thanks for the report and sorry for the make problem.
> 
> Could you try below debug patch? I add more debug code with Takashi's 
> guideline.
> 

I get this when I plug the device directly to a usb3 port:
[   63.643706] usb 1-2: new full-speed USB device number 7 using xhci_hcd
[   63.767089] usb 1-2: device descriptor read/64, error -71
[   64.100906] input: HiFimeDIY Audio HiFimeDIY DAC as
/devices/pci:00/:00:14.0/usb1/1-2/1-2:1.0/0003:1852:7022.0003/input/input20
[   64.101208] hid-generic 0003:1852:7022.0003: input,hidraw2: USB HID
v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-2/input0
[   64.208385] usb 1-2: [DEBUG] nameid:0, len:0
[   64.208390] usb 1-2: [DEBUG] len:3, get_term_name:PCM
[   64.208393] usb 1-2: [11] SU [PCM] items = 2
[   64.208881] usbcore: registered new interface driver snd-usb-audio

If I plug it to a usb2 hub where I usually have it connected the output
is almost the same:
[  129.159898] usb 1-3.3: new full-speed USB device number 8 using xhci_hcd
[  129.246587] usb 1-3.3: device descriptor read/64, error -32
[  129.532552] input: HiFimeDIY Audio HiFimeDIY DAC as
/devices/pci:00/:00:14.0/usb1/1-3/1-3.3/1-3.3:1.0/0003:1852:7022.0004/input/input21
[  129.532790] hid-generic 0003:1852:7022.0004: input,hidraw2: USB HID
v1.00 Device [HiFimeDIY Audio HiFimeDIY DAC] on usb-:00:14.0-3.3/input0
[  129.560090] usb 1-3.3: [DEBUG] nameid:0, len:0
[  129.560096] usb 1-3.3: [DEBUG] len:3, get_term_name:PCM
[  129.560100] usb 1-3.3: [11] SU [PCM] items = 2

I'm compiling now a kernel with your other patch, I'll get back to you
with all the information I can gather on the device once I test with the
other patch.

> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 84b9f9c..0233425 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -595,7 +595,7 @@ int snd_usb_mixer_add_control(struct
> usb_mixer_elem_list *list,
> while (snd_ctl_find_id(mixer->chip->card, >id))
> kctl->id.index++;
> if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
> -   usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
> +   usb_audio_err(mixer->chip, "[DEBUG] cannot add control
> (err = %d)\n",
>   err);
> return err;
> }
> @@ -2162,14 +2162,23 @@ static int parse_audio_selector_unit(struct
> mixer_build *state, int unitid,
> 
> nameid = uac_selector_unit_iSelector(desc);
> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> +
> +   usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, 
> len);
> +
> if (len)
> ;
> -   else if (nameid)
> +   else if (nameid) {
> len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
>  sizeof(kctl->id.name));
> -   else
> +   usb_audio_err(state->chip, "[DEBUG] len:%d,
> copy_string id.name:%s\n",
> +   len, (len > 0) ? kctl->id.name : " ");
> +   }
> +   else {
> len = get_term_name(state, >oterm,
> kctl->id.name, sizeof(kctl->id.name), 0);
> +   usb_audio_err(state->chip, "[DEBUG] len:%d, 
> get_term_name:%s\n",
> +   len, (len > 0) ? kctl->id.name : " ");
> +   }
> 
> if (!len) {
> strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
> @@ -2182,7 +2191,7 @@ static int parse_audio_selector_unit(struct
> mixer_build *state, int unitid,
> append_ctl_name(kctl, " Playback Source");
> }
> 
> -   usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
> +   usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
> cval->head.id, kctl->id.name, desc->bNrInPins);
> return snd_usb_mixer_add_control(>head, kctl);
>  }
> 
> 
> 2017-12-19 0:45 GMT+09:00 Takashi Iwai :
>> On Mon, 18 Dec 2017 16:30:13 +0100,
>> Mauro Santos wrote:
>>>
>>> On 18-12-2017 13:53, Takashi Iwai wrote:
 On Mon, 18 Dec 2017 14:44:44 +0100,
 Greg KH wrote:
>
> On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
>> I believe this is the right place to report this problem, but if it
>> isn't please point me in the right direction.
>
> Adding the developer of that patch, and the sound maintainer and
> developers to the thread.
>
>> I have noticed that after the update from kernel 4.14.5 to 4.14.6
>> alsamixer does not show the usual volume controls for my usb soundcard.
>>
>> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
>> check return value for usb_string() (from linux-stable) makes the
>> controls come back again with kernel 4.14.6.
 (snip)
>
> This is odd, Takashi, I thought 

Re: [PATCH] usb: Add device quirk for Logitech HD Pro Webcam C925e

[Kai-Heng Feng]

Hi, 

> On 19 Dec 2017, at 12:04 AM, Dmitry Fleytman  
> wrote:
> 
> From: Dmitry Fleytman Dmitry Fleytman 
> 
> Commit e0429362ab15
> ("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
> introduced quirk to workaround an issue with some Logitech webcams.
> 
> There is one more model that has the same issue - C925e, so applying
> the same quirk as well.
> 
> See aforementioned commit message for detailed explanation of the problem.
> 
> Signed-off-by: Dmitry Fleytman 
> ---
> drivers/usb/core/quirks.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index a10b346b9777..6d2d3b0bcc2a 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -52,10 +52,11 @@ static const struct usb_device_id usb_quirk_list[] = {
>   /* Microsoft LifeCam-VX700 v2.0 */
>   { USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
> 
> - /* Logitech HD Pro Webcams C920, C920-C and C930e */
> + /* Logitech HD Pro Webcams C920, C920-C, C925e and C930e */
>   { USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
>   { USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
>   { USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
> + { USB_DEVICE(0x046d, 0x085B), .driver_info = USB_QUIRK_DELAY_INIT },

Nitpicking here, but maybe use 0x085b instead of 0x085B?

Kai-Heng

> 
>   /* Logitech ConferenceCam CC3000e */
>   { USB_DEVICE(0x046d, 0x0847), .driver_info = USB_QUIRK_DELAY_INIT },
> -- 
> 2.14.1
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: pd: fix the offset for SVID specific commands

[Heikki Krogerus]

On Mon, Dec 18, 2017 at 03:08:35PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 18, 2017 at 05:03:03PM +0300, Heikki Krogerus wrote:
> > The SVID specific commands in the Command field of the
> > Structured VDM Header start from 16, not 10. Changing the
> > value used in VDO_CMD_VENDOR() macro from 10 to 0x10.
> > 
> > Signed-off-by: Heikki Krogerus 
> 
> Does this fix any specific code that uses this macro?  Should it go to
> the stable tree(s)?

No, nothing uses the macro yet.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Jaejoong Kim]

Mauro,

Could you please try debug patch(I also attach the patch file)?

And can you share your usb audio dac? Is this same device with yours?
https://hifimediy.com/index.php?route=product/product_id=83

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..6200aa2 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -595,7 +595,7 @@ int snd_usb_mixer_add_control(struct
usb_mixer_elem_list *list,
  while (snd_ctl_find_id(mixer->chip->card, >id))
  kctl->id.index++;
  if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
- usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
+ usb_audio_err(mixer->chip, "[DEBUG] cannot add control (err = %d)\n",
err);
  return err;
  }
@@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build
*state, struct usb_audio_term *iterm
  unsigned char *name, int maxlen, int term_only)
 {
  struct iterm_name_combo *names;
+ int len;

- if (iterm->name)
- return snd_usb_copy_string_desc(state, iterm->name,
+ if (iterm->name) {
+ len = snd_usb_copy_string_desc(state, iterm->name,
  name, maxlen);
+ usb_audio_err(state->chip, "[DEBUG] len:%d, iterm->name:%d\n", len,
iterm->name);
+ return len;
+ }

  /* virtual type - not a real terminal */
  if (iterm->type >> 16) {
@@ -2162,14 +2166,23 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,

  nameid = uac_selector_unit_iSelector(desc);
  len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+
+ usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);
+
  if (len)
  ;
- else if (nameid)
+ else if (nameid) {
  len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
  sizeof(kctl->id.name));
- else
+ usb_audio_err(state->chip, "[DEBUG] len:%d, copy_string id.name:%s\n",
+ len, (len > 0) ? kctl->id.name : " ");
+ }
+ else {
  len = get_term_name(state, >oterm,
  kctl->id.name, sizeof(kctl->id.name), 0);
+ usb_audio_err(state->chip, "[DEBUG] len:%d, get_term_name:%s\n",
+ len, (len > 0) ? kctl->id.name : " ");
+ }

  if (!len) {
  strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
@@ -2182,7 +2195,7 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,
  append_ctl_name(kctl, " Playback Source");
  }

- usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
+ usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
  cval->head.id, kctl->id.name, desc->bNrInPins);
  return snd_usb_mixer_add_control(>head, kctl);
 }


2017-12-19 2:19 GMT+09:00 Jaejoong Kim :
> 2017-12-19 2:13 GMT+09:00 Takashi Iwai :
>> On Mon, 18 Dec 2017 18:05:18 +0100,
>> Mauro Santos wrote:
>>>
>>> On 18-12-2017 15:45, Takashi Iwai wrote:
>>> > On Mon, 18 Dec 2017 16:30:13 +0100,
>>> > Mauro Santos wrote:
>>> >>
>>> >> On 18-12-2017 13:53, Takashi Iwai wrote:
>>> >>> On Mon, 18 Dec 2017 14:44:44 +0100,
>>> >>> Greg KH wrote:
>>> 
>>>  On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
>>> > I believe this is the right place to report this problem, but if it
>>> > isn't please point me in the right direction.
>>> 
>>>  Adding the developer of that patch, and the sound maintainer and
>>>  developers to the thread.
>>> 
>>> > I have noticed that after the update from kernel 4.14.5 to 4.14.6
>>> > alsamixer does not show the usual volume controls for my usb 
>>> > soundcard.
>>> >
>>> > Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: 
>>> > Add
>>> > check return value for usb_string() (from linux-stable) makes the
>>> > controls come back again with kernel 4.14.6.
>>> >>> (snip)
>>> 
>>>  This is odd, Takashi, I thought we fixed up the problem that if the
>>>  string was invalid, the code would continue to go on, it's not a "real"
>>>  error.  Did that not get marked for the stable trees?
>>> >>>
>>> >>> Yes, it was marked as stable, and it's odd that the revert of the
>>> >>> given commit changes the behavior in that way.
>>> >>>
>>> >>> Mauro, could you double-check whether reverting the commit really does
>>> >>> fix it as expected?  If yes, could you put some debug print at the
>>> >>> part the patch touches, and check which unit id gives len=0 from
>>> >>> snd_usb_copy_string_desc()?
>>> >>
>>> >> I'm sure reverting that patch makes things work as expected. I noticed
>>> >> the problem after an update and that is the only thing I revert on top
>>> >> of the kernel provided by the distro (Arch Linux).
>>> >
>>> > Did you revert only one patch, not both patches?
>>> > Just to be sure.
>>>
>>> I have reverted only one patch.
>>>
>>> >> Alsamixer works fine for the built in soundcard in my laptop, but the
>>> >> mixer for the usb soundcard was not working so it's specific to usb and
>>> >> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
>>> >> tried reversing both, one at a time, and only reverting this one
>>> >> restored the expected behavior.
>>> >>
>>> >> 

Re: [PATCH] usb: pd: fix the offset for SVID specific commands

[Guenter Roeck]

On Mon, Dec 18, 2017 at 05:03:03PM +0300, Heikki Krogerus wrote:
> The SVID specific commands in the Command field of the
> Structured VDM Header start from 16, not 10. Changing the
> value used in VDO_CMD_VENDOR() macro from 10 to 0x10.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  include/linux/usb/pd_vdo.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
> index d92259f8de0a..2b64d23ace5c 100644
> --- a/include/linux/usb/pd_vdo.h
> +++ b/include/linux/usb/pd_vdo.h
> @@ -65,7 +65,7 @@
>  #define CMD_EXIT_MODE5
>  #define CMD_ATTENTION6
>  
> -#define VDO_CMD_VENDOR(x)(((10 + (x)) & 0x1f))
> +#define VDO_CMD_VENDOR(x)(((0x10 + (x)) & 0x1f))
>  
Good catch. Bad part is that this originates from
https://chromium.googlesource.com/chromiumos/platform/ec,
which uses 10 as starting point. Nothing we can do about that.

Anyway,

Reviewed-by: Guenter Roeck 

>  /* ChromeOS specific commands */
>  #define VDO_CMD_VERSION  VDO_CMD_VENDOR(0)
> -- 
> 2.15.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Jaejoong Kim]

2017-12-19 2:13 GMT+09:00 Takashi Iwai :
> On Mon, 18 Dec 2017 18:05:18 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 15:45, Takashi Iwai wrote:
>> > On Mon, 18 Dec 2017 16:30:13 +0100,
>> > Mauro Santos wrote:
>> >>
>> >> On 18-12-2017 13:53, Takashi Iwai wrote:
>> >>> On Mon, 18 Dec 2017 14:44:44 +0100,
>> >>> Greg KH wrote:
>> 
>>  On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
>> > I believe this is the right place to report this problem, but if it
>> > isn't please point me in the right direction.
>> 
>>  Adding the developer of that patch, and the sound maintainer and
>>  developers to the thread.
>> 
>> > I have noticed that after the update from kernel 4.14.5 to 4.14.6
>> > alsamixer does not show the usual volume controls for my usb soundcard.
>> >
>> > Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
>> > check return value for usb_string() (from linux-stable) makes the
>> > controls come back again with kernel 4.14.6.
>> >>> (snip)
>> 
>>  This is odd, Takashi, I thought we fixed up the problem that if the
>>  string was invalid, the code would continue to go on, it's not a "real"
>>  error.  Did that not get marked for the stable trees?
>> >>>
>> >>> Yes, it was marked as stable, and it's odd that the revert of the
>> >>> given commit changes the behavior in that way.
>> >>>
>> >>> Mauro, could you double-check whether reverting the commit really does
>> >>> fix it as expected?  If yes, could you put some debug print at the
>> >>> part the patch touches, and check which unit id gives len=0 from
>> >>> snd_usb_copy_string_desc()?
>> >>
>> >> I'm sure reverting that patch makes things work as expected. I noticed
>> >> the problem after an update and that is the only thing I revert on top
>> >> of the kernel provided by the distro (Arch Linux).
>> >
>> > Did you revert only one patch, not both patches?
>> > Just to be sure.
>>
>> I have reverted only one patch.
>>
>> >> Alsamixer works fine for the built in soundcard in my laptop, but the
>> >> mixer for the usb soundcard was not working so it's specific to usb and
>> >> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
>> >> tried reversing both, one at a time, and only reverting this one
>> >> restored the expected behavior.
>> >>
>> >> Regarding adding the debug print I'm going to need guidance. Without
>> >> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
>> >> following be enough?
>> >>
>> >> printk("usbmixdbg: nameid=%d len=%d 
>> >> id.name=%s\n",nameid,len,kctl->id.name);
>> >>
>> >> It would then look like this (minus the line wrapping):
>> >> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>> >> printk("usbmixdbg: nameid=%d len=%d 
>> >> id.name=%s\n",nameid,len,kctl->id.name);
>> >> if (len)
>> >
>> > Well, at that point, there should be no difference.
>> > The difference is after that, so what I'd like to see is something
>> > like:
>> >
>> > --- a/sound/usb/mixer.c
>> > +++ b/sound/usb/mixer.c
>> > @@ -2175,14 +2175,18 @@ static int parse_audio_selector_unit(struct 
>> > mixer_build *state, int unitid,
>> >
>> > nameid = uac_selector_unit_iSelector(desc);
>> > len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>> > +   pr_info("XXX id=%d, check_mapped_name=%d\n", id, len);
>> > if (len)
>> > ;
>> > -   else if (nameid)
>> > +   else if (nameid) {
>> > len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
>> >  sizeof(kctl->id.name));
>> > -   else
>> > +   pr_info("XXX id=%d, copy_string=%d\n", len);
>> > +   } else {
>> > len = get_term_name(state, >oterm,
>> > kctl->id.name, sizeof(kctl->id.name), 0);
>> > +   pr_info("XXX id=%d, get_term_name=%d\n", len);
>> > +   }
>> >
>> > if (!len) {
>> > strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
>> >
>> >
>> > If you see copy_string=0, it means that your hardware reports a bogus
>> > entry, and the driver does it correctly.  If ignoring that error
>> > really restores the old behavior back, it essentially means that it
>> > worked casually in the past...
>>
>> I have applied your patch on top of 4.14.7 without reverting anything
>> and I was planning to reply only after I had some result but building
>> failed (without any errors strangely).
>>
>> I took a second look at your patch and I have a (maybe silly/naive)
>> question, don't the second and third pr_info calls need an extra
>> argument? There are two %d in the string but only one variable (len).
>
> Yeah, sure, of course you need them :)
> I haven't tested the patch, but just to give you an idea.
> Sorry if you wasted your time due to that.

OK. I will make a debug patch with you last debug code.

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -656,10 +656,14 @@ 

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 18:05:18 +0100,
Mauro Santos wrote:
> 
> On 18-12-2017 15:45, Takashi Iwai wrote:
> > On Mon, 18 Dec 2017 16:30:13 +0100,
> > Mauro Santos wrote:
> >>
> >> On 18-12-2017 13:53, Takashi Iwai wrote:
> >>> On Mon, 18 Dec 2017 14:44:44 +0100,
> >>> Greg KH wrote:
> 
>  On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> > I believe this is the right place to report this problem, but if it
> > isn't please point me in the right direction.
> 
>  Adding the developer of that patch, and the sound maintainer and
>  developers to the thread.
> 
> > I have noticed that after the update from kernel 4.14.5 to 4.14.6
> > alsamixer does not show the usual volume controls for my usb soundcard.
> >
> > Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
> > check return value for usb_string() (from linux-stable) makes the
> > controls come back again with kernel 4.14.6.
> >>> (snip)
> 
>  This is odd, Takashi, I thought we fixed up the problem that if the
>  string was invalid, the code would continue to go on, it's not a "real"
>  error.  Did that not get marked for the stable trees?
> >>>
> >>> Yes, it was marked as stable, and it's odd that the revert of the
> >>> given commit changes the behavior in that way.
> >>>
> >>> Mauro, could you double-check whether reverting the commit really does
> >>> fix it as expected?  If yes, could you put some debug print at the
> >>> part the patch touches, and check which unit id gives len=0 from
> >>> snd_usb_copy_string_desc()?
> >>
> >> I'm sure reverting that patch makes things work as expected. I noticed
> >> the problem after an update and that is the only thing I revert on top
> >> of the kernel provided by the distro (Arch Linux).
> > 
> > Did you revert only one patch, not both patches?
> > Just to be sure.
> 
> I have reverted only one patch.
> 
> >> Alsamixer works fine for the built in soundcard in my laptop, but the
> >> mixer for the usb soundcard was not working so it's specific to usb and
> >> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
> >> tried reversing both, one at a time, and only reverting this one
> >> restored the expected behavior.
> >>
> >> Regarding adding the debug print I'm going to need guidance. Without
> >> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
> >> following be enough?
> >>
> >> printk("usbmixdbg: nameid=%d len=%d 
> >> id.name=%s\n",nameid,len,kctl->id.name);
> >>
> >> It would then look like this (minus the line wrapping):
> >> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> >> printk("usbmixdbg: nameid=%d len=%d 
> >> id.name=%s\n",nameid,len,kctl->id.name);
> >> if (len)
> > 
> > Well, at that point, there should be no difference.
> > The difference is after that, so what I'd like to see is something
> > like:
> > 
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -2175,14 +2175,18 @@ static int parse_audio_selector_unit(struct 
> > mixer_build *state, int unitid,
> >  
> > nameid = uac_selector_unit_iSelector(desc);
> > len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> > +   pr_info("XXX id=%d, check_mapped_name=%d\n", id, len);
> > if (len)
> > ;
> > -   else if (nameid)
> > +   else if (nameid) {
> > len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
> >  sizeof(kctl->id.name));
> > -   else
> > +   pr_info("XXX id=%d, copy_string=%d\n", len);
> > +   } else {
> > len = get_term_name(state, >oterm,
> > kctl->id.name, sizeof(kctl->id.name), 0);
> > +   pr_info("XXX id=%d, get_term_name=%d\n", len);
> > +   }
> >  
> > if (!len) {
> > strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
> > 
> > 
> > If you see copy_string=0, it means that your hardware reports a bogus
> > entry, and the driver does it correctly.  If ignoring that error
> > really restores the old behavior back, it essentially means that it
> > worked casually in the past...
> 
> I have applied your patch on top of 4.14.7 without reverting anything
> and I was planning to reply only after I had some result but building
> failed (without any errors strangely).
> 
> I took a second look at your patch and I have a (maybe silly/naive)
> question, don't the second and third pr_info calls need an extra
> argument? There are two %d in the string but only one variable (len).

Yeah, sure, of course you need them :)
I haven't tested the patch, but just to give you an idea.
Sorry if you wasted your time due to that.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 17:59:38 +0100,
Jaejoong Kim wrote:
> 
> AudioControl Interface Descriptor:
> bLength 8
> bDescriptorType36
> bDescriptorSubtype  5 (SELECTOR_UNIT)
> bUnitID11
> bNrInPins   2
> baSource( 0)   14
> baSource( 1)5
> iSelector   0
> ^
> 
> >From the lsusb.txt, iSelector is '0' which means
> uac_selector_unit_iSelector() return zero I think.

Yes, and in that case, nameid=0, thus it should go to
get_term_name(), and there must be no difference.
If any difference came from the given commit, it must be nameid!=0,
and yet snd_usb_copy_string_desc(namei) returns an error.

In anyway, another potential fix would be the patch like below.
It covers the case where an error is returned from
snb_usb_copy_string_desc() and doesn't fall back to the default name
resolution.


thanks,

Takashi

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -656,10 +656,14 @@ static int get_term_name(struct mixer_build *state, 
struct usb_audio_term *iterm
 unsigned char *name, int maxlen, int term_only)
 {
struct iterm_name_combo *names;
+   int len;
 
-   if (iterm->name)
-   return snd_usb_copy_string_desc(state, iterm->name,
+   if (iterm->name) {
+   len = snd_usb_copy_string_desc(state, iterm->name,
name, maxlen);
+   if (len)
+   return len;
+   }
 
/* virtual type - not a real terminal */
if (iterm->type >> 16) {
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 15:45, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 16:30:13 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 13:53, Takashi Iwai wrote:
>>> On Mon, 18 Dec 2017 14:44:44 +0100,
>>> Greg KH wrote:

 On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> I believe this is the right place to report this problem, but if it
> isn't please point me in the right direction.

 Adding the developer of that patch, and the sound maintainer and
 developers to the thread.

> I have noticed that after the update from kernel 4.14.5 to 4.14.6
> alsamixer does not show the usual volume controls for my usb soundcard.
>
> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
> check return value for usb_string() (from linux-stable) makes the
> controls come back again with kernel 4.14.6.
>>> (snip)

 This is odd, Takashi, I thought we fixed up the problem that if the
 string was invalid, the code would continue to go on, it's not a "real"
 error.  Did that not get marked for the stable trees?
>>>
>>> Yes, it was marked as stable, and it's odd that the revert of the
>>> given commit changes the behavior in that way.
>>>
>>> Mauro, could you double-check whether reverting the commit really does
>>> fix it as expected?  If yes, could you put some debug print at the
>>> part the patch touches, and check which unit id gives len=0 from
>>> snd_usb_copy_string_desc()?
>>
>> I'm sure reverting that patch makes things work as expected. I noticed
>> the problem after an update and that is the only thing I revert on top
>> of the kernel provided by the distro (Arch Linux).
> 
> Did you revert only one patch, not both patches?
> Just to be sure.

I have reverted only one patch.

>> Alsamixer works fine for the built in soundcard in my laptop, but the
>> mixer for the usb soundcard was not working so it's specific to usb and
>> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
>> tried reversing both, one at a time, and only reverting this one
>> restored the expected behavior.
>>
>> Regarding adding the debug print I'm going to need guidance. Without
>> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
>> following be enough?
>>
>> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>>
>> It would then look like this (minus the line wrapping):
>> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>> if (len)
> 
> Well, at that point, there should be no difference.
> The difference is after that, so what I'd like to see is something
> like:
> 
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2175,14 +2175,18 @@ static int parse_audio_selector_unit(struct 
> mixer_build *state, int unitid,
>  
>   nameid = uac_selector_unit_iSelector(desc);
>   len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> + pr_info("XXX id=%d, check_mapped_name=%d\n", id, len);
>   if (len)
>   ;
> - else if (nameid)
> + else if (nameid) {
>   len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
>sizeof(kctl->id.name));
> - else
> + pr_info("XXX id=%d, copy_string=%d\n", len);
> + } else {
>   len = get_term_name(state, >oterm,
>   kctl->id.name, sizeof(kctl->id.name), 0);
> + pr_info("XXX id=%d, get_term_name=%d\n", len);
> + }
>  
>   if (!len) {
>   strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
> 
> 
> If you see copy_string=0, it means that your hardware reports a bogus
> entry, and the driver does it correctly.  If ignoring that error
> really restores the old behavior back, it essentially means that it
> worked casually in the past...

I have applied your patch on top of 4.14.7 without reverting anything
and I was planning to reply only after I had some result but building
failed (without any errors strangely).

I took a second look at your patch and I have a (maybe silly/naive)
question, don't the second and third pr_info calls need an extra
argument? There are two %d in the string but only one variable (len).

-- 
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Jaejoong Kim]

Mauro,
thanks for the report and sorry for the make problem.

Could you try below debug patch? I add more debug code with Takashi's guideline.

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 84b9f9c..0233425 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -595,7 +595,7 @@ int snd_usb_mixer_add_control(struct
usb_mixer_elem_list *list,
while (snd_ctl_find_id(mixer->chip->card, >id))
kctl->id.index++;
if ((err = snd_ctl_add(mixer->chip->card, kctl)) < 0) {
-   usb_audio_dbg(mixer->chip, "cannot add control (err = %d)\n",
+   usb_audio_err(mixer->chip, "[DEBUG] cannot add control
(err = %d)\n",
  err);
return err;
}
@@ -2162,14 +2162,23 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,

nameid = uac_selector_unit_iSelector(desc);
len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+
+   usb_audio_err(state->chip, "[DEBUG] nameid:%d, len:%d\n", nameid, len);
+
if (len)
;
-   else if (nameid)
+   else if (nameid) {
len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
 sizeof(kctl->id.name));
-   else
+   usb_audio_err(state->chip, "[DEBUG] len:%d,
copy_string id.name:%s\n",
+   len, (len > 0) ? kctl->id.name : " ");
+   }
+   else {
len = get_term_name(state, >oterm,
kctl->id.name, sizeof(kctl->id.name), 0);
+   usb_audio_err(state->chip, "[DEBUG] len:%d, get_term_name:%s\n",
+   len, (len > 0) ? kctl->id.name : " ");
+   }

if (!len) {
strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
@@ -2182,7 +2191,7 @@ static int parse_audio_selector_unit(struct
mixer_build *state, int unitid,
append_ctl_name(kctl, " Playback Source");
}

-   usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n",
+   usb_audio_err(state->chip, "[%d] SU [%s] items = %d\n",
cval->head.id, kctl->id.name, desc->bNrInPins);
return snd_usb_mixer_add_control(>head, kctl);
 }


2017-12-19 0:45 GMT+09:00 Takashi Iwai :
> On Mon, 18 Dec 2017 16:30:13 +0100,
> Mauro Santos wrote:
>>
>> On 18-12-2017 13:53, Takashi Iwai wrote:
>> > On Mon, 18 Dec 2017 14:44:44 +0100,
>> > Greg KH wrote:
>> >>
>> >> On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
>> >>> I believe this is the right place to report this problem, but if it
>> >>> isn't please point me in the right direction.
>> >>
>> >> Adding the developer of that patch, and the sound maintainer and
>> >> developers to the thread.
>> >>
>> >>> I have noticed that after the update from kernel 4.14.5 to 4.14.6
>> >>> alsamixer does not show the usual volume controls for my usb soundcard.
>> >>>
>> >>> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
>> >>> check return value for usb_string() (from linux-stable) makes the
>> >>> controls come back again with kernel 4.14.6.
>> > (snip)
>> >>
>> >> This is odd, Takashi, I thought we fixed up the problem that if the
>> >> string was invalid, the code would continue to go on, it's not a "real"
>> >> error.  Did that not get marked for the stable trees?
>> >
>> > Yes, it was marked as stable, and it's odd that the revert of the
>> > given commit changes the behavior in that way.
>> >
>> > Mauro, could you double-check whether reverting the commit really does
>> > fix it as expected?  If yes, could you put some debug print at the
>> > part the patch touches, and check which unit id gives len=0 from
>> > snd_usb_copy_string_desc()?
>>
>> I'm sure reverting that patch makes things work as expected. I noticed
>> the problem after an update and that is the only thing I revert on top
>> of the kernel provided by the distro (Arch Linux).
>
> Did you revert only one patch, not both patches?
> Just to be sure.
>
>> Alsamixer works fine for the built in soundcard in my laptop, but the
>> mixer for the usb soundcard was not working so it's specific to usb and
>> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
>> tried reversing both, one at a time, and only reverting this one
>> restored the expected behavior.
>>
>> Regarding adding the debug print I'm going to need guidance. Without
>> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
>> following be enough?
>>
>> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>>
>> It would then look like this (minus the line wrapping):
>> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
>> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
>> if (len)
>
> Well, at that point, there should be no difference.
> The difference is 

Re: [PATCH] USB: serial: option: adding support for YUGA CLM920-NC5

[Johan Hovold]

[ +CC: Bjørn ]

On Fri, Dec 15, 2017 at 06:32:03PM +0800, SZ Lin wrote:
> This patch adds support for YUGA CLM920-NC5 PID 0x9625 USB modem to option
> driver.

Could you specify what each interface is used for here?

> Signed-off-by: Taiyi Wu 
> Signed-off-by: SZ Lin 

Is SZ your legal name?

> ---
> 
> Please refer to following lsusb output:
> 
> Bus 001 Device 003: ID 05c6:9625 Qualcomm, Inc.
> Device Descriptor:
>   bLength18
>   bDescriptorType 1
>   bcdUSB   2.00
>   bDeviceClass0 (Defined at Interface level)
>   bDeviceSubClass 0
>   bDeviceProtocol 0
>   bMaxPacketSize064
>   idVendor   0x05c6 Qualcomm, Inc.
>   idProduct  0x9625
>   bcdDevice3.18
>   iManufacturer   1 Shanghai Yuge
>   iProduct2 YUGA Incorporated
>   iSerial 3 0123456789ABCDEF
>   bNumConfigurations  1
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength  183
> bNumInterfaces  5
> bConfigurationValue 1
> iConfiguration  0
> bmAttributes 0xa0
>   (Bus Powered)
>   Remote Wakeup
> MaxPower  500mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
>   bNumEndpoints   2
>   bInterfaceClass   255 Vendor Specific Class
>   bInterfaceSubClass255 Vendor Specific Subclass
>   bInterfaceProtocol255 Vendor Specific Protocol
>   iInterface  0
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81  EP 1 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0200  1x 512 bytes
> bInterval   0
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x01  EP 1 OUT
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0200  1x 512 bytes
> bInterval   0
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber1
>   bAlternateSetting   0
>   bNumEndpoints   2
>   bInterfaceClass   255 Vendor Specific Class
>   bInterfaceSubClass 66
>   bInterfaceProtocol  1
>   iInterface  6 ADB Interface
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x02  EP 2 OUT
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0200  1x 512 bytes
> bInterval   0
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x82  EP 2 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0200  1x 512 bytes
> bInterval   0
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber2
>   bAlternateSetting   0
>   bNumEndpoints   3
>   bInterfaceClass   255 Vendor Specific Class
>   bInterfaceSubClass  0
>   bInterfaceProtocol  0
>   iInterface  0
>   ** UNRECOGNIZED:  05 24 00 10 01
>   ** UNRECOGNIZED:  05 24 01 00 00
>   ** UNRECOGNIZED:  04 24 02 02
>   ** UNRECOGNIZED:  05 24 06 00 00
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x84  EP 4 IN
> bmAttributes3
>   Transfer TypeInterrupt
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x000a  1x 10 bytes
> bInterval   9
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x83  EP 3 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0200  1x 512 bytes
> bInterval   0
>   Endpoint Descriptor:
>

Re: [PATCH V1 4/4] usb: serial: f81534: add H/W disable port support

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:09PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 can be disable port by manufacturer with
> following H/W design.
> 1: Connect DCD/DSR/CTS/RI pin to ground.
> 2: Connect RX pin to ground.
> 
> In driver, we'll implements some detect method likes following:
> 1: Read MSR.
> 2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms.
>It'll contain BREAK status in LSR.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
>  drivers/usb/serial/f81534.c | 74 
> +
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 30b966d71ae8..18bd2a478199 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -751,6 +751,74 @@ static int f81534_find_config_idx(struct usb_serial 
> *serial, u8 *index)
>  }
>  
>  /*
> + * The F81532/534 will not report serial port to USB serial subsystem when
> + * H/W DCD/DSR/CTS/RI/RX pin connected to ground.
> + *
> + * To detect RX pin status, we'll enable MCR interal loopback, disable it and
> + * delayed for 60ms. It connected to ground If LSR register report 
> UART_LSR_BI.
> + */
> +static int f81534_check_port_hw_disabled(struct usb_serial *serial, int phy)

You treat errors as "disabled" so return a bool instead.

> +{
> + int status;
> + u8 old_mcr;
> + u8 msr;
> + u8 lsr;
> + u8 msr_mask;
> +
> + msr_mask = UART_MSR_DCD | UART_MSR_RI | UART_MSR_DSR | UART_MSR_CTS;
> +
> + status = f81534_get_register(serial,
> + F81534_MODEM_STATUS_REG + phy * 0x10, );

You already have a define for the magic 0x10 that you should be using.

And you also have port register accessors that take care of the offset
for you. Perhaps add another helper which takes a phy num and use that
to implement the current accessor functions?

> + if (status)
> + return status;
> +
> + if ((msr & msr_mask) != msr_mask)
> + return 0;
> +
> + status = f81534_set_register(serial,
> + F81534_FIFO_CONTROL_REG + phy * 0x10,
> + UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> + UART_FCR_CLEAR_XMIT);
> + if (status)
> + return status;
> +
> + status = f81534_get_register(serial,
> + F81534_MODEM_CONTROL_REG + phy * 0x10,
> + _mcr);
> + if (status)
> + return status;
> +
> + status = f81534_set_register(serial,
> + F81534_MODEM_CONTROL_REG + phy * 0x10,
> + UART_MCR_LOOP);
> + if (status)
> + return status;
> +
> + status = f81534_set_register(serial,
> + F81534_MODEM_CONTROL_REG + phy * 0x10, 0x0);
> + if (status)
> + return status;
> +
> + msleep(60);
> +
> + status = f81534_get_register(serial,
> + F81534_LINE_STATUS_REG + phy * 0x10, );
> + if (status)
> + return status;
> +
> + status = f81534_set_register(serial,
> + F81534_MODEM_CONTROL_REG + phy * 0x10,
> + old_mcr);
> + if (status)
> + return status;
> +
> + if ((lsr & UART_LSR_BI) == UART_LSR_BI)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +/*
>   * We had 2 generation of F81532/534 IC. All has an internal storage.
>   *
>   * 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any
> @@ -832,6 +900,9 @@ static int f81534_calc_num_ports(struct usb_serial 
> *serial,
>  
>   /* New style, find all possible ports */
>   for (i = 0; i < F81534_NUM_PORT; ++i) {
> + if (f81534_check_port_hw_disabled(serial, i))
> + continue;
> +
>   if (setting[i] & F81534_PORT_UNAVAILABLE)
>   continue;
>  
> @@ -1306,6 +1377,9 @@ static int f81534_attach(struct usb_serial *serial)
>  
>   /* Assign phy-to-logic mapping */
>   for (i = 0; i < F81534_NUM_PORT; ++i) {
> + if (f81534_check_port_hw_disabled(serial, i))
> + serial_priv->conf_data[i] |= F81534_PORT_UNAVAILABLE;
> +
>   if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
>   continue;

So this adds at least half a second during probe just from the msleep
alone. Can this be reduced somehow?

We should at least try to half that time by only doing that loopback
test once per port. You may need to use devres to allocate memory for
the result in calc_num_ports (or probe).

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 3/4] usb: serial: f81534: add output pin control

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:08PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
> control transceiver. We'll read it from internal Flash with address
> 0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
> MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
> M1, M2 as 1, 1, 0.
> 
> Register mapping for output value:
>   Port 0:
>   M2: 0x2ae8 bit7, M1: 0x2a90 bit5, M0/SD: 0x2a90 bit4
>   Port 1:
>   M2: 0x2ae8 bit6, M1: 0x2ae8 bit0, M0/SD: 0x2ae8 bit3
>   Port 2:
>   M2: 0x2a90 bit0, M1: 0x2ae8 bit2, M0/SD: 0x2a80 bit6
>   Port 3:
>   M2: 0x2a90 bit3, M1: 0x2a90 bit2, M0/SD: 0x2a90 bit1
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
>  drivers/usb/serial/f81534.c | 67 
> -
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index b2d10309c335..30b966d71ae8 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -56,6 +56,7 @@
>  #define F81534_CUSTOM_NO_CUSTOM_DATA 0xff
>  #define F81534_CUSTOM_VALID_TOKEN0xf0
>  #define F81534_CONF_OFFSET   1
> +#define F81534_CONF_GPIO_OFFSET  4
>  
>  #define F81534_MAX_DATA_BLOCK64
>  #define F81534_MAX_BUS_RETRY 20
> @@ -166,6 +167,23 @@ struct f81534_port_private {
>   u8 phy_num;
>  };
>  
> +struct f81534_pin_data {
> + const u16 reg_addr;
> + const u16 reg_mask;

Should the mask not be u8?

> +};
> +
> +struct f81534_port_out_pin {
> + struct f81534_pin_data pin[3];
> +};
> +
> +/* Pin output value for M2/M1/M0(SD) */
> +static const struct f81534_port_out_pin f81534_port_out_pins[] = {
> +  {{{0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
> +  {{{0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
> +  {{{0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
> +  {{{0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },

Please use a space after { and before } consistently.

> +};
> +
>  static int f81534_logic_to_phy_port(struct usb_serial *serial,
>   struct usb_serial_port *port)
>  {
> @@ -271,6 +289,22 @@ static int f81534_get_register(struct usb_serial 
> *serial, u16 reg, u8 *data)
>   return status;
>  }
>  
> +static int f81534_set_mask_register(struct usb_serial *serial, u16 reg,
> + u8 mask, u8 data)
> +{
> + int status;
> + u8 tmp;
> +
> + status = f81534_get_register(serial, reg, );
> + if (status)
> + return status;
> +
> + tmp &= ~mask;
> + tmp |= (mask & data);
> +
> + return f81534_set_register(serial, reg, tmp);
> +}
> +
>  static int f81534_set_port_register(struct usb_serial_port *port, u16 reg,
>   u8 data)
>  {
> @@ -1299,6 +1333,37 @@ static void f81534_lsr_worker(struct work_struct *work)
>   dev_warn(>dev, "read LSR failed: %d\n", status);
>  }
>  
> +static int f81534_set_port_output_pin(struct usb_serial_port *port)
> +{
> + struct f81534_serial_private *serial_priv;
> + struct f81534_port_private *port_priv;
> + struct usb_serial *serial;
> + const struct f81534_port_out_pin *pins;
> + int status;
> + int i;
> + u8 value;
> + u8 idx;
> +
> + serial = port->serial;
> + serial_priv = usb_get_serial_data(serial);
> + port_priv = usb_get_serial_port_data(port);
> +
> + idx = F81534_CONF_GPIO_OFFSET + port_priv->phy_num;
> + value = serial_priv->conf_data[idx];
> + pins = _port_out_pins[port_priv->phy_num];
> +
> + for (i = 0; i < ARRAY_SIZE(pins->pin); ++i) {
> + status = f81534_set_mask_register(serial,
> + pins->pin[i].reg_addr, pins->pin[i].reg_mask,
> + value & BIT(i) ? pins->pin[i].reg_mask : 0);
> + if (status)
> + return status;
> + }

You're using 24 (get or set) accesses to update these three registers
here. Why not read them out (if necessary), determine their new values
and then write them back when done instead?

> +
> + dev_info(>dev, "Output pin (M0/M1/M2): %d\n", value);

Use dev_dbg here.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: Add device quirk for Logitech HD Pro Webcam C925e

[Dmitry Fleytman]

From: Dmitry Fleytman Dmitry Fleytman 

Commit e0429362ab15
("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
introduced quirk to workaround an issue with some Logitech webcams.

There is one more model that has the same issue - C925e, so applying
the same quirk as well.

See aforementioned commit message for detailed explanation of the problem.

Signed-off-by: Dmitry Fleytman 
---
 drivers/usb/core/quirks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index a10b346b9777..6d2d3b0bcc2a 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -52,10 +52,11 @@ static const struct usb_device_id usb_quirk_list[] = {
/* Microsoft LifeCam-VX700 v2.0 */
{ USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
 
-   /* Logitech HD Pro Webcams C920, C920-C and C930e */
+   /* Logitech HD Pro Webcams C920, C920-C, C925e and C930e */
{ USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
{ USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
{ USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
+   { USB_DEVICE(0x046d, 0x085B), .driver_info = USB_QUIRK_DELAY_INIT },
 
/* Logitech ConferenceCam CC3000e */
{ USB_DEVICE(0x046d, 0x0847), .driver_info = USB_QUIRK_DELAY_INIT },
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 16:30:13 +0100,
Mauro Santos wrote:
> 
> On 18-12-2017 13:53, Takashi Iwai wrote:
> > On Mon, 18 Dec 2017 14:44:44 +0100,
> > Greg KH wrote:
> >>
> >> On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> >>> I believe this is the right place to report this problem, but if it
> >>> isn't please point me in the right direction.
> >>
> >> Adding the developer of that patch, and the sound maintainer and
> >> developers to the thread.
> >>
> >>> I have noticed that after the update from kernel 4.14.5 to 4.14.6
> >>> alsamixer does not show the usual volume controls for my usb soundcard.
> >>>
> >>> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
> >>> check return value for usb_string() (from linux-stable) makes the
> >>> controls come back again with kernel 4.14.6.
> > (snip)
> >>
> >> This is odd, Takashi, I thought we fixed up the problem that if the
> >> string was invalid, the code would continue to go on, it's not a "real"
> >> error.  Did that not get marked for the stable trees?
> > 
> > Yes, it was marked as stable, and it's odd that the revert of the
> > given commit changes the behavior in that way.
> > 
> > Mauro, could you double-check whether reverting the commit really does
> > fix it as expected?  If yes, could you put some debug print at the
> > part the patch touches, and check which unit id gives len=0 from
> > snd_usb_copy_string_desc()?
> 
> I'm sure reverting that patch makes things work as expected. I noticed
> the problem after an update and that is the only thing I revert on top
> of the kernel provided by the distro (Arch Linux).

Did you revert only one patch, not both patches?
Just to be sure.

> Alsamixer works fine for the built in soundcard in my laptop, but the
> mixer for the usb soundcard was not working so it's specific to usb and
> only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
> tried reversing both, one at a time, and only reverting this one
> restored the expected behavior.
> 
> Regarding adding the debug print I'm going to need guidance. Without
> reverting anything, would adding at line 2178 of sound/usb/mixer.c the
> following be enough?
> 
> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
> 
> It would then look like this (minus the line wrapping):
> len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
> printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
> if (len)

Well, at that point, there should be no difference.
The difference is after that, so what I'd like to see is something
like:

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2175,14 +2175,18 @@ static int parse_audio_selector_unit(struct mixer_build 
*state, int unitid,
 
nameid = uac_selector_unit_iSelector(desc);
len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
+   pr_info("XXX id=%d, check_mapped_name=%d\n", id, len);
if (len)
;
-   else if (nameid)
+   else if (nameid) {
len = snd_usb_copy_string_desc(state, nameid, kctl->id.name,
 sizeof(kctl->id.name));
-   else
+   pr_info("XXX id=%d, copy_string=%d\n", len);
+   } else {
len = get_term_name(state, >oterm,
kctl->id.name, sizeof(kctl->id.name), 0);
+   pr_info("XXX id=%d, get_term_name=%d\n", len);
+   }
 
if (!len) {
strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));


If you see copy_string=0, it means that your hardware reports a bogus
entry, and the driver does it correctly.  If ignoring that error
really restores the old behavior back, it essentially means that it
worked casually in the past...


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Mauro Santos]

On 18-12-2017 13:53, Takashi Iwai wrote:
> On Mon, 18 Dec 2017 14:44:44 +0100,
> Greg KH wrote:
>>
>> On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
>>> I believe this is the right place to report this problem, but if it
>>> isn't please point me in the right direction.
>>
>> Adding the developer of that patch, and the sound maintainer and
>> developers to the thread.
>>
>>> I have noticed that after the update from kernel 4.14.5 to 4.14.6
>>> alsamixer does not show the usual volume controls for my usb soundcard.
>>>
>>> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
>>> check return value for usb_string() (from linux-stable) makes the
>>> controls come back again with kernel 4.14.6.
> (snip)
>>
>> This is odd, Takashi, I thought we fixed up the problem that if the
>> string was invalid, the code would continue to go on, it's not a "real"
>> error.  Did that not get marked for the stable trees?
> 
> Yes, it was marked as stable, and it's odd that the revert of the
> given commit changes the behavior in that way.
> 
> Mauro, could you double-check whether reverting the commit really does
> fix it as expected?  If yes, could you put some debug print at the
> part the patch touches, and check which unit id gives len=0 from
> snd_usb_copy_string_desc()?

I'm sure reverting that patch makes things work as expected. I noticed
the problem after an update and that is the only thing I revert on top
of the kernel provided by the distro (Arch Linux).

Alsamixer works fine for the built in soundcard in my laptop, but the
mixer for the usb soundcard was not working so it's specific to usb and
only 2 patches touch the mixer.c file between 4.14.5 and 4.14.6. I've
tried reversing both, one at a time, and only reverting this one
restored the expected behavior.

Regarding adding the debug print I'm going to need guidance. Without
reverting anything, would adding at line 2178 of sound/usb/mixer.c the
following be enough?

printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);

It would then look like this (minus the line wrapping):
len = check_mapped_name(map, kctl->id.name, sizeof(kctl->id.name));
printk("usbmixdbg: nameid=%d len=%d id.name=%s\n",nameid,len,kctl->id.name);
if (len)

-- 
Mauro Santos
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 2/4] usb: serial: f81534: add auto RTS direction support

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:07PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had auto RTS direction support for RS485 mode.
> We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
> There are 4 conditions below:
>   0: F81534_PORT_CONF_RS232.
>   1: F81534_PORT_CONF_RS485.
>   2: value error, default to F81534_PORT_CONF_RS232.
>   3: F81534_PORT_CONF_RS485_INVERT.
> 
> F81532/534 Clock register (offset +08h)
> 
> Bit0: UART Enable (always on)
> Bit2-1:   Clock source selector
>   00: 1.846MHz.
>   01: 18.46MHz.
>   10: 24MHz.
>   11: 14.77MHz.
> Bit4: Auto direction(RTS) control (RTS pin Low when TX)
> Bit5: Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
>  drivers/usb/serial/f81534.c | 54 
> +++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 76c676ef5f0d..b2d10309c335 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -102,11 +102,16 @@
>  
>  #define F81534_DEFAULT_BAUD_RATE 9600
>  
> +#define F81534_PORT_CONF_RS232   0
> +#define F81534_PORT_CONF_RS485   BIT(0)
> +#define F81534_PORT_CONF_RS485_INVERT(BIT(0) | BIT(1))
>  #define F81534_PORT_CONF_DISABLE_PORTBIT(3)
>  #define F81534_PORT_CONF_NOT_EXIST_PORT  BIT(7)
>  #define F81534_PORT_UNAVAILABLE  \
>   (F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
>  
> +#define F81534_UART_MODE_MASK(BIT(0) | BIT(1))
> +
>  #define F81534_1X_RXTRIGGER  0xc3
>  #define F81534_8X_RXTRIGGER  0xcf
>  
> @@ -119,6 +124,8 @@
>   *   01: 18.46MHz.
>   *   10: 24MHz.
>   *   11: 14.77MHz.
> + * Bit4: Auto direction(RTS) control (RTS pin Low when TX)
> + * Bit5: Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
>   */
>  
>  #define F81534_CLK_1_846_MHZ BIT(0)
> @@ -126,6 +133,9 @@
>  #define F81534_CLK_24_MHZ(BIT(0) | BIT(2))
>  #define F81534_CLK_14_77_MHZ (BIT(0) | BIT(1) | BIT(2))
>  
> +#define F81534_CLK_RS485_MODEBIT(4)
> +#define F81534_CLK_RS485_INVERT  BIT(5)
> +
>  static const struct usb_device_id f81534_id_table[] = {
>   { USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
>   { USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -485,16 +495,20 @@ static int f81534_set_port_config(struct 
> usb_serial_port *port,
>   struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
>  {
>   struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
> + struct f81534_serial_private *serial_priv;
>   u32 divisor;
>   int status;
>   int idx;
>   u8 value;
> + u8 tmp;
>   static u32 const baudrate_table[] = {115200, 921600, 1152000,
>   150};
>   static u8 const clock_table[] = {F81534_CLK_1_846_MHZ,
>   F81534_CLK_14_77_MHZ, F81534_CLK_18_46_MHZ,
>   F81534_CLK_24_MHZ};
>  
> + serial_priv = usb_get_serial_data(port->serial);
> +
>   do {
>   for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
>   if (baudrate <= baudrate_table[idx] &&
> @@ -520,8 +534,25 @@ static int f81534_set_port_config(struct usb_serial_port 
> *port,
>   } while (1);
>  
>   port_priv->baud_base = baudrate_table[idx];
> - status = f81534_set_port_register(port, F81534_CLOCK_REG,
> - clock_table[idx]);
> + tmp = serial_priv->conf_data[port_priv->phy_num];
> +
> + switch (tmp & F81534_UART_MODE_MASK) {
> + case F81534_PORT_CONF_RS485_INVERT:
> + value = F81534_CLK_RS485_MODE | F81534_CLK_RS485_INVERT;
> + break;
> + case F81534_PORT_CONF_RS485:
> + value = F81534_CLK_RS485_MODE;
> + break;
> +
> + default:
> + /* fall through, default RS232 Mode */
> + case F81534_PORT_CONF_RS232:
> + value = 0;
> + break;
> + }
> +
> + value |= clock_table[idx];

With the shadow clock register I mentioned earlier, this can be
determined once at probe instead of at every termios change.

> + status = f81534_set_port_register(port, F81534_CLOCK_REG, value);
>   if (status) {
>   dev_err(>dev, "CLOCK_REG setting failed\n");
>   return status;
> @@ -1270,9 +1301,12 @@ static void f81534_lsr_worker(struct work_struct *work)
>  
>  static int f81534_port_probe(struct usb_serial_port *port)
>  {
> + struct f81534_serial_private *serial_priv;
>   struct f81534_port_private *port_priv;
>   int ret;
> + u8 value;
>  
> + 

[PATCH] usb: dwc3: of-simple: fix oops by unbalanced clk disable call

[Enric Balletbo i Serra]

dwc3_of_simple_dev_pm_ops has never been used since commit a0d8c4cfdf31
("usb: dwc3: of-simple: set dev_pm_ops"), but this commit has brought
and oops when unbind the device due this sequence:

  dwc3_of_simple_remove
   -> clk_disable ...
  -> pm_runtime_put_sync
 -> dwc3_of_simple_runtime_suspend
-> clk_disable (again)

This double call to clk_core_disable causes a kernel oops like this:

 WARNING: CPU: 1 PID: 4022 at drivers/clk/clk.c:656 clk_core_disable+0x78/0x80
 CPU: 1 PID: 4022 Comm: bash Not tainted 4.15.0-rc4+ #44
 Hardware name: Google Kevin (DT)
 pstate: 8085 (Nzcv daIf -PAN -UAO)
 pc : clk_core_disable+0x78/0x80
 lr : clk_core_disable_lock+0x20/0x38
 sp : 0bbf3a90
 ...
 Call trace:
  clk_core_disable+0x78/0x80
  clk_disable+0x1c/0x30
  dwc3_of_simple_runtime_suspend+0x30/0x50
  pm_generic_runtime_suspend+0x28/0x40

This patch fixes the unbalanced clk disable call by setting the num_clocks
variable to zero once the clocks were disabled.

Fixes: a0d8c4cfdf31 ("usb: dwc3: of-simple: set dev_pm_ops")
Signed-off-by: Enric Balletbo i Serra 
---
 drivers/usb/dwc3/dwc3-of-simple.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c 
b/drivers/usb/dwc3/dwc3-of-simple.c
index 7ae0eef..e54c3622 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -143,6 +143,7 @@ static int dwc3_of_simple_remove(struct platform_device 
*pdev)
clk_disable_unprepare(simple->clks[i]);
clk_put(simple->clks[i]);
}
+   simple->num_clocks = 0;
 
reset_control_assert(simple->resets);
reset_control_put(simple->resets);
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 1/4] usb: serial: f81534: add high baud rate support

[Johan Hovold]

On Thu, Nov 16, 2017 at 03:46:06PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates can
> be up to 1.5Mbits with 24MHz.
> 
> F81532/534 Clock register (offset +08h)
> 
> Bit0: UART Enable (always on)
> Bit2-1:   Clock source selector
>   00: 1.846MHz.
>   01: 18.46MHz.
>   10: 24MHz.
>   11: 14.77MHz.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
>  drivers/usb/serial/f81534.c | 84 
> -
>  1 file changed, 68 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index cb8214860192..76c676ef5f0d 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -45,6 +45,7 @@
>  #define F81534_MODEM_CONTROL_REG (0x04 + F81534_UART_BASE_ADDRESS)
>  #define F81534_LINE_STATUS_REG   (0x05 + 
> F81534_UART_BASE_ADDRESS)
>  #define F81534_MODEM_STATUS_REG  (0x06 + 
> F81534_UART_BASE_ADDRESS)
> +#define F81534_CLOCK_REG (0x08 + F81534_UART_BASE_ADDRESS)
>  #define F81534_CONFIG1_REG   (0x09 + F81534_UART_BASE_ADDRESS)
>  
>  #define F81534_DEF_CONF_ADDRESS_START0x3000
> @@ -61,7 +62,7 @@
>  
>  /* Default URB timeout for USB operations */
>  #define F81534_USB_MAX_RETRY 10
> -#define F81534_USB_TIMEOUT   1000
> +#define F81534_USB_TIMEOUT   2000

You need to at least mention in the commit message why this is needed.

>  #define F81534_SET_GET_REGISTER  0xA0
>  
>  #define F81534_NUM_PORT  4
> @@ -100,7 +101,6 @@
>  #define F81534_CMD_READ  0x03
>  
>  #define F81534_DEFAULT_BAUD_RATE 9600
> -#define F81534_MAX_BAUDRATE  115200
>  
>  #define F81534_PORT_CONF_DISABLE_PORTBIT(3)
>  #define F81534_PORT_CONF_NOT_EXIST_PORT  BIT(7)
> @@ -110,6 +110,22 @@
>  #define F81534_1X_RXTRIGGER  0xc3
>  #define F81534_8X_RXTRIGGER  0xcf
>  
> +/*
> + * F81532/534 Clock registers (offset +08h)
> + *
> + * Bit0: UART Enable (always on)
> + * Bit2-1:   Clock source selector
> + *   00: 1.846MHz.
> + *   01: 18.46MHz.
> + *   10: 24MHz.
> + *   11: 14.77MHz.
> + */
> +
> +#define F81534_CLK_1_846_MHZ BIT(0)
> +#define F81534_CLK_18_46_MHZ (BIT(0) | BIT(1))
> +#define F81534_CLK_24_MHZ(BIT(0) | BIT(2))
> +#define F81534_CLK_14_77_MHZ (BIT(0) | BIT(1) | BIT(2))

Use a separate define for the enable bit.

> +
>  static const struct usb_device_id f81534_id_table[] = {
>   { USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
>   { USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -133,6 +149,7 @@ struct f81534_port_private {
>   struct usb_serial_port *port;
>   unsigned long tx_empty;
>   spinlock_t msr_lock;
> + u32 baud_base;
>   u8 shadow_mcr;
>   u8 shadow_lcr;
>   u8 shadow_msr;
> @@ -464,13 +481,51 @@ static u32 f81534_calc_baud_divisor(u32 baudrate, u32 
> clockrate)
>   return DIV_ROUND_CLOSEST(clockrate, baudrate);
>  }
>  
> -static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
> - u8 lcr)
> +static int f81534_set_port_config(struct usb_serial_port *port,
> + struct tty_struct *tty, u32 baudrate, u32 old_baudrate, u8 lcr)
>  {
>   struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
>   u32 divisor;
>   int status;
> + int idx;
>   u8 value;
> + static u32 const baudrate_table[] = {115200, 921600, 1152000,
> + 150};
> + static u8 const clock_table[] = {F81534_CLK_1_846_MHZ,
> + F81534_CLK_14_77_MHZ, F81534_CLK_18_46_MHZ,
> + F81534_CLK_24_MHZ};
> +
> + do {
> + for (idx = 0; idx < ARRAY_SIZE(baudrate_table); ++idx) {
> + if (baudrate <= baudrate_table[idx] &&
> + baudrate_table[idx] % baudrate == 0)
> + break;
> + }
> +
> + /* Found acceptable baud rate */
> + if (idx != ARRAY_SIZE(baudrate_table))
> + break;
> +
> + if (baudrate == old_baudrate &&
> + old_baudrate != F81534_DEFAULT_BAUD_RATE)
> + old_baudrate = F81534_DEFAULT_BAUD_RATE;
> +
> + dev_warn(>dev,
> + "baudrate: %d not supported, change to: %d\n",
> + baudrate, old_baudrate);

No need to warn here as you report back the chosen speed in the termios.

> +
> + baudrate = old_baudrate;
> + tty_encode_baud_rate(tty, baudrate, baudrate);
> +
> + } while (1);

This looks 

Re: [debian bug:883345] vhci-hcd: kernel oops when attaching a mass storage on client side of usbip

[Borissh1983]

On 12/5/17, Greg KH  wrote:
> On Tue, Dec 05, 2017 at 10:17:08AM +0200, borissh1...@gmail.com wrote:
>> On Tuesday, 5 December 2017 9:39:33 IST Greg KH wrote:
>> > On Mon, Dec 04, 2017 at 10:05:45PM +0200, borissh1...@gmail.com wrote:
>> > > Hi ,
>> > >
>> > > vhci-hcd kernel oops when attaching a mass storage on 4.13.13.
>> > >
>> > > When I try to attach a mass storage device to a vhci-hcd, it generates
>> > > an
>> > > oops. no problem with other devices.
>> > >
>> > > A second user had also confirmed on a different hardware ( https://
>> > > bugs.debian.org/cgi-bin/bugreport.cgi?bug=878866)
>> > > debian's bug report attached bellow.
>> > >
>> > > steps to reproduce with debian :
>> > >
>> > > On server (debian stable running kernel 4.9) :
>> > >
>> > > modprobe usbip-core
>> > > modprobe usbip-host
>> > > usbipd -D
>> > > usbip bind -b bus
>> > >
>> > > On client (debian sid running kernel 4.13):
>> > > modprobe usbip-core
>> > > modprobe vhci-hcd
>> > > usbsip attach -b bus -r serverip
>> > >
>> > > config values :
>> > >
>> > > CONFIG_USBIP_CORE=m
>> > > CONFIG_USBIP_VHCI_HCD=m
>> > > CONFIG_USBIP_VHCI_HC_PORTS=4
>> > > CONFIG_USBIP_VHCI_NR_HCS=8
>> > > CONFIG_USBIP_HOST=m
>> > > CONFIG_USBIP_VUDC=m
>> >
>> > A number of vhci bug fixes have recently been posted to the linux-usb
>> > mailing list and will be merged into Linus's tree soon.  Any chance you
>> > could test those out to see if it solves your problem or not?
>> >
>>
>> Yes , I would gladly check it, just need to understand what I should build
>> and
>> with what options.
>>
>> We are talking about
>> git://git.kernel.org/pub/scm/linux/kernel/git/stable/
>> linux-stable.git correct ?
>
> No, that does not have the patches in it yet.  If you use the usb.git
> tree, in the usb-linus branch, the patches are all there.
>
> Heck, I've just attached them here, as I already have them around, just
> try these on top of 4.14.
>
>> And are these usbip config setting correct (should work ) ?
>
> No idea :)
>
> thanks,
>
> greg k-h
>

Thank you , and sorry for the delay.

Using an updated kernel with a specially crafted config values removed
the oops on mass storage devices (debian bugs 878866 and 883345) .

I took usb-linus branch (up to  7a3c296ae08f), I checkedout
git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git and built.
build was done using make deb-config LOCALVERSION=-khan
DEB_PKGVERSION=$(make kernelversion)-1

uname -a
Linux pc 4.15.0-rc3-khan #1 SMP Sun Dec 17 21:00:45 IST 2017 x86_64 GNU/Linux

my usbip config settings are :

grep USBIP /boot/config-4.15.0-rc3-khan
CONFIG_USBIP_CORE=m
CONFIG_USBIP_VHCI_HCD=m
CONFIG_USBIP_VHCI_HC_PORTS=4
CONFIG_USBIP_VHCI_NR_HCS=8
CONFIG_USBIP_HOST=m
CONFIG_USBIP_VUDC=m

Thanks !
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: pd: fix the offset for SVID specific commands

[Greg Kroah-Hartman]

On Mon, Dec 18, 2017 at 05:03:03PM +0300, Heikki Krogerus wrote:
> The SVID specific commands in the Command field of the
> Structured VDM Header start from 16, not 10. Changing the
> value used in VDO_CMD_VENDOR() macro from 10 to 0x10.
> 
> Signed-off-by: Heikki Krogerus 

Does this fix any specific code that uses this macro?  Should it go to
the stable tree(s)?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: pd: fix the offset for SVID specific commands

[Heikki Krogerus]

The SVID specific commands in the Command field of the
Structured VDM Header start from 16, not 10. Changing the
value used in VDO_CMD_VENDOR() macro from 10 to 0x10.

Signed-off-by: Heikki Krogerus 
---
 include/linux/usb/pd_vdo.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
index d92259f8de0a..2b64d23ace5c 100644
--- a/include/linux/usb/pd_vdo.h
+++ b/include/linux/usb/pd_vdo.h
@@ -65,7 +65,7 @@
 #define CMD_EXIT_MODE  5
 #define CMD_ATTENTION  6
 
-#define VDO_CMD_VENDOR(x)(((10 + (x)) & 0x1f))
+#define VDO_CMD_VENDOR(x)(((0x10 + (x)) & 0x1f))
 
 /* ChromeOS specific commands */
 #define VDO_CMD_VERSIONVDO_CMD_VENDOR(0)
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Takashi Iwai]

On Mon, 18 Dec 2017 14:44:44 +0100,
Greg KH wrote:
> 
> On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> > I believe this is the right place to report this problem, but if it
> > isn't please point me in the right direction.
> 
> Adding the developer of that patch, and the sound maintainer and
> developers to the thread.
> 
> > I have noticed that after the update from kernel 4.14.5 to 4.14.6
> > alsamixer does not show the usual volume controls for my usb soundcard.
> > 
> > Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
> > check return value for usb_string() (from linux-stable) makes the
> > controls come back again with kernel 4.14.6.
(snip)
> 
> This is odd, Takashi, I thought we fixed up the problem that if the
> string was invalid, the code would continue to go on, it's not a "real"
> error.  Did that not get marked for the stable trees?

Yes, it was marked as stable, and it's odd that the revert of the
given commit changes the behavior in that way.

Mauro, could you double-check whether reverting the commit really does
fix it as expected?  If yes, could you put some debug print at the
part the patch touches, and check which unit id gives len=0 from
snd_usb_copy_string_desc()?


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Mixer regression with usb soundcard

[Greg KH]

On Sun, Dec 17, 2017 at 06:56:05PM +, Mauro Santos wrote:
> I believe this is the right place to report this problem, but if it
> isn't please point me in the right direction.

Adding the developer of that patch, and the sound maintainer and
developers to the thread.

> I have noticed that after the update from kernel 4.14.5 to 4.14.6
> alsamixer does not show the usual volume controls for my usb soundcard.
> 
> Reverting 3884d12e17ab770aa0f5d4bc65bfbfd006f417fa ALSA: usb-audio: Add
> check return value for usb_string() (from linux-stable) makes the
> controls come back again with kernel 4.14.6.
> 
> Amixer shows a difference between the good and bad case.
> 
> Good amixer output (4.14.5):
> Simple mixer control 'PCM',0
>   Capabilities: pvolume pswitch pswitch-joined
>   Playback channels: Front Left - Front Right
>   Limits: Playback 0 - 110
>   Mono:
>   Front Left: Playback 0 [0%] [-55.00dB] [on]
>   Front Right: Playback 0 [0%] [-55.00dB] [on]
> Simple mixer control 'PCM Capture Source',0
>   Capabilities: enum
>   Items: 'Line' 'IEC958 In'
>   Item0: 'Line'
> Simple mixer control 'Line',0
>   Capabilities: cvolume cswitch cswitch-joined
>   Capture channels: Front Left - Front Right
>   Limits: Capture 0 - 104
>   Front Left: Capture 20 [19%] [-30.00dB] [off]
>   Front Right: Capture 20 [19%] [-30.00dB] [off]
> 
> Bad amixer output (4.14.6):
> Simple mixer control 'PCM',0
>   Capabilities: pvolume pswitch pswitch-joined enum
>   Items: 'Line' 'IEC958 In'
>   Item0: 'Line'
>   Item1: 'Line'
> Simple mixer control 'Line',0
>   Capabilities: cvolume cswitch cswitch-joined
>   Capture channels: Front Left - Front Right
>   Limits: Capture 0 - 104
>   Front Left: Capture 20 [19%] [-30.00dB] [off]
>   Front Right: Capture 20 [19%] [-30.00dB] [off]
> 
> The output of lsusb -v for the affected sound card is attached.
> 
> -- 
> Mauro Santos

> Bus 001 Device 006: ID 1852:7022 GYROCOM C Co., LTD 
> Device Descriptor:
>   bLength18
>   bDescriptorType 1
>   bcdUSB   1.10
>   bDeviceClass0 
>   bDeviceSubClass 0 
>   bDeviceProtocol 0 
>   bMaxPacketSize0 8
>   idVendor   0x1852 GYROCOM C Co., LTD
>   idProduct  0x7022 
>   bcdDevice0.01
>   iManufacturer   1 HiFimeDIY Audio
>   iProduct2 HiFimeDIY DAC
>   iSerial 0 
>   bNumConfigurations  1
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength  428
> bNumInterfaces  4
> bConfigurationValue 1
> iConfiguration  0 
> bmAttributes 0x80
>   (Bus Powered)
> MaxPower  500mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
>   bNumEndpoints   1
>   bInterfaceClass 3 Human Interface Device
>   bInterfaceSubClass  0 
>   bInterfaceProtocol  0 
>   iInterface  0 
> HID Device Descriptor:
>   bLength 9
>   bDescriptorType33
>   bcdHID   1.00
>   bCountryCode0 Not supported
>   bNumDescriptors 1
>   bDescriptorType34 Report
>   wDescriptorLength  58
>  Report Descriptors: 
>** UNAVAILABLE **
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81  EP 1 IN
> bmAttributes3
>   Transfer TypeInterrupt
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0012  1x 18 bytes
> bInterval  32
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber1
>   bAlternateSetting   0
>   bNumEndpoints   0
>   bInterfaceClass 1 Audio
>   bInterfaceSubClass  1 Control Device
>   bInterfaceProtocol  0 
>   iInterface  3 SABRE 24/96 DAC_DigiT
>   AudioControl Interface Descriptor:
> bLength10
> bDescriptorType36
> bDescriptorSubtype  1 (HEADER)
> bcdADC   1.00
> wTotalLength   92
> bInCollection   2
> baInterfaceNr( 0)   2
> baInterfaceNr( 1)   3
>   AudioControl Interface Descriptor:
> bLength12
> bDescriptorType36
> bDescriptorSubtype  2 (INPUT_TERMINAL)
> bTerminalID 1
> wTerminalType  0x0603 Line Connector
> bAssocTerminal  0
> bNrChannels 2
> wChannelConfig 0x0003
>   Left Front (L)
>   

[PATCH v3 2/7] typec: tcpm: Add ADO header for Alert message handling

[Adam Thomson]

This commit adds a header providing definitions for handling Alert
messages. Currently the header only focuses on handling incoming
alerts.

Signed-off-by: Adam Thomson 
---
 include/linux/usb/pd_ado.h | 42 ++
 1 file changed, 42 insertions(+)
 create mode 100644 include/linux/usb/pd_ado.h

diff --git a/include/linux/usb/pd_ado.h b/include/linux/usb/pd_ado.h
new file mode 100644
index 000..9aa1cf3
--- /dev/null
+++ b/include/linux/usb/pd_ado.h
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2017 Dialog Semiconductor
+ *
+ * Author: Adam Thomson 
+ */
+
+#ifndef __LINUX_USB_PD_ADO_H
+#define __LINUX_USB_PD_ADO_H
+
+/* ADO : Alert Data Object */
+#define USB_PD_ADO_TYPE_SHIFT  24
+#define USB_PD_ADO_TYPE_MASK   0xff
+#define USB_PD_ADO_FIXED_BATT_SHIFT20
+#define USB_PD_ADO_FIXED_BATT_MASK 0xf
+#define USB_PD_ADO_HOT_SWAP_BATT_SHIFT 16
+#define USB_PD_ADO_HOT_SWAP_BATT_MASK  0xf
+
+#define USB_PD_ADO_TYPE_BATT_STATUS_CHANGE BIT(1)
+#define USB_PD_ADO_TYPE_OCPBIT(2)
+#define USB_PD_ADO_TYPE_OTPBIT(3)
+#define USB_PD_ADO_TYPE_OP_COND_CHANGE BIT(4)
+#define USB_PD_ADO_TYPE_SRC_INPUT_CHANGE   BIT(5)
+#define USB_PD_ADO_TYPE_OVPBIT(6)
+
+static inline unsigned int usb_pd_ado_type(u32 ado)
+{
+   return (ado >> USB_PD_ADO_TYPE_SHIFT) & USB_PD_ADO_TYPE_MASK;
+}
+
+static inline unsigned int usb_pd_ado_fixed_batt(u32 ado)
+{
+   return (ado >> USB_PD_ADO_FIXED_BATT_SHIFT) &
+  USB_PD_ADO_FIXED_BATT_MASK;
+}
+
+static inline unsigned int usb_pd_ado_hot_swap_batt(u32 ado)
+{
+   return (ado >> USB_PD_ADO_HOT_SWAP_BATT_SHIFT) &
+  USB_PD_ADO_HOT_SWAP_BATT_MASK;
+}
+#endif /* __LINUX_USB_PD_ADO_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/7] power: supply: Add 'connected_type' property and supporting code

[Adam Thomson]

This commit adds the 'connected_type' property to represent supplies
which can report a number of different types of supply based on a
connection event.

Examples of this already exist in drivers whereby the existing 'type'
property is updated, based on an event, to represent what was
connected (e.g. USB_DCP, USB_ACA, ...). Current implementations
however don't show all supported connectable types, so this knowledge
has to be exlicitly known for each driver that supports this.

The 'connected_type' property is intended to fill this void and show
users all possible types supported by a driver. The property, when
read, shows all available types for the driver, and the one currently
chosen is highlighted/bracketed. It is expected that the 'type'
property would then just show the top-level type, such as 'USB', and
this would be static.

Currently the 'conn_type' enum contains all of the USB variant types
that exist for the 'type' enum at this time, and in addition has
the PPS type. In the future this can be extended further for other
types which have multiple connected types supported. The mirroring
is intentional so as to not impact existing usage of the 'type'
property.

Signed-off-by: Adam Thomson 
---
 drivers/power/supply/power_supply_sysfs.c | 50 +++
 include/linux/power_supply.h  | 15 ++
 2 files changed, 65 insertions(+)

diff --git a/drivers/power/supply/power_supply_sysfs.c 
b/drivers/power/supply/power_supply_sysfs.c
index 5204f11..1b3b202 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -46,6 +46,11 @@
"USB_PD", "USB_PD_DRP", "BrickID"
 };
 
+static const char * const power_supply_conn_type_text[] = {
+   "Unknown", "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
+   "USB_PD", "USB_PD_DRP", "USB_PD_PPS", "BrickID"
+};
+
 static const char * const power_supply_status_text[] = {
"Unknown", "Charging", "Discharging", "Not charging", "Full"
 };
@@ -73,6 +78,46 @@
"Unknown", "System", "Device"
 };
 
+static ssize_t power_supply_show_conn_type(struct device *dev,
+  enum power_supply_conn_type 
*conn_types,
+  ssize_t num_conn_types,
+  union power_supply_propval *value,
+  char *buf)
+{
+   enum power_supply_conn_type conn_type;
+   ssize_t count = 0;
+   bool match = false;
+   int i;
+
+   if ((!conn_types) || (num_conn_types <= 0)) {
+   dev_warn(dev, "driver has no valid connected types\n");
+   return -ENODATA;
+   }
+
+   for (i = 0; i < num_conn_types; ++i) {
+   conn_type = conn_types[i];
+
+   if (value->intval == conn_type) {
+   count += sprintf(buf + count, "[%s] ",
+
power_supply_conn_type_text[conn_type]);
+   match = true;
+   } else {
+   count += sprintf(buf + count, "%s ",
+
power_supply_conn_type_text[conn_type]);
+   }
+   }
+
+   if (!match) {
+   dev_warn(dev, "driver reporting unsupported connected type\n");
+   return -EINVAL;
+   }
+
+   if (count)
+   buf[count - 1] = '\n';
+
+   return count;
+}
+
 static ssize_t power_supply_show_property(struct device *dev,
  struct device_attribute *attr,
  char *buf) {
@@ -115,6 +160,10 @@ static ssize_t power_supply_show_property(struct device 
*dev,
else if (off == POWER_SUPPLY_PROP_TYPE)
return sprintf(buf, "%s\n",
   power_supply_type_text[value.intval]);
+   else if (off == POWER_SUPPLY_PROP_CONNECTED_TYPE)
+   return power_supply_show_conn_type(dev, psy->desc->conn_types,
+  psy->desc->num_conn_types,
+  , buf);
else if (off == POWER_SUPPLY_PROP_SCOPE)
return sprintf(buf, "%s\n",
   power_supply_scope_text[value.intval]);
@@ -241,6 +290,7 @@ static ssize_t power_supply_store_property(struct device 
*dev,
POWER_SUPPLY_ATTR(time_to_full_now),
POWER_SUPPLY_ATTR(time_to_full_avg),
POWER_SUPPLY_ATTR(type),
+   POWER_SUPPLY_ATTR(connected_type),
POWER_SUPPLY_ATTR(scope),
POWER_SUPPLY_ATTR(precharge_current),
POWER_SUPPLY_ATTR(charge_term_current),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 79e90b3..e15a629 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -145,6 +145,7 @@ enum power_supply_property {

[PATCH v3 4/7] typec: tcpm: Add core support for sink side PPS

[Adam Thomson]

This commit adds code to handle requesting of PPS APDOs. Switching
between standard PDOs and APDOs, and re-requesting an APDO to
modify operating voltage/current will be triggered by an
external call into TCPM.

Signed-off-by: Adam Thomson 
---
 drivers/usb/typec/tcpm.c | 533 ++-
 include/linux/usb/tcpm.h |   2 +-
 2 files changed, 523 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index f4d563e..b66d26c 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -47,6 +47,7 @@
S(SNK_DISCOVERY_DEBOUNCE_DONE), \
S(SNK_WAIT_CAPABILITIES),   \
S(SNK_NEGOTIATE_CAPABILITIES),  \
+   S(SNK_NEGOTIATE_PPS_CAPABILITIES),  \
S(SNK_TRANSITION_SINK), \
S(SNK_TRANSITION_SINK_VBUS),\
S(SNK_READY),   \
@@ -166,6 +167,16 @@ struct pd_mode_data {
struct typec_altmode_desc altmode_desc[SVID_DISCOVERY_MAX];
 };
 
+struct pd_pps_data {
+   u32 min_volt;
+   u32 max_volt;
+   u32 max_curr;
+   u32 out_volt;
+   u32 op_curr;
+   bool supported;
+   bool active;
+};
+
 struct tcpm_port {
struct device *dev;
 
@@ -233,6 +244,7 @@ struct tcpm_port {
struct completion swap_complete;
int swap_status;
 
+   unsigned int negotiated_rev;
unsigned int message_id;
unsigned int caps_count;
unsigned int hard_reset_count;
@@ -255,6 +267,7 @@ struct tcpm_port {
unsigned int nr_fixed; /* number of fixed sink PDOs */
unsigned int nr_var; /* number of variable sink PDOs */
unsigned int nr_batt; /* number of battery sink PDOs */
+   unsigned int nr_apdo; /* number of APDO type PDOs */
u32 snk_vdo[VDO_MAX_OBJECTS];
unsigned int nr_snk_vdo;
 
@@ -262,6 +275,7 @@ struct tcpm_port {
unsigned int max_snk_ma;
unsigned int max_snk_mw;
unsigned int operating_snk_mw;
+   bool update_sink_caps;
 
/* Requested current / voltage */
u32 current_limit;
@@ -278,8 +292,13 @@ struct tcpm_port {
/* VDO to retry if UFP responder replied busy */
u32 vdo_retry;
 
-   /* Alternate mode data */
+   /* PPS */
+   struct pd_pps_data pps_data;
+   struct completion pps_complete;
+   bool pps_pending;
+   int pps_status;
 
+   /* Alternate mode data */
struct pd_mode_data mode_data;
struct typec_altmode *partner_altmode[SVID_DISCOVERY_MAX];
struct typec_altmode *port_altmode[SVID_DISCOVERY_MAX];
@@ -497,6 +516,16 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
  pdo_max_voltage(pdo),
  pdo_max_power(pdo));
break;
+   case PDO_TYPE_APDO:
+   if (pdo_apdo_type(pdo) == APDO_TYPE_PPS)
+   scnprintf(msg, sizeof(msg),
+ "%u-%u mV, %u mA",
+ pdo_pps_apdo_min_voltage(pdo),
+ pdo_pps_apdo_max_voltage(pdo),
+ pdo_pps_apdo_max_current(pdo));
+   else
+   strcpy(msg, "undefined APDO");
+   break;
default:
strcpy(msg, "undefined");
break;
@@ -791,11 +820,13 @@ static int tcpm_pd_send_source_caps(struct tcpm_port 
*port)
msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
  port->pwr_role,
  port->data_role,
+ port->negotiated_rev,
  port->message_id, 0);
} else {
msg.header = PD_HEADER_LE(PD_DATA_SOURCE_CAP,
  port->pwr_role,
  port->data_role,
+ port->negotiated_rev,
  port->message_id,
  port->nr_src_pdo);
}
@@ -816,11 +847,13 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
  port->pwr_role,
  port->data_role,
+ port->negotiated_rev,
  port->message_id, 0);
} else {
msg.header = PD_HEADER_LE(PD_DATA_SINK_CAP,
  port->pwr_role,
  port->data_role,
+ 

[PATCH v3 0/7] typec: tcpm: Add sink side support for PPS

[Adam Thomson]

This patch set adds sink side support for the PPS feature introduced in the
USB PD 3.0 specification.

The source PPS supply is represented using the Power Supply framework to provide
access and control APIs for dealing with it's operating voltage and current,
and switching between a standard PDO and PPS APDO operation. During standard PDO
operation the voltage and current is read-only, but for APDO PPS these are
writable as well to allow for control.

It should be noted that the keepalive for PPS is not handled within TCPM. The
expectation is that the external user will be required to ensure re-requests
occur regularly to ensure PPS remains and the source does not hard reset.

Changes in v3:
 - Drop 'RFC' from patch series titles
 - Rename PPS related defines to be PPS specific rather than generic APDO titles
 - Update source caps logging to only print PPS APDOs, and for others report as
   undefined.
 - Add ABI documentation for tcpm-source-psy sysfs properties
 - Rebase PDO selection on top of 'typec: tcpm: Only request matching pdos'
   patch.
 - Update capabilities validation introduced in
   'typec: tcpm: Validate source and sink caps' to support PPS APDOs.
 - Dropped power_supply 'type' property update for PPS addition
 - Added 'connected_type' property to power_supply framework, to support
   supplies which can report multiple connected types (e.g. USB), as discussed
   with Heikki.

Changes in v2:
 - Use USB_PD and usb_pd prefixes for macros and inline functions in headers.
 - Negotiate spec revision of PD headers during initial contract agreement.
 - New headers now use SPDX tags for referencing correct license.

NOTE: Code changes are based on linux-next tag 'next-20171212' to pick up
capabilities validation and selection updates.

Adam Thomson (7):
  typec: tcpm: Add PD Rev 3.0 definitions to PD header
  typec: tcpm: Add ADO header for Alert message handling
  typec: tcpm: Add SDB header for Status message handling
  typec: tcpm: Add core support for sink side PPS
  power: supply: Add 'connected_type' property and supporting code
  typec: tcpm: Represent source supply through power_supply class
  typec: tcpm: Add support for sink PPS related messages

 .../ABI/testing/sysfs-class-power-tcpm-source-psy  |  92 +++
 drivers/power/supply/power_supply_sysfs.c  |  50 ++
 drivers/usb/typec/Kconfig  |   1 +
 drivers/usb/typec/fusb302/Kconfig  |   2 +-
 drivers/usb/typec/fusb302/fusb302.c|  63 +-
 drivers/usb/typec/tcpm.c   | 918 -
 include/linux/power_supply.h   |  15 +
 include/linux/usb/pd.h | 187 -
 include/linux/usb/pd_ado.h |  42 +
 include/linux/usb/pd_ext_sdb.h |  31 +
 include/linux/usb/tcpm.h   |   2 +-
 11 files changed, 1308 insertions(+), 95 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy
 create mode 100644 include/linux/usb/pd_ado.h
 create mode 100644 include/linux/usb/pd_ext_sdb.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 6/7] typec: tcpm: Represent source supply through power_supply class

[Adam Thomson]

This commit adds a power_supply class instance to represent a
PD source's voltage and current properties. This provides an
interface for reading these properties from user-space or other
drivers.

For PPS enabled Sources, this also provides write access to set
the current and voltage and allows for swapping between standard
PDO and PPS APDO.

As this represents a superset of the information provided in the
fusb302 driver, the power_supply instance in that code is removed
as part of this change, so reverting the commit titled
'typec: tcpm: Represent source supply through power_supply class'

Signed-off-by: Adam Thomson 
---
 .../ABI/testing/sysfs-class-power-tcpm-source-psy  |  92 
 drivers/usb/typec/Kconfig  |   1 +
 drivers/usb/typec/fusb302/Kconfig  |   2 +-
 drivers/usb/typec/fusb302/fusb302.c|  63 +-
 drivers/usb/typec/tcpm.c   | 235 -
 5 files changed, 330 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy

diff --git a/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy 
b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy
new file mode 100644
index 000..4986cba
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-power-tcpm-source-psy
@@ -0,0 +1,92 @@
+What:  /sys/class/power_supply/tcpm-source-psy/type
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-only property describes the main type of source supply.
+   Type-C is a USB standard so this property always returns "USB".
+
+What:  /sys/class/power_supply/tcpm-source-psy/connected_type
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-only property describes the type of source supply that is
+   connected, if the supply is online. The value is always Type C
+   unless a source has been attached which is identified as USB-PD capable.
+
+   Valid values:
+   - "USB_TYPE_C"  : Type C connected supply, not UBS-PD capable
+ (default value)
+   - "USB_PD"  : USB-PD capable source supply connected
+   - "USB_PD_PPS"  : USB-PD PPS capable source supply connected
+
+What:  /sys/class/power_supply/tcpm-source-psy/online
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-write property describes the online state of the source
+   supply. When the value of this property is not 0, and the supply allows
+   it, then it's possible to switch between online states (i.e. 1 -> 2,
+   2 -> 1)
+
+   Valid values:
+   - 0 : Offline, no source supply attached
+   - 1 : Fixed Online, Type-C or USB-PD capable supply
+ attached, non-configurable current and voltage
+ properties in this state.
+   - 2 : PPS Online, USB-PD PPS feature enabled, 'current_now'
+ and 'voltage_now' properties can be modified in this
+ state. Re-writing of this value again, once already
+ set, will re-request the same configured voltage and
+ current values. This can be used as a keep-alive for
+ the PPS connection.
+ [NOTE: This is value only selectable if
+  'connected_type' reports a value of "USB_PD_PPS"]
+
+What:  /sys/class/power_supply/tcpm-source-psy/voltage_min
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-only property describes the minimum voltage the source supply
+   can provide.
+
+   Value in microvolts.
+
+What:  /sys/class/power_supply/tcpm-source-psy/voltage_max
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-only property describes the maximum voltage the source supply
+   can provide.
+
+   Value in microvolts.
+
+What:  /sys/class/power_supply/tcpm-source-psy/voltage_now
+Date:  December 2017
+Contact:   Adam Thomson 
+Description:
+   This read-write property describes the voltage the source supply is
+   providing now. This property can only be written to if the source supply
+   is in online state '2' (PPS enabled), otherwise it's read-only
+   information.
+
+   Value in microvolts.
+
+What:  /sys/class/power_supply/tcpm-source-psy/current_max
+Date:  December 2017
+Contact:   Adam Thomson 

[PATCH v3 7/7] typec: tcpm: Add support for sink PPS related messages

[Adam Thomson]

This commit adds sink side support for Get_Status, Status,
Get_PPS_Status and PPS_Status handling. As there's the
potential for a partner to respond with Not_Supported
handling of this message is also added. Sending of
Not_Supported is added is added to handle messages
received but not yet handled.

Signed-off-by: Adam Thomson 
---
 drivers/usb/typec/tcpm.c | 152 ---
 1 file changed, 143 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index f2bac3d..fe3bbb9 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -19,7 +19,9 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -113,6 +115,11 @@
S(SNK_TRYWAIT_VBUS),\
S(BIST_RX), \
\
+   S(GET_STATUS_SEND), \
+   S(GET_STATUS_SEND_TIMEOUT), \
+   S(GET_PPS_STATUS_SEND), \
+   S(GET_PPS_STATUS_SEND_TIMEOUT), \
+   \
S(ERROR_RECOVERY),  \
S(PORT_RESET),  \
S(PORT_RESET_WAIT_OFF)
@@ -143,6 +150,7 @@ enum pd_msg_request {
PD_MSG_NONE = 0,
PD_MSG_CTRL_REJECT,
PD_MSG_CTRL_WAIT,
+   PD_MSG_CTRL_NOT_SUPP,
PD_MSG_DATA_SINK_CAP,
PD_MSG_DATA_SOURCE_CAP,
 };
@@ -1413,10 +1421,42 @@ static int tcpm_validate_caps(struct tcpm_port *port, 
const u32 *pdo,
 /*
  * PD (data, control) command handling functions
  */
+static inline enum tcpm_state ready_state(struct tcpm_port *port)
+{
+   if (port->pwr_role == TYPEC_SOURCE)
+   return SRC_READY;
+   else
+   return SNK_READY;
+}
 
 static int tcpm_pd_send_control(struct tcpm_port *port,
enum pd_ctrl_msg_type type);
 
+static void tcpm_handle_alert(struct tcpm_port *port, const __le32 *payload,
+ int cnt)
+{
+   u32 p0 = le32_to_cpu(payload[0]);
+   unsigned int type = usb_pd_ado_type(p0);
+
+   if (!type) {
+   tcpm_log(port, "Alert message received with no type");
+   return;
+   }
+
+   /* Just handling non-battery alerts for now */
+   if (!(type & USB_PD_ADO_TYPE_BATT_STATUS_CHANGE)) {
+   switch (port->state) {
+   case SRC_READY:
+   case SNK_READY:
+   tcpm_set_state(port, GET_STATUS_SEND, 0);
+   break;
+   default:
+   tcpm_queue_message(port, PD_MSG_CTRL_WAIT);
+   break;
+   }
+   }
+}
+
 static void tcpm_pd_data_request(struct tcpm_port *port,
 const struct pd_message *msg)
 {
@@ -1502,6 +1542,14 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
tcpm_set_state(port, BIST_RX, 0);
}
break;
+   case PD_DATA_ALERT:
+   tcpm_handle_alert(port, msg->payload, cnt);
+   break;
+   case PD_DATA_BATT_STATUS:
+   case PD_DATA_GET_COUNTRY_INFO:
+   /* Currently unsupported */
+   tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP);
+   break;
default:
tcpm_log(port, "Unhandled data message type %#x", type);
break;
@@ -1584,6 +1632,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
break;
case PD_CTRL_REJECT:
case PD_CTRL_WAIT:
+   case PD_CTRL_NOT_SUPP:
switch (port->state) {
case SNK_NEGOTIATE_CAPABILITIES:
/* USB PD specification, Figure 8-43 */
@@ -1703,12 +1752,84 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
break;
}
break;
+   case PD_CTRL_GET_SOURCE_CAP_EXT:
+   case PD_CTRL_GET_STATUS:
+   case PD_CTRL_FR_SWAP:
+   case PD_CTRL_GET_PPS_STATUS:
+   case PD_CTRL_GET_COUNTRY_CODES:
+   /* Currently not supported */
+   tcpm_queue_message(port, PD_MSG_CTRL_NOT_SUPP);
+   break;
default:
tcpm_log(port, "Unhandled ctrl message type %#x", type);
break;
}
 }
 
+static void tcpm_pd_ext_msg_request(struct tcpm_port *port,
+   const struct pd_message *msg)
+{
+   enum pd_ext_msg_type type = pd_header_type_le(msg->header);
+   unsigned int data_size = 
pd_ext_header_data_size_le(msg->ext_msg.header);
+   u8 *data;
+
+   if (!(msg->ext_msg.header && PD_EXT_HDR_CHUNKED)) {
+   tcpm_log(port, "Unchunked extended messages unsupported");
+   return;
+   }
+
+   if (data_size > 

[PATCH v3 3/7] typec: tcpm: Add SDB header for Status message handling

[Adam Thomson]

This commit adds a header providing definitions for handling
Status messages. Currently the header only focuses on handling
incoming Status messages.

Signed-off-by: Adam Thomson 
---
 include/linux/usb/pd_ext_sdb.h | 31 +++
 1 file changed, 31 insertions(+)
 create mode 100644 include/linux/usb/pd_ext_sdb.h

diff --git a/include/linux/usb/pd_ext_sdb.h b/include/linux/usb/pd_ext_sdb.h
new file mode 100644
index 000..0eb83ce
--- /dev/null
+++ b/include/linux/usb/pd_ext_sdb.h
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2017 Dialog Semiconductor
+ *
+ * Author: Adam Thomson 
+ */
+
+#ifndef __LINUX_USB_PD_EXT_SDB_H
+#define __LINUX_USB_PD_EXT_SDB_H
+
+/* SDB : Status Data Block */
+enum usb_pd_ext_sdb_fields {
+   USB_PD_EXT_SDB_INTERNAL_TEMP = 0,
+   USB_PD_EXT_SDB_PRESENT_INPUT,
+   USB_PD_EXT_SDB_PRESENT_BATT_INPUT,
+   USB_PD_EXT_SDB_EVENT_FLAGS,
+   USB_PD_EXT_SDB_TEMP_STATUS,
+   USB_PD_EXT_SDB_DATA_SIZE,
+};
+
+/* Event Flags */
+#define USB_PD_EXT_SDB_EVENT_OCP   BIT(1)
+#define USB_PD_EXT_SDB_EVENT_OTP   BIT(2)
+#define USB_PD_EXT_SDB_EVENT_OVP   BIT(3)
+#define USB_PD_EXT_SDB_EVENT_CF_CV_MODEBIT(4)
+
+#define USB_PD_EXT_SDB_PPS_EVENTS  (USB_PD_EXT_SDB_EVENT_OCP | \
+USB_PD_EXT_SDB_EVENT_OTP | \
+USB_PD_EXT_SDB_EVENT_OVP)
+
+#endif /* __LINUX_USB_PD_EXT_SDB_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/7] typec: tcpm: Add PD Rev 3.0 definitions to PD header

[Adam Thomson]

This commit adds definitions for PD Rev 3.0 messages, including
APDO PPS and extended message support for TCPM.

Signed-off-by: Adam Thomson 
---
 include/linux/usb/pd.h | 187 +
 1 file changed, 175 insertions(+), 12 deletions(-)

diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index b3d41d7..09b570f 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -35,6 +35,13 @@ enum pd_ctrl_msg_type {
PD_CTRL_WAIT = 12,
PD_CTRL_SOFT_RESET = 13,
/* 14-15 Reserved */
+   PD_CTRL_NOT_SUPP = 16,
+   PD_CTRL_GET_SOURCE_CAP_EXT = 17,
+   PD_CTRL_GET_STATUS = 18,
+   PD_CTRL_FR_SWAP = 19,
+   PD_CTRL_GET_PPS_STATUS = 20,
+   PD_CTRL_GET_COUNTRY_CODES = 21,
+   /* 22-31 Reserved */
 };
 
 enum pd_data_msg_type {
@@ -43,13 +50,39 @@ enum pd_data_msg_type {
PD_DATA_REQUEST = 2,
PD_DATA_BIST = 3,
PD_DATA_SINK_CAP = 4,
-   /* 5-14 Reserved */
+   PD_DATA_BATT_STATUS = 5,
+   PD_DATA_ALERT = 6,
+   PD_DATA_GET_COUNTRY_INFO = 7,
+   /* 8-14 Reserved */
PD_DATA_VENDOR_DEF = 15,
+   /* 16-31 Reserved */
+};
+
+enum pd_ext_msg_type {
+   /* 0 Reserved */
+   PD_EXT_SOURCE_CAP_EXT = 1,
+   PD_EXT_STATUS = 2,
+   PD_EXT_GET_BATT_CAP = 3,
+   PD_EXT_GET_BATT_STATUS = 4,
+   PD_EXT_BATT_CAP = 5,
+   PD_EXT_GET_MANUFACTURER_INFO = 6,
+   PD_EXT_MANUFACTURER_INFO = 7,
+   PD_EXT_SECURITY_REQUEST = 8,
+   PD_EXT_SECURITY_RESPONSE = 9,
+   PD_EXT_FW_UPDATE_REQUEST = 10,
+   PD_EXT_FW_UPDATE_RESPONSE = 11,
+   PD_EXT_PPS_STATUS = 12,
+   PD_EXT_COUNTRY_INFO = 13,
+   PD_EXT_COUNTRY_CODES = 14,
+   /* 15-31 Reserved */
 };
 
 #define PD_REV10   0x0
 #define PD_REV20   0x1
+#define PD_REV30   0x2
+#define PD_MAX_REV PD_REV30
 
+#define PD_HEADER_EXT_HDR  BIT(15)
 #define PD_HEADER_CNT_SHIFT12
 #define PD_HEADER_CNT_MASK 0x7
 #define PD_HEADER_ID_SHIFT 9
@@ -59,18 +92,19 @@ enum pd_data_msg_type {
 #define PD_HEADER_REV_MASK 0x3
 #define PD_HEADER_DATA_ROLEBIT(5)
 #define PD_HEADER_TYPE_SHIFT   0
-#define PD_HEADER_TYPE_MASK0xf
+#define PD_HEADER_TYPE_MASK0x1f
 
-#define PD_HEADER(type, pwr, data, id, cnt)\
+#define PD_HEADER(type, pwr, data, rev, id, cnt, ext_hdr)  \
type) & PD_HEADER_TYPE_MASK) << PD_HEADER_TYPE_SHIFT) | \
 ((pwr) == TYPEC_SOURCE ? PD_HEADER_PWR_ROLE : 0) | \
 ((data) == TYPEC_HOST ? PD_HEADER_DATA_ROLE : 0) | \
-(PD_REV20 << PD_HEADER_REV_SHIFT) |\
+(rev << PD_HEADER_REV_SHIFT) | \
 (((id) & PD_HEADER_ID_MASK) << PD_HEADER_ID_SHIFT) |   \
-(((cnt) & PD_HEADER_CNT_MASK) << PD_HEADER_CNT_SHIFT))
+(((cnt) & PD_HEADER_CNT_MASK) << PD_HEADER_CNT_SHIFT) |\
+((ext_hdr) ? PD_HEADER_EXT_HDR : 0))
 
-#define PD_HEADER_LE(type, pwr, data, id, cnt) \
-   cpu_to_le16(PD_HEADER((type), (pwr), (data), (id), (cnt)))
+#define PD_HEADER_LE(type, pwr, data, rev, id, cnt) \
+   cpu_to_le16(PD_HEADER((type), (pwr), (data), (rev), (id), (cnt), (0)))
 
 static inline unsigned int pd_header_cnt(u16 header)
 {
@@ -102,16 +136,75 @@ static inline unsigned int pd_header_msgid_le(__le16 
header)
return pd_header_msgid(le16_to_cpu(header));
 }
 
+static inline unsigned int pd_header_rev(u16 header)
+{
+   return (header >> PD_HEADER_REV_SHIFT) & PD_HEADER_REV_MASK;
+}
+
+static inline unsigned int pd_header_rev_le(__le16 header)
+{
+   return pd_header_rev(le16_to_cpu(header));
+}
+
+#define PD_EXT_HDR_CHUNKED BIT(15)
+#define PD_EXT_HDR_CHUNK_NUM_SHIFT 11
+#define PD_EXT_HDR_CHUNK_NUM_MASK  0xf
+#define PD_EXT_HDR_REQ_CHUNK   BIT(10)
+#define PD_EXT_HDR_DATA_SIZE_SHIFT 0
+#define PD_EXT_HDR_DATA_SIZE_MASK  0x1ff
+
+#define PD_EXT_HDR(data_size, req_chunk, chunk_num, chunked)   
\
+   data_size) & PD_EXT_HDR_DATA_SIZE_MASK) << 
PD_EXT_HDR_DATA_SIZE_SHIFT) |\
+((req_chunk) ? PD_EXT_HDR_REQ_CHUNK : 0) | 
\
+(((chunk_num) & PD_EXT_HDR_CHUNK_NUM_MASK) << 
PD_EXT_HDR_CHUNK_NUM_SHIFT) |\
+((chunked) ? PD_EXT_HDR_CHUNKED : 0))
+
+#define PD_EXT_HDR_LE(data_size, req_chunk, chunk_num, chunked) \
+   cpu_to_le16(PD_EXT_HDR((data_size), (req_chunk), (chunk_num), 
(chunked)))
+
+static inline unsigned int pd_ext_header_chunk_num(u16 ext_header)
+{
+   return (ext_header >> PD_EXT_HDR_CHUNK_NUM_SHIFT) &
+   PD_EXT_HDR_CHUNK_NUM_MASK;
+}
+
+static inline unsigned int pd_ext_header_data_size(u16 ext_header)
+{
+   return (ext_header >> PD_EXT_HDR_DATA_SIZE_SHIFT) &
+   PD_EXT_HDR_DATA_SIZE_MASK;
+}
+
+static inline unsigned int 

Re: [PATCH] phy: rockchip-typec: Try to turn the PHY on several times

[Enric Balletbo Serra]

Hi,

2017-12-13 20:14 GMT+01:00 Doug Anderson :
> Hi,
>
> On Wed, Dec 13, 2017 at 4:41 AM, Enric Balletbo Serra
>  wrote:
>> Hi Doug,
>>
>> 2017-12-11 22:45 GMT+01:00 Douglas Anderson :
>>> Bind / unbind stress testing of the USB controller on rk3399 found
>>> that we'd often end up with lots of failures that looked like this:
>>>
>>>   phy phy-ff80.phy.9: phy poweron failed --> -110
>>>   dwc3 fe90.dwc3: failed to initialize core
>>>   dwc3: probe of fe90.dwc3 failed with error -110
>>>
>>> Those errors were sometimes seen at bootup too, in which case USB
>>> peripherals wouldn't work until unplugged and re-plugged in.
>>>
>>> I spent some time trying to figure out why the PHY was failing to
>>> power on but I wasn't able to.  Possibly this has to do with the fact
>>> that the PHY docs say that the USB controller "needs to be held in
>>> reset to hold pipe power state in P2 before initializing the Type C
>>> PHY" but that doesn't appear to be easy to do with the dwc3 driver
>>> today.  Messing around with the ordering of the reset vs. the PHY
>>> initialization in the dwc3 driver didn't seem to fix things.
>>>
>>> I did, however, find that if I simply retry the power on it seems to
>>> have a good chance of working.  So let's add some retries.  I ran a
>>> pretty tight bind/unbind loop overnight.  When I did so, I found that
>>> I need to retry between 1% and 2% of the time.  Overnight I found only
>>> a small handful of times where I needed 2 retries.  I never found a
>>> case where I needed 3 retries.
>>>
>>> I'm completely aware of the fact that this is quite an ugly hack and I
>>> wish I didn't have to resort to it, but I have no other real idea how
>>> to make this hardware reliable.  If Rockchip in the future can come up
>>> with a solution we can always revert this hack.  Until then, let's at
>>> least have something that works.
>>>
>>> This patch is tested atop Enric's latest dwc3 patch series ending at:
>>>   https://patchwork.kernel.org/patch/10095527/
>>> ...but it could be applied independently of that series without any
>>> bad effects.
>>>
>>> For some more details on this bug, you can refer to:
>>>   https://bugs.chromium.org/p/chromium/issues/detail?id=783464
>>>
>>> Signed-off-by: Douglas Anderson 
>>> ---
>>>
>>>  drivers/phy/rockchip/phy-rockchip-typec.c | 24 ++--
>>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c 
>>> b/drivers/phy/rockchip/phy-rockchip-typec.c
>>> index ee85fa0ca4b0..5c2157156ce1 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-typec.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-typec.c
>>> @@ -349,6 +349,8 @@
>>>  #define MODE_DFP_USB   BIT(1)
>>>  #define MODE_DFP_DPBIT(2)
>>>
>>> +#define POWER_ON_TRIES 5
>>> +
>>
>> I did the test of increase the number of tries to 100 because
>> unfortunately, even with this patch applied, I can see the problem on
>> my kevin with current mainline.
>>
>> [  244.309094] rockchip-typec-phy ff80.phy: Turn on failed after 100 
>> loops
>>
>> That's an extra debug print ^
>>
>> [  244.317019] phy phy-ff80.phy.8: phy poweron failed --> -110
>> [  244.323824] dwc3 fe90.dwc3: failed to initialize core
>> [  244.330057] dwc3: probe of fe90.dwc3 failed with error -110
>>
>> So I'm wondering if there is something else that I need to apply to
>> really fix this as you didn't reproduce the issue doing lots of tests
>> and I can reproduce the issue very easily.
>
> Ah!  I added that message to the top of my upstream series and,
> indeed, I sometimes see the PHY fail to turn on.  Doh.
>
> OK, so here's what I've done:
>
> * The place where I ran the overnight loops was actually the Chrome OS
> 4.4 kernel.  In that kernel I had a message very similar to yours and
> I didn't hit it.  I just re-ran this for 20 minutes now and I can
> re-confirm.  In the Chrome OS kernel I never see it needing more than
> a 1 (or 2) loops and it doesn't ever get into the "totally failed"
> case.
>

Ok, blame me, so the reason I saw that message often is because my DT
missed the entries to define the reset (fixed in the latest series I
sent) So completely my fault.

> * Previously I ran ~10 minutes with the upstream kernel, but at the
> time I didn't have your printout.  After 10 minutes I checked my logs
> and I definitely saw the "Needed 1 loops to turn on", so I knew my
> patch was doing something useful.  It didn't occur to me to re-confirm
> that I didn't get the "totally failed" upstream, though now that I say
> it out loud it's stupid that I didn't think to do this.
>

Doing your bind/unbind stress test with mainline I'm getting some
errors though, the 'phy poweron failed --> -110' disapeared and your
patch helps.

But sometimes I get :

[ 6451.626640] usb 5-1: new high-speed USB device number 2 using 

Re: Scatter Gather Implementation in DWC3

[Felipe Balbi]

Hi,

Arjav Parikh  writes:
> I want to use scatter gather logic in DWC3. As per code in DWC3 I
> identified that DWC3 supports scatter-gather implementation.
>
> Now, to check the working of scatter-gather I randomly prepared a
> buffer containing some strings and pass its address to sg list.
>
> I am doing the above operation in below  manner:
> 1. Created and array containing the strings which I want to send as
> packets over ECM gadget.
> 2. sg_init_table(array_name,array_length)
> sg_set_set_buf(array_list,pieces_packets,length_of_packets)
> 3. Copied the address of array_list, length of packet, no of scatter
> gather and transfer_complete routine to  into usb_request structure.
> Then this usb_request_structure is passed to usb_ep_queue function.
>
> Now expecting that whenever ping is done then these packets will also
> be transferred to USB which I am checking at other end using wireshark
> but I am not receiving these packets.
>
> On adding Debugs I identified that these packets are not
> scatter-gathered and sent in form of single buffers only.
>
> Can anyone please explain the procedure of scatter gather and how to
> check the scatter gather in DWC3.

where is your diff showing your changes to the ethernet gadget? There
is, btw, one gadget in tree which uses SG lists. It's the TCM gadget
fabric. You may want to try that one instead, since it's supposed to
work as is.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] dt-bindings: usb: add DT binding for RK3328 dwc3 controller

[Felipe Balbi]

Hi,

Heiko Stuebner  writes:

> Hi Greg, Felipe,
>
> Am Montag, 4. Dezember 2017, 10:40:38 CET schrieb Heiko Stuebner:
>> From: William Wu 
>> 
>> Adds the device tree bindings description for RK3328 and
>> compatible USB DWC3 controller.
>> 
>> Signed-off-by: William Wu 
>> Acked-by: Rob Herring 
>> Signed-off-by: Heiko Stuebner 
>
> do you want to pick this patch with only the small dt-binding
> change or Ack it for me to pick up together with the actual
> devicetree changes in the other patches?

it seems best if you carry this one:

Acked-by: Felipe Balbi 

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] dt-bindings: usb-xhci: add usb3-resume-missing-cas property

[Jun Li]

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: Saturday, December 16, 2017 6:26 AM
> To: Jun Li 
> Cc: mathias.ny...@intel.com; gre...@linuxfoundation.org;
> mark.rutl...@arm.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: usb-xhci: add usb3-resume-missing-cas
> property
> 
> On Tue, Dec 12, 2017 at 11:57:17AM +0800, Li Jun wrote:
> > Adding 'usb3-resume-missing-cas' property to enable XHCI_MISSING_CAS
> > quirk flag in case there is CAS missing if device plugged in S3.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  Documentation/devicetree/bindings/usb/usb-xhci.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > index e2ea59b..c413e188 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> > @@ -32,6 +32,8 @@ Optional properties:
> >- usb3-lpm-capable: determines if platform is USB3 LPM capable
> >- quirk-broken-port-ped: set if the controller has broken port disable
> mechanism
> >- imod-interval-ns: default interrupt moderation interval is 5000ns
> > +  - usb3-resume-missing-cas: set if the CAS(Cold Attach Status) may lose in
> case
> > +usb3 device plugged in while system sleep.
> 
> This should be implied by an SoC specific compatible string for the XHCI 
> block.

The quirk is already using on some PCI platforms(xhci-pci.c), so I think it will
be possible used for other SoCs as well in future.

thanks
Jun Li

> 
> Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html