netdev - Re: [PATCH net v4 5/5] net: dsa: microchip: monitor potential faults in half-duplex mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f24ce57f4e24da7c058ed15ecf2b7bf25237e998.camel@microchip.com>
Date: Fri, 31 May 2024 15:14:29 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <enguerrand.de-ribaucourt@...oirfairelinux.com>, <netdev@...r.kernel.org>
CC: <linux@...linux.org.uk>, <Woojung.Huh@...rochip.com>,
	<hkallweit1@...il.com>, <UNGLinuxDriver@...rochip.com>, <andrew@...n.ch>
Subject: Re: [PATCH net v4 5/5] net: dsa: microchip: monitor potential faults
 in half-duplex mode

Hi Enguerrand,

On Fri, 2024-05-31 at 14:24 +0000, Enguerrand de Ribaucourt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> The errata DS80000754 recommends monitoring potential faults in
> half-duplex mode for the KSZ9477 familly.
> 
> half-duplex is not very common so I just added a critical message
> when the fault conditions are detected. The switch can be expected
> to be unable to communicate anymore in these states and a software
> reset of the switch would be required which I did not implement.
> 
> 
>  
> 
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                           u64 tx_late_col)
> +{
> +       u32 pmavbc;
> +       u8 status;
> +       u16 pqm;
> +
> +       ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);

It good to check return value for ksz_pread8/16/32.

> +       if (!((status & PORT_INTF_SPEED_MASK) ==
> PORT_INTF_SPEED_MASK) &&
> +           !(status & PORT_INTF_FULL_DUPLEX)) {
> +               dev_warn_once(dev->dev,
> +                             "Half-duplex detected on port %d,
> transmission halt may occur\n",
> +                             port);
> +               /* Errata DS80000754 recommends monitoring potential
> faults in
> +                * half-duplex mode. The switch might not be able to
> communicate anymore
> +                * in these states.
> +                */
> +               if (tx_late_col != 0) {
> +                       /* Transmission halt with late collisions */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "TX late collisions
> detected, transmission may be halted on port %d\n",
> +                                            port);
> +               }
> +               ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4,
> &pqm);
> +               ksz_read32(dev, REG_PMAVBC, &pmavbc);
> +               if (((pmavbc & PMAVBC_MASK) >> PMAVBC_SHIFT <= 0x580)

magic numbers can be replaced by macros.
Also it can be replaced by FIELD_GET(PMAVBC_MASK, pmavbc)
> ||
> +                   ((pqm & PORT_QM_TX_CNT_M) >= 0x200)) {
> +                       /* Transmission halt with Half-Duplex and
> VLAN */
> +                       dev_crit_ratelimited(dev->dev,
> +                                            "resources out of
> limits, transmission may be halted\n");
> +               }
> +       }
> +}
> +
>  void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
>  {
>         struct ksz_port_mib *mib = &dev->ports[port].mib;
> diff --git a/drivers/net/dsa/microchip/ksz9477.h
> b/drivers/net/dsa/microchip/ksz9477.h
> index ce1e656b800b..3312ef28e99c 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -36,6 +36,8 @@ int ksz9477_port_mirror_add(struct ksz_device *dev,
> int port,
>                             bool ingress, struct netlink_ext_ack
> *extack);
>  void ksz9477_port_mirror_del(struct ksz_device *dev, int port,
>                              struct dsa_mall_mirror_tc_entry
> *mirror);
> +void ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +                           u64 tx_late_col);
>  void ksz9477_get_caps(struct ksz_device *dev, int port,
>                       struct phylink_config *config);
>  int ksz9477_fdb_dump(struct ksz_device *dev, int port,
> diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h
> b/drivers/net/dsa/microchip/ksz9477_reg.h
> index f3a205ee483f..3238b9748d0f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -842,8 +842,7 @@
> 
>  #define REG_PORT_STATUS_0              0x0030
> 
> -#define PORT_INTF_SPEED_M              0x3
> -#define PORT_INTF_SPEED_S              3
> +#define PORT_INTF_SPEED_MASK           0x0018

nitpick: it can be replaced by GENMASK(4,3)

>  #define PORT_INTF_FULL_DUPLEX          BIT(2)
>  #define PORT_TX_FLOW_CTRL              BIT(1)
>  #define PORT_RX_FLOW_CTRL              BIT(0)
> @@ -1167,6 +1166,11 @@
>  #define PORT_RMII_CLK_SEL              BIT(7)
>  #define PORT_MII_SEL_EDGE              BIT(5)
> 
> +#define REG_PMAVBC                             0x03AC
> +
> +#define PMAVBC_MASK                            0x7ff0000

nitpick: it can be replaced by GENMASK(26, 16)

> +#define PMAVBC_SHIFT                   16
> +
>  /* 4 - MAC */
>  #define REG_PORT_MAC_CTRL_0            0x0400
> 
> 
>