Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
CC:     Colin Foster <colin.foster@in-advantage.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities
 for all ports
Thread-Topic: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities
 for all ports
Thread-Index: AQHYxhmBz++3+4sxbkG2gbfVifdN+a3bfLEAgAAYhIA=
Date:   Mon, 12 Sep 2022 10:16:21 +0000
Message-ID: <20220912101621.ttnsxmjmaor2cd7d@skbuf>
References: <20220911200244.549029-1-colin.foster@in-advantage.com>
 <20220911200244.549029-7-colin.foster@in-advantage.com>
 <Yx7yZESuK6Jh0Q8X@shell.armlinux.org.uk>
In-Reply-To: <Yx7yZESuK6Jh0Q8X@shell.armlinux.org.uk>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?DAoJ0reHxEkWLOKv+Jp3Wveustr9fskS3UlhsZVLWcdTr4ft7GozjeL2sNRR?=
 =?us-ascii?Q?JYEpF2utqKTSYPGhRC4NHlkcXRGB6k2oK4QvSw9gZPB/1KBY6QanIRNIM0ev?=
 =?us-ascii?Q?QCXaGoie5qMrgy0e0G9VRZSee+f5At4H/eiobzxZiQF8ivOu8xarWkcx7fMo?=
 =?us-ascii?Q?BzABrkCixW4aoRmNnoCyOF56TDKmn+5rlMiJ3zZ9KrwQH70C1o4By7HubyQT?=
 =?us-ascii?Q?pDCVrh5l/sLK8A4Y2ilXtJC7ofBkaR68QK1CPHb+9eN5SQtKn2PRY/WQ0RQv?=
 =?us-ascii?Q?2xLJ7rjN2YaSNyVCBSWW0cNne7G7tFFUQgTgCt4LpBmtU8aQf1cZah//a5JZ?=
 =?us-ascii?Q?5rgjdv/emRMQ6LPWKr+skV+64OPN5E1oCJLPcpQbbWLDZ1SQLhWvE8BWlneW?=
 =?us-ascii?Q?tdGOCuSa9vq5Jyo+KIZszpVHQXgG7cRlY8oUTCv4rt8nAb2G4MCA3sn9G3qr?=
 =?us-ascii?Q?uBMWSkd5EAen4bgw68tKFWhOlyzatS0FBh7wlFBa0avE7PBJPSBfR5+wLV/h?=
 =?us-ascii?Q?1Bm1UZfl0G9CKzwrUxlQ+Lzc6xwNWePkMVvaR7WBGCJdYmrdZa+I+UhWp3Z7?=
 =?us-ascii?Q?mjuEzabJGQ67BPBDerO5PnCgeun/7hD0zGaT3Z2e3p5X3XvCWiBC6uuWz6K1?=
 =?us-ascii?Q?hh4BgTqjzbxBSd4XOLxWYGk4S5aGXstM3/2cz/pmWvwpgl9HFMQHPbw1SYlH?=
 =?us-ascii?Q?whiU8thBYboTYomsgAyfB8lbvxMir1Gw2OAI1UcYn5pMpDFK+T71PcApEaF8?=
 =?us-ascii?Q?aY02X4LZzG4wP7TezjDToDkklOgUwzxXMAmmKHcRYdAptTma5iWw9Y6BPxB0?=
 =?us-ascii?Q?uyD2mIg1qxE7SvovT0BJGUImQQ3RRjk+tpPQtpbqR9lMaXgoYkOZ6UNqA9ca?=
 =?us-ascii?Q?XCEkEkiXSVP+pAp0bGO+JfaJhh3ilQkS97d8h2GO2fGWqaIUgkanWMqfyzMw?=
 =?us-ascii?Q?hOM0DAS98ZPhrZOMPMkY/rC+7205TusOqfzKYsQEtsdiLbKk0pzABWlPwbjY?=
 =?us-ascii?Q?e7O9kJtVcM69fo8bNU2IB8ZycrSOH03bZVe8DmFVSRW+qG5deVM0lxvHxz7m?=
 =?us-ascii?Q?6k5M/dYlbe6ecJcuvWmSWGKzbii4yrxGA3bzwNb6mYCYnUpfbGE1yvjnZDrC?=
 =?us-ascii?Q?ZJSTWlOdQ/v44dDiekBBUKeJf72afW6JvoRapFnxCLmLBzkkOttg648C4Fz7?=
 =?us-ascii?Q?jNlE2cjhxvz2FF4LPfacIEvqfEcFTD+PmBKPw4fVsI51q+HMY6qJl1I65tFY?=
 =?us-ascii?Q?DAcU9lg1BL/wKuSrsoQUWnS/ezORFjmUGuJwvbY5G1OJffRwLjfuvwDm2ZSk?=
 =?us-ascii?Q?Qx+XCIrYvyRDjb7ZGHpGCnM1FmHooLSH17nuOT7DGMuuZ7FZkBKokdRpS/v4?=
 =?us-ascii?Q?E5rvpUaCCvsiTzmc1oqRb5df59G6qoRP8omgcS6RIyg6H904FdIKJFunDvGN?=
 =?us-ascii?Q?0iKSiOfTos6UIlYbtwjUU/ZwMcvdLgre45d1xZDmxiniDDg1wMcac7bOzd8C?=
 =?us-ascii?Q?zeB5aZU5KBQU6mwAT/Rbh8Nz8qf9KkbNADwl+xw6UjwU5HtaWrydMSh5bMVF?=
 =?us-ascii?Q?DKKMl5kytF/5/GKyNHARlPQUp2pHbcQ9x5eD7sNZF+Eyn/ePvHvGwDORysGy?=
 =?us-ascii?Q?9A=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9F76332D997AE4409C9271ACC188C369@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1984fd32-a476-46cd-0091-08da94a7d75e
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Sep 2022 10:16:21.9432
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Eg/rFZDpJgsY3IBi5/41lX8d9OjHWMmLdHSGbeaz9URDptAFcSJpDGSA9s0f8YzNIwIWU+KwSe1WOM5/XWFovA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8853
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote:
> On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote:
> > phylink_generic_validate() requires that mac_capabilities is correctly
> > populated. While no existing drivers have used phylink_generic_validate=
(),
> > the ocelot_ext.c driver will. Populate this element so the use of exist=
ing
> > functions is possible.
>=20
> Ocelot always fills in the .phylink_validate method in struct
> dsa_switch_ops, mac_capabilities won't be used as
> phylink_generic_validate() will not be called by
> dsa_port_phylink_validate().

