| Message ID | 20190509115039.6127-1-freifunk@adrianschmutzler.de |
|---|---|
| State | Superseded |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel] ath79: Add missing read-only properties | expand |
Adrian Schmutzler <freifunk@adrianschmutzler.de> [2019-05-09 13:50:39]: Hi, > The read-only properties for non-firmware partitions seem to have been > assigned rather randomly. Instead of randomly I would say, that it was developer/submitter preference. > label = "u-boot-env"; > + read-only; > }; as ath79 has `DEFAULT_PACKAGES += uboot-envtools` and just few devices set `DEFAULT_PACKAGES -= uboot-envtools` I would assume, that it's expected to be writeable. > label = "art"; > + read-only; > }; this one seems legit and should be probably ok. -- ynezz
Hi, > -----Original Message----- > From: Petr Štetiar [mailto:ynezz@true.cz] > Sent: Donnerstag, 9. Mai 2019 14:42 > To: Adrian Schmutzler <freifunk@adrianschmutzler.de> > Cc: openwrt-devel@lists.openwrt.org > Subject: Re: [OpenWrt-Devel] [PATCH] ath79: Add missing read-only properties > > Adrian Schmutzler <freifunk@adrianschmutzler.de> [2019-05-09 13:50:39]: > > Hi, > > > The read-only properties for non-firmware partitions seem to have been > > assigned rather randomly. > > Instead of randomly I would say, that it was developer/submitter preference. > > > label = "u-boot-env"; > > + read-only; > > }; > > as ath79 has `DEFAULT_PACKAGES += uboot-envtools` and just few devices set > `DEFAULT_PACKAGES -= uboot-envtools` I would assume, that it's expected to > be > writeable. Okay. I also found some devices where u-boot-env IS read-only, e.g. https://github.com/openwrt/openwrt/blob/master/target/linux/ath79/dts/ar7240_buffalo_whr-g301n.dts#L131 What would you suggest: 1. Remove read-only from the remaining ones 2. Set it based on how DEFAULT_PACKAGES is set 3. Don't change. Leave it as it was set by the submitters In case 1 or 2, I would separate this from the other changes into a separate patch. Best Adrian > > > label = "art"; > > + read-only; > > }; > > this one seems legit and should be probably ok. > > -- ynezz
Adrian Schmutzler <mail@adrianschmutzler.de> [2019-05-09 14:52:11]: > > as ath79 has `DEFAULT_PACKAGES += uboot-envtools` and just few devices set > > `DEFAULT_PACKAGES -= uboot-envtools` I would assume, that it's expected to > > be > > writeable. > > Okay. I also found some devices where u-boot-env IS read-only, e.g. > https://github.com/openwrt/openwrt/blob/master/target/linux/ath79/dts/ar7240_buffalo_whr-g301n.dts#L131 > > What would you suggest: > > 1. Remove read-only from the remaining ones > 2. Set it based on how DEFAULT_PACKAGES is set even if you remove uboot-envtools from the base image, due to the flash space constraints for example, the user is still able to post-install this package, so we can't probably base our decision on the content of DEFAULT_PACKAGES variable. > 3. Don't change. Leave it as it was set by the submitters Probably this option unless we've strong reason to do otherwise. -- ynezz
On Thursday, 9 May 2019 13:50:39 CEST Adrian Schmutzler wrote: > diff --git a/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts b/ target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts > index 1e3cf40f71..fa74cf2344 100644 > --- a/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts > +++ b/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts > @@ -114,6 +114,7 @@ > partition@1 { > label = "u-boot-env"; > reg = <0x040000 0x010000>; > + read-only; > }; > > partition@2 { I think this device is in a weird state for ath79 but following info would be relevant when it would support flash installations and sysupgrade like under ar71xx: This device needs to write access to the u-boot-env to switch the firmware partition and adjust the kernel + (static part of) rootfs checksums during syspgrade. Kind regards, Sven
Hello Adrian, On 09.05.19 13:50, Adrian Schmutzler wrote: > The read-only properties for non-firmware partitions seem to have > been assigned rather randomly. > I went through the DTS files in ath79 and assigned the read-only > to all partitions that seem to require protection. > diff --git a/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts b/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts > index 0875c319b9..c5b4775167 100644 > --- a/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts > +++ b/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts > @@ -100,6 +100,7 @@ > partition@40000 { > label = "u-boot-env"; > reg = <0x040000 0x010000>; > + read-only; > }; > > partition@50000 { > diff --git a/target/linux/ath79/dts/qca9558_ocedo_koala.dts b/target/linux/ath79/dts/qca9558_ocedo_koala.dts > index 23835492a3..2b861ec8c0 100644 > --- a/target/linux/ath79/dts/qca9558_ocedo_koala.dts > +++ b/target/linux/ath79/dts/qca9558_ocedo_koala.dts > @@ -95,6 +95,7 @@ > partition@40000 { > label = "u-boot-env"; > reg = <0x040000 0x010000>; > + read-only; > }; > > partition@50000 { > diff --git a/target/linux/ath79/dts/qca9558_ocedo_ursus.dts b/target/linux/ath79/dts/qca9558_ocedo_ursus.dts > index 1a92da3946..926cc70e4d 100644 > --- a/target/linux/ath79/dts/qca9558_ocedo_ursus.dts > +++ b/target/linux/ath79/dts/qca9558_ocedo_ursus.dts > @@ -68,6 +68,7 @@ > partition@40000 { > label = "u-boot-env"; > reg = <0x040000 0x010000>; > + read-only; > }; > > partition@50000 { This would break the current flashing instructions, which require to set the correct partition to boot from the initramfs. [0] I would prefer not to add read-only flags on uboot-environment without a real reason. However, I'm fine with adding read-only on ART partitions. [0] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=c4931713df8ffb3c4e5c1be7d0b6d4aa96a7dd4c Best wishes David
diff --git a/target/linux/ath79/dts/ar7240_netgear_wnr612-v2.dtsi b/target/linux/ath79/dts/ar7240_netgear_wnr612-v2.dtsi index 8e934429a3..7d185825a1 100644 --- a/target/linux/ath79/dts/ar7240_netgear_wnr612-v2.dtsi +++ b/target/linux/ath79/dts/ar7240_netgear_wnr612-v2.dtsi @@ -79,6 +79,7 @@ partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/ar7241_ubnt_airrouter.dts b/target/linux/ath79/dts/ar7241_ubnt_airrouter.dts index 9e38bf8087..774a45f22d 100644 --- a/target/linux/ath79/dts/ar7241_ubnt_airrouter.dts +++ b/target/linux/ath79/dts/ar7241_ubnt_airrouter.dts @@ -30,9 +30,9 @@ }; &usb_phy { - status = "okay"; + status = "okay"; }; &usb { - status = "okay"; + status = "okay"; }; diff --git a/target/linux/ath79/dts/ar7241_ubnt_unifi.dts b/target/linux/ath79/dts/ar7241_ubnt_unifi.dts index 27aec88cca..99a6677bd7 100644 --- a/target/linux/ath79/dts/ar7241_ubnt_unifi.dts +++ b/target/linux/ath79/dts/ar7241_ubnt_unifi.dts @@ -78,6 +78,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { diff --git a/target/linux/ath79/dts/ar7241_ubnt_xm.dtsi b/target/linux/ath79/dts/ar7241_ubnt_xm.dtsi index 5466575229..88e6952b2e 100644 --- a/target/linux/ath79/dts/ar7241_ubnt_xm.dtsi +++ b/target/linux/ath79/dts/ar7241_ubnt_xm.dtsi @@ -57,6 +57,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { diff --git a/target/linux/ath79/dts/ar7242_buffalo_wzr-bhr.dtsi b/target/linux/ath79/dts/ar7242_buffalo_wzr-bhr.dtsi index 5165efa859..a9ff732b6e 100644 --- a/target/linux/ath79/dts/ar7242_buffalo_wzr-bhr.dtsi +++ b/target/linux/ath79/dts/ar7242_buffalo_wzr-bhr.dtsi @@ -87,6 +87,7 @@ partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; + read-only; }; ART: partition@50000 { diff --git a/target/linux/ath79/dts/ar7242_buffalo_wzr-hp-g302h-a1a0.dts b/target/linux/ath79/dts/ar7242_buffalo_wzr-hp-g302h-a1a0.dts index 97bfd0f842..2219497225 100644 --- a/target/linux/ath79/dts/ar7242_buffalo_wzr-hp-g302h-a1a0.dts +++ b/target/linux/ath79/dts/ar7242_buffalo_wzr-hp-g302h-a1a0.dts @@ -139,6 +139,7 @@ partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; + read-only; }; art: partition@50000 { diff --git a/target/linux/ath79/dts/ar9330_glinet_gl-ar150.dts b/target/linux/ath79/dts/ar9330_glinet_gl-ar150.dts index 53df36760f..a219a3b6fa 100644 --- a/target/linux/ath79/dts/ar9330_glinet_gl-ar150.dts +++ b/target/linux/ath79/dts/ar9330_glinet_gl-ar150.dts @@ -102,6 +102,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { diff --git a/target/linux/ath79/dts/ar9330_pqi_air-pen.dts b/target/linux/ath79/dts/ar9330_pqi_air-pen.dts index 06f728b267..b1238288e6 100644 --- a/target/linux/ath79/dts/ar9330_pqi_air-pen.dts +++ b/target/linux/ath79/dts/ar9330_pqi_air-pen.dts @@ -92,6 +92,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; art: partition@50000 { diff --git a/target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts b/target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts index 43bec35fa2..4881f1b4f2 100644 --- a/target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts +++ b/target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts @@ -85,6 +85,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { diff --git a/target/linux/ath79/dts/ar9342_iodata_etg3-r.dts b/target/linux/ath79/dts/ar9342_iodata_etg3-r.dts index 9fce43fd4f..8b0ac7e402 100644 --- a/target/linux/ath79/dts/ar9342_iodata_etg3-r.dts +++ b/target/linux/ath79/dts/ar9342_iodata_etg3-r.dts @@ -72,6 +72,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts b/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts index 0875c319b9..c5b4775167 100644 --- a/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts +++ b/target/linux/ath79/dts/ar9344_ocedo_raccoon.dts @@ -100,6 +100,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9531_engenius_ews511ap.dts b/target/linux/ath79/dts/qca9531_engenius_ews511ap.dts index 62b6a766fc..e64911b6b6 100644 --- a/target/linux/ath79/dts/qca9531_engenius_ews511ap.dts +++ b/target/linux/ath79/dts/qca9531_engenius_ews511ap.dts @@ -113,11 +113,13 @@ ubootenv: partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; art: partition@50000 { label = "art"; reg = <0x050000 0x010000>; + read-only; }; partition@60000 { diff --git a/target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi b/target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi index ceb2bfa0ff..a68e5e9f64 100644 --- a/target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi +++ b/target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi @@ -88,6 +88,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { @@ -99,6 +100,7 @@ art: partition@3 { label = "art"; reg = <0xff0000 0x010000>; + read-only; }; }; }; diff --git a/target/linux/ath79/dts/qca9531_glinet_gl-x750.dts b/target/linux/ath79/dts/qca9531_glinet_gl-x750.dts index 79cd51673a..c3000dd85a 100644 --- a/target/linux/ath79/dts/qca9531_glinet_gl-x750.dts +++ b/target/linux/ath79/dts/qca9531_glinet_gl-x750.dts @@ -103,11 +103,13 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; art: partition@50000 { label = "art"; reg = <0x050000 0x010000>; + read-only; }; partition@60000 { diff --git a/target/linux/ath79/dts/qca9531_yuncore_a770.dts b/target/linux/ath79/dts/qca9531_yuncore_a770.dts index 18ad6307a1..9b9129d14e 100644 --- a/target/linux/ath79/dts/qca9531_yuncore_a770.dts +++ b/target/linux/ath79/dts/qca9531_yuncore_a770.dts @@ -98,6 +98,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9557_buffalo_bhr-4grv2.dts b/target/linux/ath79/dts/qca9557_buffalo_bhr-4grv2.dts index 995ecb3b73..5dce94e222 100644 --- a/target/linux/ath79/dts/qca9557_buffalo_bhr-4grv2.dts +++ b/target/linux/ath79/dts/qca9557_buffalo_bhr-4grv2.dts @@ -90,6 +90,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9558_engenius_epg5000.dts b/target/linux/ath79/dts/qca9558_engenius_epg5000.dts index 6179150fdb..d4863db147 100644 --- a/target/linux/ath79/dts/qca9558_engenius_epg5000.dts +++ b/target/linux/ath79/dts/qca9558_engenius_epg5000.dts @@ -126,6 +126,7 @@ partition@30000 { label = "u-boot-env"; reg = <0x030000 0x010000>; + read-only; }; partition@40000 { diff --git a/target/linux/ath79/dts/qca9558_librerouter_librerouter-v1.dts b/target/linux/ath79/dts/qca9558_librerouter_librerouter-v1.dts index 9e2f67977a..8dbbdf12ef 100644 --- a/target/linux/ath79/dts/qca9558_librerouter_librerouter-v1.dts +++ b/target/linux/ath79/dts/qca9558_librerouter_librerouter-v1.dts @@ -134,6 +134,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9558_netgear_ex7300.dtsi b/target/linux/ath79/dts/qca9558_netgear_ex7300.dtsi index 21c25a5717..07209549cb 100644 --- a/target/linux/ath79/dts/qca9558_netgear_ex7300.dtsi +++ b/target/linux/ath79/dts/qca9558_netgear_ex7300.dtsi @@ -156,6 +156,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; caldata: partition@50000 { @@ -173,11 +174,13 @@ partition@70000 { label = "config"; reg = <0x070000 0x010000>; + read-only; }; partition@80000 { label = "pot"; reg = <0x080000 0x010000>; + read-only; }; partition@90000 { @@ -189,6 +192,7 @@ partition@fc0000 { label = "language"; reg = <0xfc0000 0x040000>; + read-only; }; }; }; diff --git a/target/linux/ath79/dts/qca9558_ocedo_koala.dts b/target/linux/ath79/dts/qca9558_ocedo_koala.dts index 23835492a3..2b861ec8c0 100644 --- a/target/linux/ath79/dts/qca9558_ocedo_koala.dts +++ b/target/linux/ath79/dts/qca9558_ocedo_koala.dts @@ -95,6 +95,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9558_ocedo_ursus.dts b/target/linux/ath79/dts/qca9558_ocedo_ursus.dts index 1a92da3946..926cc70e4d 100644 --- a/target/linux/ath79/dts/qca9558_ocedo_ursus.dts +++ b/target/linux/ath79/dts/qca9558_ocedo_ursus.dts @@ -68,6 +68,7 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@50000 { diff --git a/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts b/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts index 1e3cf40f71..fa74cf2344 100644 --- a/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts +++ b/target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts @@ -114,6 +114,7 @@ partition@1 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; partition@2 { diff --git a/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts b/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts index 439acaae85..de43fcda26 100644 --- a/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts +++ b/target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts @@ -87,11 +87,13 @@ partition@40000 { label = "u-boot-env"; reg = <0x040000 0x010000>; + read-only; }; art: partition@50000 { label = "art"; reg = <0x050000 0x010000>; + read-only; }; partition@60000 { diff --git a/target/linux/ath79/dts/qca9563_tplink_archer-a7-v5.dts b/target/linux/ath79/dts/qca9563_tplink_archer-a7-v5.dts index a9174df4fa..42db1d7a24 100644 --- a/target/linux/ath79/dts/qca9563_tplink_archer-a7-v5.dts +++ b/target/linux/ath79/dts/qca9563_tplink_archer-a7-v5.dts @@ -39,16 +39,19 @@ info: info@f40000 { label = "info"; reg = <0xf40000 0x020000>; + read-only; }; config: config@f60000 { label = "config"; reg = <0xf60000 0x050000>; + read-only; }; partition@fc0000 { label = "partition-table"; reg = <0xfc0000 0x010000>; + read-only; }; art: art@ff0000 { diff --git a/target/linux/ath79/dts/qca9563_tplink_archer-c2-v3.dts b/target/linux/ath79/dts/qca9563_tplink_archer-c2-v3.dts index c106a63eb8..7a88f56a02 100644 --- a/target/linux/ath79/dts/qca9563_tplink_archer-c2-v3.dts +++ b/target/linux/ath79/dts/qca9563_tplink_archer-c2-v3.dts @@ -124,6 +124,7 @@ partition@20000 { label = "uboot"; reg = <0x020000 0x10000>; + read-only; }; partition@30000 { diff --git a/target/linux/ath79/dts/qca9563_tplink_archer-c7-v5.dts b/target/linux/ath79/dts/qca9563_tplink_archer-c7-v5.dts index 8059b48510..efe83cd5ee 100644 --- a/target/linux/ath79/dts/qca9563_tplink_archer-c7-v5.dts +++ b/target/linux/ath79/dts/qca9563_tplink_archer-c7-v5.dts @@ -33,6 +33,7 @@ partition@40000 { label = "partition-table"; reg = <0x040000 0x010000>; + read-only; }; art: partition@50000 { @@ -44,11 +45,13 @@ info: partition@60000 { label = "info"; reg = <0x060000 0x020000>; + read-only; }; partition@80000 { label = "user-config"; reg = <0x080000 0x040000>; + read-only; }; partition@c0000 { @@ -60,5 +63,6 @@ partition@ff0000 { label = "default-config"; reg = <0xff0000 0x010000>; + read-only; }; };
The read-only properties for non-firmware partitions seem to have been assigned rather randomly. I went through the DTS files in ath79 and assigned the read-only to all partitions that seem to require protection. Also fixed a whitespace error on the way. Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> --- target/linux/ath79/dts/ar7240_netgear_wnr612-v2.dtsi | 1 + target/linux/ath79/dts/ar7241_ubnt_airrouter.dts | 4 ++-- target/linux/ath79/dts/ar7241_ubnt_unifi.dts | 1 + target/linux/ath79/dts/ar7241_ubnt_xm.dtsi | 1 + target/linux/ath79/dts/ar7242_buffalo_wzr-bhr.dtsi | 1 + target/linux/ath79/dts/ar7242_buffalo_wzr-hp-g302h-a1a0.dts | 1 + target/linux/ath79/dts/ar9330_glinet_gl-ar150.dts | 1 + target/linux/ath79/dts/ar9330_pqi_air-pen.dts | 1 + target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts | 1 + target/linux/ath79/dts/ar9342_iodata_etg3-r.dts | 1 + target/linux/ath79/dts/ar9344_ocedo_raccoon.dts | 1 + target/linux/ath79/dts/qca9531_engenius_ews511ap.dts | 2 ++ target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi | 2 ++ target/linux/ath79/dts/qca9531_glinet_gl-x750.dts | 2 ++ target/linux/ath79/dts/qca9531_yuncore_a770.dts | 1 + target/linux/ath79/dts/qca9557_buffalo_bhr-4grv2.dts | 1 + target/linux/ath79/dts/qca9558_engenius_epg5000.dts | 1 + target/linux/ath79/dts/qca9558_librerouter_librerouter-v1.dts | 1 + target/linux/ath79/dts/qca9558_netgear_ex7300.dtsi | 4 ++++ target/linux/ath79/dts/qca9558_ocedo_koala.dts | 1 + target/linux/ath79/dts/qca9558_ocedo_ursus.dts | 1 + target/linux/ath79/dts/qca9558_openmesh_om5p-ac-v2.dts | 1 + target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dts | 2 ++ target/linux/ath79/dts/qca9563_tplink_archer-a7-v5.dts | 3 +++ target/linux/ath79/dts/qca9563_tplink_archer-c2-v3.dts | 1 + target/linux/ath79/dts/qca9563_tplink_archer-c7-v5.dts | 4 ++++ 26 files changed, 39 insertions(+), 2 deletions(-)