* [PATCH] xhci: Set DMA parameters appropriately
@ 2017-10-11 13:56 ` Robin Murphy
2017-10-13 8:15 ` Marek Szyprowski
0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2017-10-11 13:56 UTC (permalink / raw)
To: mathias.nyman, gregkh; +Cc: linux-usb, linux-kernel, hch, m.szyprowski, iommu
xHCI requires that data buffers do not cross 64KB boundaries (and are
thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
already split their input buffers into individual TRBs as necessary,
it's still a good idea to advertise the limitations via the standard DMA
API mechanism, so that most producers like the block layer and the DMA
mapping implementations can lay things out correctly to begin with.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/usb/host/xhci.c | 4 ++++
drivers/usb/host/xhci.h | 3 +++
2 files changed, 7 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 74b4500641c2..1e7e1e3d8c48 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
}
+ dev->dma_parms = &xhci->dma_parms;
+ dma_set_max_seg_size(dev, SZ_64K);
+ dma_set_seg_boundary(dev, SZ_64K - 1);
+
xhci_dbg(xhci, "Calling HCD init\n");
/* Initialize HCD and host controller data structures. */
retval = xhci_init(hcd);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7ef69ea0b480..afcae4cc908d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1767,6 +1767,9 @@ struct xhci_hcd {
struct dma_pool *small_streams_pool;
struct dma_pool *medium_streams_pool;
+ /* DMA alignment restrictions */
+ struct device_dma_parameters dma_parms;
+
/* Host controller watchdog timer structures */
unsigned int xhc_state;
--
2.13.4.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xhci: Set DMA parameters appropriately
2017-10-11 13:56 ` [PATCH] xhci: Set DMA parameters appropriately Robin Murphy
@ 2017-10-13 8:15 ` Marek Szyprowski
2017-10-13 10:48 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Marek Szyprowski @ 2017-10-13 8:15 UTC (permalink / raw)
To: Robin Murphy, mathias.nyman, gregkh; +Cc: linux-usb, linux-kernel, hch, iommu
Hi Robin,
On 2017-10-11 15:56, Robin Murphy wrote:
> xHCI requires that data buffers do not cross 64KB boundaries (and are
> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
> already split their input buffers into individual TRBs as necessary,
> it's still a good idea to advertise the limitations via the standard DMA
> API mechanism, so that most producers like the block layer and the DMA
> mapping implementations can lay things out correctly to begin with.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/usb/host/xhci.c | 4 ++++
> drivers/usb/host/xhci.h | 3 +++
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 74b4500641c2..1e7e1e3d8c48 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
> dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> }
>
> + dev->dma_parms = &xhci->dma_parms;
> + dma_set_max_seg_size(dev, SZ_64K);
> + dma_set_seg_boundary(dev, SZ_64K - 1);
> +
> xhci_dbg(xhci, "Calling HCD init\n");
> /* Initialize HCD and host controller data structures. */
> retval = xhci_init(hcd);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 7ef69ea0b480..afcae4cc908d 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
> struct dma_pool *small_streams_pool;
> struct dma_pool *medium_streams_pool;
>
> + /* DMA alignment restrictions */
> + struct device_dma_parameters dma_parms;
> +
> /* Host controller watchdog timer structures */
> unsigned int xhc_state;
>
Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
that when driver gets removed and xhci_hcd is freed, the dma_parms will
point to freed memory. Maybe it would make sense to clear dev->dma_parms
somewhere or definitely change the way dma_parms are allocated?
On the other hand 64K is the default segment size if no dma_parms are
provided, so there is very little value added by this patch.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xhci: Set DMA parameters appropriately
@ 2017-10-13 10:48 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2017-10-13 10:48 UTC (permalink / raw)
To: Marek Szyprowski, mathias.nyman, gregkh
Cc: linux-usb, linux-kernel, hch, iommu
Hi Marek,
On 13/10/17 09:15, Marek Szyprowski wrote:
> Hi Robin,
>
> On 2017-10-11 15:56, Robin Murphy wrote:
>> xHCI requires that data buffers do not cross 64KB boundaries (and are
>> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
>> already split their input buffers into individual TRBs as necessary,
>> it's still a good idea to advertise the limitations via the standard DMA
>> API mechanism, so that most producers like the block layer and the DMA
>> mapping implementations can lay things out correctly to begin with.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/usb/host/xhci.c | 4 ++++
>> drivers/usb/host/xhci.h | 3 +++
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 74b4500641c2..1e7e1e3d8c48 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>> xhci_get_quirks_t get_quirks)
>> dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>> }
>> + dev->dma_parms = &xhci->dma_parms;
>> + dma_set_max_seg_size(dev, SZ_64K);
>> + dma_set_seg_boundary(dev, SZ_64K - 1);
>> +
>> xhci_dbg(xhci, "Calling HCD init\n");
>> /* Initialize HCD and host controller data structures. */
>> retval = xhci_init(hcd);
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 7ef69ea0b480..afcae4cc908d 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
>> struct dma_pool *small_streams_pool;
>> struct dma_pool *medium_streams_pool;
>> + /* DMA alignment restrictions */
>> + struct device_dma_parameters dma_parms;
>> +
>> /* Host controller watchdog timer structures */
>> unsigned int xhc_state;
>>
>
> Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
> that when driver gets removed and xhci_hcd is freed, the dma_parms will
> point to freed memory. Maybe it would make sense to clear dev->dma_parms
> somewhere or definitely change the way dma_parms are allocated?
AFAICS it lives until the last usb_put_hcd() call, which is pretty much
the last thing in the drivers' .remove paths, so it looks to follow the
standard paradigm evidenced by other dma_parms users. Any dangling
pointer after the driver has been unbound will be reinitialised by a
subsequent probe, and anyone using an unbound device for DMA API calls
is very wrong anyway.
> On the other hand 64K is the default segment size if no dma_parms are
> provided, so there is very little value added by this patch.
I prefer to explicitly set the segment size for cleanliness and to
emphasize the difference between "whatever the default value is is fine"
and "we have a real hardware limit of 64K". What really matters here
though is the non-default segment boundary mask - that's the motiviation
for the patch.
Robin.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xhci: Set DMA parameters appropriately
@ 2017-10-13 10:48 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2017-10-13 10:48 UTC (permalink / raw)
To: Marek Szyprowski, mathias.nyman-ral2JQCrhuEAvxtiuMwx3w,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, hch-jcswGhMUV9g
Hi Marek,
On 13/10/17 09:15, Marek Szyprowski wrote:
> Hi Robin,
>
> On 2017-10-11 15:56, Robin Murphy wrote:
>> xHCI requires that data buffers do not cross 64KB boundaries (and are
>> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
>> already split their input buffers into individual TRBs as necessary,
>> it's still a good idea to advertise the limitations via the standard DMA
>> API mechanism, so that most producers like the block layer and the DMA
>> mapping implementations can lay things out correctly to begin with.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/usb/host/xhci.c | 4 ++++
>> drivers/usb/host/xhci.h | 3 +++
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 74b4500641c2..1e7e1e3d8c48 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>> xhci_get_quirks_t get_quirks)
>> dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>> }
>> + dev->dma_parms = &xhci->dma_parms;
>> + dma_set_max_seg_size(dev, SZ_64K);
>> + dma_set_seg_boundary(dev, SZ_64K - 1);
>> +
>> xhci_dbg(xhci, "Calling HCD init\n");
>> /* Initialize HCD and host controller data structures. */
>> retval = xhci_init(hcd);
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 7ef69ea0b480..afcae4cc908d 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
>> struct dma_pool *small_streams_pool;
>> struct dma_pool *medium_streams_pool;
>> + /* DMA alignment restrictions */
>> + struct device_dma_parameters dma_parms;
>> +
>> /* Host controller watchdog timer structures */
>> unsigned int xhc_state;
>>
>
> Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
> that when driver gets removed and xhci_hcd is freed, the dma_parms will
> point to freed memory. Maybe it would make sense to clear dev->dma_parms
> somewhere or definitely change the way dma_parms are allocated?
AFAICS it lives until the last usb_put_hcd() call, which is pretty much
the last thing in the drivers' .remove paths, so it looks to follow the
standard paradigm evidenced by other dma_parms users. Any dangling
pointer after the driver has been unbound will be reinitialised by a
subsequent probe, and anyone using an unbound device for DMA API calls
is very wrong anyway.
> On the other hand 64K is the default segment size if no dma_parms are
> provided, so there is very little value added by this patch.
I prefer to explicitly set the segment size for cleanliness and to
emphasize the difference between "whatever the default value is is fine"
and "we have a real hardware limit of 64K". What really matters here
though is the non-default segment boundary mask - that's the motiviation
for the patch.
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xhci: Set DMA parameters appropriately
@ 2017-10-17 12:05 ` Marek Szyprowski
0 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2017-10-17 12:05 UTC (permalink / raw)
To: Robin Murphy, mathias.nyman, gregkh; +Cc: linux-usb, linux-kernel, hch, iommu
Hi Robin,
On 2017-10-13 12:48, Robin Murphy wrote:
> On 13/10/17 09:15, Marek Szyprowski wrote:
>> On 2017-10-11 15:56, Robin Murphy wrote:
>>> xHCI requires that data buffers do not cross 64KB boundaries (and are
>>> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
>>> already split their input buffers into individual TRBs as necessary,
>>> it's still a good idea to advertise the limitations via the standard DMA
>>> API mechanism, so that most producers like the block layer and the DMA
>>> mapping implementations can lay things out correctly to begin with.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>> drivers/usb/host/xhci.c | 4 ++++
>>> drivers/usb/host/xhci.h | 3 +++
>>> 2 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 74b4500641c2..1e7e1e3d8c48 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>>> xhci_get_quirks_t get_quirks)
>>> dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>> }
>>> + dev->dma_parms = &xhci->dma_parms;
>>> + dma_set_max_seg_size(dev, SZ_64K);
>>> + dma_set_seg_boundary(dev, SZ_64K - 1);
>>> +
>>> xhci_dbg(xhci, "Calling HCD init\n");
>>> /* Initialize HCD and host controller data structures. */
>>> retval = xhci_init(hcd);
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 7ef69ea0b480..afcae4cc908d 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
>>> struct dma_pool *small_streams_pool;
>>> struct dma_pool *medium_streams_pool;
>>> + /* DMA alignment restrictions */
>>> + struct device_dma_parameters dma_parms;
>>> +
>>> /* Host controller watchdog timer structures */
>>> unsigned int xhc_state;
>>>
>> Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
>> that when driver gets removed and xhci_hcd is freed, the dma_parms will
>> point to freed memory. Maybe it would make sense to clear dev->dma_parms
>> somewhere or definitely change the way dma_parms are allocated?
> AFAICS it lives until the last usb_put_hcd() call, which is pretty much
> the last thing in the drivers' .remove paths, so it looks to follow the
> standard paradigm evidenced by other dma_parms users. Any dangling
> pointer after the driver has been unbound will be reinitialised by a
> subsequent probe, and anyone using an unbound device for DMA API calls
> is very wrong anyway.
Okay.
>> On the other hand 64K is the default segment size if no dma_parms are
>> provided, so there is very little value added by this patch.
> I prefer to explicitly set the segment size for cleanliness and to
> emphasize the difference between "whatever the default value is is fine"
> and "we have a real hardware limit of 64K". What really matters here
> though is the non-default segment boundary mask - that's the motiviation
> for the patch.
Okay. What about other type of USB HCD (EHCI, OHCI, UHCI)?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xhci: Set DMA parameters appropriately
@ 2017-10-17 12:05 ` Marek Szyprowski
0 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2017-10-17 12:05 UTC (permalink / raw)
To: Robin Murphy, mathias.nyman-ral2JQCrhuEAvxtiuMwx3w,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, hch-jcswGhMUV9g
Hi Robin,
On 2017-10-13 12:48, Robin Murphy wrote:
> On 13/10/17 09:15, Marek Szyprowski wrote:
>> On 2017-10-11 15:56, Robin Murphy wrote:
>>> xHCI requires that data buffers do not cross 64KB boundaries (and are
>>> thus at most 64KB long as well) - whilst xhci_queue_{bulk,isoc}_tx()
>>> already split their input buffers into individual TRBs as necessary,
>>> it's still a good idea to advertise the limitations via the standard DMA
>>> API mechanism, so that most producers like the block layer and the DMA
>>> mapping implementations can lay things out correctly to begin with.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>> drivers/usb/host/xhci.c | 4 ++++
>>> drivers/usb/host/xhci.h | 3 +++
>>> 2 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 74b4500641c2..1e7e1e3d8c48 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -4883,6 +4883,10 @@ int xhci_gen_setup(struct usb_hcd *hcd,
>>> xhci_get_quirks_t get_quirks)
>>> dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>> }
>>> + dev->dma_parms = &xhci->dma_parms;
>>> + dma_set_max_seg_size(dev, SZ_64K);
>>> + dma_set_seg_boundary(dev, SZ_64K - 1);
>>> +
>>> xhci_dbg(xhci, "Calling HCD init\n");
>>> /* Initialize HCD and host controller data structures. */
>>> retval = xhci_init(hcd);
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 7ef69ea0b480..afcae4cc908d 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1767,6 +1767,9 @@ struct xhci_hcd {
>>> struct dma_pool *small_streams_pool;
>>> struct dma_pool *medium_streams_pool;
>>> + /* DMA alignment restrictions */
>>> + struct device_dma_parameters dma_parms;
>>> +
>>> /* Host controller watchdog timer structures */
>>> unsigned int xhc_state;
>>>
>> Are you sure that xhci_hcd life time is proper to keep dma_parms? It looks
>> that when driver gets removed and xhci_hcd is freed, the dma_parms will
>> point to freed memory. Maybe it would make sense to clear dev->dma_parms
>> somewhere or definitely change the way dma_parms are allocated?
> AFAICS it lives until the last usb_put_hcd() call, which is pretty much
> the last thing in the drivers' .remove paths, so it looks to follow the
> standard paradigm evidenced by other dma_parms users. Any dangling
> pointer after the driver has been unbound will be reinitialised by a
> subsequent probe, and anyone using an unbound device for DMA API calls
> is very wrong anyway.
Okay.
>> On the other hand 64K is the default segment size if no dma_parms are
>> provided, so there is very little value added by this patch.
> I prefer to explicitly set the segment size for cleanliness and to
> emphasize the difference between "whatever the default value is is fine"
> and "we have a real hardware limit of 64K". What really matters here
> though is the non-default segment boundary mask - that's the motiviation
> for the patch.
Okay. What about other type of USB HCD (EHCI, OHCI, UHCI)?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-17 12:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20171011135703epcas1p246932a3a94ed2f137c0187bdab883ad8@epcas1p2.samsung.com>
2017-10-11 13:56 ` [PATCH] xhci: Set DMA parameters appropriately Robin Murphy
2017-10-13 8:15 ` Marek Szyprowski
2017-10-13 10:48 ` Robin Murphy
2017-10-13 10:48 ` Robin Murphy
2017-10-17 12:05 ` Marek Szyprowski
2017-10-17 12:05 ` Marek Szyprowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.