Re: [Intel-wired-lan] [PATCH net-next v2 2/2] ice: add qsfp transceiver reset, interrupt and presence pin control

[Andrew Lunn <andrew@lunn.ch>]

> + * ice_get_module_mgmt_signal - get module management signal status
> + * @dev: network interface device structure
> + * @params: ethtool module management signal params
> + * @extack: extended ACK from the Netlink message
> + *
> + * Returns -EIO if AQ command for GPIO get failed, otherwise
> + * returns 0 and current status of requested signal in params.
> + */
> +static int
> +ice_get_module_mgmt_signal(struct net_device *dev,
> +			   struct ethtool_module_mgmt_params *params,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct ice_netdev_priv *np = netdev_priv(dev);
> +	struct ice_pf *pf = np->vsi->back;
> +	struct ice_hw *hw = &pf->hw;
> +	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
> +	bool value;
> +	int ret = 0;
> +
> +	if (hw->has_module_mgmt_gpio) {
> +		switch (params->type) {
> +		case ETHTOOL_MODULE_MGMT_RESET:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_RESET, &value, NULL);
> +			break;

Reset, i can kind of understand being used this way.

> +		case ETHTOOL_MODULE_MGMT_INT:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_INT, &value, NULL);
> +			break;
> +		case ETHTOOL_MODULE_MGMT_PRESENT:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_PRESENT, &value, NULL);
> +			break;

but not these two. These represent events. I've not looked at the
datasheet... Does the GPIO controller support interrupts? For PRESENT
you are interested in the edges, maybe with some debounce logic. For
INT, i _guess_ it is active low? But i've no idea what user space is
going to actually do on an interrupt, and how is it going to clear the
interrupt? This smells like a user space driver, which is not
something we want. Even if there is a legitimate use case, which there
might be for PRESENT, polling does not make much sense when the kernel
can broadcast a netlink message to user space if the GPIO controller
has interrupt support.

    Andrew

---
pw-bot: cr