linux-kernel - Re: [PATCH] USB: typec: tps6598x: use device 'type' field to identify devices

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <292f5d48-8567-4b60-ad03-6cf70f71bacc@kernel.org>
Date:   Thu, 30 Nov 2023 15:30:54 +0200
From:   Roger Quadros <rogerq@...nel.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Alexandru Ardelean <alex@...uggie.ro>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        gregkh@...uxfoundation.org, christophe.jaillet@...adoo.fr,
        a-govindraju@...com, trix@...hat.com, abdelalkuor@...tab.com,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH] USB: typec: tps6598x: use device 'type' field to identify
 devices

Hi Heikki,

On 30/11/2023 12:54, Heikki Krogerus wrote:
> Hi Roger,
> 
>>> Why not just match against the structures themselves?
>>>
>>>         if (tps->data == &tps25750_data)
>>>                 ...
>>
>> Then you need to declare tps25750_data and friends at the top of the file?
>>
>> A better approach might be to have type agnostic quirk flags for the special
>> behavior required for different types. This way, multiple devices can share
>> the same quirk if needed.
>>
>> e.g.
>> NEEDS_POWER_UP instead of TIPD_TYPE_APPLE_CD321X
>> SKIP_VID_READ instead of TIPD_TYPE_TI_TPS25750X
>> INIT_ON_RESUME instead of TIPD_TYPE_TI_TPS25750X
>>
>> Also rename cd321x_switch_power_state() to tps6598x_switch_power_state().
> 
> No. Functions like that isolate cd321x specific functionality into an
> actual "function" just like they should.
> 
> Quirk flags mean that if something breaks, it will almost always break
> for everybody (there is no real isolation with quirk flags), and when
> things are fixed and when features are added, we are forced to always
> "dance" around those quirk flags - you always have to consider them.
> 
> Platform/device type checks are just as bad IMO, but in one way they
> are better than quirk flags. There is no question about what a
> platform check is checking, but quirk flags can so easily become
> incomprehensible (just what exactly does it mean when you say
> NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document
> those quirks, which is waste of effort, and in reality nobody will do).
> 
> In case of tipd/code.c, it should be converted into a library that
> only has the common/shared functionality. CD321, TPS2579x, TPS6598x
> and what ever there is, then will have a glue driver that handles
> everything that specific for their controller type.

Do you mean that you want to treat the 3 devices as different incompatible devices
so each one has a separate driver which warrants for a different DT binding
for each and also Kconfig symbol?

> 
> Before this driver is reorganised like that (any volunteers?), we'll
> have the PD controller type checks, but quirk flags we will not have.
> 
> In general, you should only use quirk flags if there is no other
> way to move forward - they are the last resort. They are dangerous,
> and even in the best case they reduce the maintenability of the code.
> 
> thanks,
> 

-- 
cheers,
-roger