linux-kernel - Re: [PATCH net v4] phy: mscc: Fix timestamping for vsc8584

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: <20250819082546.7455mixbmqccsv5p@skbuf>
Date: Tue, 19 Aug 2025 11:25:46 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, richardcochran@...il.com,
	rmk+kernel@...linux.org.uk, rosenp@...il.com,
	christophe.jaillet@...adoo.fr, viro@...iv.linux.org.uk,
	quentin.schulz@...tlin.com, atenart@...nel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v4] phy: mscc: Fix timestamping for vsc8584

On Tue, Aug 19, 2025 at 08:40:11AM +0200, Horatiu Vultur wrote:
> The 08/18/2025 17:37, Vladimir Oltean wrote:
> > 
> > On Mon, Aug 18, 2025 at 04:19:25PM +0200, Horatiu Vultur wrote:
> > > Nothing prevents me for looking at this issue. I just need to alocate
> > > some time for this.
> > >
> > > > The two problems are introduced by the same commit, and fixes will be
> > > > backported to all the same stable kernels. I don't exactly understand
> > > > why you'd add some code to the PHY's remove() method, but not enough in
> > > > order for it to work.
> > >
> > > Yes, I understand that but the fix for ptp_clock_unregister will fix a
> > > different issue that this patch is trying to fix. That is the reason why
> > > I prefer not to add that fix now, just to make things more clear.
> > 
> > Not sure "clear" for whom. One of the rules from Documentation/process/stable-kernel-rules.rst
> > is "It must be obviously correct and tested.", which to me makes it confusing
> > why you wouldn't fix that issue first (within the same patch set), and then
> > test this patch during unbind/bind to confirm that it achieves what it intends.
> 
> I have tested the patch by inserting and removing the kernel module. And
> I have check that remove function was called and see that it tries to
> flush the queue.

Ok, it's great that you tested it.

> > I think the current state of the art is that unbinding a PHY that the
> > MAC hasn't connected to will work, whereas unbinding a connected PHY,
> > where the state machine is running, will crash the kernel. To be
> > perfectly clear, the request is just for the case that is supposed to
> > work given current phylib implementation, aka with the MAC unconnected
> > (put administratively down or also unbound, depending on whether it
> > connects to the PHY at probe time or ndo_open() time).
> > 
> > I don't see where the reluctance comes from - is it that there are going
> > to be 2 patches instead of 1? My reluctance as a reviewer comes from the
> > fact that I'm analyzing the change in the larger context and not seeing
> > how the remove() method you introduced makes any practical difference.
> > Not sure what I'm supposed to say.
> 
> I don't have anything against it, like I said before I thought those are
> 2 different issues. But if you think otherwise I can add a new patch in
> this series, no problem.
> 
> Why do you say that the function remove() doesn't make any practical
> difference?

I had thought that the rx_skbs_list can still be queued to, through the
dangling ops that are left behind in /sys/class/ptp/ when the PHY driver
is removed. But it looks like this isn't the case, and the issues are
indeed unrelated.