linux-kernel - Re: [PATCH 1/6] platform/x86: firmware_attributes

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: <3a553631-1936-4f29-ae91-8276327d5fb4@t-8ch.de>
Date: Sat, 4 Jan 2025 08:06:18 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Armin Wolf <W_Armin@....de>
Cc: Hans de Goede <hdegoede@...hat.com>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Mark Pearson <markpearson@...ovo.com>, 
	Jorge Lopez <jorge.lopez2@...com>, Prasanth Ksr <prasanth.ksr@...l.com>, 
	Joshua Grisham <josh@...huagrisham.com>, platform-driver-x86@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH 1/6] platform/x86: firmware_attributes_class: Move
 include linux/device/class.h

Hi,

On 2025-01-04 07:55:15+0100, Armin Wolf wrote:
> Am 04.01.25 um 00:05 schrieb Thomas Weißschuh:
> 
> > The header firmware_attributes_class.h uses 'struct class'. It should
> > also include the necessary dependency header.

> i like this patch series, but i would prefer that we do not expose the raw class through the header.
> 
> Looking at the callers of fw_attributes_class_get(), everywhere the class value is used only to call:
> 
> 	device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", <driver name>);
> 
> I suggest that we introduce two new functions for that:
> 
> 	struct device *firmware_attributes_device_register(struct device *parent, const char *name);
> 
> 	void firmware_attributes_device_unregister(struct device *dev);
> 
> This would have three major benefits:
> 
> - the raw class can be made internal
> - reduced code size
> - drivers would stop copying the flawed use of device_destroy()
> 
> Regarding the use of device_destroy(): this is actually an error since device_destroy() cannot be
> reliably used when devt is not unique. Since all those drivers are setting devt to MKDEV(0, 0) this
> could result in a kernel panic should multiple firmware-attribute class devices exist at the same time.
> 
> What do you think?

Completely agree. This is exactly what the "further improvements"
mentioned in the cover letter do.
And also add devm_firmware_attributes_device_register() and a custom
sysfs attribute type that makes the driver code much simplerr.

But this will be some more work. Also the conversions of the drivers
will be harder and take longer, so we can't drop the raw exposed class
as easily and have to keep the "legacy" interface for a bit.

> Thanks,
> Armin Wolf
> 
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > ---
> >   drivers/platform/x86/firmware_attributes_class.c | 1 -
> >   drivers/platform/x86/firmware_attributes_class.h | 2 ++
> >   2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> > index 182a07d8ae3dfa8925bb5b71a43d0219c3cf0fa0..cbc56e5db59283ba99ac0b915ac5fb2432afbdc9 100644
> > --- a/drivers/platform/x86/firmware_attributes_class.c
> > +++ b/drivers/platform/x86/firmware_attributes_class.c
> > @@ -3,7 +3,6 @@
> >   /* Firmware attributes class helper module */
> > 
> >   #include <linux/mutex.h>
> > -#include <linux/device/class.h>
> >   #include <linux/module.h>
> >   #include "firmware_attributes_class.h"
> > 
> > diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> > index 363c75f1ac1b89df879a8689b070e6b11d3bb7fd..8e0f47cfdf92eb4dc8722b7d8371916af0d84efa 100644
> > --- a/drivers/platform/x86/firmware_attributes_class.h
> > +++ b/drivers/platform/x86/firmware_attributes_class.h
> > @@ -5,6 +5,8 @@
> >   #ifndef FW_ATTR_CLASS_H
> >   #define FW_ATTR_CLASS_H
> > 
> > +#include <linux/device/class.h>
> > +
> >   int fw_attributes_class_get(const struct class **fw_attr_class);
> >   int fw_attributes_class_put(void);
> > 
> >