| Message ID | 20250219092555.112631-1-min_halo@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | usbip: Fix the error limitation on max_hw_sectors for usbip device | expand |
On 2/19/25 02:25, Zongmin Zhou wrote: > From: Zongmin Zhou <zhouzongmin@kylinos.cn> > > This patch fixes an issue that usb device through usbip protocol, > the max_hw_sectors will be limited to 512,and then > read/write rate of high speed USB devices will be limited. > > After the commit d74ffae8b8dd ("usb-storage: Add a limitation > for blk_queue_max_hw_sectors()") is applied, > This issue happened on the swiotlb enabled environment. > This commit will checks the maximum size of a mapping for the device, > and adjusts the max_hw_sectors.On vhci-hcd driver, > the dma mask setting follows the platform device default setting(32-bit). > So dma_addressing_limited() will be true,then the maximum mapping size > use the swiotlb max mapping size(512).The max_hw_sectors reset to 512. > > To fix this issue,have to get the dma mask bit that > the real USB controllers support,and set this value on vhci-hcd driver, > usbip device will get the correct max_hw_sectors. I don't have objections to this change. This does change the API between the kernel and user-space and requires more testing with mispatched kernel and user-space. Can you give me more details on testing you have done with the revision mismatches environments? Also this would require bumping up the version on kernel and user-space sides. thanks, -- Shuah
On 2/24/25 01:36, Zongmin Zhou wrote: > Dear Shuah, > > > Yes，the usbip server get the real USB controller message send to usbip client vhci-hcd， > > must have to changethe API between the kernel and user-space. > > The easiest way is to simply set vhci-hcd dma mask to 64 by default, > > but not all USB controllers support 64bit，That doesn't look good？ This is an expetnsive change to fix the problem. In addition this change is unnecessary for non-storage devices where USB over IP is used. You mentioned this happens only in swiotlb cases? Can you explain the configuration on host and client side. I would like to explore fixes that are simpler than what is proposed in this patch. thanks, -- Shuah
On 3/2/25 05:37, Zongmin Zhou wrote: > Dear shuah, > > > Yes, I agree with you.It would be better if there have a more simpler fixes than This patch. > > I can just think of the two possible solutions that mentioned before. What are the two possible solutions? > > > If SWIOTLB disabled,dma_max_mapping_size() return SIZE_MAX. Right when CONFIG_HAS_DMA, if not it returns 0. Perhaps we can ignore CONFIG_HAS_DMA=n for this for this discussion. > > Only if SWIOTLB is active and dma addressing limited will return the swiotlb max mapping size. > > > The swiotlb config seems rely on many other config options like x86_64/IOMMU_SUPPORT and so on, > > and the configuration on host and client side only use the default at all,Like the default ubuntu release version. > > It seems that switlb is enabled by default on most platforms. If understand correctly the problem happens only when SWIOTLB is enabled on client or host? The following combinations are possible: SWILTLB enabled on client and disabled on host - rate limited? SWILTLB enabled on client and enabled on host - rate limited? SWILTLB disabled on client and enabled on host - rate limited? SWILTLB disabled on client and disabled on host - not a problem thanks, -- Shuah
On 3/13/25 04:02, Zongmin Zhou wrote: > > On 2025/3/11 00:49, Shuah Khan wrote: >> On 3/5/25 03:03, Zongmin Zhou wrote: >>> At 2025-03-05 03:45:28, "Shuah Khan" <skhan@linuxfoundation.org> wrote: >>> >>>> On 3/2/25 05:37, Zongmin Zhou wrote: >>>>> Dear shuah, >>>>> >>>>> >>>>> Yes, I agree with you.It would be better if there have a more simpler fixes than This patch. >>>>> >>>>> I can just think of the two possible solutions that mentioned before. >>>> >>> >What are the two possible solutions? >>> 1. The patch we are discussing now,have to change the API between the kernel and user-space. >> >> 2. Simply set vhci-hcd dma mask to 64 by default,just modify the vhci-hcd driver. Then dma_max_mapping_size() will always return SIZE_MAX. >> >> I prefer option #2 - What are the downsides if any with this option? >> > If set vhci-hcd dma mask to 64 by default,I can't predict what will happen when the real USB controller support less than 64bit? > > After all, the data flows from vhci-hcd to usbip-host and finally to the USB controller to which the device is actually connected. > > the data is ultimately processed through the real USB controller? Sorry for the delay. That is the case. I have to check the code to see what the host would do if it receives larger buffers from the client (vhci) > > However, the default setting to 64-bit is equivalent to eliminating the impact of > > the patch(commit d74ffae8b8dd) on usbip protocol devices, sounds feasible? > > I am not very professional in this field, waiting for your evaluation. We can give this a try. Send me the patch with default testing the following cases: Host - swiotlb enabled and disabled in your environment to see what happens when there is a mismatch swiotlb enabled case and client side doesn't limit the size. thanks, -- Shuah
On 4/2/25 02:34, Zongmin Zhou wrote: > > On 2025/3/29 05:14, Shuah Khan wrote: >> On 3/13/25 04:02, Zongmin Zhou wrote: >>> >>> On 2025/3/11 00:49, Shuah Khan wrote: >>>> On 3/5/25 03:03, Zongmin Zhou wrote: >>>>> At 2025-03-05 03:45:28, "Shuah Khan" <skhan@linuxfoundation.org> wrote: >>>>> >>>>>> On 3/2/25 05:37, Zongmin Zhou wrote: >>>>>>> Dear shuah, >>>>>>> >>>>>>> >>>>>>> Yes, I agree with you.It would be better if there have a more simpler fixes than This patch. >>>>>>> >>>>>>> I can just think of the two possible solutions that mentioned before. >>>>>> >>>>> >What are the two possible solutions? >>>>> 1. The patch we are discussing now,have to change the API between the kernel and user-space. >>>> >>>> 2. Simply set vhci-hcd dma mask to 64 by default,just modify the vhci-hcd driver. Then dma_max_mapping_size() will always return SIZE_MAX. >>>> >>>> I prefer option #2 - What are the downsides if any with this option? >>>> >>> If set vhci-hcd dma mask to 64 by default,I can't predict what will happen when the real USB controller support less than 64bit? >>> >>> After all, the data flows from vhci-hcd to usbip-host and finally to the USB controller to which the device is actually connected. >>> >>> the data is ultimately processed through the real USB controller? >> >> Sorry for the delay. >> >> That is the case. I have to check the code to see what the host >> would do if it receives larger buffers from the client (vhci) >>> >>> However, the default setting to 64-bit is equivalent to eliminating the impact of >>> >>> the patch(commit d74ffae8b8dd) on usbip protocol devices, sounds feasible? >>> >>> I am not very professional in this field, waiting for your evaluation. >> >> We can give this a try. Send me the patch with default testing the >> following cases: >> >> Host - swiotlb enabled and disabled in your environment to see what >> happens when there is a mismatch swiotlb enabled case and client >> side doesn't limit the size. > > If you want to test swiotlb disabled mode, you can modify the kernel cmd to force disable swiotlb: > > modify the grub.cfg, add the swiotlb=noforce parameter to kernel command line,and reboot. > > cat /proc/cmdline to check whether modified successfully. > > > The patch set vhci-hcd dma mask to 64 by default like below: > > --- > drivers/usb/usbip/vhci_hcd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > index e70fba9f55d6..fca3a4a6e94d 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -1345,6 +1345,9 @@ static int vhci_hcd_probe(struct platform_device *pdev) > > usbip_dbg_vhci_hc("name %s id %d\n", pdev->name, pdev->id); > > + /* Set the dma mask to support 64bit for vhci-hcd driver. */ > + dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)); > + > /* > * Allocate and initialize hcd. > * Our private data is also allocated automatically. Please send me a patch I can apply. thanks, -- Shuah
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c index ce625b1ce9a5..095de5a8a307 100644 --- a/drivers/usb/usbip/stub_dev.c +++ b/drivers/usb/usbip/stub_dev.c @@ -7,6 +7,7 @@ #include <linux/file.h> #include <linux/kthread.h> #include <linux/module.h> +#include <linux/dma-mapping.h> #include "usbip_common.h" #include "stub.h" @@ -34,6 +35,46 @@ static ssize_t usbip_status_show(struct device *dev, } static DEVICE_ATTR_RO(usbip_status); +/* + * The real USB controllers may support larger than 32-bit address memory pointers actually. + * But vhci-hcd driver use the default platform device dma mask set(32-bit), + * and usbip device's max_sectors will be limited by dma max mapping size. + * usbip_dma_bits shows the real dma mask bit of the real usb controller + * which usbip device is bound to. + */ +static unsigned int mask_convert_to_bits(u64 dma_mask) +{ + unsigned int bits = 0; + + while (dma_mask & 0x1) { + dma_mask >>= 1; + bits++; + } + return bits; +} + +static ssize_t usbip_dma_bits_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct stub_device *sdev = dev_get_drvdata(dev); + struct device *sys_dev = sdev->udev->bus->sysdev; + u64 dma_mask = 0; + unsigned int dma_bits = 0; + + if (!sdev || !sys_dev) { + dev_err(dev, "sdev or sys_dev is null\n"); + return -ENODEV; + } + + spin_lock_irq(&sdev->ud.lock); + dma_mask = dma_get_mask(sys_dev); + dma_bits = mask_convert_to_bits(dma_mask); + spin_unlock_irq(&sdev->ud.lock); + + return sysfs_emit(buf, "%d\n", dma_bits); +} +static DEVICE_ATTR_RO(usbip_dma_bits); + /* * usbip_sockfd gets a socket descriptor of an established TCP connection that * is used to transfer usbip requests by kernel threads. -1 is a magic number @@ -144,6 +185,7 @@ static DEVICE_ATTR_WO(usbip_sockfd); static struct attribute *usbip_attrs[] = { &dev_attr_usbip_status.attr, + &dev_attr_usbip_dma_bits.attr, &dev_attr_usbip_sockfd.attr, &dev_attr_usbip_debug.attr, NULL, diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index d5865460e82d..972beac653f6 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -12,6 +12,7 @@ /* Hardening for Spectre-v1 */ #include <linux/nospec.h> +#include <linux/dma-mapping.h> #include "usbip_common.h" #include "vhci.h" @@ -311,7 +312,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, { struct socket *socket; int sockfd = 0; - __u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0; + __u32 port = 0, pdev_nr = 0, rhport = 0, devid = 0, speed = 0, dma_bits = 0; struct usb_hcd *hcd; struct vhci_hcd *vhci_hcd; struct vhci_device *vdev; @@ -327,15 +328,15 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, * @devid: unique device identifier in a remote host * @speed: usb device speed in a remote host */ - if (sscanf(buf, "%u %u %u %u", &port, &sockfd, &devid, &speed) != 4) + if (sscanf(buf, "%u %u %u %u %u", &port, &sockfd, &devid, &speed, &dma_bits) != 5) return -EINVAL; pdev_nr = port_to_pdev_nr(port); rhport = port_to_rhport(port); usbip_dbg_vhci_sysfs("port(%u) pdev(%d) rhport(%u)\n", port, pdev_nr, rhport); - usbip_dbg_vhci_sysfs("sockfd(%u) devid(%u) speed(%u)\n", - sockfd, devid, speed); + usbip_dbg_vhci_sysfs("sockfd(%u) devid(%u) speed(%u) dma_bits(%u)\n", + sockfd, devid, speed, dma_bits); /* check received parameters */ if (!valid_args(&pdev_nr, &rhport, speed)) @@ -346,6 +347,8 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr, dev_err(dev, "port %d is not ready\n", port); return -EAGAIN; } + /* Set the real USB controller's dma mask to vhci-hcd driver. */ + dma_set_mask(dev, DMA_BIT_MASK(dma_bits)); vhci_hcd = hcd_to_vhci_hcd(hcd); vhci = vhci_hcd->vhci; diff --git a/tools/usb/usbip/libsrc/usbip_common.h b/tools/usb/usbip/libsrc/usbip_common.h index 73a367a7fa10..c87d74917c2a 100644 --- a/tools/usb/usbip/libsrc/usbip_common.h +++ b/tools/usb/usbip/libsrc/usbip_common.h @@ -115,6 +115,7 @@ struct usbip_usb_device { uint32_t busnum; uint32_t devnum; uint32_t speed; + uint32_t dma_bits; uint16_t idVendor; uint16_t idProduct; diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c b/tools/usb/usbip/libsrc/usbip_host_common.c index ca78aa368476..7f5cc44236c9 100644 --- a/tools/usb/usbip/libsrc/usbip_host_common.c +++ b/tools/usb/usbip/libsrc/usbip_host_common.c @@ -61,6 +61,38 @@ static int32_t read_attr_usbip_status(struct usbip_usb_device *udev) return value; } +static int32_t read_attr_usbip_dma_bits(struct usbip_usb_device *udev) +{ + char dma_bits_attr_path[SYSFS_PATH_MAX]; + int size; + int fd; + int length; + char dma_bits[3] = { 0 }; + + size = snprintf(dma_bits_attr_path, sizeof(dma_bits_attr_path), + "%s/usbip_dma_bits", udev->path); + if (size < 0 || (unsigned int)size >= sizeof(dma_bits_attr_path)) { + err("usbip_dma_bits path length %i >= %lu or < 0", size, + (unsigned long)sizeof(dma_bits_attr_path)); + return -1; + } + + fd = open(dma_bits_attr_path, O_RDONLY); + if (fd < 0) { + err("error opening attribute %s", dma_bits_attr_path); + return -1; + } + length = read(fd, dma_bits, 2); + if (length < 0) { + err("error reading attribute %s", dma_bits_attr_path); + close(fd); + return -1; + } + udev->dma_bits = atoi(dma_bits); + close(fd); + return 0; +} + static struct usbip_exported_device *usbip_exported_device_new( struct usbip_host_driver *hdriver, const char *sdevpath) @@ -82,6 +114,8 @@ struct usbip_exported_device *usbip_exported_device_new( if (hdriver->ops.read_device(edev->sudev, &edev->udev) < 0) goto err; + read_attr_usbip_dma_bits(&edev->udev); + edev->status = read_attr_usbip_status(&edev->udev); if (edev->status < 0) goto err; diff --git a/tools/usb/usbip/libsrc/vhci_driver.c b/tools/usb/usbip/libsrc/vhci_driver.c index 8159fd98680b..e5fc4a048a5f 100644 --- a/tools/usb/usbip/libsrc/vhci_driver.c +++ b/tools/usb/usbip/libsrc/vhci_driver.c @@ -355,15 +355,15 @@ int usbip_vhci_get_free_port(uint32_t speed) } int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid, - uint32_t speed) { + uint32_t speed, uint32_t dma_bits) { char buff[200]; /* what size should be ? */ char attach_attr_path[SYSFS_PATH_MAX]; char attr_attach[] = "attach"; const char *path; int ret; - snprintf(buff, sizeof(buff), "%u %d %u %u", - port, sockfd, devid, speed); + snprintf(buff, sizeof(buff), "%u %d %u %u %u", + port, sockfd, devid, speed, dma_bits); dbg("writing: %s", buff); path = udev_device_get_syspath(vhci_driver->hc_device); @@ -389,11 +389,11 @@ static unsigned long get_devid(uint8_t busnum, uint8_t devnum) /* will be removed */ int usbip_vhci_attach_device(uint8_t port, int sockfd, uint8_t busnum, - uint8_t devnum, uint32_t speed) + uint8_t devnum, uint32_t speed, uint32_t dma_bits) { int devid = get_devid(busnum, devnum); - return usbip_vhci_attach_device2(port, sockfd, devid, speed); + return usbip_vhci_attach_device2(port, sockfd, devid, speed, dma_bits); } int usbip_vhci_detach_device(uint8_t port) diff --git a/tools/usb/usbip/libsrc/vhci_driver.h b/tools/usb/usbip/libsrc/vhci_driver.h index 6c9aca216705..bf40aa23b6f3 100644 --- a/tools/usb/usbip/libsrc/vhci_driver.h +++ b/tools/usb/usbip/libsrc/vhci_driver.h @@ -54,11 +54,11 @@ int usbip_vhci_refresh_device_list(void); int usbip_vhci_get_free_port(uint32_t speed); int usbip_vhci_attach_device2(uint8_t port, int sockfd, uint32_t devid, - uint32_t speed); + uint32_t speed, uint32_t dma_bits); /* will be removed */ int usbip_vhci_attach_device(uint8_t port, int sockfd, uint8_t busnum, - uint8_t devnum, uint32_t speed); + uint8_t devnum, uint32_t speed, uint32_t dma_bits); int usbip_vhci_detach_device(uint8_t port); diff --git a/tools/usb/usbip/src/usbip_attach.c b/tools/usb/usbip/src/usbip_attach.c index 531a415538f9..453987782eec 100644 --- a/tools/usb/usbip/src/usbip_attach.c +++ b/tools/usb/usbip/src/usbip_attach.c @@ -100,7 +100,7 @@ static int import_device(int sockfd, struct usbip_usb_device *udev) dbg("got free port %d", port); rc = usbip_vhci_attach_device(port, sockfd, udev->busnum, - udev->devnum, udev->speed); + udev->devnum, udev->speed, udev->dma_bits); if (rc < 0 && errno != EBUSY) { err("import device"); goto err_driver_close; diff --git a/tools/usb/usbip/src/usbip_network.c b/tools/usb/usbip/src/usbip_network.c index ed4dc8c14269..2c97db9ea567 100644 --- a/tools/usb/usbip/src/usbip_network.c +++ b/tools/usb/usbip/src/usbip_network.c @@ -79,6 +79,7 @@ void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev) udev->busnum = usbip_net_pack_uint32_t(pack, udev->busnum); udev->devnum = usbip_net_pack_uint32_t(pack, udev->devnum); udev->speed = usbip_net_pack_uint32_t(pack, udev->speed); + udev->dma_bits = usbip_net_pack_uint32_t(pack, udev->dma_bits); udev->idVendor = usbip_net_pack_uint16_t(pack, udev->idVendor); udev->idProduct = usbip_net_pack_uint16_t(pack, udev->idProduct);