netdev - bad commit touching stmmac

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-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.20.1610182339340.25105@knanqh.ubzr>
Date:   Wed, 19 Oct 2016 00:16:09 -0400 (EDT)
From:   Nicolas Pitre <nicolas.pitre@...aro.org>
To:     Giuseppe CAVALLARO <peppe.cavallaro@...com>
cc:     Alexandre TORGUE <alexandre.torgue@...com>,
        Rayagond Kokatanur <rayagond@...avyalabs.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: bad commit touching stmmac_ptp.c

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