linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] usb: isp1760: clang analyzer fixes
@ 2021-08-19 18:09 Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 1/5] usb: isp1760: ignore return value for bus change pattern Rui Miguel Silva
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

This are fixes for new and pre existing warnings noted by clang
analyzer in this driver which complains about some unused read values,
shift left using negative uninitialized slot numbers and zero
maxpacketsize.

Cheers,
  Rui

Rui Miguel Silva (5):
  usb: isp1760: ignore return value for bus change pattern
  usb: isp1760: check maxpacketsize before using it
  usb: isp1760: do not reset retval
  usb: isp1760: do not shift in uninitialized slot
  usb: isp1760: clean never read udc_enabled warning

 drivers/usb/isp1760/isp1760-core.c |  4 ++--
 drivers/usb/isp1760/isp1760-hcd.c  | 31 ++++++++++++++++++------------
 2 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/5] usb: isp1760: ignore return value for bus change pattern
  2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
@ 2021-08-19 18:09 ` Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 2/5] usb: isp1760: check maxpacketsize before using it Rui Miguel Silva
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

We do not care about the return value of that read between the
scratch register write and read, we really just want to make sure that
the pattern in the bus get changed to make sure we are testing
correctly the scratch pattern.

Clang-analyzer complains about the never read scratch variable:
>> drivers/usb/isp1760/isp1760-hcd.c:735:2: warning: Value stored to 'scratch' is never read [clang-analyzer-deadcode.DeadStores]
    scratch = isp1760_hcd_read(hcd, HC_CHIP_ID_HIGH);

Just ignore the return value of that CHIP_ID_HIGH read, add more
information to the comment above why we are doing this. And as at it,
just do a small format change in the error message bellow.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/usb/isp1760/isp1760-hcd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c
index 825be736be33..2a21fe5aa7a8 100644
--- a/drivers/usb/isp1760/isp1760-hcd.c
+++ b/drivers/usb/isp1760/isp1760-hcd.c
@@ -731,11 +731,15 @@ static int isp1760_hc_setup(struct usb_hcd *hcd)
 
 	isp1760_hcd_write(hcd, HC_SCRATCH, pattern);
 
-	/* Change bus pattern */
-	scratch = isp1760_hcd_read(hcd, HC_CHIP_ID_HIGH);
+	/*
+	 * we do not care about the read value here we just want to
+	 * change bus pattern.
+	 */
+	isp1760_hcd_read(hcd, HC_CHIP_ID_HIGH);
 	scratch = isp1760_hcd_read(hcd, HC_SCRATCH);
 	if (scratch != pattern) {
-		dev_err(hcd->self.controller, "Scratch test failed. 0x%08x\n", scratch);
+		dev_err(hcd->self.controller, "Scratch test failed. 0x%08x\n",
+			scratch);
 		return -ENODEV;
 	}
 
-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/5] usb: isp1760: check maxpacketsize before using it
  2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 1/5] usb: isp1760: ignore return value for bus change pattern Rui Miguel Silva
@ 2021-08-19 18:09 ` Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 3/5] usb: isp1760: do not reset retval Rui Miguel Silva
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

When checking if we need one more packet on a bulk pipe we may, even
though not probable at all, get there with a zero maxpacketsize.
In that case for sure no packet, no even a one more, will be
allocated.

This will clean the clang-analyzer warning:
drivers/usb/isp1760/isp1760-hcd.c:1856:38: warning: Division by zero [clang-analyzer-core.DivideZero]
                                && !(urb->transfer_buffer_length %

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/usb/isp1760/isp1760-hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c
index 2a21fe5aa7a8..5c947a1eae49 100644
--- a/drivers/usb/isp1760/isp1760-hcd.c
+++ b/drivers/usb/isp1760/isp1760-hcd.c
@@ -1854,7 +1854,7 @@ static void packetize_urb(struct usb_hcd *hcd,
 				packet_type = OUT_PID;
 			else
 				packet_type = IN_PID;
-		} else if (usb_pipebulk(urb->pipe)
+		} else if (usb_pipebulk(urb->pipe) && maxpacketsize
 				&& (urb->transfer_flags & URB_ZERO_PACKET)
 				&& !(urb->transfer_buffer_length %
 							maxpacketsize)) {
-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/5] usb: isp1760: do not reset retval
  2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 1/5] usb: isp1760: ignore return value for bus change pattern Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 2/5] usb: isp1760: check maxpacketsize before using it Rui Miguel Silva
