| Message ID | 20190705164355.14025-2-suwan.kim027@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usbip: Implement SG support | expand |
Hi Suwan, On 7/5/19 10:43 AM, Suwan Kim wrote: > vhci doesn’t do DMA for remote device. Actually, the real DMA > operation is done by network card driver. vhci just passes virtual > address of the buffer to the network stack, so vhci doesn’t use and > need dma address of the buffer of the URB. > > When it comes to supporting SG for vhci, it is useful to use native > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping > fnuction can adjust the number of SG list (urb->num_mapped_sgs). > > But HCD provides DMA mapping and unmapping function by default. > Moreover, it causes unnecessary DMA mapping and unmapping which > will be done again at the NIC driver and it wastes CPU cycles. > So, implement map_urb_for_dma and unmap_urb_for_dma function for > vhci in order to skip the DMA mapping and unmapping procedure. > > To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in > urb->transfer_flags if URB has SG list and this flag will tell the > stub driver to use SG list. > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > --- > drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > index 000ab7225717..14fc6d9f4e6a 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev, > return 0; > } > > +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > + gfp_t mem_flags) > +{ > + dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n"); > + > + if (urb->num_sgs) > + urb->transfer_flags |= URB_DMA_MAP_SG; > + Shouldn't this be part of patch 2. The debug message saying "no map" and setting flag doesn't make sense. > + return 0; This should be a tab and no spaces btw. chekpatch isn't happy. > +} > + > +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) > +{ > + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n"); This should be a tab and no spaces btw. chekpatch isn't happy. WARNING: please, no spaces at the start of a line #144: FILE: drivers/usb/usbip/vhci_hcd.c:1299: + return 0;$ WARNING: please, no spaces at the start of a line #149: FILE: drivers/usb/usbip/vhci_hcd.c:1304: + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");$ total: 0 errors, 2 warnings, 31 lines checked > +} > + > static const struct hc_driver vhci_hc_driver = { > .description = driver_name, > .product_desc = driver_desc, > @@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = { > > .get_frame_number = vhci_get_frame_number, > > + .map_urb_for_dma = vhci_map_urb_for_dma, > + .unmap_urb_for_dma = vhci_unmap_urb_for_dma, > + > .hub_status_data = vhci_hub_status, > .hub_control = vhci_hub_control, > .bus_suspend = vhci_bus_suspend, > thanks, -- Shuah
On Mon, Jul 22, 2019 at 02:26:42PM -0600, shuah wrote: > Hi Suwan, > > On 7/5/19 10:43 AM, Suwan Kim wrote: > > vhci doesn’t do DMA for remote device. Actually, the real DMA > > operation is done by network card driver. vhci just passes virtual > > address of the buffer to the network stack, so vhci doesn’t use and > > need dma address of the buffer of the URB. > > > > When it comes to supporting SG for vhci, it is useful to use native > > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping > > fnuction can adjust the number of SG list (urb->num_mapped_sgs). > > > > But HCD provides DMA mapping and unmapping function by default. > > Moreover, it causes unnecessary DMA mapping and unmapping which > > will be done again at the NIC driver and it wastes CPU cycles. > > So, implement map_urb_for_dma and unmap_urb_for_dma function for > > vhci in order to skip the DMA mapping and unmapping procedure. > > > > To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in > > urb->transfer_flags if URB has SG list and this flag will tell the > > stub driver to use SG list. > > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > > --- > > drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > > index 000ab7225717..14fc6d9f4e6a 100644 > > --- a/drivers/usb/usbip/vhci_hcd.c > > +++ b/drivers/usb/usbip/vhci_hcd.c > > @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev, > > return 0; > > } > > +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > > + gfp_t mem_flags) > > +{ > > + dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n"); > > + > > + if (urb->num_sgs) > > + urb->transfer_flags |= URB_DMA_MAP_SG; > > + > > Shouldn't this be part of patch 2. The debug message saying "no map" > and setting flag doesn't make sense. I think you are right. Setting flag should be in patch 2. Thank you for pointing out :) I will remove unnecessary debug messages and move setting flag to patch 2. > > + return 0; > > This should be a tab and no spaces btw. chekpatch isn't happy. > > > +} > > + > > +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) > > +{ > > + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n"); > > This should be a tab and no spaces btw. chekpatch isn't happy. > > > WARNING: please, no spaces at the start of a line > #144: FILE: drivers/usb/usbip/vhci_hcd.c:1299: > + return 0;$ > > WARNING: please, no spaces at the start of a line > #149: FILE: drivers/usb/usbip/vhci_hcd.c:1304: > + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");$ > > total: 0 errors, 2 warnings, 31 lines checked I'm sorry for my fault. I will check it. Regards, Suwan Kim
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 000ab7225717..14fc6d9f4e6a 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev, return 0; } +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, + gfp_t mem_flags) +{ + dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n"); + + if (urb->num_sgs) + urb->transfer_flags |= URB_DMA_MAP_SG; + + return 0; +} + +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) +{ + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n"); +} + static const struct hc_driver vhci_hc_driver = { .description = driver_name, .product_desc = driver_desc, @@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = { .get_frame_number = vhci_get_frame_number, + .map_urb_for_dma = vhci_map_urb_for_dma, + .unmap_urb_for_dma = vhci_unmap_urb_for_dma, + .hub_status_data = vhci_hub_status, .hub_control = vhci_hub_control, .bus_suspend = vhci_bus_suspend,
vhci doesn’t do DMA for remote device. Actually, the real DMA operation is done by network card driver. vhci just passes virtual address of the buffer to the network stack, so vhci doesn’t use and need dma address of the buffer of the URB. When it comes to supporting SG for vhci, it is useful to use native SG list (urb->num_sgs) instead of mapped SG list because DMA mapping fnuction can adjust the number of SG list (urb->num_mapped_sgs). But HCD provides DMA mapping and unmapping function by default. Moreover, it causes unnecessary DMA mapping and unmapping which will be done again at the NIC driver and it wastes CPU cycles. So, implement map_urb_for_dma and unmap_urb_for_dma function for vhci in order to skip the DMA mapping and unmapping procedure. To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in urb->transfer_flags if URB has SG list and this flag will tell the stub driver to use SG list. Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> --- drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)