Subject: Re: [PATCH net 1/2] net: fec_ptp: add clock rate zero check

From: "Russell King (Oracle)" <[email-masked]>
Date: Wed, 16 Jun 2021 14:28:59 +0100

Hi Joakim,

On Wed, Jun 16, 2021 at 11:40:29AM +0000, Joakim Zhang wrote:
> Do you mean that print an error message then return directly? It seems better.

Nearly - one has to ensure that the cleanup functions don't provoke a
crash though. I notice fec_ptp_stop() makes use of fep->time_keep
and also fep->ptp_clock.

fep->time_keep is initialised after where you need to test for zero
cycle_speed, so the initialisation would need moving earlier.

I would have thought that ftp->ptp_clock should be NULL, so that's
probably okay, but should be checked that this assumption is in fact
true.

> if (!fep->cycle_speed) {
> 	dev_err(&fep->pdev->dev, "PTP clock rate should not be zero!\n");

I'd still say something like "PTP clock rate should not be zero,
disabling PTP" - say what's wrong and what we are doing. Also,
please avoid exclaimation marks in error messages.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!