linux-kernel - Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

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: <20210318233608.GA140894@roeck-us.net>
Date:   Thu, 18 Mar 2021 16:36:08 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Jonas Malaco <jonas@...tocubo.io>
Cc:     linux-hwmon@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        linux-input@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote:

[ ... ]

> > Either case, the spinlocks are overkill. It would be much easier to
> > convert raw readings here into temperature and fan speed and store
> > the resulting values in struct kraken2_priv_data, and then to
> > just report it in the read functions. That would be much less costly
> > because the spinlock would not be needed at all, and calculations
> > would be done only once per event.
> 
> Oddly enough, this is very close to how the code read last week.
> 
> But I was storing the values in kraken2_priv_data as longs, and I'm not
> sure that storing and loading longs is atomic on all architectures.
> 
> Was that safe, or should I use something else instead of longs?
> 

Hard to say, but do you see any code in the kernel that assumes
that loading or storing a long is not atomic, for any architecture ?

Also, I don't see how u16 * 1000 could ever require a long
for storage. int would be good enough.

> > 
> > > +	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
> > > +	spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > 
> > For my education: What triggers those events ? Are they reported
> > by the hardware autonomously whenever something changes ?
> > A comment at the top of the driver explaining how this works
> > might be useful.
> 
> The device autonomously sends these status reports twice a second.
> 
> I'll add the comment for v2.
> 
That would be great, thanks.

> > 
> > Also, is there a way to initialize values during probe ? Otherwise
> > the driver would report values of 0 until the hardware reports
> > something.
> 
> The device doesn't respond to HID Get_Report, so we can't get valid
> initial values.
> 
> The best we can do is identify the situation and report it to
> user-space.  Am I right that ENODATA should be used for this?
> 
Yes, I think that would be a good idea, and ENODATA sounds good.

Thanks,
Guenter