| Message ID | 20190806123154.23798-2-suwan.kim027@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usbip: Implement SG support | expand |
On 8/6/19 6:31 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. > > 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. > > 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). > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > hcd_map_urb_for_dma() does. > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > --- > v3 - v4: > - Replace WARN_ON() with pr_err() in the error path. > > v2 - v3 > - Move setting URB_DMA_MAP_SG flag to the patch 2. > - Prevent isoc pipe from using SG buffer. > > v1 - v2 > - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell > stub driver to use SG buffer. > --- > 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..429e4e989f38 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) > +{ > + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) { > + pr_err("SG is not supported for isochronous transfer\n"); Any reason to not use dev_err()? Looks good otherwise. thanks, -- Shuah
On Tue, Aug 06, 2019 at 09:11:30AM -0600, shuah wrote: > On 8/6/19 6:31 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. > > > > 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. > > > > 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). > > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > > hcd_map_urb_for_dma() does. > > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > > --- > > v3 - v4: > > - Replace WARN_ON() with pr_err() in the error path. > > > > v2 - v3 > > - Move setting URB_DMA_MAP_SG flag to the patch 2. > > - Prevent isoc pipe from using SG buffer. > > > > v1 - v2 > > - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell > > stub driver to use SG buffer. > > --- > > 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..429e4e989f38 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) > > +{ > > + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) { > > + pr_err("SG is not supported for isochronous transfer\n"); > > Any reason to not use dev_err()? Because some codes in vhci_hcd.c use pr_err().There is no other reason. However, dev_err() seems more appropriate than pr_err(). I will replace pr_err() with dev_err(urb->dev->dev, "SG is ...") Is it ok? Regards Suwan Kim
On 8/6/19 9:32 AM, Suwan Kim wrote: > On Tue, Aug 06, 2019 at 09:11:30AM -0600, shuah wrote: >> On 8/6/19 6:31 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. >>> >>> 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. >>> >>> 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). >>> And vhci_map_urb_for_dma() prevents isoc pipe from using SG as >>> hcd_map_urb_for_dma() does. >>> >>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> >>> --- >>> v3 - v4: >>> - Replace WARN_ON() with pr_err() in the error path. >>> >>> v2 - v3 >>> - Move setting URB_DMA_MAP_SG flag to the patch 2. >>> - Prevent isoc pipe from using SG buffer. >>> >>> v1 - v2 >>> - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell >>> stub driver to use SG buffer. >>> --- >>> 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..429e4e989f38 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) >>> +{ >>> + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) { >>> + pr_err("SG is not supported for isochronous transfer\n"); >> >> Any reason to not use dev_err()? > > Because some codes in vhci_hcd.c use pr_err().There is no other > reason. However, dev_err() seems more appropriate than pr_err(). > I will replace pr_err() with dev_err(urb->dev->dev, "SG is ...") > Is it ok? > Please. This way we will have the device information. pr_err() won't us that. In general I prefer dev_* if dev is available in the code path, which is the case here. thanks, -- Shuah
On Tue, Aug 06, 2019 at 09:38:54AM -0600, shuah wrote: > On 8/6/19 9:32 AM, Suwan Kim wrote: > > On Tue, Aug 06, 2019 at 09:11:30AM -0600, shuah wrote: > > > On 8/6/19 6:31 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. > > > > > > > > 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. > > > > > > > > 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). > > > > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > > > > hcd_map_urb_for_dma() does. > > > > > > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > > > > --- > > > > v3 - v4: > > > > - Replace WARN_ON() with pr_err() in the error path. > > > > > > > > v2 - v3 > > > > - Move setting URB_DMA_MAP_SG flag to the patch 2. > > > > - Prevent isoc pipe from using SG buffer. > > > > > > > > v1 - v2 > > > > - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell > > > > stub driver to use SG buffer. > > > > --- > > > > 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..429e4e989f38 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) > > > > +{ > > > > + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) { > > > > + pr_err("SG is not supported for isochronous transfer\n"); > > > > > > Any reason to not use dev_err()? > > > > Because some codes in vhci_hcd.c use pr_err().There is no other > > reason. However, dev_err() seems more appropriate than pr_err(). > > I will replace pr_err() with dev_err(urb->dev->dev, "SG is ...") > > Is it ok? > > > > Please. This way we will have the device information. pr_err() > won't us that. In general I prefer dev_* if dev is available in > the code path, which is the case here. Ok. I will resend v5. Regards Suwan Kim
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 000ab7225717..429e4e989f38 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) +{ + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) { + pr_err("SG is not supported for isochronous transfer\n"); + return -EINVAL; + } + + 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. 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. 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). And vhci_map_urb_for_dma() prevents isoc pipe from using SG as hcd_map_urb_for_dma() does. Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> --- v3 - v4: - Replace WARN_ON() with pr_err() in the error path. v2 - v3 - Move setting URB_DMA_MAP_SG flag to the patch 2. - Prevent isoc pipe from using SG buffer. v1 - v2 - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell stub driver to use SG buffer. --- drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)