> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 12, 2017 9:02 AM
> To: Limonciello, Mario <mario_limoncie...@dell.com>
> Cc: lars_ch...@asmedia.com.tw; felipe.ba...@linux.intel.com;
> jia...@gmail.com; linux-usb@vger.kernel.org; mathias.ny...@intel.com;
> brain_...@asmedia.com.tw; justin_cy_c...@asmedia.com.tw; Wang, Keith
> <keith_w...@dell.com>; yd_ts...@asmedia.com.tw
> Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in ASM1042A
> host
>
> On Mon, Jun 12, 2017 at 01:52:36PM +0000, mario.limoncie...@dell.com wrote:
> > > -----Original Message-----
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Monday, June 12, 2017 8:42 AM
> > > To: Limonciello, Mario <mario_limoncie...@dell.com>
> > > Cc: lars_ch...@asmedia.com.tw; felipe.ba...@linux.intel.com;
> > > jia...@gmail.com; linux-usb@vger.kernel.org; mathias.ny...@intel.com;
> > > brain_...@asmedia.com.tw; justin_cy_c...@asmedia.com.tw; Wang, Keith
> > > <keith_w...@dell.com>; yd_ts...@asmedia.com.tw
> > > Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in ASM1042A
> host
> > >
> > > On Mon, Jun 12, 2017 at 01:28:57PM +0000, mario.limoncie...@dell.com
> wrote:
> > > > > -----Original Message-----
> > > > > From: Lars Chang(張家豪) [mailto:lars_ch...@asmedia.com.tw]
> > > > > Sent: Friday, June 9, 2017 1:34 AM
> > > > > To: 'Felipe Balbi' <felipe.ba...@linux.intel.com>; Jiahau Chang
> > > > > <jia...@gmail.com>; linux-usb@vger.kernel.org;
> mathias.ny...@intel.com
> > > > > Cc: Brain Lee(李魁中) <brain_...@asmedia.com.tw>; Limonciello, Mario
> > > > > <mario_limoncie...@dell.com>; Justin_CY Chen(陳志勇)
> > > > > <justin_cy_c...@asmedia.com.tw>; Wang, Keith
> <keith_w...@dell.com>;
> > > Yd
> > > > > Tseng(曾裕達) <yd_ts...@asmedia.com.tw>; Jiahau Chang
> > > <jia...@gmail.com>
> > > > > Subject: RE: [PATCH v2] xhci: Bad Ethernet performance plugged in
> ASM1042A
> > > host
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Felipe Balbi [mailto:felipe.ba...@linux.intel.com]
> > > > > > Sent: Friday, June 09, 2017 2:04 PM
> > > > > > To: Jiahau Chang; linux-usb@vger.kernel.org; mathias.ny...@intel.com
> > > > > > Cc: Brain Lee(李魁中); mario.limoncie...@dell.com; Justin_CY Chen(陳志
> 勇);
> > > > > > keith_w...@dell.com; Yd Tseng(曾裕達); Jiahau Chang; Lars Chang(張家
> 豪)
> > > > > > Subject: Re: [PATCH v2] xhci: Bad Ethernet performance plugged in
> ASM1042A
> > > > > > host
> > > > > >
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Jiahau Chang <jia...@gmail.com> 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_ch...@asmedia.com.tw>
> > > > > > > ---
> > > > > > > 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�����r��y����b�X��ǧv�^�){.n�+����{������^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
