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] [day] [month] [year] [list]
Message-Id: <3f244316-f259-4d61-90ae-4caa183c1a32@app.fastmail.com>
Date: Wed, 18 Sep 2024 20:50:31 +1200
From: "Luke Jones" <luke@...nes.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
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 Mon, 12 Aug 2024, at 7:37 PM, Ilpo Järvinen wrote:
> 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.

Thanks. I did try this but the signature is different to the bulk of what I;ve done already. I don't have the time for it yet and will revisit it later.

> 
> > > > + * 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"?

This function body wasn't correct. It now is and that retval == 0 is removed.

> 
> > > > + 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().

Got it. I did wonder about this while reading a few kernel books (rather old now).

> 
> -- 
> i.
>