[PATCH] ALSA: hda - Fixing background noise on Dell Inspiron 3162

[Kai-Heng Feng]

After login to the desktop on Dell Inspiron 3162,
there's a very loud background noise comes from the builtin speaker.
The noise does not go away even if the speaker is muted.

The noise disappears after using the aamix fixup.

Codec: Realtek ALC3234
Address: 0
AFG Function Id: 0x1 (unsol 1)
Vendor Id: 0x10ec0255
Subsystem Id: 0x10280725
Revision Id: 0x12
No Modem Function Group found

BugLink: http://bugs.launchpad.net/bugs/1549620
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/pci/hda/patch_realtek.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index efd4980..72fa58d 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4749,6 +4749,7 @@ enum {
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE,
ALC293_FIXUP_LENOVO_SPK_NOISE,
ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY,
+   ALC255_FIXUP_DELL_SPK_NOISE,
 };
 
 static const struct hda_fixup alc269_fixups[] = {
@@ -5368,6 +5369,12 @@ static const struct hda_fixup alc269_fixups[] = {
.type = HDA_FIXUP_FUNC,
.v.func = alc233_fixup_lenovo_line2_mic_hotkey,
},
+   [ALC255_FIXUP_DELL_SPK_NOISE] = {
+   .type = HDA_FIXUP_FUNC,
+   .v.func = alc_fixup_disable_aamix,
+   .chained = true,
+   .chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE
+   },
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -5410,6 +5417,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1028, 0x06df, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", 
ALC255_FIXUP_DELL_SPK_NOISE),
SND_PCI_QUIRK(0x1028, 0x164a, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1028, 0x164b, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x103c, 0x1586, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC2),
-- 
2.5.0

[PATCH] ALSA: hda - Fix headphone noise on Dell XPS 13 9360

[Kai-Heng Feng]

The headphone has noise when playing sound or switching microphone sources.
It uses the same codec on XPS 13 9350, but with different subsystem ID.
Applying the fixup can solve the issue.
Also, changing the model name to better differentiate models.

v2: Reorder by device ID.
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/pci/hda/patch_realtek.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 002f153..10ca900 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5466,8 +5466,9 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1028, 0x06de, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06df, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
-   SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13 9350", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", 
ALC255_FIXUP_DELL_SPK_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x075b, "Dell XPS 13 9360", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
SND_PCI_QUIRK(0x1028, 0x164a, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1028, 0x164b, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x103c, 0x1586, "HP", ALC269_FIXUP_HP_MUTE_LED_MIC2),
-- 
2.8.1

[PATCH] ALSA: hda - Fix headphone noise on Dell XPS 13 9360

[Kai-Heng Feng]

The headphone has noise when playing sound or switching microphone sources.
It uses the same codec on XPS 13 9350, but with different subsystem ID.
Applying the fixup can solve the issue.
Also, changing the model name to better differentiate models.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/pci/hda/patch_realtek.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 002f153..e6af865 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5466,7 +5466,8 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1028, 0x06de, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06df, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
-   SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13 9350", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x075b, "Dell XPS 13 9360", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", 
ALC255_FIXUP_DELL_SPK_NOISE),
SND_PCI_QUIRK(0x1028, 0x164a, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1028, 0x164b, "Dell", 
ALC293_FIXUP_DELL1_MIC_NO_PRESENCE),
-- 
2.8.1

[PATCH] Bluetooth: btusb: Add support for 0cf3:e009

[Kai-Heng Feng]

Device 0cf3:e009 is one of the QCA ROME family.

