Re: [xhci:fun-streams-fixes 33/48] drivers/usb/host/xhci-ring.c:1154:29: sparse: incorrect type in assignment (different base types)
Hi,
On 10/17/2013 06:42 PM, Sarah Sharp wrote:
All right, so we have a couple sparse warnings with this patch.
On Thu, Oct 17, 2013 at 01:40:15PM +0800, kbuild test robot wrote:
tree: git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git
fun-streams-fixes
head: e79db70b0f7d028cb9e9cf8ee424a1e92b7d232f
commit: 0f899d55a18ce75eed9dccd2cc2cd6d333f9ff16 [33/48] xhci: For streams the
dequeue ptr must be read from the stream ctx
reproduce: make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/usb/host/xhci-ring.c:1154:29: sparse: incorrect type in assignment
(different base types)
drivers/usb/host/xhci-ring.c:1154:29:expected restricted __le64
[usertype] deq
drivers/usb/host/xhci-ring.c:1154:29:got unsigned long long
drivers/usb/host/xhci-ring.c:1156:29: sparse: incorrect type in assignment
(different base types)
drivers/usb/host/xhci-ring.c:1156:29:expected restricted __le64
[usertype] deq
drivers/usb/host/xhci-ring.c:1156:29:got unsigned long long
drivers/usb/host/xhci-ring.c:1161:65: sparse: restricted __le64 degrades to
integer
drivers/usb/host/xhci-ring.c:1674:19: sparse: restricted __le32 degrades to
integer
vim +1154 drivers/usb/host/xhci-ring.c
1148 } else {
1149 __le64 deq;
1150 /* 4.6.10 deq ptr is written to the stream ctx for
streams */
1151 if (ep->ep_state & EP_HAS_STREAMS) {
1152 struct xhci_stream_ctx *ctx =
1153
&ep->stream_info->stream_ctx_array[stream_id];
1154deq = le64_to_cpu(ctx->stream_ring) &
SCTX_DEQ_MASK;
1155 } else {
1156 deq = le64_to_cpu(ep_ctx->deq) &
~EP_CTX_CYCLE_MASK;
1157 }
1158 xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
1159 "Successful Set TR Deq Ptr cmd, deq = @%08llx",
deq);
1160 if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
1161 ep->queued_deq_ptr) == deq) {
1162 /* Update the ring's dequeue segment and
dequeue pointer
1163 * to reflect the new position.
1164 */
---
0-DAY kernel build testing backend Open Source Technology Center
http://lists.01.org/mailman/listinfo/kbuild Intel Corporation
Looking at the patch itself:
+ __le64 deq;
+ /* 4.6.10 deq ptr is written to the stream ctx for streams */
+ if (ep->ep_state & EP_HAS_STREAMS) {
+ struct xhci_stream_ctx *ctx =
+ &ep->stream_info->stream_ctx_array[stream_id];
+ deq = le64_to_cpu(ctx->stream_ring) & SCTX_DEQ_MASK;
+ } else {
+ deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
+ }
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
- "Successful Set TR Deq Ptr cmd, deq = @%08llx",
-le64_to_cpu(ep_ctx->deq));
- if (xhci_trb_virt_to_dma(dev->eps[ep_index].queued_deq_seg,
-dev->eps[ep_index].queued_deq_ptr) ==
- (le64_to_cpu(ep_ctx->deq) & ~(EP_CTX_CYCLE_MASK))) {
+ "Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
+ if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
+ep->queued_deq_ptr) == deq) {
/* Update the ring's dequeue segment and dequeue
pointer
* to reflect the new position.
*/
I don't think it makes sense to use __le64 for the deq variable. It
should probably be dma_addr_t, which is what xhci_trb_virt_to_dma
returns.
Can you fix this?
You already have a patch for this further down in your inbox :)
(I used u64 rather then dma_addr_t)
I also wondered if we could have multiple Set TR dequeue
commands pending for different stream rings, and thus overwrite the
endpoint queued_deq_seg and queued_dequeue_ptr fields. However, I
convinced myself that isn't possible.
When the Stop Endpoint command completes, all stream rings will stop,
but the host will indicate the stream ring that it was in the middle of
processing. That will get stored in xhci_virt_ep->stopped_td, and thus
only one of the stream rings could possibly need a Set TR Dequeue
command at a time. The rest will have canceled TDs get turned into
no-ops.
Right, note their already is a check against this + an xhci_warn
in queue_set_tr_deq, so if it does somehow happen at least we will get
a warning (and the second set_deq will be ignored).
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord
Re: [xhci:fun-streams-fixes 33/48] drivers/usb/host/xhci-ring.c:1154:29: sparse: incorrect type in assignment (different base types)
All right, so we have a couple sparse warnings with this patch.
On Thu, Oct 17, 2013 at 01:40:15PM +0800, kbuild test robot wrote:
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git
> fun-streams-fixes
> head: e79db70b0f7d028cb9e9cf8ee424a1e92b7d232f
> commit: 0f899d55a18ce75eed9dccd2cc2cd6d333f9ff16 [33/48] xhci: For streams
> the dequeue ptr must be read from the stream ctx
> reproduce: make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> >> drivers/usb/host/xhci-ring.c:1154:29: sparse: incorrect type in assignment
> >> (different base types)
>drivers/usb/host/xhci-ring.c:1154:29:expected restricted __le64
> [usertype] deq
>drivers/usb/host/xhci-ring.c:1154:29:got unsigned long long
> >> drivers/usb/host/xhci-ring.c:1156:29: sparse: incorrect type in assignment
> >> (different base types)
>drivers/usb/host/xhci-ring.c:1156:29:expected restricted __le64
> [usertype] deq
>drivers/usb/host/xhci-ring.c:1156:29:got unsigned long long
> >> drivers/usb/host/xhci-ring.c:1161:65: sparse: restricted __le64 degrades
> >> to integer
>drivers/usb/host/xhci-ring.c:1674:19: sparse: restricted __le32 degrades
> to integer
>
> vim +1154 drivers/usb/host/xhci-ring.c
>
> 1148} else {
> 1149__le64 deq;
> 1150/* 4.6.10 deq ptr is written to the stream ctx
> for streams */
> 1151if (ep->ep_state & EP_HAS_STREAMS) {
> 1152struct xhci_stream_ctx *ctx =
> 1153
> &ep->stream_info->stream_ctx_array[stream_id];
> > 1154deq = le64_to_cpu(ctx->stream_ring) &
> > SCTX_DEQ_MASK;
> 1155} else {
> 1156deq = le64_to_cpu(ep_ctx->deq) &
> ~EP_CTX_CYCLE_MASK;
> 1157}
> 1158xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> 1159"Successful Set TR Deq Ptr cmd, deq =
> @%08llx", deq);
> 1160if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
> 1161 ep->queued_deq_ptr) ==
> deq) {
> 1162/* Update the ring's dequeue segment
> and dequeue pointer
> 1163 * to reflect the new position.
> 1164 */
>
> ---
> 0-DAY kernel build testing backend Open Source Technology Center
> http://lists.01.org/mailman/listinfo/kbuild Intel Corporation
Looking at the patch itself:
+ __le64 deq;
+ /* 4.6.10 deq ptr is written to the stream ctx for streams */
+ if (ep->ep_state & EP_HAS_STREAMS) {
+ struct xhci_stream_ctx *ctx =
+ &ep->stream_info->stream_ctx_array[stream_id];
+ deq = le64_to_cpu(ctx->stream_ring) & SCTX_DEQ_MASK;
+ } else {
+ deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
+ }
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
- "Successful Set TR Deq Ptr cmd, deq = @%08llx",
-le64_to_cpu(ep_ctx->deq));
- if (xhci_trb_virt_to_dma(dev->eps[ep_index].queued_deq_seg,
-dev->eps[ep_index].queued_deq_ptr) ==
- (le64_to_cpu(ep_ctx->deq) & ~(EP_CTX_CYCLE_MASK))) {
+ "Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
+ if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
+ep->queued_deq_ptr) == deq) {
/* Update the ring's dequeue segment and dequeue pointer
* to reflect the new position.
*/
I don't think it makes sense to use __le64 for the deq variable. It
should probably be dma_addr_t, which is what xhci_trb_virt_to_dma
returns.
Can you fix this?
I also wondered if we could have multiple Set TR dequeue
commands pending for different stream rings, and thus overwrite the
endpoint queued_deq_seg and queued_dequeue_ptr fields. However, I
convinced myself that isn't possible.
When the Stop Endpoint command completes, all stream rings will stop,
but the host will indicate the stream ring that it was in the middle of
processing. That will get stored in xhci_virt_ep->stopped_td, and thus
only one of the stream rings could possibly need a Set TR Dequeue
command at a time. The rest will have canceled TDs get turned into
no-ops.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
