Re: USB mass storage device inaccessible, freezes lsusb
On Sun, 7 Oct 2018, Alan Stern wrote: > On Fri, 5 Oct 2018, Christoph Groth wrote: > > > I would be grateful for hints on how solve or further debug this > > problem. > > A good start would be to post a usbmon trace showing what happens when > you plug in the Garmin device. Also you could post the dmesg log, > because it might contain some useful information about what happened > during the failure. Thanks, Alan. Here are two sets of logs: * x220: The device freezes. Taken on a ThinkPad X220 (running 4.17.0-3-amd64 #1 SMP Debian 4.17.17-1 (2018-08-18) x86_64 GNU/Linux) * cubie: The device works. Taken on a Cubieboard2 (running 4.9.0-6-armmp-lpae #1 SMP Debian 4.9.88-1+deb9u1 (2018-05-07) armv7l GNU/Linux) The usbmon logs should contain lines only from the device in question. In x220.usbmon.gz, I marked the point where output ceases before the device is unplugged. x220.dmesg.gz Description: application/gzip x220.usbmon.gz Description: application/gzip cubie.dmesg.gz Description: application/gzip cubie.usbmon.gz Description: application/gzip
Query on usb/core/devio.c
Hi Greg KH, I have a query regarding the USB-FS driver, usb/core/devio.c. It looks like it takes a PM ref-count on the USB device in its .open() and release the PM ref-count on the USB device in its .release(). As a result of this, the USB suspend (L2) does not seem to happen, even if all the interface drivers of a composite USB device report "idle" to USB core driver. The USB suspend seem to happen only when the caller in user-space (in our case) closes the device file. Is this correct understanding? If yes, could you please help understand - 1. Any specific reason to choose this design approach? Apologies, but "git blame" does not reveal much information (or maybe I did not do git blame on correct kernel version). 2. Is it possible to modify this driver to take PM ref-count on USB device, only when user-space is actively interacting with the USB device (so in open/close and appropriate ioctl calls, with special handling for async URB submission)? 3. Will (2) break any known existing device(s)? Please feel free to correct if any of above information/questions are incorrect or incorrectly understood. Best Regards, Mayuresh
Re: Query on usb/core/devio.c
On Mo, 2018-10-08 at 10:50 +0100, Mayuresh Kulkarni wrote: > > As a result of this, the USB suspend (L2) does not seem to happen, even if > all the interface drivers of a composite USB device report "idle" to USB core > driver. The USB suspend seem to happen only when the caller in user-space (in > our case) closes the device file. > > Is this correct understanding? Yes, it is. > If yes, could you please help understand - > 1. Any specific reason to choose this design approach? Apologies, but "git > blame" does not reveal much information (or maybe I did not do git blame on > correct kernel version). We cannot assume that a device is done executing a command as soon as communication is done. Think of a scanner moving its sensor bar or a printer printing a page. Or a display displaying something. > 2. Is it possible to modify this driver to take PM ref-count on USB device, > only when user-space is actively interacting with the USB device (so in > open/close and appropriate ioctl calls, with special handling for async URB > submission)? Technically this is possible, but it is a bad idea. > 3. Will (2) break any known existing device(s)? Yes, it would and that makes it a bad idea. Regards Oliver
Re: [GIT PULL] usb: chipidea: changes for v4.20-rc1
On Mon, Oct 08, 2018 at 01:58:00AM +, Peter Chen wrote: > The following changes since commit 1652a83fa494b12e20fc02a2cc3ddbcd75d53170: > > Merge 4.19-rc4 into usb-next (2018-09-16 22:44:14 +0200) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git/ > tags/usb-ci-v4.20-rc1 Now merged, thanks. greg k-h
Re: Query on usb/core/devio.c
On Mon, 8 Oct 2018, Oliver Neukum wrote: > On Mo, 2018-10-08 at 10:50 +0100, Mayuresh Kulkarni wrote: > > > > As a result of this, the USB suspend (L2) does not seem to happen, even if > > all the interface drivers of a composite USB device report "idle" to USB > > core driver. The USB suspend seem to happen only when the caller in > > user-space (in our case) closes the device file. > > > > Is this correct understanding? > > Yes, it is. > > > If yes, could you please help understand - > > 1. Any specific reason to choose this design approach? Apologies, but "git > > blame" does not reveal much information (or maybe I did not do git blame on > > correct kernel version). > > We cannot assume that a device is done executing a command as soon as > communication is done. Think of a scanner moving its sensor bar > or a printer printing a page. Or a display displaying something. > > > 2. Is it possible to modify this driver to take PM ref-count on USB device, > > only when user-space is actively interacting with the USB device (so in > > open/close and appropriate ioctl calls, with special handling for async URB > > submission)? > > Technically this is possible, but it is a bad idea. > > > 3. Will (2) break any known existing device(s)? > > Yes, it would and that makes it a bad idea. In theory we could add ioctls to perform the runtime PM put and get operations. Alan Stern
Re: Query on usb/core/devio.c
On Mo, 2018-10-08 at 11:16 -0400, Alan Stern wrote: > On Mon, 8 Oct 2018, Oliver Neukum wrote: > > > On Mo, 2018-10-08 at 10:50 +0100, Mayuresh Kulkarni wrote: > > > > > > As a result of this, the USB suspend (L2) does not seem to happen, even > > > if all the interface drivers of a composite USB device report "idle" to > > > USB core driver. The USB suspend seem to happen only when the caller in > > > user-space (in our case) closes the device file. > > > > > > Is this correct understanding? > > > > Yes, it is. > > > > > If yes, could you please help understand - > > > 1. Any specific reason to choose this design approach? Apologies, but > > > "git blame" does not reveal much information (or maybe I did not do git > > > blame on correct kernel version). > > > > We cannot assume that a device is done executing a command as soon as > > communication is done. Think of a scanner moving its sensor bar > > or a printer printing a page. Or a display displaying something. > > > > > 2. Is it possible to modify this driver to take PM ref-count on USB > > > device, only when user-space is actively interacting with the USB device > > > (so in open/close and appropriate ioctl calls, with special handling for > > > async URB submission)? > > > > Technically this is possible, but it is a bad idea. > > > > > 3. Will (2) break any known existing device(s)? > > > > Yes, it would and that makes it a bad idea. > > In theory we could add ioctls to perform the runtime PM put and get > operations. Hi, we had this discussion years ago. Then the majority view was that an application should close the device. Do we have a reason to revisit that decision? Regards Oliver
Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
Hi Andrew, > Nice patch set, well broken up, easy to review. > Thanks a lot for your detailed review, your comments are really useful! I'll respond to some of your comments separately. Regards, Igor
RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
From: David Miller > Sent: 05 October 2018 22:24 > > From: Ben Dooks > Date: Tue, 2 Oct 2018 17:56:02 +0100 > > > - memcpy(skb->data, &tx_cmd_a, 4); > > + ptr = skb_push(skb, 8); > > + tx_cmd_a = cpu_to_le32(tx_cmd_a); > > + tx_cmd_b = cpu_to_le32(tx_cmd_b); > > + memcpy(ptr, &tx_cmd_a, 4); > > + memcpy(ptr+4, &tx_cmd_b, 4); > > Even a memcpy() through a void pointer does not guarantee that gcc will > not emit word sized loads and stores. True, but only if gcc can 'see' something that would require the pointer be aligned. In this case the void pointer comes from an external function so is fine. > You must use the get_unaligned()/put_unaligned() facilities to do this > properly. > > I also agree that making a proper type and structure instead of using > a void pointer would be better. The structure would need to be marked 'packed' - since its alignment isn't guaranteed. Then you don't need to use put_unaligned(). If it wasn't 'packed' then gcc would implement memcpy(&hdr->tx_cmd_a, &tx_cmd_a, 4) using an aligned write. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
Hi Andrew, >> >> +struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; > > Having to do this cast all the time is quiet ugly. It seems like some > other usb_net drivers use netdev_priv(). As I see most of usb usbnet based devices use the same theme with accessing private data via dev->data. netdev_priv() is used to store struct usbnet itself. >> +u8 dpa; /*direct PHY access*/ >> +struct aqc111_phy_options phy_ops; >> +} __packed; > > Why pack this? Do you send it to the firmware? Agreed, no. We have to pack phy_ops and wol_config only. Regards, Igor
Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
Hi Andrew, >> aqc111_read_fw_version(dev, aqc111_data); >> +aqc111_data->autoneg = AUTONEG_ENABLE; >> +aqc111_data->advertised_speed = (usb_speed == USB_SPEED_SUPER) ? >> + SPEED_5000 : SPEED_1000; > > USB 3 has a raw bandwidth of 5Gbps. But it is a shared bus. So you > have no guaranteed you are actually going to get the needed bandwidth > to support line rate. > > USB 2.0 only gives you 480Mbps. So it won't even give you the full > 1G. So using the same reasoning for USB3, maybe you should limit it to > 100Mbps? > > I personally would not apply restrictions on the PHY depending on what > USB is being used. First argument here is to reduce power consumption on USB2. 2.5G/5G uses OCSGMII/XFI serdes which consumes more power. Of course in normal conditions usb2 is capable to feed that, but the risk still exists on legacy usb2 hardware. > This becomes more important when using SFPs. If i have an SFP peer > which is expecting 2500Base-X, but because the device is plugged into > USB 2 port it is forced to use 1000Base-X, it is not going to get > link. Do you mean here 2500Base-T? This particular device is an integrated mac+phy, thus we can't easily link it with -X SFP endpoint. Although its not a common usecase for the consumer dongle to connect to SFP endpoints, think your comment is quite reasonable. We'll clarify this internally. Regards, Igor
Re: [PATCH] usb: typec: Fix copy/paste on typec_set_vconn_role() kerneldoc
On Sun, Oct 07, 2018 at 04:46:12PM -0700, Stephen Boyd wrote: > This must have been copy pasted from the function above. Fix it. > > Signed-off-by: Stephen Boyd Acked-by: Heikki Krogerus > --- > drivers/usb/typec/class.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 00141e05bc72..5db0593ca0bd 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -1322,7 +1322,7 @@ void typec_set_pwr_role(struct typec_port *port, enum > typec_role role) > EXPORT_SYMBOL_GPL(typec_set_pwr_role); > > /** > - * typec_set_pwr_role - Report VCONN source change > + * typec_set_vconn_role - Report VCONN source change > * @port: The USB Type-C Port which VCONN role changed > * @role: Source when @port is sourcing VCONN, or Sink when it's not > * > -- > Sent by a computer through tubes Thanks, -- heikki
[PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
A spanking new machine I just got has all but one USB ports wired as 3.0. Booting defconfig resulted in no keyboard or mouse, which was pretty uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. As 'y' not 'm' -- recovering from initrd problems needs a keyboard. Signed-off-by: Adam Borowski --- arch/x86/configs/x86_64_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/configs/x86_64_defconfig b/arch/x86/configs/x86_64_defconfig index e32fc1f274d8..ac9ae487cfeb 100644 --- a/arch/x86/configs/x86_64_defconfig +++ b/arch/x86/configs/x86_64_defconfig @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y CONFIG_USB=y CONFIG_USB_ANNOUNCE_NEW_DEVICES=y CONFIG_USB_MON=y +CONFIG_USB_XHCI_HCD=y CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_TT_NEWSCHED=y CONFIG_USB_OHCI_HCD=y -- 2.19.0
Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
On Mon, Oct 08, 2018 at 09:29:26AM +, Igor Russkikh wrote: > Hi Andrew, > > >>aqc111_read_fw_version(dev, aqc111_data); > >> + aqc111_data->autoneg = AUTONEG_ENABLE; > >> + aqc111_data->advertised_speed = (usb_speed == USB_SPEED_SUPER) ? > >> + SPEED_5000 : SPEED_1000; > > > > > USB 3 has a raw bandwidth of 5Gbps. But it is a shared bus. So you > > have no guaranteed you are actually going to get the needed bandwidth > > to support line rate. > > > > USB 2.0 only gives you 480Mbps. So it won't even give you the full > > 1G. So using the same reasoning for USB3, maybe you should limit it to > > 100Mbps? > > > > I personally would not apply restrictions on the PHY depending on what > > USB is being used. > > First argument here is to reduce power consumption on USB2. > 2.5G/5G uses OCSGMII/XFI serdes which consumes more power. > Of course in normal conditions usb2 is capable to feed that, but > the risk still exists on legacy usb2 hardware. O.K, that sounds like a sensible argument. Please add a comment. I hope the Marketing Department also understand this. It should probably explain this on the product packaging. > > This becomes more important when using SFPs. If i have an SFP peer > > which is expecting 2500Base-X, but because the device is plugged into > > USB 2 port it is forced to use 1000Base-X, it is not going to get > > link. > > Do you mean here 2500Base-T? This particular device is an integrated > mac+phy, thus we can't easily link it with -X SFP endpoint. I only went to find the product brief after finishing the review. Without an external SERDES interface, SFP is not possible. But from your comment above, i does sound like internally it has such a SERDES. So it is not out of the question a follow up device is produced which could connect to an SFP. I actually have a no-name USB based SFP dongle... Andrew
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
On Mon, Oct 08, 2018 at 09:09:54AM +, Igor Russkikh wrote: > Hi Andrew, > > >> > >> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; > > > > Having to do this cast all the time is quiet ugly. It seems like some > > other usb_net drivers use netdev_priv(). > > As I see most of usb usbnet based devices use the same theme with accessing > private data via dev->data. It is just ugly. It would of been better if dev->data[] was a void pointer. This is the first usbnet driver i've reviewed, so i don't know the history behind this. I wonder if adding a void *priv would be accepted? Andrew
[PATCH v2] usb: typec: tcpm: Report back negotiated PPS voltage and current
Currently when requesting a specific voltage or current through the psy interface, for PPS, when reading back from that interface the values will always be the same as previously given, if the request was successful. However PPS only allows for 20mV voltage steps and 50mA current steps, and the psy class expects microvolt and micro amp requests, so inbetween values can be provided through this interface. Really when reading back the true values negotiated should be given, and not the ones originally asked for. To report the actual values negotiated with the Source, the values stored are now rounded down to the relevant step units prior to building the PPS request, so that those values are later correctly reported through the psy interface. In addition this improves the adjustments made to meet the operating power requirements of the platform, which previously could have been slightly out due to not using valid PPS units of voltage and current. Signed-off-by: Adam Thomson --- Changes are based on usb-testing (e7a2c3fa2857) Changes in v2: - Rounding down of PPS voltage and current values moved to relevant API functions so stored values are correctly updated prior to all subsequent usage and decision making, as discuessed with Guenter. drivers/usb/typec/tcpm/tcpm.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index b06eac8..dbbd71f 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4113,6 +4113,9 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr) goto port_unlock; } + /* Round down operating current to align with PPS valid steps */ + op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP); + reinit_completion(&port->pps_complete); port->pps_data.op_curr = op_curr; port->pps_status = 0; @@ -4166,6 +4169,9 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt) goto port_unlock; } + /* Round down output voltage to align with PPS valid steps */ + out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP); + reinit_completion(&port->pps_complete); port->pps_data.out_volt = out_volt; port->pps_status = 0; -- 1.9.1
Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
Hi Andrew, > Hi Igor, Dmitry > > Please could you explain why you decided to not use drivers/net/phy? > The previous patch introduced basically what you need to export a > standard Linux MDIO bus. From that you can use a standard Linux PHY > driver. Thats again because of this product has tightly integrated MAC+Phy. MAC FW controls system interface and reports/alters link state as a joint state on copper and SIF (even in dpa direct phy mode). We can't extract phy api into a standalone fully functional phylib therefore. Also as far as I know this particular phy is not available in the wild. Regards, Igor
Re: [PATCH v2] usb: typec: tcpm: Report back negotiated PPS voltage and current
On 10/08/2018 05:53 AM, Adam Thomson wrote: Currently when requesting a specific voltage or current through the psy interface, for PPS, when reading back from that interface the values will always be the same as previously given, if the request was successful. However PPS only allows for 20mV voltage steps and 50mA current steps, and the psy class expects microvolt and micro amp requests, so inbetween values can be provided through this interface. Really when reading back the true values negotiated should be given, and not the ones originally asked for. To report the actual values negotiated with the Source, the values stored are now rounded down to the relevant step units prior to building the PPS request, so that those values are later correctly reported through the psy interface. In addition this improves the adjustments made to meet the operating power requirements of the platform, which previously could have been slightly out due to not using valid PPS units of voltage and current. Signed-off-by: Adam Thomson Reviewed-by: Guenter Roeck --- Changes are based on usb-testing (e7a2c3fa2857) Changes in v2: - Rounding down of PPS voltage and current values moved to relevant API functions so stored values are correctly updated prior to all subsequent usage and decision making, as discuessed with Guenter. drivers/usb/typec/tcpm/tcpm.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index b06eac8..dbbd71f 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4113,6 +4113,9 @@ static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr) goto port_unlock; } + /* Round down operating current to align with PPS valid steps */ + op_curr = op_curr - (op_curr % RDO_PROG_CURR_MA_STEP); + reinit_completion(&port->pps_complete); port->pps_data.op_curr = op_curr; port->pps_status = 0; @@ -4166,6 +4169,9 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt) goto port_unlock; } + /* Round down output voltage to align with PPS valid steps */ + out_volt = out_volt - (out_volt % RDO_PROG_VOLT_MV_STEP); + reinit_completion(&port->pps_complete); port->pps_data.out_volt = out_volt; port->pps_status = 0;
[RFC] usb: chipidea: Add minimal support for HSIC interface on i.MX6QDL
Current mainline doesn't support USB hosts 2 and 3 which only use
HSIC mode and I was wondering how this would need to be implemented.
The topic has been discussed before: [1]
And there is some implementation in the vendor kernel: [2]
It seems like two things need to be done:
1. Switch the pinmux of the strobe signal to use a pullup after
the core has been initialized.
2. Enable HSIC mode and HSIC clock
This patch only implements these basics in a minimal approach.
You need to have an additional pinmux setting "active" in the dt,
that sets the pullup.
It was tested with the SMSC LAN9730 USB Ethernet adapter on the
iMXceet Solo S board.
[1] https://patchwork.kernel.org/patch/3541771/
[2]
http://git.freescale.com/git/cgit.cgi/imx/linux-imx.git/commit/?id=cf2d3ff6b217ef41f0e594daa9615e2
Signed-off-by: Frieder Schrempf
---
drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++
drivers/usb/chipidea/usbmisc_imx.c | 22 ++
2 files changed, 32 insertions(+)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 19f5f5f..fef8bda 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -256,6 +256,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
const struct of_device_id *of_id;
const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
struct device_node *np = pdev->dev.of_node;
+ struct pinctrl *pinctrl;
of_id = of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
if (!of_id)
@@ -331,6 +332,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
}
+ pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (!IS_ERR(pinctrl)) {
+ struct pinctrl_state *state;
+
+ state = pinctrl_lookup_state(pinctrl, "active");
+ if (!IS_ERR(state))
+ pinctrl_select_state(pinctrl, state);
+ }
+
device_set_wakeup_capable(&pdev->dev, true);
return 0;
diff --git a/drivers/usb/chipidea/usbmisc_imx.c
b/drivers/usb/chipidea/usbmisc_imx.c
index 34ad5bf..a6556c8 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -64,10 +64,16 @@
#define MX6_BM_OVER_CUR_DISBIT(7)
#define MX6_BM_OVER_CUR_POLARITY BIT(8)
#define MX6_BM_WAKEUP_ENABLE BIT(10)
+#define MX6_BM_UTMI_ON_CLOCK BIT(13)
#define MX6_BM_ID_WAKEUP BIT(16)
#define MX6_BM_VBUS_WAKEUP BIT(17)
#define MX6SX_BM_DPDM_WAKEUP_ENBIT(29)
#define MX6_BM_WAKEUP_INTR BIT(31)
+
+#define MX6_USB_HSIC_CTRL_OFFSET 0x10
+#define MX6_BM_HSIC_CLK_ON BIT(11)
+#define MX6_BM_HSIC_EN BIT(12)
+
#define MX6_USB_OTG1_PHY_CTRL 0x18
/* For imx6dql, it is host-only controller, for later imx6, it is otg's */
#define MX6_USB_OTG2_PHY_CTRL 0x1c
@@ -351,6 +357,22 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
*data)
writel(reg | MX6_BM_NON_BURST_SETTING,
usbmisc->base + data->index * 4);
+ /*
+* Core 2 and 3 are host only and HSIC only,
+* so we enable HSIC by default to make them usable
+*/
+ if (data->index == 2 || data->index == 3) {
+ reg = readl(usbmisc->base + data->index * 4);
+ writel(reg | MX6_BM_UTMI_ON_CLOCK,
+ usbmisc->base + data->index * 4);
+
+ reg = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET +
+ (data->index - 2) * 4);
+ reg |= MX6_BM_HSIC_EN | MX6_BM_HSIC_CLK_ON;
+ writel(reg, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET +
+ (data->index - 2) * 4);
+ }
+
spin_unlock_irqrestore(&usbmisc->lock, flags);
usbmisc_imx6q_set_wakeup(data, false);
--
2.7.4
Re: [PATCH net-next 08/19] net: usb: aqc111: Implement TX data path
>> +skb_push(skb, AQ_TX_HEADER_SIZE); >> +cpu_to_le64s(&tx_hdr); > > Is that portable? tx_hdr is a structure of 2x u32 bitfields. What > endian have you tested that one? > You are right, this is wrong for BE hardware. We don't have such a hardware to check unfortunately. Think its better to drop endianess conversions and declare the driver as little endian only. Do you think that'll be acceptable? Regards, Igor
Re: [PATCH net-next 14/19] net: usb: aqc111: Implement set_rx_mode callback
Hi Andrew,
>> +{
>> +struct usbnet *dev = netdev_priv(net);
>> +struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
>
>> +u8 *m_filter = ((u8 *)dev->data) + 12;
>
> Please could you explain is.
Oh, that was a legacy code, the idea is it used spare area in dev->data array to
keep permanently 8 bytes for mcast filter request.
But the truth is usbnet_write anyway preallocates the data buffer and copies
the data in there.
Therefore its really better to just allocate m_filter on stack to make the code
clean.
Regards,
Igor
Re: [PATCH net-next 03/19] net: usb: aqc111: Add implementation of read and write commands
On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> Read/write command register defines and functions
>
> Signed-off-by: Dmitry Bezrukov
> Signed-off-by: Igor Russkikh
> ---
> drivers/net/usb/aqc111.c | 124
> +++
> drivers/net/usb/aqc111.h | 19
> 2 files changed, 143 insertions(+)
> create mode 100644 drivers/net/usb/aqc111.h
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index c914e19387f2..7f3e5a615750 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -14,6 +14,130 @@
> #include
> #include
>
> +#include "aqc111.h"
> +
> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> + u16 index, u16 size, void *data, int nopm)
> +{
> + int ret;
> + int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> + u16 index, void *data, u16 size);
> +
> + if (nopm)
> + fn = usbnet_read_cmd_nopm;
> + else
> + fn = usbnet_read_cmd;
If you really want to do this, pass the function.
> +
> + ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + value, index, data, size);
> + if (size == 2)
> + le16_to_cpus(data);
That is incredibly dirty
> +
> + if (unlikely(ret < 0))
> + netdev_warn(dev->net,
> + "Failed to read(0x%x) reg index 0x%04x: %d\n",
> + cmd, index, ret);
> + return ret;
> +}
> +
> +static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> + u16 index, u16 size, void *data)
> +{
> + return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
> +}
> +
> +static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +u16 index, u16 size, void *data)
> +{
> + return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
> +}
> +
> +static int __aq_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
> + u16 value, u16 index, const void *data, u16 size)
> +{
> + void *buf = NULL;
> + int err = -ENOMEM;
> +
> + netdev_dbg(dev->net,
> +"%s cmd=%#x reqtype=%#x value=%#x index=%#x size=%d\n",
> +__func__, cmd, reqtype, value, index, size);
> +
> + if (data) {
> + buf = kmemdup(data, size, GFP_KERNEL);
Under which contexts is this used?
Regards
Oliver
Re: [PATCH net-next 04/19] net: usb: aqc111: Various callbacks implementation
On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> Reset, stop callbacks, driver unbind callback.
> More register defines required for these callbacks.
>
> Signed-off-by: Dmitry Bezrukov
> Signed-off-by: Igor Russkikh
> ---
> drivers/net/usb/aqc111.c | 48 ++
> drivers/net/usb/aqc111.h | 101
> +++
> 2 files changed, 149 insertions(+)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 7f3e5a615750..22bb259d71fb 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -169,12 +169,60 @@ static int aqc111_bind(struct usbnet *dev, struct
> usb_interface *intf)
>
> static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
> {
> + u8 reg8;
> + u16 reg16;
> +
> + /* Force bz */
> + reg16 = SFR_PHYPWR_RSTCTL_BZ;
> + aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL,
> + 2, 2, ®16);
No, I am sorry, you are doing DMA on the kernel stack. That is not
allowed. These functions will all have to be fixed.
Regards
Oliver
Re: [PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
On Mon, Oct 08, 2018 at 12:32:23PM +0200, Adam Borowski wrote: > A spanking new machine I just got has all but one USB ports wired as 3.0. > Booting defconfig resulted in no keyboard or mouse, which was pretty > uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. > As 'y' not 'm' -- recovering from initrd problems needs a keyboard. > > Signed-off-by: Adam Borowski > --- > arch/x86/configs/x86_64_defconfig | 1 + > 1 file changed, 1 insertion(+) defconfig for x86 actually still works? That's amazing in itself :) > diff --git a/arch/x86/configs/x86_64_defconfig > b/arch/x86/configs/x86_64_defconfig > index e32fc1f274d8..ac9ae487cfeb 100644 > --- a/arch/x86/configs/x86_64_defconfig > +++ b/arch/x86/configs/x86_64_defconfig > @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y > CONFIG_USB=y > CONFIG_USB_ANNOUNCE_NEW_DEVICES=y > CONFIG_USB_MON=y > +CONFIG_USB_XHCI_HCD=y > CONFIG_USB_EHCI_HCD=y > CONFIG_USB_EHCI_TT_NEWSCHED=y > CONFIG_USB_OHCI_HCD=y You can probably turn ehci and ohci off if you have xhci in the "default" system now. thanks, greg k-h
Re: [PATCH 01/14] phy: phy-pxa-usb: add a new driver
On Sun, Oct 07, 2018 at 08:47:28PM +0200, Lubomir Rintel wrote: > On Tue, 2018-09-25 at 10:53 +0530, Kishon Vijay Abraham I wrote: > > > > On Thursday 23 August 2018 02:12 AM, Lubomir Rintel wrote: > > > Turned from arch/arm/mach-mmp/devices.c into a proper PHY driver, > > > so > > > that in can be instantiated from a DT. > > > > > > Signed-off-by: Lubomir Rintel > > > > Acked-by: Kishon Vijay Abraham I > > > > If this has to be merged via linux-phy tree, please let me know. > > Yes, either linux-phy or the usb tree. > > The EHCI patches have already been pulled into the usb tree, presumably > because they got an Ack from Alan Stern. That includes "USB: EHCI: > ehci-mv: use phy-pxa-usb" that depends on this. Perhaps the rest of > the patches can go via the same tree? > > I haven't submitted a patchset that would have dependencies spanning > across different subsystems before. I don't know what's usually done in > such cases. Advice welcome. > > Greg? You can either send them through the same tree, or wait a release cycle and get the rest in through a different one. As I usually take the phy tree into the USB tree for issues like this, I can take them all if you get the phy maintainer to ack them. thanks, greg k-h
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> Implement PHY power up/down sequences.
> AQC111, depending on FW used, may has PHY being controlled either
> directly (dpa = 1) or via vendor command interface (dpa = 0).
> Drivers supports both themes.
> We determine this from firmware versioning agreement.
>
> Signed-off-by: Dmitry Bezrukov
> Signed-off-by: Igor Russkikh
> ---
> drivers/net/usb/aqc111.c | 93
>
> drivers/net/usb/aqc111.h | 70
> 2 files changed, 163 insertions(+)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 22bb259d71fb..30219bb6ddfd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -138,15 +138,44 @@ static int aqc111_write_cmd(struct usbnet *dev, u8 cmd,
> u16 value,
> return __aqc111_write_cmd(dev, cmd, value, index, size, data, 0);
> }
>
> +static int aq_mdio_read_cmd(struct usbnet *dev, u16 value, u16 index,
> + u16 size, void *data)
> +{
> + return aqc111_read_cmd(dev, AQ_PHY_CMD, value, index, size, data);
> +}
> +
> +static int aq_mdio_write_cmd(struct usbnet *dev, u16 value, u16 index,
> + u16 size, void *data)
> +{
> + return aqc111_write_cmd(dev, AQ_PHY_CMD, value, index, size, data);
> +}
> +
> static const struct net_device_ops aqc111_netdev_ops = {
> .ndo_open = usbnet_open,
> .ndo_stop = usbnet_stop,
> };
>
> +static void aqc111_read_fw_version(struct usbnet *dev,
> +struct aqc111_data *aqc111_data)
> +{
> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR,
> + 1, 1, &aqc111_data->fw_ver.major);
> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR,
> + 1, 1, &aqc111_data->fw_ver.minor);
> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV,
> + 1, 1, &aqc111_data->fw_ver.rev);
Why read the stuff you don't need?
> +
> + if (aqc111_data->fw_ver.major & 0x80)
> + aqc111_data->fw_ver.major &= ~0x80;
> + else
> + aqc111_data->dpa = 1;
> +}
> +
> static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
> {
> int ret;
> struct usb_device *udev = interface_to_usbdev(intf);
> + struct aqc111_data *aqc111_data;
>
> /* Check if vendor configuration */
> if (udev->actconfig->desc.bConfigurationValue != 1) {
> @@ -162,8 +191,18 @@ static int aqc111_bind(struct usbnet *dev, struct
> usb_interface *intf)
> return ret;
> }
>
> + aqc111_data = kzalloc(sizeof(*aqc111_data), GFP_KERNEL);
> + if (!aqc111_data)
> + return -ENOMEM;
> +
> + /* store aqc111_data pointer in device data field */
> + dev->data[0] = (unsigned long)aqc111_data;
> + memset(aqc111_data, 0, sizeof(*aqc111_data));
Either kzalloc() or a memset. Not both.
> +
> dev->net->netdev_ops = &aqc111_netdev_ops;
>
> + aqc111_read_fw_version(dev, aqc111_data);
> +
> return 0;
> }
Regards
Oliver
Re: [PATCH net-next 06/19] net: usb: aqc111: Introduce link management
On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> +static void aqc111_configure_rx(struct usbnet *dev,
> + struct aqc111_data *aqc111_data)
> +{
> + u8 reg8 = 0;
> + u8 queue_num = 0;
> + u16 reg16 = 0;
> + u16 link_speed = 0, usb_host = 0;
> + u8 buf[5] = { 0 };
DMA on stack.
> + enum usb_device_speed usb_speed = dev->udev->speed;
> +
> + buf[0] = 0x00;
> + buf[1] = 0xF8;
> + buf[2] = 0x07;
> + switch (aqc111_data->link_speed) {
> + case AQ_INT_SPEED_5G:
> + {
> + link_speed = 5000;
> + reg8 = 0x05;
> + reg16 = 0x001F;
> + break;
> + }
> + case AQ_INT_SPEED_2_5G:
> + {
> + link_speed = 2500;
> + reg16 = 0x003F;
> + break;
> + }
> + case AQ_INT_SPEED_1G:
> + {
> + link_speed = 1000;
> + reg16 = 0x009F;
> + break;
> + }
> + case AQ_INT_SPEED_100M:
> + {
> + link_speed = 100;
> + queue_num = 1;
> + reg16 = 0x063F;
> + buf[1] = 0xFB;
> + buf[2] = 0x4;
> + break;
> + }
> + }
> +
> + if (aqc111_data->dpa) {
> + /* Set Phy Flow control */
> + aq_mdio_write_cmd(dev, AQ_GLB_ING_PAUSE_CTRL_REG,
> + AQ_PHY_AUTONEG_ADDR, 2, ®16);
> + aq_mdio_write_cmd(dev, AQ_GLB_EGR_PAUSE_CTRL_REG,
> + AQ_PHY_AUTONEG_ADDR, 2, ®16);
> + }
> +
> + aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_INTER_PACKET_GAP_0,
> + 1, 1, ®8);
> +
> + aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_TX_PAUSE_RESEND_T, 3, 3, buf);
> +
> + switch (usb_speed) {
Needs to handle all speeds. And please add a comment explaining the
rationale.
> + case USB_SPEED_SUPER:
> + {
> + usb_host = 3;
> + break;
> + }
> + case USB_SPEED_HIGH:
> + {
> + usb_host = 2;
> + break;
> + }
> + case USB_SPEED_FULL:
> + case USB_SPEED_LOW:
> + {
> + usb_host = 1;
> + queue_num = 0;
> + break;
> + }
> + default:
> + {
> + usb_host = 0;
> + break;
> + }
> + }
> +
> + memcpy(buf, &AQC111_BULKIN_SIZE[queue_num], 5);
> + /* RX bulk configuration */
> + aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_RX_BULKIN_QCTRL, 5, 5, buf);
> +
> + /* Set high low water level */
> + reg16 = 0x0810;
> +
> + aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_PAUSE_WATERLVL_LOW,
> + 2, 2, ®16);
> + netdev_info(dev->net, "Link Speed %d, USB %d", link_speed, usb_host);
> +}
Regards
Oliver
Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet
Hi Andrew,
>> +if (aqc111_data->dpa) {
>> +aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
>
> I don't think that works. You should leave AUTONEG on, but only
> advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> there is no guarantee the peer will figure out what the new link speed
> is. I've often seen failed auto-net result in 10/Half. So you will
> loose the link, making WoL pointless.
Phy does not support 10M, low power mode explicitly uses 100M
for power safety reasons.
It is meaningless here to add Autoneg to 100M because thats the only
speedmask bit anyway.
>> +aqc111_set_phy_speed(dev, aqc111_data->autoneg,
>> + aqc111_data->advertised_speed);
>> +
>
> Should that be conditional on aqc111_data->dpa?
Actually no, because set_phy_speed internally checks this flag.
>> +u8 rsvd[283];
>> +};
>
> Do you really need these 283 bytes??
>> struct aqc111_phy_options phy_ops;
>> +struct aqc111_wol_cfg wol_cfg;
>
> Those 283 bytes make this whole structure bigger...
FW interface expects the WOL config request WOL_CFG_SIZE bytes.
These reserved fields are just not used now by linux driver.
They configure extra wol features like a sleep proxy.
Thus, we anyway have to allocate this somewhere.
Regards,
Igor
Re: [PATCH net-next 08/19] net: usb: aqc111: Implement TX data path
On Mo, 2018-10-08 at 13:43 +, Igor Russkikh wrote: > > > + skb_push(skb, AQ_TX_HEADER_SIZE); > > > + cpu_to_le64s(&tx_hdr); > > > > Is that portable? tx_hdr is a structure of 2x u32 bitfields. What > > endian have you tested that one? > > > > You are right, this is wrong for BE hardware. > > We don't have such a hardware to check unfortunately. > Think its better to drop endianess conversions and declare > the driver as little endian only. > > Do you think that'll be acceptable? No. If worse comes to worse define it u64 and set the values manually. Regards Oliver
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
Hi Andrew, + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; >>> >>> Having to do this cast all the time is quiet ugly. It seems like some >>> other usb_net drivers use netdev_priv(). >> >> As I see most of usb usbnet based devices use the same theme with accessing >> private data via dev->data. > > It is just ugly. It would of been better if dev->data[] was a void > pointer. This is the first usbnet driver i've reviewed, so i don't > know the history behind this. I wonder if adding a void *priv would be > accepted? I can't comment on history of this, but... net-next/drivers/net/usb$ grep "dev->data" * | wc -l 173 Regards, Igor
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
Hi Oliver, >> +aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, >> +1, 1, &aqc111_data->fw_ver.major); >> +aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, >> +1, 1, &aqc111_data->fw_ver.minor); >> +aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, >> +1, 1, &aqc111_data->fw_ver.rev); > > Why read the stuff you don't need? fw_ver is used below to determine phy access mode. fw_ver.rev is not used in this exact patch, thats true, but it gets reported in later patches in the set. Regards, Igor
Re: [PATCH net-next 13/19] net: usb: aqc111: Add support for TSO
On Fr, 2018-10-05 at 10:25 +, Igor Russkikh wrote: > From: Dmitry Bezrukov > > Signed-off-by: Dmitry Bezrukov > Signed-off-by: Igor Russkikh > --- > drivers/net/usb/aqc111.c | 3 +++ > drivers/net/usb/aqc111.h | 6 -- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c > index 6efd9a9ad44e..f61fa7446b72 100644 > --- a/drivers/net/usb/aqc111.c > +++ b/drivers/net/usb/aqc111.c > @@ -964,6 +964,9 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet > *dev, struct sk_buff *skb, > /*Length of actual data*/ > tx_hdr.length = (skb->len & 0x1F); > > + /* TSO MSS */ > + tx_hdr.max_seg_size = skb_shinfo(skb)->gso_size; Endianness > + > headroom = (skb->len + AQ_TX_HEADER_SIZE) % 8; > if (headroom != 0) > padding_size = 8 - headroom; Regards Oliver
Re: [PATCH net-next 15/19] net: usb: aqc111: Add support for VLAN_CTAG_TX/RX offload
On Fr, 2018-10-05 at 10:25 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> Signed-off-by: Dmitry Bezrukov
> Signed-off-by: Igor Russkikh
> ---
> drivers/net/usb/aqc111.c | 14 ++
> drivers/net/usb/aqc111.h | 7 ++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index cc23c39beab3..a9051dd7c5bd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -524,6 +524,7 @@ static int aqc111_bind(struct usbnet *dev, struct
> usb_interface *intf)
>
> dev->net->hw_features |= AQ_SUPPORT_HW_FEATURE;
> dev->net->features |= AQ_SUPPORT_FEATURE;
> + dev->net->vlan_features |= AQ_SUPPORT_VLAN_FEATURE;
>
> aqc111_read_fw_version(dev, aqc111_data);
> aqc111_data->autoneg = AUTONEG_ENABLE;
> @@ -817,6 +818,7 @@ static int aqc111_reset(struct usbnet *dev)
>
> dev->net->hw_features |= AQ_SUPPORT_HW_FEATURE;
> dev->net->features |= AQ_SUPPORT_FEATURE;
> + dev->net->vlan_features |= AQ_SUPPORT_VLAN_FEATURE;
>
> /* Power up ethernet PHY */
> aqc111_data->phy_ops.advertising = 0;
> @@ -992,6 +994,11 @@ static int aqc111_rx_fixup(struct usbnet *dev, struct
> sk_buff *skb)
> new_skb->truesize = new_skb->len + sizeof(struct sk_buff);
> if (aqc111_data->rx_checksum)
> aqc111_rx_checksum(new_skb, &pkt_desc);
> + if (pkt_desc->vlan_ind)
> + __vlan_hwaccel_put_tag(new_skb,
> +htons(ETH_P_8021Q),
> +pkt_desc->vlan_tag &
> +VLAN_VID_MASK);
>
> usbnet_skb_return(dev, new_skb);
> if (pkt_count == 0)
> @@ -1020,6 +1027,7 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet
> *dev, struct sk_buff *skb,
> int tailroom = 0;
> int padding_size = 0;
> struct sk_buff *new_skb = NULL;
> + u16 tci = 0;
>
> memset(&tx_hdr, 0x00, sizeof(tx_hdr));
>
> @@ -1038,6 +1046,12 @@ static struct sk_buff *aqc111_tx_fixup(struct usbnet
> *dev, struct sk_buff *skb,
> tx_hdr.drop_padding = 1;
> }
>
> + /* Vlan Tag */
> + if (vlan_get_tag(skb, &tci) >= 0) {
> + tx_hdr.vlan_tag = 1;
> + tx_hdr.vlan_info = tci;
Endianness
Regards
Oliver
Re: [PATCH net-next 18/19] net: usb: aqc111: Implement get/set_link_ksettings callbacks
On Fr, 2018-10-05 at 10:25 +, Igor Russkikh wrote:
> From: Dmitry Bezrukov
>
> +static int aqc111_get_link_ksettings(struct net_device *net,
> + struct ethtool_link_ksettings *elk)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + enum usb_device_speed usb_speed = dev->udev->speed;
> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];
> + u32 speed = SPEED_UNKNOWN;
> +
> + ethtool_link_ksettings_zero_link_mode(elk, supported);
> + ethtool_link_ksettings_add_link_mode(elk, supported,
> + 100baseT_Full);
> + ethtool_link_ksettings_add_link_mode(elk, supported,
> + 1000baseT_Full);
> + if (usb_speed == USB_SPEED_SUPER) {
And SUPER_PLUS?
Regards
Oliver
Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
On Fr, 2018-10-05 at 10:24 +, Igor Russkikh wrote: > This patchset introduces support for new multigig ethernet to USB dongle, > developed jointly by Aquantia (Phy) and ASIX (USB MAC). > > The driver has similar structure with other ASIX MAC drivers (AX88179), but > with a number of important differences: > - Driver supports both direct Phy and custom firmware interface for Phy > programming. This is due to different firmware modules available with this > product. > - Driver handles new 2.5G/5G link speed configuration and reporting. > - Device support all speeds from 100M up to 5G. > - Device supports MTU up to 16K. > > Device supports various standard networking features, like > checksum offloads, vlan tagging/filtering, TSO. > > The code of this driver is based on original ASIX sources and was extended > by Aquantia for 5G multigig support. Thank you for the driver. It is good to see drivers for cool hardware. Unfortunately there have been a few issues I have tried to point out in reviews. Please fix them and resubmit. Regards Oliver
Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access
On Mo, 2018-10-08 at 17:10 +0300, Igor Russkikh wrote: > Hi Oliver, > > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, > > > + 1, 1, &aqc111_data->fw_ver.major); > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, > > > + 1, 1, &aqc111_data->fw_ver.minor); > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, > > > + 1, 1, &aqc111_data->fw_ver.rev); > > > > Why read the stuff you don't need? > > fw_ver is used below to determine phy access mode. > > fw_ver.rev is not used in this exact patch, thats true, > but it gets reported in later patches in the set. Hi, OK that makes sense. Regards Oliver
Re: [PATCH net-next 19/19] net: usb: aqc111: Add support for wake on LAN by MAGIC packet
On Mon, Oct 08, 2018 at 02:12:59PM +, Igor Russkikh wrote:
> Hi Andrew,
>
> >> + if (aqc111_data->dpa) {
> >> + aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
> >
> > I don't think that works. You should leave AUTONEG on, but only
> > advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> > there is no guarantee the peer will figure out what the new link speed
> > is. I've often seen failed auto-net result in 10/Half. So you will
> > loose the link, making WoL pointless.
>
> Phy does not support 10M, low power mode explicitly uses 100M
> for power safety reasons.
>
> It is meaningless here to add Autoneg to 100M because thats the only
> speedmask bit anyway.
If you have AUTONEG_DISABLE, i would assume you PHY is not even trying
to auto_neg. So the speedmask is irrelevent, it is not sent to the
peer. And since the peer is not receiving any auto-neg information, it
will fail to auto-neg, and most likely default to 10/Half.
To do this right, please take a look at this commit
commit 2b9672ddb6f347467d7b33b86c5dfc4d5c0501a8
Author: Heiner Kallweit
Date: Thu Jul 12 21:32:53 2018 +0200
net: phy: add phy_speed_down and phy_speed_up
Some network drivers include functionality to speed down the PHY when
suspending and just waiting for a WoL packet because this saves energy.
This functionality is quite generic, therefore let's factor it out to
phylib.
>
> >> + aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> >> + aqc111_data->advertised_speed);
> >> +
> >
> > Should that be conditional on aqc111_data->dpa?
>
> Actually no, because set_phy_speed internally checks this flag.
So you should probably remove the check above when forcing the speed
to 100. Make the code symmetrical.
>
> >> + u8 rsvd[283];
> >> +};
> >
> > Do you really need these 283 bytes??
>
> >>struct aqc111_phy_options phy_ops;
> >> + struct aqc111_wol_cfg wol_cfg;
> >
> > Those 283 bytes make this whole structure bigger...
>
> FW interface expects the WOL config request WOL_CFG_SIZE bytes.
> These reserved fields are just not used now by linux driver.
> They configure extra wol features like a sleep proxy.
> Thus, we anyway have to allocate this somewhere.
Well, i think your low level function for actually sending a command
does a dup before sending. You don't actually send this, you send a
copy. Maybe you can pad it out then?
Andrew
Re: [PATCH net-next 00/19] Add support for Aquantia AQtion USB to 5/2.5GbE devices
On 08.10.2018 17:21, Oliver Neukum wrote: >> The code of this driver is based on original ASIX sources and was extended >> by Aquantia for 5G multigig support. > > Thank you for the driver. It is good to see drivers for cool hardware. > Unfortunately there have been a few issues I have tried to point out > in reviews. Please fix them and resubmit. > Thank you Oliver for valuable input, working on this already. Regards, Igor
Re: [PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
On Mon, 8 Oct 2018, Greg Kroah-Hartman wrote: > On Mon, Oct 08, 2018 at 12:32:23PM +0200, Adam Borowski wrote: > > A spanking new machine I just got has all but one USB ports wired as 3.0. > > Booting defconfig resulted in no keyboard or mouse, which was pretty > > uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. > > As 'y' not 'm' -- recovering from initrd problems needs a keyboard. > > > > Signed-off-by: Adam Borowski > > --- > > arch/x86/configs/x86_64_defconfig | 1 + > > 1 file changed, 1 insertion(+) > > defconfig for x86 actually still works? That's amazing in itself :) > > > diff --git a/arch/x86/configs/x86_64_defconfig > > b/arch/x86/configs/x86_64_defconfig > > index e32fc1f274d8..ac9ae487cfeb 100644 > > --- a/arch/x86/configs/x86_64_defconfig > > +++ b/arch/x86/configs/x86_64_defconfig > > @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y > > CONFIG_USB=y > > CONFIG_USB_ANNOUNCE_NEW_DEVICES=y > > CONFIG_USB_MON=y > > +CONFIG_USB_XHCI_HCD=y > > CONFIG_USB_EHCI_HCD=y > > CONFIG_USB_EHCI_TT_NEWSCHED=y > > CONFIG_USB_OHCI_HCD=y > > You can probably turn ehci and ohci off if you have xhci in the > "default" system now. Heh. My office PC has both EHCI and xHCI controllers, but it uses xHCI _only_ for SuperSpeed devices, and only on some of the ports. Not a combination I have heard of anywhere else. Enabling xHCI without EHCI on that computer would be a bad idea. :-) Alan Stern
RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
Hi Felipe, Please let us know if you have any suggestions / comments on this patch series. If you feel this patch series are okay, can we proceed with them? Thanks, Anurag Kumar Vulisha >-Original Message- >From: Anurag Kumar Vulisha [mailto:anurag.kumar.vuli...@xilinx.com] >Sent: Saturday, September 15, 2018 8:00 PM >To: ba...@kernel.org; gre...@linuxfoundation.org >Cc: v.anuragku...@gmail.com; linux-usb@vger.kernel.org; linux- >ker...@vger.kernel.org; thinh.ngu...@synopsys.com; Ajay Yugalkishore Pandey >; Anurag Kumar Vulisha >Subject: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget >driver > >These patch series fixes the broken BULK streaming support in >dwc3 gadget driver. > >Changes in v5: > 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" > >Changes in v4: > 1. Corrected the commit messgae and stream timeout description > as suggested by "Thinh Nguyen" > >Changes in v3: > 1. Added the changes suggested by "Thinh Nguyen" > >Changes in v2: > 1. Added "usb: dwc3:" in subject heading > >Anurag Kumar Vulisha (8): > usb: dwc3: Correct the logic for checking TRB full in >__dwc3_prepare_one_trb() > usb: dwc3: update stream id in depcmd > usb: dwc3: make controller clear transfer resources after complete > usb: dwc3: implement stream transfer timeout > usb: dwc3: don't issue no-op trb for stream capable endpoints > usb: dwc3: check for requests in started list for stream capable >endpoints > usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl >fields > usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints > > drivers/usb/dwc3/core.h | 7 > drivers/usb/dwc3/gadget.c | 85 ++- > > 2 files changed, 84 insertions(+), 8 deletions(-) > >-- >2.1.1
Re: [PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
On 10/8/18 8:20 AM, Alan Stern wrote: > On Mon, 8 Oct 2018, Greg Kroah-Hartman wrote: > >> On Mon, Oct 08, 2018 at 12:32:23PM +0200, Adam Borowski wrote: >>> A spanking new machine I just got has all but one USB ports wired as 3.0. >>> Booting defconfig resulted in no keyboard or mouse, which was pretty >>> uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. >>> As 'y' not 'm' -- recovering from initrd problems needs a keyboard. >>> >>> Signed-off-by: Adam Borowski >>> --- >>> arch/x86/configs/x86_64_defconfig | 1 + >>> 1 file changed, 1 insertion(+) >> >> defconfig for x86 actually still works? That's amazing in itself :) >> >>> diff --git a/arch/x86/configs/x86_64_defconfig >>> b/arch/x86/configs/x86_64_defconfig >>> index e32fc1f274d8..ac9ae487cfeb 100644 >>> --- a/arch/x86/configs/x86_64_defconfig >>> +++ b/arch/x86/configs/x86_64_defconfig >>> @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y >>> CONFIG_USB=y >>> CONFIG_USB_ANNOUNCE_NEW_DEVICES=y >>> CONFIG_USB_MON=y >>> +CONFIG_USB_XHCI_HCD=y >>> CONFIG_USB_EHCI_HCD=y >>> CONFIG_USB_EHCI_TT_NEWSCHED=y >>> CONFIG_USB_OHCI_HCD=y >> >> You can probably turn ehci and ohci off if you have xhci in the >> "default" system now. > > Heh. My office PC has both EHCI and xHCI controllers, but it uses xHCI > _only_ for SuperSpeed devices, and only on some of the ports. Not a > combination I have heard of anywhere else. Enabling xHCI without EHCI > on that computer would be a bad idea. :-) Yeah, I have some mixed HCD usages as well. -- ~Randy
Re: [PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
On Mon, Oct 08, 2018 at 03:19:44PM +0200, Greg Kroah-Hartman wrote: > On Mon, Oct 08, 2018 at 12:32:23PM +0200, Adam Borowski wrote: > > A spanking new machine I just got has all but one USB ports wired as 3.0. > > Booting defconfig resulted in no keyboard or mouse, which was pretty > > uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. > > As 'y' not 'm' -- recovering from initrd problems needs a keyboard. > > > > Signed-off-by: Adam Borowski > > --- > > arch/x86/configs/x86_64_defconfig | 1 + > > 1 file changed, 1 insertion(+) > > defconfig for x86 actually still works? That's amazing in itself :) IIRC Gentoo even suggests it in the install instructions (although I haven't looked at Gentoo in quite a while). Other distros have handcrafted approximations of allmodconfig. > > +CONFIG_USB_XHCI_HCD=y > > CONFIG_USB_EHCI_HCD=y > > CONFIG_USB_OHCI_HCD=y > > You can probably turn ehci and ohci off if you have xhci in the > "default" system now. Besides mixed wiring (as Alan and Randy mentioned), some machines without USB3 are still usable. I haven't upgraded my home machine in ages -- yet despite it being an ancient Phenom2 x6 it builds defconfig in 6m 13s while this fresh work box in 3m something. Defconfig is supposed to handle a wide range of common hardware, so keeping support for 2011 stuff would be wise. As for !x86, I see that defconfigs for non-fringe archs already enable XHCI. Meow! -- ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢰⠒⠀⣿⡁ 10 people enter a bar: 1 who understands binary, ⢿⡄⠘⠷⠚⠋⠀ 1 who doesn't, D who prefer to write it as hex, ⠈⠳⣄ and 1 who narrowly avoided an off-by-one error.
[PATCH] usbip: vhci_hcd: check port number before using
From: Sudip Mukherjee
The port number is checked and it just prints an error message but it
still continues to use the invalid port. And as a result it accesses
memory which is not its resulting in BUG report from KASAN.
Reported-by: syzbot+600b03e0cf1b73bb2...@syzkaller.appspotmail.com
Cc: stable
Signed-off-by: Sudip Mukherjee
---
drivers/usb/usbip/vhci_hcd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index d11f3f8dad40..71883aa788ac 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -334,8 +334,10 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16
typeReq, u16 wValue,
usbip_dbg_vhci_rh("typeReq %x wValue %x wIndex %x\n", typeReq, wValue,
wIndex);
- if (wIndex > VHCI_HC_PORTS)
+ if (wIndex > VHCI_HC_PORTS) {
pr_err("invalid port number %d\n", wIndex);
+ return -ENODEV;
+ }
rhport = wIndex - 1;
vhci_hcd = hcd_to_vhci_hcd(hcd);
--
2.11.0
Re: [PATCH] usbip: vhci_hcd: check port number before using
Hi Sudip, On 10/08/2018 01:19 PM, Sudip Mukherjee wrote: > From: Sudip Mukherjee > > The port number is checked and it just prints an error message but it > still continues to use the invalid port. And as a result it accesses > memory which is not its resulting in BUG report from KASAN. Yes there is an issue with out of bounds access. But this isn't the right fix. > > Reported-by: syzbot+600b03e0cf1b73bb2...@syzkaller.appspotmail.com > Cc: stable > Signed-off-by: Sudip Mukherjee I sent in a fix for this last Friday. https://patchwork.kernel.org/patch/10628833/ thanks, -- Shuah
Re: [PATCH] usbip: vhci_hcd: check port number before using
On Mon, Oct 8, 2018 at 8:29 PM Shuah Khan wrote: > > Hi Sudip, > > On 10/08/2018 01:19 PM, Sudip Mukherjee wrote: > > From: Sudip Mukherjee > > > > The port number is checked and it just prints an error message but it > > still continues to use the invalid port. And as a result it accesses > > memory which is not its resulting in BUG report from KASAN. > > Yes there is an issue with out of bounds access. But this isn't the > right fix. > > > > > Reported-by: syzbot+600b03e0cf1b73bb2...@syzkaller.appspotmail.com > > Cc: stable > > Signed-off-by: Sudip Mukherjee > > I sent in a fix for this last Friday. > > https://patchwork.kernel.org/patch/10628833/ And I can confirm this patch also fixes the issue tested with the reproducer I was using in my vm. -- Regards Sudip
Re: [PATCH] usbip: vhci_hcd: check port number before using
On 10/08/2018 02:01 PM, Sudip Mukherjee wrote: > On Mon, Oct 8, 2018 at 8:29 PM Shuah Khan wrote: >> >> Hi Sudip, >> >> On 10/08/2018 01:19 PM, Sudip Mukherjee wrote: >>> From: Sudip Mukherjee >>> >>> The port number is checked and it just prints an error message but it >>> still continues to use the invalid port. And as a result it accesses >>> memory which is not its resulting in BUG report from KASAN. >> >> Yes there is an issue with out of bounds access. But this isn't the >> right fix. >> >>> >>> Reported-by: syzbot+600b03e0cf1b73bb2...@syzkaller.appspotmail.com >>> Cc: stable >>> Signed-off-by: Sudip Mukherjee >> >> I sent in a fix for this last Friday. >> >> https://patchwork.kernel.org/patch/10628833/ > > And I can confirm this patch also fixes the issue tested with the > reproducer I was using in my vm. > > Great Thanks for testing the patch. thanks, -- Shuah
RE: [RFC] usb: chipidea: Add minimal support for HSIC interface on i.MX6QDL
>
> The topic has been discussed before: [1] And there is some implementation in
> the
> vendor kernel: [2]
>
> It seems like two things need to be done:
>
> 1. Switch the pinmux of the strobe signal to use a pullup after
>the core has been initialized.
> 2. Enable HSIC mode and HSIC clock
>
> This patch only implements these basics in a minimal approach.
> You need to have an additional pinmux setting "active" in the dt, that sets
> the pullup.
>
> It was tested with the SMSC LAN9730 USB Ethernet adapter on the iMXceet Solo S
> board.
>
Thanks, I should do it earlier, I could not find a suitable board with HSIC
support at mainline kernel.
It is very kind that you could help on it and test function at real board. My
suggestion is
follow all flows in Link [2] since we need to cover suspend/resume and remote
wakeup.
Peter
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.
> kernel.org%2Fpatch%2F3541771%2F&data=02%7C01%7CPeter.Chen%40nx
> p.com%7Cea362518eae9440028aa08d62d22246b%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636746022015840824&sdata=sHE2yZha9%2FhN
> SSBjXogDMtdflhNc2n9mNlUe9t8E1kY%3D&reserved=0
> [2]
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.freescale
> .com%2Fgit%2Fcgit.cgi%2Fimx%2Flinux-
> imx.git%2Fcommit%2F%3Fid%3Dcf2d3ff6b217ef41f0e594daa9615e2&data=0
> 2%7C01%7CPeter.Chen%40nxp.com%7Cea362518eae9440028aa08d62d22246b
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636746022015840824
> &sdata=iDRlnA9liWua7UMHBcVdUlMkBP6MIG7KUpHIAwQbDA0%3D&re
> served=0
>
> Signed-off-by: Frieder Schrempf
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 10 ++
> drivers/usb/chipidea/usbmisc_imx.c | 22 ++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 19f5f5f..fef8bda 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -256,6 +256,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
> const struct of_device_id *of_id;
> const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
> struct device_node *np = pdev->dev.of_node;
> + struct pinctrl *pinctrl;
>
> of_id = of_match_device(ci_hdrc_imx_dt_ids, &pdev->dev);
> if (!of_id)
> @@ -331,6 +332,15 @@ static int ci_hdrc_imx_probe(struct platform_device
> *pdev)
> pm_runtime_enable(&pdev->dev);
> }
>
> + pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (!IS_ERR(pinctrl)) {
> + struct pinctrl_state *state;
> +
> + state = pinctrl_lookup_state(pinctrl, "active");
> + if (!IS_ERR(state))
> + pinctrl_select_state(pinctrl, state);
> + }
> +
> device_set_wakeup_capable(&pdev->dev, true);
>
> return 0;
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> b/drivers/usb/chipidea/usbmisc_imx.c
> index 34ad5bf..a6556c8 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -64,10 +64,16 @@
> #define MX6_BM_OVER_CUR_DIS BIT(7)
> #define MX6_BM_OVER_CUR_POLARITY BIT(8)
> #define MX6_BM_WAKEUP_ENABLE BIT(10)
> +#define MX6_BM_UTMI_ON_CLOCK BIT(13)
> #define MX6_BM_ID_WAKEUP BIT(16)
> #define MX6_BM_VBUS_WAKEUP BIT(17)
> #define MX6SX_BM_DPDM_WAKEUP_EN BIT(29)
> #define MX6_BM_WAKEUP_INTR BIT(31)
> +
> +#define MX6_USB_HSIC_CTRL_OFFSET 0x10
> +#define MX6_BM_HSIC_CLK_ON BIT(11)
> +#define MX6_BM_HSIC_EN BIT(12)
> +
> #define MX6_USB_OTG1_PHY_CTRL0x18
> /* For imx6dql, it is host-only controller, for later imx6, it is otg's */
> #define MX6_USB_OTG2_PHY_CTRL0x1c
> @@ -351,6 +357,22 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
> *data)
> writel(reg | MX6_BM_NON_BURST_SETTING,
> usbmisc->base + data->index * 4);
>
> + /*
> + * Core 2 and 3 are host only and HSIC only,
> + * so we enable HSIC by default to make them usable
> + */
> + if (data->index == 2 || data->index == 3) {
> + reg = readl(usbmisc->base + data->index * 4);
> + writel(reg | MX6_BM_UTMI_ON_CLOCK,
> +usbmisc->base + data->index * 4);
> +
> + reg = readl(usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET +
> + (data->index - 2) * 4);
> + reg |= MX6_BM_HSIC_EN | MX6_BM_HSIC_CLK_ON;
> + writel(reg, usbmisc->base + MX6_USB_HSIC_CTRL_OFFSET +
> +(data->index - 2) * 4);
> + }
> +
> spin_unlock_irqrestore(&usbmisc->lock, flags);
>
> usbmisc_imx6q_set_wakeup(data, false);
> --
> 2.7.4
[PATCH] usb: roles: intel_xhci: Fix Unbalanced pm_runtime_enable
From: Wan Ahmad Zainie
Add missing pm_runtime_disable() to remove(), in order to avoid
an Unbalanced pm_runtime_enable when the module is removed and
re-probed.
Error log:
root@intel-corei7-64:~# modprobe -r intel_xhci_usb_role_switch
root@intel-corei7-64:~# modprobe intel_xhci_usb_role_switch
intel_xhci_usb_sw intel_xhci_usb_sw: Unbalanced pm_runtime_enable!
Fixes: cb2968468605 (usb: roles: intel_xhci: Enable runtime PM)
Cc:
Reviewed-by: Heikki Krogerus
Signed-off-by: Wan Ahmad Zainie
---
drivers/usb/roles/intel-xhci-usb-role-switch.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c
b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 1fb3dd0f1dfa..277de96181f9 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -161,6 +161,8 @@ static int intel_xhci_usb_remove(struct platform_device
*pdev)
{
struct intel_xhci_usb_data *data = platform_get_drvdata(pdev);
+ pm_runtime_disable(&pdev->dev);
+
usb_role_switch_unregister(data->role_sw);
return 0;
}
--
1.9.1
RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
(no top-posting, please) Hi, Anurag Kumar Vulisha writes: > Hi Felipe, > > Please let us know if you have any suggestions / comments on this patch > series. > If you feel this patch series are okay, can we proceed with them? I really don't like this dwc3-specific timer. The best way here would be to add a timer on udc/core.c which can be reused by any udc. This would mean, of course, teaching udc/core about streams and lettting it do part of the handling. Best -- balbi signature.asc Description: PGP signature
Re: [PATCH] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
* Greg Kroah-Hartman wrote: > On Mon, Oct 08, 2018 at 12:32:23PM +0200, Adam Borowski wrote: > > A spanking new machine I just got has all but one USB ports wired as 3.0. > > Booting defconfig resulted in no keyboard or mouse, which was pretty > > uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. > > As 'y' not 'm' -- recovering from initrd problems needs a keyboard. > > > > Signed-off-by: Adam Borowski > > --- > > arch/x86/configs/x86_64_defconfig | 1 + > > 1 file changed, 1 insertion(+) > > defconfig for x86 actually still works? That's amazing in itself :) Many of my test machines are able to boot the x86 defconfig bzImage to a shell, so it's more like an "Ingo's minconfig". ;-) /me tries to look innocently but fails miserably > > diff --git a/arch/x86/configs/x86_64_defconfig > > b/arch/x86/configs/x86_64_defconfig > > index e32fc1f274d8..ac9ae487cfeb 100644 > > --- a/arch/x86/configs/x86_64_defconfig > > +++ b/arch/x86/configs/x86_64_defconfig > > @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y > > CONFIG_USB=y > > CONFIG_USB_ANNOUNCE_NEW_DEVICES=y > > CONFIG_USB_MON=y > > +CONFIG_USB_XHCI_HCD=y > > CONFIG_USB_EHCI_HCD=y > > CONFIG_USB_EHCI_TT_NEWSCHED=y > > CONFIG_USB_OHCI_HCD=y > > You can probably turn ehci and ohci off if you have xhci in the > "default" system now. Also, could we please refresh this in the 32-bit defconfig as well. Other than that the patch looks good to me, no objections to adding convenience options for people who have actually booted the thing on reasonably common PC hardware. Thanks, Ingo
[PATCH v2] x86/defconfig: Enable CONFIG_USB_XHCI_HCD
A spanking new machine I just got has all but one USB ports wired as 3.0. Booting defconfig resulted in no keyboard or mouse, which was pretty uncool. Let's enable that -- USB3 is ubiquitous rather than an oddity. As 'y' not 'm' -- recovering from initrd problems needs a keyboard. Signed-off-by: Adam Borowski --- v2: also i386 Ingo Molnar said: > Also, could we please refresh this in the 32-bit defconfig as well. While real 32-bit machines are ancient, folks tend to test such kernels on something modern, thus you have a good point. Changed. arch/x86/configs/i386_defconfig | 1 + arch/x86/configs/x86_64_defconfig | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/x86/configs/i386_defconfig b/arch/x86/configs/i386_defconfig index 0eb9f92f3717..6c3ab05c231d 100644 --- a/arch/x86/configs/i386_defconfig +++ b/arch/x86/configs/i386_defconfig @@ -247,6 +247,7 @@ CONFIG_USB_HIDDEV=y CONFIG_USB=y CONFIG_USB_ANNOUNCE_NEW_DEVICES=y CONFIG_USB_MON=y +CONFIG_USB_XHCI_HCD=y CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_TT_NEWSCHED=y CONFIG_USB_OHCI_HCD=y diff --git a/arch/x86/configs/x86_64_defconfig b/arch/x86/configs/x86_64_defconfig index e32fc1f274d8..ac9ae487cfeb 100644 --- a/arch/x86/configs/x86_64_defconfig +++ b/arch/x86/configs/x86_64_defconfig @@ -243,6 +243,7 @@ CONFIG_USB_HIDDEV=y CONFIG_USB=y CONFIG_USB_ANNOUNCE_NEW_DEVICES=y CONFIG_USB_MON=y +CONFIG_USB_XHCI_HCD=y CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_TT_NEWSCHED=y CONFIG_USB_OHCI_HCD=y -- 2.19.1
[PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
Handling short packets (length < max packet size) in the Inventra DMA
engine in the MUSB driver causes the MUSB DMA controller to hang. An
example of a problem that is caused by this problem is when streaming
video out of a UVC gadget, only the first video frame is transferred.
For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
set manually by the driver. This was previously done in musb_g_tx
(musb_gadget.c), but incorrectly (all csr flags were cleared, and only
MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
some requests to be transferred correctly, but multiple requests were
often put together in one USB packet, and caused problems if the packet
size was not a multiple of 4.
Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
just like host mode transfers, then musb_g_tx forces the packet to be
flushed, by setting MUSB_TXCSR_FLUSHFIFO.
This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed
further at [2] as part of his GSoC project [3].
[0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
[1]
https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
[2]
http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
[3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
I have forward-ported this patch from 2.6.34 to 4.19-rc1.
Signed-off-by: Paul Elder
---
drivers/usb/musb/musb_gadget.c | 21 ++---
drivers/usb/musb/musbhsdma.c | 21 +++--
2 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index eae8b1b1b45b..d3f33f449445 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
&& (request->actual == request->length))
short_packet = true;
- if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
- (is_dma && (!dma->desired_mode ||
- (request->actual &
- (musb_ep->packet_sz - 1)
- short_packet = true;
+ if (is_dma && (musb_dma_inventra(musb) ||
musb_dma_ux500(musb))) {
+ if (!dma->desired_mode ||
+ request->actual % musb_ep->packet_sz) {
+ musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
+ musb_ep->end_point.name);
+ musb_writew(epio, MUSB_TXCSR,
+ csr | MUSB_TXCSR_FLUSHFIFO);
+ return;
+ }
+ }
if (short_packet) {
/*
@@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
if (csr & MUSB_TXCSR_TXPKTRDY)
return;
- musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
- | MUSB_TXCSR_TXPKTRDY);
+ musb_dbg(musb, "sending zero pkt (zero=%d, length=%d,
actual=%d, dma->desired_mode=%d)\n",
+request->zero, request->length,
request->actual,
+dma->desired_mode);
+ musb_writew(epio, MUSB_TXCSR, csr |
MUSB_TXCSR_TXPKTRDY);
request->zero = 0;
}
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f87829..e514d4700a6b 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int irq, void
*private_data)
channel->status = MUSB_DMA_STATUS_FREE;
/* completed */
- if ((devctl & MUSB_DEVCTL_HM)
- && (musb_channel->transmit)
- && ((channel->desired_mode == 0)
- || (channel->actual_len &
- (musb_channel->max_packet_sz - 1)))
- ) {
+ if (musb_channel->transmit &&
+ (!channel->desired_mode ||
+(channel->actual_len %
+ musb_channel->max_packet_sz))) {
u8 epnum = musb_channel->epnum;
int offset = musb->io.ep_offset(epnum,
RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver
Hi Felipe, >-Original Message- >From: Felipe Balbi [mailto:ba...@kernel.org] >Sent: Tuesday, October 09, 2018 11:07 AM >To: Anurag Kumar Vulisha ; Anurag Kumar Vulisha >; gre...@linuxfoundation.org >Cc: v.anuragku...@gmail.com; linux-usb@vger.kernel.org; linux- >ker...@vger.kernel.org; thinh.ngu...@synopsys.com; Ajay Yugalkishore Pandey > >Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget driver > > >(no top-posting, please) > Will ensure this won't happen again >Hi, > > >Anurag Kumar Vulisha writes: > >> Hi Felipe, >> >> Please let us know if you have any suggestions / comments on this patch >> series. >> If you feel this patch series are okay, can we proceed with them? > >I really don't like this dwc3-specific timer. The best way here would be >to add a timer on udc/core.c which can be reused by any udc. This would >mean, of course, teaching udc/core about streams and lettting it do part >of the handling. > Thanks for spending your time in reviewing this patch. The reason for adding the timer is when streams are enabled there could be chances for the host and gadget controller to become out of sync, the gadget may wait for the host to issue prime transaction and the host may wait for the gadget to issue ERDY. To avoid such a potential deadlock conditions, timeout needs to be implemented in dwc3 driver. After timeout occurs, gadget will first stop transfer and restart the transfer again. This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how other controllers are handling the streams, but since this issue looks more like a dwc3 specific issue, I think it would be more convincing to add the timer in dwc3 gadget driver rather than adding in udc framework. Also we are stopping the timer when a valid StreamEvnt is found, which would be difficult to handle if the timer is moved into udc. Please help me by correcting , if I am missing something or my understanding is wrong. Thanks, Anurag Kumar Vulisha >Best > >-- >balbi