T:  Bus=01 Lev=01 Prnt=01 Port=07 Cnt=04 Dev#=  4 Spd=12  MxCh= 0
D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0cf3 ProdID=e009 Rev=00.01
C:  #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
I:  If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
I:  If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/bluetooth/btusb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c58a00c..80ae854 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -248,6 +248,7 @@ static const struct usb_device_id blacklist_table[] = {
 
/* QCA ROME chipset */
{ USB_DEVICE(0x0cf3, 0xe007), .driver_info = BTUSB_QCA_ROME },
+   { USB_DEVICE(0x0cf3, 0xe009), .driver_info = BTUSB_QCA_ROME },
{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME },
{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME },
{ USB_DEVICE(0x0489, 0xe092), .driver_info = BTUSB_QCA_ROME },
-- 
2.8.1

[PATCH] ALSA: hda - Enable subwoofer on Dell Inspiron 7559

[Kai-Heng Feng]

The subwoofer on Inspiron 7559 does not work originally.
Applying a pin fixup can make it work.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/pci/hda/patch_realtek.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 7100f05..2a3dd18 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4855,6 +4855,7 @@ enum {
ALC221_FIXUP_HP_FRONT_MIC,
ALC292_FIXUP_TPT460,
ALC298_FIXUP_SPK_VOLUME,
+   ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER,
 };
 
 static const struct hda_fixup alc269_fixups[] = {
@@ -5516,6 +5517,15 @@ static const struct hda_fixup alc269_fixups[] = {
.chained = true,
.chain_id = ALC298_FIXUP_DELL1_MIC_NO_PRESENCE,
},
+   [ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = {
+   .type = HDA_FIXUP_PINS,
+   .v.pins = (const struct hda_pintbl[]) {
+   { 0x1b, 0x90170151 },
+   { }
+   },
+   .chained = true,
+   .chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE
+   },
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -5560,6 +5570,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1028, 0x06df, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13 9350", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x0706, "Dell Inspiron 7559", 
ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER),
SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", 
ALC255_FIXUP_DELL_SPK_NOISE),
SND_PCI_QUIRK(0x1028, 0x075b, "Dell XPS 13 9360", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
SND_PCI_QUIRK(0x1028, 0x075d, "Dell AIO", ALC298_FIXUP_SPK_VOLUME),
-- 
2.9.3

Re: [PATCH] ALSA: hda - Enable subwoofer on Dell Inspiron 7559

[Kai Heng Feng]

On Tue, Aug 30, 2016 at 1:33 PM, Takashi Iwai <ti...@suse.de> wrote:
> On Tue, 30 Aug 2016 07:27:41 +0200,
> Kai-Heng Feng wrote:
>>
>> The subwoofer on Inspiron 7559 does not work originally.
>> Applying a pin fixup can make it work.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  sound/pci/hda/patch_realtek.c | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> index 7100f05..2a3dd18 100644
>> --- a/sound/pci/hda/patch_realtek.c
>> +++ b/sound/pci/hda/patch_realtek.c
>> @@ -4855,6 +4855,7 @@ enum {
>>   ALC221_FIXUP_HP_FRONT_MIC,
>>   ALC292_FIXUP_TPT460,
>>   ALC298_FIXUP_SPK_VOLUME,
>> + ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER,
>>  };
>>
>>  static const struct hda_fixup alc269_fixups[] = {
>> @@ -5516,6 +5517,15 @@ static const struct hda_fixup alc269_fixups[] = {
>>   .chained = true,
>>   .chain_id = ALC298_FIXUP_DELL1_MIC_NO_PRESENCE,
>>   },
>> + [ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = {
>> + .type = HDA_FIXUP_PINS,
>> + .v.pins = (const struct hda_pintbl[]) {
>> + { 0x1b, 0x90170151 },
>
> What's the original value of this pin?
>
>
> Takashi

The original value of 0x1b is 0x41f0.

[PATCH v2] ALSA: hda - Enable subwoofer on Dell Inspiron 7559

[Kai-Heng Feng]

The subwoofer on Inspiron 7559 was disabled originally.
Applying a pin fixup to node 0x1b can enable it and make it work.

Old pin: 0x41f0
New pin: 0x90170151

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/pci/hda/patch_realtek.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 7100f05..2a3dd18 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4855,6 +4855,7 @@ enum {
ALC221_FIXUP_HP_FRONT_MIC,
ALC292_FIXUP_TPT460,
ALC298_FIXUP_SPK_VOLUME,
+   ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER,
 };
 
 static const struct hda_fixup alc269_fixups[] = {
@@ -5516,6 +5517,15 @@ static const struct hda_fixup alc269_fixups[] = {
.chained = true,
.chain_id = ALC298_FIXUP_DELL1_MIC_NO_PRESENCE,
},
+   [ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = {
+   .type = HDA_FIXUP_PINS,
+   .v.pins = (const struct hda_pintbl[]) {
+   { 0x1b, 0x90170151 },
+   { }
+   },
+   .chained = true,
+   .chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE
+   },
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -5560,6 +5570,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1028, 0x06df, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", 
ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK),
SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13 9350", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
+   SND_PCI_QUIRK(0x1028, 0x0706, "Dell Inspiron 7559", 
ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER),
SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", 
ALC255_FIXUP_DELL_SPK_NOISE),
SND_PCI_QUIRK(0x1028, 0x075b, "Dell XPS 13 9360", 
ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE),
SND_PCI_QUIRK(0x1028, 0x075d, "Dell AIO", ALC298_FIXUP_SPK_VOLUME),
-- 
2.9.3

Re: [PATCH] ALSA: hda - Enable subwoofer on Dell Inspiron 7559

[Kai Heng Feng]

On Tue, Aug 30, 2016 at 3:05 PM, Takashi Iwai <ti...@suse.de> wrote:
> On Tue, 30 Aug 2016 08:25:18 +0200,
> Kai Heng Feng wrote:
>>
>> On Tue, Aug 30, 2016 at 1:33 PM, Takashi Iwai <ti...@suse.de> wrote:
>> > On Tue, 30 Aug 2016 07:27:41 +0200,
>> > Kai-Heng Feng wrote:
>> >>
>> >> The subwoofer on Inspiron 7559 does not work originally.
>> >> Applying a pin fixup can make it work.
>> >>
>> >> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> >> ---
>> >>  sound/pci/hda/patch_realtek.c | 11 +++
>> >>  1 file changed, 11 insertions(+)
>> >>
>> >> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
>> >> index 7100f05..2a3dd18 100644
>> >> --- a/sound/pci/hda/patch_realtek.c
>> >> +++ b/sound/pci/hda/patch_realtek.c
>> >> @@ -4855,6 +4855,7 @@ enum {
>> >>   ALC221_FIXUP_HP_FRONT_MIC,
>> >>   ALC292_FIXUP_TPT460,
>> >>   ALC298_FIXUP_SPK_VOLUME,
>> >> + ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER,
>> >>  };
>> >>
>> >>  static const struct hda_fixup alc269_fixups[] = {
>> >> @@ -5516,6 +5517,15 @@ static const struct hda_fixup alc269_fixups[] = {
>> >>   .chained = true,
>> >>   .chain_id = ALC298_FIXUP_DELL1_MIC_NO_PRESENCE,
>> >>   },
>> >> + [ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = {
>> >> + .type = HDA_FIXUP_PINS,
>> >> + .v.pins = (const struct hda_pintbl[]) {
>> >> + { 0x1b, 0x90170151 },
>> >
>> > What's the original value of this pin?
>> >
>> >
>> > Takashi
>>
>> The original value of 0x1b is 0x41f0.
>
> OK, so it was disabled.  Maybe better to mention it in the patch
> description.
>
>
> Takashi

OK, I'll resend one with better description. Thanks for the advice.

[PATCH] HID: alps: fix stick device not working after resume

[Kai-Heng Feng]

The stick device does not work after resume, add U1_SP_ABS_MODE flag can
make the device work after resume.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/hid/hid-alps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 048befd..390f8d3 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -244,13 +244,13 @@ static int alps_raw_event(struct hid_device *hdev,
 static int alps_post_reset(struct hid_device *hdev)
 {
return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1,
-   NULL, U1_TP_ABS_MODE, false);
+   NULL, U1_TP_ABS_MODE | U1_SP_ABS_MODE, false);
 }
 
 static int alps_post_resume(struct hid_device *hdev)
 {
return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1,
-   NULL, U1_TP_ABS_MODE, false);
+   NULL, U1_TP_ABS_MODE | U1_SP_ABS_MODE, false);
 }
 #endif /* CONFIG_PM */
 
-- 
2.9.3

Re: [PATCH] HID: alps: fix stick device not working after resume

[Kai Heng Feng]

On Mon, Sep 19, 2016 at 8:37 PM, Jiri Kosina <ji...@kernel.org> wrote:
> On Mon, 19 Sep 2016, Kai-Heng Feng wrote:
>
>> The stick device does not work after resume, add U1_SP_ABS_MODE flag can
>> make the device work after resume.
>
> Do you happen to have any more details on why it doesn't work without
> U1_SP_ABS_MODE? Or was this a pure guesswork?

It' pure guesswork, based on how the existing code uses U1_TP_ABS_MODE flag
on both initialization and resume.

I also tested the the patch on an ALPS touchpad without stick device,
did not notice
any side effect on suspend/resume, so I made the U1_SP_ABS_MODE flag mandatory.

>
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/hid/hid-alps.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
>> index 048befd..390f8d3 100644
>> --- a/drivers/hid/hid-alps.c
>> +++ b/drivers/hid/hid-alps.c
>> @@ -244,13 +244,13 @@ static int alps_raw_event(struct hid_device *hdev,
>>  static int alps_post_reset(struct hid_device *hdev)
>>  {
>>   return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1,
>> - NULL, U1_TP_ABS_MODE, false);
>> + NULL, U1_TP_ABS_MODE | U1_SP_ABS_MODE, false);
>>  }
>>
>>  static int alps_post_resume(struct hid_device *hdev)
>>  {
>>   return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1,
>> - NULL, U1_TP_ABS_MODE, false);
>> + NULL, U1_TP_ABS_MODE | U1_SP_ABS_MODE, false);
>>  }
>>  #endif /* CONFIG_PM */
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH] HID: alps: fix stick device not working after resume

[Kai Heng Feng]

On Wed, Sep 21, 2016 at 8:00 PM, Jiri Kosina <ji...@kernel.org> wrote:
> On Mon, 19 Sep 2016, Kai Heng Feng wrote:
>
>> >> The stick device does not work after resume, add U1_SP_ABS_MODE flag can
>> >> make the device work after resume.
>> >
>> > Do you happen to have any more details on why it doesn't work without
>> > U1_SP_ABS_MODE? Or was this a pure guesswork?
>>
>> It' pure guesswork, based on how the existing code uses U1_TP_ABS_MODE flag
>> on both initialization and resume.
>>
>> I also tested the the patch on an ALPS touchpad without stick device,
>> did not notice
>> any side effect on suspend/resume, so I made the U1_SP_ABS_MODE flag 
>> mandatory.
>
> I'll fold this information into the patch changelog before comitting; if
> you disagree, please let me know.

That will be great.
Appreciate!

>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Hi,

On Wed, Nov 9, 2016 at 8:32 PM, Bjørn Mork  wrote:
> Oliver Neukum  writes:
>
>> On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:
>>
>>> These problems could very well be caused by running at SuperSpeed
>>> (USB-3) instead of high speed (USB-2).

Yes, it's running at SuperSpeed, on a Kabylake laptop.

It does not have this issue on a Broadwell laptop, also running at SuperSpeed.

>>>
>>> Is there any way to test what happens when the device is attached to
>>> the computer by a USB-2 cable?  That would prevent it from operating at
>>> SuperSpeed.

I recall old Intel PCH can change the USB host from XHCI to EHCI,
newer PCH does not have this option.

Is there a way to force XHCI run at HighSpeed?

>>>
>>> The main point, however, is that the proposed patch doesn't seem to
>>> address the true problem, which is that the device gets suspended
>>> between probes.  The patch only tries to prevent it from being
>>> suspended during a probe -- which is already prevented by the USB core.
>>
>> But why doesn't it fail during normal operation?
>>
>> I suspect that its firmware requires the altsetting
>>
>> /* should we change control altsetting on a NCM/MBIM function? */
>> if (cdc_ncm_select_altsetting(intf) == CDC_NCM_COMM_ALTSETTING_MBIM) 
>> {
>> data_altsetting = CDC_NCM_DATA_ALTSETTING_MBIM;
>> ret = cdc_mbim_set_ctrlalt(dev, intf, 
>> CDC_NCM_COMM_ALTSETTING_MBIM);
>>
>> to be set before it accepts a suspension.
>
> Could be, but I don't think so.  The above code is effectively a noop
> unless the function is a combined NCM/MBIM function.  Something I've
> never seen on a Sierra Wireless device (ignoring the infamous EM7345,
> which really is an Intel device).
>
> This is a typical example of a Sierra Wireless modem configured for
> MBIM:
>
> P:  Vendor=1199 ProdID=9079 Rev= 0.06
> S:  Manufacturer=Sierra Wireless, Incorporated
> S:  Product=Sierra Wireless EM7455 Qualcomm Snapdragon X7 LTE-A
> S:  SerialNumber=LF615126xxx
> C:* #Ifs= 2 Cfg#= 1 Atr=a0 MxPwr=500mA
> A:  FirstIf#=12 IfCount= 2 Cls=02(comm.) Sub=0e Prot=00
> I:* If#=12 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=(none)
> E:  Ad=82(I) Atr=03(Int.) MxPS=  64 Ivl=32ms
> I:* If#=13 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=02 Driver=(none)
> I:  If#=13 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=(none)
> E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
>
>
> The control interface of plain MBIM functions will always have a single
> altsetting, like the example above. So cdc_ncm_select_altsetting(intf)
> returns "0", while CDC_NCM_COMM_ALTSETTING_MBIM is "1".
>
>
> Just for reference, using the Intel^H^H^H^H^HEM7345 as example, this is
> what a combined NCM/MBIM function looks like:
>
>
> P:  Vendor=1199 ProdID=a001 Rev=17.29
> S:  Manufacturer=Sierra Wireless Inc.
> S:  Product=Sierra Wireless EM7345 4G LTE
> S:  SerialNumber=013937000xx
> C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=100mA
> A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=0d Prot=00
> A:  FirstIf#= 2 IfCount= 2 Cls=02(comm.) Sub=02 Prot=01
> I:  If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0d Prot=00 Driver=cdc_mbim
> E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:* If#= 0 Alt= 1 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=cdc_mbim
> E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
> I:  If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 1 Alt= 2 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 2 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=02 Prot=01 Driver=(none)
> E:  Ad=83(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:* If#= 3 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=(none)
> E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
>
>
> And this is what the code you quote is trying to deal with.  Note the
> different subclass of altsetting 0 and 1 This is incredibly ugly.
>
> FWIW, the modem in question cannot be an EM7345. That modem does not
> have the static interface numbering oddity.  Another sign that it isn't
> a true Sierra device.

Yes, this modem is an EM7445.

>
>
>
>
> Bjørn

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Hi,

On Mon, Nov 7, 2016 at 7:02 PM, Oliver Neukum <oneu...@suse.com> wrote:
> On Fri, 2016-11-04 at 17:57 +0800, Kai-Heng Feng wrote:
>> Sometimes cdc_mbim failed to probe if runtime pm is enabled:
>> [9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
>>
>> This can be solved by increase its pm usage counter.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>
> For the record:
>
> NAK. This fixes a symptom. If this patch helps something is broken in
> device core. We need to find that.
>

Please check attached dmesg with usbcore.dyndbg="+p".

Thanks!

> Regards
> Oliver
>
>


dmesg
Description: Binary data

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

On Mon, Nov 14, 2016 at 3:34 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Fri, Nov 11, 2016 at 10:44 PM, Mathias Nyman
> <mathias.ny...@linux.intel.com> wrote:
>> On 10.11.2016 13:22, Oliver Neukum wrote:
>>>
>>> On Thu, 2016-11-10 at 12:09 +0100, Bjørn Mork wrote:
>>>>
>>>> Kai-Heng Feng <kai.heng.f...@canonical.com> writes:
>>>>>
>>>>> On Wed, Nov 9, 2016 at 8:32 PM, Bjørn Mork <bj...@mork.no> wrote:
>>>>>>
>>>>>> Oliver Neukum <oneu...@suse.com> writes:
>>>>>>
>>>>>>> On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:
>>>>>>>
>>>>>>>> These problems could very well be caused by running at SuperSpeed
>>>>>>>> (USB-3) instead of high speed (USB-2).
>>>>>
>>>>>
>>>>> Yes, it's running at SuperSpeed, on a Kabylake laptop.
>>>>>
>>>>> It does not have this issue on a Broadwell laptop, also running at
>>>>> SuperSpeed.
>>>>
>>>>
>>>> Then I must join Oliver, being very surprised by where in the stack you
>>>> attempt to fix the issue.  What you write above indicates a problem in
>>>> pci bridge or usb host controller, doesn't it?
>
> Yes, I was totally wrong about that.
>
>>>
>>>
>>> Indeed. And this means we need an XHCI specialist.
>>> Mathias, we have a failure specific to one implementation of XHCI.
>>>
>>
>>
>> Could be related to resume singnalling time.
>> Does the xhci fix for it in 4.9-rc3 help?
>>
>> commit 7d3b016a6f5a0fa610dfd02b05654c08fa4ae514
>> xhci: use default USB_RESUME_TIMEOUT when resuming ports.
>>
>> It doesn't directly explain why it would work on Broadwell but not Kabylake,
>> but it resolved very similar cases.
>>
>> If not, then adding dynamic debug for xhci could show something.
>
> I tried the latest commit, 6005a545cadb2adca64350c7aee17d002563e8c7,
> on for-usb-next branch.
>
> Now the cdc_mbim still probe failed at the first time, but somehow it
> re-probed again with a success.
>
> I reverted commit 7d3b016a6f5a0fa610dfd02b05654c08fa4ae514 and the
> behavior is the same, first time probed failed, second time probed
> success.
>
> The attached dmesg is with usbcore and xhci_hcd dynamic debug enabled.

I filed a bug report on bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=187861

>
>>
>> -Mathias
>>

[PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Sometimes cdc_mbim failed to probe if runtime pm is enabled:
[9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22

This can be solved by increase its pm usage counter.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/usb/usbnet.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index d5071e3..f77b4bf 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1674,12 +1674,15 @@ usbnet_probe (struct usb_interface *udev, const struct 
usb_device_id *prod)
net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
net->ethtool_ops = _ethtool_ops;
 
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   goto out1;
+
// allow device-specific bind/init procedures
// NOTE net->name still not usable ...
if (info->bind) {
status = info->bind (dev, udev);
if (status < 0)
-   goto out1;
+   goto out2;
 
// heuristic:  "usb%d" for links we know are two-host,
// else "eth%d" when there's reasonable doubt.  userspace
@@ -1772,6 +1775,8 @@ usbnet_probe (struct usb_interface *udev, const struct 
usb_device_id *prod)
 out3:
if (info->unbind)
info->unbind (dev, udev);
+out2:
+   usb_autopm_put_interface(dev->intf);
 out1:
/* subdrivers must undo all they did in bind() if they
 * fail it, but we may fail later and a deferred kevent
-- 
2.7.4

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

[snip]

> Let me explain it in more detail. rt5670 need to set a serious of
> registers to prevent the pop noise of powering up/down muting/
> unmuting headphone. That's what rt5670_hp_event() does. But,
> what rt286_hp_power_event do is only mute/unmute headphone
> which is done by "HPO L" and "HPO R" widget.

Thanks for the explanation.

[snip]

> If HPO is already muted as what we expected, it means "HPO L" and "HPO R"
> work properly. And there is no reason we create an event to do the same
> thing.

Can you advise me how to do a simple check on HPO L mute status?

>
>> >>
>> >> I found that the effect is most noticeable if the mute callback is
>> >> associated with "LDO2" and "HP Power".
>> >> But again, this is just what I observed.
>> >
>> > Could you try only associated with "LDO2"?
>> > It makes sense that will reduce the noise if a jack is plugged in/out
>> > when HPO is already powered up.
>>
>> Does it also help to reduce noise at other power events?
>
> I don't know. In theory, you shouldn't hear any sound when HPO
> is muted. If you need our help for debugging, please send a mail
> to our FAE and cc me.

Unfortunately it did happen. AMP mute did well for me and another user
- please check the bug report link.

>
>>
>> >
>> > I have question about the code below
>> > +   /* Fix headphone click noise */
>> > +   if (dmi_check_system(dmi_dell_dino))
>> > +   regmap_write(rt286->regmap,
>> > +   RT286_MIC1_DET_CTRL,
>> 0x0020);
>> > +
>> >
>> > What does this for? How did you get the value 0x0020?
>> > I just checked with Kailang, but he have no idea about that.
>>
>> It's PIN_VREFHIZ. It's from commit 3e1b0c4a9d56.
>
> snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ);
> 0x19 here means nid 0x19. But if you write 0x19 in rt286.c means
> write a hidden register with index 0x19. It is totally different.
> The corresponding code on rt286.c will be
> rt286->regmap(rt286->regmap,
> VERB_CMD(AC_VERB_SET_PIN_WIDGET_CONTROL, 0x19, 0x20));

Understood, will use it instead.

>>
>> >
>> >>
>> >> >
>> >> >
>> >>
>> >> --Please consider the environment before printing this e-mail.

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Tue, Mar 21, 2017 at 11:07 AM, Bard Liao <bardl...@realtek.com> wrote:
>> -Original Message-----
>> From: Kai-Heng Feng [mailto:kai.heng.f...@canonical.com]
>> Sent: Monday, March 20, 2017 11:59 AM
>> To: broo...@kernel.org
>> Cc: lgirdw...@gmail.com; Bard Liao; Oder Chiou;
>> alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; Kai-Heng Feng
>> Subject: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS
>> 9343 I2S mode
>>
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMD:
>> + case SND_SOC_DAPM_POST_PMD:
>> + case SND_SOC_DAPM_POST_PMU:
>> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO,
>> AMP_OUT_MUTE);
>> + break;
>> + case SND_SOC_DAPM_PRE_PMU:
>> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO,
>> AMP_OUT_UNMUTE);
>> + break;
>
> Besides Mark's comment, I have question here. It seems you want to mute
> HPO before "HP Power" is powered up and after "HP Power" is powered down.
> But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to

What I really want to do is something rt5670's rt5670_hp_event(),
maybe autodisable is not enough sometimes?

> "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power"
> is powered down. Any specific reason for muting HPO again before "HP Power"
> is powered up?

You are right. Either one of them should be sufficient.

> Will HPO be unmuted before "HP Power" is powered up on your system?

Yes.
I am no audio expert here - but from what I read from HDA, there's
actually no AMP unmute counterpart to AMP mute.

> Or should the event be associated with "LDO1"?  Which power will
> cause the click noise?

I found that the effect is most noticeable if the mute callback is
associated with "LDO2" and "HP Power".
But again, this is just what I observed.

>
>

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Tue, Mar 21, 2017 at 1:26 AM, Mark Brown <broo...@kernel.org> wrote:
> On Tue, Mar 21, 2017 at 12:23:53AM +0800, Kai-Heng Feng wrote:
>> On Tue, Mar 21, 2017 at 12:06 AM, Mark Brown <broo...@kernel.org> wrote:
>
>> > As it says there "...and inserted automatically following the three dash
>> > line".
>
>> I saw iteration changelog in git log all over the place, maybe add a
>> rule section for each subsystem?
>
> Some people won't push back, the only people who insist on anything
> different are the graphics people.

Got it. I think the way you suggested is better.

>
>> I had the same thought originally, but printk under each case suggests
>> otherwise - _POST_PMU is triggered not right after _PRE_PMU but right
>> before _PRE_PMD.
>
> Then you've broken something else on your system, that is obviously
> completely nonsensical and would break anything that relies on having a
> _POST_PMU event.  Why would we have two events that run at the same time
> one of which is obviously misnamed?

Hmm, that's weird though. I did the same test to rt286_spk_event()
(without applying the patch I sent), what I observed was the same:
_POST_PMU was triggered right after I stopped play sound, i.e. right
before _PRE_PMD not right after _PRE_PMU.

>
>> > > You didn't reply to my review comment and you sent the same code
>> > > again.
>> > That looks an awful lot like being ignored.
>
>> Fair enough, I thought changelog is sufficient.
>
> I'm not seeing anything in the changelog that addresses this.

[PATCH v2] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

HDA mode fixed the issue by these two commits:

'9476d369d7b3 ALSA: hda - Mute headphone pin on suspend on XPS13 9333'
'3e1b0c4a9d56 ALSA: hda - Fix click noise at start on Dell XPS13'

Apply the same workarounds to rt286 can solve the issue.

When jack is plugged, it rapidly generates I2C interrupts, which
triggers rt286_irq() and rt286_jack_detect(), which produces the click
noise.
alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
the frantic interrupts, hence avoids the click noise.

When rt286 is under powersaving state, play a sound with headphone or
plug a headphone in will produce a loud crack sound.
Set AMP_OUT_MUTE before power events can make the noise less noticeable.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/soc/codecs/rt286.c | 48 +---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index 9c365a7f758d..ec4caef045e9 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -36,6 +36,9 @@
 #define RT286_VENDOR_ID 0x10ec0286
 #define RT288_VENDOR_ID 0x10ec0288
 
+#define AMP_OUT_MUTE0xb080
+#define AMP_OUT_UNMUTE  0xb000
+
 struct rt286_priv {
struct reg_default *index_cache;
int index_cache_size;
@@ -47,6 +50,7 @@ struct rt286_priv {
struct delayed_work jack_detect_work;
int sys_clk;
int clk_id;
+   bool is_dell_dino;
 };
 
 static const struct reg_default rt286_index_def[] = {
@@ -472,6 +476,32 @@ static int rt286_set_dmic1_event(struct 
snd_soc_dapm_widget *w,
return 0;
 }
 
+/* Power event function to workaround headphone crack noise */
+static int rt286_hp_power_event(struct snd_soc_dapm_widget *w,
+struct snd_kcontrol *kcontrol, int event)
+{
+   struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+   struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+   if (!rt286->is_dell_dino)
+   return 0;
+
+   switch (event) {
+   case SND_SOC_DAPM_PRE_PMD:
+   case SND_SOC_DAPM_POST_PMD:
+   case SND_SOC_DAPM_POST_PMU:
+   snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
+   break;
+   case SND_SOC_DAPM_PRE_PMU:
+   snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_UNMUTE);
+   break;
+   default:
+   return 0;
+   }
+
+   return 0;
+}
+
 static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
 struct snd_kcontrol *kcontrol, int event)
 {
@@ -578,7 +608,9 @@ static const struct snd_soc_dapm_widget 
rt286_dapm_widgets[] = {
SND_SOC_DAPM_MUX("HPO Mux", SND_SOC_NOPM, 0, 0, _hpo_mux),
 
SND_SOC_DAPM_SUPPLY("HP Power", RT286_SET_PIN_HPO,
-   RT286_SET_PIN_SFT, 0, NULL, 0),
+   RT286_SET_PIN_SFT, 0, rt286_hp_power_event,
+   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_PRE_PMU |
+   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
 
/* Output Mixer */
SND_SOC_DAPM_MIXER("Front", RT286_SET_POWER(RT286_DAC_OUT1), 0, 1,
@@ -1175,8 +1207,10 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
if (pdata)
rt286->pdata = *pdata;
 
+   rt286->is_dell_dino = dmi_check_system(dmi_dell_dino) ? true : false;
+
if (dmi_check_system(force_combo_jack_table) ||
-   dmi_check_system(dmi_dell_dino))
+   rt286->is_dell_dino)
rt286->pdata.cbj_en = true;
 
regmap_write(rt286->regmap, RT286_SET_AUDIO_POWER, AC_PWRST_D3);
@@ -1192,6 +1226,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
regmap_update_bits(rt286->regmap,
RT286_CBJ_CTRL1, 0xf000, 0xb000);
} else {
+   /* Fix headphone click noise */
+   if (rt286->is_dell_dino)
+   regmap_write(rt286->regmap,
+   RT286_MIC1_DET_CTRL, 0x0020);
+
regmap_update_bits(rt286->regmap,
RT286_CBJ_CTRL1, 0xf000, 0x5000);
}
@@ -1215,7 +1254,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL3, 0xf777, 0x4737);
regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL4, 0x00ff, 0x003f);
 
-   if (dmi_check_system(dmi_dell_dino)) {
+   if (rt286->is_dell_dino) {
regmap_update_bits(rt286->regmap,
RT286_SET_GPIO_MASK, 0x40, 0x40);
regmap_update_bits(rt286->regmap,
@@ -1224,6 +1263,9 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
  

[PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

HDA mode fixed the issue by these two commits:

'9476d369d7b3 ALSA: hda - Mute headphone pin on suspend on XPS13 9333'
'3e1b0c4a9d56 ALSA: hda - Fix click noise at start on Dell XPS13'

Apply the same workarounds to rt286 can solve the issue.

When jack is plugged, it rapidly generates I2C interrupts, which
triggers rt286_irq() and rt286_jack_detect(), which produces the click
noise.
alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
the frantic interrupts, hence avoids the click noise.

When rt286 is under powersaving state, play a sound with headphone or
plug a headphone in will produce a loud crack sound.
Set AMP_OUT_MUTE before power events can make the noise less noticeable.
Unmute the AMP when the power is up.

v3:
Implicit conversion instead of tenary operator.

v2:
Use 'HP Power' instead of individual power events.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/soc/codecs/rt286.c | 48 +---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index 9c365a7f758d..97b52697f974 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -36,6 +36,9 @@
 #define RT286_VENDOR_ID 0x10ec0286
 #define RT288_VENDOR_ID 0x10ec0288
 
+#define AMP_OUT_MUTE0xb080
+#define AMP_OUT_UNMUTE  0xb000
+
 struct rt286_priv {
struct reg_default *index_cache;
int index_cache_size;
@@ -47,6 +50,7 @@ struct rt286_priv {
struct delayed_work jack_detect_work;
int sys_clk;
int clk_id;
+   bool is_dell_dino;
 };
 
 static const struct reg_default rt286_index_def[] = {
@@ -472,6 +476,32 @@ static int rt286_set_dmic1_event(struct 
snd_soc_dapm_widget *w,
return 0;
 }
 
+/* Power event function to workaround headphone crack noise */
+static int rt286_hp_power_event(struct snd_soc_dapm_widget *w,
+struct snd_kcontrol *kcontrol, int event)
+{
+   struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+   struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+   if (!rt286->is_dell_dino)
+   return 0;
+
+   switch (event) {
+   case SND_SOC_DAPM_PRE_PMD:
+   case SND_SOC_DAPM_POST_PMD:
+   case SND_SOC_DAPM_POST_PMU:
+   snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
+   break;
+   case SND_SOC_DAPM_PRE_PMU:
+   snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_UNMUTE);
+   break;
+   default:
+   return 0;
+   }
+
+   return 0;
+}
+
 static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
 struct snd_kcontrol *kcontrol, int event)
 {
@@ -578,7 +608,9 @@ static const struct snd_soc_dapm_widget 
rt286_dapm_widgets[] = {
SND_SOC_DAPM_MUX("HPO Mux", SND_SOC_NOPM, 0, 0, _hpo_mux),
 
SND_SOC_DAPM_SUPPLY("HP Power", RT286_SET_PIN_HPO,
-   RT286_SET_PIN_SFT, 0, NULL, 0),
+   RT286_SET_PIN_SFT, 0, rt286_hp_power_event,
+   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_PRE_PMU |
+   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
 
/* Output Mixer */
SND_SOC_DAPM_MIXER("Front", RT286_SET_POWER(RT286_DAC_OUT1), 0, 1,
@@ -1175,8 +1207,10 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
if (pdata)
rt286->pdata = *pdata;
 
+   rt286->is_dell_dino = dmi_check_system(dmi_dell_dino);
+
if (dmi_check_system(force_combo_jack_table) ||
-   dmi_check_system(dmi_dell_dino))
+   rt286->is_dell_dino)
rt286->pdata.cbj_en = true;
 
regmap_write(rt286->regmap, RT286_SET_AUDIO_POWER, AC_PWRST_D3);
@@ -1192,6 +1226,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
regmap_update_bits(rt286->regmap,
RT286_CBJ_CTRL1, 0xf000, 0xb000);
} else {
+   /* Fix headphone click noise */
+   if (rt286->is_dell_dino)
+   regmap_write(rt286->regmap,
+   RT286_MIC1_DET_CTRL, 0x0020);
+
regmap_update_bits(rt286->regmap,
RT286_CBJ_CTRL1, 0xf000, 0x5000);
}
@@ -1215,7 +1254,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL3, 0xf777, 0x4737);
regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL4, 0x00ff, 0x003f);
 
-   if (dmi_check_system(dmi_dell_dino)) {
+   if (rt286->is_dell_dino) {
regmap_update_bits(rt286->regmap,
RT286_SET_GPIO_MASK, 0x40, 0x40);
regmap_update_bits(

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Tue, Mar 21, 2017 at 12:06 AM, Mark Brown <broo...@kernel.org> wrote:
> On Mon, Mar 20, 2017 at 03:46:13PM +0000, Kai-Heng Feng wrote:
>> On Mon, Mar 20, 2017 at 11:08 PM Mark Brown <broo...@kernel.org> wrote:
>
>> > As covered in SubmittingPatches this should come after the ---, it
>> > doesn't need to end up in the changelogs.
>
>> Do you mean
>> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L197
>>  ?
>> I didn't find any hard rules regarding this, but I'll keep it in mind.
>
> As it says there "...and inserted automatically following the three dash
> line".

I saw iteration changelog in git log all over the place, maybe add a
rule section for each subsystem?

>
>> > > SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: +
>> > > snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); +
>> > > break; + case SND_SOC_DAPM_PRE_PMU:
>
> Please fix your mail client not to completely mangle quoted patches when
> replying.

Okay. I was toying with Google Inbox.

>
>> > To repeat what I said last time:
>
>> > | After power up we mute the amplifier?  That's worthy of a
>> > comment...
>
>> IIUC, HPO Power's  _POST_PMU is triggered right before power down
>> (_PRE_PMD), hence it's pretty logical to mute the amplifier at this
>> stage.  I can't quite see anything wrong here.
>
> No, that is not the case - I'm not sure what would lead you to believe
> that it is.  _POST_PMU is triggered as the last step of powering up the
> widget as the name might suggest.  Has this code been tested at all?

I had the same thought originally, but printk under each case suggests
otherwise - _POST_PMU is triggered not right after _PRE_PMU but right
before _PRE_PMD.

And yes, the patch was tested on a real machine.

>
>> So no I didn't ignore your comment, I simply misinterpreted what you
>> meant.  Because of the logical assumption, I thought you were talking
>> the unmute part in _PRE_PMU, which I did add in the changelog.
>
> You didn't reply to my review comment and you sent the same code again.
> That looks an awful lot like being ignored.

Fair enough, I thought changelog is sufficient.

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Tue, Mar 21, 2017 at 4:59 PM, Bard Liao <bardl...@realtek.com> wrote:
>> -Original Message-----
>> From: Kai-Heng Feng [mailto:kai.heng.f...@canonical.com]
>> Sent: Tuesday, March 21, 2017 1:39 PM
>> To: Bard Liao
>> Cc: broo...@kernel.org; lgirdw...@gmail.com; Oder Chiou;
>> alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell
>> XPS 9343 I2S mode
>> >>
>> >> + switch (event) {
>> >> + case SND_SOC_DAPM_PRE_PMD:
>> >> + case SND_SOC_DAPM_POST_PMD:
>> >> + case SND_SOC_DAPM_POST_PMU:
>> >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO,
>> >> AMP_OUT_MUTE);
>> >> + break;
>> >> + case SND_SOC_DAPM_PRE_PMU:
>> >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO,
>> >> AMP_OUT_UNMUTE);
>> >> + break;
>> >
>> > Besides Mark's comment, I have question here. It seems you want to mute
>> > HPO before "HP Power" is powered up and after "HP Power" is powered
>> down.
>> > But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to
>>
>> What I really want to do is something rt5670's rt5670_hp_event(),
>> maybe autodisable is not enough sometimes?
>
> It is different. rt5670_hp_event() is doing depop sequence for
> headphone. And there is no other mute/unmute controls on other
> dapm widgets. For me, what you do here is not different from
> "HPO L" and "HPO R" do.

There are two issues - background click noise and the cracking pop noise.
Depop is exactly what I want to do here.

>
>>
>> > "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power"
>> > is powered down. Any specific reason for muting HPO again before "HP
>> Power"
>> > is powered up?
>>
>> You are right. Either one of them should be sufficient.
>
> My point is that you seem to do things that driver is already done.
> But why and how it can reduce the click noise?

This is for the crack (pop) noise not click noise - see below.

>
>>
>> > Will HPO be unmuted before "HP Power" is powered up on your system?
>>
>> Yes.
>> I am no audio expert here - but from what I read from HDA, there's
>> actually no AMP unmute counterpart to AMP mute.
>
> I didn't get it. How did you check if HPO is muted?

I didn't. Now sure why do we need to check that?

>
>>
>> > Or should the event be associated with "LDO1"?  Which power will
>> > cause the click noise?
>>
>> I found that the effect is most noticeable if the mute callback is
>> associated with "LDO2" and "HP Power".
>> But again, this is just what I observed.
>
> Could you try only associated with "LDO2"?
> It makes sense that will reduce the noise if a jack is plugged in/out
> when HPO is already powered up.

Does it also help to reduce noise at other power events?

>
> I have question about the code below
> +   /* Fix headphone click noise */
> +   if (dmi_check_system(dmi_dell_dino))
> +   regmap_write(rt286->regmap,
> +   RT286_MIC1_DET_CTRL, 0x0020);
> +
>
> What does this for? How did you get the value 0x0020?
> I just checked with Kailang, but he have no idea about that.

It's PIN_VREFHIZ. It's from commit 3e1b0c4a9d56.

>
>>
>> >
>> >
>>
>> --Please consider the environment before printing this e-mail.

Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Tue, Mar 21, 2017 at 5:15 PM, Bard Liao <bardl...@realtek.com> wrote:
>> -Original Message-----
>> From: Kai-Heng Feng [mailto:kai.heng.f...@canonical.com]
>> Sent: Tuesday, March 21, 2017 1:26 PM
>> To: Mark Brown
>> Cc: Liam Girdwood; Bard Liao; Oder Chiou; alsa-de...@alsa-project.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell
>> XPS 9343 I2S mode
>> >
>> >> I had the same thought originally, but printk under each case suggests
>> >> otherwise - _POST_PMU is triggered not right after _PRE_PMU but right
>> >> before _PRE_PMD.
>> >
>> > Then you've broken something else on your system, that is obviously
>> > completely nonsensical and would break anything that relies on having a
>> > _POST_PMU event.  Why would we have two events that run at the same
>> time
>> > one of which is obviously misnamed?
>>
>> Hmm, that's weird though. I did the same test to rt286_spk_event()
>> (without applying the patch I sent), what I observed was the same:
>> _POST_PMU was triggered right after I stopped play sound, i.e. right
>> before _PRE_PMD not right after _PRE_PMU.
>>
>
> Although I don't think it will happen on my side, but I still
> ran the same test as you. The result is just as what we
> expected, _PRE_PMU and _POST_PMU run on powering
> up stage and _PRE_PMD and _POST_PMD run on powering
> down stage. Please check what's going on with your system.
>

Maybe mine is broken, maybe other XPS 9343 share the same behavior. I
guess we need more users to provide information to continue the
discussion.

>
>> --Please consider the environment before printing this e-mail.

[PATCH] ACPI / blacklist: add _REV quirk for Dell Inspiron 7537

[Kai-Heng Feng]

The battery can only be detected after AC power adapter event.

Adding the machine to acpi_rev_dmi_table[] can workaround this issue.

BugLink: https://bugs.launchpad.net/bugs/1678590
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=105721
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/acpi/blacklist.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index 4421f7c9981c..bb542acc0574 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -188,6 +188,14 @@ static struct dmi_system_id acpi_rev_dmi_table[] 
__initdata = {
  DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 3350"),
},
},
+   {
+.callback = dmi_enable_rev_override,
+.ident = "DELL Inspiron 7537",
+.matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron 7537"),
+   },
+   },
 #endif
{}
 };
-- 
2.12.2

[PATCH] Input: i8042 - add noloop quirk for Dell Embedded Box PC 3000

[Kai-Heng Feng]

The aux port does not get detected without noloop quirk, so external PS/2
mouse cannot work as result.

The PS/2 mouse can work with this quirk.

BugLink: https://bugs.launchpad.net/bugs/1591053
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/input/serio/i8042-x86ia64io.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/serio/i8042-x86ia64io.h 
b/drivers/input/serio/i8042-x86ia64io.h
index 05afd16ea9c9..e856b073d533 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -120,6 +120,13 @@ static const struct dmi_system_id __initconst 
i8042_dmi_noloop_table[] = {
},
},
{
+   /* Dell Embedded Box PC 3000 */
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Embedded Box PC 3000"),
+   },
+   },
+   {
/* OQO Model 01 */
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "OQO"),
-- 
2.12.0

[PATCH] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

The Realtek r8153 ethernet does not work on Genesys Logic hub, no-lpm
quirk can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..c96daf34431e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Moshi USB to Ethernet Adapter */
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.13.4

[PATCH] usb: quirks: add delay init quirk for Corsair Strafe RGB keyboard

[Kai-Heng Feng]

Corsair Strafe RGB keyboard has trouble to initialize:

[ 1.679455] usb 3-6: new full-speed USB device number 4 using xhci_hcd
[ 6.871136] usb 3-6: unable to read config index 0 descriptor/all
[ 6.871138] usb 3-6: can't read configurations, error -110
[ 6.991019] usb 3-6: new full-speed USB device number 5 using xhci_hcd
[ 12.246642] usb 3-6: unable to read config index 0 descriptor/all
[ 12.246644] usb 3-6: can't read configurations, error -110
[ 12.366555] usb 3-6: new full-speed USB device number 6 using xhci_hcd
[ 17.622145] usb 3-6: unable to read config index 0 descriptor/all
[ 17.622147] usb 3-6: can't read configurations, error -110
[ 17.742093] usb 3-6: new full-speed USB device number 7 using xhci_hcd
[ 22.997715] usb 3-6: unable to read config index 0 descriptor/all
[ 22.997716] usb 3-6: can't read configurations, error -110

Although it may work after several times unpluging/pluging:

[ 68.195240] usb 3-6: new full-speed USB device number 11 using xhci_hcd
[ 68.337459] usb 3-6: New USB device found, idVendor=1b1c, idProduct=1b20
[ 68.337463] usb 3-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 68.337466] usb 3-6: Product: Corsair STRAFE RGB Gaming Keyboard
[ 68.337468] usb 3-6: Manufacturer: Corsair
[ 68.337470] usb 3-6: SerialNumber: 0F013021AEB8046755A93ED3F5001941

Tried three quirks: USB_QUIRK_DELAY_INIT, USB_QUIRK_NO_LPM and
USB_QUIRK_DEVICE_QUALIFIER, user confirmed that USB_QUIRK_DELAY_INIT alone
can workaround this issue. Hence add the quirk for Corsair Strafe RGB.

BugLink: https://bugs.launchpad.net/bugs/1678477
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 574da2b4529c..1ea5060dae69 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -217,6 +217,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1a0a, 0x0200), .driver_info =
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
 
+   /* Corsair Strafe RGB */
+   { USB_DEVICE(0x1b1c, 0x1b20), .driver_info = USB_QUIRK_DELAY_INIT },
+
/* Acer C120 LED Projector */
{ USB_DEVICE(0x1de1, 0xc102), .driver_info = USB_QUIRK_NO_LPM },
 
-- 
2.14.1

[PATCH] Input: elan_i2c - add ELAN0608 to the ACPI table

[Kai-Heng Feng]

Similar to commit 722c5ac708b4f ("Input: elan_i2c - add ELAN0605 to the
ACPI table"), ELAN0608 should be handled by elan_i2c.

This touchpad can be found in Lenovo ideapad 320-14IKB.

BugLink: https://bugs.launchpad.net/bugs/1708852

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/input/mouse/elan_i2c_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c 
b/drivers/input/mouse/elan_i2c_core.c
index 3b616cb7c67f..9fe3908d12d5 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1248,6 +1248,7 @@ static const struct acpi_device_id elan_acpi_id[] = {
{ "ELAN0100", 0 },
{ "ELAN0600", 0 },
{ "ELAN0605", 0 },
+   { "ELAN0608", 0 },
{ "ELAN1000", 0 },
{ }
 };
-- 
2.14.0

Re: [PATCH] Input: i8042 - add Gigabyte P57 to the keyboard reset table

[Kai-Heng Feng]

On Wed, Jul 12, 2017 at 11:39 AM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> Similar to other Gigabyte laptops, the touchpad on P57 requires a
> keyboard reset to detect Elantech touchpad correctly.
>
> BugLink: https://bugs.launchpad.net/bugs/1594214
> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
> ---
>  drivers/input/serio/i8042-x86ia64io.h | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/input/serio/i8042-x86ia64io.h 
> b/drivers/input/serio/i8042-x86ia64io.h
> index 09720d950686..1c6a49b5b93d 100644
> --- a/drivers/input/serio/i8042-x86ia64io.h
> +++ b/drivers/input/serio/i8042-x86ia64io.h
> @@ -833,6 +833,13 @@ static const struct dmi_system_id __initconst 
> i8042_dmi_kbdreset_table[] = {
> },
> },
> {
> +   /* Gigabyte P57 - Elantech touchpad */
> +   .matches = {
> +   DMI_MATCH(DMI_SYS_VENDOR, "GIGABYTE"),
> +   DMI_MATCH(DMI_PRODUCT_NAME, "P57"),
> +   },
> +   },
> +   {
> /* Schenker XMG C504 - Elantech touchpad */
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "XMG"),
> --
> 2.13.2
>

Hi Dimitry,

Can you also take a look at this patch I sent in Jun? I guess this one
got slipped through :)

Thanks!

[PATCH] Input: i8042 - add Gigabyte P57 to the keyboard reset table

[Kai-Heng Feng]

Similar to other Gigabyte laptops, the touchpad on P57 requires a
keyboard reset to detect Elantech touchpad correctly.

BugLink: https://bugs.launchpad.net/bugs/1594214
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/input/serio/i8042-x86ia64io.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/serio/i8042-x86ia64io.h 
b/drivers/input/serio/i8042-x86ia64io.h
index 09720d950686..1c6a49b5b93d 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -833,6 +833,13 @@ static const struct dmi_system_id __initconst 
i8042_dmi_kbdreset_table[] = {
},
},
{
+   /* Gigabyte P57 - Elantech touchpad */
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "GIGABYTE"),
+   DMI_MATCH(DMI_PRODUCT_NAME, "P57"),
+   },
+   },
+   {
/* Schenker XMG C504 - Elantech touchpad */
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "XMG"),
-- 
2.13.2

Re: [PATCH v2] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

On Tue, Aug 8, 2017 at 4:28 PM, Oliver Neukum <oneu...@suse.com> wrote:
> Am Dienstag, den 08.08.2017, 14:32 +0800 schrieb Kai-Heng Feng:
>> Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
>> connect to Realtek r8153.
>>
>> The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
>> can make it work.
>>
>> Since another r8153 dongle at my hand does not have the issue, so add
>> the quirk to the hub instead.
>
> But in the comment you say you add it to the device!
> This makes no sense.

You are right, I forgot to update the comment. My bad.

>
> [..]
>> + /* Moshi USB to Ethernet Adapter */
>> + { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
>> +
>
> Now is this a hub or a device? If this is a Genesys Logic hub,
> you need to say that clearly and state that it is an internal hub.

It's the Genesys Logic hub that being used internally.

I'll update the comment more clearly.

Thanks.

>
> Regards
> Oliver
>

[PATCH v2] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
connect to Realtek r8153.

The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Clarify that the adapter uses a hub internally.

 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..c96daf34431e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Moshi USB to Ethernet Adapter */
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.13.4

[PATCH v3] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
connect to Realtek r8153.

The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the Genesys Logic hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v3: Update comment to reflect the quirk is for the hub.

v2: Clarify that the adapter uses a hub internally.

 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..65a87efc100e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Genesys Logic hub, internally used by Moshi USB to Ethernet Adapter 
*/
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.14.0

Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 3:51 PM, Mika Westerberg
 wrote:
> At this point we should find out that the ICM is already running and the
> function never calls pci2cio_write().

I guess you mean this code section:

/* Check if the ICM firmware is already running */
val = ioread32(nhi->iobase + REG_FW_STS);
if (val & REG_FW_STS_ICM_EN)
return 0;

>
> The reason why it is not happening needs to be resolved.
>
>>   pcie2cio_write()
>> pci_write_config_dword(pdev, vnd_cap + PCIE2CIO_WRDATA, data);
>>
>> > Is there an actual issue you are trying to solve here?
>>
>> Yes, please take a look at [1].
>>
>> Although both the patch I sent and the diff above still failed to
>> probe the device
>> But there are no more NULL pointer dereference.
>>
>> [1]  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1708043/comments/11
>
> I would like to understand what the actual problem is here because in
> normal cases we should not end up starting ICM firmware in the first
> place.
>
> So no, let's not fix it like this until we know the root cause.
>
> I'll be participating the discussion on the above bug in hopes we could
> figure out the root cause.

Thanks for the information and explanation.

Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
<mika.westerb...@linux.intel.com> wrote:
> On Mon, Aug 07, 2017 at 02:50:49PM +0800, Kai-Heng Feng wrote:
>> On Mon, Aug 7, 2017 at 12:49 PM, Kai-Heng Feng
>> <kai.heng.f...@canonical.com> wrote:
>> > In icm_ar_is_supported(), icm->upstream_port will be uninitialized if
>> > the hardware is not an Apple one.
>> >
>> > The uninitialized icm->upstream_port will later be dereferenced in
>> > pcie2cio_write(), causes a NULL pointer dereference issue.
>> >
>> > Commit f67cf491175a ("thunderbolt: Add support for Internal Connection
>> > Manager (ICM)") states that all Alpine Ridge will use ICM, so I guess
>> > it's safe to remove the Apple check.
>
> Yes, Alpine Ridge uses ICM but on Apple systems we need to additional
> steps to get it up and running. That's why the check is there. So no it
> cannot be removed.

If that's the case, it probably should be like this:

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index bdaac1ff00a5..95c255996ff0 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -888,9 +888,11 @@ static int icm_driver_ready(struct tb *tb)
struct icm *icm = tb_priv(tb);
int ret;

-   ret = icm_firmware_init(tb);
-   if (ret)
-   return ret;
+   if (is_apple()) {
+   ret = icm_firmware_init(tb);
+   if (ret)
+   return ret;
+   }

if (icm->safe_mode) {
tb_info(tb, "Thunderbolt host controller is in safe mode.\n");
---

The uninitialized icm->upstream_port, will be used at here:

icm_firmware_init()
  icm_firmware_start()
icm_firmware_reset()
  pcie2cio_write()
pci_write_config_dword(pdev, vnd_cap + PCIE2CIO_WRDATA, data);

> Is there an actual issue you are trying to solve here?

Yes, please take a look at [1].

Although both the patch I sent and the diff above still failed to
probe the device
But there are no more NULL pointer dereference.

[1]  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1708043/comments/11

>
>> > Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> > ---
>> >  drivers/thunderbolt/icm.c | 7 ---
>> >  1 file changed, 7 deletions(-)
>> >
>> > diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
>> > index bdaac1ff00a5..2ab25aac5446 100644
>> > --- a/drivers/thunderbolt/icm.c
>> > +++ b/drivers/thunderbolt/icm.c
>> > @@ -514,13 +514,6 @@ static bool icm_ar_is_supported(struct tb *tb)
>> > struct icm *icm = tb_priv(tb);
>> >
>> > /*
>> > -* Starting from Alpine Ridge we can use ICM on Apple machines
>> > -* as well. We just need to reset and re-enable it first.
>> > -*/
>> > -   if (!is_apple())
>> > -   return true;
>> > -
>> > -   /*
>
> How did you test this?

Re: [PATCH] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 5:08 PM, Oliver Neukum <oneu...@suse.com> wrote:
> Am Freitag, den 04.08.2017, 17:34 +0800 schrieb Kai-Heng Feng:
>> The Realtek r8153 ethernet does not work on Genesys Logic hub, no-lpm
>> quirk can make it work.
>
> So can you confirm it works with LPM on another hub?

Yes, another r8153 dongle (Dell Type-C to Ethernet) does not have this issue.

>
>> Since another r8153 dongle at my hand does not have the issue, so add
>> the quirk to the hub instead.
>
> This does not match the comment in the patch. This needs to
> be clarified.

The Moshi USB to Ethernet Adapter internally has a Genesys Logic hub.
Ethernet chip r8153 is connected to the Genesys Logic hub.

I'll resend a patch with this information if it's clear enough.

>
> Regards
> Oliver
>

Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 12:49 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> In icm_ar_is_supported(), icm->upstream_port will be uninitialized if
> the hardware is not an Apple one.
>
> The uninitialized icm->upstream_port will later be dereferenced in
> pcie2cio_write(), causes a NULL pointer dereference issue.
>
> Commit f67cf491175a ("thunderbolt: Add support for Internal Connection
> Manager (ICM)") states that all Alpine Ridge will use ICM, so I guess
> it's safe to remove the Apple check.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
> ---
>  drivers/thunderbolt/icm.c | 7 ---
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
> index bdaac1ff00a5..2ab25aac5446 100644
> --- a/drivers/thunderbolt/icm.c
> +++ b/drivers/thunderbolt/icm.c
> @@ -514,13 +514,6 @@ static bool icm_ar_is_supported(struct tb *tb)
> struct icm *icm = tb_priv(tb);
>
> /*
> -* Starting from Alpine Ridge we can use ICM on Apple machines
> -* as well. We just need to reset and re-enable it first.
> -*/
> -   if (!is_apple())
> -   return true;
> -
> -   /*
>  * Find the upstream PCIe port in case we need to do reset
>  * through its vendor specific registers.
>  */
> --
> 2.13.4
>

Forgot to CC LKML...

[PATCH] usb: xhci: Renesas uPD720202 needs short TX quirk

[Kai-Heng Feng]

When plugging Logitech C920 webcam, warning messages filled up dmesg:
[77117.655018] xhci_hcd :0c:00.0: WARN Successful completion on short TX: 
needs XHCI_TRUST_TX_LENGTH quirk?
[77117.659018] xhci_hcd :0c:00.0: WARN Successful completion on short TX: 
needs XHCI_TRUST_TX_LENGTH quirk?
[77122.622952] handle_tx_event: 541 callbacks suppressed

No more warning messages with XHCI_TRUST_TX_LENGTH applied.

BugLink: https://bugs.launchpad.net/bugs/1710548
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8071c8fdd15e..8566b43e19ba 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -202,8 +202,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == 0x0015) {
xhci->quirks |= XHCI_RESET_ON_RESUME;
+   xhci->quirks |= XHCI_TRUST_TX_LENGTH;
+   }
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
 
-- 
2.14.1

Re: [PATCH] usb: xhci: Renesas uPD720202 needs short TX quirk

[Kai-Heng Feng]

Hi,

On Fri, Aug 18, 2017 at 3:22 PM, Felipe Balbi
<felipe.ba...@linux.intel.com> wrote:
>
> hi,
>
> Kai-Heng Feng <kai.heng.f...@canonical.com> writes:
>> When plugging Logitech C920 webcam, warning messages filled up dmesg:
>> [77117.655018] xhci_hcd :0c:00.0: WARN Successful completion on short 
>> TX: needs XHCI_TRUST_TX_LENGTH quirk?
>> [77117.659018] xhci_hcd :0c:00.0: WARN Successful completion on short 
>> TX: needs XHCI_TRUST_TX_LENGTH quirk?
>
> have you confirmed this is needed for this controller?
I just found commit d2f48f05cd2a2 ("usb: xhci: ASMedia ASM1042A
chipset need shorts TX quirk") and did the same thing for this
controller.

> Anybody from Renesas has confirmed it?
No, it's a user reported problem, please check the bug report in the link.

> Do you have an errata document to refer to?
No. Probably need Renesas guy to provide it.

>
>> [77122.622952] handle_tx_event: 541 callbacks suppressed
>>
>> No more warning messages with XHCI_TRUST_TX_LENGTH applied.
>>
>> BugLink: https://bugs.launchpad.net/bugs/1710548
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/usb/host/xhci-pci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 8071c8fdd15e..8566b43e19ba 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -202,8 +202,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
>> xhci_hcd *xhci)
>>   xhci->quirks |= XHCI_BROKEN_STREAMS;
>>   }
>>   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> - pdev->device == 0x0015)
>> + pdev->device == 0x0015) {
>
> unnecessary
>
>>   xhci->quirks |= XHCI_RESET_ON_RESUME;
>> + xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> xhci->quirks |= XHCI_RESET_ON_RESUME |
> XHCI_TRUST_TX_LENGTH;
>
>> + }
>
> unnecessary

Do you mean that this quirk just hide the warning, it doesn't fix the
root cause?

>
> --
> balbi

[PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

As Alan Stern points out [1], the PME signal is not enabled when
controller is in D3, therefore it's not being woken up when new deivces
get plugged in.

Workaround this bug by preventing the controller enters D3 power state.

[1] https://www.spinics.net/lists/linux-usb/msg157462.html

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/ehci-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..616685f83954 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
if (pdev->device == 0x7808) {
ehci->use_dummy_qh = 1;
ehci_info(ehci, "applying AMD SB700/SB800/Hudson-2/3 
EHCI dummy qh workaround\n");
+
+   pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
}
break;
case PCI_VENDOR_ID_VIA:
-- 
2.13.0

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Tue, Jun 20, 2017 at 2:32 AM, Alan Stern  wrote:
>
> It's possible that the test was invalid.  Kai-Heng did not say whether
> /sys/.../power/wakeup was set to "enabled" for both the EHCI controller
> and the USB root hub beneath it, before the test was started.  If
> either of them was set to "disabled" then we would not expect a plug or
> unplug event to wake up the system.

You are right, it's "disabled" on USB root hub.
Changed it to "enabled", the test result remains the same.

>
> In any case, the controller should be set to the lowest power setting
> that is consistent with the desired wakeup behavior.  If wakeup is set
> to "enabled" then the state should be D2 -- if possible.  That's the
> theory, anyway.  If the system supports putting devices only into D3
> during S3 sleep then there's no choice, but if we do have a choice then
> we should take it.
>
> BTW, I just noticed that pci_target_state() uses device_may_wakeup() to
> get the desired wakeup behavior.  That is correct for system sleep, but
> it is wrong for runtime PM.  For runtime PM, wakeup should be enabled
> whenever the hardware allows it, so the test should be
> device_can_wakeup().
>
> This means that pci_target_state() should behave differently depending
> on whether it is called from pci_prepare_to_sleep() or from
> pci_finish_runtime_suspend().  Probably nobody noticed this before
> because it usually doesn't make any difference.
>
> Alan Stern
>

Re: [PATCH] PCI: Workaround AMD EHCI controller PME bug

[Kai-Heng Feng]

On Tue, Jun 20, 2017 at 2:01 AM, Bjorn Helgaas  wrote:
>
> Applied (patch below) to pci/pm for v4.13, thanks!
>
> Note that I added parens because bitwise NOT is higher precedence than
> bitwise shift right, and I think we want the shift before the NOT.  Please
> double-check.

You are right. NOT should be the last operation here.

Thanks!

[PATCH v4] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

Dell Latitude 3160 does not have keyboard backlight, but there is a
sysfs interface for it, which does nothing at all.

KBD_LED_ON_TOKEN is the only token can be found. Since it doesn't have
KBD_LED_OFF_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume
at least two tokens should be present to support keyboard backlight.
Not all models have ON token - they may have multiple AUTO tokens instead.

Models which do not use SMBIOS token to control keyboard backlight, also
have this issue. Brightness level is 0 on these models. Verified on Dell
Inspiron 3565.

Reports keyboard backlight is supported only when at least two modes are
present.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---

v4:
Change commit log - not all models have ON token.

v3:
Change the logic, there should be at least two modes present to claim it
supports backlight. Suggested by Pali Rohár.

v2:
The only token can be found is actually KBD_LED_ON_TOKEN, which is BIT(5),
instead of KBD_LED_OFF_TOKEN. Change commit log accordingly.

Use token bit to make the intention more clear.
Also consider kbd_mode_levels_count and kbd_info.levels, suggested by
Pali Rohár.

Use XOR to simplify the bitmask comparison, suggested by
Andy Shevchenko.

 drivers/platform/x86/dell-laptop.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c 
b/drivers/platform/x86/dell-laptop.c
index ec202094bd50..f42159fd2031 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1510,7 +1510,11 @@ static void kbd_init(void)
ret = kbd_init_info();
kbd_init_tokens();
 
-   if (kbd_token_bits != 0 || ret == 0)
+   /*
+* Only supports keyboard backlight when it has at least two modes.
+*/
+   if ((ret == 0 && (kbd_info.levels != 0 || kbd_mode_levels_count >= 2))
+   || kbd_get_valid_token_counts() >= 2)
kbd_led_present = true;
 }
 
-- 
2.13.1

[PATCH] nvme: explicitly disable APST on quirked devices

[Kai-Heng Feng]

A user reports APST is enabled, even when the NVMe is quirked or with
option "default_ps_max_latency_us=0".

The current logic will not set APST if the device is quirked. But the
NVMe in question will enable APST automatically.

Separate the logic "apst is supported" and "to enable apst", so we can
use the latter one to explicitly disable APST at initialiaztion.

BugLink: https://bugs.launchpad.net/bugs/1699004
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/nvme/host/core.c | 25 +
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0ddd6b9af7fc..c459d15d18f5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1477,6 +1477,14 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
if (!ctrl->apsta)
return;
 
+   if (!ctrl->apst_enabled) {
+   if (ctrl->state == NVME_CTRL_NEW ||
+   ctrl->state == NVME_CTRL_RESETTING)
+   dev_info(ctrl->device, "Disable APST at 
initialization\n");
+   else
+   return;
+   }
+
if (ctrl->npss > 31) {
dev_warn(ctrl->device, "NPSS is invalid; not using APST\n");
return;
@@ -1486,7 +1494,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
if (!table)
return;
 
-   if (ctrl->ps_max_latency_us == 0) {
+   if (ctrl->ps_max_latency_us == 0 || !ctrl->apst_enabled) {
/* Turn off APST. */
apste = 0;
dev_dbg(ctrl->device, "APST disabled\n");
@@ -1653,7 +1661,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
u64 cap;
int ret, page_shift;
u32 max_hw_sectors;
-   u8 prev_apsta;
+   bool prev_apst_enabled;
 
ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, >vs);
if (ret) {
@@ -1721,16 +1729,17 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
ctrl->kas = le16_to_cpu(id->kas);
 
ctrl->npss = id->npss;
-   prev_apsta = ctrl->apsta;
+   ctrl->apsta = id->apsta;
+   prev_apst_enabled = ctrl->apst_enabled;
if (ctrl->quirks & NVME_QUIRK_NO_APST) {
if (force_apst && id->apsta) {
dev_warn(ctrl->device, "forcibly allowing APST due to 
nvme_core.force_apst -- use at your own risk\n");
-   ctrl->apsta = 1;
+   ctrl->apst_enabled = true;
} else {
-   ctrl->apsta = 0;
+   ctrl->apst_enabled = false;
}
} else {
-   ctrl->apsta = id->apsta;
+   ctrl->apst_enabled = true;
}
memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
@@ -1760,9 +1769,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
kfree(id);
 
-   if (ctrl->apsta && !prev_apsta)
+   if (ctrl->apst_enabled && !prev_apst_enabled)
dev_pm_qos_expose_latency_tolerance(ctrl->device);
-   else if (!ctrl->apsta && prev_apsta)
+   else if (!ctrl->apst_enabled && prev_apst_enabled)
dev_pm_qos_hide_latency_tolerance(ctrl->device);
 
nvme_configure_apst(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ec8c7363934d..95d0cdafe0d5 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -165,6 +165,7 @@ struct nvme_ctrl {
 
/* Power saving configuration */
u64 ps_max_latency_us;
+   bool apst_enabled;
 
u32 hmpre;
u32 hmmin;
-- 
2.13.1

Re: [PATCH] nvme: explicitly disable APST on quirked devices

[Kai-Heng Feng]

On Sat, Jun 24, 2017 at 1:17 AM, Andy Lutomirski <l...@kernel.org> wrote:
> On Thu, Jun 22, 2017 at 11:19 PM, Kai-Heng Feng
> <kai.heng.f...@canonical.com> wrote:
>> A user reports APST is enabled, even when the NVMe is quirked or with
>> option "default_ps_max_latency_us=0".
>>
>> The current logic will not set APST if the device is quirked. But the
>> NVMe in question will enable APST automatically.
>>
>> Separate the logic "apst is supported" and "to enable apst", so we can
>> use the latter one to explicitly disable APST at initialiaztion.
>>
>> BugLink: https://bugs.launchpad.net/bugs/1699004
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/nvme/host/core.c | 25 +
>>  drivers/nvme/host/nvme.h |  1 +
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 0ddd6b9af7fc..c459d15d18f5 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1477,6 +1477,14 @@ static void nvme_configure_apst(struct nvme_ctrl 
>> *ctrl)
>> if (!ctrl->apsta)
>> return;
>>
>> +   if (!ctrl->apst_enabled) {
>> +   if (ctrl->state == NVME_CTRL_NEW ||
>> +   ctrl->state == NVME_CTRL_RESETTING)
>> +   dev_info(ctrl->device, "Disable APST at 
>> initialization\n");
>> +   else
>> +   return;
>> +   }
>> +
>
> Is this change really necessary?  ISTM that, if we want to optimize
> the case where we're not changing anything, we should do it more
> generally.

Do you mean combining the check on ctrl->apsta and ctrl->apst_enabled
if we do nothing and just want to return?

>
>> if (ctrl->npss > 31) {
>> dev_warn(ctrl->device, "NPSS is invalid; not using APST\n");
>> return;
>> @@ -1486,7 +1494,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
>> if (!table)
>> return;
>>
>> -   if (ctrl->ps_max_latency_us == 0) {
>> +   if (ctrl->ps_max_latency_us == 0 || !ctrl->apst_enabled) {
>> /* Turn off APST. */
>> apste = 0;
>> dev_dbg(ctrl->device, "APST disabled\n");
>> @@ -1653,7 +1661,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>> u64 cap;
>> int ret, page_shift;
>> u32 max_hw_sectors;
>> -   u8 prev_apsta;
>> +   bool prev_apst_enabled;
>>
>> ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, >vs);
>> if (ret) {
>> @@ -1721,16 +1729,17 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>> ctrl->kas = le16_to_cpu(id->kas);
>>
>> ctrl->npss = id->npss;
>> -   prev_apsta = ctrl->apsta;
>> +   ctrl->apsta = id->apsta;
>
> So ctrl->apsta now means, literally, is APSTA set in the features.
> This seems good.
>
>> +   prev_apst_enabled = ctrl->apst_enabled;
>> if (ctrl->quirks & NVME_QUIRK_NO_APST) {
>> if (force_apst && id->apsta) {
>> dev_warn(ctrl->device, "forcibly allowing APST due 
>> to nvme_core.force_apst -- use at your own risk\n");
>> -   ctrl->apsta = 1;
>> +   ctrl->apst_enabled = true;
>> } else {
>> -   ctrl->apsta = 0;
>> +   ctrl->apst_enabled = false;
>> }
>> } else {
>> -   ctrl->apsta = id->apsta;
>> +   ctrl->apst_enabled = true;
>
> Shouldn't this be ctrl->apst_enabled = id->apsta?
>
> The way you have it could cause us to do the wrong thing if id->apsta
> somehow changes between identifications.

You are right. It should be initialized with id->apsta.

I am curious though, when does NVMe do multiple identifications?

>
>
>> memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
>>
>> @@ -1760,9 +1769,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>>
>> kfree(id);
>>
>> -   if (ctrl->apsta && !prev_apsta)
>> +   if (ctrl->apst_enabled && !prev_apst_enabled)
>> dev_pm_qos_expose_latency_tolerance(ctrl->device);
>> -   else if (!ctrl->apsta && prev_apsta)
>> +   else if (!ctrl->apst_enabled && prev_apst_enabled)
>> dev_pm_qos_hide_latency_tolerance(ctrl->device);
>
> This is also wrong unless you make the change above, I think.

Thanks, I'll address these issues on later version.

>
> --Andy

Re: [PATCH v2] nvme: explicitly disable APST on quirked devices

[Kai-Heng Feng]

On Tue, Jun 27, 2017 at 2:05 AM, Andy Lutomirski <l...@kernel.org> wrote:
> On Mon, Jun 26, 2017 at 12:01 AM, Kai-Heng Feng
> <kai.heng.f...@canonical.com> wrote:
>> A user reports APST is enabled, even when the NVMe is quirked or with
>> option "default_ps_max_latency_us=0".
>>
>> The current logic will not set APST if the device is quirked. But the
>> NVMe in question will enable APST automatically.
>>
>> Separate the logic "apst is supported" and "to enable apst", so we can
>> use the latter one to explicitly disable APST at initialiaztion.
>
> Reviewed-by: Andy Lutomirski <l...@kernel.org>
>
> That being said, I smell a giant WTF here.  The affected hardware
> seems to have APST on by default, and APST is buggy so the disk stops
> working when APST is on.  So here's the $1M question: how does the
> system *boot*?  After all, it's running for a while before the kernel
> gets around to turning off APST, and I really doubt that BIOS does
> this.

>From my experience, systems never failed to boot on those faulty
NVMes. Probably because the constantly disk read required by boot
never let the NVMe transited to PS4. The problem always occurs after
some usage after boot.

Seems like the user has a tricky system. At first, APST wasn't
enabled. It's enabled after boot with a new kernel, and it's enabled
forever. Even if it's disabled explicitly, the APST is still enabled
by default on the system. The user didn't upgrade BIOS in the interim.

>
> Here's a wild theory: what if the problem on all these disks is
> actually our CSTS polling?  Could it be that some of the disks
> implement CSTS reads in firmware and malfunction if CSTS is read while
> in PS4?  This would be a blatant spec violation, but that's never
> stopped anyone before...
>
> --Andy

[PATCH v2] nvme: explicitly disable APST on quirked devices

[Kai-Heng Feng]

A user reports APST is enabled, even when the NVMe is quirked or with
option "default_ps_max_latency_us=0".

The current logic will not set APST if the device is quirked. But the
NVMe in question will enable APST automatically.

Separate the logic "apst is supported" and "to enable apst", so we can
use the latter one to explicitly disable APST at initialiaztion.

BugLink: https://bugs.launchpad.net/bugs/1699004
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2:
 - Remove unnecessary check on device state.
 - Correct apst_enabled value when there's no APST quirk matched.

 drivers/nvme/host/core.c | 17 +
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 903d5813023a..f0fd1bba3cdb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1372,7 +1372,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
if (!table)
return;
 
-   if (ctrl->ps_max_latency_us == 0) {
+   if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
/* Turn off APST. */
apste = 0;
dev_dbg(ctrl->device, "APST disabled\n");
@@ -1539,7 +1539,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
u64 cap;
int ret, page_shift;
u32 max_hw_sectors;
-   u8 prev_apsta;
+   bool prev_apst_enabled;
 
ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, >vs);
if (ret) {
@@ -1607,16 +1607,17 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
ctrl->kas = le16_to_cpu(id->kas);
 
ctrl->npss = id->npss;
-   prev_apsta = ctrl->apsta;
+   ctrl->apsta = id->apsta;
+   prev_apst_enabled = ctrl->apst_enabled;
if (ctrl->quirks & NVME_QUIRK_NO_APST) {
if (force_apst && id->apsta) {
dev_warn(ctrl->dev, "forcibly allowing APST due to 
nvme_core.force_apst -- use at your own risk\n");
-   ctrl->apsta = 1;
+   ctrl->apst_enabled = true;
} else {
-   ctrl->apsta = 0;
+   ctrl->apst_enabled = false;
}
} else {
-   ctrl->apsta = id->apsta;
+   ctrl->apst_enabled = id->apsta;
}
memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
@@ -1644,9 +1645,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
kfree(id);
 
-   if (ctrl->apsta && !prev_apsta)
+   if (ctrl->apst_enabled && !prev_apst_enabled)
dev_pm_qos_expose_latency_tolerance(ctrl->device);
-   else if (!ctrl->apsta && prev_apsta)
+   else if (!ctrl->apst_enabled && prev_apst_enabled)
dev_pm_qos_hide_latency_tolerance(ctrl->device);
 
nvme_configure_apst(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..dbabe2b728cb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -165,6 +165,7 @@ struct nvme_ctrl {
 
/* Power saving configuration */
u64 ps_max_latency_us;
+   bool apst_enabled;
 
/* Fabrics only */
u16 sqsize;
-- 
2.13.1

[PATCH] pinctrl: amd: don't load pinctrl-amd on Gigabyte AM4 boards

[Kai-Heng Feng]

On Gigabyte AM4 boards, pinctrl-amd generates tons of irq, makes the
system not able to boot properly.

Don't load the module until Gigabyte fixes the issue.

BugLink: https://bugs.launchpad.net/bugs/1671360
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/pinctrl/pinctrl-amd.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 1482d132fbb8..5b015d3552b8 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -739,6 +740,26 @@ static struct pinctrl_desc amd_pinctrl_desc = {
.owner = THIS_MODULE,
 };
 
+static bool amd_gpio_is_gigabyte_am4(void)
+{
+   const char *board_name;
+
+   if (!dmi_match(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."))
+   return false;
+
+   board_name = dmi_get_system_info(DMI_BOARD_NAME);
+
+   if (!board_name)
+   return false;
+
+   if (strstr(board_name, "A320") ||
+   strstr(board_name, "B350") ||
+   strstr(board_name, "X370"))
+   return true;
+
+   return false;
+}
+
 static int amd_gpio_probe(struct platform_device *pdev)
 {
int ret = 0;
@@ -746,6 +767,10 @@ static int amd_gpio_probe(struct platform_device *pdev)
struct resource *res;
struct amd_gpio *gpio_dev;
 
+   /* Don't load this module if it's a Gigabyte AM4 board */
+   if (amd_gpio_is_gigabyte_am4())
+   return -ENODEV;
+
gpio_dev = devm_kzalloc(>dev,
sizeof(struct amd_gpio), GFP_KERNEL);
if (!gpio_dev)
-- 
2.13.0

Re: [PATCH] pinctrl: amd: don't load pinctrl-amd on Gigabyte AM4 boards

[Kai-Heng Feng]

On Fri, May 26, 2017 at 6:09 PM, Thomas Gleixner <t...@linutronix.de> wrote:
> On Thu, 25 May 2017, Kai-Heng Feng wrote:
>
>> On Gigabyte AM4 boards, pinctrl-amd generates tons of irq, makes the
>> system not able to boot properly.
>>
>> Don't load the module until Gigabyte fixes the issue.
>
> NAK.
>
> That's the completely wrong approach. This crap will surface on a gazillion
> of other boards and will hit users faster than you can keep that DMI list
> up to date.

Yea. I'll try to understand your patch, and hopefully I can find
correct solutions next time.

>
> There is a proper solution to this problem:
>
> http://lkml.kernel.org/r/alpine.DEB.2.20.1705232307330.2409@nanos

Wow, this is great news for Gigabyte AM4 users!

Thanks for the patch, I'll build a custom Ubuntu kernel and get
feedback from users.

>
> Thanks,
>
> tglx

[PATCH 2/2] nvme: relax APST default max latency to 100ms

[Kai-Heng Feng]

Christoph Hellwig suggests we should to make APST work out of the box.
Hence relax the the default max latency to make them able to enter
deepest power state on default.

Here are id-ctrl excerpts from two high latency NVMes:

vid : 0x14a4
ssvid   : 0x1b4b
mn  : CX2-GB1024-Q11 NVMe LITEON 1024GB
ps3 : mp:0.1000W non-operational enlat:5000 exlat:5000 rrt:3 rrl:3
  rwt:3 rwl:3 idle_power:- active_power:-
ps4 : mp:0.0100W non-operational enlat:5 exlat:10 rrt:4 rrl:4
  rwt:4 rwl:4 idle_power:- active_power:-

vid : 0x15b7
ssvid   : 0x1b4b
mn  : A400 NVMe SanDisk 512GB
ps3 : mp:0.0500W non-operational enlat:51000 exlat:1 rrt:0 rrl:0
  rwt:0 rwl:0 idle_power:- active_power:-
ps4 : mp:0.0055W non-operational enlat:100 exlat:10 rrt:0 rrl:0
  rwt:0 rwl:0 idle_power:- active_power:-

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c07d8d4e18c9..903d5813023a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(max_retries, "max number of retries a 
command may have");
 static int nvme_char_major;
 module_param(nvme_char_major, int, 0);
 
-static unsigned long default_ps_max_latency_us = 25000;
+static unsigned long default_ps_max_latency_us = 10;
 module_param(default_ps_max_latency_us, ulong, 0644);
 MODULE_PARM_DESC(default_ps_max_latency_us,
 "max power saving latency for new devices; use PM QOS to 
change per device");
-- 
2.13.0

[PATCH 1/2] nvme: only consider exit latency when choosing useful non-op power states

[Kai-Heng Feng]

When a NVMe is in non-op states, the latency is exlat.
The latency will be enlat + exlat only when the NVMe tries to transit
from operational state right atfer it begins to transit to
non-operational state, which should be a rare case.

Therefore, as Andy Lutomirski suggests, use exlat only when deciding power
states to trainsit to.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/nvme/host/core.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0f9cc0c55e15..c07d8d4e18c9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1342,7 +1342,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 * transitioning between power states.  Therefore, when running
 * in any given state, we will enter the next lower-power
 * non-operational state after waiting 50 * (enlat + exlat)
-* microseconds, as long as that state's total latency is under
+* microseconds, as long as that state's exit latency is under
 * the requested maximum latency.
 *
 * We will not autonomously enter any non-operational state for
@@ -1387,7 +1387,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 * lowest-power state, not the number of states.
 */
for (state = (int)ctrl->npss; state >= 0; state--) {
-   u64 total_latency_us, transition_ms;
+   u64 total_latency_us, exit_latency_us, transition_ms;
 
if (target)
table->entries[state] = target;
@@ -1408,12 +1408,15 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  NVME_PS_FLAGS_NON_OP_STATE))
continue;
 
-   total_latency_us =
-   (u64)le32_to_cpu(ctrl->psd[state].entry_lat) +
-   + le32_to_cpu(ctrl->psd[state].exit_lat);
-   if (total_latency_us > ctrl->ps_max_latency_us)
+   exit_latency_us =
+   (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
+   if (exit_latency_us > ctrl->ps_max_latency_us)
continue;
 
+   total_latency_us =
+   exit_latency_us +
+   le32_to_cpu(ctrl->psd[state].entry_lat);
+
/*
 * This state is good.  Use it as the APST idle
 * target for higher power states.
-- 
2.13.0

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas  wrote:
>
> The lspci output [1] shows:
>
>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
> Controller (rev 39) (prog-if 20 [EHCI])
> Capabilities: [c0] Power Management version 2
>   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>   Bridge: PM- B3+
>
> The device claims it can assert PME# from D3hot.  If it can't, that
> sounds like a hardware defect that should be addressed with a quirk.
> Ideally we would also have a pointer to the AMD hardware erratum.

Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
with Low Speed Devices" in [1].
It points to a workaround in appendix A2 from [2].
However it says this bug only effects Low Speed devices, but it
actually also happens on High Speed devices.

[1] https://support.amd.com/TechDocs/46837.pdf
[2] https://support.amd.com/TechDocs/42413.pdf

>
> Is the following path involved here?
>
>   pci_finish_runtime_suspend
> target_state = pci_target_state()
>   if (device_may_wakup())
> if (dev->pme_support)
>   ...
> pci_set_power_state(..., target_state)

Yes, it's involved.

>
> If so, I would naively expect that a quirk could clear the
> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
> and pci_target_state() would then avoid selecting D3 or D3cold.  But
> I'm not an expert in power management.

Clearing those two bits does the trick, thanks for the tip.

>
> Bjorn
>
> [1] http://marc.info/?l=linux-usb=149570231732519=2

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Thu, Jun 15, 2017 at 2:55 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Tue, 13 Jun 2017, Bjorn Helgaas wrote:
>
>> [+cc Rafael, linux-pm]
>>
>> On Tue, Jun 13, 2017 at 12:21:15PM +0800, Kai-Heng Feng wrote:
>> > On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> > > Let's get some help from people who understand PCI well.
>> > >
>> > > Here's the general problem: Kai-Heng has a PCI-based USB host
>> > > controller that advertises wakeup capability from D3, but it doesn't
>> > > assert PME# from D3 when it should.  For "lspci -vv" output, see
>> > >
>> > > http://marc.info/?l=linux-usb=149570231732519=2
>> > >
>> > > On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
>> > >
>> > >> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
>> > >> <kai.heng.f...@canonical.com> wrote:
>> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern 
>> > >> > <st...@rowland.harvard.edu> wrote:
>> > >> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>> > >> >>
>> > >> >> Is this really the right solution?  Maybe it would be better to allow
>> > >> >> the controller to go into D3 provided no wakeup signal is needed.  
>> > >> >> You
>> > >> >> could do:
>> > >> >>
>> > >> >> device_set_wakeup_capable(>dev, 0);
>> > >> >
>> > >> > This doesn't work.
>> > >> > After applying this function, still nothing happens when devices get 
>> > >> > plugged in.
>> > >> > IIUC this function disable the wakeup function, but what I want to do
>> > >> > here is to have PME signal works even when runtime PM is enabled.
>> > >
>> > > This may indicate a bug in either the PCI or USB stacks (or both!).  If
>> > > a driver requires wakeup capability from runtime suspend but the device
>> > > does not provide it, the PCI core should not allow the device to go
>> > > into runtime suspend.  Or is that the driver's responsibility?
>> > >
>> > >> > I also saw some legacy PCI PM stuff, so I also tried:
>> > >> > device_set_wakeup_capable(>dev, 1);
>> > >> > ...doesn't work either.
>> > >> >
>> > >> >>
>> > >> >> Another alternative is to put the controller into D2 instead of D3, 
>> > >> >> but
>> > >> >> (1) I don't know how to do that, and (2) we don't know if wakeup
>> > >> >> signalling works any better in D2 than it does in D3.
>> > >> >
>> > >> > I'll try if D2 works.
>> > >>
>> > >> Put the device into D2 instead of D3 can make the wakeup signaling
>> > >> work, i.e. USB devices can be correctly detected after plugged into
>> > >> EHCI port.
>> > >>
>> > >> Do you think this alternative an acceptable workaround?
>> > >
>> > > Yes, it is.  The difficulty is that I don't know how to tell the PCI
>> > > core that the device should go in D2 during runtime suspend instead of
>> > > D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
>
>> The lspci output [1] shows:
>>
>>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
>> Controller (rev 39) (prog-if 20 [EHCI])
>> Capabilities: [c0] Power Management version 2
>>   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
>> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>>   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>>   Bridge: PM- B3+
>>
>> The device claims it can assert PME# from D3hot.  If it can't, that
>> sounds like a hardware defect that should be addressed with a quirk.
>> Ideally we would also have a pointer to the AMD hardware erratum.
>>
>> Is the following path involved here?
>>
>>   pci_finish_runtime_suspend
>> target_state = pci_target_state()
>>   if (device_may_wakup())
>>   if (dev->pme_support)
>> ...
>> pci_set_power_state(..., target_state)
>>
>> If so, I would naively expect that a quirk could clear the
>> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
>> and pci_target_state() would then avoid selecting D3 or D3cold.

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>
>> As Alan Stern points out [1], the PME signal is not enabled when
>> controller is in D3, therefore it's not being woken up when new deivces
>> get plugged in.
>>
>> Workaround this bug by preventing the controller enters D3 power state.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg157462.html
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/usb/host/ehci-pci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index 93326974ff4b..616685f83954 100644
>> --- a/drivers/usb/host/ehci-pci.c
>> +++ b/drivers/usb/host/ehci-pci.c
>> @@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>>   if (pdev->device == 0x7808) {
>>   ehci->use_dummy_qh = 1;
>>   ehci_info(ehci, "applying AMD SB700/SB800/Hudson-2/3 
>> EHCI dummy qh workaround\n");
>> +
>> + pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
>>   }
>>   break;
>>   case PCI_VENDOR_ID_VIA:
>
> Is this really the right solution?  Maybe it would be better to allow
> the controller to go into D3 provided no wakeup signal is needed.  You
> could do:
>
> device_set_wakeup_capable(>dev, 0);

This doesn't work.
After applying this function, still nothing happens when devices get plugged in.
IIUC this function disable the wakeup function, but what I want to do
here is to have PME signal works even when runtime PM is enabled.

I also saw some legacy PCI PM stuff, so I also tried:
device_set_wakeup_capable(>dev, 1);
...doesn't work either.

>
> Another alternative is to put the controller into D2 instead of D3, but
> (1) I don't know how to do that, and (2) we don't know if wakeup
> signalling works any better in D2 than it does in D3.

I'll try if D2 works.

Thanks for the review.

>
> Alan Stern
>

[PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

Dell Latitude 3160 does not have keyboard backlight, but there is a
sysfs interface for it, which does nothing at all.

KBD_LED_OFF_TOKEN is the only token can be found. Since it doesn't have
KBD_LED_ON_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
does not support keyboard backlight.

Reports keyboard backlight is supported only when tokens other than
KBD_LED_OFF_TOKEN can be found.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c 
b/drivers/platform/x86/dell-laptop.c
index ec202094bd50..743d7ce8c0c8 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1510,7 +1510,11 @@ static void kbd_init(void)
ret = kbd_init_info();
kbd_init_tokens();
 
-   if (kbd_token_bits != 0 || ret == 0)
+   /*
+* If KBD_LED_OFF_TOKEN is the only token,
+* consider there is no keyboard backlight.
+*/
+   if ((kbd_token_bits & ~BIT(5)) != 0 || ret == 0)
kbd_led_present = true;
 }
 
-- 
2.13.1

Re: [PATCH] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

On Fri, Jun 16, 2017 at 3:52 PM, Pali Rohár  wrote:
> Should not this check to be rather:
>
> (kbd_token_bits != 0 && (kbd_token_bits & BIT(KBD_LED_OFF_TOKEN)) != 
> BIT(KBD_LED_OFF_TOKEN))
>
> To express that we have at least one token at it is different from
> KBD_LED_OFF_TOKEN token?

Yes, this expresses the intention more clearly. I'll use it instead.

>
>>   kbd_led_present = true;
>>  }
>>
>
> And more important, there are three ways how to control keyboard
> backlight level:
>
> 1) Via SMBIOS token
> 2) Via SMBIOS call 4/11/0x2 (arg2, byte0)
> 3) Via SMBIOS call 4/11/0x2 (arg3, byte2)
>
> You are adding special case when only one SMBIOS toekn OFF is present
> which belongs to 1).
>
> Therefore there should be same check for 2) and 3) that there are more
> the one option to set...

I am not familiar with SMBIOS call.
Can you point out where 2) and 3) functions are?

>
> --
> Pali Rohár
> pali.ro...@gmail.com

[PATCH] PCI: Workaround AMD EHCI controller PME bug

[Kai-Heng Feng]

On an AMD Carrizo laptop, when EHCI runtime PM is enabled, EHCI ports do
not respond to any device plugging event.

As Alan Stern points out [1], the PME signal is not enabled when
controller is in D3, therefore it's not being woken up when new deivces
get plugged in.

Testing shows PME signal works when the EHCI power state is D2.

Bjorn Helgaas suggests to flip bits PCI_PM_CAP_PME_D3 and
PCI_PM_CAP_PME_D3cold in PCI fixup.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196091
Link: https://support.amd.com/TechDocs/46837.pdf (Section 23)
Link: https://support.amd.com/TechDocs/42413.pdf (Appendix A2)
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 arch/x86/pci/fixup.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6d52b94f4bb9..0f71a908e262 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -571,3 +571,18 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, 
pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
+
+/*
+ * Device [1022:7808]
+ * 23. USB Wake on Connect/Disconnect with Low Speed Devices
+ * https://support.amd.com/TechDocs/46837.pdf
+ * Appendix A2
+ * https://support.amd.com/TechDocs/42413.pdf
+ */
+static void pci_fixup_amd_ehci_pme(struct pci_dev *dev)
+{
+   dev_info(>dev, "PME# does not work under D3, disabling it\n");
+   dev->pme_support &= ~(PCI_PM_CAP_PME_D3 | PCI_PM_CAP_PME_D3cold)
+   >> PCI_PM_CAP_PME_SHIFT;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x7808, pci_fixup_amd_ehci_pme);
-- 
2.13.1

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 15 Jun 2017, Kai-Heng Feng wrote:
>
>> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <helg...@kernel.org> wrote:
>> >
>> > The lspci output [1] shows:
>> >
>> >   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
>> > Controller (rev 39) (prog-if 20 [EHCI])
>> > Capabilities: [c0] Power Management version 2
>> >   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
>> > PME(D0+,D1+,D2+,D3hot+,D3cold+)
>> >   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>> >   Bridge: PM- B3+
>> >
>> > The device claims it can assert PME# from D3hot.  If it can't, that
>> > sounds like a hardware defect that should be addressed with a quirk.
>> > Ideally we would also have a pointer to the AMD hardware erratum.
>>
>> Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
>> with Low Speed Devices" in [1].
>> It points to a workaround in appendix A2 from [2].
>> However it says this bug only effects Low Speed devices, but it
>> actually also happens on High Speed devices.
>>
>> [1] https://support.amd.com/TechDocs/46837.pdf
>> [2] https://support.amd.com/TechDocs/42413.pdf
>
> Those documents refer to a hardware bug with a workaround in the BIOS.
> Have you checked to see if your BIOS is up to date?

Yes, it's up to date.

>
> Alan Stern
>

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> 
> wrote:
>> Those documents refer to a hardware bug with a workaround in the BIOS.
>> Have you checked to see if your BIOS is up to date?
>
> Yes, it's up to date.
>

Alan, I re-sent a patch but I forgot to add you to CC list:
http://marc.info/?l=linux-pci=149760607914628=2

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Sat, Jun 17, 2017 at 1:30 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
>
>> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
>> <kai.heng.f...@canonical.com> wrote:
>> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> >> Those documents refer to a hardware bug with a workaround in the BIOS.
>> >> Have you checked to see if your BIOS is up to date?
>> >
>> > Yes, it's up to date.
>> >
>>
>> Alan, I re-sent a patch but I forgot to add you to CC list:
>> http://marc.info/?l=linux-pci=149760607914628=2
>
> Thanks for letting me know.  The patch seems reasonable.
>
> Have you tested it with system suspend?  That is, if you suspend the
> whole computer, does plugging or unplugging a USB device cause the
> system to wake up?

No, the system will not wake up when plugging or unplugging.
Tried several times, nether runtime PM enabled nor runtime PM disabled
will wake up the system under S3, when (un)plugging USB devices.

>
> Alan Stern
>

Re: [PATCH v2] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

On Mon, Jun 19, 2017 at 3:57 PM, Pali Rohár <pali.ro...@gmail.com> wrote:
> On Monday 19 June 2017 15:36:29 Kai-Heng Feng wrote:
>> Dell Latitude 3160 does not have keyboard backlight, but there is a
>> sysfs interface for it, which does nothing at all.
>>
>> KBD_LED_ON_TOKEN is the only token can be found. Since it doesn't have
>
> *ON* really?
>
>> KBD_LED_OFF_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
>> does not support keyboard backlight.
>>
>> Models which do not use SMBIOS to control keyboard backlight, also have
>> this issue. Brightness level is 0 on these models. Verified on Dell
>> Inspiron 3565.
>>
>> Reports keyboard backlight is supported only when tokens other than
>> KBD_LED_ON_TOKEN can be found, or brightness level is not 0.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>
>> v2:
>> The only token can be found is actually KBD_LED_ON_TOKEN, which is BIT(5),
>> instead of KBD_LED_OFF_TOKEN. Change commit log accordingly.
>>
>> Use token bit to make the intention more clear.
>> Also consider kbd_mode_levels_count and kbd_info.levels, suggested by
>> Pali Rohár.
>>
>> Use XOR to simplify the bitmask comparison, suggested by
>> Andy Shevchenko.
>>
>>  drivers/platform/x86/dell-laptop.c | 16 +++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c 
>> b/drivers/platform/x86/dell-laptop.c
>> index ec202094bd50..4b21fc982cb0 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -1148,6 +1148,15 @@ static const int kbd_tokens[] = {
>>   KBD_LED_ON_TOKEN,
>>  };
>>
>> +enum kbd_led_token_bit {
>> + KBD_LED_TOKEN_BIT_OFF = 0,
>> + KBD_LED_TOKEN_BIT_AUTO_25,
>> + KBD_LED_TOKEN_BIT_AUTO_50,
>> + KBD_LED_TOKEN_BIT_AUTO_75,
>> + KBD_LED_TOKEN_BIT_AUTO_100,
>> + KBD_LED_TOKEN_BIT_ON,
>> +};
>> +
>>  static u16 kbd_token_bits;
>>
>>  static struct kbd_info kbd_info;
>> @@ -1510,7 +1519,12 @@ static void kbd_init(void)
>>   ret = kbd_init_info();
>>   kbd_init_tokens();
>>
>> - if (kbd_token_bits != 0 || ret == 0)
>> + /*
>> +  * If brightness level is 0, or KBD_LED_ON_TOKEN is the only token,
>> +  * consider there is no keyboard backlight.
>> +  */
>> + if ((ret == 0 && (kbd_info.levels || kbd_mode_levels_count)) ||
>> + kbd_token_bits ^ BIT(KBD_LED_TOKEN_BIT_ON))
>
> So in case there would be only *OFF* token then interface is exported
> too... This does not seems to be good idea.
>
> If there is machine for which firmware provides only *ON* token, then I
> think we should presence for at least two tokens.

So I can use kbd_get_valid_token_counts() >= 2 here.

>
> And similarly kbd_mode_levels_count needs to be checked that is at least
> two (as "off" is in count).

So it should be kbd_mode_levels_count >= 2.
The same rule should also apply to kbd_info.levels, right?

>
>>   kbd_led_present = true;
>>  }
>>
>
> --
> Pali Rohár
> pali.ro...@gmail.com

[PATCH v2] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

Dell Latitude 3160 does not have keyboard backlight, but there is a
sysfs interface for it, which does nothing at all.

KBD_LED_ON_TOKEN is the only token can be found. Since it doesn't have
KBD_LED_OFF_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume it
does not support keyboard backlight.

Models which do not use SMBIOS to control keyboard backlight, also have
this issue. Brightness level is 0 on these models. Verified on Dell
Inspiron 3565.

Reports keyboard backlight is supported only when tokens other than
KBD_LED_ON_TOKEN can be found, or brightness level is not 0.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---

v2:
The only token can be found is actually KBD_LED_ON_TOKEN, which is BIT(5),
instead of KBD_LED_OFF_TOKEN. Change commit log accordingly.

Use token bit to make the intention more clear.
Also consider kbd_mode_levels_count and kbd_info.levels, suggested by
Pali Rohár.

Use XOR to simplify the bitmask comparison, suggested by
Andy Shevchenko.

 drivers/platform/x86/dell-laptop.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c 
b/drivers/platform/x86/dell-laptop.c
index ec202094bd50..4b21fc982cb0 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1148,6 +1148,15 @@ static const int kbd_tokens[] = {
KBD_LED_ON_TOKEN,
 };
 
+enum kbd_led_token_bit {
+   KBD_LED_TOKEN_BIT_OFF = 0,
+   KBD_LED_TOKEN_BIT_AUTO_25,
+   KBD_LED_TOKEN_BIT_AUTO_50,
+   KBD_LED_TOKEN_BIT_AUTO_75,
+   KBD_LED_TOKEN_BIT_AUTO_100,
+   KBD_LED_TOKEN_BIT_ON,
+};
+
 static u16 kbd_token_bits;
 
 static struct kbd_info kbd_info;
@@ -1510,7 +1519,12 @@ static void kbd_init(void)
ret = kbd_init_info();
kbd_init_tokens();
 
-   if (kbd_token_bits != 0 || ret == 0)
+   /*
+* If brightness level is 0, or KBD_LED_ON_TOKEN is the only token,
+* consider there is no keyboard backlight.
+*/
+   if ((ret == 0 && (kbd_info.levels || kbd_mode_levels_count)) ||
+   kbd_token_bits ^ BIT(KBD_LED_TOKEN_BIT_ON))
kbd_led_present = true;
 }
 
-- 
2.13.1

[PATCH v3] platform/x86: dell-laptop: Fix bogus keyboard backlight sysfs interface

[Kai-Heng Feng]

Dell Latitude 3160 does not have keyboard backlight, but there is a
sysfs interface for it, which does nothing at all.

KBD_LED_ON_TOKEN is the only token can be found. Since it doesn't have
KBD_LED_OFF_TOKEN or KBD_LED_AUTO_*_TOKEN, it should be safe to assume
at least both ON and OFF token should be present to support keyboard
backlight.

Models which do not use SMBIOS token to control keyboard backlight, also
have this issue. Brightness level is 0 on these models. Verified on Dell
Inspiron 3565.

Reports keyboard backlight is supported only when at least two modes are
present.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---

v3:
Change the logic, there should be at least two modes present to claim it
supports backlight. Suggested by Pali Rohár.

v2:
The only token can be found is actually KBD_LED_ON_TOKEN, which is BIT(5),
instead of KBD_LED_OFF_TOKEN. Change commit log accordingly.

Use token bit to make the intention more clear.
Also consider kbd_mode_levels_count and kbd_info.levels, suggested by
Pali Rohár.

Use XOR to simplify the bitmask comparison, suggested by
Andy Shevchenko.

 drivers/platform/x86/dell-laptop.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell-laptop.c 
b/drivers/platform/x86/dell-laptop.c
index ec202094bd50..f42159fd2031 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1510,7 +1510,11 @@ static void kbd_init(void)
ret = kbd_init_info();
kbd_init_tokens();
 
-   if (kbd_token_bits != 0 || ret == 0)
+   /*
+* Only supports keyboard backlight when it has at least two modes.
+*/
+   if ((ret == 0 && (kbd_info.levels != 0 || kbd_mode_levels_count >= 2))
+   || kbd_get_valid_token_counts() >= 2)
kbd_led_present = true;
 }
 
-- 
2.13.1

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> Let's get some help from people who understand PCI well.
>
> Here's the general problem: Kai-Heng has a PCI-based USB host
> controller that advertises wakeup capability from D3, but it doesn't
> assert PME# from D3 when it should.  For "lspci -vv" output, see
>
> http://marc.info/?l=linux-usb=149570231732519=2
>
> On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
>
>> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
>> <kai.heng.f...@canonical.com> wrote:
>> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>> >>
>> >> Is this really the right solution?  Maybe it would be better to allow
>> >> the controller to go into D3 provided no wakeup signal is needed.  You
>> >> could do:
>> >>
>> >> device_set_wakeup_capable(>dev, 0);
>> >
>> > This doesn't work.
>> > After applying this function, still nothing happens when devices get 
>> > plugged in.
>> > IIUC this function disable the wakeup function, but what I want to do
>> > here is to have PME signal works even when runtime PM is enabled.
>
> This may indicate a bug in either the PCI or USB stacks (or both!).  If
> a driver requires wakeup capability from runtime suspend but the device
> does not provide it, the PCI core should not allow the device to go
> into runtime suspend.  Or is that the driver's responsibility?
>
>> > I also saw some legacy PCI PM stuff, so I also tried:
>> > device_set_wakeup_capable(>dev, 1);
>> > ...doesn't work either.
>> >
>> >>
>> >> Another alternative is to put the controller into D2 instead of D3, but
>> >> (1) I don't know how to do that, and (2) we don't know if wakeup
>> >> signalling works any better in D2 than it does in D3.
>> >
>> > I'll try if D2 works.
>>
>> Put the device into D2 instead of D3 can make the wakeup signaling
>> work, i.e. USB devices can be correctly detected after plugged into
>> EHCI port.
>>
>> Do you think this alternative an acceptable workaround?
>
> Yes, it is.  The difficulty is that I don't know how to tell the PCI
> core that the device should go in D2 during runtime suspend instead of
> D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
>

FWIW, this is the diff I used to make the controller transits to D2
instead of D3:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..36663688404a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -922,6 +922,9 @@ int pci_set_power_state(struct pci_dev *dev,
pci_power_t state)
if (dev->current_state == state)
return 0;

+   if (state > PCI_D2 && (dev->dev_flags & PCI_DEV_FLAGS_MAX_D2))
+   state = PCI_D2;
+
__pci_start_power_transition(dev, state);

/* This device is quirked not to be put into D3, so
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..a2c1fe62974a 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
if (pdev->device == 0x7808) {
ehci->use_dummy_qh = 1;
ehci_info(ehci, "applying AMD
SB700/SB800/Hudson-2/3 EHCI dummy qh workaround\n");
+
+   pdev->dev_flags |= PCI_DEV_FLAGS_MAX_D2;
}
break;
case PCI_VENDOR_ID_VIA:
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..6f86f2aa8548 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -188,6 +188,7 @@ enum pci_dev_flags {
 * the direct_complete optimization.
 */
PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
+   PCI_DEV_FLAGS_MAX_D2 = (__force pci_dev_flags_t) (1 << 12),
 };

 enum pci_irq_reroute_variant {

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
>> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>>
>> Is this really the right solution?  Maybe it would be better to allow
>> the controller to go into D3 provided no wakeup signal is needed.  You
>> could do:
>>
>> device_set_wakeup_capable(>dev, 0);
>
> This doesn't work.
> After applying this function, still nothing happens when devices get plugged 
> in.
> IIUC this function disable the wakeup function, but what I want to do
> here is to have PME signal works even when runtime PM is enabled.
>
> I also saw some legacy PCI PM stuff, so I also tried:
> device_set_wakeup_capable(>dev, 1);
> ...doesn't work either.
>
>>
>> Another alternative is to put the controller into D2 instead of D3, but
>> (1) I don't know how to do that, and (2) we don't know if wakeup
>> signalling works any better in D2 than it does in D3.
>
> I'll try if D2 works.

Put the device into D2 instead of D3 can make the wakeup signaling
work, i.e. USB devices can be correctly detected after plugged into
EHCI port.

Do you think this alternative an acceptable workaround?

>
> Thanks for the review.
>

Re: [PATCH] firmware: add firmware to new device's devres list for second time cache

[Kai-Heng Feng]

On Tue, Aug 22, 2017 at 12:52 AM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> Currently, firmware will only be chached if assign_firmware_buf() gets
> called.
>
> When a device loses its power or a USB device gets plugged to another
> port under suspend, request_firmware() can still find cached firmware,
> but firmware name no longer associates with the new device's devres.
> So next time the system suspend, those firmware won't be cached.
>
> Hence, we should add the firmware name to the devres when the firmware
> is found in cache, to make the firmware cacheable next time.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
> ---
>  drivers/base/firmware_class.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index bfbe1e154128..a99de34e3fdc 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1177,6 +1177,10 @@ _request_firmware_prepare(struct firmware 
> **firmware_p, const char *name,
>
> ret = fw_lookup_and_allocate_buf(name, _cache, , dbuf, size);
>
> +   /* device might be a new one, add it to devres list */
> +   if (ret == 0 || ret == 1)
> +   fw_add_devm_name(device, name);
> +
> /*
>  * bind with 'buf' now to avoid warning in failure path
>  * of requesting firmware.
> --
> 2.14.1
>

*A gentle ping here*

Re: [PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

On Mon, Aug 28, 2017 at 9:56 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Mon, Aug 28, 2017 at 6:14 PM, Mathias Nyman
> <mathias.ny...@linux.intel.com> wrote:
>> On 28.08.2017 12:29, Greg KH wrote:
>>
>> Adding more people who were involved in the original patch.
>>
>> Users are now seeing the unresponsive USB2 ports with Promontory hosts.
>> Is there any update on a better way to solve the original issue.
>>
>> To me a "dead" USB2 port seems like a much worse issue for a user
>> than a BIOS disabled port waking up on plug/unplug (wake on
>> connect/disconnect),
>> so I'm myself in favor of doing this revert.
>
> At least I can't find "Disable USB2" on my ASUS PRIME B350M-A, so the
> new behavior is quite surprising.
>
>>
>> But there was a strong push from Promontory developers to get the original
>> fix in,
>> and I would like to get some comment from them before I do anything about
>> it.
>
> You looped them to the mail thread which I reported the regression two
> weeks ago, and there is no response since then...

Still no response from AMD and ASMedia guys.

I don't like to use out-of-tree patches for myself, but probably it's
the only way here?

>
>>
>> Thanks
>> -Mathias
>>

[PATCH] ALSA: usb-audio: Add sample rate quirk for Plantronics C310/C520-M

[Kai-Heng Feng]

Like other Plantronics devices, C310 and C520-M do not support sample
rate reading. Add them to the sample rate quirk accordingly.

BugLink: https://bugs.launchpad.net/bugs/1708499
BugLink: https://bugs.launchpad.net/bugs/1709282
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/usb/quirks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 913552078285..b8cb57aeec77 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -1137,6 +1137,8 @@ bool snd_usb_get_sample_rate_quirk(struct snd_usb_audio 
*chip)
case USB_ID(0x047F, 0x02F7): /* Plantronics BT-600 */
case USB_ID(0x047F, 0x0415): /* Plantronics BT-300 */
case USB_ID(0x047F, 0xAA05): /* Plantronics DA45 */
+   case USB_ID(0x047F, 0xC022): /* Plantronics C310 */
+   case USB_ID(0x047F, 0xC036): /* Plantronics C520-M */
case USB_ID(0x04D8, 0xFEEA): /* Benchmark DAC1 Pre */
case USB_ID(0x0556, 0x0014): /* Phoenix Audio TMX320VC */
case USB_ID(0x05A3, 0x9420): /* ELP HD USB Camera */
-- 
2.14.1

[PATCH] ACPI / battery: add quirk for Asus GL502VSK and UX305LA

[Kai-Heng Feng]

On Asus GL502VSK and UX305LA, ACPI incorrectly reports discharging when
battery is full and AC is plugged.

However rate_now is correct under this circumstance, hence we can use
"rate_now == 0" as a predicate to report battery full status correctly.

BugLink: https://bugs.launchpad.net/bugs/1482390
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/acpi/battery.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 13e7b56e33ae..f9f008cf3da7 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -70,6 +70,7 @@ static async_cookie_t async_cookie;
 static bool battery_driver_registered;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
+static int battery_full_discharging;
 static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
@@ -214,7 +215,10 @@ static int acpi_battery_get_property(struct power_supply 
*psy,
return -ENODEV;
switch (psp) {
case POWER_SUPPLY_PROP_STATUS:
-   if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
+   if (battery_full_discharging && battery->rate_now == 0 &&
+   battery->state & ACPI_BATTERY_STATE_DISCHARGING)
+   val->intval = POWER_SUPPLY_STATUS_FULL;
+   else if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
val->intval = POWER_SUPPLY_STATUS_CHARGING;
@@ -1166,6 +1170,13 @@ battery_notification_delay_quirk(const struct 
dmi_system_id *d)
return 0;
 }
 
+static int __init
+battery_full_discharging_quirk(const struct dmi_system_id *d)
+{
+   battery_full_discharging = 1;
+   return 0;
+}
+
 static const struct dmi_system_id bat_dmi_table[] __initconst = {
{
.callback = battery_bix_broken_package_quirk,
@@ -1183,6 +1194,22 @@ static const struct dmi_system_id bat_dmi_table[] 
__initconst = {
DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
},
},
+   {
+   .callback = battery_full_discharging_quirk,
+   .ident = "ASUS GL502VSK",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "GL502VSK"),
+   },
+   },
+   {
+   .callback = battery_full_discharging_quirk,
+   .ident = "ASUS UX305LA",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "UX305LA"),
+   },
+   },
{},
 };
 
-- 
2.14.1

Re: [PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Kai-Heng Feng]

Hi Hans,

On Fri, Oct 6, 2017 at 2:04 AM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi All,
>
> This is a resend with Kai Heng Feng added to the Cc. Kai can you please
> test this patch. This patch replaces yours, as many devices seem to be
> affected so a whitelist is the better approach here.

I'll let the affected user to test the patch.

Thanks!

>
> Regards,
>
> Hans

Re: [PATCH] firmware: add firmware to new device's devres list for second time cache

[Kai-Heng Feng]

On Thu, Oct 5, 2017 at 5:17 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> On Tue, Aug 22, 2017 at 03:52:46PM +0800, Kai-Heng Feng wrote:
>> Currently, firmware will only be chached if assign_firmware_buf() gets
>> called.
>
> True, but also more importantly we peg the fw cache to the device via devres
> *iff* the firmware actually was found. We do this so that we don't try to look
> for bogus firmwares or firmwares which we currently do not have on our
> filesystem (consider a driver that uses a series of revisions of firmwares).
>
>> When a device loses its power or a USB device gets plugged to another
>> port under suspend, request_firmware() can still find cached firmware,
>> but firmware name no longer associates with the new device's devres.
>> So next time the system suspend, those firmware won't be cached.
>
> Gah, its a bit more complicated than that. During suspend we call out to
> request firmware proactively for all firmwares in our fw cache. The
> fw cache is used simply to fetch the caches for firwmares during suspend
> so that on resume they exist to avoid races against the filesystem. It
> however uses the same functionality as the batched firwmare feature, which
> in turn is used to share one buf over different requests.
>
> If a device is unplugged its not clear to me why the old cache would
> not work for the new one as its all shared, so the only thing that I
> can think of is if the old device being disconnected is processed first,
> and therefore releases the old cache, so when the new device is processed
> it does not use the new cache. This should still not be an issue though,
> unless of course real races happen with the filesystem.

Because the new one's devres doesn't have the fw, i.e.
fw_add_devm_name() not gets called in this case.

>
> As of recently though we have had new findings which indicate that the
> old UMH lock was causing issues on resume on BT devices which *only*
> needed firmware on resume, but not on boot, so their first fw cache
> was not generated. That issue can resemble this one, in that no cache
> can be present, and a race happens on resume.
>
> The old UMH lock then was causing a failure on resume, and one of the
> solutions which could have worked was a proactive "hey set this cache
> up for me" even though the device didn't need the firmware. This
> is no longer needed given that the UMH lock stuff is gone from
> direct FS lookups and the issue should no longer be present.
>
> That is, Linus' revert of commit f007cad159e99fa2acd3b2e9364fbb32ad28b971
> ("Revert "firmware: add sanity check on shutdown/suspend") I believe
> should fix this issue.
>
> I'm actually inclined to remove the fw cache stuff as I no longer see
> the advantage of it given we are doing FS lookups and I see no races
> possible anymore.
>
>> Hence, we should add the firmware name to the devres when the firmware
>> is found in cache, to make the firmware cacheable next time.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/base/firmware_class.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index bfbe1e154128..a99de34e3fdc 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -1177,6 +1177,10 @@ _request_firmware_prepare(struct firmware 
>> **firmware_p, const char *name,
>>
>>   ret = fw_lookup_and_allocate_buf(name, _cache, , dbuf, size);
>>
>> + /* device might be a new one, add it to devres list */
>> + if (ret == 0 || ret == 1)
>> + fw_add_devm_name(device, name);
>> +
>
> Even if this was correct notice we have requests for which a FW cache is not
> desired -- see FW_OPT_NOCACHE, and the above does not respect it.

I think this is needed when it's not FW_OPT_NOCACHE.

>
> Given all the above, can you test with a kernel which has
> commit f007cad159e99fa2acd3b2e9364fbb32ad28b971 and tell me if you
> still see the issue?

It's fixed after f007cad159e99fa2acd3b2e9364fbb32ad28b971.
Again, thanks for your detailed explanation.


Kai-Heng,

>
>   Luis

Re: [PATCH] ALSA: usb-audio: Add sample rate quirk for Plantronics P610

[Kai-Heng Feng]

Hi,

On Fri, Oct 6, 2017 at 2:22 AM, Takashi Iwai <ti...@suse.de> wrote:
> On Thu, 05 Oct 2017 20:04:06 +0200,
> Kai-Heng Feng wrote:
>>
>> Like other Plantronics devices, P610 does not support sample
>> rate reading. Apply sample rate quirk to it.
>>
>> BugLink: https://bugs.launchpad.net/bugs/1719853
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>
> Hrm, maybe we should ignore all Plantronics devices?
> Also MS Lifecam and Phoenix devices are such candidates.
>
> So something like below.
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <ti...@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Apply vendor ID matching for sample rate
>  quirk
>
> So far, lots of Plantronics, MS and Phoenix Audio devices need the
> quirk not to read sample rate back, and the list just grows.
> In this patch, instead of adding each device, apply the quirk by
> matching with these vendors.
>
> Signed-off-by: Takashi Iwai <ti...@suse.de>
> ---
>  sound/usb/quirks.c | 23 +--
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index b8cb57aeec77..9aeb05f61f78 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -1128,29 +1128,24 @@ bool snd_usb_get_sample_rate_quirk(struct 
> snd_usb_audio *chip)
> /* devices which do not support reading the sample rate. */
> switch (chip->usb_id) {
> case USB_ID(0x041E, 0x4080): /* Creative Live Cam VF0610 */
> -   case USB_ID(0x045E, 0x075D): /* MS Lifecam Cinema  */
> -   case USB_ID(0x045E, 0x076D): /* MS Lifecam HD-5000 */
> -   case USB_ID(0x045E, 0x076E): /* MS Lifecam HD-5001 */
> -   case USB_ID(0x045E, 0x076F): /* MS Lifecam HD-6000 */
> -   case USB_ID(0x045E, 0x0772): /* MS Lifecam Studio */
> -   case USB_ID(0x045E, 0x0779): /* MS Lifecam HD-3000 */
> -   case USB_ID(0x047F, 0x02F7): /* Plantronics BT-600 */
> -   case USB_ID(0x047F, 0x0415): /* Plantronics BT-300 */
> -   case USB_ID(0x047F, 0xAA05): /* Plantronics DA45 */
> -   case USB_ID(0x047F, 0xC022): /* Plantronics C310 */
> -   case USB_ID(0x047F, 0xC036): /* Plantronics C520-M */
> case USB_ID(0x04D8, 0xFEEA): /* Benchmark DAC1 Pre */
> case USB_ID(0x0556, 0x0014): /* Phoenix Audio TMX320VC */
> case USB_ID(0x05A3, 0x9420): /* ELP HD USB Camera */
> case USB_ID(0x074D, 0x3553): /* Outlaw RR2150 (Micronas UAC3553B) */
> case USB_ID(0x1395, 0x740a): /* Sennheiser DECT */
> case USB_ID(0x1901, 0x0191): /* GE B850V3 CP2114 audio interface */
> -   case USB_ID(0x1de7, 0x0013): /* Phoenix Audio MT202exe */
> -   case USB_ID(0x1de7, 0x0014): /* Phoenix Audio TMX320 */
> -   case USB_ID(0x1de7, 0x0114): /* Phoenix Audio MT202pcs */
> case USB_ID(0x21B4, 0x0081): /* AudioQuest DragonFly */
> return true;
> }
> +
> +   /* devices of these vendors don't support reading rate, either */
> +   switch (USB_ID_VENDOR(chip->usb_id)) {
> +   case 0x045E: /* MS Lifecam */
> +   case 0x047F: /* Plantronics */
> +   case 0x1de7: /* Phoenix Audio */
> +   return true;
> +   }
> +
> return false;
>  }
>

This is definitely a better approach. Thanks!

Acked-by: Kai-Heng Feng <kai.heng.f...@canonical.com>

> --
> 2.14.2
>

Re: btusb "firmware request while host is not available" at resume

[Kai-Heng Feng]

Hi Luis,

On Wed, Oct 4, 2017 at 8:20 AM, Luis R. Rodriguez  wrote:
> So please retest now that the revert happened: commit
> f007cad159e99fa2acd3b2e9364fbb32ad28b971 ("Revert "firmware: add sanity check
> on shutdown/suspend").

I can confirm the issue is gone after the commit.
Also, thanks for your detailed explanation.


Kai-Heng

>
>   Luis

[PATCH] ALSA: usb-audio: Add sample rate quirk for Plantronics P610

[Kai-Heng Feng]

Like other Plantronics devices, P610 does not support sample
rate reading. Apply sample rate quirk to it.

BugLink: https://bugs.launchpad.net/bugs/1719853

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 sound/usb/quirks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index b8cb57aeec77..9ddaae3784f5 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -1138,6 +1138,7 @@ bool snd_usb_get_sample_rate_quirk(struct snd_usb_audio 
*chip)
case USB_ID(0x047F, 0x0415): /* Plantronics BT-300 */
case USB_ID(0x047F, 0xAA05): /* Plantronics DA45 */
case USB_ID(0x047F, 0xC022): /* Plantronics C310 */
+   case USB_ID(0x047F, 0xC02F): /* Plantronics P610 */
case USB_ID(0x047F, 0xC036): /* Plantronics C520-M */
case USB_ID(0x04D8, 0xFEEA): /* Benchmark DAC1 Pre */
case USB_ID(0x0556, 0x0014): /* Phoenix Audio TMX320VC */
-- 
2.14.1

[PATCH] platform/x86: peaq-wmi: Blacklist Lenovo ideapad 700-15ISK

[Kai-Heng Feng]

peaq-wmi on Lenovo ideapad 700-15ISK keeps sending KEY_SOUND,
which makes user's repeated keys gets interrupted.

The system does not have Dolby button, let's blacklist it.

BugLink: https://bugs.launchpad.net/bugs/1720219
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/platform/x86/peaq-wmi.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
index bc98ef95514a..5673d5daebc3 100644
--- a/drivers/platform/x86/peaq-wmi.c
+++ b/drivers/platform/x86/peaq-wmi.c
@@ -11,6 +11,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define PEAQ_DOLBY_BUTTON_GUID "ABBC0F6F-8EA1-11D1-00A0-C9062910"
 #define PEAQ_DOLBY_BUTTON_METHOD_ID5
@@ -64,9 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
}
 }
 
+static const struct dmi_system_id peaq_blacklist[] __initconst = {
+   {
+   /* Lenovo ideapad 700-15ISK does not have Dolby button */
+   .ident = "Lenovo ideapad 700-15ISK",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+   DMI_MATCH(DMI_PRODUCT_NAME, "80RU"),
+   },
+   },
+   {}
+};
+
 static int __init peaq_wmi_init(void)
 {
-   if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
+   if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID) ||
+   dmi_check_system(peaq_blacklist))
return -ENODEV;
 
peaq_poll_dev = input_allocate_polled_device();
@@ -86,7 +100,8 @@ static int __init peaq_wmi_init(void)
 
 static void __exit peaq_wmi_exit(void)
 {
-   if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
+   if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID) ||
+   dmi_check_system(peaq_blacklist))
return;
 
input_unregister_polled_device(peaq_poll_dev);
-- 
2.14.1

[PATCH 1/1] Revert "HID: multitouch: Support ALPS PTP stick with pid 0x120A"

[Kai-Heng Feng]

This reverts commit fcaa4a07d2a4b541e91da7a55d8b3331f96d1865.

As noted by Masaki [1], 0x120A + trackpoint will not be used in mass
production machines, so remove the ID accordingly.

[1] http://www.spinics.net/lists/linux-input/msg53222.html

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/hid/hid-ids.h| 1 -
 drivers/hid/hid-multitouch.c | 4 
 2 files changed, 5 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 832897d4a849..a98919199858 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -75,7 +75,6 @@
 
 #define USB_VENDOR_ID_ALPS_JP  0x044E
 #define HID_DEVICE_ID_ALPS_U1_DUAL 0x120B
-#define HID_DEVICE_ID_ALPS_U1_PTP_20x120A
 #define HID_DEVICE_ID_ALPS_U1_DUAL_PTP 0x121F
 #define HID_DEVICE_ID_ALPS_U1_DUAL_3BTN_PTP0x1220
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c78625dceced..9e8c4d2ba11d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1419,10 +1419,6 @@ static const struct hid_device_id mt_devices[] = {
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
USB_VENDOR_ID_ALPS_JP,
HID_DEVICE_ID_ALPS_U1_DUAL_3BTN_PTP) },
-   { .driver_data = MT_CLS_WIN_8_DUAL,
-   HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
-   USB_VENDOR_ID_ALPS_JP,
-   HID_DEVICE_ID_ALPS_U1_PTP_2) },
 
/* Lenovo X1 TAB Gen 2 */
{ .driver_data = MT_CLS_WIN_8_DUAL,
-- 
2.14.2

Re: btusb "firmware request while host is not available" at resume

[Kai-Heng Feng]

Hi Luis,

On Thu, Sep 14, 2017 at 1:39 AM, Luis R. Rodriguez  wrote:
[snipped]
> Would a fw_cache_hint(device, name_list) be reasonable then sometime *before*
> suspend? All this would do is ask the firmware API to extend the fw cache
> list with the entries. It would not load firmware immediately, instead this
> would trigger a request for the hinted firmware to become available for
> suspend. Then these drivers could request for firmware at resume and it
> will pick up the cached firmware.

I think I am the author the patch [1] mentioned by Marcel. I have to
admit, it's quite
clunky in it's current form.
So yes, the new API you proposed is definitely better to solve the
issue. I'll send
new patch for btusb once we have the new API to use.

Also, with patch [1], the "firmware request while host is not
available" may still get hit
on some corner cases. I proposed another patch [2] to tackle the edge
case, can you
take a look?

[1] https://lkml.org/lkml/2017/8/25/58
[2] https://lkml.org/lkml/2017/8/22/123

Kai-Heng

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

[PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

This reverts commit dec08194ffeccfa1cf085906b53d301930eae18f.

Commit dec08194ffec ("xhci: Limit USB2 port wake support for AMD Promontory
hosts") makes all high speed USB ports on ASUS PRIME B350M-A cease to
function after enabling runtime PM.

All boards with this chipsets will be affected, so revert the commit.

Conflicts:
drivers/usb/host/xhci-pci.c
drivers/usb/host/xhci.h

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-hub.c |  3 ---
 drivers/usb/host/xhci-pci.c | 12 
 drivers/usb/host/xhci.h |  2 +-
 3 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8807ab..4bd8654b1233 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1473,9 +1473,6 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
t2 |= PORT_WKOC_E | PORT_WKCONN_E;
t2 &= ~PORT_WKDISC_E;
}
-   if ((xhci->quirks & XHCI_U2_DISABLE_WAKE) &&
-   (hcd->speed < HCD_USB3))
-   t2 &= ~PORT_WAKE_BITS;
} else
t2 &= ~PORT_WAKE_BITS;
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8071c8fdd15e..76f392954733 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -54,11 +54,6 @@
 #define PCI_DEVICE_ID_INTEL_APL_XHCI   0x5aa8
 #define PCI_DEVICE_ID_INTEL_DNV_XHCI   0x19d0
 
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_40x43b9
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_30x43ba
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_20x43bb
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_10x43bc
-
 #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI   0x1142
 
 static const char hcd_name[] = "xhci_hcd";
@@ -142,13 +137,6 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 
-   if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
-   ((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_2) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
-   xhci->quirks |= XHCI_U2_DISABLE_WAKE;
-
if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
xhci->quirks |= XHCI_LPM_SUPPORT;
xhci->quirks |= XHCI_INTEL_HOST;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e935291ed6..d6b471823a0b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1819,7 +1819,7 @@ struct xhci_hcd {
 /* For controller with a broken Port Disable implementation */
 #define XHCI_BROKEN_PORT_PED   (1 << 25)
 #define XHCI_LIMIT_ENDPOINT_INTERVAL_7 (1 << 26)
-#define XHCI_U2_DISABLE_WAKE   (1 << 27)
+/* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL(1 << 28)
 
unsigned intnum_active_eps;
-- 
2.14.1

[PATCH] firmware: add firmware to new device's devres list for second time cache

[Kai-Heng Feng]

Currently, firmware will only be chached if assign_firmware_buf() gets
called.

When a device loses its power or a USB device gets plugged to another
port under suspend, request_firmware() can still find cached firmware,
but firmware name no longer associates with the new device's devres.
So next time the system suspend, those firmware won't be cached.

Hence, we should add the firmware name to the devres when the firmware
is found in cache, to make the firmware cacheable next time.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/base/firmware_class.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index bfbe1e154128..a99de34e3fdc 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1177,6 +1177,10 @@ _request_firmware_prepare(struct firmware **firmware_p, 
const char *name,
 
ret = fw_lookup_and_allocate_buf(name, _cache, , dbuf, size);
 
+   /* device might be a new one, add it to devres list */
+   if (ret == 0 || ret == 1)
+   fw_add_devm_name(device, name);
+
/*
 * bind with 'buf' now to avoid warning in failure path
 * of requesting firmware.
-- 
2.14.1

[PATCH v2 1/4] Bluetooth: btusb: Add firmware caching function for already patched Bluetooth chip

[Kai-Heng Feng]

When a system reboot, the USB power never gets cut off, so the firmware
is already updated on the Bluetooth chip.

Several btusb setup functions check firmware updated status before
download firmware, the loading part will be skipped if it's updated.
Because the firmware is never asked by request_firmware(),
firmware_class does not know it needs to be cached before system enters
sleep.

Now, system suspend/resume may cause the driver failed to request the
firmware because it's not in the firmware cache:

[   87.539434] firmware request while host is not available

This can be solved by calling request_firmware() even if the chip is
already updated - now the firmware_class knows what to cache.

In this case, we don't really need to wait for the firmware content, so
we use the async version of request_firmware().

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Split patches for different vendors.

 drivers/bluetooth/btusb.c | 48 +++
 1 file changed, 48 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a5c06aaa181..2313d20c6d60 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1476,6 +1476,54 @@ static void btusb_waker(struct work_struct *work)
usb_autopm_put_interface(data->intf);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void btusb_request_firmware_done(const struct firmware *firmware,
+   void *context)
+{
+   const char *name = (const char *)context;
+
+   if (!firmware) {
+   BT_WARN("firmware %s will not be cached", name);
+   goto done;
+   }
+
+   BT_DBG("firmware %s will be cached", name);
+
+   release_firmware(firmware);
+done:
+   kfree_const(name);
+}
+
+static int btusb_request_firmware_async(struct hci_dev *hdev,
+   const char *fwname)
+{
+   const char *name;
+   int err;
+
+   name = kstrdup_const(fwname, GFP_KERNEL);
+   if (!name)
+   return -ENOMEM;
+
+   err = request_firmware_nowait(THIS_MODULE, true, name, >dev,
+ GFP_KERNEL, (void *)name,
+ btusb_request_firmware_done);
+   if (err) {
+   BT_WARN("%s: failed to async request firmware for file: %s 
(%d)",
+   hdev->name, name, err);
+   kfree_const(name);
+   return err;
+   }
+
+   return 0;
+}
+#else
+static int btusb_request_firmware_async(struct hci_dev *hdev,
+   const char *fwname)
+{
+   return 0;
+}
+#endif
+
 static int btusb_setup_bcm92035(struct hci_dev *hdev)
 {
struct sk_buff *skb;
-- 
2.14.1

Re: [PATCH] usb: Add device quirk for Logitech HD Pro Webcam C920-C

[Kai-Heng Feng]

On Mon, Aug 21, 2017 at 6:03 PM, Dmitry Fleytman  wrote:
> Commit e0429362ab15
> ("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
> introduced quirk to workaround an issue with some Logitech webcams.
>
> Apparently model C920-C has the same issue so applying
> the same quirk as well.

I think it's better to also mention "C920-C" in the comment section.

>
> See aforementioned commit message for detailed explanation of the problem.
>
> Signed-off-by: Dmitry Fleytman 
> ---
>  drivers/usb/core/quirks.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 574da2b..36d4841 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -59,6 +59,7 @@ static const struct usb_device_id usb_quirk_list[] = {
>
> /* Logitech HD Pro Webcams C920 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 },
>
> /* Logitech ConferenceCam CC3000e */
> --
> 2.7.4
>

[PATCH 2/2] Bluetooth: btusb: ath3k: Cache firmware for already patched Bluetooth chip

[Kai-Heng Feng]

When a system reboot, the USB power never gets cut off, so the firmware
is already updated on the Bluetooth chip.

Several btusb setup functions check firmware updated status before
download firmware, the loading part will be skipped if it's updated.
Because the firmware is never asked by request_firmware(),
firmware_class does not know it needs to be cached before system enters
sleep.

Now, system suspend/resume may cause the driver failed to request the
firmware because it's not in the firmware cache:

[   87.539434] firmware request while host is not available

This can be solved by calling request_firmware() even if the chip is
already updated - now the firmware_class knows what to cache.

In this case, we don't really need to wait for the firmware content, so
we use the async version of request_firmware().

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/bluetooth/ath3k.c | 49 +
 drivers/bluetooth/btusb.c | 56 +--
 2 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index 280849dba51e..27a7415f9fd7 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -208,6 +208,54 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
 #define TIMEGAP_USEC_MIN   50
 #define TIMEGAP_USEC_MAX   100
 
+#ifdef CONFIG_PM_SLEEP
+static void ath3k_request_firmware_done(const struct firmware *firmware,
+   void *context)
+{
+   const char *name = (const char *)context;
+
+   if (!firmware) {
+   BT_WARN("firmware %s will not be cached", name);
+   goto done;
+   }
+
+   BT_DBG("firmware %s will be cached", name);
+
+   release_firmware(firmware);
+done:
+   kfree_const(name);
+}
+
+static int ath3k_request_firmware_async(struct usb_device *udev,
+   const char *fwname)
+{
+   const char *name;
+   int err;
+
+   name = kstrdup_const(fwname, GFP_KERNEL);
+   if (!name)
+   return -ENOMEM;
+
+   err = request_firmware_nowait(THIS_MODULE, true, name, >dev,
+ GFP_KERNEL, (void *)name,
+ ath3k_request_firmware_done);
+   if (err) {
+   BT_WARN("%s %s: failed to async request firmware for file: %s 
(%d)",
+   udev->manufacturer, udev->product, name, err);
+   kfree_const(name);
+   return err;
+   }
+
+   return 0;
+}
+#else
+static int ath3k_request_firmware_async(struct usb_device *udev,
+   const char *fwname)
+{
+   return 0;
+}
+#endif
+
 static int ath3k_load_firmware(struct usb_device *udev,
const struct firmware *firmware)
 {
@@ -420,6 +468,7 @@ static int ath3k_load_patch(struct usb_device *udev)
 
if (fw_state & ATH3K_PATCH_UPDATE) {
BT_DBG("Patch was already downloaded");
+   ath3k_request_firmware_async(udev, filename);
return 0;
}
 
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 732fe6c3e789..7de2156debd8 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1459,6 +1459,54 @@ static void btusb_waker(struct work_struct *work)
usb_autopm_put_interface(data->intf);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static void btusb_request_firmware_done(const struct firmware *firmware,
+   void *context)
+{
+   const char *name = (const char *)context;
+
+   if (!firmware) {
+   BT_WARN("firmware %s will not be cached", name);
+   goto done;
+   }
+
+   BT_DBG("firmware %s will be cached", name);
+
+   release_firmware(firmware);
+done:
+   kfree_const(name);
+}
+
+static int btusb_request_firmware_async(struct hci_dev *hdev,
+   const char *fwname)
+{
+   const char *name;
+   int err;
+
+   name = kstrdup_const(fwname, GFP_KERNEL);
+   if (!name)
+   return -ENOMEM;
+
+   err = request_firmware_nowait(THIS_MODULE, true, name, >dev,
+ GFP_KERNEL, (void *)name,
+ btusb_request_firmware_done);
+   if (err) {
+   BT_WARN("%s: failed to async request firmware for file: %s 
(%d)",
+   hdev->name, name, err);
+   kfree_const(name);
+   return err;
+   }
+
+   return 0;
+}
+#else
+static int btusb_request_firmware_async(struct hci_dev *hdev,
+   const char *fwname)
+{
+   return 0;
+}
+#endif
+
 static int btusb_setup_bcm92035(struct hci_dev *hdev)

[PATCH 1/2] Bluetooth: btusb: ath3k: Decide firmware name before checking update status

[Kai-Heng Feng]

Decide firmware name before checking patch update status. Firmware name is
required for caching.

Also, version information in btusb_setup_qca() is being calculated twice,
reduce it to one.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/bluetooth/ath3k.c | 10 +++
 drivers/bluetooth/btusb.c | 72 +++
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index b793853ff05f..280849dba51e 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -409,11 +409,6 @@ static int ath3k_load_patch(struct usb_device *udev)
return ret;
}
 
-   if (fw_state & ATH3K_PATCH_UPDATE) {
-   BT_DBG("Patch was already downloaded");
-   return 0;
-   }
-
ret = ath3k_get_version(udev, _version);
if (ret < 0) {
BT_ERR("Can't get version to change to load ram patch err");
@@ -423,6 +418,11 @@ static int ath3k_load_patch(struct usb_device *udev)
snprintf(filename, ATH3K_NAME_LEN, "ar3k/AthrBT_0x%08x.dfu",
 le32_to_cpu(fw_version.rom_version));
 
+   if (fw_state & ATH3K_PATCH_UPDATE) {
+   BT_DBG("Patch was already downloaded");
+   return 0;
+   }
+
ret = request_firmware(, filename, >dev);
if (ret < 0) {
BT_ERR("Patch file not found %s", filename);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index fa24d693af24..732fe6c3e789 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1518,18 +1518,12 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 }
 
 static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
-  struct intel_version 
*ver)
+  const char *fwname,
+  const char 
*default_fwname)
 {
const struct firmware *fw;
-   char fwname[64];
int ret;
 
-   snprintf(fwname, sizeof(fwname),
-"intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq",
-ver->hw_platform, ver->hw_variant, ver->hw_revision,
-ver->fw_variant,  ver->fw_revision, ver->fw_build_num,
-ver->fw_build_ww, ver->fw_build_yy);
-
ret = request_firmware(, fwname, >dev);
if (ret < 0) {
if (ret == -EINVAL) {
@@ -1544,11 +1538,9 @@ static const struct firmware 
*btusb_setup_intel_get_fw(struct hci_dev *hdev,
/* If the correct firmware patch file is not found, use the
 * default firmware patch file instead
 */
-   snprintf(fwname, sizeof(fwname), "intel/ibt-hw-%x.%x.bseq",
-ver->hw_platform, ver->hw_variant);
-   if (request_firmware(, fwname, >dev) < 0) {
+   if (request_firmware(, default_fwname, >dev) < 0) {
BT_ERR("%s failed to open default Intel fw file: %s",
-  hdev->name, fwname);
+  hdev->name, default_fwname);
return NULL;
}
}
@@ -1676,6 +1668,8 @@ static int btusb_setup_intel_patching(struct hci_dev 
*hdev,
 static int btusb_setup_intel(struct hci_dev *hdev)
 {
struct sk_buff *skb;
+   char fwname[64];
+   char default_fwname[64];
const struct firmware *fw;
const u8 *fw_ptr;
int disable_patch, err;
@@ -1714,6 +1708,15 @@ static int btusb_setup_intel(struct hci_dev *hdev)
ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
ver.fw_build_ww, ver.fw_build_yy, ver.fw_patch_num);
 
+   snprintf(fwname, sizeof(fwname),
+"intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq",
+ver.hw_platform, ver.hw_variant, ver.hw_revision,
+ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
+ver.fw_build_ww, ver.fw_build_yy);
+
+   snprintf(default_fwname, sizeof(default_fwname),
+   "intel/ibt-hw-%x.%x.bseq", ver.hw_platform, ver.hw_variant);
+
/* fw_patch_num indicates the version of patch the device currently
 * have. If there is no patch data in the device, it is always 0x00.
 * So, if it is other than 0x00, no need to patch the device again.
@@ -1730,7 +1733,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
 * If no patch file is found, allow the device to operate without
 * a patch.
 */
-   fw = btusb_setup_intel_get_fw(hdev, );
+   fw = btusb_setup_intel_get_fw(hdev, fwname, default_fwname);

Re: [PATCH 2/2] Bluetooth: btusb: ath3k: Cache firmware for already patched Bluetooth chip

[Kai-Heng Feng]

On Thu, Aug 24, 2017 at 5:15 PM, Marcel Holtmann <mar...@holtmann.org> wrote:
> Hi Kai-Heng,
>
>> When a system reboot, the USB power never gets cut off, so the firmware
>> is already updated on the Bluetooth chip.
>>
>> Several btusb setup functions check firmware updated status before
>> download firmware, the loading part will be skipped if it's updated.
>> Because the firmware is never asked by request_firmware(),
>> firmware_class does not know it needs to be cached before system enters
>> sleep.
>>
>> Now, system suspend/resume may cause the driver failed to request the
>> firmware because it's not in the firmware cache:
>>
>> [   87.539434] firmware request while host is not available
>>
>> This can be solved by calling request_firmware() even if the chip is
>> already updated - now the firmware_class knows what to cache.
>>
>> In this case, we don't really need to wait for the firmware content, so
>> we use the async version of request_firmware().
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>> drivers/bluetooth/ath3k.c | 49 +
>> drivers/bluetooth/btusb.c | 56 
>> +--
>> 2 files changed, 103 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
>> index 280849dba51e..27a7415f9fd7 100644
>> --- a/drivers/bluetooth/ath3k.c
>> +++ b/drivers/bluetooth/ath3k.c
>> @@ -208,6 +208,54 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
>> #define TIMEGAP_USEC_MIN  50
>> #define TIMEGAP_USEC_MAX  100
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static void ath3k_request_firmware_done(const struct firmware *firmware,
>> + void *context)
>> +{
>> + const char *name = (const char *)context;
>> +
>> + if (!firmware) {
>> + BT_WARN("firmware %s will not be cached", name);
>> + goto done;
>> + }
>> +
>> + BT_DBG("firmware %s will be cached", name);
>> +
>> + release_firmware(firmware);
>> +done:
>> + kfree_const(name);
>> +}
>> +
>> +static int ath3k_request_firmware_async(struct usb_device *udev,
>> + const char *fwname)
>> +{
>> + const char *name;
>> + int err;
>> +
>> + name = kstrdup_const(fwname, GFP_KERNEL);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + err = request_firmware_nowait(THIS_MODULE, true, name, >dev,
>> +   GFP_KERNEL, (void *)name,
>> +   ath3k_request_firmware_done);
>> + if (err) {
>> + BT_WARN("%s %s: failed to async request firmware for file: %s 
>> (%d)",
>> + udev->manufacturer, udev->product, name, err);
>> + kfree_const(name);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +static int ath3k_request_firmware_async(struct usb_device *udev,
>> + const char *fwname)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> static int ath3k_load_firmware(struct usb_device *udev,
>>   const struct firmware *firmware)
>> {
>> @@ -420,6 +468,7 @@ static int ath3k_load_patch(struct usb_device *udev)
>>
>>   if (fw_state & ATH3K_PATCH_UPDATE) {
>>   BT_DBG("Patch was already downloaded");
>> + ath3k_request_firmware_async(udev, filename);
>>   return 0;
>>   }
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 732fe6c3e789..7de2156debd8 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -1459,6 +1459,54 @@ static void btusb_waker(struct work_struct *work)
>>   usb_autopm_put_interface(data->intf);
>> }
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static void btusb_request_firmware_done(const struct firmware *firmware,
>> + void *context)
>> +{
>> + const char *name = (const char *)context;
>> +
>> + if (!firmware) {
>> + BT_WARN("firmware %s will not be cached", name);
>> + goto done;
>> + }
>> +
>> + BT_DBG("firmware %s will be cached",

[PATCH v2 4/4] Bluetooth: ath3k: Cache firmware for Atheros Bluetooth

[Kai-Heng Feng]

For Intel Bluetooth that downloads firmware based on patched status,
it should still call request_firmware() once if download is not needed.

Verified on DW1707 wireless module (0cf3:e005).

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Split patches for different vendors.

 drivers/bluetooth/ath3k.c | 59 +++
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index 204afe66de92..f7c7077d312b 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -209,6 +209,54 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
 #define TIMEGAP_USEC_MIN   50
 #define TIMEGAP_USEC_MAX   100
 
+#ifdef CONFIG_PM_SLEEP
+static void ath3k_request_firmware_done(const struct firmware *firmware,
+   void *context)
+{
+   const char *name = (const char *)context;
+
+   if (!firmware) {
+   BT_WARN("firmware %s will not be cached", name);
+   goto done;
+   }
+
+   BT_DBG("firmware %s will be cached", name);
+
+   release_firmware(firmware);
+done:
+   kfree_const(name);
+}
+
+static int ath3k_request_firmware_async(struct usb_device *udev,
+   const char *fwname)
+{
+   const char *name;
+   int err;
+
+   name = kstrdup_const(fwname, GFP_KERNEL);
+   if (!name)
+   return -ENOMEM;
+
+   err = request_firmware_nowait(THIS_MODULE, true, name, >dev,
+ GFP_KERNEL, (void *)name,
+ ath3k_request_firmware_done);
+   if (err) {
+   BT_WARN("%s %s: failed to async request firmware for file: %s 
(%d)",
+   udev->manufacturer, udev->product, name, err);
+   kfree_const(name);
+   return err;
+   }
+
+   return 0;
+}
+#else
+static int ath3k_request_firmware_async(struct usb_device *udev,
+   const char *fwname)
+{
+   return 0;
+}
+#endif
+
 static int ath3k_load_firmware(struct usb_device *udev,
const struct firmware *firmware)
 {
@@ -410,11 +458,6 @@ static int ath3k_load_patch(struct usb_device *udev)
return ret;
}
 
-   if (fw_state & ATH3K_PATCH_UPDATE) {
-   BT_DBG("Patch was already downloaded");
-   return 0;
-   }
-
ret = ath3k_get_version(udev, _version);
if (ret < 0) {
BT_ERR("Can't get version to change to load ram patch err");
@@ -424,6 +467,12 @@ static int ath3k_load_patch(struct usb_device *udev)
snprintf(filename, ATH3K_NAME_LEN, "ar3k/AthrBT_0x%08x.dfu",
 le32_to_cpu(fw_version.rom_version));
 
+   if (fw_state & ATH3K_PATCH_UPDATE) {
+   BT_DBG("Patch was already downloaded");
+   ath3k_request_firmware_async(udev, filename);
+   return 0;
+   }
+
ret = request_firmware(, filename, >dev);
if (ret < 0) {
BT_ERR("Patch file not found %s", filename);
-- 
2.14.1

[PATCH v2 2/4] Bluetooth: btusb: Cache firmware for Intel Bluetooth

[Kai-Heng Feng]

For Intel Bluetooth that downloads firmware based on patched status, it
should still call request_firmware() once if download is not needed.

Verified on Intel 7265 wireless module (8087:0a2a).

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Split patches for different vendors.

 drivers/bluetooth/btusb.c | 31 ++-
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2313d20c6d60..4cb206ecfa7d 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1583,18 +1583,12 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 }
 
 static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev *hdev,
-  struct intel_version 
*ver)
+  const char *fwname,
+  const char 
*default_fwname)
 {
const struct firmware *fw;
-   char fwname[64];
int ret;
 
-   snprintf(fwname, sizeof(fwname),
-"intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq",
-ver->hw_platform, ver->hw_variant, ver->hw_revision,
-ver->fw_variant,  ver->fw_revision, ver->fw_build_num,
-ver->fw_build_ww, ver->fw_build_yy);
-
ret = request_firmware(, fwname, >dev);
if (ret < 0) {
if (ret == -EINVAL) {
@@ -1609,11 +1603,9 @@ static const struct firmware 
*btusb_setup_intel_get_fw(struct hci_dev *hdev,
/* If the correct firmware patch file is not found, use the
 * default firmware patch file instead
 */
-   snprintf(fwname, sizeof(fwname), "intel/ibt-hw-%x.%x.bseq",
-ver->hw_platform, ver->hw_variant);
-   if (request_firmware(, fwname, >dev) < 0) {
+   if (request_firmware(, default_fwname, >dev) < 0) {
BT_ERR("%s failed to open default Intel fw file: %s",
-  hdev->name, fwname);
+  hdev->name, default_fwname);
return NULL;
}
}
@@ -1741,6 +1733,8 @@ static int btusb_setup_intel_patching(struct hci_dev 
*hdev,
 static int btusb_setup_intel(struct hci_dev *hdev)
 {
struct sk_buff *skb;
+   char fwname[64];
+   char default_fwname[64];
const struct firmware *fw;
const u8 *fw_ptr;
int disable_patch, err;
@@ -1779,6 +1773,15 @@ static int btusb_setup_intel(struct hci_dev *hdev)
ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
ver.fw_build_ww, ver.fw_build_yy, ver.fw_patch_num);
 
+   snprintf(fwname, sizeof(fwname),
+"intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq",
+ver.hw_platform, ver.hw_variant, ver.hw_revision,
+ver.fw_variant,  ver.fw_revision, ver.fw_build_num,
+ver.fw_build_ww, ver.fw_build_yy);
+
+   snprintf(default_fwname, sizeof(default_fwname),
+   "intel/ibt-hw-%x.%x.bseq", ver.hw_platform, ver.hw_variant);
+
/* fw_patch_num indicates the version of patch the device currently
 * have. If there is no patch data in the device, it is always 0x00.
 * So, if it is other than 0x00, no need to patch the device again.
@@ -1786,6 +1789,8 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (ver.fw_patch_num) {
BT_INFO("%s: Intel device is already patched. patch num: %02x",
hdev->name, ver.fw_patch_num);
+   btusb_request_firmware_async(hdev, fwname);
+   btusb_request_firmware_async(hdev, default_fwname);
goto complete;
}
 
@@ -1795,7 +1800,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
 * If no patch file is found, allow the device to operate without
 * a patch.
 */
-   fw = btusb_setup_intel_get_fw(hdev, );
+   fw = btusb_setup_intel_get_fw(hdev, fwname, default_fwname);
if (!fw)
goto complete;
fw_ptr = fw->data;
-- 
2.14.1

[PATCH v2 3/4] Bluetooth: btusb: Cache firmware for QCA Bluetooth

[Kai-Heng Feng]

For QCA Bluetooth that downloads firmware based on patched status, it
should still call request_firmware() once if download is not needed.

Also, version information was converted twice, reduce it to one.

Verified on DW1810 wireless module (0cf3:e009).

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Split patches for different vendors.

 drivers/bluetooth/btusb.c | 49 +++
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 4cb206ecfa7d..ec246302d7e6 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2662,21 +2662,15 @@ static int btusb_setup_qca_download_fw(struct hci_dev 
*hdev,
 }
 
 static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
-struct qca_version *ver,
+const char *fwname,
+const struct qca_version *ver,
 const struct qca_device_info *info)
 {
struct qca_rampatch_version *rver;
const struct firmware *fw;
-   u32 ver_rom, ver_patch;
u16 rver_rom, rver_patch;
-   char fwname[64];
int err;
 
-   ver_rom = le32_to_cpu(ver->rom_version);
-   ver_patch = le32_to_cpu(ver->patch_version);
-
-   snprintf(fwname, sizeof(fwname), "qca/rampatch_usb_%08x.bin", ver_rom);
-
err = request_firmware(, fwname, >dev);
if (err) {
BT_ERR("%s: failed to request rampatch file: %s (%d)",
@@ -2690,11 +2684,11 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev 
*hdev,
rver_rom = le16_to_cpu(rver->rom_version);
rver_patch = le16_to_cpu(rver->patch_version);
 
-   BT_INFO("%s: QCA: patch rome 0x%x build 0x%x, firmware rome 0x%x "
-   "build 0x%x", hdev->name, rver_rom, rver_patch, ver_rom,
-   ver_patch);
+   BT_INFO("%s: QCA: patch rome 0x%x build 0x%x, firmware rome 0x%x build 
0x%x",
+   hdev->name, rver_rom, rver_patch,
+   ver->rom_version, ver->patch_version);
 
-   if (rver_rom != ver_rom || rver_patch <= ver_patch) {
+   if (rver_rom != ver->rom_version || rver_patch <= ver->patch_version) {
BT_ERR("%s: rampatch file version did not match with firmware",
   hdev->name);
err = -EINVAL;
@@ -2710,16 +2704,12 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev 
*hdev,
 }
 
 static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
-   struct qca_version *ver,
+   const char *fwname,
const struct qca_device_info *info)
 {
const struct firmware *fw;
-   char fwname[64];
int err;
 
-   snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
-le32_to_cpu(ver->rom_version));
-
err = request_firmware(, fwname, >dev);
if (err) {
BT_ERR("%s: failed to request NVM file: %s (%d)",
@@ -2740,7 +2730,7 @@ static int btusb_setup_qca(struct hci_dev *hdev)
 {
const struct qca_device_info *info = NULL;
struct qca_version ver;
-   u32 ver_rom;
+   char fwname[64];
u8 status;
int i, err;
 
@@ -2749,14 +2739,15 @@ static int btusb_setup_qca(struct hci_dev *hdev)
if (err < 0)
return err;
 
-   ver_rom = le32_to_cpu(ver.rom_version);
+   ver.rom_version = le32_to_cpu(ver.rom_version);
+   ver.patch_version = le32_to_cpu(ver.patch_version);
for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
-   if (ver_rom == qca_devices_table[i].rom_version)
+   if (ver.rom_version == qca_devices_table[i].rom_version)
info = _devices_table[i];
}
if (!info) {
BT_ERR("%s: don't support firmware rome 0x%x", hdev->name,
-  ver_rom);
+  ver.rom_version);
return -ENODEV;
}
 
@@ -2765,17 +2756,25 @@ static int btusb_setup_qca(struct hci_dev *hdev)
if (err < 0)
return err;
 
+   snprintf(fwname, sizeof(fwname), "qca/rampatch_usb_%08x.bin",
+ver.rom_version);
+
if (!(status & QCA_PATCH_UPDATED)) {
-   err = btusb_setup_qca_load_rampatch(hdev, , info);
+   err = btusb_setup_qca_load_rampatch(hdev, fwname, , info);
if (err < 0)
return err;
-   }
+   } else
+   btusb_request_firmware_async(hdev, fwname);
+
+   snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
+ver.rom_ve

Re: [PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

On Mon, Aug 28, 2017 at 6:14 PM, Mathias Nyman
<mathias.ny...@linux.intel.com> wrote:
> On 28.08.2017 12:29, Greg KH wrote:
>>
>> On Tue, Aug 22, 2017 at 05:14:47PM +0800, Kai-Heng Feng wrote:
>>>
>>> This reverts commit dec08194ffeccfa1cf085906b53d301930eae18f.
>>>
>>> Commit dec08194ffec ("xhci: Limit USB2 port wake support for AMD
>>> Promontory
>>> hosts") makes all high speed USB ports on ASUS PRIME B350M-A cease to
>>> function after enabling runtime PM.
>>>
>>> All boards with this chipsets will be affected, so revert the commit.
>>>
>>> Conflicts:
>>> drivers/usb/host/xhci-pci.c
>>> drivers/usb/host/xhci.h
>>
>>
>> Why are these "Conflicts:" lines here, you did fix up the issues, so
>> there shouldn't be any more conflicts.
>>
>> And if you revert this, don't we still have the original problem here?
>>
>
> Adding more people who were involved in the original patch.
>
> Users are now seeing the unresponsive USB2 ports with Promontory hosts.
> Is there any update on a better way to solve the original issue.
>
> To me a "dead" USB2 port seems like a much worse issue for a user
> than a BIOS disabled port waking up on plug/unplug (wake on
> connect/disconnect),
> so I'm myself in favor of doing this revert.

At least I can't find "Disable USB2" on my ASUS PRIME B350M-A, so the
new behavior is quite surprising.

>
> But there was a strong push from Promontory developers to get the original
> fix in,
> and I would like to get some comment from them before I do anything about
> it.

You looped them to the mail thread which I reported the regression two
weeks ago, and there is no response since then...

>
> Thanks
> -Mathias
>

Re: [PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Kai-Heng Feng]

Hi,

On Fri, Oct 6, 2017 at 12:23 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> Hi Hans,
>
> On Fri, Oct 6, 2017 at 2:04 AM, Hans de Goede <hdego...@redhat.com> wrote:
>> Hi All,
>>
>> This is a resend with Kai Heng Feng added to the Cc. Kai can you please
>> test this patch. This patch replaces yours, as many devices seem to be
>> affected so a whitelist is the better approach here.
>
> I'll let the affected user to test the patch.

The user reports the new patch works.

Thanks.

>
> Thanks!
>
>>
>> Regards,
>>
>> Hans

Re: [PATCH] ACPI / battery: add quirk for Asus GL502VSK and UX305LA

[Kai-Heng Feng]

On Fri, Sep 22, 2017 at 4:27 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Asus GL502VSK and UX305LA, ACPI incorrectly reports discharging when
> battery is full and AC is plugged.
>
> However rate_now is correct under this circumstance, hence we can use
> "rate_now == 0" as a predicate to report battery full status correctly.
>
> BugLink: https://bugs.launchpad.net/bugs/1482390
> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
> ---
>  drivers/acpi/battery.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 13e7b56e33ae..f9f008cf3da7 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -70,6 +70,7 @@ static async_cookie_t async_cookie;
>  static bool battery_driver_registered;
>  static int battery_bix_broken_package;
>  static int battery_notification_delay_ms;
> +static int battery_full_discharging;
>  static unsigned int cache_time = 1000;
>  module_param(cache_time, uint, 0644);
>  MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> @@ -214,7 +215,10 @@ static int acpi_battery_get_property(struct power_supply 
> *psy,
> return -ENODEV;
> switch (psp) {
> case POWER_SUPPLY_PROP_STATUS:
> -   if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
> +   if (battery_full_discharging && battery->rate_now == 0 &&
> +   battery->state & ACPI_BATTERY_STATE_DISCHARGING)
> +   val->intval = POWER_SUPPLY_STATUS_FULL;
> +   else if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
> val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
> val->intval = POWER_SUPPLY_STATUS_CHARGING;
> @@ -1166,6 +1170,13 @@ battery_notification_delay_quirk(const struct 
> dmi_system_id *d)
> return 0;
>  }
>
> +static int __init
> +battery_full_discharging_quirk(const struct dmi_system_id *d)
> +{
> +   battery_full_discharging = 1;
> +   return 0;
> +}
> +
>  static const struct dmi_system_id bat_dmi_table[] __initconst = {
> {
> .callback = battery_bix_broken_package_quirk,
> @@ -1183,6 +1194,22 @@ static const struct dmi_system_id bat_dmi_table[] 
> __initconst = {
> DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
> },
> },
> +   {
> +   .callback = battery_full_discharging_quirk,
> +   .ident = "ASUS GL502VSK",
> +   .matches = {
> +   DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +   DMI_MATCH(DMI_PRODUCT_NAME, "GL502VSK"),
> +   },
> +   },
> +   {
> +   .callback = battery_full_discharging_quirk,
> +   .ident = "ASUS UX305LA",
> +   .matches = {
> +   DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +   DMI_MATCH(DMI_PRODUCT_NAME, "UX305LA"),
> +   },
> +   },
> {},
>  };
>

Hi,

Is there any improvement I can make for this patch?

> --
> 2.14.1
>

[PATCH 1/2] r8169: reinstate ALDPS for power saving

[Kai-Heng Feng]

Commit ("r8169: enable ALDPS for power saving") caused a regression on
RTL8168evl/8111evl [1], so it got reverted.

Instead of reverting the whole commit, let's reinstate ALDPS for all
models other than RTL8168evl/8111evl.

[1] https://lkml.org/lkml/2013/1/5/36

Cc: Francois Romieu <rom...@fr.zoreil.com>
Cc: Hayes Wang <hayesw...@realtek.com>
Cc: Jörg Otte <jrg.o...@gmail.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/ethernet/realtek/r8169.c | 58 +---
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c 
b/drivers/net/ethernet/realtek/r8169.c
index e03fcf914690..54bb9b15d541 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -733,6 +733,7 @@ enum features {
RTL_FEATURE_WOL = (1 << 0),
RTL_FEATURE_MSI = (1 << 1),
RTL_FEATURE_GMII= (1 << 2),
+   RTL_FEATURE_FW_LOADED   = (1 << 3),
 };
 
 struct rtl8169_counters {
@@ -2785,8 +2786,10 @@ static void rtl_apply_firmware(struct rtl8169_private 
*tp)
struct rtl_fw *rtl_fw = tp->rtl_fw;
 
/* TODO: release firmware once rtl_phy_write_fw signals failures. */
-   if (!IS_ERR_OR_NULL(rtl_fw))
+   if (!IS_ERR_OR_NULL(rtl_fw)) {
rtl_phy_write_fw(tp, rtl_fw);
+   tp->features |= RTL_FEATURE_FW_LOADED;
+   }
 }
 
 static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 
val)
@@ -2797,6 +2800,31 @@ static void rtl_apply_firmware_cond(struct 
rtl8169_private *tp, u8 reg, u16 val)
rtl_apply_firmware(tp);
 }
 
+static void r810x_aldps_disable(struct rtl8169_private *tp)
+{
+   rtl_writephy(tp, 0x1f, 0x);
+   rtl_writephy(tp, 0x18, 0x0310);
+   msleep(100);
+}
+
+static void r810x_aldps_enable(struct rtl8169_private *tp)
+{
+   if (!(tp->features & RTL_FEATURE_FW_LOADED))
+   return;
+
+   rtl_writephy(tp, 0x1f, 0x);
+   rtl_writephy(tp, 0x18, 0x8310);
+}
+
+static void r8168_aldps_enable_1(struct rtl8169_private *tp)
+{
+   if (!(tp->features & RTL_FEATURE_FW_LOADED))
+   return;
+
+   rtl_writephy(tp, 0x1f, 0x);
+   rtl_w0w1_phy(tp, 0x15, 0x1000, 0x);
+}
+
 static void rtl8169s_hw_phy_config(struct rtl8169_private *tp)
 {
static const struct phy_reg phy_reg_init[] = {
@@ -3587,6 +3615,10 @@ static void rtl8168e_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0400);
rtl_writephy(tp, 0x1f, 0x);
 
+   /* Don't enable ALDPS for RTL8168evl/8111evl.
+* https://lkml.org/lkml/2013/1/5/36
+*/
+
/* Broken BIOS workaround: feed GigaMAC registers with MAC address. */
rtl_rar_exgmac_set(tp, tp->dev->dev_addr);
 }
@@ -3661,6 +3693,8 @@ static void rtl8168f_1_hw_phy_config(struct 
rtl8169_private *tp)
rtl_writephy(tp, 0x05, 0x8b85);
rtl_w0w1_phy(tp, 0x06, 0x4000, 0x);
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp)
@@ -3668,6 +3702,8 @@ static void rtl8168f_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_apply_firmware(tp);
 
rtl8168f_hw_phy_config(tp);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8411_hw_phy_config(struct rtl8169_private *tp)
@@ -4019,6 +4055,8 @@ static void rtl8168h_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0004);
 
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp)
@@ -4188,21 +4226,19 @@ static void rtl8105e_hw_phy_config(struct 
rtl8169_private *tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_writephy(tp, 0x1f, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(100);
+   r810x_aldps_disable(tp);
 
rtl_apply_firmware(tp);
 
rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init));
+
+   r810x_aldps_enable(tp);
 }
 
 static void rtl8402_hw_phy_config(struct rtl8169_private *tp)
 {
/* Disable ALDPS before setting firmware */
-   rtl_writephy(tp, 0x1f, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(20);
+   r810x_aldps_disable(tp);
 
rtl_apply_firmware(tp);
 
@@ -4212,6 +4248,8 @@ static void rtl8402_hw_phy_config(struct rtl8169_private 
*tp)
rtl_writephy(tp, 0x10, 0x401f);
rtl_writephy(tp, 0x19, 0x7030);
rtl_writephy(tp, 0x1f, 0x);
+
+   r810x_aldps_enable(tp);
 }
 
 static void rtl8106e_hw_phy_config(struct rtl8169_private *tp)
@@ -4224,9 +4262,7 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private 
*tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_wr

[PATCH 2/2] r8169: reinstate internal ASPM and clock request settings

[Kai-Heng Feng]

Commit ("r8169: enable internal ASPM and clock request settings") caused
a regression on RTL8168evl/8111evl [1], so it got reverted.

Instead of reverting the whole commit, let's reinstate ASPM for all
models other than RTL8168evl/8111evl.

[1] https://lkml.org/lkml/2013/1/5/36

Cc: Francois Romieu <rom...@fr.zoreil.com>
Cc: Hayes Wang <hayesw...@realtek.com>
Cc: Jörg Otte <jrg.o...@gmail.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/ethernet/realtek/r8169.c | 28 ++--
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c 
b/drivers/net/ethernet/realtek/r8169.c
index 54bb9b15d541..6d6f3079320a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -510,6 +510,7 @@ enum rtl8168_registers {
 #define PWM_EN (1 << 22)
 #define RXDV_GATED_EN  (1 << 19)
 #define EARLY_TALLY_EN (1 << 16)
+#define FORCE_CLK  (1 << 15) /* force clock request */
 };
 
 enum rtl_register_content {
@@ -5992,6 +5993,11 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private 
*tp)
 
RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
RTL_W32(MISC, RTL_R32(MISC) | PWM_EN);
+
+   /* Don't enable ASPM for RTL8168evl/8111evl.
+* https://lkml.org/lkml/2013/1/5/36
+*/
+
RTL_W8(Config5, RTL_R8(Config5) & ~Spi_en);
 }
 
@@ -6017,13 +6023,12 @@ static void rtl_hw_start_8168f(struct rtl8169_private 
*tp)
 
RTL_W8(MaxTxPacketSize, EarlySize);
 
-   rtl_disable_clock_request(pdev);
-
RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO);
RTL_W8(MCU, RTL_R8(MCU) & ~NOW_IS_OOB);
RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
-   RTL_W32(MISC, RTL_R32(MISC) | PWM_EN);
-   RTL_W8(Config5, RTL_R8(Config5) & ~Spi_en);
+   RTL_W32(MISC, RTL_R32(MISC) | PWM_EN | FORCE_CLK);
+   RTL_W8(Config5, (RTL_R8(Config5) & ~Spi_en) | ASPM_en);
+   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 }
 
 static void rtl_hw_start_8168f_1(struct rtl8169_private *tp)
@@ -6083,8 +6088,10 @@ static void rtl_hw_start_8168g(struct rtl8169_private 
*tp)
rtl_w0w1_eri(tp, 0xdc, ERIAR_MASK_0001, 0x01, 0x00, ERIAR_EXGMAC);
rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
 
-   RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
+   RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
RTL_W8(MaxTxPacketSize, EarlySize);
+   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 
rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x, ERIAR_EXGMAC);
rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x, ERIAR_EXGMAC);
@@ -6594,6 +6601,9 @@ static void rtl_hw_start_8105e_1(struct rtl8169_private 
*tp)
 
RTL_W8(MCU, RTL_R8(MCU) | EN_NDP | EN_OOB_RESET);
RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
+   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
+   RTL_W32(MISC, RTL_R32(MISC) | FORCE_CLK);
 
rtl_ephy_init(tp, e_info_8105e_1, ARRAY_SIZE(e_info_8105e_1));
 
@@ -6621,6 +6631,9 @@ static void rtl_hw_start_8402(struct rtl8169_private *tp)
 
RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO);
RTL_W8(MCU, RTL_R8(MCU) & ~NOW_IS_OOB);
+   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
+   RTL_W32(MISC, RTL_R32(MISC) | FORCE_CLK);
 
rtl_ephy_init(tp, e_info_8402, ARRAY_SIZE(e_info_8402));
 
@@ -6644,7 +6657,10 @@ static void rtl_hw_start_8106(struct rtl8169_private *tp)
/* Force LAN exit from ASPM if Rx/Tx are not idle */
RTL_W32(FuncEvent, RTL_R32(FuncEvent) | 0x002800);
 
-   RTL_W32(MISC, (RTL_R32(MISC) | DISABLE_LAN_EN) & ~EARLY_TALLY_EN);
+   RTL_W32(MISC,
+   (RTL_R32(MISC) | DISABLE_LAN_EN | FORCE_CLK) & ~EARLY_TALLY_EN);
+   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
RTL_W8(MCU, RTL_R8(MCU) | EN_NDP | EN_OOB_RESET);
RTL_W8(DLLPR, RTL_R8(DLLPR) & ~PFM_EN);
 
-- 
2.14.1

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 27 Nov 2017, at 11:13 PM,  
>  wrote:
> 
> This is quite surprising to me too.  The externally plugged in r8153 dongle,
> was it connected over type C port or over type A port?  AFAIK Type C port is
> actually Alpine ridge pass through port.  It is not connected to XHCI 
> controller
> or USB hub.

Over the front type A port, which connects to SMSC hub then ASMedia controller.

The type C port indeed connects to AR directly, hence no such issue.

Kai-Heng

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 24 Nov 2017, at 4:28 PM, Greg KH  wrote:
> 
> The bcdDevice is different between the dock device and the "real"
> device, why not use that?

Yea, I’ll poke around and see if bcdDevice alone can be a good predicate.

> Then there is still a bug.  Who as ASMedia is working on this, have they
> posted anything to the linux-usb mailing list about it?

I think they are doing this internally. I’ll advice them to ask questions here 
if
they encounter any problem.

> Maybe.  Have you tried using usbmon to see what the data streams are for
> the two devices and where they have problems and diverge?  Is the dock
> device doing something different in response to something from the host
> that the "real" device does not do?

No I haven’t.
Not really sure how do debug network packets over USB. I’ll do some research
on the topic.

Kai-Heng

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> Also the MAC address is different, can you just trigger off of Dell's
> MAC address space instead of the address space of the dongle device?

A really good idea, never thought of this. Thanks for the hint :)
Still, I need to ask Dell folks to get all the answers.

Kai-Heng

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 23 Nov 2017, at 5:24 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Thu, Nov 23, 2017 at 04:53:41PM +0800, Kai Heng Feng wrote:
>> 
>> What I want to do here is to finding this connection:
>> Realtek r8153 <-> SMSC hub (USD ID: 0424:5537) <-> 
>> ASMedia XHCI controller (PCI ID: 1b21:1142).
>> 
>> Is there a safer way to do this?
> 
> Nope!  You can't do that at all from within a USB driver, sorry.  As you
> really should not care at all :)

Got it :)

The r8153 in Dell TB dock has version information, RTL_VER_05.
We can use it to check for workaround, but many working RTL_VER_05 devices
will also be affected.
Do you think it’s an acceptable compromise?

>> I have a r8153 <-> USB 3.0 dongle which work just fine. I can’t find any 
>> information to differentiate them. Hence I want to use the connection to
>> identify if r8153 is on a Dell TB dock.
> 
> Are you sure there is nothing different in the version or release number
> of the device?  'lsusb -v' shows the exact same information for both
> devices?

Yes. I attached `lsusb -v` for r8153 on Dell TB dock, on a RJ45 <-> USB 3.0 
dongle,
and on a RJ45 <-> USB Type-C dongle.

>> Yes. From what I know, ASMedia is working on it, but not sure how long it
>> will take. In the meantime, I’d like to workaround this issue for the users.
> 
> Again, it's a host controller bug, it should be fixed there, don't try
> to paper over the real issue in different individual drivers.
> 
> I think I've seen various patches on the linux-usb list for this
> controller already, have you tried them?

Yes. These patches are all in mainline Linux now.

>> Actually no.
>> I just plugged r8153 dongle into the same hub, surprisingly the issue
>> doesn’t happen in this scenario.
> 
> Then something seems to be wrong with the device itself, as that would
> be the same exact electrical/logical path, right?

I have no idea why externally plugged one doesn’t have this issue.
Maybe it’s related how it’s wired inside the Dell TB dock...

Kai-Heng



lsusb-a
Description: Binary data


lsusb-c
Description: Binary data


lsusb-dock
Description: Binary data

> thanks,
> 
> greg k-h

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 23 Nov 2017, at 3:58 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Thu, Nov 23, 2017 at 01:38:38AM -0500, Kai-Heng Feng wrote:
>> r8153 on Dell TB dock corrupts rx packets.
>> 
>> The root cause is not found yet, but disabling rx checksumming can
>> workaround the issue. We can use this connection to decide if it's
>> a Dell TB dock:
>> Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
>> 
>> BugLink: https://bugs.launchpad.net/bugs/1729674
>> Cc: Mario Limonciello <mario.limoncie...@dell.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>> drivers/net/usb/r8152.c | 33 -
>> 1 file changed, 32 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index d51d9abf7986..58b80b5e7803 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -27,6 +27,8 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> +#include 
>> 
>> /* Information for net-next */
>> #define NETNEXT_VERSION  "09"
>> @@ -5135,6 +5137,35 @@ static u8 rtl_get_version(struct usb_interface *intf)
>>  return version;
>> }
>> 
>> +/* Ethernet on Dell TB 15/16 dock is connected this way:
>> + * Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
>> + * We use this connection to make sure r8153 is on the Dell TB dock.
>> + */
>> +static bool check_dell_tb_dock(struct usb_device *udev)
>> +{
>> +struct usb_device *hub = udev->parent;
>> +struct usb_device *root_hub;
>> +struct pci_dev *controller;
>> +
>> +if (!hub)
>> +return false;
>> +
>> +if (!(le16_to_cpu(hub->descriptor.idVendor) == 0x0424 &&
>> +  le16_to_cpu(hub->descriptor.idProduct) == 0x5537))
>> +return false;
>> +
>> +root_hub = hub->parent;
>> +if (!root_hub || root_hub->parent)
>> +return false;
>> +
>> +controller = to_pci_dev(bus_to_hcd(root_hub->bus)->self.controller);
> 
> That's a very scary, and dangerous, cast.  You can not ever be sure that
> the hub really is a "root hub" like this.

What I want to do here is to finding this connection:
Realtek r8153 <-> SMSC hub (USD ID: 0424:5537) <-> 
ASMedia XHCI controller (PCI ID: 1b21:1142).

Is there a safer way to do this?

> 
>> +if (controller->vendor == 0x1b21 && controller->device == 0x1142)
>> +return true;
> 
> Why can't you just look at the USB device itself and go off of a quirk
> in it?  Something like a version or string or something else?

I have a r8153 <-> USB 3.0 dongle which work just fine. I can’t find any 
information to differentiate them. Hence I want to use the connection to
identify if r8153 is on a Dell TB dock.

> 
> This sounds like a USB host controller issue, not a USB device issue,
> can't we fix the "real" problem here instead of this crazy work-around?

Yes. From what I know, ASMedia is working on it, but not sure how long it
will take. In the meantime, I’d like to workaround this issue for the users.

> Odds are any device plugged into the hub should have the same issue,
> right?

Actually no.
I just plugged r8153 dongle into the same hub, surprisingly the issue
doesn’t happen in this scenario.

Kai-Heng

> 
> thanks,
> 
> greg k-h

[PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai-Heng Feng]

r8153 on Dell TB dock corrupts rx packets.

The root cause is not found yet, but disabling rx checksumming can
workaround the issue. We can use this connection to decide if it's
a Dell TB dock:
Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller

BugLink: https://bugs.launchpad.net/bugs/1729674
Cc: Mario Limonciello <mario.limoncie...@dell.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/usb/r8152.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index d51d9abf7986..58b80b5e7803 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* Information for net-next */
 #define NETNEXT_VERSION"09"
@@ -5135,6 +5137,35 @@ static u8 rtl_get_version(struct usb_interface *intf)
return version;
 }
 
+/* Ethernet on Dell TB 15/16 dock is connected this way:
+ * Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
+ * We use this connection to make sure r8153 is on the Dell TB dock.
+ */
+static bool check_dell_tb_dock(struct usb_device *udev)
+{
+   struct usb_device *hub = udev->parent;
+   struct usb_device *root_hub;
+   struct pci_dev *controller;
+
+   if (!hub)
+   return false;
+
+   if (!(le16_to_cpu(hub->descriptor.idVendor) == 0x0424 &&
+ le16_to_cpu(hub->descriptor.idProduct) == 0x5537))
+   return false;
+
+   root_hub = hub->parent;
+   if (!root_hub || root_hub->parent)
+   return false;
+
+   controller = to_pci_dev(bus_to_hcd(root_hub->bus)->self.controller);
+
+   if (controller->vendor == 0x1b21 && controller->device == 0x1142)
+   return true;
+
+   return false;
+}
+
 static int rtl8152_probe(struct usb_interface *intf,
 const struct usb_device_id *id)
 {
@@ -5202,7 +5233,7 @@ static int rtl8152_probe(struct usb_interface *intf,
NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
 
-   if (tp->version == RTL_VER_01) {
+   if (tp->version == RTL_VER_01 || check_dell_tb_dock(udev)) {
netdev->features &= ~NETIF_F_RXCSUM;
netdev->hw_features &= ~NETIF_F_RXCSUM;
}
-- 
2.14.1

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi,

> On 16 Dec 2017, at 11:05 AM, Matthias Kaehlcke  wrote:
> stable branches are currently broken for BTUSB_QCA_ROME with USB
> autosuspend enabled, since the above patch is not included (I only
> checked v4.4 and v4.9), so we probably want to integrate it.

Thanks for this information.

I just found the same issue and I can confirm your finding.

I was testing with runtime suspend disabled.
Once the runtime suspend gets enabled, the additional patch is needed.

I’ll remember to enable runtime pm next time.

Thanks again.

Kai-Heng

> 
> Thanks
> 
> Matthias

Re: [PATCH 4.13 03/28] Bluetooth: btusb: fix QCA Rome suspend/resume

[Kai Heng Feng]

Hi Brian,

> On 19 Dec 2017, at 2:13 AM, Brian Norris  wrote:
> 
> Hi Greg,
> 
> On Mon, Dec 18, 2017 at 12:43:48PM +0100, Greg Kroah-Hartman wrote:
>> On Fri, Dec 15, 2017 at 07:05:39PM -0800, Matthias Kaehlcke wrote:
>>> We identified the above patch as the culprit, in combination with USB
>>> autosuspend being enabled for the Bluetooth controller.
>>> 
>>> We found that the following (recent) upstream patch fixes the issue on
>>> most devices (we are still investigating one case on a specific device):
> 
> A key word to highlight here is "most". I have at least one device that
> is consistently having problems even with both patches included; the
> only way things work reliably so far is to simply revert the $subject
> patch in 4.4.x.

The problem we have is that the QCA Rome Bluetooth stops working
after system S3. The reset resume quirk workaround the issue on both
mainline and 4.4.x (at least for me).

> 
> I'll try to investigate further, since this result is a bit confusing
> still. If there's a real problem with the patch (and I suspect there
> might be), it probably shouldn't be in mainline either…

Do you have the same issue on mainline?

> 
>>> commit a0085f2510e8976614ad8f766b209448b385492f
>>> Author: Sukumar Ghorai 
>>> Date:   Wed Aug 16 14:46:55 2017 -0700
>>> 
>>>Bluetooth: btusb: driver to enable the usb-wakeup feature
> [...]
>>> stable branches are currently broken for BTUSB_QCA_ROME with USB
>>> autosuspend enabled, since the above patch is not included (I only
>>> checked v4.4 and v4.9), so we probably want to integrate it.
>> 
>> Now queued up, thanks for letting me know.
> 
> I think you have almost as much information as I do at this point, and
> I'll try to reply back here if I figure out anything more, so I'll leave
> you to your decisions. But I would personally revert, not backport more
> patches.
> 
> Among the reasons:
> (a) I have at least one device that does not work better with both
>patches [1]
> (b) So far, I haven't seen any explanation why commit a0085f2510e8
>should be a dependency for $subject patch; the closest I can imagine
>would be that commit a0085f2510e8 could effectively neutralize
>$subject patch -- if the device is marked as a wakeup source, then
>it will never try to RESET on resume -- but that's still a fuzzy
>signal; just because it's marked a wakeup source sometimes doesn't
>mean it always will be. We can disable it in user space too.

Hi Leif,

Can you try if a0085f2510e8 breaks your $subject commit?
If it’s a yes, then what Brian suggests is correct.

Also, can you share the output of "usb-device” for your btusb device?


> (c) Isn't it safer to revert than to backport more? You're delving into
>feature-land, not bugfix-land…

Sounds reasonable.

We’ll need more information from Leif if we need to do ID-specific quirks.

Kai-Heng

> Brian
> 
> [1] Before you ask: this particular device is not quite fully supported
> on upstream yet, though its sibling devices are. With a bit of effort, I
> might be able to test a clean upstream. At the moment, I'm using our
> Chrom{e,ium}OS 4.4 kernel, where we regularly merge in -stable updates.