Re: [PATCH net v7 3/3] net: dsa: microchip: monitor potential faults in half-duplex mode

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Jun 21, 2024 at 04:43:22PM +0200, Enguerrand de Ribaucourt wrote:
> The errata DS80000754 recommends monitoring potential faults in
> half-duplex mode for the KSZ9477 family.
> 
> 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.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
> ---
> v7:
>  - use dev_crit_once instead of dev_crit_ratelimited
>    The condition will remain true forever and the routine called every
>    5 seconds. There's no need to repeat the message.
>  - add explanations in the comment above the warning to help users
>    anticipate the consequences of the fault.
> v6: https://lore.kernel.org/netdev/20240614094642.122464-4-enguerrand.de-ribaucourt@savoirfairelinux.com/
>  - use macros for PORT_INTF_SPEED_MASK check
>  - add VLAN condition before checking the resources
> v5: https://lore.kernel.org/all/20240604092304.314636-5-enguerrand.de-ribaucourt@savoirfairelinux.com/
>  - use macros for bitmasks
>  - check for return values on ksz_pread*
> v4: https://lore.kernel.org/all/20240531142430.678198-6-enguerrand.de-ribaucourt@savoirfairelinux.com/
>  - rebase on net/main
>  - add Fixes tag
>  - reverse x-mas tree
> v3: https://lore.kernel.org/all/20240530102436.226189-6-enguerrand.de-ribaucourt@savoirfairelinux.com/
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 51 +++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz9477.h     |  2 +
>  drivers/net/dsa/microchip/ksz9477_reg.h | 10 ++++-
>  drivers/net/dsa/microchip/ksz_common.c  | 11 ++++++
>  drivers/net/dsa/microchip/ksz_common.h  |  1 +
>  5 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index c2878dd0ad7e..6924b3818357 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -429,6 +429,57 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
>  	mutex_unlock(&p->mib.cnt_mutex);
>  }
>  
> +int ksz9477_errata_monitor(struct ksz_device *dev, int port,
> +			   u64 tx_late_col)
> +{
> +	u32 pmavbc;
> +	u8 status;
> +	u16 pqm;
> +	int ret;
> +
> +	ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> +	if (ret)
> +		return ret;
> +	if (!(FIELD_GET(PORT_INTF_SPEED_MASK, status) == PORT_INTF_SPEED_NONE) &&
> +	    !(status & PORT_INTF_FULL_DUPLEX)) {
> +		/* Errata DS80000754 recommends monitoring potential faults in
> +		 * half-duplex mode. The switch might not be able to communicate anymore
> +		 * in these states.
> +		 * If you see this message, please read the errata-sheet for more information:

While I realise that the URL is long, netdev still prefers lines to be
no longer than 80 characters, so please wrap the comment accordingly.
The URL is probably fine (since there isn't any option), and then
dev_warn_once() string shouldn't be broken over separate lones either.

However, also consider whether moving this code block into a function
would tidy things up - the compiler can automatically  inline static
functions (and does in many cases, especially when they only have one
callsite.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!