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: <7555c528c90e6151f54d0e17c278527f95fac184.camel@codeconstruct.com.au>
Date: Wed, 16 Oct 2024 15:37:07 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Chanh Nguyen <chanh@...eremail.onmicrosoft.com>, Chanh Nguyen
	 <chanh@...amperecomputing.com>
Cc: "Rob Herring (Arm)" <robh@...nel.org>, Joel Stanley <joel@....id.au>, 
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Khanh Pham
 <khpham@...erecomputing.com>, linux-arm-kernel@...ts.infradead.org, Thang
 Nguyen <thang@...amperecomputing.com>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Phong Vo <phong@...amperecomputing.com>, Conor Dooley
 <conor+dt@...nel.org>, Rob Herring <robh+dt@...nel.org>, OpenBMC Maillist
 <openbmc@...ts.ozlabs.org>, Open Source Submission
 <patches@...erecomputing.com>, Quan Nguyen <quan@...amperecomputing.com>, 
 linux-aspeed@...ts.ozlabs.org
Subject: Re: [PATCH] ARM: dts: aspeed: Add device tree for Ampere's Mt.
 Jefferson BMC

On Tue, 2024-10-15 at 13:39 +0700, Chanh Nguyen wrote:
> 
> On 15/10/2024 07:44, Andrew Jeffery wrote:
> > Hi Chanh,
> > 
> > On Mon, 2024-10-14 at 09:05 -0500, Rob Herring (Arm) wrote:
> > > On Mon, 14 Oct 2024 10:50:31 +0000, Chanh Nguyen wrote:
> > > > The Mt. Jefferson BMC is an ASPEED AST2600-based BMC for the Mt. Jefferson
> > > > hardware reference platform with AmpereOne(TM)M processor.
> > > > 
> > > > Signed-off-by: Chanh Nguyen <chanh@...amperecomputing.com>
> > > > ---
> > > >   arch/arm/boot/dts/aspeed/Makefile             |   1 +
> > > >   .../aspeed/aspeed-bmc-ampere-mtjefferson.dts  | 646 ++++++++++++++++++
> > > >   2 files changed, 647 insertions(+)
> > > >   create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dts
> > > > 
> > > 
> > > 
> > > My bot found new DTB warnings on the .dts files added or changed in this
> > > series.
> > > 
> > > Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> > > are fixed by another series. Ultimately, it is up to the platform
> > > maintainer whether these warnings are acceptable or not. No need to reply
> > > unless the platform maintainer has comments.
> > > 
> > > If you already ran DT checks and didn't see these error(s), then
> > > make sure dt-schema is up to date:
> > > 
> > >    pip3 install dtschema --upgrade
> > > 
> > > 
> > > New warnings running 'make CHECK_DTBS=y aspeed/aspeed-bmc-ampere-mtjefferson.dtb' for 20241014105031.1963079-1-chanh@...amperecomputing.com:
> > > 
> > > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: /: compatible: 'oneOf' conditional failed, one must be fixed:
> > > 	'ampere,mtjefferson-bmc' is not one of ['delta,ahe50dc-bmc', 'facebook,galaxy100-bmc', 'facebook,wedge100-bmc', 'facebook,wedge40-bmc', 'microsoft,olympus-bmc', 'quanta,q71l-bmc', 'tyan,palmetto-bmc', 'yadro,vesnin-bmc']
> > > 	'ampere,mtjefferson-bmc' is not one of ['amd,daytonax-bmc', 'amd,ethanolx-bmc', 'ampere,mtjade-bmc', 'aspeed,ast2500-evb', 'asrock,e3c246d4i-bmc', 'asrock,e3c256d4i-bmc', 'asrock,romed8hm3-bmc', 'asrock,spc621d8hm3-bmc', 'asrock,x570d4u-bmc', 'bytedance,g220a-bmc', 'facebook,cmm-bmc', 'facebook,minipack-bmc', 'facebook,tiogapass-bmc', 'facebook,yamp-bmc', 'facebook,yosemitev2-bmc', 'facebook,wedge400-bmc', 'hxt,stardragon4800-rep2-bmc', 'ibm,mihawk-bmc', 'ibm,mowgli-bmc', 'ibm,romulus-bmc', 'ibm,swift-bmc', 'ibm,witherspoon-bmc', 'ingrasys,zaius-bmc', 'inspur,fp5280g2-bmc', 'inspur,nf5280m6-bmc', 'inspur,on5263m5-bmc', 'intel,s2600wf-bmc', 'inventec,lanyang-bmc', 'lenovo,hr630-bmc', 'lenovo,hr855xg2-bmc', 'portwell,neptune-bmc', 'qcom,centriq2400-rep-bmc', 'supermicro,x11spi-bmc', 'tyan,s7106-bmc', 'tyan,s8036-bmc', 'yadro,nicole-bmc', 'yadro,vegman-n110-bmc', 'yadro,vegman-rx20-bmc', 'yadro,vegman-sx20-bmc']
> > > 	'ampere,mtjefferson-bmc' is not one of ['ampere,mtmitchell-bmc', 'aspeed,ast2600-evb', 'aspeed,ast2600-evb-a1', 'asus,x4tf-bmc', 'facebook,bletchley-bmc', 'facebook,catalina-bmc', 'facebook,cloudripper-bmc', 'facebook,elbert-bmc', 'facebook,fuji-bmc', 'facebook,greatlakes-bmc', 'facebook,harma-bmc', 'facebook,minerva-cmc', 'facebook,yosemite4-bmc', 'ibm,blueridge-bmc', 'ibm,everest-bmc', 'ibm,fuji-bmc', 'ibm,rainier-bmc', 'ibm,system1-bmc', 'ibm,tacoma-bmc', 'inventec,starscream-bmc', 'inventec,transformer-bmc', 'jabil,rbp-bmc', 'qcom,dc-scm-v1-bmc', 'quanta,s6q-bmc', 'ufispace,ncplite-bmc']
> > > 	'aspeed,ast2400' was expected
> > > 	'aspeed,ast2500' was expected
> > > 	from schema $id: http://devicetree.org/schemas/arm/aspeed/aspeed.yaml#
> > > 
> > 
> > This needs to be fixed as pointed out by Krzysztof.
> > 
> 
> Thank Andrew, I'll update that in patch v2
> 
> > *snip*
> > 
> > > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: /ahb/apb/bus@...8a000/i2c@.../i2c-mux@...i2c@...su@58: failed to match any schema with compatible: ['pmbus']
> > > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjefferson.dtb: /ahb/apb/bus@...8a000/i2c@.../i2c-mux@...i2c@...su@59: failed to match any schema with compatible: ['pmbus']
> > 
> > These two should also be fixed. The compatible must describe the
> > physical device, not the communication/application protocol. It may be
> > necessary to add a binding if there's not one already for the device.
> > 
> 
> Hi Andrew, My device is following the pmbus specification. So I'm using 
> the generic pmbus driver 
> (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/hwmon/pmbus/pmbus.c#n237) 
> to probe my device.
> 

The devicetree doesn't describe drivers though, it describes devices.
The compatible string needs to represent the device.

>  In arch/arm/boot/dts/aspeed/ directory, many boards 
> are also using this compatible to probe our devices.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-lenovo-hr855xg2.dts#n219
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-inventec-transformers.dts#n263
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-tyan-s8036.dts#n260
> 
> Andrew, Recently I saw the ASPEED platform's maintainer accept the 
> "pmbus" compatible with a warning log. You can see in the below list 
> that patches were merged recently.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=bb3776e564d2190db0ef45609e66f13c60ce5b48
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=28cfb03afcb20a841e96e821ba20870a7c437034
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=36d96827f480e90037d162098061333e279ea35f
> 

Unfortunately for your argument, I'm not trying to make the case that
some people are allowed to do this and others (such as yourself) are
not.

Rather, this is the review process in action, where hopefully everyone,
including myself, tries to improve their practices with feedback.

You can also find discussions where other maintainers (Guenter, hwmon
maintainer; Krzysztof, devicetree maintainer) have asked that "pmbus"
not be used as a compatible:

https://lore.kernel.org/all/f76798ea-6edd-4888-8057-c09aaed88f25@roeck-us.net/

The tools are asking you to do something different via the warnings,
and so am I :) Please define the compatible according to the device
used in the design.

Thanks,

Andrew

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