linux-kernel - Re: [PATCH v2] arm64: dts: allwinner: Add disable-wp for boards with micro SD card

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240920115206.1ea2666b@donnerap.manchester.arm.com>
Date: Fri, 20 Sep 2024 11:52:06 +0100
From: Andre Przywara <andre.przywara@....com>
To: Kryštof Černý <cleverline1mc@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej
 Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: allwinner: Add disable-wp for boards
 with micro SD card

On Fri, 20 Sep 2024 07:38:53 +0200
Kryštof Černý <cleverline1mc@...il.com> wrote:

Hi,

> Dne 19. 09. 24 v 22:21 Andre Przywara napsal(a):
> > On Thu, 19 Sep 2024 20:35:39 +0200
> > Kryštof Černý <cleverline1mc@...il.com> wrote:
> >   
> >> Adding disable-wp property for micro SD nodes of Allwinner arm64 devices.
> >> Boards were verified from online pictures/tables
> >> that they have micro SD slots.  
> > 
> > The changes itself look good to me, and I checked that the boards in
> > question have a microSD card slot, so do not feature a write-protect
> > switch.
> > You seem to be missing the mmc0 node in
> > sun50i-h616-bigtreetech-cb1.dtsi though, can you please add this file,
> > to fix the two boards using this file as well?
> >   
> 
> I wondered about that one, as it's just a compute module without the
> actual slot, I thought it was not right to put it there, as
> someone could wire it to full SD or eMMC in their own board.
> Maybe put it to sun-50i-h616-bigtreetech-cb1-manta.dts and
> sun-50i-h616-bigtreetech-pi.dts instead? The dtsi has broken-cd
> property already, but as it seems it is wired to the connector.
> I was not sure about this, what are your thoughts?

Oh, you are right, so this is actually a "bug" in the existing DT.
Could you please send a separate patch, moving this out of the .dtsi and
into the two board .dts files, adding the disable-wp on the way?
Please CC: Martin (who sent the original DTs).

> > With that added:
> >   
> >> Signed-off-by: Kryštof Černý <cleverline1mc@...il.com>  
> > 
> > Reviewed-by: Andre Przywara <andre.przywara@....com>
> > 
> > There are some boards which have their base .dtsi in the arch/arm
> > directory, but we can tackle those separately.  
> 
> I missed them, if I add the property to sunxi-libretech-all-h3-cc.dtsi
> and sunxi-bananapi-m2-plus.dtsi, could tag you as a reviewer?

You can always CC: me, though I would spot it through the linux-sunxi list
anyway (I have a special eye on this one).

Anyway, if you feel like it, you can do the same exercise for the arch/arm
directory, though it's a bit more tricky there, since there are far more
and also older boards, some indeed with a full scale SD card slot.
Definitely make this a separate patch, maybe even split this up by SoC or
SoC groups. For instance I'd believe that any H3 boards would have microSD
only, courtesy of being "newer". Since those are the ones that share .dtsi
files with arch/arm64, that's probably a good target for one patch.

It looks like there is not a single wp-gpios property for any of those
boards. I wonder if disable-wp; is then correct regardless. If there are
boards with full scale SD slots *and* the WP pin connected, this could be
fixed later, on a case-by-case base. Compared to the current situation we
don't seem to lose anything.
And I just checked the BananaPi(-M1), with a full scale slot, and this one
doesn't connect the WP pin, so "disable-wp;" is definitely correct there
as well.

> There is also a module called Emlid Neutis, the compute module itself
> has not a slot, but the board that uses it does, unfortunately they are
> not separate files, so I do not see a clear solution here, as above.

Right, so similar to the Bigtreetech boards above, the split
between the SoM .dtsi and the board .dts files doesn't seem to be right,
but it affects more than just the SD card - namely UARTs, USB, audio
routing, etc.
So this would be a separate patch as well. I have a board with the N5
module at home, so happy to verify that.

Cheers,
Andre

