Re: [PATCH net-next v2 14/21] motorcomm:yt6801: Implement the WOL function of ethtool_ops

[Andrew Lunn]

On Wed, Nov 20, 2024 at 06:56:18PM +0800, Frank Sae wrote:
> Implement the .get_wol and .set_wol callback function.
> 
> Signed-off-by: Frank Sae <Frank.Sae@xxxxxxxxxxxxxx>
> ---
>  .../motorcomm/yt6801/yt6801_ethtool.c         | 169 +++++
>  .../net/ethernet/motorcomm/yt6801/yt6801_hw.c | 576 ++++++++++++++++++
>  .../ethernet/motorcomm/yt6801/yt6801_net.c    | 118 ++++
>  3 files changed, 863 insertions(+)
> 
> diff --git a/drivers/net/ethernet/motorcomm/yt6801/yt6801_ethtool.c b/drivers/net/ethernet/motorcomm/yt6801/yt6801_ethtool.c
> index 7989ccbc3..af643a16a 100644
> --- a/drivers/net/ethernet/motorcomm/yt6801/yt6801_ethtool.c
> +++ b/drivers/net/ethernet/motorcomm/yt6801/yt6801_ethtool.c
> @@ -565,6 +565,173 @@ static int fxgmac_set_ringparam(struct net_device *netdev,
>  	return ret;
>  }
>  
> +static void fxgmac_get_wol(struct net_device *netdev,
> +			   struct ethtool_wolinfo *wol)
> +{
> +	struct fxgmac_pdata *pdata = netdev_priv(netdev);
> +
> +	wol->supported = WAKE_UCAST | WAKE_MCAST | WAKE_BCAST | WAKE_MAGIC |
> +			 WAKE_ARP | WAKE_PHY;

You are not asking the PHY what it can do. Ideally you want the PHY to
do WoL so you can power off the MAC. You should only use MAC WoL when
asked to do something the PHY does not support.

> +	wol->wolopts = 0;
> +	if (!(pdata->hw_feat.rwk) || !device_can_wakeup(pdata->dev)) {
> +		yt_err(pdata, "%s, pci does not support wakeup.\n", __func__);
> +		return;
> +	}
> +
> +	wol->wolopts = pdata->wol;
> +}
> +
> +#pragma pack(1)
> +/* it's better to make this struct's size to 128byte. */
> +struct pattern_packet {
> +	u8 ether_daddr[ETH_ALEN];
> +	u8 ether_saddr[ETH_ALEN];
> +	u16 ether_type;
> +
> +	__be16 ar_hrd;		/* format of hardware address  */
> +	__be16 ar_pro;		/* format of protocol          */
> +	u8 ar_hln;		/* length of hardware address  */
> +	u8 ar_pln;		/* length of protocol address  */
> +	__be16 ar_op;		/* ARP opcode (command)        */
> +	u8 ar_sha[ETH_ALEN];	/* sender hardware address     */
> +	u8 ar_sip[4];		/* sender IP address           */
> +	u8 ar_tha[ETH_ALEN];	/* target hardware address     */
> +	u8 ar_tip[4];		/* target IP address           */
> +
> +	u8 reverse[86];
> +};
> +
> +#pragma pack()
> +
> +static int fxgmac_set_pattern_data(struct fxgmac_pdata *pdata)
> +{
> +	struct fxgmac_hw_ops *hw_ops = &pdata->hw_ops;
> +	u8 type_offset, tip_offset, op_offset;
> +	struct wol_bitmap_pattern pattern[4];
> +	struct pattern_packet packet;
> +	u32 ip_addr, i = 0;
> +
> +	memset(pattern, 0, sizeof(struct wol_bitmap_pattern) * 4);

This is somewhat error prone. It is better to use

       memset(pattern, 0, sizeof(pattern));

> +
> +	/* Config ucast */
> +	if (pdata->wol & WAKE_UCAST) {
> +		pattern[i].mask_info[0] = 0x3F;

What does 0x3F mean? Can you replace the magic number with a #define
which explains it?

> +		pattern[i].mask_size = sizeof(pattern[0].mask_info);
> +		memcpy(pattern[i].pattern_info, pdata->mac_addr, ETH_ALEN);
> +		pattern[i].pattern_offset = 0;
> +		i++;
> +	}
> +
> +	/* Config bcast */
> +	if (pdata->wol & WAKE_BCAST) {
> +		pattern[i].mask_info[0] = 0x3F;
> +		pattern[i].mask_size = sizeof(pattern[0].mask_info);
> +		memset(pattern[i].pattern_info, 0xFF, ETH_ALEN);
> +		pattern[i].pattern_offset = 0;
> +		i++;
> +	}
> +
> +	/* Config mcast */
> +	if (pdata->wol & WAKE_MCAST) {
> +		pattern[i].mask_info[0] = 0x7;
> +		pattern[i].mask_size = sizeof(pattern[0].mask_info);
> +		pattern[i].pattern_info[0] = 0x1;
> +		pattern[i].pattern_info[1] = 0x0;
> +		pattern[i].pattern_info[2] = 0x5E;

I don't think that is the correct definition of multicast.

https://elixir.bootlin.com/linux/v6.12/source/include/linux/etherdevice.h#L122

You are just interested in one bit to indicate it is an Ethernet
multicast frame.

> +	/* Config arp */
> +	if (pdata->wol & WAKE_ARP) {
> +		memset(pattern[i].mask_info, 0, sizeof(pattern[0].mask_info));
> +		type_offset = offsetof(struct pattern_packet, ar_pro);
> +		pattern[i].mask_info[type_offset / 8] |= 1 << type_offset % 8;
> +		type_offset++;
> +		pattern[i].mask_info[type_offset / 8] |= 1 << type_offset % 8;
> +		op_offset = offsetof(struct pattern_packet, ar_op);
> +		pattern[i].mask_info[op_offset / 8] |= 1 << op_offset % 8;
> +		op_offset++;
> +		pattern[i].mask_info[op_offset / 8] |= 1 << op_offset % 8;
> +		tip_offset = offsetof(struct pattern_packet, ar_tip);
> +		pattern[i].mask_info[tip_offset / 8] |= 1 << tip_offset % 8;
> +		tip_offset++;
> +		pattern[i].mask_info[tip_offset / 8] |= 1 << type_offset % 8;
> +		tip_offset++;
> +		pattern[i].mask_info[tip_offset / 8] |= 1 << type_offset % 8;
> +		tip_offset++;
> +		pattern[i].mask_info[tip_offset / 8] |= 1 << type_offset % 8;
> +
> +		packet.ar_pro = 0x0 << 8 | 0x08;
> +		/* ARP type is 0x0800, notice that ar_pro and ar_op is
> +		 * big endian
> +		 */
> +		packet.ar_op = 0x1 << 8;
> +		/* 1 is arp request,2 is arp replay, 3 is rarp request,
> +		 * 4 is rarp replay
> +		 */
> +		ip_addr = fxgmac_get_netdev_ip4addr(pdata);

What happens when there is no IPv4 address on the interface? You
probably want to return -EINVAL.

> +static int fxgmac_set_wol(struct net_device *netdev,
> +			  struct ethtool_wolinfo *wol)
> +{
> +	struct fxgmac_pdata *pdata = netdev_priv(netdev);
> +	int ret;
> +
> +	if (wol->wolopts & (WAKE_MAGICSECURE | WAKE_FILTER)) {
> +		yt_err(pdata, "%s, not supported wol options, 0x%x\n", __func__,
> +		       wol->wolopts);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!(pdata->hw_feat.rwk)) {
> +		yt_err(pdata, "%s, hw wol feature is n/a\n", __func__);
> +		return wol->wolopts ? -EOPNOTSUPP : 0;
> +	}

You should be reporting that in get_wol().

> +	pdata->wol = 0;
> +	if (wol->wolopts & WAKE_UCAST)
> +		pdata->wol |= WAKE_UCAST;
> +
> +	if (wol->wolopts & WAKE_MCAST)
> +		pdata->wol |= WAKE_MCAST;

It is not 100% clear, but i think set_wol should be additive. You can
use ethtool to keep adding more WoL options. The clear them you use
wol d.

	Andrew