netdev - Re: [PATCH net-next 1/3] net: dsa: mv88e6xxx: Refactor mv88e6352 SERDES code into an op

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: <874lwj8afr.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Wed, 17 May 2017 17:01:28 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>, nikita.yoush@...entembedded.com,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next 1/3] net: dsa: mv88e6xxx: Refactor mv88e6352 SERDES code into an op

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

>  static int mv88e6xxx_phy_page_get(struct mv88e6xxx_chip *chip, int phy, u8 page)
>  {
> -	if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_PHY_PAGE))
> -		return -EOPNOTSUPP;
> -
>  	return mv88e6xxx_phy_write(chip, phy, PHY_PAGE, page);
>  }
>  
> @@ -300,8 +298,8 @@ static void mv88e6xxx_phy_page_put(struct mv88e6xxx_chip *chip, int phy)
>  	}
>  }
>  
> -static int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
> -				   u8 page, int reg, u16 *val)
> +int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
> +			    u8 page, int reg, u16 *val)
>  {
>  	int err;
>  
> @@ -318,8 +316,8 @@ static int mv88e6xxx_phy_page_read(struct mv88e6xxx_chip *chip, int phy,
>  	return err;
>  }
>  
> -static int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip *chip, int phy,
> -				    u8 page, int reg, u16 val)
> +int mv88e6xxx_phy_page_write(struct mv88e6xxx_chip *chip, int phy,
> +			     u8 page, int reg, u16 val)

Hum, this patch does a bit too much... Please at least add a first one
providing phy.[ch] for the PHY Registers related functions, then add the
serdes_power operation.

> +#define MV88E6XXX_FLAG_G1_ATU_FID	BIT_ULL(MV88E6XXX_CAP_G1_ATU_FID)

This seems to be added back by mistake.

> @@ -889,6 +868,9 @@ struct mv88e6xxx_ops {
>  			   struct mv88e6xxx_vtu_entry *entry);
>  	int (*vtu_loadpurge)(struct mv88e6xxx_chip *chip,
>  			     struct mv88e6xxx_vtu_entry *entry);
> +
> +	/* Power on/off a SERDES interface */
> +	int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
>  };

While the get_* and set_* ops are an exception, we might want to sort
that at least before the vtu_* ops. But that is a minor comment.

> +#define MV88E6352_ADDR_SERDES		0x0f
> +#define MV88E6352_SERDES_PAGE_FIBER	0x01

Why not moving this in the serdes.h header with the others?


Thank you,

      Vivien