Re: bad commit touching stmmac_ptp.c

[Richard Cochran]

On Wed, Oct 19, 2016 at 12:16:09AM -0400, Nicolas Pitre wrote:
> Was this patch actually reviewed?

And please, PLEASE put the PTP maintainer on CC for patches that use the
PTP subsystem!

Thanks,
Richard

Re: bad commit touching stmmac_ptp.c

[Giuseppe CAVALLARO]

Hello Nicolas

I have just sent a new patch to try to fix the problems you
raised.

Please let me know if

   [PATCH (net.git)] stmmac: fix and review the ptp registration

actually covers and fixes the points.

FYI, I am trying to review the PTP, especially for
for the GMAC4, in these days so I will send other
patches on top of this if OK.

Thanks for your advice and warning.

Peppe

On 10/19/2016 6:16 AM, Nicolas Pitre wrote:

Hello,

I noticed a recently added commit 7086605a6a ("stmmac: fix error check
when init ptp") to the mainline linux tree from you. This commit is
wrong.  The affected code now reads as:

int stmmac_ptp_register(struct stmmac_priv *priv)
{
spin_lock_init(&priv->ptp_lock);
priv->ptp_clock_ops = stmmac_ptp_clock_ops;

priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
 priv->device);
if (IS_ERR(priv->ptp_clock)) {
priv->ptp_clock = NULL;
return PTR_ERR(priv->ptp_clock);
}

spin_lock_init(&priv->ptp_lock);

netdev_dbg(priv->dev, "Added PTP HW clock successfully\n");

return 0;
}

Firstly, you basically reverted the change I did with commit
efee95f42b ("ptp_clock: future-proofing drivers against PTP subsystem
becoming optional").  Please have a look at that commit and ponder its
implications.

Secondly, the error you're actually returning to the caller with your
patch is actually PTR_ERR(NULL) which is basically a more convoluted way
to return the same value as what was returned before your patch, which
is probably not what you intended.

And finally you added a needless initialization of priv->ptp_lock given
that this was already done a few lines before that addition.

Was this patch actually reviewed?


Nicolas

bad commit touching stmmac_ptp.c

[Nicolas Pitre]

Hello,

I noticed a recently added commit 7086605a6a ("stmmac: fix error check 
when init ptp") to the mainline linux tree from you. This commit is 
wrong.  The affected code now reads as:

int stmmac_ptp_register(struct stmmac_priv *priv)
{
spin_lock_init(&priv->ptp_lock);
priv->ptp_clock_ops = stmmac_ptp_clock_ops;

priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
 priv->device);
if (IS_ERR(priv->ptp_clock)) {
priv->ptp_clock = NULL;
return PTR_ERR(priv->ptp_clock);
}

spin_lock_init(&priv->ptp_lock);

netdev_dbg(priv->dev, "Added PTP HW clock successfully\n");

return 0;
}

Firstly, you basically reverted the change I did with commit 
efee95f42b ("ptp_clock: future-proofing drivers against PTP subsystem 
becoming optional").  Please have a look at that commit and ponder its 
implications.

Secondly, the error you're actually returning to the caller with your 
patch is actually PTR_ERR(NULL) which is basically a more convoluted way 
to return the same value as what was returned before your patch, which 
is probably not what you intended.

And finally you added a needless initialization of priv->ptp_lock given 
that this was already done a few lines before that addition.

Was this patch actually reviewed?


Nicolas