Correct, but felix_phylink_validate() _can_ still directly call
phylink_validate(), right? Colin does not have the full support for
ocelot_ext in this patch set, but this is what he intends to do.

> Also "no existing drivers have used phylink_generic_validate()" I
> wonder which drivers you are referring to there. If you are referring
> to DSA drivers, then it is extensively used. The following is from
> Linus' tree as of today:

By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c,
two drivers in their own right, which use the common felix.c to talk to
(a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/.
It is true that these existing drivers do not use phylink_generic_validate(=
).
Furthermore, Colin's new ocelot_ext.c is on the same level as
felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way,
and will want to use phylink_generic_validate().

> Secondly, I don't see a purpose for this patch in the following
> patches, as Ocelot continues to always fill in .phylink_validate,
> and as I mentioned above, as long as that member is filled in,
> mac_capabilities won't be used unless you explicitly call
> phylink_generic_validate() in your .phylink_validate() callback.

Yes, explicit calling is what Colin explained that he wants to do.

> Therefore, I think you can drop this patch from your series and
> you won't see any functional change.

This is true. I am also a bit surprised at Colin's choices to
(a) not ask the netdev maintainers to pull into net-next the immutable
    branch that Lee provided here:
    https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
    and instead send some patches for review which are difficult to
    apply directly to any tree
(b) split the work he submitted such that he populates mac_capabilities
    but does not make any use of it (not call phylink_generic_validate
    from anywhere). We try as much as possible to not leave dead code
    behind in the mainline tree, even if future work is intended to
    bring it to life. I do understand that this is an RFC so the patches
    weren't intended to be applied as is, but it is still confusing to
    review a change which, as you've correctly pointed out, has no
    effect to the git tree as it stands.=