> >   
> >> ---
> >> Sorry that my last messages were not in mailing list,
> >> one was wrongly sent and second was rejected, as the bot claimed it
> >> contained html. ---
> >> Changes in v2:
> >> - NEW: Added the property to all Sunxi arm64 boards, as discussed in
> >> mailing list
> >> - Link to v1:
> >> https://lore.kernel.org/r/20240914-b4-nanopineoplus2-fix-mmc0-wp-v1-1-12f54f0d6620@gmail.com
> >> --- arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts    |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts         |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-r1s-h5.dts       |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts        |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-prime.dts      |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus.dts  |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus2.dts |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts         |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts          |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi           |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts            |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h6-tanix.dtsi              |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h616-orangepi-zero.dtsi    |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h616-x96-mate.dts          |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts       |
> >> 1 + arch/arm64/boot/dts/allwinner/sun50i-h618-transpeed-8k618-t.dts |
> >> 1 + 16 files changed, 16 insertions(+)
> >>
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts index
> >> 526443bb736c..18fa541795a6 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo-plus2.dts @@
> >> -136,6 +136,7 @@ &mmc0 { vmmc-supply = <&reg_vcc3v3>; bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >> +	disable-wp;
> >>   	status = "okay";
> >>   };
> >>   
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts index
> >> 05486cccee1c..128295f5a5d6 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts @@ -88,6
> >> +88,7 @@ ext_rgmii_phy: ethernet-phy@7 {
> >>   &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >>   	status = "okay";
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-r1s-h5.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-r1s-h5.dts index
> >> 3a7ee44708a2..44fdc8b3f79d 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-r1s-h5.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-r1s-h5.dts @@ -157,6
> >> +157,7 @@ eth_mac1: mac-address@fa { &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >>   	status = "okay";
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts index
> >> ce3ae19e72db..0f29da7d51e6 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts @@ -153,6
> >> +153,7 @@ &ir {
> >>   &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >>   	status = "okay";
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-prime.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-prime.dts index
> >> b699bb900e13..d4fc4e60e4e7 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-prime.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-prime.dts @@
> >> -153,6 +153,7 @@ &ir { &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >>   	status = "okay";
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus.dts
> >> index ae85131aac9c..3322cc4d9aa4 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus.dts @@
> >> -82,6 +82,7 @@ ext_rgmii_phy: ethernet-phy@1 { &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >>   	status = "okay";
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus2.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus2.dts
> >> index 734481e998b8..3eb986c354a9 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus2.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h5-orangepi-zero-plus2.dts @@
> >> -79,6 +79,7 @@ hdmi_out_con: endpoint { &mmc0 {
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> >>   	status = "okay";
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts index
> >> 3be1e8c2fdb9..13a0e63afeaf 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts @@ -129,6
> >> +129,7 @@ ext_rgmii_phy: ethernet-phy@1 { &mmc0 {
> >>   	vmmc-supply = <&reg_cldo1>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts index
> >> 6c3bfe3d09d9..ab87c3447cd7 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts @@ -131,6
> >> +131,7 @@ hdmi_out_con: endpoint { &mmc0 {
> >>   	vmmc-supply = <&reg_cldo1>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi index
> >> 13b07141c334..d05dc5d6e6b9 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi @@ -94,6
> >> +94,7 @@ hdmi_out_con: endpoint { &mmc0 {
> >>   	vmmc-supply = <&reg_cldo1>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts index
> >> c8b275552872..fa7a765ee828 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts @@ -133,6
> >> +133,7 @@ ext_rgmii_phy: ethernet-phy@1 { &mmc0 {
> >>   	vmmc-supply = <&reg_cldo1>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix.dtsi
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix.dtsi index
> >> 855b7d43bc50..bb7de37c0d58 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix.dtsi +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix.dtsi @@ -124,6 +124,7
> >> @@ &mmc0 { pinctrl-0 = <&mmc0_pins>;
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h616-orangepi-zero.dtsi
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h616-orangepi-zero.dtsi index
> >> fc7315b94406..a3fe39f8e2ca 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h616-orangepi-zero.dtsi +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h616-orangepi-zero.dtsi @@
> >> -81,6 +81,7 @@ ext_rgmii_phy: ethernet-phy@1 { &mmc0 {
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;	/* PF6 */
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616-x96-mate.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h616-x96-mate.dts index
> >> 26d25b5b59e0..dd3bd9cca710 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h616-x96-mate.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h616-x96-mate.dts @@ -52,6
> >> +52,7 @@ &ir { &mmc0 {
> >>   	vmmc-supply = <&reg_dcdce>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;	/* PF6 */
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts index
> >> 18b29c6b867f..16c68177ff69 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h618-longanpi-3h.dts @@ -111,6
> >> +111,7 @@ ext_rgmii_phy: ethernet-phy@1 { };
> >>   &mmc0 {
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>;	/* PF6 */
> >>   	vmmc-supply = <&reg_vcc3v3>;
> >> diff --git
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h618-transpeed-8k618-t.dts
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h618-transpeed-8k618-t.dts
> >> index d6631bfe629f..024377b333c1 100644 ---
> >> a/arch/arm64/boot/dts/allwinner/sun50i-h618-transpeed-8k618-t.dts +++
> >> b/arch/arm64/boot/dts/allwinner/sun50i-h618-transpeed-8k618-t.dts @@
> >> -71,6 +71,7 @@ &ir { &mmc0 { vmmc-supply = <&reg_dldo1>;
> >>   	cd-gpios = <&pio 8 16 GPIO_ACTIVE_LOW>;	/* PI16 */
> >> +	disable-wp;
> >>   	bus-width = <4>;
> >>   	status = "okay";
> >>   };
> >>
> >> ---
> >> base-commit: 57f962b956f1d116cd64d5c406776c4975de549d
> >> change-id: 20240914-b4-nanopineoplus2-fix-mmc0-wp-9d77fb9e6513
> >>
> >> Best regards,  
> >   
>