RE: [PATCH] USBNET: fix handling padding packet
As my below test on ax99179_178a, I believe the patch should fix padding for dma sg, but need a little update, and I will send out v1 later: $ping -s 974 another_machine #from host with ax99179_178a attached If FLAG_SEND_ZLP is set for ax99179_178a, the above ping won't work any more either on USB3.0 or USB 2.0 host controller. So don't assume that these brand new devices can support ZLP well. I've just posted a fix to the xhci driver to implement URB_ZERO_PACKET. With that fix (and ZLP enabled in the ax88179_178a driver) the above ping works fine (ax88179 Ge card on USB3). I wonder how many other usb drivers fail to support URB_ZERO_PACKET. Maybe the driver should pass a flag to its users (along with SG support). I've also posted (to linux-usb) another fix to the xhci driver that is needed to get SG (and one that cross 64k boundaries) transfers working. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote: Oliver Neukum oneu...@suse.de wrote: No, USB 3.0 uses no companion controllers, so you can have devices of any speed connected to it. Ah, right. I don't own such modern hardware, but I should have known this anyway. This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else. For such devices we might assume it. But how does that matter for generic code? As any kind of device may be connected to XHCI, the sg code is relevant for every driver. And I certainly don't want trouble for older devices' drivers converted to sg. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
Oliver Neukum oneu...@suse.de writes: On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote: Oliver Neukum oneu...@suse.de wrote: No, USB 3.0 uses no companion controllers, so you can have devices of any speed connected to it. Ah, right. I don't own such modern hardware, but I should have known this anyway. This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else. For such devices we might assume it. But how does that matter for generic code? The code isn't generic yet. Most of it is placed inside the ax88179_178a minidriver. But I do agree that adding this padding support can be seen as one step towards making the code generic. So if you really anticipate this being enabled for e.g. the ECM class driver, then yes, padding must be supported. I would have had less trouble understanding the value if this patch was part of a generalisation series, though... As any kind of device may be connected to XHCI, the sg code is relevant for every driver. And I certainly don't want trouble for older devices' drivers converted to sg. I wonder what the gain of that really is? Yes, I can see the advantage of making the class drivers more effective. But padding is only relevant for the ECM class, isn't it? And are there any ECM class devices where SG support matters? Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] USBNET: fix handling padding packet
I wonder what the gain of that really is? Yes, I can see the advantage of making the class drivers more effective. But padding is only relevant for the ECM class, isn't it? And are there any ECM class devices where SG support matters? AFAICT the requirement for avoiding ZLP is a property of the slave. Support for scatter-gather is a property of the host. So connect an old slave to a modern host and you may be able to use sg to add the header or padding (and possibly have a fragmented skb). Generating frames that would have a ZLP should just be a matter of sending UDP frames with ever increasing lengths - although a printf in the driver would confirm it. However the is a report (somewhere) that it would work sometimes and not others with certain targets. I've got one of the ASIX USB3 devices here, but the system is running ubuntu 12.04 - which doesn't have the sg changes. I've not looked far enough to see if the sg changes in usbnet.c can be applied to that kernel. David
Re: [PATCH] USBNET: fix handling padding packet
On Thu, Sep 19, 2013 at 3:18 PM, Bjørn Mork bj...@mork.no wrote: Oliver Neukum oneu...@suse.de writes: On Wed, 2013-09-18 at 20:56 +0200, Bjørn Mork wrote: Oliver Neukum oneu...@suse.de wrote: No, USB 3.0 uses no companion controllers, so you can have devices of any speed connected to it. Ah, right. I don't own such modern hardware, but I should have known this anyway. This still doesn't change the fact that the driver is brand new for brand new devices. I believe we should assume such devices will support ZLPs unless we have documentation stating anything else. For such devices we might assume it. But how does that matter for generic code? The code isn't generic yet. Most of it is placed inside the ax88179_178a minidriver. No, the patch doesn't touch code of ax99179_178a. And it is really generic to fix the padding in case of dma sg. But I do agree that adding this padding support can be seen as one step towards making the code generic. So if you really anticipate this being enabled for e.g. the ECM class driver, then yes, padding must be supported. 1byte padding frame is already supported by usbnet before, isn't it? I would have had less trouble understanding the value if this patch was part of a generalisation series, though... As my below test on ax99179_178a, I believe the patch should fix padding for dma sg, but need a little update, and I will send out v1 later: $ping -s 974 another_machine #from host with ax99179_178a attached If FLAG_SEND_ZLP is set for ax99179_178a, the above ping won't work any more either on USB3.0 or USB 2.0 host controller. So don't assume that these brand new devices can support ZLP well. As any kind of device may be connected to XHCI, the sg code is relevant for every driver. And I certainly don't want trouble for older devices' drivers converted to sg. I wonder what the gain of that really is? Yes, I can see the advantage of making the class drivers more effective. But padding is only relevant for the ECM class, isn't it? And are there any ECM class devices where SG support matters? Why is padding only relevant for the ECM? Of course, other devices need it too, such as asix devices. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
On Tue, 2013-09-17 at 17:10 +0800, Ming Lei wrote: Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG if the usb host controller is capable of building packet from discontinuous buffers, but missed handling padding packet when building DMA SG. This patch attachs the pre-allocated padding packet at the end of the sg list, so padding packet can be sent to device if drivers require that. Reported-by: David Laight david.lai...@aculab.com Cc: Oliver Neukum oli...@neukum.org Signed-off-by: Ming Lei ming@canonical.com Acked-by: Oliver Neukum oli...@neukum.org -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
Ming Lei ming@canonical.com writes:
Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.
This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.
Reported-by: David Laight david.lai...@aculab.com
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/usbnet.c | 27 +--
include/linux/usb/usbnet.h |1 +
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..4a9bed3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb,
struct urb *urb)
if (num_sgs == 1)
return 0;
- urb-sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+ /* reserve one for zero packet */
+ urb-sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+ GFP_ATOMIC);
if (!urb-sg)
return -ENOMEM;
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
- entry-length = length = urb-transfer_buffer_length;
+ length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
* NOTE: strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (length % dev-maxpacket == 0) {
if (!(info-flags FLAG_SEND_ZLP)) {
if (!(info-flags FLAG_MULTI_PACKET)) {
- urb-transfer_buffer_length++;
- if (skb_tailroom(skb)) {
+ length++;
+ if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
- }
+ } else if (dev-can_dma_sg)
+ sg_set_buf(urb-sg[urb-num_sgs++],
+ dev-padding_pkt, 1);
}
} else
urb-transfer_flags |= URB_ZERO_PACKET;
}
+ entry-length = urb-transfer_buffer_length = length;
spin_lock_irqsave(dev-txq.lock, flags);
retval = usb_autopm_get_interface_async(dev-intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev-interrupt);
usb_free_urb(dev-interrupt);
+ kfree(dev-padding_pkt);
free_netdev(net);
}
@@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
/* initialize max rx_qlen and tx_qlen */
usbnet_update_max_qlen(dev);
+ if (dev-can_dma_sg !(info-flags FLAG_SEND_ZLP)
+ !(info-flags FLAG_MULTI_PACKET)) {
+ dev-padding_pkt = kzalloc(1, GFP_KERNEL);
+ if (!dev-padding_pkt)
+ goto out4;
+ }
+
status = register_netdev (net);
if (status)
- goto out4;
+ goto out5;
netif_info(dev, probe, dev-net,
register '%s' at usb-%s-%s, %s, %pM\n,
udev-dev.driver-name,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
return 0;
+out5:
+ kfree(dev-padding_pkt);
out4:
usb_free_urb(dev-interrupt);
out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
struct usb_host_endpoint *status;
unsignedmaxpacket;
struct timer_list delay;
+ const char *padding_pkt;
/* protocol/interface state */
struct net_device *net;
The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
strictly conforming cdc-ether devices should expect the ZLP here
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
I also believe it would be nice to move the initialisation of
Re: [PATCH] USBNET: fix handling padding packet
On Wed, Sep 18, 2013 at 9:59 PM, Bjørn Mork bj...@mork.no wrote:
Ming Lei ming@canonical.com writes:
Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
if the usb host controller is capable of building packet from
discontinuous buffers, but missed handling padding packet when
building DMA SG.
This patch attachs the pre-allocated padding packet at the
end of the sg list, so padding packet can be sent to device
if drivers require that.
Reported-by: David Laight david.lai...@aculab.com
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/usbnet.c | 27 +--
include/linux/usb/usbnet.h |1 +
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7b331e6..4a9bed3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb,
struct urb *urb)
if (num_sgs == 1)
return 0;
- urb-sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+ /* reserve one for zero packet */
+ urb-sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
+ GFP_ATOMIC);
if (!urb-sg)
return -ENOMEM;
@@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (build_dma_sg(skb, urb) 0)
goto drop;
}
- entry-length = length = urb-transfer_buffer_length;
+ length = urb-transfer_buffer_length;
/* don't assume the hardware handles USB_ZERO_PACKET
* NOTE: strictly conforming cdc-ether devices should expect
@@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
if (length % dev-maxpacket == 0) {
if (!(info-flags FLAG_SEND_ZLP)) {
if (!(info-flags FLAG_MULTI_PACKET)) {
- urb-transfer_buffer_length++;
- if (skb_tailroom(skb)) {
+ length++;
+ if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
- }
+ } else if (dev-can_dma_sg)
+ sg_set_buf(urb-sg[urb-num_sgs++],
+ dev-padding_pkt, 1);
}
} else
urb-transfer_flags |= URB_ZERO_PACKET;
}
+ entry-length = urb-transfer_buffer_length = length;
spin_lock_irqsave(dev-txq.lock, flags);
retval = usb_autopm_get_interface_async(dev-intf);
@@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
usb_kill_urb(dev-interrupt);
usb_free_urb(dev-interrupt);
+ kfree(dev-padding_pkt);
free_netdev(net);
}
@@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const
struct usb_device_id *prod)
/* initialize max rx_qlen and tx_qlen */
usbnet_update_max_qlen(dev);
+ if (dev-can_dma_sg !(info-flags FLAG_SEND_ZLP)
+ !(info-flags FLAG_MULTI_PACKET)) {
+ dev-padding_pkt = kzalloc(1, GFP_KERNEL);
+ if (!dev-padding_pkt)
+ goto out4;
+ }
+
status = register_netdev (net);
if (status)
- goto out4;
+ goto out5;
netif_info(dev, probe, dev-net,
register '%s' at usb-%s-%s, %s, %pM\n,
udev-dev.driver-name,
@@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct
usb_device_id *prod)
return 0;
+out5:
+ kfree(dev-padding_pkt);
out4:
usb_free_urb(dev-interrupt);
out3:
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 9cb2fe8..e303eef 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -42,6 +42,7 @@ struct usbnet {
struct usb_host_endpoint *status;
unsignedmaxpacket;
struct timer_list delay;
+ const char *padding_pkt;
/* protocol/interface state */
struct net_device *net;
The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
strictly conforming cdc-ether devices should expect
RE: [PATCH] USBNET: fix handling padding packet
I also believe it would be nice to move the initialisation of can_dma_sg
away from the minidriver and into usbnet_probe. It's confusing that
this field is used uninitialized (well, defaulting to zero) in all but
one minidriver. It would much nicer if the logic was more like
usbnet_probe:
if (...)
dev-can_dma_sg = 1;
minidriver_bind:
if (dev-can_dma_sg) {
dev-net-features |= NETIF_F_SG | NETIF_F_TSO;
dev-net-hw_features |= NETIF_F_SG | NETIF_F_TSO;
}
Actually it would probably be nicer if the minidriver set
a flag to indicate that it could support fragmented skb
(a lot can't because of the way they add trailers)
and also provided the length of the header it will add.
The usbnet code could then allocate the header space.
If scatter-gather dma is available (a host feature) then
the header can be allocated outside the skb data area
to avoid having to copy the entire skb data.
The check for ZLP avoidance could then be done once only
(not sure how the info would be passed to teh minidriver apart
from adding more additional parameters to teh tx_fixup function).
I did a quick scan of the minidrivers, some of them don't
seem to have the correct checks for fragmented skb (etc).
Most of them only add a header.
David
Re: [PATCH] USBNET: fix handling padding packet
From: Bjørn Mork bj...@mork.no Date: Wed, 18 Sep 2013 17:52:42 +0200 Ming Lei ming@canonical.com writes: There is no reason to forbid DMA SG for one driver which requires padding, right? Yes there is: Added complexity for everybody, based on a combination of features which just does not make any sense. No modern device should need the padding. No old device will be able to use the SG feature as implemented. You only enable it on USB3, don't you? If this feature is restricted to USB3 capable devices, then it most certainly can be restricted to ZLP capable devices with absolutely no difference in the resulting set of supported devices. I completely agree. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
Ming Lei ming@canonical.com writes:
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that. I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.
This flag should really have been inverted. ZLP should be the default
for all new usbnet drivers.
Why don't you test it on the device you tested the SG patch with? I am
pretty sure it works just fine using proper ZLP transfer termination.
strictly conforming cdc-ether devices should expect the ZLP here
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
There is no reason to forbid DMA SG for one driver which requires
padding, right?
Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.
No modern device should need the padding. No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:
if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
} else if (dev-can_dma_sg) {
sg_set_buf(urb-sg[urb-num_sgs++], skb-data,
1);
}
I.e. cheat and use the skb-data buffer twice, if that is allowed? The
actual value of the padding byte should not matter, I believe?
Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork bj...@mork.no wrote:
Ming Lei ming@canonical.com writes:
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
At least for the effected driver of ax88179, the padding packet is needed
since the driver does set the padding flag in the header, see
ax88179_tx_fixup().
Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that. I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.
This flag should really have been inverted. ZLP should be the default
for all new usbnet drivers.
In theory, it is, but who knows the reality.
Why don't you test it on the device you tested the SG patch with? I am
pretty sure it works just fine using proper ZLP transfer termination.
I should have planned to test it, but didn't know how to build TCP TSO
to make the whole frame length plus 8 dividable by 1024.
Could you or other guys share how to build such packet so that I can
do the test?
Once we can confirm the padding isn't needed, we can remove the
padding flag. But if the padding flag is still there, this patch should be
dropped.
strictly conforming cdc-ether devices should expect the ZLP here
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
There is no reason to forbid DMA SG for one driver which requires
padding, right?
Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.
No modern device should need the padding. No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
See above, if we can prove that padding isn't needed, we can remove
the padding, then the patch can be dropped. If we can't, the patch is needed.
Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:
if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
} else if (dev-can_dma_sg) {
sg_set_buf(urb-sg[urb-num_sgs++], skb-data,
1);
}
I.e. cheat and use the skb-data buffer twice, if that is allowed? The
actual value of the padding byte should not matter, I believe?
If so, we can remove the assignment of zero to skb-data[skb-len],
but probably it might cause regression, that is why I wouldn't like to
do that. Also looks introducing one extra dev-padding_frame doesn't
cause much complexity.
Thanks
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
On Wed, 2013-09-18 at 17:52 +0200, Bjørn Mork wrote:
No modern device should need the padding. No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
On XHCI.
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
No, USB 3.0 uses no companion controllers, so you can have devices
of any speed connected to it.
Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:
if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
} else if (dev-can_dma_sg) {
sg_set_buf(urb-sg[urb-num_sgs++], skb-data,
1);
}
I.e. cheat and use the skb-data buffer twice, if that is allowed? The
actual value of the padding byte should not matter, I believe?
That makes me immediately suspect a violation of the DMA rules.
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
Ming Lei ming@canonical.com writes: On Wed, Sep 18, 2013 at 11:52 PM, Bjørn Mork bj...@mork.no wrote: Why don't you test it on the device you tested the SG patch with? I am pretty sure it works just fine using proper ZLP transfer termination. I should have planned to test it, but didn't know how to build TCP TSO to make the whole frame length plus 8 dividable by 1024. You could test this without enabling TSO and just send IP packets of the appriopriate size, taking any additional framing into account Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
Oliver Neukum oneu...@suse.de wrote:
On Wed, 2013-09-18 at 17:52 +0200, Bjørn Mork wrote:
No modern device should need the padding. No old device will be able
to
use the SG feature as implemented. You only enable it on USB3, don't
On XHCI.
you? If this feature is restricted to USB3 capable devices, then it
most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.
No, USB 3.0 uses no companion controllers, so you can have devices
of any speed connected to it.
Ah, right. I don't own such modern hardware, but I should have known this
anyway.
This still doesn't change the fact that the driver is brand new for brand new
devices. I believe we should assume such devices will support ZLPs unless we
have documentation stating anything else.
Anyway, if you want to keep the padding for SG then maybe this will
work
and allow you to drop the extra struct usbnet field and allocation:
if (skb_tailroom(skb) !dev-can_dma_sg) {
skb-data[skb-len] = 0;
__skb_put(skb, 1);
} else if (dev-can_dma_sg) {
sg_set_buf(urb-sg[urb-num_sgs++],
skb-data, 1);
}
I.e. cheat and use the skb-data buffer twice, if that is allowed?
The
actual value of the padding byte should not matter, I believe?
That makes me immediately suspect a violation of the DMA rules.
Sounds likely. And it's an ugly hack in any case. Probably not a good idea.
Just one of the many random thoughts I should have kept to myself :-)
Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USBNET: fix handling padding packet
From: Ming Lei ming@canonical.com Date: Tue, 17 Sep 2013 17:10:02 +0800 Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG if the usb host controller is capable of building packet from discontinuous buffers, but missed handling padding packet when building DMA SG. This patch attachs the pre-allocated padding packet at the end of the sg list, so padding packet can be sent to device if drivers require that. Reported-by: David Laight david.lai...@aculab.com Cc: Oliver Neukum oli...@neukum.org Signed-off-by: Ming Lei ming@canonical.com Some reviews and ACKs would be appreciated on this one, thanks. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
