linux-kernel - Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

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] [day] [month] [year] [list]

Date: Fri, 19 Apr 2024 05:33:18 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "SanBuenaventura, Jose" <Jose.SanBuenaventura@...log.com>
Cc: "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	Jean Delvare <jdelvare@...e.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>,
	Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
Subject: Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support

On Fri, Apr 19, 2024 at 02:46:15AM +0000, SanBuenaventura, Jose wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> > Sent: Thursday, April 18, 2024 7:55 PM
> > To: SanBuenaventura, Jose <Jose.SanBuenaventura@...log.com>
> > Cc: linux-hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-
> > kernel@...r.kernel.org; linux-doc@...r.kernel.org; linux-i2c@...r.kernel.org;
> > Jean Delvare <jdelvare@...e.com>; Rob Herring <robh@...nel.org>;
> > Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> > <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>; Delphine CC
> > Chiu <Delphine_CC_Chiu@...ynn.com>
> > Subject: Re: [PATCH 2/2] hwmon: pmbus: adm1275: add adm1281 support
> > 
> > [External]
> > 
> > On Thu, Apr 18, 2024 at 08:31:42AM +0000, SanBuenaventura, Jose wrote:
> > >
> > > The lines mentioned were added initially because the STATUS_CML read
> > capability
> > > seems to be only available in the adm1281 and so reading the said register
> > with
> > > another device shouldn't be permitted.
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Why ? Sure, doing so causes the CML bit to be set, but the PMBus core uses
> > that method throughout to determine if a command/register is supported.
> > There are exceptions - some chips react badly if an attempt is made to read
> > unsupported registers. That is not the case for chips in this series, at
> > least not for the ones where I have evaluation boards. In such cases,
> > the chip driver should do nothing and let the PMBus core do its job.
> > 
> > > It seems though that the functionality is redundant and is already handled
> > by
> > > the PMBus core and maybe these lines can be removed and CML related
> > errors
> > > can be checked using the status0 and status0_cml debugfs entries.
> > 
> > This has nothing to do with status0 and status0_cml debugfs entries. The
> > PMBUs core reads STATUS_CML if the CML status bit is set in the status
> > byte/word to determine if a command is supported or not. This is as
> > intended. There is nothing special to be done by a chip driver.
> > 
> > Thanks,
> > Guenter
> 
> Based on the feedback, it seems that the lines mentioned can be removed since
> the pmbus_core.c handles the checking of status_cml through different functions
> like pmbus_check_status_cml and pmbus_check_register among others.
> 
> One thing we observed in the pmbus_core is that the invalid command flag was the
> only one being utilized (PB_CML_FAULT_INVALID_COMMAND) but there are other
> flags for cml fault in pmbus.h such as PB_CML_FAULT_PROCESSOR or 
> PB_CML_FAULT_PACKET_ERROR that were not used. 
> 
> Given these observations, it would again seem right to eliminate the lines you 
> pointed out since those items are already handled by the pmbus core and that
> the status0_cml debugfs entry can be probed to check the register content directly
> and see if there's any other cml fault aside from the invalid command that occurs
> and the user can address it accordingly.
> 
> If ever there would be changes to address the other cml fault errors aside from the
> invalid command it seems that the changes should be applied in the pmbus core 
> and not on the chip driver itself.
> 
> I hope that I understood correctly the points you brought up and identified the 
> possible next step to address those which is to eliminate the added case in the 
> adm1275_read_byte_data.
> 
Correct.

Thanks,
Guenter