netdev - Re: [PATCH net-next 11/12] bnxt_en: Expose threshold temperatures through hwmon

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: <d6e093d7-0ff3-4545-9ff8-1c342879fe40@roeck-us.net>
Date: Wed, 16 Aug 2023 12:25:42 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
Cc: Michael Chan <michael.chan@...adcom.com>, davem@...emloft.net,
	netdev@...r.kernel.org, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, gospo@...adcom.com,
	Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH net-next 11/12] bnxt_en: Expose threshold temperatures
 through hwmon

On Wed, Aug 16, 2023 at 09:42:17PM +0530, Kalesh Anakkur Purayil wrote:
> On Wed, Aug 16, 2023 at 5:43 PM Guenter Roeck <linux@...ck-us.net> wrote:
> 
> > On Wed, Aug 16, 2023 at 03:58:34PM +0530, Kalesh Anakkur Purayil wrote:
> > > Thank you Guenter for the review and the suggestions.
> > >
> > > Please see my response inline.
> > >
> > > On Tue, Aug 15, 2023 at 8:35 PM Guenter Roeck <linux@...ck-us.net>
> > wrote:
> > >
> > [ ... ]
> >
> > > >
> > > > Hmm, that isn't really the purpose of alarm attributes. The expectation
> > > > would be that the chip sets alarm flags and the driver reports it.
> > > > I guess there is some value in having it, so I won't object.
> > > >
> > > > Anyway, the ordering is wrong. max_alarm should be the lowest
> > > > alarm level, followed by crit and emergency. So
> > > >                 max_alarm -> temp >= bp->warn_thresh_temp
> > > >                 crit_alarm -> temp >= bp->crit_thresh_temp
> > > >                 emergency_alarm -> temp >= bp->fatal_thresh_temp
> > > >                                 or temp >= bp->shutdown_thresh_temp
> > > >
> > > > There are only three levels of upper temperature alarms.
> > > > Abusing lcrit as 4th upper alarm is most definitely wrong.
> > > >
> > > [Kalesh]: Thank you for the clarification.
> > > bnxt_en driver wants to expose 4 threshold temperatures to the user
> > through
> > > hwmon sysfs.
> > > 1. warning threshold temperature
> > > 2. critical threshold temperature
> > > 3. fatal threshold temperature
> > > 4. shutdown threshold temperature
> > >
> > > I will use the following mapping:
> > >
> > > hwmon_temp_max : warning threshold temperature
> > > hwmon_temp_crit : critical threshold temperature
> > > hwmon_temp_emergency : fatal threshold temperature
> > >
> > > hwmon_temp_max_alarm : temp >= bp->warn_thresh_temp
> > > hwmon_temp_crit_alarm : temp >= bp->crit_thresh_temp
> > > hwmon_temp_emergency_alarm : temp >= bp->fatal_thresh_temp
> > >
> > > Is it OK to map the shutdown threshold temperature to "hwmon_temp_fault"?
> >
> > That is a flag, not a temperature, and it is intended to signal
> > a problem ith the sensor.
> >
> > > If not, can you please suggest an alternative?
> > >
> >
> > The only one I can think of is to add non-standard attributes
> > such as temp1_shutdown and temp1_shutdown_alarm.
> >
> [Kalesh]: Sorry, I don't quite get this part. I was looking at the kernel
> hwmon code, but could not find any reference.
> 

It would be non-standard attributes, so, correct, there is no reference.

> Can we add new attributes "shutdown" and "shutdown_alarm" for tempX? For
> example:
> 
> #define HWMON_T_SHUTDOWN BIT(hwmon_temp_shutdown)
> 

Not for a single driver. You can implement the sysfs attributes
directly in the driver and pass an extra attribute group to the
hwmon core when registering the hwmon device.

Guenter