| 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: <CAMRHcQyp0MppaoL8fT+U7hh45zkZbFFRsDFU=nrYBpqFptTu6g@mail.gmail.com>
Date: Wed, 12 Feb 2025 23:32:46 +0100
From: Piotr Wejman <wejmanpm@...il.com>
To: "Lifshits, Vitaly" <vitaly.lifshits@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, intel-wired-lan@...ts.osuosl.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH v2] net: e1000e: convert to
ndo_hwtstamp_get() and ndo_hwtstamp_set()
On Wed, Feb 12, 2025 at 3:10 PM Lifshits, Vitaly
<vitaly.lifshits@...el.com> wrote:
>
>
>
> On 2/8/2025 5:43 PM, Piotr Wejman wrote:
> > Update the driver to use the new hardware timestamping API added in commit
> > 66f7223039c0 ("net: add NDOs for configuring hardware timestamping").
> > Use Netlink extack for error reporting in e1000e_hwtstamp_set.
> > Align the indentation of net_device_ops.
> >
> > Signed-off-by: Piotr Wejman <wejmanpm@...il.com>
> > ---
> > Changes in v2:
> > - amend commit message
> > - use extack for error reporting
> > - rename e1000_mii_ioctl to e1000_ioctl
> > - Link to v1: https://lore.kernel.org/netdev/20250202170839.47375-1-piotrwejman90@gmail.com/
> >
> > drivers/net/ethernet/intel/e1000e/e1000.h | 2 +-
> > drivers/net/ethernet/intel/e1000e/netdev.c | 68 ++++++++++------------
> > 2 files changed, 31 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
> > index ba9c19e6994c..952898151565 100644
> > --- a/drivers/net/ethernet/intel/e1000e/e1000.h
> > +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
> > @@ -319,7 +319,7 @@ struct e1000_adapter {
> > u16 tx_ring_count;
> > u16 rx_ring_count;
> >
> > - struct hwtstamp_config hwtstamp_config;
> > + struct kernel_hwtstamp_config hwtstamp_config;
> > struct delayed_work systim_overflow_work;
> > struct sk_buff *tx_hwtstamp_skb;
> > unsigned long tx_hwtstamp_start;
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> > index 286155efcedf..43933e64819b 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -3574,6 +3574,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca)
> > * e1000e_config_hwtstamp - configure the hwtstamp registers and enable/disable
> > * @adapter: board private structure
> > * @config: timestamp configuration
> > + * @extack: netlink extended ACK for error report
> > *
> > * Outgoing time stamping can be enabled and disabled. Play nice and
> > * disable it when requested, although it shouldn't cause any overhead
> > @@ -3587,7 +3588,8 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca)
> > * exception of "all V2 events regardless of level 2 or 4".
> > **/
> > static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > - struct hwtstamp_config *config)
> > + struct kernel_hwtstamp_config *config,
> > + struct netlink_ext_ack *extack)
> > {
> > struct e1000_hw *hw = &adapter->hw;
> > u32 tsync_tx_ctl = E1000_TSYNCTXCTL_ENABLED;
> > @@ -3598,8 +3600,10 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > bool is_l2 = false;
> > u32 regval;
> >
> > - if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP))
> > + if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP)) {
> > + NL_SET_ERR_MSG(extack, "No HW timestamp support\n");
> > return -EINVAL;
> > + }
> >
> > switch (config->tx_type) {
> > case HWTSTAMP_TX_OFF:
> > @@ -3608,6 +3612,7 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > case HWTSTAMP_TX_ON:
> > break;
> > default:
> > + NL_SET_ERR_MSG(extack, "Unsupported TX HW timestamp type\n");
> > return -ERANGE;
> > }
> >
> > @@ -3681,6 +3686,7 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > config->rx_filter = HWTSTAMP_FILTER_ALL;
> > break;
> > default:
> > + NL_SET_ERR_MSG(extack, "Unsupported RX HW timestamp filter\n");
> > return -ERANGE;
> > }
> >
> > @@ -3693,7 +3699,7 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > ew32(TSYNCTXCTL, regval);
> > if ((er32(TSYNCTXCTL) & E1000_TSYNCTXCTL_ENABLED) !=
> > (regval & E1000_TSYNCTXCTL_ENABLED)) {
> > - e_err("Timesync Tx Control register not set as expected\n");
> > + NL_SET_ERR_MSG(extack, "Timesync Tx Control register not set as expected\n");
>
> In the case where this function is being called from e1000e_systim_reset
> function, won't it cause this debug print to do nothing?
Yes, you're right.
>
> > return -EAGAIN;
> > }
> >
> > @@ -3706,7 +3712,7 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
> > E1000_TSYNCRXCTL_TYPE_MASK)) !=
> > (regval & (E1000_TSYNCRXCTL_ENABLED |
> > E1000_TSYNCRXCTL_TYPE_MASK))) {
> > - e_err("Timesync Rx Control register not set as expected\n");
>
> Same question here.
>
> > + NL_SET_ERR_MSG(extack, "Timesync Rx Control register not set as expected\n");
> > return -EAGAIN;
> > }
> >
> > @@ -3932,7 +3938,7 @@ static void e1000e_systim_reset(struct e1000_adapter *adapter)
> > spin_unlock_irqrestore(&adapter->systim_lock, flags);
> >
> > /* restore the previous hwtstamp configuration settings */
> > - e1000e_config_hwtstamp(adapter, &adapter->hwtstamp_config);
> > + e1000e_config_hwtstamp(adapter, &adapter->hwtstamp_config, NULL);
I'll pass an extack instead of NULL and add a print here.
> > }
> >
> > /**
> > @@ -6079,8 +6085,8 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
> > return 0;
> > }
> >
> > -static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> > - int cmd)
> > +static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr,
> > + int cmd)
> > {
> > struct e1000_adapter *adapter = netdev_priv(netdev);
> > struct mii_ioctl_data *data = if_mii(ifr);
> > @@ -6140,7 +6146,8 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> > /**
> > * e1000e_hwtstamp_set - control hardware time stamping
> > * @netdev: network interface device structure
> > - * @ifr: interface request
> > + * @config: timestamp configuration
> > + * @extack: netlink extended ACK report
> > *
> > * Outgoing time stamping can be enabled and disabled. Play nice and
> > * disable it when requested, although it shouldn't cause any overhead
> > @@ -6153,20 +6160,18 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> > * specified. Matching the kind of event packet is not supported, with the
> > * exception of "all V2 events regardless of level 2 or 4".
> > **/
> > -static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
> > +static int e1000e_hwtstamp_set(struct net_device *netdev,
> > + struct kernel_hwtstamp_config *config,
> > + struct netlink_ext_ack *extack)
> > {
> > struct e1000_adapter *adapter = netdev_priv(netdev);
> > - struct hwtstamp_config config;
> > int ret_val;
> >
> > - if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> > - return -EFAULT;
> > -
> > - ret_val = e1000e_config_hwtstamp(adapter, &config);
> > + ret_val = e1000e_config_hwtstamp(adapter, config, extack);
> > if (ret_val)
> > return ret_val;
> >
> > - switch (config.rx_filter) {
> > + switch (config->rx_filter) {
> > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > @@ -6178,38 +6183,23 @@ static int e1000e_hwtstamp_set(struct net_device *netdev, struct ifreq *ifr)
> > * by hardware so notify the caller the requested packets plus
> > * some others are time stamped.
> > */
> > - config.rx_filter = HWTSTAMP_FILTER_SOME;
> > + config->rx_filter = HWTSTAMP_FILTER_SOME;
> > break;
> > default:
> > break;
> > }
> >
> > - return copy_to_user(ifr->ifr_data, &config,
> > - sizeof(config)) ? -EFAULT : 0;
> > + return 0;
> > }
> >
> > -static int e1000e_hwtstamp_get(struct net_device *netdev, struct ifreq *ifr)
> > +static int e1000e_hwtstamp_get(struct net_device *netdev,
> > + struct kernel_hwtstamp_config *kernel_config)
> > {
> > struct e1000_adapter *adapter = netdev_priv(netdev);
> >
> > - return copy_to_user(ifr->ifr_data, &adapter->hwtstamp_config,
> > - sizeof(adapter->hwtstamp_config)) ? -EFAULT : 0;
> > -}
> > + *kernel_config = adapter->hwtstamp_config;
> >
> > -static int e1000_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> > -{
> > - switch (cmd) {
> > - case SIOCGMIIPHY:
> > - case SIOCGMIIREG:
> > - case SIOCSMIIREG:
> > - return e1000_mii_ioctl(netdev, ifr, cmd);
> > - case SIOCSHWTSTAMP:
> > - return e1000e_hwtstamp_set(netdev, ifr);
> > - case SIOCGHWTSTAMP:
> > - return e1000e_hwtstamp_get(netdev, ifr);
> > - default:
> > - return -EOPNOTSUPP;
> > - }
> > + return 0;
> > }
> >
> > static int e1000_init_phy_wakeup(struct e1000_adapter *adapter, u32 wufc)
> > @@ -7346,9 +7336,11 @@ static const struct net_device_ops e1000e_netdev_ops = {
> > #ifdef CONFIG_NET_POLL_CONTROLLER
> > .ndo_poll_controller = e1000_netpoll,
> > #endif
> > - .ndo_set_features = e1000_set_features,
> > - .ndo_fix_features = e1000_fix_features,
> > + .ndo_set_features = e1000_set_features,
> > + .ndo_fix_features = e1000_fix_features,
> > .ndo_features_check = passthru_features_check,
> > + .ndo_hwtstamp_get = e1000e_hwtstamp_get,
> > + .ndo_hwtstamp_set = e1000e_hwtstamp_set,
> > };
> >
> > /**
> >
>
>
> Also you are missing a subject prefix, I assume that you mean to send it
> to iwl-next since it is not a bug fix. Please add it to your patch.
Powered by blists - more mailing lists