@ 2021-08-19 18:09 ` Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 4/5] usb: isp1760: do not shift in uninitialized slot Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 5/5] usb: isp1760: clean never read udc_enabled warning Rui Miguel Silva
  4 siblings, 0 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

We do not really need to reset retval before get used bellow.
This will avoid the clang-analyzer warning:

drivers/usb/isp1760/isp1760-hcd.c:1919:2: warning: Value stored to 'retval' is never read [clang-analyzer-deadcode.DeadStores]
        retval = 0;

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/usb/isp1760/isp1760-hcd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c
index 5c947a1eae49..aed2714ce0cf 100644
--- a/drivers/usb/isp1760/isp1760-hcd.c
+++ b/drivers/usb/isp1760/isp1760-hcd.c
@@ -1919,7 +1919,6 @@ static int isp1760_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
 	if (list_empty(&new_qtds))
 		return -ENOMEM;
 
-	retval = 0;
 	spin_lock_irqsave(&priv->lock, spinflags);
 
 	if (!test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)) {
-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 4/5] usb: isp1760: do not shift in uninitialized slot
  2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
                   ` (2 preceding siblings ...)
  2021-08-19 18:09 ` [PATCH 3/5] usb: isp1760: do not reset retval Rui Miguel Silva
@ 2021-08-19 18:09 ` Rui Miguel Silva
  2021-08-19 18:09 ` [PATCH 5/5] usb: isp1760: clean never read udc_enabled warning Rui Miguel Silva
  4 siblings, 0 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

Even though it is not expected, and would trigger a WARN_ON, killing a
transfer in a uninitialized slot this sequence is warned by clang
analyzer, twice:

drivers/usb/isp1760/isp1760-hcd.c:1976:18: warning: The result of the left shift is undefined because the right operand is negative [clang-analyzer-core.UndefinedBinaryOperatorResult]
                skip_map |= (1 << qh->slot);
drivers/usb/isp1760/isp1760-hcd.c:1983:18: warning: The result of the left shift is undefined because the right operand is negative [clang-analyzer-core.UndefinedBinaryOperatorResult]
                skip_map |= (1 << qh->slot);

Only set skip map if slot is active.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/usb/isp1760/isp1760-hcd.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/isp1760/isp1760-hcd.c b/drivers/usb/isp1760/isp1760-hcd.c
index aed2714ce0cf..bf8ab3fe2e5a 100644
--- a/drivers/usb/isp1760/isp1760-hcd.c
+++ b/drivers/usb/isp1760/isp1760-hcd.c
@@ -1974,16 +1974,20 @@ static void kill_transfer(struct usb_hcd *hcd, struct urb *urb,
 	/* We need to forcefully reclaim the slot since some transfers never
 	   return, e.g. interrupt transfers and NAKed bulk transfers. */
 	if (usb_pipecontrol(urb->pipe) || usb_pipebulk(urb->pipe)) {
-		skip_map = isp1760_hcd_read(hcd, HC_ATL_PTD_SKIPMAP);
-		skip_map |= (1 << qh->slot);
-		isp1760_hcd_write(hcd, HC_ATL_PTD_SKIPMAP, skip_map);
-		ndelay(100);
+		if (qh->slot != -1) {
+			skip_map = isp1760_hcd_read(hcd, HC_ATL_PTD_SKIPMAP);
+			skip_map |= (1 << qh->slot);
+			isp1760_hcd_write(hcd, HC_ATL_PTD_SKIPMAP, skip_map);
+			ndelay(100);
+		}
 		priv->atl_slots[qh->slot].qh = NULL;
 		priv->atl_slots[qh->slot].qtd = NULL;
 	} else {
-		skip_map = isp1760_hcd_read(hcd, HC_INT_PTD_SKIPMAP);
-		skip_map |= (1 << qh->slot);
-		isp1760_hcd_write(hcd, HC_INT_PTD_SKIPMAP, skip_map);
+		if (qh->slot != -1) {
+			skip_map = isp1760_hcd_read(hcd, HC_INT_PTD_SKIPMAP);
+			skip_map |= (1 << qh->slot);
+			isp1760_hcd_write(hcd, HC_INT_PTD_SKIPMAP, skip_map);
+		}
 		priv->int_slots[qh->slot].qh = NULL;
 		priv->int_slots[qh->slot].qtd = NULL;
 	}
-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 5/5] usb: isp1760: clean never read udc_enabled warning
  2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
                   ` (3 preceding siblings ...)
  2021-08-19 18:09 ` [PATCH 4/5] usb: isp1760: do not shift in uninitialized slot Rui Miguel Silva
@ 2021-08-19 18:09 ` Rui Miguel Silva
  4 siblings, 0 replies; 6+ messages in thread
