> -----Original Message----- > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Monday, June 12, 2017 9:02 AM > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > Cc: Lars_Chang@xxxxxxxxxxxxxx; felipe.balbi@xxxxxxxxxxxxxxx; > jiahau@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; mathias.nyman@xxxxxxxxx; > Brain_Lee@xxxxxxxxxxxxxx; Justin_CY_Chen@xxxxxxxxxxxxxx; Wang, Keith > <Keith_Wang@xxxxxxxx>; Yd_Tseng@xxxxxxxxxxxxxx > Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in ASM1042A host > > On Mon, Jun 12, 2017 at 01:52:36PM +0000, Mario.Limonciello@xxxxxxxx wrote: > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > > Sent: Monday, June 12, 2017 8:42 AM > > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > > > Cc: Lars_Chang@xxxxxxxxxxxxxx; felipe.balbi@xxxxxxxxxxxxxxx; > > > jiahau@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; mathias.nyman@xxxxxxxxx; > > > Brain_Lee@xxxxxxxxxxxxxx; Justin_CY_Chen@xxxxxxxxxxxxxx; Wang, Keith > > > <Keith_Wang@xxxxxxxx>; Yd_Tseng@xxxxxxxxxxxxxx > > > Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in ASM1042A > host > > > > > > On Mon, Jun 12, 2017 at 01:28:57PM +0000, Mario.Limonciello@xxxxxxxx > wrote: > > > > > -----Original Message----- > > > > > From: Lars Chang(張家豪) [mailto:Lars_Chang@xxxxxxxxxxxxxx] > > > > > Sent: Friday, June 9, 2017 1:34 AM > > > > > To: 'Felipe Balbi' <felipe.balbi@xxxxxxxxxxxxxxx>; Jiahau Chang > > > > > <jiahau@xxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; > mathias.nyman@xxxxxxxxx > > > > > Cc: Brain Lee(李魁中) <Brain_Lee@xxxxxxxxxxxxxx>; Limonciello, Mario > > > > > <Mario_Limonciello@xxxxxxxx>; Justin_CY Chen(陳志勇) > > > > > <Justin_CY_Chen@xxxxxxxxxxxxxx>; Wang, Keith > <Keith_Wang@xxxxxxxx>; > > > Yd > > > > > Tseng(曾裕達) <Yd_Tseng@xxxxxxxxxxxxxx>; Jiahau Chang > > > <jiahau@xxxxxxxxx> > > > > > Subject: RE: [PATCH v2] xhci: Bad Ethernet performance plugged in > ASM1042A > > > host > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Felipe Balbi [mailto:felipe.balbi@xxxxxxxxxxxxxxx] > > > > > > Sent: Friday, June 09, 2017 2:04 PM > > > > > > To: Jiahau Chang; linux-usb@xxxxxxxxxxxxxxx; mathias.nyman@xxxxxxxxx > > > > > > Cc: Brain Lee(李魁中); mario.limonciello@xxxxxxxx; Justin_CY Chen(陳志 > 勇); > > > > > > keith_Wang@xxxxxxxx; Yd Tseng(曾裕達); Jiahau Chang; Lars Chang(張家 > 豪) > > > > > > Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in > ASM1042A > > > > > > host > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > Jiahau Chang <jiahau@xxxxxxxxx> writes: > > > > > > > v2 : fix coding format > > > > > > > > > > > > this doesn't need to be in the commit log. You should place such notes > > > > > > after the tearline (---) below > > > > > > > > > > I will move it after the tearline. Should I need to resend the patch? > > > > > > > > > > > > > When USB Ethernet is plugged in ASMEDIA ASM1042A xHCI host, bad > > > > > > > performance was manifesting in Web browser use (like download > > > > > > > large file such as ISO image). It is known limitation of > > > > > > > ASM1042A that is not compatible with driver scheduling, > > > > > > > As a workaround we can modify flow control handling of ASM1042A. > > > > > > > > > > > > > > Signed-off-by: Jiahau Chang <Lars_chang@xxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > drivers/usb/host/pci-quirks.c | 62 > > > > > > +++++++++++++++++++++++++++++++++++++++++++ > > > > > > > drivers/usb/host/pci-quirks.h | 1 + > > > > > > > drivers/usb/host/xhci-pci.c | 5 ++++ > > > > > > > drivers/usb/host/xhci.c | 3 +++ > > > > > > > drivers/usb/host/xhci.h | 1 + > > > > > > > 5 files changed, 72 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c > > > > > > > index a9a1e4c..bdeaf75 100644 > > > > > > > --- a/drivers/usb/host/pci-quirks.c > > > > > > > +++ b/drivers/usb/host/pci-quirks.c > > > > > > > @@ -77,6 +77,12 @@ > > > > > > > #define USB_INTEL_USB3_PSSEN 0xD8 > > > > > > > #define USB_INTEL_USB3PRM 0xDC > > > > > > > > > > > > > > +/*ASMEDIA quirk use*/ > > > > > > > +#define ASMT_DATA_WRITE0_REG 0xF8 > > > > > > > +#define ASMT_DATA_WRITE1_REG 0xFC > > > > > > > +#define ASMT_CONTROL_REG 0xE0 > > > > > > > +#define ASMT_CONTROL_WRITE_BIT 0x02 > > > > > > > + > > > > > > > /* > > > > > > > * amd_chipset_gen values represent AMD different chipset generations > > > > > > > */ > > > > > > > @@ -412,6 +418,62 @@ void usb_amd_quirk_pll_disable(void) > > > > > > > } > > > > > > > EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_disable); > > > > > > > > > > > > > > +void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev) > > > > > > > +{ > > > > > > > + u32 value_low, value_high; > > > > > > > + unsigned char value; > > > > > > > + unsigned long wait_time_count; > > > > > > > + > > > > > > > + wait_time_count = 1000; > > > > > > > + while (wait_time_count) { > > > > > > > + pci_read_config_byte(pdev, ASMT_CONTROL_REG, > > > &value); > > > > > > > + if (value == 0xff) { > > > > > > > + dev_dbg(&pdev->dev, "%s: wait_write_ready > > > IO_ERROR, > > > > > > value=%x\n", > > > > > > > + __func__, value); > > > > > > > + goto err_exit; > > > > > > > + } else if ((value & ASMT_CONTROL_WRITE_BIT) == 0) { > > > > > > > + break; > > > > > > > + } > > > > > > > + wait_time_count--; > > > > > > > + udelay(50); > > > > > > > + } > > > > > > > + if (wait_time_count == 0) { > > > > > > > + dev_dbg(&pdev->dev, "%s: wait_write_ready timeout\n", > > > > > > > + __func__); > > > > > > > + goto err_exit; > > > > > > > + } > > > > > > > + value_low = 0x00010423; > > > > > > > + value_high = 0xFA30; > > > > > > > > > > > > sorry, no magic constants > > > > > > > > > > > > > + pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, > > > value_low); > > > > > > > + pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, > > > value_high); > > > > > > > + pci_write_config_byte(pdev, ASMT_CONTROL_REG, > > > > > > ASMT_CONTROL_WRITE_BIT); > > > > > > > + wait_time_count = 1000; > > > > > > > + while (wait_time_count) { > > > > > > > + pci_read_config_byte(pdev, ASMT_CONTROL_REG, > > > &value); > > > > > > > + if (value == 0xff) { > > > > > > > + dev_dbg(&pdev->dev, "%s: wait_write_ready > > > IO_ERROR, > > > > > > value=%x\n", > > > > > > > + __func__, value); > > > > > > > + goto err_exit; > > > > > > > > > > > > indentation > > > > > > > > > > > > > + } else if ((value & ASMT_CONTROL_WRITE_BIT) == 0) { > > > > > > > + break; > > > > > > > + } > > > > > > > + wait_time_count--; > > > > > > > + udelay(50); > > > > > > > + } > > > > > > > + if (wait_time_count == 0) { > > > > > > > + dev_dbg(&pdev->dev, "%s: wait_write_ready timeout\n", > > > > > > > + __func__); > > > > > > > + goto err_exit; > > > > > > > + } > > > > > > > + pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, 0xBA); > > > > > > > + pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, 0); > > > > > > > > > > > > likewise > > > > > > > > > > > > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > > > > > > > index 73a28a9..ed58f87 100644 > > > > > > > --- a/drivers/usb/host/xhci.h > > > > > > > +++ b/drivers/usb/host/xhci.h > > > > > > > @@ -1819,6 +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_ASMEDIA_MODIFY_FLOWCONTROL (1<<27) > > > > > > > > > > > > spaces around << > > > > > > > > > > > > -- > > > > > > balbi > > > > > > > > Aside from the style comments and mistakenly added email footer on one the > > > > responses, are there any other comments about this patch/approach or > should > > > > ASMedia resubmit v3 to take these proposed changes into account now? > > > > > > Why wouldn't they fix it up and resend? We can't do anything with > > > patches with footers saying they were sent out to the world illegally. > > > And we aren't going to fix up whitespace issues on our own, that's the > > > patch owner's job. > > > > > Sorry that may have been unclear. > > I meant should they wait for more feedback or fixup/resend now? > > Why wouldn't someone resend after any type of feedback? There is > nothing we can do with the patch as-is, right? Felipe explicitly said to wait in another response, so that's why I wanted to confirm if someone is still reviewing the patch that ASMedia should wait for. " it's best to wait for more review comments. You don't want to spam the list." Based on the lack of other response and your comments I think ASMedia should just resubmit v3 now with the mentioned fixes now. > > > The footer was on one of their responses, not the original patch, so I > > don't think the footer should prevent feedback on the original patch. > > It causes lawyers to get really angry when they see those, and I have > learned to just delete the email entirely, and so does most everyone > else. It is not compatible with kernel development at all. > I understand and agree. ASMedia is new to this (and I suspect their email system probably does that automatically), so I'm trying to be as helpful as we can be. ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
