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

[Horatiu Vultur]

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.

> 
> 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?

-- 
/Horatiu