Re: [PATCH 2/6] platform/x86/uniwill: Handle more WMI events required for TUXEDO devices

[Werner Sembach <wse@tuxedocomputers.com>]

Am 25.11.25 um 01:50 schrieb Armin Wolf:
> Am 24.11.25 um 19:40 schrieb Werner Sembach:
>
>> Hi
>>
>> Am 20.11.25 um 01:53 schrieb Armin Wolf:
>>> [snip]
>>>
>>> Anyway, i attached the patch with the device descriptor infrastructure. The 
>>> callback called during probe cannot modify
>>> the feature bitmap anymore, but i suggest that you simply set the limit for 
>>> cTGP to zero. The code responsible for
>>> initializing cTGP support can then check if the cTGP limit is zero and 
>>> return early. 
>>
>> Looked into it: whats the reason for the "__ro_after_init" in "static struct 
>> uniwill_device_descriptor device_descriptor __ro_after_init;"?
>>
> __ro_after_init tells the kernel to mark the variables annotated with it as 
> read-only after module initialization has finished.
> I used it to ensure that the static device configuration data cannot be 
> (accidentally) modified after module initialization.
Hope it's ok that in my rfc the copy in driver data is no longer read only, but 
is outside of the probe only accessed via a getter method.
>
>> The thing Ilpo wrote sounded like just .driver_data itself should be read 
>> only, but as soon as it has an indirection, like here being copied over to a 
>> static variable, read/write is ok.
>
> No, the device descriptor needs to be treated as read-only because, 
> theoretically, multiple instances of the uniwill-laptop driver might probe.
> If one of those instances modifies the descriptor, the other instances might 
> run into trouble.
Every instance in my RFC has now it's own copy of the feature bitmap.
>
> I suggest that in order to dynamically modify the supported_features bitmap, 
> you simply copy said variable from the descriptor
> into uniwill_data and then use the probe() callback to modify it.
Yes, I did that.
>
>>
>> Also: Why not just copy the device_descriptor over to uniwill_data instead of 
>> a static variable?
>
> uniwill_data is not available during module initialization, so we have store 
> it separately.
sorry I'm stupid xD
>
>>
>> And do I get that correctly: All you can do during the init callback is doing 
>> more sophisticated DMI matching?
>>
> Inside the callback provided dmi_system_id, you indeed can only do more 
> sophisticated DMI matching. You should
> use the probe() callback inside the descriptor to perform device-specific 
> initialization that requires access
> to the EC and/or uniwill_data.

I sent a RFC based on your PoC patch, I would suggest moving the discussion 
thread there.

Thanks so far for your PoC/WIP patch

Best regards

Werner

>
> Thanks,
> Armin Wolf
>
>> Best regards,
>>
>> Werner
>>
>>> [snip]
>>