Re: [RFT 0/5] realtek: support boards similar to DGS-1210-10
Em dom., 17 de jul. de 2022 06:55, Paul Fertser escreveu: > > On Sat, Jul 16, 2022 at 11:32:52PM -0300, Luiz Angelo Daros de Luca wrote: > > It uses SOC := rtl8380 while all existing dgs-1210 F1 variants use > > rtl8382 (except for the pending -52 variant). The commit didn't > > mention why that happened. > > It's just cosmetic AFAICT but the datasheet clearly states that the > SoC used for <=18 ports switches is called RTL8380. It seems we have multiple SoCs for DGS-1210: 1) RTL8380 for -10 2) RTL8382 for -28 3) RTL8393 for -52 It is not the best approach to include a shared config and redefine a property. The dgs-1210 definition should go in Makefile (we also have an rtl8393) with only common properties and SoC should be defined by each device. I was preparing something like that for -52 here: https://github.com/openwrt/openwrt/pull/10227/commits/8e5b473bc1f7f1a8ad796e8b8cc7587fedbad9f5 > > > I'm not sure which one is correct here. However, if it is really a > > different SoC and with what we currently know, we could create a > > generic rtl83xx_d-link_dgs-1210.dtsi as the -52 variant uses even a > > more different SoC (rtl8393). They share a lot of stuff like flash > > layout and gpios (and the vendor firmware even uses the same image). I > > could do some generic and family review but I only have -28 and -52 > > variants. > > I only have access to non-PoE dgs-1210-10 R1 board. > > You say they share GPIO layout, does it mean you currently can't fully > handle SFP ports on your hardware but my patches make it work? I believe the same setup might work for any dgs-1210 F series. It makes sense as d-link uses a common firmware for Fx-Series. However, I didn't test SFP patches in my -28 because I lost all my 1g modules a couple years ago and 10g modules don't work. Anyway, the -52 variant does seem to share the same GPIOs, even using a different SoC. Besides reboot, reset button and led, I could only test the pin that detects the module presence. All of them match those same pins used by other variantes. I would expect that the remaining SFP pins are also at the same positions. I only tried SFP patches to fix (it didn't) the combo ports initialization in the -52 model, although they might touch another part of the driver not used by that device (as it uses different SoC). > > > -- > Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software! > mailto:fercer...@gmail.com ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH 1/2] ubox: fix GCC fanalyzer warnings
memory leaks and missing NULL checks.
Signed-off-by: Rosen Penev
---
kmodloader.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/kmodloader.c b/kmodloader.c
index 63bae5e..bc5f20c 100644
--- a/kmodloader.c
+++ b/kmodloader.c
@@ -336,6 +336,11 @@ static int scan_loaded_modules(void)
/* possibly a module outside /lib/modules// */
n = alloc_module(m.name, NULL, 0, m.depends, m.size);
}
+ if (!n) {
+ ULOG_ERR("Failed to allocate memory for module\n");
+ return -1;
+ }
+
n->usage = m.usage;
n->state = LOADED;
}
@@ -416,7 +421,8 @@ out:
if (fd >= 0)
close(fd);
- free(aliases);
+ if (aliases)
+ free(aliases);
return m;
}
@@ -581,6 +587,11 @@ static int insert_module(char *path, const char *options)
struct stat s;
int fd, ret = -1;
+ if (!path) {
+ ULOG_ERR("Path not specified\n");
+ return ret;
+ }
+
if (stat(path, )) {
ULOG_ERR("missing module %s\n", path);
return ret;
@@ -1162,6 +1173,8 @@ load_options(void)
continue;
}
}
+
+ fclose(f);
}
int main(int argc, char **argv)
--
2.36.1
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH 2/2] ubox: fix bad realloc usage
Both cppcheck and gcc's -fanalyzer complain here that realloc is being
used improperly.
Signed-off-by: Rosen Penev
---
kmodloader.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kmodloader.c b/kmodloader.c
index bc5f20c..5f8c9c1 100644
--- a/kmodloader.c
+++ b/kmodloader.c
@@ -356,6 +356,7 @@ static struct module* get_module_info(const char *module,
const char *name)
unsigned int offset, size;
char *map = MAP_FAILED, *strings, *dep = NULL;
const char **aliases = NULL;
+ const char **aliasesr;
int naliases = 0;
struct module *m = NULL;
struct stat s;
@@ -398,12 +399,13 @@ static struct module* get_module_info(const char *module,
const char *name)
if (!strncmp(strings, "depends=", len + 1))
dep = sep;
else if (!strncmp(strings, "alias=", len + 1)) {
- aliases = realloc(aliases, sizeof(sep) * (naliases +
1));
- if (!aliases) {
+ aliasesr = realloc(aliases, sizeof(sep) * (naliases +
1));
+ if (!aliasesr) {
ULOG_ERR("out of memory\n");
goto out;
}
+ aliases = aliasesr;
aliases[naliases++] = sep;
}
strings = [strlen(sep)];
--
2.36.1
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[sdwalker/sdwalker.github.io] 80a1be: This week's update
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software.--- Begin Message --- Branch: refs/heads/master Home: https://github.com/sdwalker/sdwalker.github.io Commit: 80a1be38f20507e409ae3827ee8856077a25cc28 https://github.com/sdwalker/sdwalker.github.io/commit/80a1be38f20507e409ae3827ee8856077a25cc28 Author: Stephen Walker Date: 2022-07-17 (Sun, 17 Jul 2022) Changed paths: M uscan/index-19.07.html M uscan/index-21.02.html M uscan/index.html Log Message: --- This week's update --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFC PATCH 0/7] realtek: MFD for switch core
Hi Sander, On 7/17/22 15:37, Sander Vanheule wrote: On Sat, 2022-07-16 at 23:09 +0200, Birger Koblitz wrote: Hi, On 7/16/22 21:31, Sander Vanheule wrote: On Sat, 2022-07-16 at 21:09 +0200, Sander Vanheule wrote: This RFC series introduces a new MFD device for the switch core found in the Realtek SoCs. Currently only an implementation is provided for RTL8380, but it written with the register structure of other generations in mind. this looks very promising as it offers the pin control we always needed. I have not looked through the code in detail, yet. The biggest question I have at this point is that at this point the code does not include the other SoC generations. It doesn't because most users are currently on RTL8380, so that's where I can get some feedback from real use cases. I usually run an initramfs image loaded via u-boot, so it's quite possible some setup is still missing. Especially when it comes to the LEDs, they are very different. In fact the RTL838x is the odd-man-out. I would really like to see how they will be included. In the past several design decision turned out to be not so optimal after we learned about the newer SoCs. Today we have all of them very well understood, so it should not be an issue to add at least some code for the RTL93xx generation, which has the LEDs modernized quite a bit. Although I only wrote the RTL8380 implementation, the code is already structured to follow the RTL8390 and later. Aside from correctly defining most pin muxes, I was able to add port LED and pinctrl support for RTL8390 this morning in about 4h. I wouldn't say my code was that badly suited to those chips then. I don't see that you actually do the hardware setup for the LEDs, or am I missing something? You rely on u-boot or the existing led_init() functions (e.g. https://github.com/openwrt/openwrt/commit/3190361ace26cc63fe64a166067c7c543d568337) to configure the HW offload settings, to setup the LED topology and attributing it to a LED-set and define what port type it is (fibre, ethernet, combo,...), right? For that one needs to have access to the PHY-information. Would you add that into the LED driver, or should that be a property of the switch driver? On the RTL93xx devices that correctly initializes the LEDs in u-boot are an exception, that was BTW the reason I pointed to the other SoCs. This is still an RFC, and if it goes upstream it needs to go through staging. Staging should allow for things to evolve more freely. You're right that is isn't a finished product, but if we don't push this out early to get feedback, it's never going to be. I don't really see the hurry. There are different design options for this, I would like to understand implications, first. At least we are now in a position to understand all SoC generations and can take that into account. The other question I have is whether this allows us to solve the other big issue we have with these SoCs: Providing information between drivers. Thats why this is an MFD. IIUC these can provide extra data to child devices, so that could solve that issue (if needed). Generally speaking, this is something you want to avoid however IMHO. In most cases, a SoC-generation specific DT compatible already provides sufficient info. One potential issue that comes to mind, is that the regmap is currently globally locked on any access. I can imagine this will casue performace issues for the ethernet driver. Those registers are mostly independent from the rest of the register space though, so custom regmap locking will probably need to be used. We should be sure this will work. Ethernet performance is not fantastic as it is, further degradation would be really bad. The driver should also anticipate more of the possible offloading, e.g. not copying over TX buffers as MIPS can do DMA from anywhere. For example you have written the 2nd copy of the model name reading function in rtl8380_probe_model_name(), and it does not even probe the other 3 SoC generations. See above, this isn't intended to be a complete implementation. And it's really there just to print the SoC name so the user knows which platform they are on, nothing else. But this information is needed already at the very start of the boot process and several other drivers. So why not provide a global structure like on other MIPS architectures and populate it with information such as machine name and especially switch structure for other drivers to use plus all the other configuration details of a particular SoC (there are nearly 2 dozen different RTL9300 SoCs all having a different structure for the MII ports). In a very simple example, NOR access needs information about the 3/4 byte strapping pin from the switch core, but its registers live at a completely different place in the SoC. The lack of such global information is evident from e.g. your Netgear .dts. I could be wrong, but providing global structures seems to be a thing of the past. I
Re: [RFC PATCH 0/7] realtek: MFD for switch core
Hi Birger, In the future, could you please CC me when replying to my patches? On Sat, 2022-07-16 at 23:09 +0200, Birger Koblitz wrote: > Hi, > > On 7/16/22 21:31, Sander Vanheule wrote: > > On Sat, 2022-07-16 at 21:09 +0200, Sander Vanheule wrote: > > > This RFC series introduces a new MFD device for the switch core found > > > in the Realtek SoCs. Currently only an implementation is provided for > > > RTL8380, but it written with the register structure of other generations > > > in mind. > this looks very promising as it offers the pin control we always needed. > > I have not looked through the code in detail, yet. The biggest question > I have at this point is that at this point the code does not include the > other SoC generations. It doesn't because most users are currently on RTL8380, so that's where I can get some feedback from real use cases. I usually run an initramfs image loaded via u-boot, so it's quite possible some setup is still missing. > Especially when it comes to the LEDs, they are > very different. In fact the RTL838x is the odd-man-out. I would really > like to see how they will be included. In the past several design > decision turned out to be not so optimal after we learned about the > newer SoCs. Today we have all of them very well understood, so it should > not be an issue to add at least some code for the RTL93xx generation, > which has the LEDs modernized quite a bit. Although I only wrote the RTL8380 implementation, the code is already structured to follow the RTL8390 and later. Aside from correctly defining most pin muxes, I was able to add port LED and pinctrl support for RTL8390 this morning in about 4h. I wouldn't say my code was that badly suited to those chips then. This is still an RFC, and if it goes upstream it needs to go through staging. Staging should allow for things to evolve more freely. You're right that is isn't a finished product, but if we don't push this out early to get feedback, it's never going to be. > > The other question I have is whether this allows us to solve the other > big issue we have with these SoCs: Providing information between > drivers. Thats why this is an MFD. IIUC these can provide extra data to child devices, so that could solve that issue (if needed). Generally speaking, this is something you want to avoid however IMHO. In most cases, a SoC-generation specific DT compatible already provides sufficient info. One potential issue that comes to mind, is that the regmap is currently globally locked on any access. I can imagine this will casue performace issues for the ethernet driver. Those registers are mostly independent from the rest of the register space though, so custom regmap locking will probably need to be used. > For example you have written the 2nd copy of the model name > reading function in rtl8380_probe_model_name(), and it does not even > probe the other 3 SoC generations. See above, this isn't intended to be a complete implementation. And it's really there just to print the SoC name so the user knows which platform they are on, nothing else. > But this information is needed > already at the very start of the boot process and several other drivers. > So why not provide a global structure like on other MIPS architectures > and populate it with information such as machine name and especially > switch structure for other drivers to use plus all the other > configuration details of a particular SoC (there are nearly 2 dozen > different RTL9300 SoCs all having a different structure for the MII > ports). In a very simple example, NOR access needs information about the > 3/4 byte strapping pin from the switch core, but its registers live at a > completely different place in the SoC. The lack of such global > information is evident from e.g. your Netgear .dts. I could be wrong, but providing global structures seems to be a thing of the past. I think device tree is the way to go wherever possible. If needed, the SPI-peripheral could get a reference to the switchcore (syscon) node, to be able to also access the regmap and read any required info by itself. > Although you know > that the first used port is 8, from the fact its an RTL8382M SoC or > alternatively the ports in the .dts, you need to add another time the > information about port to LED number. Is there another way to know which internal switch port an LED happens to be connected to? We don't set up an LED-to-PHY interconnect matrix, it's all static. Yes, the device tree spec is verbose, but that's just the way it is. Every element needs to be specified and all that's really required is the 'reg' property. That's _one_ line per LED node. Only referencing a specific phy instead wouldn't work, because then the LED index info is still missing. The way the port LEDs are currently defined in the DT in my RFC, allows one to reference one of the port LEDs just like any other DT LED; for example as the "boot_led" in OpenWrt. This is useful
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sun, Jul 17, 2022 at 01:48:41AM +0200, Ansuel Smith wrote: > Hi, > some background about this. > > I'm trying to improve our CI system more and more by finally adding > support for real > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > to make sure everything works and all compiles correctly... > > While testing it I notice a specific target fails to compile ubox package. > While still to investigate why this is only present on that target, i > found out why > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > The error is this > > kmodloader.c: In function 'main_loader': > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > declared with attribute 'warn_unused_result' [-Werror=unused-result] > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] Error 1 > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > 1342 | ^~~ > 1343cc1: all warnings being treated as errors > > The package is compiled with -Wall so it does make sense that the > error is printed... > > Fact is that the error(warning) is actually correct but I couldn't > understand why it was > not flagged on normal build and here the reason... > > In rules.mk we have > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > -Wno-error=unused-result > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > Now the question... WHY? > > Considering even the linux kernel started to use Wall by default, > should we also start > enforcing correct code and fix every package that present such error? > Fixing these kind of error is trivial enough and IMHO we should drop these > warning disable. > > I will create a PR in the next few days but wonder if anyone wants to > give some feedback > about why these extra flags are set. To me it seems they are just > leftover from older times? For everyone interested... I create a pr [1] if someone prefer to continue the discussion on github [1] https://github.com/openwrt/openwrt/pull/10291 -- Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFT 0/5] realtek: support boards similar to DGS-1210-10
Hi, On 7/17/22 11:55, Paul Fertser wrote: On Sat, Jul 16, 2022 at 11:32:52PM -0300, Luiz Angelo Daros de Luca wrote: It uses SOC := rtl8380 while all existing dgs-1210 F1 variants use rtl8382 (except for the pending -52 variant). The commit didn't mention why that happened. It's just cosmetic AFAICT but the datasheet clearly states that the SoC used for <=18 ports switches is called RTL8380. during OpenWRT boot, the SoC is identified by reading out a configuration register. It will say something like [0.00] Linux version 5.10.127 (birger@MintDesktop) (mips-openwrt-linux-musl-gcc (OpenWrt GCC 11.3.0 r18457+1542-4b587f2561) 11.3.0, GNU ld (GNU Binutils) 2.37) #0 SMP Tue Jul 12 19:01:38 2022 [0.00] RTL838X model is 8380 (4 first hex digits give model ID) [0.00] SoC Type: RTL8380 That should always be correct, unless there is something we are missing. Cheers, Birger ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sun, Jul 17, 2022 at 02:54:24AM -0700, Rosen Penev wrote: > On Sun, Jul 17, 2022 at 2:29 AM Christian Marangi > wrote: > > > > On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote: > > > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith wrote: > > > > > > > > Hi, > > > > some background about this. > > > > > > > > I'm trying to improve our CI system more and more by finally adding > > > > support for real > > > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > > > > to make sure everything works and all compiles correctly... > > > > > > > > While testing it I notice a specific target fails to compile ubox > > > > package. > > > > While still to investigate why this is only present on that target, i > > > > found out why > > > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > > > > > > > The error is this > > > > > > > > kmodloader.c: In function 'main_loader': > > > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > > > > declared with attribute 'warn_unused_result' [-Werror=unused-result] > > > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] > > > > Error 1 > > > > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > > > > 1342 | ^~~ > > > > 1343cc1: all warnings being treated as errors > > > > > > > > The package is compiled with -Wall so it does make sense that the > > > > error is printed... > > > > > > > > Fact is that the error(warning) is actually correct but I couldn't > > > > understand why it was > > > > not flagged on normal build and here the reason... > > > > > > > > In rules.mk we have > > > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > > > > -Wno-error=unused-result > > > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > > > > > > > Now the question... WHY? > > > > > > > > Considering even the linux kernel started to use Wall by default, > > > > should we also start > > > > enforcing correct code and fix every package that present such error? > > > Yes > > > > Ok will prepare a patch. > > > > > > Fixing these kind of error is trivial enough and IMHO we should drop > > > > these > > > > warning disable. > > > I've had issues getting stuff merged to core openwrt utilities in the > > > past, especially when it comes to fixing compilation warnings. > > > > Mhhh I notice sometime patch getting rejected as it was trying to fix an > > false-positive error from a faulty version of gcc. > > But I think fixing error caused by disabling warning should be > > accepted... Real problem is that some trivial fix may cause problems... > > Example the error i just fixed for kmodloader... If I wasn't carfule i > > could totally check the error condition for (fail) instead of (fail < 0) > > and that would have caused breakage as asprintf return the bytes written > > so it could totally return a value != 0. (just an example of a simple > > error handling destryong the function of the package) > Not even rejected. example: > >https://patchwork.ozlabs.org/project/openwrt/patch/20220622185800.3156-1-ros...@gmail.com/ > > and > >https://patchwork.ozlabs.org/project/openwrt/patch/20201225011158.35592-1-ros...@gmail.com/ > > I've had others but retired them after figuring out they were never > going to get merged. I mean I know they are gigantic corner case where you can build an entire house in the corner... But what are the drawbacks of such small fix? The NULL check one for example seems pretty important... probably won't ever happen... But now that I look about it, could be that it was ignored as returning early from that function would create much bigger problems? > > > > > > > > I will create a PR in the next few days but wonder if anyone wants to > > > > give some feedback > > > > about why these extra flags are set. To me it seems they are just > > > > leftover from older times? > > > Most likely. > > > > Still can't understand why these errors are only in archs38/generic... > > Still a mistery to me... > > > > > > > > > > ___ > > > > openwrt-devel mailing list > > > > openwrt-devel@lists.openwrt.org > > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > > > -- > > Ansuel -- Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [RFT 0/5] realtek: support boards similar to DGS-1210-10
On Sat, Jul 16, 2022 at 11:32:52PM -0300, Luiz Angelo Daros de Luca wrote: > It uses SOC := rtl8380 while all existing dgs-1210 F1 variants use > rtl8382 (except for the pending -52 variant). The commit didn't > mention why that happened. It's just cosmetic AFAICT but the datasheet clearly states that the SoC used for <=18 ports switches is called RTL8380. > I'm not sure which one is correct here. However, if it is really a > different SoC and with what we currently know, we could create a > generic rtl83xx_d-link_dgs-1210.dtsi as the -52 variant uses even a > more different SoC (rtl8393). They share a lot of stuff like flash > layout and gpios (and the vendor firmware even uses the same image). I > could do some generic and family review but I only have -28 and -52 > variants. I only have access to non-PoE dgs-1210-10 R1 board. You say they share GPIO layout, does it mean you currently can't fully handle SFP ports on your hardware but my patches make it work? -- Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software! mailto:fercer...@gmail.com ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sun, Jul 17, 2022 at 02:50:36AM -0700, Rosen Penev wrote: > On Sun, Jul 17, 2022 at 2:29 AM Christian Marangi > wrote: > > > > On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote: > > > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith wrote: > > > > > > > > Hi, > > > > some background about this. > > > > > > > > I'm trying to improve our CI system more and more by finally adding > > > > support for real > > > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > > > > to make sure everything works and all compiles correctly... > > > > > > > > While testing it I notice a specific target fails to compile ubox > > > > package. > > > > While still to investigate why this is only present on that target, i > > > > found out why > > > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > > > > > > > The error is this > > > > > > > > kmodloader.c: In function 'main_loader': > > > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > > > > declared with attribute 'warn_unused_result' [-Werror=unused-result] > > > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] > > > > Error 1 > > > > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > > > > 1342 | ^~~ > > > > 1343cc1: all warnings being treated as errors > > > > > > > > The package is compiled with -Wall so it does make sense that the > > > > error is printed... > > > > > > > > Fact is that the error(warning) is actually correct but I couldn't > > > > understand why it was > > > > not flagged on normal build and here the reason... > > > > > > > > In rules.mk we have > > > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > > > > -Wno-error=unused-result > > > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > > > > > > > Now the question... WHY? > > > > > > > > Considering even the linux kernel started to use Wall by default, > > > > should we also start > > > > enforcing correct code and fix every package that present such error? > > > Yes > > > > Ok will prepare a patch. > > > > > > Fixing these kind of error is trivial enough and IMHO we should drop > > > > these > > > > warning disable. > > > I've had issues getting stuff merged to core openwrt utilities in the > > > past, especially when it comes to fixing compilation warnings. > > > > Mhhh I notice sometime patch getting rejected as it was trying to fix an > > false-positive error from a faulty version of gcc. > > But I think fixing error caused by disabling warning should be > > accepted... Real problem is that some trivial fix may cause problems... > > Example the error i just fixed for kmodloader... If I wasn't carfule i > > could totally check the error condition for (fail) instead of (fail < 0) > > and that would have caused breakage as asprintf return the bytes written > > so it could totally return a value != 0. (just an example of a simple > > error handling destryong the function of the package) > > > > > > > > > > I will create a PR in the next few days but wonder if anyone wants to > > > > give some feedback > > > > about why these extra flags are set. To me it seems they are just > > > > leftover from older times? > > > Most likely. > > > > Still can't understand why these errors are only in archs38/generic... > > Still a mistery to me... > very clear actually. glibc marks asprintf with > attribute((warn_unused_result)) (nodiscard in C++). musl does not. Ohhh! And archs38/generic use glibc NOW IT MAKES SENSE! And it's the only target that use glibc toolchains... Thanks a lot. Well time to use glibc instead of using github actions. -- Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sun, Jul 17, 2022 at 2:29 AM Christian Marangi wrote: > > On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote: > > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith wrote: > > > > > > Hi, > > > some background about this. > > > > > > I'm trying to improve our CI system more and more by finally adding > > > support for real > > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > > > to make sure everything works and all compiles correctly... > > > > > > While testing it I notice a specific target fails to compile ubox package. > > > While still to investigate why this is only present on that target, i > > > found out why > > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > > > > > The error is this > > > > > > kmodloader.c: In function 'main_loader': > > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > > > declared with attribute 'warn_unused_result' [-Werror=unused-result] > > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] > > > Error 1 > > > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > > > 1342 | ^~~ > > > 1343cc1: all warnings being treated as errors > > > > > > The package is compiled with -Wall so it does make sense that the > > > error is printed... > > > > > > Fact is that the error(warning) is actually correct but I couldn't > > > understand why it was > > > not flagged on normal build and here the reason... > > > > > > In rules.mk we have > > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > > > -Wno-error=unused-result > > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > > > > > Now the question... WHY? > > > > > > Considering even the linux kernel started to use Wall by default, > > > should we also start > > > enforcing correct code and fix every package that present such error? > > Yes > > Ok will prepare a patch. > > > > Fixing these kind of error is trivial enough and IMHO we should drop these > > > warning disable. > > I've had issues getting stuff merged to core openwrt utilities in the > > past, especially when it comes to fixing compilation warnings. > > Mhhh I notice sometime patch getting rejected as it was trying to fix an > false-positive error from a faulty version of gcc. > But I think fixing error caused by disabling warning should be > accepted... Real problem is that some trivial fix may cause problems... > Example the error i just fixed for kmodloader... If I wasn't carfule i > could totally check the error condition for (fail) instead of (fail < 0) > and that would have caused breakage as asprintf return the bytes written > so it could totally return a value != 0. (just an example of a simple > error handling destryong the function of the package) Not even rejected. example: >https://patchwork.ozlabs.org/project/openwrt/patch/20220622185800.3156-1-ros...@gmail.com/ > and >https://patchwork.ozlabs.org/project/openwrt/patch/20201225011158.35592-1-ros...@gmail.com/ I've had others but retired them after figuring out they were never going to get merged. > > > > > > I will create a PR in the next few days but wonder if anyone wants to > > > give some feedback > > > about why these extra flags are set. To me it seems they are just > > > leftover from older times? > > Most likely. > > Still can't understand why these errors are only in archs38/generic... > Still a mistery to me... > > > > > > > ___ > > > openwrt-devel mailing list > > > openwrt-devel@lists.openwrt.org > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > -- > Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sun, Jul 17, 2022 at 2:29 AM Christian Marangi wrote: > > On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote: > > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith wrote: > > > > > > Hi, > > > some background about this. > > > > > > I'm trying to improve our CI system more and more by finally adding > > > support for real > > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > > > to make sure everything works and all compiles correctly... > > > > > > While testing it I notice a specific target fails to compile ubox package. > > > While still to investigate why this is only present on that target, i > > > found out why > > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > > > > > The error is this > > > > > > kmodloader.c: In function 'main_loader': > > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > > > declared with attribute 'warn_unused_result' [-Werror=unused-result] > > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] > > > Error 1 > > > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > > > 1342 | ^~~ > > > 1343cc1: all warnings being treated as errors > > > > > > The package is compiled with -Wall so it does make sense that the > > > error is printed... > > > > > > Fact is that the error(warning) is actually correct but I couldn't > > > understand why it was > > > not flagged on normal build and here the reason... > > > > > > In rules.mk we have > > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > > > -Wno-error=unused-result > > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > > > > > Now the question... WHY? > > > > > > Considering even the linux kernel started to use Wall by default, > > > should we also start > > > enforcing correct code and fix every package that present such error? > > Yes > > Ok will prepare a patch. > > > > Fixing these kind of error is trivial enough and IMHO we should drop these > > > warning disable. > > I've had issues getting stuff merged to core openwrt utilities in the > > past, especially when it comes to fixing compilation warnings. > > Mhhh I notice sometime patch getting rejected as it was trying to fix an > false-positive error from a faulty version of gcc. > But I think fixing error caused by disabling warning should be > accepted... Real problem is that some trivial fix may cause problems... > Example the error i just fixed for kmodloader... If I wasn't carfule i > could totally check the error condition for (fail) instead of (fail < 0) > and that would have caused breakage as asprintf return the bytes written > so it could totally return a value != 0. (just an example of a simple > error handling destryong the function of the package) > > > > > > > I will create a PR in the next few days but wonder if anyone wants to > > > give some feedback > > > about why these extra flags are set. To me it seems they are just > > > leftover from older times? > > Most likely. > > Still can't understand why these errors are only in archs38/generic... > Still a mistery to me... very clear actually. glibc marks asprintf with attribute((warn_unused_result)) (nodiscard in C++). musl does not. > > > > > > > ___ > > > openwrt-devel mailing list > > > openwrt-devel@lists.openwrt.org > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > -- > Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: Question about ancient TARGET_CFLAGS in rules.mk?
On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote: > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith wrote: > > > > Hi, > > some background about this. > > > > I'm trying to improve our CI system more and more by finally adding > > support for real > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI > > to make sure everything works and all compiles correctly... > > > > While testing it I notice a specific target fails to compile ubox package. > > While still to investigate why this is only present on that target, i > > found out why > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build. > > > > The error is this > > > > kmodloader.c: In function 'main_loader': > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf' > > declared with attribute 'warn_unused_result' [-Werror=unused-result] > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] Error 1 > > 1341 1027 | asprintf(>opts, "%s %s", prev, opts); > > 1342 | ^~~ > > 1343cc1: all warnings being treated as errors > > > > The package is compiled with -Wall so it does make sense that the > > error is printed... > > > > Fact is that the error(warning) is actually correct but I couldn't > > understand why it was > > not flagged on normal build and here the reason... > > > > In rules.mk we have > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable > > -Wno-error=unused-result > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected... > > > > Now the question... WHY? > > > > Considering even the linux kernel started to use Wall by default, > > should we also start > > enforcing correct code and fix every package that present such error? > Yes Ok will prepare a patch. > > Fixing these kind of error is trivial enough and IMHO we should drop these > > warning disable. > I've had issues getting stuff merged to core openwrt utilities in the > past, especially when it comes to fixing compilation warnings. Mhhh I notice sometime patch getting rejected as it was trying to fix an false-positive error from a faulty version of gcc. But I think fixing error caused by disabling warning should be accepted... Real problem is that some trivial fix may cause problems... Example the error i just fixed for kmodloader... If I wasn't carfule i could totally check the error condition for (fail) instead of (fail < 0) and that would have caused breakage as asprintf return the bytes written so it could totally return a value != 0. (just an example of a simple error handling destryong the function of the package) > > > > I will create a PR in the next few days but wonder if anyone wants to > > give some feedback > > about why these extra flags are set. To me it seems they are just > > leftover from older times? > Most likely. Still can't understand why these errors are only in archs38/generic... Still a mistery to me... > > > > ___ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel -- Ansuel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
