Now not all DMA paremters configured properly for "xhci-hcd" platform
device which is created manually. For example: dma_pfn_offset, dam_ops
and iommu configuration will not corresponds "dwc3" devices
configuration. As result, this will cause problems like wrong DMA
addresses translation on platforms with LPAE enabled like Keystone 2.
When platform is using DT boot mode the DMA configuration will be
parsed and applied from DT, so, to fix this issue, reuse
of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
from DWC3 device node.
Cc: David Fisher <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: "Thang Q. Nguyen" <[email protected]>
Cc: Yoshihiro Shimoda <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/usb/dwc3/host.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..93c8ef9 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -17,6 +17,7 @@
#include <linux/platform_device.h>
#include <linux/usb/xhci_pdriver.h>
+#include <linux/of_device.h>
#include "core.h"
@@ -32,12 +33,7 @@ int dwc3_host_init(struct dwc3 *dwc)
return -ENOMEM;
}
- dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
-
xhci->dev.parent = dwc->dev;
- xhci->dev.dma_mask = dwc->dev->dma_mask;
- xhci->dev.dma_parms = dwc->dev->dma_parms;
-
dwc->xhci = xhci;
ret = platform_device_add_resources(xhci, dwc->xhci_resources,
@@ -62,6 +58,15 @@ int dwc3_host_init(struct dwc3 *dwc)
phy_create_lookup(dwc->usb3_generic_phy, "usb3-phy",
dev_name(&xhci->dev));
+ if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node) {
+ of_dma_configure(&xhci->dev, dwc->dev->of_node);
+ } else {
+ dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
+
+ xhci->dev.dma_mask = dwc->dev->dma_mask;
+ xhci->dev.dma_parms = dwc->dev->dma_parms;
+ }
+
ret = platform_device_add(xhci);
if (ret) {
dev_err(dwc->dev, "failed to register xHCI device\n");
--
2.8.1
Hi,
Grygorii Strashko <[email protected]> writes:
> Now not all DMA paremters configured properly for "xhci-hcd" platform
> device which is created manually. For example: dma_pfn_offset, dam_ops
> and iommu configuration will not corresponds "dwc3" devices
> configuration. As result, this will cause problems like wrong DMA
> addresses translation on platforms with LPAE enabled like Keystone 2.
>
> When platform is using DT boot mode the DMA configuration will be
> parsed and applied from DT, so, to fix this issue, reuse
> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
> from DWC3 device node.
patch is incomplete. You left out non-DT users which might suffer from
the same problem.
--
balbi
On 04/26/2016 09:17 AM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <[email protected]> writes:
>> Now not all DMA paremters configured properly for "xhci-hcd" platform
>> device which is created manually. For example: dma_pfn_offset, dam_ops
>> and iommu configuration will not corresponds "dwc3" devices
>> configuration. As result, this will cause problems like wrong DMA
>> addresses translation on platforms with LPAE enabled like Keystone 2.
>>
>> When platform is using DT boot mode the DMA configuration will be
>> parsed and applied from DT, so, to fix this issue, reuse
>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
>> from DWC3 device node.
>
> patch is incomplete. You left out non-DT users which might suffer from
> the same problem.
>
Honestly, I don't know how to fix it gracefully for non-DT case.
I can update commit message to mention that this is fix for DT case only.
--
regards,
-grygorii
Hi,
Grygorii Strashko <[email protected]> writes:
> On 04/26/2016 09:17 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Grygorii Strashko <[email protected]> writes:
>>> Now not all DMA paremters configured properly for "xhci-hcd" platform
>>> device which is created manually. For example: dma_pfn_offset, dam_ops
>>> and iommu configuration will not corresponds "dwc3" devices
>>> configuration. As result, this will cause problems like wrong DMA
>>> addresses translation on platforms with LPAE enabled like Keystone 2.
>>>
>>> When platform is using DT boot mode the DMA configuration will be
>>> parsed and applied from DT, so, to fix this issue, reuse
>>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
>>> from DWC3 device node.
>>
>> patch is incomplete. You left out non-DT users which might suffer from
>> the same problem.
>>
>
> Honestly, I don't know how to fix it gracefully for non-DT case.
> I can update commit message to mention that this is fix for DT case only.
no, that won't do :-) There are other users for this driver and they are
all "out-of-compliance" when it comes to DMA usage. Apparently, the
desired behavior is to pass correct device to DMA API which the gadget
side is already doing (see below). For the host side, the fix has to be
more involved.
Frankly, I'd prefer that DMA setup could be inherited from parent
device, then it wouldn't really matter and a bunch of this could be
simplified. Some sort of dma_inherit(struct device *dev, struct device
*parent) would go a long way, IMHO.
8<-------------------------------- cut here ------------------------
commit 2725d6f974c4c268ae5fb746f8e3b33b76135aa8
Author: Felipe Balbi <[email protected]>
Date: Tue Apr 19 16:18:12 2016 +0300
usb: dwc3: use parent device for DMA
our parent device is the one which was initialized
by either PCI or DeviceTree and that's the one which
is configured properly for DMA access.
Instead of copying DMA bits from parent to child,
let's just rely on parent device for the entire DMA
API.
Signed-off-by: Felipe Balbi <[email protected]>
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c050a88c16d4..09e4ff71a50f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -180,7 +180,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj)
static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
struct dwc3_event_buffer *evt)
{
- dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
+ dma_free_coherent(dwc->dev->parent, evt->length, evt->buf, evt->dma);
}
/**
@@ -202,7 +202,7 @@ static struct dwc3_event_buffer *dwc3_alloc_one_event_buffer(struct dwc3 *dwc,
evt->dwc = dwc;
evt->length = length;
- evt->buf = dma_alloc_coherent(dwc->dev, length,
+ evt->buf = dma_alloc_coherent(dwc->dev->parent, length,
&evt->dma, GFP_KERNEL);
if (!evt->buf)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 143deb420481..c78238dc76fc 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -967,7 +967,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
u32 transfer_size = 0;
u32 maxpacket;
- ret = usb_gadget_map_request(&dwc->gadget, &req->request,
+ ret = usb_gadget_map_request_by_dev(dwc->dev->parent, &req->request,
dep->number);
if (ret) {
dwc3_trace(trace_dwc3_ep0, "failed to map request\n");
@@ -995,7 +995,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
dwc->ep0_bounce_addr, transfer_size,
DWC3_TRBCTL_CONTROL_DATA, false);
} else {
- ret = usb_gadget_map_request(&dwc->gadget, &req->request,
+ ret = usb_gadget_map_request_by_dev(dwc->dev->parent, &req->request,
dep->number);
if (ret) {
dwc3_trace(trace_dwc3_ep0, "failed to map request\n");
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 88fd30bf0c46..6929775bde57 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -191,7 +191,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
if (dwc->ep0_bounced && dep->number == 0)
dwc->ep0_bounced = false;
else
- usb_gadget_unmap_request(&dwc->gadget, &req->request,
+ usb_gadget_unmap_request_by_dev(dwc->dev->parent, &req->request,
req->direction);
trace_dwc3_gadget_giveback(req);
@@ -335,7 +335,7 @@ static int dwc3_alloc_trb_pool(struct dwc3_ep *dep)
if (dep->trb_pool)
return 0;
- dep->trb_pool = dma_alloc_coherent(dwc->dev,
+ dep->trb_pool = dma_alloc_coherent(dwc->dev->parent,
sizeof(struct dwc3_trb) * DWC3_TRB_NUM,
&dep->trb_pool_dma, GFP_KERNEL);
if (!dep->trb_pool) {
@@ -351,7 +351,8 @@ static void dwc3_free_trb_pool(struct dwc3_ep *dep)
{
struct dwc3 *dwc = dep->dwc;
- dma_free_coherent(dwc->dev, sizeof(struct dwc3_trb) * DWC3_TRB_NUM,
+ dma_free_coherent(dwc->dev->parent,
+ sizeof(struct dwc3_trb) * DWC3_TRB_NUM,
dep->trb_pool, dep->trb_pool_dma);
dep->trb_pool = NULL;
@@ -972,7 +973,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, u16 cmd_param,
* here and stop, unmap, free and del each of the linked
* requests instead of what we do now.
*/
- usb_gadget_unmap_request(&dwc->gadget, &req->request,
+ usb_gadget_unmap_request_by_dev(dwc->dev->parent, &req->request,
req->direction);
list_del(&req->list);
return ret;
@@ -1057,7 +1058,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
* This will also avoid Host cancelling URBs due to too
* many NAKs.
*/
- ret = usb_gadget_map_request(&dwc->gadget, &req->request,
+ ret = usb_gadget_map_request_by_dev(dwc->dev->parent, &req->request,
dep->direction);
if (ret)
return ret;
@@ -2734,7 +2735,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
{
int ret;
- dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
+ dwc->ctrl_req = dma_alloc_coherent(dwc->dev->parent,
+ sizeof(*dwc->ctrl_req),
&dwc->ctrl_req_addr, GFP_KERNEL);
if (!dwc->ctrl_req) {
dev_err(dwc->dev, "failed to allocate ctrl request\n");
@@ -2742,7 +2744,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
goto err0;
}
- dwc->ep0_trb = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ep0_trb) * 2,
+ dwc->ep0_trb = dma_alloc_coherent(dwc->dev->parent,
+ sizeof(*dwc->ep0_trb) * 2,
&dwc->ep0_trb_addr, GFP_KERNEL);
if (!dwc->ep0_trb) {
dev_err(dwc->dev, "failed to allocate ep0 trb\n");
@@ -2756,7 +2759,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
goto err2;
}
- dwc->ep0_bounce = dma_alloc_coherent(dwc->dev,
+ dwc->ep0_bounce = dma_alloc_coherent(dwc->dev->parent,
DWC3_EP0_BOUNCE_SIZE, &dwc->ep0_bounce_addr,
GFP_KERNEL);
if (!dwc->ep0_bounce) {
@@ -2828,18 +2831,18 @@ err5:
err4:
dwc3_gadget_free_endpoints(dwc);
- dma_free_coherent(dwc->dev, DWC3_EP0_BOUNCE_SIZE,
+ dma_free_coherent(dwc->dev->parent, DWC3_EP0_BOUNCE_SIZE,
dwc->ep0_bounce, dwc->ep0_bounce_addr);
err3:
kfree(dwc->setup_buf);
err2:
- dma_free_coherent(dwc->dev, sizeof(*dwc->ep0_trb),
+ dma_free_coherent(dwc->dev->parent, sizeof(*dwc->ep0_trb),
dwc->ep0_trb, dwc->ep0_trb_addr);
err1:
- dma_free_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
+ dma_free_coherent(dwc->dev->parent, sizeof(*dwc->ctrl_req),
dwc->ctrl_req, dwc->ctrl_req_addr);
err0:
@@ -2854,16 +2857,16 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
dwc3_gadget_free_endpoints(dwc);
- dma_free_coherent(dwc->dev, DWC3_EP0_BOUNCE_SIZE,
+ dma_free_coherent(dwc->dev->parent, DWC3_EP0_BOUNCE_SIZE,
dwc->ep0_bounce, dwc->ep0_bounce_addr);
kfree(dwc->setup_buf);
kfree(dwc->zlp_buf);
- dma_free_coherent(dwc->dev, sizeof(*dwc->ep0_trb),
+ dma_free_coherent(dwc->dev->parent, sizeof(*dwc->ep0_trb),
dwc->ep0_trb, dwc->ep0_trb_addr);
- dma_free_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
+ dma_free_coherent(dwc->dev->parent, sizeof(*dwc->ctrl_req),
dwc->ctrl_req, dwc->ctrl_req_addr);
}
--
balbi
On 04/27/2016 08:41 AM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <[email protected]> writes:
>> On 04/26/2016 09:17 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Grygorii Strashko <[email protected]> writes:
>>>> Now not all DMA paremters configured properly for "xhci-hcd" platform
>>>> device which is created manually. For example: dma_pfn_offset, dam_ops
>>>> and iommu configuration will not corresponds "dwc3" devices
>>>> configuration. As result, this will cause problems like wrong DMA
>>>> addresses translation on platforms with LPAE enabled like Keystone 2.
>>>>
>>>> When platform is using DT boot mode the DMA configuration will be
>>>> parsed and applied from DT, so, to fix this issue, reuse
>>>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
>>>> from DWC3 device node.
>>>
>>> patch is incomplete. You left out non-DT users which might suffer from
>>> the same problem.
>>>
>>
>> Honestly, I don't know how to fix it gracefully for non-DT case.
>> I can update commit message to mention that this is fix for DT case only.
>
> no, that won't do :-) There are other users for this driver and they are
> all "out-of-compliance" when it comes to DMA usage. Apparently, the
> desired behavior is to pass correct device to DMA API which the gadget
> side is already doing (see below). For the host side, the fix has to be
> more involved.
>
> Frankly, I'd prefer that DMA setup could be inherited from parent
> device, then it wouldn't really matter and a bunch of this could be
> simplified. Some sort of dma_inherit(struct device *dev, struct device
> *parent) would go a long way, IMHO.
>
> 8<-------------------------------- cut here ------------------------
> commit 2725d6f974c4c268ae5fb746f8e3b33b76135aa8
> Author: Felipe Balbi <[email protected]>
> Date: Tue Apr 19 16:18:12 2016 +0300
>
> usb: dwc3: use parent device for DMA
>
> our parent device is the one which was initialized
> by either PCI or DeviceTree and that's the one which
> is configured properly for DMA access.
>
> Instead of copying DMA bits from parent to child,
> let's just rely on parent device for the entire DMA
> API.
1) Patch I've sent fixes "xhci-hcd" platform device and not dwc3 core.
On TI boards dwc3 core devices are created from DT and so, I do not see
any problems with dwc3 core. Problem is with xhci.
2) there is minimum one dtsi file where dwc3 ("snps,dwc3") does not have parent device
ls1021a.dtsi (and 5 dtsi in arm64 folder)
>
> Signed-off-by: Felipe Balbi <[email protected]>
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c050a88c16d4..09e4ff71a50f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -180,7 +180,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj)
> static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
> struct dwc3_event_buffer *evt)
> {
> - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> + dma_free_coherent(dwc->dev->parent, evt->length, evt->buf, evt->dma);
> }
[...]
--
regards,
-grygorii
On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote:
> Grygorii Strashko <[email protected]> writes:
> > On 04/26/2016 09:17 AM, Felipe Balbi wrote:
> >> Grygorii Strashko <[email protected]> writes:
> >>> Now not all DMA paremters configured properly for "xhci-hcd" platform
> >>> device which is created manually. For example: dma_pfn_offset, dam_ops
> >>> and iommu configuration will not corresponds "dwc3" devices
> >>> configuration. As result, this will cause problems like wrong DMA
> >>> addresses translation on platforms with LPAE enabled like Keystone 2.
> >>>
> >>> When platform is using DT boot mode the DMA configuration will be
> >>> parsed and applied from DT, so, to fix this issue, reuse
> >>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
> >>> from DWC3 device node.
> >>
> >> patch is incomplete. You left out non-DT users which might suffer from
> >> the same problem.
> >
> > Honestly, I don't know how to fix it gracefully for non-DT case.
> > I can update commit message to mention that this is fix for DT case only.
>
> no, that won't do :-) There are other users for this driver and they are
> all "out-of-compliance" when it comes to DMA usage. Apparently, the
> desired behavior is to pass correct device to DMA API which the gadget
> side is already doing (see below). For the host side, the fix has to be
> more involved.
>
> Frankly, I'd prefer that DMA setup could be inherited from parent
> device, then it wouldn't really matter and a bunch of this could be
> simplified. Some sort of dma_inherit(struct device *dev, struct device
> *parent) would go a long way, IMHO.
I would be in favour of a dma_inherit() function as well. We could hack
something up in the arch code (like below) but I would rather prefer an
explicit dma_inherit() call by drivers creating such devices.
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ba437f090a74..ea6fb9b0e8fa 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
{
- if (dev && dev->archdata.dma_ops)
- return dev->archdata.dma_ops;
+ while (dev) {
+ if (dev->archdata.dma_ops)
+ return dev->archdata.dma_ops;
+ dev = dev->parent;
+ }
/*
* We expect no ISA devices, and all other DMA masters are expected to
--
Catalin
On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
>
> I would be in favour of a dma_inherit() function as well. We could hack
> something up in the arch code (like below) but I would rather prefer an
> explicit dma_inherit() call by drivers creating such devices.
>
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index ba437f090a74..ea6fb9b0e8fa 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>
> static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> {
> - if (dev && dev->archdata.dma_ops)
> - return dev->archdata.dma_ops;
> + while (dev) {
> + if (dev->archdata.dma_ops)
> + return dev->archdata.dma_ops;
> + dev = dev->parent;
> + }
I think this would be a very bad idea: we don't want to have random
devices be able to perform DMA just because their parent devices
have been set up that way.
Arnd
On 04/27/2016 04:59 PM, Catalin Marinas wrote:
> On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote:
>> Grygorii Strashko <[email protected]> writes:
>>> On 04/26/2016 09:17 AM, Felipe Balbi wrote:
>>>> Grygorii Strashko <[email protected]> writes:
>>>>> Now not all DMA paremters configured properly for "xhci-hcd" platform
>>>>> device which is created manually. For example: dma_pfn_offset, dam_ops
>>>>> and iommu configuration will not corresponds "dwc3" devices
>>>>> configuration. As result, this will cause problems like wrong DMA
>>>>> addresses translation on platforms with LPAE enabled like Keystone 2.
>>>>>
>>>>> When platform is using DT boot mode the DMA configuration will be
>>>>> parsed and applied from DT, so, to fix this issue, reuse
>>>>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
>>>>> from DWC3 device node.
>>>>
>>>> patch is incomplete. You left out non-DT users which might suffer from
>>>> the same problem.
>>>
>>> Honestly, I don't know how to fix it gracefully for non-DT case.
>>> I can update commit message to mention that this is fix for DT case only.
>>
>> no, that won't do :-) There are other users for this driver and they are
>> all "out-of-compliance" when it comes to DMA usage. Apparently, the
>> desired behavior is to pass correct device to DMA API which the gadget
>> side is already doing (see below). For the host side, the fix has to be
>> more involved.
>>
>> Frankly, I'd prefer that DMA setup could be inherited from parent
>> device, then it wouldn't really matter and a bunch of this could be
>> simplified. Some sort of dma_inherit(struct device *dev, struct device
>> *parent) would go a long way, IMHO.
>
> I would be in favour of a dma_inherit() function as well. We could hack
> something up in the arch code (like below) but I would rather prefer an
> explicit dma_inherit() call by drivers creating such devices.
>
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index ba437f090a74..ea6fb9b0e8fa 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>
> static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> {
> - if (dev && dev->archdata.dma_ops)
> - return dev->archdata.dma_ops;
> + while (dev) {
> + if (dev->archdata.dma_ops)
> + return dev->archdata.dma_ops;
> + dev = dev->parent;
> + }
>
> /*
> * We expect no ISA devices, and all other DMA masters are expected to
>
It's no enough to W/A just dma_ops :(
dma_inherit()...
FYI: http://www.spinics.net/lists/arm-kernel/msg384012.html
Maybe you'll be able to find the way to make it acceptable.
--
regards,
-grygorii
On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> >
> > I would be in favour of a dma_inherit() function as well. We could hack
> > something up in the arch code (like below) but I would rather prefer an
> > explicit dma_inherit() call by drivers creating such devices.
> >
> > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> > index ba437f090a74..ea6fb9b0e8fa 100644
> > --- a/arch/arm64/include/asm/dma-mapping.h
> > +++ b/arch/arm64/include/asm/dma-mapping.h
> > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> >
> > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> > {
> > - if (dev && dev->archdata.dma_ops)
> > - return dev->archdata.dma_ops;
> > + while (dev) {
> > + if (dev->archdata.dma_ops)
> > + return dev->archdata.dma_ops;
> > + dev = dev->parent;
> > + }
>
> I think this would be a very bad idea: we don't want to have random
> devices be able to perform DMA just because their parent devices
> have been set up that way.
I agree, it's a big hack. It would be nice to have a simpler way to do
this in driver code rather than explicitly calling
of_dma_configure/arch_setup_dma_ops as per the original patch in this
thread.
--
Catalin
On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> > >
> > > I would be in favour of a dma_inherit() function as well. We could hack
> > > something up in the arch code (like below) but I would rather prefer an
> > > explicit dma_inherit() call by drivers creating such devices.
> > >
> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> > > index ba437f090a74..ea6fb9b0e8fa 100644
> > > --- a/arch/arm64/include/asm/dma-mapping.h
> > > +++ b/arch/arm64/include/asm/dma-mapping.h
> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> > >
> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> > > {
> > > - if (dev && dev->archdata.dma_ops)
> > > - return dev->archdata.dma_ops;
> > > + while (dev) {
> > > + if (dev->archdata.dma_ops)
> > > + return dev->archdata.dma_ops;
> > > + dev = dev->parent;
> > > + }
> >
> > I think this would be a very bad idea: we don't want to have random
> > devices be able to perform DMA just because their parent devices
> > have been set up that way.
>
> I agree, it's a big hack. It would be nice to have a simpler way to do
> this in driver code rather than explicitly calling
> of_dma_configure/arch_setup_dma_ops as per the original patch in this
> thread.
>
I haven't followed the entire discussion, but what's wrong with passing
around a pointer to a 'struct device *hwdev' that represents the physical
device that does the DMA?
Arnd
Hi,
Arnd Bergmann <[email protected]> writes:
> On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
>> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
>> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
>> > >
>> > > I would be in favour of a dma_inherit() function as well. We could hack
>> > > something up in the arch code (like below) but I would rather prefer an
>> > > explicit dma_inherit() call by drivers creating such devices.
>> > >
>> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>> > > index ba437f090a74..ea6fb9b0e8fa 100644
>> > > --- a/arch/arm64/include/asm/dma-mapping.h
>> > > +++ b/arch/arm64/include/asm/dma-mapping.h
>> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>> > >
>> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
>> > > {
>> > > - if (dev && dev->archdata.dma_ops)
>> > > - return dev->archdata.dma_ops;
>> > > + while (dev) {
>> > > + if (dev->archdata.dma_ops)
>> > > + return dev->archdata.dma_ops;
>> > > + dev = dev->parent;
>> > > + }
>> >
>> > I think this would be a very bad idea: we don't want to have random
>> > devices be able to perform DMA just because their parent devices
>> > have been set up that way.
>>
>> I agree, it's a big hack. It would be nice to have a simpler way to do
>> this in driver code rather than explicitly calling
>> of_dma_configure/arch_setup_dma_ops as per the original patch in this
>> thread.
>
> I haven't followed the entire discussion, but what's wrong with passing
> around a pointer to a 'struct device *hwdev' that represents the physical
> device that does the DMA?
that will likely create duplicated solutions in several drivers and
it'll be a pain to maintain. There's another complication, dwc3 can be
integrated in many different ways. See the device child-parent tree
representations below:
a) with a parent PCI device:
pci_bus_type
- dwc3-pci
- dwc3
- xhci-plat
b) with a parent platform_device (OF):
platform_bus_type
- dwc3-${omap,st,of-simple,exynos,keystone}
- dwc3
- xhci-plat
c) without a parent at all (thanks Grygorii):
platform_bus_type
- dwc3
- xhci-plat
(a) and (b) above are the common cases. The DMA-capable device is
clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
having proper DMA configuration in OF platforms (because of the
unconditional of_dma_configure() during OF device creation) and
xhci-plat not knowing about DMA at all and hardcoding some crappy
defaults.
(c) is the uncommon case which creates some problems. In this case, dwc3
itself is the DMA-capable device and dwc3->dev->parent is the
platform_bus_type itself. Now consider the problem this creates:
i. the patch that I wrote [1] becomes invalid for (c), thanks to
Grygorii for pointing this out before it was too late.
ii. xhci-plat can also be described directly in DT (and is in some
cases). This means that assuming xhci-plat's parent's parent to be the
DMA-capable device is also an invalid assumption.
iii. one might argue that for DT-based platforms *with* a glue layer
((b) above), OF already "copies" some sensible DMA defaults during
device creation. PCI-based systems just don't have the luxury of
creating random PCI devices like that :-) I say it copies because I can
pass *any* struct device_node pointer and it'll just copy that to the
struct device argument. Here's of_dma_configure() to make your life
easier:
void of_dma_configure(struct device *dev, struct device_node *np)
{
u64 dma_addr, paddr, size;
int ret;
bool coherent;
unsigned long offset;
struct iommu_ops *iommu;
/*
* Set default coherent_dma_mask to 32 bit. Drivers are expected to
* setup the correct supported mask.
*/
if (!dev->coherent_dma_mask)
dev->coherent_dma_mask = DMA_BIT_MASK(32);
/*
* Set it to coherent_dma_mask by default if the architecture
* code has not set it.
*/
if (!dev->dma_mask)
dev->dma_mask = &dev->coherent_dma_mask;
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
size = dev->coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
/*
* Add a work around to treat the size as mask + 1 in case
* it is defined in DT as a mask.
*/
if (size & 1) {
dev_warn(dev, "Invalid size 0x%llx for dma-range\n",
size);
size = size + 1;
}
if (!size) {
dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
return;
}
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
}
dev->dma_pfn_offset = offset;
/*
* Limit coherent and dma mask based on size and default mask
* set by the driver.
*/
dev->coherent_dma_mask = min(dev->coherent_dma_mask,
DMA_BIT_MASK(ilog2(dma_addr + size)));
*dev->dma_mask = min((*dev->dma_mask),
DMA_BIT_MASK(ilog2(dma_addr + size)));
coherent = of_dma_is_coherent(np);
dev_dbg(dev, "device is%sdma coherent\n",
coherent ? " " : " not ");
iommu = of_iommu_configure(dev, np);
dev_dbg(dev, "device is%sbehind an iommu\n",
iommu ? " " : " not ");
arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
}
The only clean way to fix this up is with a dma_inherit()-like API which
would allow dwc3's glue layers ((a) and (b) above) to initialize child's
(dwc3) DMA configuration during child's creation. Something like below:
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index adc1e8a624cb..74b599269e2c 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
return -ENOMEM;
}
+ dma_inherit(&dwc->dev, dev);
+
memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
res[0].start = pci_resource_start(pci, 0);
that's all I'm asking for :-) dma_inherit() should, probably, be
arch-specific to handle details like IOMMU (which today sits under
archdata).
without something like this, we're gonna have to resort to tons of
checks trying to find the correct DMA configuration to use in all
possible usage scenarios of dwc3.
Note, however, that I'm using dwc3 as an example, but anywhere you see
manual platform_device creation, there's a potential problem WRT DMA
and/or IOMMU.
[1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=2725d6f974c4c268ae5fb746f8e3b33b76135aa8
--
balbi
On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote:
> Arnd Bergmann <[email protected]> writes:
> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> >> > >
> >> > > I would be in favour of a dma_inherit() function as well. We could hack
> >> > > something up in the arch code (like below) but I would rather prefer an
> >> > > explicit dma_inherit() call by drivers creating such devices.
> >> > >
> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> >> > > index ba437f090a74..ea6fb9b0e8fa 100644
> >> > > --- a/arch/arm64/include/asm/dma-mapping.h
> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h
> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> >> > >
> >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >> > > {
> >> > > - if (dev && dev->archdata.dma_ops)
> >> > > - return dev->archdata.dma_ops;
> >> > > + while (dev) {
> >> > > + if (dev->archdata.dma_ops)
> >> > > + return dev->archdata.dma_ops;
> >> > > + dev = dev->parent;
> >> > > + }
> >> >
> >> > I think this would be a very bad idea: we don't want to have random
> >> > devices be able to perform DMA just because their parent devices
> >> > have been set up that way.
> >>
> >> I agree, it's a big hack. It would be nice to have a simpler way to do
> >> this in driver code rather than explicitly calling
> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this
> >> thread.
> >
> > I haven't followed the entire discussion, but what's wrong with passing
> > around a pointer to a 'struct device *hwdev' that represents the physical
> > device that does the DMA?
>
> that will likely create duplicated solutions in several drivers and
> it'll be a pain to maintain. There's another complication, dwc3 can be
> integrated in many different ways. See the device child-parent tree
> representations below:
>
> a) with a parent PCI device:
>
> pci_bus_type
> - dwc3-pci
> - dwc3
> - xhci-plat
>
> b) with a parent platform_device (OF):
>
> platform_bus_type
> - dwc3-${omap,st,of-simple,exynos,keystone}
> - dwc3
> - xhci-plat
>
> c) without a parent at all (thanks Grygorii):
>
> platform_bus_type
> - dwc3
> - xhci-plat
>
> (a) and (b) above are the common cases. The DMA-capable device is
> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
> having proper DMA configuration in OF platforms (because of the
> unconditional of_dma_configure() during OF device creation) and
> xhci-plat not knowing about DMA at all and hardcoding some crappy
> defaults.
>
> (c) is the uncommon case which creates some problems. In this case, dwc3
> itself is the DMA-capable device and dwc3->dev->parent is the
> platform_bus_type itself. Now consider the problem this creates:
>
> i. the patch that I wrote [1] becomes invalid for (c), thanks to
> Grygorii for pointing this out before it was too late.
>
> ii. xhci-plat can also be described directly in DT (and is in some
> cases). This means that assuming xhci-plat's parent's parent to be the
> DMA-capable device is also an invalid assumption.
>
> iii. one might argue that for DT-based platforms *with* a glue layer
> ((b) above), OF already "copies" some sensible DMA defaults during
> device creation.
But that is exactly the point I was trying to make: instead of copying
all the data into the xhci-plat device, just assign one pointer
inside the xhci-plat device from whoever creates it.
> The only clean way to fix this up is with a dma_inherit()-like API which
> would allow dwc3's glue layers ((a) and (b) above) to initialize child's
> (dwc3) DMA configuration during child's creation. Something like below:
>
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index adc1e8a624cb..74b599269e2c 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
> return -ENOMEM;
> }
>
> + dma_inherit(&dwc->dev, dev);
> +
> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
>
> res[0].start = pci_resource_start(pci, 0);
>
> that's all I'm asking for :-) dma_inherit() should, probably, be
> arch-specific to handle details like IOMMU (which today sits under
> archdata).
It's still wrong: the archdata in the device can contain all sorts of
additional information about how to do DMA, and some of that information
is bus specific, e.g. when your dma_map_ops look like the on sparc:
static inline struct dma_map_ops *get_dma_ops(struct device *dev)
{
#ifdef CONFIG_SPARC_LEON
if (sparc_cpu_model == sparc_leon)
return leon_dma_ops;
#endif
#if defined(CONFIG_SPARC32) && defined(CONFIG_PCI)
if (dev->bus == &pci_bus_type)
return &pci32_dma_ops;
#endif
return dma_ops;
}
There is no way for a device to "inherit" the bus_type of its parent,
so it becomes an architecture specific hack. However, if you change
all the dma operations to work on the actual device that the dma
mapping API knows about, it just works.
I've looked at the usb HCD code now and see this:
struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
struct device *dev, const char *bus_name,
struct usb_hcd *primary_hcd)
{
...
hcd->self.controller = dev;
hcd->self.uses_dma = (dev->dma_mask != NULL);
...
}
What I think we need to do here is ensure that the device that gets
passed here and assigned to hcd->self.controller is the actual DMA
master device, i.e. the pci_device or platform_device that was created
from outside of the xhci stack. This is after all the pointer that
gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
functions.
Arnd
On Wed, 27 Apr 2016, Arnd Bergmann wrote:
> I've looked at the usb HCD code now and see this:
>
> struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> struct device *dev, const char *bus_name,
> struct usb_hcd *primary_hcd)
> {
> ...
> hcd->self.controller = dev;
> hcd->self.uses_dma = (dev->dma_mask != NULL);
> ...
> }
>
> What I think we need to do here is ensure that the device that gets
> passed here and assigned to hcd->self.controller is the actual DMA
> master device, i.e. the pci_device or platform_device that was created
> from outside of the xhci stack. This is after all the pointer that
> gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> functions.
It would be better to add a new field, since self.controller is also
used for lots of other purposes. Something like hcd->self.dma_dev.
Alan Stern
On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
>
> > I've looked at the usb HCD code now and see this:
> >
> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> > struct device *dev, const char *bus_name,
> > struct usb_hcd *primary_hcd)
> > {
> > ...
> > hcd->self.controller = dev;
> > hcd->self.uses_dma = (dev->dma_mask != NULL);
> > ...
> > }
> >
> > What I think we need to do here is ensure that the device that gets
> > passed here and assigned to hcd->self.controller is the actual DMA
> > master device, i.e. the pci_device or platform_device that was created
> > from outside of the xhci stack. This is after all the pointer that
> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> > functions.
>
> It would be better to add a new field, since self.controller is also
> used for lots of other purposes. Something like hcd->self.dma_dev.
>
Ok, fair enough. I only took a brief look and all uses I found were
either for the DMA mapping API or some printk logging.
Arnd
Hi,
Arnd Bergmann <[email protected]> writes:
> On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
>> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
>>
>> > I've looked at the usb HCD code now and see this:
>> >
>> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
>> > struct device *dev, const char *bus_name,
>> > struct usb_hcd *primary_hcd)
>> > {
>> > ...
>> > hcd->self.controller = dev;
>> > hcd->self.uses_dma = (dev->dma_mask != NULL);
>> > ...
>> > }
>> >
>> > What I think we need to do here is ensure that the device that gets
>> > passed here and assigned to hcd->self.controller is the actual DMA
>> > master device, i.e. the pci_device or platform_device that was created
>> > from outside of the xhci stack. This is after all the pointer that
>> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
>> > functions.
>>
>> It would be better to add a new field, since self.controller is also
>> used for lots of other purposes. Something like hcd->self.dma_dev.
>
> Ok, fair enough. I only took a brief look and all uses I found were
> either for the DMA mapping API or some printk logging.
I have a feeling you guys are not considering how the patch to implement
this will look like.
How are you expecting dwc3 to pass a pointer to the DMA device from
dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible :-)
Also, remember that the DMA device for dwc3 is not always
dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting
us to figure that one out ?
I still think dma_inherit() (or something along those lines) is
necessary. Specially when you consider that, as I said previously,
that's pretty much what of_dma_configure() does.
Anyway, I'd really like to see a patch implementing this
hcd->self.dma_dev logic. Consider all the duplication with this
approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its
own. Will that be passed to dwc3 as platform_data from glue layer ? What
about platforms which don't even use a glue layer ?
--
balbi
Hi,
Arnd Bergmann <[email protected]> writes:
> On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote:
>> Arnd Bergmann <[email protected]> writes:
>> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
>> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
>> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
>> >> > >
>> >> > > I would be in favour of a dma_inherit() function as well. We could hack
>> >> > > something up in the arch code (like below) but I would rather prefer an
>> >> > > explicit dma_inherit() call by drivers creating such devices.
>> >> > >
>> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>> >> > > index ba437f090a74..ea6fb9b0e8fa 100644
>> >> > > --- a/arch/arm64/include/asm/dma-mapping.h
>> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h
>> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>> >> > >
>> >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
>> >> > > {
>> >> > > - if (dev && dev->archdata.dma_ops)
>> >> > > - return dev->archdata.dma_ops;
>> >> > > + while (dev) {
>> >> > > + if (dev->archdata.dma_ops)
>> >> > > + return dev->archdata.dma_ops;
>> >> > > + dev = dev->parent;
>> >> > > + }
>> >> >
>> >> > I think this would be a very bad idea: we don't want to have random
>> >> > devices be able to perform DMA just because their parent devices
>> >> > have been set up that way.
>> >>
>> >> I agree, it's a big hack. It would be nice to have a simpler way to do
>> >> this in driver code rather than explicitly calling
>> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this
>> >> thread.
>> >
>> > I haven't followed the entire discussion, but what's wrong with passing
>> > around a pointer to a 'struct device *hwdev' that represents the physical
>> > device that does the DMA?
>>
>> that will likely create duplicated solutions in several drivers and
>> it'll be a pain to maintain. There's another complication, dwc3 can be
>> integrated in many different ways. See the device child-parent tree
>> representations below:
>>
>> a) with a parent PCI device:
>>
>> pci_bus_type
>> - dwc3-pci
>> - dwc3
>> - xhci-plat
>>
>> b) with a parent platform_device (OF):
>>
>> platform_bus_type
>> - dwc3-${omap,st,of-simple,exynos,keystone}
>> - dwc3
>> - xhci-plat
>>
>> c) without a parent at all (thanks Grygorii):
>>
>> platform_bus_type
>> - dwc3
>> - xhci-plat
>>
>> (a) and (b) above are the common cases. The DMA-capable device is
>> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
>> having proper DMA configuration in OF platforms (because of the
>> unconditional of_dma_configure() during OF device creation) and
>> xhci-plat not knowing about DMA at all and hardcoding some crappy
>> defaults.
>>
>> (c) is the uncommon case which creates some problems. In this case, dwc3
>> itself is the DMA-capable device and dwc3->dev->parent is the
>> platform_bus_type itself. Now consider the problem this creates:
>>
>> i. the patch that I wrote [1] becomes invalid for (c), thanks to
>> Grygorii for pointing this out before it was too late.
>>
>> ii. xhci-plat can also be described directly in DT (and is in some
>> cases). This means that assuming xhci-plat's parent's parent to be the
>> DMA-capable device is also an invalid assumption.
>>
>> iii. one might argue that for DT-based platforms *with* a glue layer
>> ((b) above), OF already "copies" some sensible DMA defaults during
>> device creation.
>
> But that is exactly the point I was trying to make: instead of copying
> all the data into the xhci-plat device, just assign one pointer
> inside the xhci-plat device from whoever creates it.
and how do you pass that pointer to xhci-plat from another driver ?
No, platform_data is not an option ;-)
>> The only clean way to fix this up is with a dma_inherit()-like API which
>> would allow dwc3's glue layers ((a) and (b) above) to initialize child's
>> (dwc3) DMA configuration during child's creation. Something like below:
>>
>> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
>> index adc1e8a624cb..74b599269e2c 100644
>> --- a/drivers/usb/dwc3/dwc3-pci.c
>> +++ b/drivers/usb/dwc3/dwc3-pci.c
>> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
>> return -ENOMEM;
>> }
>>
>> + dma_inherit(&dwc->dev, dev);
>> +
>> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
>>
>> res[0].start = pci_resource_start(pci, 0);
>>
>> that's all I'm asking for :-) dma_inherit() should, probably, be
>> arch-specific to handle details like IOMMU (which today sits under
>> archdata).
>
> It's still wrong: the archdata in the device can contain all sorts of
> additional information about how to do DMA, and some of that information
yes, inherit all of that ;-)
> is bus specific, e.g. when your dma_map_ops look like the on sparc:
okay, let me be clear. The underlying bus is still pci_bus_type or
platform_bus_type; that won't change.
> static inline struct dma_map_ops *get_dma_ops(struct device *dev)
> {
> #ifdef CONFIG_SPARC_LEON
> if (sparc_cpu_model == sparc_leon)
> return leon_dma_ops;
> #endif
> #if defined(CONFIG_SPARC32) && defined(CONFIG_PCI)
> if (dev->bus == &pci_bus_type)
> return &pci32_dma_ops;
> #endif
> return dma_ops;
> }
this seems to be a rather fragile assumption, no ? Only works because
*_bus_type declarations are exported. I wonder if this is the only
reason *why* they are exported, probably not...
> There is no way for a device to "inherit" the bus_type of its parent,
I don't want to inherit bus_type, I just want DMA configuration to not
depend on bus_type directly ;-)
--
balbi
On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote:
> Arnd Bergmann <[email protected]> writes:
> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
> >>
> >> > I've looked at the usb HCD code now and see this:
> >> >
> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> >> > struct device *dev, const char *bus_name,
> >> > struct usb_hcd *primary_hcd)
> >> > {
> >> > ...
> >> > hcd->self.controller = dev;
> >> > hcd->self.uses_dma = (dev->dma_mask != NULL);
> >> > ...
> >> > }
> >> >
> >> > What I think we need to do here is ensure that the device that gets
> >> > passed here and assigned to hcd->self.controller is the actual DMA
> >> > master device, i.e. the pci_device or platform_device that was created
> >> > from outside of the xhci stack. This is after all the pointer that
> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> >> > functions.
> >>
> >> It would be better to add a new field, since self.controller is also
> >> used for lots of other purposes. Something like hcd->self.dma_dev.
> >
> > Ok, fair enough. I only took a brief look and all uses I found were
> > either for the DMA mapping API or some printk logging.
>
> I have a feeling you guys are not considering how the patch to implement
> this will look like.
>
> How are you expecting dwc3 to pass a pointer to the DMA device from
> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible
Not any worse than it already is really. It already uses platform_data
for the exact case that is causing the problem here.
> Also, remember that the DMA device for dwc3 is not always
> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting
> us to figure that one out ?
Do you have an example for this? The ones I found here either
create the dwc3 device from PCI or from a platform glue driver.
> I still think dma_inherit() (or something along those lines) is
> necessary. Specially when you consider that, as I said previously,
> that's pretty much what of_dma_configure() does.
As I said, this is not an API that can work in general, because
it makes the assumption that everything related to DMA in a
device can be set in that device itself.
The simplest case where this does not work is a PCI device behind
an IOMMU: when you call dma_map_single() or dma_alloc_coherent(),
the dma_map_ops implementation for the IOMMU has to look at the
PCI device to find out the association with an IOMMU context,
and on most architectures you cannot bind an IOMMU context to
a platform device at all.
> Anyway, I'd really like to see a patch implementing this
> hcd->self.dma_dev logic. Consider all the duplication with this
> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its
> own. Will that be passed to dwc3 as platform_data from glue layer ? What
> about platforms which don't even use a glue layer ?
Let's separate the three problems here.
a) dwc3 creating a "xhci-hcd" platform_device that is not connected
to any proper bus. We can work around that by adding the "self.dma_dev"
pointer and pass that in platform_data. This is really easy, it's
actually less code (and less iffy) than the current implementation of
copying the parent dma mask.
In the long run, this could be solved by doing away with the extra
platform_device, by calling a variant of xhci_probe() from
xhci_plat_probe() like we do for the normal *HCI drivers.
b) dwc3-pci creating a "dwc3" platform_device under the covers. This
is pretty much the exact same problem as a) on another layer. In
the short run, we can pass the device pointer as part of
struct dwc3_platform_data (dwc3-pci is the only such user anway),
and in the long run, it should be easy enough to get rid of the
extra platform device by just calling a variant of dwc3_probe,
which will again simplify the driver
c) some SoCs that have two separate device nodes to describe their
dwc3 xhci. I don't think this is causing any additional problems,
but if we want to make this behave more like other drivers in the
long run or deal with machines that are missing a "dma-ranges"
property in the parent node, we can kill off the
of_platform_populate() hack and instead call dwc3_probe()
directly from the glue drivers as in b), and have that
do for_each_child_of_node() or similar to find the child node.
Arnd
Hi,
Arnd Bergmann <[email protected]> writes:
> On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote:
>> Arnd Bergmann <[email protected]> writes:
>> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
>> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
>> >>
>> >> > I've looked at the usb HCD code now and see this:
>> >> >
>> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
>> >> > struct device *dev, const char *bus_name,
>> >> > struct usb_hcd *primary_hcd)
>> >> > {
>> >> > ...
>> >> > hcd->self.controller = dev;
>> >> > hcd->self.uses_dma = (dev->dma_mask != NULL);
>> >> > ...
>> >> > }
>> >> >
>> >> > What I think we need to do here is ensure that the device that gets
>> >> > passed here and assigned to hcd->self.controller is the actual DMA
>> >> > master device, i.e. the pci_device or platform_device that was created
>> >> > from outside of the xhci stack. This is after all the pointer that
>> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
>> >> > functions.
>> >>
>> >> It would be better to add a new field, since self.controller is also
>> >> used for lots of other purposes. Something like hcd->self.dma_dev.
>> >
>> > Ok, fair enough. I only took a brief look and all uses I found were
>> > either for the DMA mapping API or some printk logging.
>>
>> I have a feeling you guys are not considering how the patch to implement
>> this will look like.
>>
>> How are you expecting dwc3 to pass a pointer to the DMA device from
>> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible
>
> Not any worse than it already is really. It already uses platform_data
> for the exact case that is causing the problem here.
there's no use of platform_data for passing around DMA configuration. By
platform_data I really mean platform_device_add_data().
>> Also, remember that the DMA device for dwc3 is not always
>> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting
>> us to figure that one out ?
>
> Do you have an example for this? The ones I found here either
> create the dwc3 device from PCI or from a platform glue driver.
arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> I still think dma_inherit() (or something along those lines) is
>> necessary. Specially when you consider that, as I said previously,
>> that's pretty much what of_dma_configure() does.
>
> As I said, this is not an API that can work in general, because
> it makes the assumption that everything related to DMA in a
> device can be set in that device itself.
>
> The simplest case where this does not work is a PCI device behind
> an IOMMU: when you call dma_map_single() or dma_alloc_coherent(),
> the dma_map_ops implementation for the IOMMU has to look at the
> PCI device to find out the association with an IOMMU context,
> and on most architectures you cannot bind an IOMMU context to
> a platform device at all.
no, it relies on dev->archdata for IOMMU. In fact, the first "patch"
(more of a hack) I wrote to fix IOMMU with dwc3 on Intel platforms was
to literally memcpy() pci's archdata to dwc3->dev and it worked just
fine with and without IOMMU enabled.
>> Anyway, I'd really like to see a patch implementing this
>> hcd->self.dma_dev logic. Consider all the duplication with this
>> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its
>> own. Will that be passed to dwc3 as platform_data from glue layer ? What
>> about platforms which don't even use a glue layer ?
>
> Let's separate the three problems here.
>
> a) dwc3 creating a "xhci-hcd" platform_device that is not connected
> to any proper bus. We can work around that by adding the "self.dma_dev"
platform_bus_type *is* a proper bus.
> pointer and pass that in platform_data. This is really easy, it's
Sorry but passing a struct device pointer in platform_data is
ridiculous. Not to mention that, as I said before, we can't assume which
device to pass to xhci_plat in the first place. It might be dwc->dev and
it might be dwc->dev->parent.
> actually less code (and less iffy) than the current implementation of
> copying the parent dma mask.
> In the long run, this could be solved by doing away with the extra
> platform_device, by calling a variant of xhci_probe() from
> xhci_plat_probe() like we do for the normal *HCI drivers.
no, that's not something I'll do for dwc3. We have had this talk before
and I'm not giving up the benefits of splitting things to separate
devices.
> b) dwc3-pci creating a "dwc3" platform_device under the covers. This
it's not under the covers at all. It's pretty similar to what MFD
drivers do. It's just not wrapped in a nice API because there's no need
for that.
> is pretty much the exact same problem as a) on another layer. In
> the short run, we can pass the device pointer as part of
> struct dwc3_platform_data (dwc3-pci is the only such user anway),
It's incredible that you'd even suggest this at all. Did we not have a
big trouble to solve on ARM land because of different mach-* passing
function pointers and other pointers from arch/arch/mach-* instead of
abstracting things away. Then came DT to the rescue, a setup where you
can't even pass code or kernel objects ;-)
> and in the long run, it should be easy enough to get rid of the
> extra platform device by just calling a variant of dwc3_probe,
> which will again simplify the driver
also again definitely not something I'll do
> c) some SoCs that have two separate device nodes to describe their
> dwc3 xhci. I don't think this is causing any additional problems,
> but if we want to make this behave more like other drivers in the
> long run or deal with machines that are missing a "dma-ranges"
> property in the parent node, we can kill off the
> of_platform_populate() hack and instead call dwc3_probe()
> directly from the glue drivers as in b), and have that
> do for_each_child_of_node() or similar to find the child node.
no, we cannot.
--
balbi
On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Arnd Bergmann <[email protected]> writes:
> > pointer and pass that in platform_data. This is really easy, it's
>
> Sorry but passing a struct device pointer in platform_data is
> ridiculous. Not to mention that, as I said before, we can't assume which
> device to pass to xhci_plat in the first place. It might be dwc->dev and
> it might be dwc->dev->parent.
+1. Passing an unref-counted struct device through platform data is
totally mad, Arnd you're off your rocker if you think that's a good
idea. What's more is that there's no way to properly refcount the
thing.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote:
> On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Arnd Bergmann <[email protected]> writes:
> > > pointer and pass that in platform_data. This is really easy, it's
> >
> > Sorry but passing a struct device pointer in platform_data is
> > ridiculous. Not to mention that, as I said before, we can't assume which
> > device to pass to xhci_plat in the first place. It might be dwc->dev and
> > it might be dwc->dev->parent.
>
> +1. Passing an unref-counted struct device through platform data is
> totally mad, Arnd you're off your rocker if you think that's a good
> idea. What's more is that there's no way to properly refcount the
> thing.
It's the parent device (or NULL), there is no way it can ever go away as
it's already refcounted through the device subsystem by the creation
of the child device.
I do realize that it's a hack, but the idea is to get rid of that
as soon as possibly by fixing the way the xhci device is probe so
we no longer need to fake a platform_device as the child here and
can just use the device itself.
Arnd
Hi,
Arnd Bergmann <[email protected]> writes:
> On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote:
>> On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote:
>> >
>> > Hi,
>> >
>> > Arnd Bergmann <[email protected]> writes:
>> > > pointer and pass that in platform_data. This is really easy, it's
>> >
>> > Sorry but passing a struct device pointer in platform_data is
>> > ridiculous. Not to mention that, as I said before, we can't assume which
>> > device to pass to xhci_plat in the first place. It might be dwc->dev and
>> > it might be dwc->dev->parent.
>>
>> +1. Passing an unref-counted struct device through platform data is
>> totally mad, Arnd you're off your rocker if you think that's a good
>> idea. What's more is that there's no way to properly refcount the
>> thing.
>
> It's the parent device (or NULL), there is no way it can ever go away as
> it's already refcounted through the device subsystem by the creation
> of the child device.
you're assuming that based on what we have today. We could get into a
situation where we need to use a completely unrelated device and the
problem exists again.
> I do realize that it's a hack, but the idea is to get rid of that
> as soon as possibly by fixing the way the xhci device is probe so
> we no longer need to fake a platform_device as the child here and
> can just use the device itself.
okay, let me try to be extra clear here:
We will *not* remove the extra platform_device because it actually
*does* exist and helps me hide/abstract a bunch of details and make
assumptions about order of certain events. We have already gone through
that in the past when I explained why I wrote dwc3 the way it is; if you
need a refresher, there are mailing list archives for that.
Moreover, this same problem exists for anything under drivers/mfd. It
just so happens that they're usually some i2c or spi device which don't
do DMA by themselves.
--
balbi
On Thu, Sep 22, 2016 at 12:02 AM, Sriram Dash <[email protected]> wrote:
>>From: Arnd Bergmann [mailto:[email protected]]
>>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>>> >From: Arnd Bergmann [mailto:[email protected]] On Wednesday, September
>>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>>
>>> ==============================================================
>>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>>> From: Sriram Dash <[email protected]>
>>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>>> from parent dev
>>>
>>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>>
>>> Signed-off-by: Sriram Dash <[email protected]>
>>> ---
>>> drivers/usb/host/xhci-mem.c | 12 ++++++------
>>> drivers/usb/host/xhci.c | 20 ++++++++++----------
>>> 2 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index 6afe323..79608df 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>
>>All the changes you did to this file seem fine, I completely missed that part.
>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>>> 01d96c9..9a1ff09 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>
> Yes. Some sanity is required over this patch.
Hi Sriram,
Have you been able to do the sanity check on the patch? I will be
good to have the formal patch submitted for integration as soon as
possible because the dwc3 USB functionality has been broken for a
while in upstream kernel.
Regards,
Leo