netdev - Re: [PATCH] fec: Restart PPS after link state change

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: <91d6794e-9e22-2a7c-b40a-436da63c78b5@prolan.hu>
Date:   Thu, 11 Aug 2022 16:45:14 +0200
From:   Csókás Bence <csokas.bence@...lan.hu>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Fugang Duan <fugang.duan@....com>
Subject: Re: [PATCH] fec: Restart PPS after link state change

On 2022. 08. 11. 15:30, Andrew Lunn wrote:
>> `fep->pps_enable` is the state of the PPS the driver *believes* to be the
>> case. After a reset, this belief may or may not be true anymore: if the
>> driver believed formerly that the PPS is down, then after a reset, its
>> belief will still be correct, thus nothing needs to be done about the
>> situation. If, however, the driver thought that PPS was up, after controller
>> reset, it no longer holds, so we need to update our world-view
>> (`fep->pps_enable = 0;`), and then correct for the fact that PPS just
>> unexpectedly stopped.
> 
> Your way of doing it just seems very unclean. I would make
> fec_ptp_enable_pps() read the actual status from the
> hardware. fep->pps_enable then has the clear meaning of user space
> requested it should be enabled.

1. It is not "my way", it is how it was in the original code. I am 
merely following those who came before me.
2. There is already a variable which holds userspace's wish: parameter 
`uint enable` in `fec_ptp_enable_pps()`. `fep->pps_enable` is whether 
the driver already fulfilled this wish.

> 
> 	  Andrew

Honestly, I would rather see the entire `fec` driver re-written from 
scratch, it is really bad code and full of bugs. Plus, Fugang Duan's 
mail server keeps bouncing back all my emails (I can only hope he sees 
these mails through the mailing list). However, that exceeds my 
capabilities unfortunately (I know not nearly enough of the various 
fec-based controllers and their internals, I only have the i.MX6 TX6UL 
to test). So the best I can do is provide fixes to the bugs we 
experienced, while changing as little of the original driver's code as 
possible, so as to (hopefully) not introduce even more bugs.

Bence