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

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] [day] [month] [year] [list]

Message-ID: <200f7d11-50c2-4ee2-a80b-15341fbbd5f4@lunn.ch>
Date: Thu, 15 May 2025 03:23:39 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Marek Pazdan <mpazdan@...sta.com>
Cc: aleksander.lobakin@...el.com, almasrymina@...gle.com,
	andrew+netdev@...n.ch, anthony.l.nguyen@...el.com,
	daniel.zahka@...il.com, davem@...emloft.net, ecree.xilinx@...il.com,
	edumazet@...gle.com, gal@...dia.com, horms@...nel.org,
	intel-wired-lan@...ts.osuosl.org, jianbol@...dia.com,
	kory.maincent@...tlin.com, kuba@...nel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, przemyslaw.kitszel@...el.com, willemb@...gle.com
Subject: Re: [Intel-wired-lan] [PATCH net-next v2 2/2] ice: add qsfp
 transceiver reset, interrupt and presence pin control

> + * 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