| 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: <YmLM8pFMVifHj7GG@lunn.ch>
Date: Fri, 22 Apr 2022 17:42:42 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Lasse Johnsen <lasse@...ebeat.app>
Cc: Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
Gordon Hollingworth <gordon@...pberrypi.com>,
Ahmad Byagowi <clk@...com>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
bcm-kernel-feedback-list@...adcom.com,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on
bcm54210pe
On Fri, Apr 22, 2022 at 04:08:18PM +0100, Lasse Johnsen wrote:
Hi Lasse
Don't attach a patch to the end of a discussion. What you email is
what comes out of git-format patch. Nothing added.
If you want to discuss review comments, just reply to the email with
the comments.
> From 3fcbbac9fe85909877f15d95f7a1e64dd6d06ab7 Mon Sep 17 00:00:00 2001
> From: Lasse Johnsen <l@...johnsen.me>
> Date: Sat, 5 Feb 2022 09:34:19 -0500
> Subject: [PATCH] Added support for IEEE1588 timestamping for the BCM54210PE
> PHY using the kernel mii_timestamper interface
>
> ---
> arch/arm/configs/bcm2711_defconfig | 1 +
> arch/arm64/configs/bcm2711_defconfig | 1 +
> .../net/ethernet/broadcom/genet/bcmgenet.c | 8 +-
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/bcm54210pe_ptp.c | 1382 +++++++++++++++++
> drivers/net/phy/bcm54210pe_ptp.h | 85 +
> drivers/net/phy/broadcom.c | 21 +-
> drivers/ptp/Kconfig | 17 +
You need to break this up into a patch series. You probably want the
following patches:
defconfig changes
The core ptp code, in library form
Extensions to drivers/net/phy/broadcom.c to use the new code
bcmgenet.c change.
> +static u64 four_u16_to_ns(u16 *four_u16)
> +{
> + u32 seconds;
> + u32 nanoseconds;
> + struct timespec64 ts;
> + u16 *ptr;
Now it has been through checkpatch, it is starting to look like Linux
code :-)
Network drivers have a few extra code style hoops to jump
through. Variables should be sorted longest to shortest. So you want:
> + struct timespec64 ts;
> + u32 nanoseconds;
> + u32 seconds;
> + u16 *ptr;
This is known as reverse Christmas tree.
> +static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)
Although Linux as a whole allows 100 character lines, networking
mostly stays with 80. I'm not sure it is strictly enforced, but it is
a good idea to try to keep with it.
> +{
> + u16 interrupt_mask;
> +
> + interrupt_mask = 0;
You can combine these into one line.
> +static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
> + struct timespec64 *ts,
> + struct ptp_system_timestamp *sts)
> +{
> + struct phy_device *phydev;
> + u16 nco_6_register_value;
> + int i;
> + u64 time_stamp_48, time_stamp_80, control_ts;
> +
> + phydev = private->phydev;
> +
> + // Capture timestamp on next framesync
> + nco_6_register_value = 0x2000;
You should not have magic numbers. Please add a #define. If the
#define has a sensible name, it should then be obvious you are
capturing a timestamp on the next frame sync and so you don't need the
comment.
> +
> + // Lock
> + mutex_lock(&private->clock_lock);
Comments like this don't add any value. I can see it is a lock because
you are calling mutex_lock.
> +static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period,
> + s64 pulsewidth, int enable)
> +{
> + struct phy_device *phydev;
> + u16 nco_6_register_value, frequency_hi, frequency_lo,
> + pulsewidth_reg, pulse_start_hi, pulse_start_lo;
> + int err;
> +
> + phydev = private->phydev;
> +
> + if (enable) {
> + frequency_hi = 0;
> + frequency_lo = 0;
> + pulsewidth_reg = 0;
> + pulse_start_hi = 0;
> + pulse_start_lo = 0;
> +
> + // Convert interval pulse spacing (period) and pulsewidth to 8 ns units
> + period /= 8;
> + pulsewidth /= 8;
> +
> + // Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
> + if (pulsewidth == 0) {
> + if (period < 2500) {
> + // At a frequency at less than 20us (2500 x 8ns) set
> + // pulse length to 1/10th of the interval pulse spacing
> + pulsewidth = period / 10;
> +
> + // Where the interval pulse spacing is short,
> + // ensure we set a pulse length of 8ns
> + if (pulsewidth == 0)
> + pulsewidth = 1;
> +
> + } else {
> + // Otherwise set pulse with to 4us (8ns x 500 = 4us)
> + pulsewidth = 500;
> + }
> + }
> +
> + if (private->perout_mode == SYNC_OUT_MODE_1) {
> + // Set period
> + private->perout_period = period;
> +
> + if (!private->perout_en) {
> + // Set enable per_out
> + private->perout_en = true;
> + schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
> + }
> +
> + err = 0;
> +
> + } else if (private->perout_mode == SYNC_OUT_MODE_2) {
> + // Set enable per_out
> + private->perout_en = true;
> +
> + // Calculate registers
> +
> + // Lowest 16 bits of 8ns interval pulse spacing [15:0]
> + frequency_lo = (u16)period;
> +
> + // Highest 14 bits of 8ns interval pulse spacing [29:16]
> + frequency_hi = (u16)(0x3FFF & (period >> 16));
> +
> + // 2 lowest bits of 8ns pulse length [1:0]
> + frequency_hi |= (u16)pulsewidth << 14;
> +
> + // 7 highest bit of 8 ns pulse length [8:2]
> + pulsewidth_reg = (u16)(0x7F & (pulsewidth >> 2));
> +
> + // Get base value
> + nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> + nco_6_register_value,
> + true);
> +
> + mutex_lock(&private->clock_lock);
> +
> + // Write to register
> + err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
> + nco_6_register_value);
> +
> + // Set sync out pulse interval spacing and pulse length
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_reg);
> +
> + // On next framesync load sync out frequency
> + err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0200);
> +
> + // Trigger immediate framesync
> + err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> + mutex_unlock(&private->clock_lock);
> + }
> + } else {
> + // Set disable pps
> + private->perout_en = false;
> +
> + // Get base value
> + nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> + nco_6_register_value,
> + false);
> +
> + mutex_lock(&private->clock_lock);
> +
> + // Write to register
> + err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> + mutex_unlock(&private->clock_lock);
> + }
> +
> + return err;
This is a pretty big function, and its indentation gets pretty deep. The coding style:
https://www.kernel.org/doc/html/latest/process/coding-style.html#functions
says:
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen size
is 80x24, as we all know), and do one thing and do that well.
Maybe you can break this up into helpers.
> +void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
> +{
> + struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private,
> + mii_ts);
> +
> + switch (private->hwts_tx_en) {
> + case HWTSTAMP_TX_ON:
> + {
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> + skb_queue_tail(&private->tx_skb_queue, skb);
> + schedule_work(&private->txts_work);
> + break;
> + }
> +
> + case HWTSTAMP_TX_OFF:
> + {
> + }
That just looks odd.
Should there be a break? Do you want to fall through? If you do want
to fall through, you need to mark it so. But since it is empty, i
guess you don't want to fall through?
> +
> + default:
> + {
> + kfree_skb(skb);
> + break;
> + }
> + }
> +}
Powered by blists - more mailing lists