From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Joakim Zhang <qiangqing.zhang@nxp.com> Cc: "davem@davemloft.net" <davem@davemloft.net>, "kuba@kernel.org" <kuba@kernel.org>, "peppe.cavallaro@st.com" <peppe.cavallaro@st.com>, "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>, "joabreu@synopsys.com" <joabreu@synopsys.com>, "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, dl-linux-imx <linux-imx@nxp.com> Subject: Re: [PATCH net 1/2] net: fec_ptp: add clock rate zero check Date: Wed, 16 Jun 2021 14:28:59 +0100 [thread overview] Message-ID: <20210616132859.GE22278@shell.armlinux.org.uk> (raw) In-Reply-To: <VI1PR04MB68004D29895652864B4C1D20E60F9@VI1PR04MB6800.eurprd04.prod.outlook.com> 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!
WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Joakim Zhang <qiangqing.zhang@nxp.com> Cc: "davem@davemloft.net" <davem@davemloft.net>, "kuba@kernel.org" <kuba@kernel.org>, "peppe.cavallaro@st.com" <peppe.cavallaro@st.com>, "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>, "joabreu@synopsys.com" <joabreu@synopsys.com>, "mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, dl-linux-imx <linux-imx@nxp.com> Subject: Re: [PATCH net 1/2] net: fec_ptp: add clock rate zero check Date: Wed, 16 Jun 2021 14:28:59 +0100 [thread overview] Message-ID: <20210616132859.GE22278@shell.armlinux.org.uk> (raw) In-Reply-To: <VI1PR04MB68004D29895652864B4C1D20E60F9@VI1PR04MB6800.eurprd04.prod.outlook.com> 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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-16 13:29 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-16 9:14 [PATCH net 0/2] net: fixes for fec ptp Joakim Zhang 2021-06-16 9:14 ` Joakim Zhang 2021-06-16 9:14 ` [PATCH net 1/2] net: fec_ptp: add clock rate zero check Joakim Zhang 2021-06-16 9:14 ` Joakim Zhang 2021-06-16 10:20 ` Russell King (Oracle) 2021-06-16 10:20 ` Russell King (Oracle) 2021-06-16 11:40 ` Joakim Zhang 2021-06-16 11:40 ` Joakim Zhang 2021-06-16 13:28 ` Russell King (Oracle) [this message] 2021-06-16 13:28 ` Russell King (Oracle) 2021-06-16 9:14 ` [PATCH net 2/2] net: fec_ptp: fix issue caused by refactor the fec_devtype Joakim Zhang 2021-06-16 9:14 ` Joakim Zhang 2021-06-16 19:50 ` [PATCH net 0/2] net: fixes for fec ptp patchwork-bot+netdevbpf 2021-06-16 19:50 ` patchwork-bot+netdevbpf
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210616132859.GE22278@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=alexandre.torgue@foss.st.com \ --cc=davem@davemloft.net \ --cc=joabreu@synopsys.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=peppe.cavallaro@st.com \ --cc=qiangqing.zhang@nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.