linux-kernel - Re: [PATCH v2 4/4] platform/x86: Add Lenovo Other Mode WMI Driver

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: <CAFqHKTnGpMbXYVcP-7k+kCJMxPcLa0QGZ2u4-s7a7cYgjA=NwA@mail.gmail.com>
Date: Sat, 11 Jan 2025 09:29:26 -0800
From: Derek John Clark <derekjohn.clark@...il.com>
To: Armin Wolf <W_Armin@....de>
Cc: Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Jonathan Corbet <corbet@....net>, Mario Limonciello <superm1@...nel.org>, Luke Jones <luke@...nes.dev>, 
	Xino Ni <nijs1@...ovo.com>, Zhixin Zhang <zhangzx36@...ovo.com>, Mia Shao <shaohz1@...ovo.com>, 
	Mark Pearson <mpearson-lenovo@...ebb.ca>, 
	"Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>, "Cody T . -H . Chiu" <codyit@...il.com>, 
	John Martens <johnfanv2@...il.com>, platform-driver-x86@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] platform/x86: Add Lenovo Other Mode WMI Driver

On Fri, Jan 10, 2025 at 4:10 PM Armin Wolf <W_Armin@....de> wrote:

> >> Would it make sense to do something similar with each attribute, so that each attribute
> >> can use container_of() to access lenovo_wmi_om_priv without having to use a list lookup?
> >>
> >> This would of course mean that each attribute as to be allocated dynamically.
> >>
> >> Heep in mind that we are currently working on a new API for registering firmware-atrtibute class
> >> devices which should fix this.
> >>
> > I'm not sure I understand what you mean exactly. I think what you're
> > saying is, instead of an attr_group, allocate each attribute as a
> > struct in priv?
>
> Kind of. I envisioned something like this (pseudo code):
>
>         struct firmware_attribute {
>                 struct kobj_attribute attr;
>                 struct lenovo_wmi_om_priv;
>         }
>
> This would allow you to use container_of() to access priv, but would force you to allocate each attribute separately.

Ah, I see. Since we have the kobj in the functions we're accessing the
list in, we could get the firmware_attribute struct instead which
gives the pointer to priv. This will take a bit of refactoring for the
probe & macro sections but I agree that it would be worth it.

> Maybe you can wait with the lenovo-wmi-other driver until the improved firmware-attribute class device API has landed.
> Meanwhile we can focus on the lenovo-wmi-gamezone driver.

I'm not opposed to that. The API update seems to be progressing
quickly and with the multiple other changes I need to figure out v3
might come after that is in for_next anyway. I'll play it by ear and
work on the gamezone changes first.

> >> Is there a reason why this needs to be put inside the header? If no then please put this
> >> inside the driver.
> > To clarify, you mean the macros? I was under the impression they
> > belonged in headers but I can move them. I will move some of the
> > enums/structs as well which are referenced here and the driver only.
>
> I mean both the macros and the show functions. They are only used inside lenovo-wmi-other, so there
> is no reason to expose them inside the public header.
>
> Thanks,
> Armin Wolf

Make sense. I'll move those as well.

Thanks again Armin,
Derek

> >
> >> Thanks,
> >> Armin Wolf
> >>
> >>> +
> >>>    #endif /* !_LENOVO_WMI_H_ */