netdev - Re: [PATCH CFT net-next] net: macb: use .mac_select

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]
Date:   Thu, 16 Dec 2021 14:15:02 +0100
From:   Nicolas Ferre <nicolas.ferre@...rochip.com>
To:     "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     Claudiu Beznea <claudiu.beznea@...rochip.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH CFT net-next] net: macb: use .mac_select_pcs() interface

On 16/12/2021 at 13:41, Russell King (Oracle) wrote:
> Convert the PCS selection to use mac_select_pcs, which allows the PCS
> to perform any validation it needs.
> 
> We must use separate phylink_pcs instances for the USX and SGMII PCS,
> rather than just changing the "ops" pointer before re-setting it to
> phylink as this interface queries the PCS, rather than requesting it
> to be changed.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

Looks good to me:
Acked-by: Nicolas Ferre <nicolas.ferre@...rochip.com>

Thanks Russell. Best regards,
   Nicolas

> ---
>   drivers/net/ethernet/cadence/macb.h      |  3 ++-
>   drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++-------------
>   2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 5620b97b3482..9ddbee7de72b 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1271,7 +1271,8 @@ struct macb {
>          struct mii_bus          *mii_bus;
>          struct phylink          *phylink;
>          struct phylink_config   phylink_config;
> -       struct phylink_pcs      phylink_pcs;
> +       struct phylink_pcs      phylink_usx_pcs;
> +       struct phylink_pcs      phylink_sgmii_pcs;
> 
>          u32                     caps;
>          unsigned int            dma_burst_length;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d4da9adf6777..a363da928e8b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -510,7 +510,7 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
>                                   phy_interface_t interface, int speed,
>                                   int duplex)
>   {
> -       struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
> +       struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>          u32 config;
> 
>          config = gem_readl(bp, USX_CONTROL);
> @@ -524,7 +524,7 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
>   static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
>                                     struct phylink_link_state *state)
>   {
> -       struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
> +       struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>          u32 val;
> 
>          state->speed = SPEED_10000;
> @@ -544,7 +544,7 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
>                                 const unsigned long *advertising,
>                                 bool permit_pause_to_mac)
>   {
> -       struct macb *bp = container_of(pcs, struct macb, phylink_pcs);
> +       struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
> 
>          gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
>                     GEM_BIT(SIGNAL_OK));
> @@ -727,28 +727,23 @@ static void macb_mac_link_up(struct phylink_config *config,
>          netif_tx_wake_all_queues(ndev);
>   }
> 
> -static int macb_mac_prepare(struct phylink_config *config, unsigned int mode,
> -                           phy_interface_t interface)
> +static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
> +                                              phy_interface_t interface)
>   {
>          struct net_device *ndev = to_net_dev(config->dev);
>          struct macb *bp = netdev_priv(ndev);
> 
>          if (interface == PHY_INTERFACE_MODE_10GBASER)
> -               bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops;
> +               return &bp->phylink_usx_pcs;
>          else if (interface == PHY_INTERFACE_MODE_SGMII)
> -               bp->phylink_pcs.ops = &macb_phylink_pcs_ops;
> +               return &bp->phylink_sgmii_pcs;
>          else
> -               bp->phylink_pcs.ops = NULL;
> -
> -       if (bp->phylink_pcs.ops)
> -               phylink_set_pcs(bp->phylink, &bp->phylink_pcs);
> -
> -       return 0;
> +               return NULL;
>   }
> 
>   static const struct phylink_mac_ops macb_phylink_ops = {
>          .validate = phylink_generic_validate,
> -       .mac_prepare = macb_mac_prepare,
> +       .mac_select_pcs = macb_mac_select_pcs,
>          .mac_config = macb_mac_config,
>          .mac_link_down = macb_mac_link_down,
>          .mac_link_up = macb_mac_link_up,
> @@ -806,6 +801,9 @@ static int macb_mii_probe(struct net_device *dev)
>   {
>          struct macb *bp = netdev_priv(dev);
> 
> +       bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
> +       bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
> +
>          bp->phylink_config.dev = &dev->dev;
>          bp->phylink_config.type = PHYLINK_NETDEV;
> 
> --
> 2.30.2
> 


-- 
Nicolas Ferre