On Tue, 7 Jan 2025, Derek John Clark wrote:
> On Tue, Jan 7, 2025 at 10:21 AM Ilpo Järvinen
> <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 2 Jan 2025, Derek John Clark wrote:
> > > On Wed, Jan 1, 2025 at 7:40 PM Mario Limonciello <superm1@xxxxxxxxxx> wrote:
> > > > On 1/1/25 18:47, Derek J. Clark wrote:
> > > > > Adds lenovo-wmi-other.c which provides a driver for the Lenovo
> > > > > "Other Mode" WMI interface that comes on some Lenovo "Gaming
> > > > > Series" hardware. Provides a firmware-attributes class which
> > > > > enables the use of tunable knobs for SPL, SPPT, and FPPT.
> > > > >
> > > > > v2:
> > > > > - Use devm_kzalloc to ensure driver can be instanced, remove global
> > > > > reference.
> > > > > - Ensure reverse Christmas tree for all variable declarations.
> > > > > - Remove extra whitespace.
> > > > > - Use guard(mutex) in all mutex instances, global mutex.
> > > > > - Use pr_fmt instead of adding the driver name to each pr_err.
> > > > > - Remove noisy pr_info usage.
> > > > > - Rename other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv.
> > > > > - Use list to get the lenovo_wmi_om_priv instance in some macro
> > > > > called functions as the data provided by the macros that use it
> > > > > doesn't pass a member of the struct for use in container_of.
> > > > > - Do not rely on GameZone interface to grab the current fan mode.
> > > > >
> > > > > Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
> > > > > +static int other_method_fw_attr_add(struct lenovo_wmi_om_priv *priv)
> > > > > +{
> > > > > + int err, i;
> > > > > +
> > > > > + err = fw_attributes_class_get(&fw_attr_class);
> > > > > + if (err) {
> > > > > + pr_err("Failed to get firmware_attributes_class: %u\n", err);
> > > > > + return err;
> > > > > + }
> > > > > +
> > > > > + priv->fw_attr_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0),
> > > > > + NULL, "%s", FW_ATTR_FOLDER);
> > > > > + if (IS_ERR(priv->fw_attr_dev)) {
> > > > > + err = PTR_ERR(priv->fw_attr_dev);
> > > > > + pr_err("Failed to create firmware_attributes_class device: %u\n",
> > > > > + err);
> > > > > + goto fail_class_get;
> > > > > + }
> > > > > +
> > > > > + priv->fw_attr_kset = kset_create_and_add("attributes", NULL,
> > > > > + &priv->fw_attr_dev->kobj);
> > > > > + if (!priv->fw_attr_kset) {
> > > > > + err = -ENOMEM;
> > > > > + pr_err("Failed to create firmware_attributes_class kset: %u\n",
> > > > > + err);
> > > > > + goto err_destroy_classdev;
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < ARRAY_SIZE(capdata01_attr_groups) - 1; i++) {
> > > > > + err = attr_capdata01_setup(
> > > > > + capdata01_attr_groups[i].tunable_attr);
> > > > > + if (err) {
> > > > > + pr_err("Failed to populate capability data for %s: %u\n",
> > > > > + capdata01_attr_groups[i].attr_group->name, err);
> > > >
> > > > This specific error could be a bit noisy because it's a dependency on
> > > > the other driver in case one attribute returns not supported.
> > > >
> > > > Could you instead detect EOPNOTSUPP specifically and only show error if
> > > > not EOPNOTSUPP?
> > > >
> > >
> > > Easy fix, will do. I'll also add a wmi_dev_exists() here before the
> > > loop to exit early.
> > >
> > > > > + continue;
> > > > > + }
> > > > > +
> > > > > + err = sysfs_create_group(&priv->fw_attr_kset->kobj,
> > > > > + capdata01_attr_groups[i].attr_group);
> > > > > + if (err) {
> > > > > + pr_err("Failed to create sysfs-group for %s: %u\n",
> > > > > + capdata01_attr_groups[i].attr_group->name, err);
> > > > > + goto err_remove_groups;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +
> > > > > +err_remove_groups:
> > > > > + while (i-- > 0) {
> > > > > + sysfs_remove_group(&priv->fw_attr_kset->kobj,
> > > > > + capdata01_attr_groups[i].attr_group);
> > > > > + }
> > > > > +
> > > > > + return err;
> > > > > +
> > > > > +err_destroy_classdev:
> > > > > + device_destroy(fw_attr_class, MKDEV(0, 0));
> > > > > +
> > > > > + return err;
> > > > > +
> > > > > +fail_class_get:
> > > > > + fw_attributes_class_put();
> > > > > +
> > > > > + return err;
> >
> > I highly suspect the intermediate return errs in the previous labels will
> > cause leaks. Don't you want to rollback everything on error?
>
> To clarify, you mean remove the returns in each fail case before
> fail_class_get so they will fall through? That would make more sense,
> yeah.
Yes, the returns before the fail_class_get label and before the
err_destroy_classdev label.
Both seemed to break the usual rollback pattern and it looked to me when
I tracked the callchains an error here will lead to a probe failure so I'd
expect you want to rollback everything in case of an error, not just the
latest step. (In some cases probe is allowed to succeed partially but I
didn't see any indication of that here.)
--
i.
