netdev - Re: 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63faa826-2e87-f73e-6c1f-34fbced9d220@st.com>
Date:   Wed, 19 Oct 2016 09:16:26 +0200
From:   Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:     Nicolas Pitre <nicolas.pitre@...aro.org>
CC:     Alexandre TORGUE <alexandre.torgue@...com>,
        Rayagond Kokatanur <rayagond@...avyalabs.com>,
        "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: bad commit touching stmmac_ptp.c

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
>