linux-kernel - Re: [PATCH v8] platform/x86: samsung-galaxybook: Add samsung-galaxybook 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: <CAMF+Kebx4sU+0p+pFaH1Lz4q1xApM8iS9UAYP=sZnE2GDa32ww@mail.gmail.com>
Date: Tue, 28 Jan 2025 20:17:53 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Kurt Borja <kuurtb@...il.com>
Cc: Joshua Grisham <josh@...huagrisham.com>, Thomas Weißschuh <linux@...ssschuh.net>, 
	W_Armin@....de, ilpo.jarvinen@...ux.intel.com, hdegoede@...hat.com, 
	platform-driver-x86@...r.kernel.org, corbet@....net, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Thank you Kurt!

Den lör 25 jan. 2025 kl 16:06 skrev Kurt Borja <kuurtb@...il.com>:
>
> Now I understand the original problem better. I didn't consider this
> possibility when designing the callback.
>
> While this is a fine solution I believe Thomas' EOPNOTSUPP solution is
> the way to go. I think positive err value would be the safest but you
> should wait for the advice of someone with more experience.
>
> Aside from that I really like how the whole platform profile sections
> works now. Good design choices :)
>
> ~ Kurt
>
> > <snip>

Regarding using this positive error code internally within the module,
I thought about maybe adding a comment to galaxybook_probe() before
all of the inits which describe this a bit -- do you all think this
will be helpful or is it clear enough / does not matter and can be
skipped?

I also realized that maybe it is worth to describe that a specific
sequence is needed for doing these "enable feature" + init calls to
the ACPI methods otherwise some devices were reported as starting to
reject the payloads if the sequence was not followed.

Based on these two then I have drafted a comment sort of like this to
put in galaxybook_probe() before the init() calls:

/*
* Features must be enabled and initialized in the following order to
* avoid failures seen on certain devices:
* - GB_SASB_POWER_MANAGEMENT (including performance mode)
* - GB_SASB_KBD_BACKLIGHT
* - GB_SASB_BLOCK_RECORDING (as part of fw_attrs init)
*
* The init function for features which are not supported on all devices
* will return EOPNOTSUPP (positive to differentiate it from upstream
* error codes) if the feature is not working and should be ignored.
*/

Does adding something like this seem like it would help make
everything more clear (especially thinking when new refactoring comes
by other maintainers in X months/years/decades, it would probably help
them to know these subtleties, right?)?

If this comment (you all are welcome to suggest wording tweaks as
well, of course!) plus the few other small tweaks make sense then I
can prep this to send as a new version. But I am holding a bit in
hopes that the 6.14 stuff gets merged to pdx86 for-next so that I can
go ahead with implementing Thomas's new power supply extension
interface at the same time.

Because there are multiple variations to these devices, and there were
some small issues that users with other devices found, I was
thinking/hoping once all looks good for all reviewers, including
implementing the power supply extension, that this could be merged in
to for-next and then I can ask a few people with other supported
devices to test this revamped (and in some ways completely refactored)
driver directly from the branch so that we can try to catch any other
issues that I did not see on my device before it is proposed as a
candidate for mainline -- does that sound reasonable?

Thanks again!

Best regards,
Joshua