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