linux-kernel - Re: [PATCH v8 2/3] phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver

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]

Date:   Thu, 25 Jun 2020 08:39:56 +0200
From:   Michal Simek <michal.simek@...inx.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Vinod Koul <vkoul@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Kishon Vijay Abraham I <kishon@...com>,
        Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>,
        Michal Simek <michal.simek@...inx.com>
Subject: Re: [PATCH v8 2/3] phy: zynqmp: Add PHY driver for the Xilinx ZynqMP
 Gigabit Transceiver

Hi Laurent and Vinod,

On 24. 06. 20 23:14, Laurent Pinchart wrote:
> Hi Vinod,
> 
> On Wed, Jun 24, 2020 at 10:56:35PM +0530, Vinod Koul wrote:
>> On 24-06-20, 19:39, Laurent Pinchart wrote:
>>
>>>>> +/* Number of GT lanes */
>>>>> +#define NUM_LANES			4
>>>>
>>>> Should this be coded in driver like this? Maybe future versions of
>>>> hardware will have more lanes..? Why not describe this in DT?
>>>
>>> This macro is used to avoid hardcoding 4 in the driver, to make sure
>>> that all the code that deal with the number of lanes use a consistent
>>> value and is readable. There's no foreseen new version of the IP that
>>> would have more lane, so I don't think this should go in DT. Otherwise
>>> we'd have to encode there pretty much any parameter that could one day
>>> possibly change in a different universe :-)
>>>
>>> Let's also note that even when parameters change between IP versions, it
>>> doesn't always make sense to specify them explicitly in DT. It's totally
>>> fine to have a table of parameter values in the driver, indexed by
>>> compatible string. Whether to set a parameter explicitly in DT or handle
>>> it in the driver usually depends on how frequently that parameter can
>>> change, if it can vary between different integrations of the same IP
>>> version, ...
>>>
>>> In this specific case, as there's no foreseen change, we can't really
>>> tell how it would change if it did one day. I thus think hardcoding the
>>> parameter in the driver is fine, and in the worst case, we can add a
>>> parameter in DT later and default to 4 if not specified. Same reasoning
>>> for CONTROLLERS_PER_LANE.
>>
>> yeah not every parameter can be coded and we should use compatible as
>> well, but I would disagree with no future revision planned. It will
>> happen not now, but sometime in year or so :) Been around devices has
>> taught me that only constant thing is change in hardware!
> 
> I don't dispute it will not happen, my point is that, without a new
> revision planned, we don't have enough information to tell which option
> would be best to support future revisions. As I know we won't be blocked
> (both compat string matching and adding new optional properties with
> appropriate defaults will be viable options), I'm not concerned.

I also don't think this should got to DT. If there is any change in
number of lanes in future very likely there will be also different
changes connected to that as well. Then we can have discussion if makes
sense to even use this driver or write different one because of
different feature set and handling.
Also if IP doesn't change still it can become optional DT parameter with
default to 4 if not defined.

Thanks,
Michal