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: <2fb5dab7-f266-4304-a637-2b9eabb1184f@microchip.com>
Date: Mon, 7 Oct 2024 07:51:36 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <kuba@...nel.org>
CC: <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<ramon.nordin.rodriguez@...roamp.se>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>,
	<Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next v3 2/7] net: phy: microchip_t1s: update new
 initial settings for LAN865X Rev.B0

Hi Jakub,

Thanks for your review comments.

On 05/10/24 12:20 am, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote:
>> +     cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
>> +                      FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
>> +                      0x03;
>> +     cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
> 
> It's really strange to OR together FIELD_PREP()s with overlapping
> fields. What's going on here? 15:10 and 15:4 ranges overlap, then
> there is 0x3 hardcoded, with no fields size definition.
This calculation has been implemented based on the logic provided in the 
configuration application note (AN1760) released with the product. 
Please refer the link [1] below for more info.

As mentioned in the AN1760 document, "it provides guidance on how to 
configure the LAN8650/1 internal PHY for optimal performance in 
10BASE-T1S networks." Unfortunately we don't have any other information 
on those each and every parameters and constants used for the 
calculation. They are all derived by design team to bring up the device 
to the nominal state.

It is also mentioned as, "The following parameters must be calculated 
from the device configuration parameters mentioned above to use for the
configuration of the registers."

uint16 cfgparam1 = (uint16) (((9 + offset1) & 0x3F) << 10) | (uint16) 
(((14 + offset1) & 0x3F) << 4) | 0x03
uint16 cfgparam2 = (uint16) (((40 + offset2) & 0x3F) << 10)

This is the reason why the above logic has been implemented.

> Could you clarify and preferably name as many of the constants
> as possible?
I would like to do that but as I mentioned above there is no info on 
those constants in the application note.
> 
> Also why are you masking the result of the sum with 0x3f?
> Can the result not fit? Is that safe or should we error out?
Hope the above info clarifies this as well.
> 
>> +             ret &= GENMASK(4, 0);
> ?               if (ret & BIT(4))
> 
> GENMASK() is nice but naming the fields would be even nicer..
> What's 3:0, what's 4:4 ?
As per the information provided in the application note, the offset 
value expected range is from -5 to 15. Offsets are stored as signed 
5-bit values in the addresses 0x04 and 0x08. So 0x1F is used to mask the 
5-bit value and if the 4th bit is set then the value from 27 to 31 will 
be considered as -ve value from -5 to -1.

I think adding the above comment in the above code snippet will clarify 
the need. What do you think?

Link:
[1]: 
https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ApplicationNotes/ApplicationNotes/LAN8650-1-Configuration-Appnote-60001760.pdf

Best regards,
Parthiban V
> --
> pw-bot: cr

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