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
| ||
|
Message-ID: <3b6d6d70-c754-d26b-bdba-6046086868fa@prolan.hu> Date: Wed, 31 Aug 2022 16:04:10 +0200 From: Csókás Bence <csokas.bence@...lan.hu> To: Richard Cochran <richardcochran@...il.com> CC: <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, "Jakub Kicinski" <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>, <kernel@...gutronix.de>, Marc Kleine-Budde <mkl@...gutronix.de> Subject: Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on` On 2022. 08. 31. 15:54, Richard Cochran wrote: > On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote: > >> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c >> index c74d04f4b2fd..dc8564a1f2d2 100644 >> --- a/drivers/net/ethernet/freescale/fec_ptp.c >> +++ b/drivers/net/ethernet/freescale/fec_ptp.c >> @@ -365,21 +365,21 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) >> */ >> static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts) >> { >> - struct fec_enet_private *adapter = >> + struct fec_enet_private *fep = >> container_of(ptp, struct fec_enet_private, ptp_caps); >> u64 ns; >> - unsigned long flags; >> + unsigned long flags, flags2; >> >> - mutex_lock(&adapter->ptp_clk_mutex); >> + spin_lock_irqsave(&fep->ptp_clk_lock, flags); >> /* Check the ptp clock */ >> - if (!adapter->ptp_clk_on) { >> - mutex_unlock(&adapter->ptp_clk_mutex); >> + if (!fep->ptp_clk_on) { > > BTW This test is silly. If functionality isn't available then the code > should simply not register the clock in the first place. As I understand, `ptp_clk_on` is a flag indicating whether `clk_ipg` is running or not, and not a capability thing. The driver switches it run-time in `fec_enet_clk_enable()`. > >> + spin_unlock_irqrestore(&fep->ptp_clk_lock, flags); >> return -EINVAL; >> } >> - spin_lock_irqsave(&adapter->tmreg_lock, flags); >> - ns = timecounter_read(&adapter->tc); >> - spin_unlock_irqrestore(&adapter->tmreg_lock, flags); >> - mutex_unlock(&adapter->ptp_clk_mutex); >> + spin_lock_irqsave(&fep->tmreg_lock, flags2); >> + ns = timecounter_read(&fep->tc); >> + spin_unlock_irqrestore(&fep->tmreg_lock, flags2); >> + spin_unlock_irqrestore(&fep->ptp_clk_lock, flags); > > Two spin locks? Why not just use one? One guards the FEC_ATIME_* registers, the other the `ptp_clk_on` flag. For instance, `fec_ptp_adjfreq()` takes `tmreg_lock` but not `ptp_clk_lock`, and similarly `fec_enet_clk_enable()` takes `ptp_clk_lock` but not `tmreg_lock`. > > Thanks, > Richard
Powered by blists - more mailing lists