linux-kernel - Re: [PATCH v2 2/6] platform/x86: asus-armoury: move existing tunings to asus-armoury module

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: <27027d00-214d-e566-6339-95ed4397b99d@linux.intel.com>
Date: Mon, 12 Aug 2024 10:37:56 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Luke Jones <luke@...nes.dev>
cc: platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    Hans de Goede <hdegoede@...hat.com>, corentin.chary@...il.com
Subject: Re: [PATCH v2 2/6] platform/x86: asus-armoury: move existing tunings
 to asus-armoury module

On Sun, 11 Aug 2024, Luke Jones wrote:

> On Tue, 6 Aug 2024, at 10:16 PM, Ilpo Järvinen wrote:
> > On Tue, 6 Aug 2024, Luke D. Jones wrote:
> > 
> > > The fw_attributes_class provides a much cleaner interface to all of the
> > > attributes introduced to asus-wmi. This patch moves all of these extra
> > > attributes over to fw_attributes_class, and shifts the bulk of these
> > > definitions to a new kernel module to reduce the clutter of asus-wmi
> > > with the intention of deprecating the asus-wmi attributes in future.
> > > 
> > > The work applies only to WMI methods which don't have a clearly defined
> > > place within the sysfs and as a result ended up lumped together in
> > > /sys/devices/platform/asus-nb-wmi/ with no standard API.
> > > 
> > > Where possible the fw attrs now implement defaults, min, max, scalar,
> > > choices, etc. As en example dgpu_disable becomes:
> > > 
> > > /sys/class/firmware-attributes/asus-armoury/attributes/dgpu_disable/
> > > ├── current_value
> > > ├── display_name
> > > ├── possible_values
> > > └── type
> > > 
> > > as do other attributes.
> > > 
> > > Signed-off-by: Luke D. Jones <luke@...nes.dev>

> > > +static ssize_t attr_int_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > +       const char *buf, size_t count, u32 min, u32 max, u32 *store_value,
> > > +       u32 wmi_dev);
> > > +
> > > +static ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > 
> > inline missing
> > 
> > > +{
> > > + return sysfs_emit(buf, "integer\n");
> > 
> > Lukas Wunner might have done something to make emitting constant strings 
> > easier, please check out if that's already in mainline.
> 
> I'm not sure what I'm looking for here. Searching my current pdx86 pull 
> isn't returning anything likely and I can't find anything on lore.

I think that was done mostly outside of pdx86, here it is:

https://lore.kernel.org/all/cover.1713608122.git.lukas@wunner.de/

So DEVICE_STRING_ATTR_RO() seems the way to go, I think.

> > > + * On success the return value is 0, and the retval is a valid value returned
> > > + * by the successful WMI function call. An error value is returned only if the
> > > + * WMI function failed, or if it returns "unsupported" which is typically a 0
> > > + * (no return, and no 'supported' bit set), or a 0xFFFFFFFE (~1) which if not
> > > + * caught here can result in unexpected behaviour later.
> > > + */
> > > +int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval)
> > > +{
> > > + int err;
> > > +
> > > + err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, dev_id, 0, retval);
> > > + if (err)
> > > + return err;
> > > + /* Be explicit about retval */
> > > + if (*retval == 0xFFFFFFFE || *retval == 0)
> > 
> > Please name the literals with defines.
> 
> Should have been ASUS_WMI_UNSUPPORTED_METHOD :)

Is one define enough if you have two ways to indicate "unsupported"?

> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(asus_wmi_get_devstate_dsts);
> > > +
> > > +/**
> > > + * asus_wmi_set_devstate() - Set the WMI function state.
> > > + * @dev_id: The WMI function to call.
> > > + * @ctrl_param: The argument to be used for this WMI function.
> > > + * @retval: A pointer to where to store the value returned from WMI.
> > > + *
> > > + * The returned WMI function state if not checked here for error as
> > > + * asus_wmi_set_devstate() is not called unless first paired with a call to
> > > + * asus_wmi_get_devstate_dsts() to check that the WMI function is supported.
> > > + *
> > > + * On success the return value is 0, and the retval is a valid value returned
> > > + * by the successful WMI function call. An error value is returned only if the
> > > + * WMI function failed.
> > > + */
> > > +int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
> > >  {
> > >  return asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS, dev_id,
> > >  ctrl_param, retval);
> > >  }
> > > +EXPORT_SYMBOL_GPL(asus_wmi_set_devstate);
> > 
> > Namespace exports.
> 
> I'm sorry, I don't understand.

Use EXPORT_SYMBOL_NS_GPL(). The driver using the functions, will then do 
MODULE_IMPORT_NS() to pair with the namespace defined by the 
EXPORT_SYMBOL_NS_GPL().

-- 
 i.