Re: [PATCH iwl-next 2/10] ice: allow setting advertised speed and duplex for all media types

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: allow setting advertised speed and duplex for all media types

This commit allows setting advertised speed and duplex for all media types by
only refusing media types NONE or UNKNOWN in strict mode. It adds AUI and NONE
to the ice_media_type enum.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index e6a20af6f63de..62d214c5582ad 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -2634,10 +2635,14 @@ ice_set_link_ksettings(struct net_device *netdev,
>  	if (!pi)
>  		return -EIO;
>  
> -	if (pi->phy.media_type != ICE_MEDIA_BASET &&
> -	    pi->phy.media_type != ICE_MEDIA_FIBER &&
> -	    pi->phy.media_type != ICE_MEDIA_BACKPLANE &&
> -	    pi->phy.media_type != ICE_MEDIA_DA &&
> +	lenient_mode = test_bit(ICE_FLAG_LINK_LENIENT_MODE_ENA, pf->flags);
> +
> +	/* Setting the speed and duplex advertised by autonegotiation is
> +	 * supported for all media types, so only return unsupported for media
> +	 * type none or unknown in strict mode.
> +	 */
> +	if ((pi->phy.media_type == ICE_MEDIA_NONE ||
> +	     (pi->phy.media_type == ICE_MEDIA_UNKNOWN && !lenient_mode)) &&
>  	    pi->phy.link_info.link_info & ICE_AQ_LINK_UP)
>  		return -EOPNOTSUPP;

Will AUI media types still be rejected here in strict mode?

The patch adds ICE_MEDIA_AUI and ICE_MEDIA_NONE to enum ice_media_type, but
doesn't appear to update ice_get_media_type() to actually return these new
values. 

If AUI media continues to fall through to ICE_MEDIA_UNKNOWN, wouldn't the new
logic still reject it when lenient mode is disabled, defeating the intent of
the patch?

[ ... ]
> @@ -2667,7 +2672,7 @@ ice_set_link_ksettings(struct net_device *netdev,
>  	if (!bitmap_subset(copy_ks.link_modes.advertising,
>  			   safe_ks.link_modes.supported,
>  			   __ETHTOOL_LINK_MODE_MASK_NBITS)) {
> -		if (!test_bit(ICE_FLAG_LINK_LENIENT_MODE_ENA, pf->flags))
> +		if (!lenient_mode)
>  			netdev_info(netdev, "The selected speed is not supported by the current media. Please select a link speed that is supported by the current media.\n");
>  		err = -EOPNOTSUPP;
>  		goto done;

This isn't a bug introduced by this patch, but does taking this error path
accidentally release another thread's lock?

The ICE_CFG_BUSY flag is used as an ad-hoc lock later in this function:

	while (test_and_set_bit(ICE_CFG_BUSY, pf->state)) {
		timeout--;
		if (!timeout) {
			err = -EBUSY;
			goto done;
		}
		usleep_range(TEST_SET_BITS_SLEEP_MIN, TEST_SET_BITS_SLEEP_MAX);
	}

If the bitmap_subset() check fails, or if the while loop times out, it jumps to
the done label:

done:
	kfree(phy_caps);
	clear_bit(ICE_CFG_BUSY, pf->state);

Since the bitmap_subset() check happens before test_and_set_bit() has acquired
the lock, wouldn't this unconditionally clear the ICE_CFG_BUSY flag even if
another thread currently holds it? Could this synchronization be replaced with a
standard lock to avoid these issues?