STM32 PCD negative numbers issue #6609

pauluap · 2018-04-11T21:57:26Z

Description

This issue was initiated by a compiler warning

Compile: stm32f7xx_hal_pcd.c
../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c: In function 'PCD_WriteEmptyTxFifo':
../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c:1310:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if (len > ep->maxpacket)
           ^
../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c:1325:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (len > ep->maxpacket)
             ^

My first pass at fixing this issue concentrated on the comparison itself. The implementation can be seen at pauluap@c6ca03d

Upon inspection, I determined that there were further problems

https://github.com/pauluap/mbed-os/blob/c6ca03df764a16b4a36e5598f881acf601fb9b0f/targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c#L1318
The line len32b = (len + 3) / 4; (and an identical one a bit further on) could end up negative even if the warning message was cleaned up.

After thinking about it for a while, I observed that the magnitude of a negative length wasn't important, the essential point is to guard the algorithm from processing negative numbers, so I refactored to make the guard operation more evident and eliminate the need for an signed integer variable.

@jeromecoutant, this is related to #6599

Pull request type

[X ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

jeromecoutant · 2018-04-12T09:09:47Z

ST_INTERNAL_REF 43044

0xc0170 · 2018-04-12T10:13:54Z

Do not merge as this should be send to master

0xc0170 · 2018-04-12T10:14:19Z

waiting for approval from @ARMmbed/team-st-mcd

jeromecoutant · 2018-04-12T15:37:55Z

Hi
Got driver team feedback, and they don't understand why we should rewrite the function.
Replace "int32_t len" by "uint32_t len" is enough

pauluap · 2018-04-12T15:52:30Z

One reason is to make the code intent clearer. Negative numbers aren't important, what is important is guarding against their use.

Looking at the code again, I now think that the driver team is correct because the second and third conditional in the while loop (https://github.com/pauluap/mbed-os/blob/c6ca03df764a16b4a36e5598f881acf601fb9b0f/targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c#L1320) do protect against probable negative numbers. The possibility that len32b is assigned an negative number is still present, but that's ok since the while loop protects against that scenario. I'll withdraw my PR

pauluap · 2018-04-12T15:54:18Z

Ooop, sorry, keeping open to deal with #6599. I'll change to the ilen variant

…es negative number issues Compile: stm32f7xx_hal_pcd.c ../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c: In function 'PCD_WriteEmptyTxFifo': ../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c:1310:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (len > ep->maxpacket) ^ ../targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c:1325:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (len > ep->maxpacket) ^

jeromecoutant · 2018-04-13T07:08:50Z

targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c

@@ -1300,14 +1300,16 @@ static HAL_StatusTypeDef PCD_WriteEmptyTxFifo(PCD_HandleTypeDef *hpcd, uint32_t
 {
  USB_OTG_GlobalTypeDef *USBx = hpcd->Instance;
  USB_OTG_EPTypeDef *ep;
-  int32_t len = 0;
+  uint32_t len;
+  int32_t ilen;


Please remove ilen new variable, and just keep len as uint32

pauluap · 2018-04-13T07:31:02Z

@jeromecoutant Does this work for you?

I also considered dropping the (int32_t)len > 0 checks because the while loop has the ep->xfer_count < ep->xfer_len conditional which would protect against negative numbers. I determined that the checks were still necessary to preserve the negative len value so that atomic_clr_u32 code section would be entered.

jeromecoutant

keep original code,
and please do the same type update for all STM32 families
Thx

jeromecoutant · 2018-04-13T07:30:48Z

targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c

  uint32_t len32b;
  uint32_t fifoemptymsk = 0;

  ep = &hpcd->IN_ep[epnum];
  len = ep->xfer_len - ep->xfer_count;

-  if (len > ep->maxpacket)
+  if (((int32_t)len > 0) && (len > ep->maxpacket))


keep original code

jeromecoutant · 2018-04-13T07:30:54Z

targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c

@@ -1322,7 +1322,7 @@ static HAL_StatusTypeDef PCD_WriteEmptyTxFifo(PCD_HandleTypeDef *hpcd, uint32_t
    /* Write the FIFO */
    len = ep->xfer_len - ep->xfer_count;

-    if (len > ep->maxpacket)
+    if (((int32_t)len > 0) && (len > ep->maxpacket))


keep original code

jeromecoutant · 2018-04-13T07:30:59Z

targets/TARGET_STM/TARGET_STM32F7/device/stm32f7xx_hal_pcd.c

@@ -1334,7 +1334,7 @@ static HAL_StatusTypeDef PCD_WriteEmptyTxFifo(PCD_HandleTypeDef *hpcd, uint32_t
    ep->xfer_count += len;
  }

-  if(len <= 0)
+  if ((int32_t)len <= 0)


keep original code

@jeromecoutant No, I don't think that'll work.

Since len is an uint32_t now, it cannot be less than zero.

That is why I added checks for (int32_t)len > 0) because if ep->xfer_count is greater than ep->xfer_len then len would have its high bit set, and enters the scope that sets len = ep->maxpacket; which is incorrect. The high bit should be preserved so that atomic_clr_u32 is called as expected.

If I make those changes as you suggest, that would change the behavior of the code.

Is my understanding/interpretation wrong?

…ed, remove need for guards

pauluap · 2018-04-13T11:48:45Z

Ok, I've made another commit which removes the need for the if guards. I think that the change also makes the point I was trying to make clearer, that the final conditional for atomic_clr_u32 execution basically needs the signum() equivalent

…ifo() function

0xc0170 · 2018-04-13T12:56:08Z

/morph build

mbed-ci · 2018-04-13T13:15:23Z

Build : SUCCESS

Build number : 1742
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6609/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-13T21:30:52Z

Exporter Build : SUCCESS

Build number : 1376
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6609/

mbed-ci · 2018-04-14T00:40:46Z

Test : SUCCESS

Build number : 1545
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6609/1545

0xc0170 · 2018-04-16T06:33:05Z

/morph mbed2-build

Cherry picked from ARMmbed/mbed-os#6609

jeromecoutant mentioned this pull request Apr 12, 2018

STM32 compilation warning issues #6599

Merged

0xc0170 added the do not merge label Apr 12, 2018

0xc0170 added the needs: review label Apr 12, 2018

pauluap closed this Apr 12, 2018

sg- removed do not merge needs: review labels Apr 12, 2018

pauluap reopened this Apr 12, 2018

pauluap force-pushed the compiler_warning_pcd_unsigned_signed branch from dcd1a3e to 430784b Compare April 12, 2018 16:51

pauluap changed the base branch from mbed-os-5.8 to master April 12, 2018 16:59

Revert to original fix concentrating on type correctness

8f4a5e2

jeromecoutant reviewed Apr 13, 2018

View reviewed changes

Drop usage of ilen, just use len and cast it to int32_t as appropriate

2211a27

jeromecoutant suggested changes Apr 13, 2018

View reviewed changes

Make the atomic_clr_u32 conditional use raw values rather than comput…

b45d423

…ed, remove need for guards

Extend changes to other STM32 devices that have the PCD_WriteEmptyTxF…

20f11bc

…ifo() function

jeromecoutant approved these changes Apr 13, 2018

View reviewed changes

0xc0170 approved these changes Apr 13, 2018

View reviewed changes

0xc0170 added the needs: CI label Apr 13, 2018

0xc0170 added ready for merge and removed needs: CI labels Apr 16, 2018

cmonr merged commit 37f36a7 into ARMmbed:master Apr 16, 2018

cmonr added ready for merge release-version: 5.8.3 and removed ready for merge labels Apr 16, 2018

pauluap deleted the compiler_warning_pcd_unsigned_signed branch April 16, 2018 17:44

mkbel pushed a commit to mkbel/Prusa-Firmware-Buddy that referenced this pull request Sep 10, 2020

Fix STM32 PCD negative numbers issue

bb193a2

Cherry picked from ARMmbed/mbed-os#6609

mkbel pushed a commit to prusa3d/Prusa-Firmware-Buddy that referenced this pull request Sep 11, 2020

Fix STM32 PCD negative numbers issue

7f4f1b5

Cherry picked from ARMmbed/mbed-os#6609

STM32 PCD negative numbers issue #6609

STM32 PCD negative numbers issue #6609

Uh oh!

Conversation

pauluap commented Apr 11, 2018

Description

Pull request type

Uh oh!

jeromecoutant commented Apr 12, 2018

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

jeromecoutant commented Apr 12, 2018

Uh oh!

pauluap commented Apr 12, 2018

Uh oh!

pauluap commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauluap commented Apr 13, 2018

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauluap commented Apr 13, 2018

Uh oh!

0xc0170 commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 13, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 14, 2018

Uh oh!

0xc0170 commented Apr 16, 2018

Uh oh!

Uh oh!