From: Rui Miguel Silva @ 2021-08-19 18:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Alan Stern; +Cc: linux-usb, Rui Miguel Silva

When CONFIG_USB_ISP1761_UDC is not enabled the udc_enabled variable is
never used since it is short circuited before in the logic operations.

This would trigger the following warning by clang analyzer:

drivers/usb/isp1760/isp1760-core.c:490:2: warning: Value stored to 'udc_enabled' is never read [clang-analyzer-deadcode.DeadStores]
        udc_enabled = ((devflags & ISP1760_FLAG_ISP1763) ||
        ^
drivers/usb/isp1760/isp1760-core.c:490:2: note: Value stored to 'udc_enabled' is never read

Just swap the other of the operands in the logic operations.

Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 drivers/usb/isp1760/isp1760-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/isp1760/isp1760-core.c b/drivers/usb/isp1760/isp1760-core.c
index ff07e2890692..cb70f9d63cdd 100644
--- a/drivers/usb/isp1760/isp1760-core.c
+++ b/drivers/usb/isp1760/isp1760-core.c
@@ -491,7 +491,7 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
 		       (devflags & ISP1760_FLAG_ISP1761));
 
 	if ((!IS_ENABLED(CONFIG_USB_ISP1760_HCD) || usb_disabled()) &&
-	    (!IS_ENABLED(CONFIG_USB_ISP1761_UDC) || !udc_enabled))
+	    (!udc_enabled || !IS_ENABLED(CONFIG_USB_ISP1761_UDC)))
 		return -ENODEV;
 
 	isp = devm_kzalloc(dev, sizeof(*isp), GFP_KERNEL);
@@ -571,7 +571,7 @@ int isp1760_register(struct resource *mem, int irq, unsigned long irqflags,
 			return ret;
 	}
 
-	if (IS_ENABLED(CONFIG_USB_ISP1761_UDC) && udc_enabled) {
+	if (udc_enabled && IS_ENABLED(CONFIG_USB_ISP1761_UDC)) {
 		ret = isp1760_udc_register(isp, irq, irqflags);
 		if (ret < 0) {
 			isp1760_hcd_unregister(hcd);
-- 
2.33.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-19 18:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 18:09 [PATCH 0/5] usb: isp1760: clang analyzer fixes Rui Miguel Silva
2021-08-19 18:09 ` [PATCH 1/5] usb: isp1760: ignore return value for bus change pattern Rui Miguel Silva
2021-08-19 18:09 ` [PATCH 2/5] usb: isp1760: check maxpacketsize before using it Rui Miguel Silva
2021-08-19 18:09 ` [PATCH 3/5] usb: isp1760: do not reset retval Rui Miguel Silva
2021-08-19 18:09 ` [PATCH 4/5] usb: isp1760: do not shift in uninitialized slot Rui Miguel Silva
2021-08-19 18:09 ` [PATCH 5/5] usb: isp1760: clean never read udc_enabled warning Rui Miguel Silva

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).