New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32 PCD negative numbers issue #6609

Merged
merged 5 commits into from Apr 16, 2018

Conversation

Projects
None yet
6 participants
@pauluap

pauluap commented Apr 11, 2018

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

This comment has been minimized.

Contributor

jeromecoutant commented Apr 12, 2018

ST_INTERNAL_REF 43044

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

Do not merge as this should be send to master

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

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

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Apr 12, 2018

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

This comment has been minimized.

pauluap commented Apr 12, 2018

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

This comment has been minimized.

pauluap commented Apr 12, 2018

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

@pauluap pauluap reopened this Apr 12, 2018

Paul Thompson
Initial work was for unsigned-signed comparison fix. Current work fix…
…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)
             ^

@pauluap pauluap force-pushed the pauluap:compiler_warning_pcd_unsigned_signed branch from dcd1a3e to 430784b Apr 12, 2018

@pauluap pauluap changed the base branch from mbed-os-5.8 to master Apr 12, 2018

@@ -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;

This comment has been minimized.

@jeromecoutant

jeromecoutant Apr 13, 2018

Contributor

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

@pauluap

This comment has been minimized.

pauluap commented Apr 13, 2018

@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

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))

This comment has been minimized.

@jeromecoutant

jeromecoutant Apr 13, 2018

Contributor

keep original code

@@ -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))

This comment has been minimized.

@jeromecoutant

jeromecoutant Apr 13, 2018

Contributor

keep original code

@@ -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)

This comment has been minimized.

@jeromecoutant

jeromecoutant Apr 13, 2018

Contributor

keep original code

This comment has been minimized.

@pauluap

pauluap Apr 13, 2018

@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?

Paul Thompson
@pauluap

This comment has been minimized.

pauluap commented Apr 13, 2018

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 13, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 16, 2018

/morph mbed2-build

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

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

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9035 cycles (+75 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@pauluap pauluap deleted the pauluap:compiler_warning_pcd_unsigned_signed branch Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment