Re: [xhci:fun-streams-fixes 33/48] drivers/usb/host/xhci-ring.c:1154:29: sparse: incorrect type in assignment (different base types)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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];
1154				deq = 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux