You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/09/27 23:00:50 UTC
[GitHub] [incubator-nuttx] saramonteiro opened a new pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
saramonteiro opened a new pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904
Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
## Summary
The cpuint variable is used in the drivers' config struct to indicate to which CPU interrupt the peripheral interrupt will be allocated to. Since there's only 32 cpu interrupts and 69 peripheral interrupts, if there's no other CPU interrupt available to address a new peripheral interrupt, them the `esp32_alloc_cpuint` function will answer with a 'Out of Memory' error. (-ENOMEM). This value is negative and will be forwarded to unsigned variables leading to 2 problems:
1. The program will never enter in the error catching routine.
2. Since ENOMEM is number 12, the driver code may deallocate the peripheral interrupt previously attached to CPU interrupt 12 leading to unpredictable behavior.
## Impact
The developer will now be able to catch the error.
If there's no CPU interrupts available, it will not detach a previous attached peripheral interrupt.
## Testing
N/A
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton merged pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-702779921
@Ouss4 what about my comment on esp32_detach. I don't think we should be setting it to 0xff
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro edited a comment on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
saramonteiro edited a comment on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-700004965
@btashton Thanks for your considerations!
I'd rather the` int `option instead of using a temp variable. I think it will be standardized with the other arches. I will take a look at other drivers to make sure this change will be done in all drivers. And I also agree with you that `esp32_detach` should answer a negative return.
Thanks for such attention and these suggestions!
I will do the necessary changes and commit it again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-702790678
> @Ouss4 what about my comment on esp32_detach. I don't think we should be setting it to 0xff
Sorry, I missed that one.
The `cpuint` variable will get a new value with the next esp32_alloc_levelint() which will override that 0xff.
But I also think that a < 0 value is clearer. So I changed it. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] saramonteiro commented on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-700004965
@btashton Thanks for your considerations!
I'd rather the` int `option instead of using a temp variable. I think it will be standardized with the other arches. I will take a look at other drivers to make sure this change will be done in all drivers. And I also agree with you that esp32_detach should answer a negative return.
Thanks for these suggestions!
I will do the necessary changes and commit it again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-699743404
We should move this to be `int` instead of `int8` since it is being used to optionally hold the negative error status from the allocation function which returns type `int`. This is also in line with the other arch. Alternatively we assign to a temporary variable prior to assigning to the struct.
Also I think this change is needed in some of the other drivers:
```
xtensa/src/esp32/esp32_serial.c: priv->cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_spi_slave.c: priv->cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_gpio.c: g_gpio_cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_i2c.c: priv->cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_cpuint.c: * Name: esp32_alloc_levelint
xtensa/src/esp32/esp32_cpuint.c:int esp32_alloc_levelint(int priority)
xtensa/src/esp32/esp32_emac.c: priv->cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_irq.c: cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_spi.c: priv->cpuint = esp32_alloc_levelint(1);
xtensa/src/esp32/esp32_cpustart.c: cpuint = esp32_alloc_levelint(1);
```
`esp32_detach` also assigns to 0xff which seems like a poor chose. It would probably be better to make that < 0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1904: xtensa/ESP32: Fixed the type of cpuint variables in esp32_emac.c esp32_i2c.c esp32_spi.c esp32_spi_slave.c
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1904:
URL: https://github.com/apache/incubator-nuttx/pull/1904#issuecomment-702742729
@btashton this came my way, so I updated the change with what you suggested. Most of the files were already addressed by @saramonteiro, the rest was using the correct type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org