On Thu, 7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
> ETHTOOL_MSG_PHY_LIST_GET,
> + ETHTOOL_MSG_PHY_GET,
The distinction between LIST_GET and GET is a bit odd for netlink.
GET has a do and a dump. The dump is effectively LIST_GET.
The dump can accept filtering arguments, like ifindex, if you want
to narrow down the results, that's perfectly fine (you may need to
give up some of the built-in ethtool scaffolding, but it shouldn't
be all that bad).
Hello Jakub,
On Fri, 8 Sep 2023 08:46:06 -0700
Jakub Kicinski <[email protected]> wrote:
> On Thu, 7 Sep 2023 11:24:04 +0200 Maxime Chevallier wrote:
> > ETHTOOL_MSG_PHY_LIST_GET,
> > + ETHTOOL_MSG_PHY_GET,
>
> The distinction between LIST_GET and GET is a bit odd for netlink.
> GET has a do and a dump. The dump is effectively LIST_GET.
>
> The dump can accept filtering arguments, like ifindex, if you want
> to narrow down the results, that's perfectly fine (you may need to
> give up some of the built-in ethtool scaffolding, but it shouldn't
> be all that bad).
I'm currently implementing this, and I was wondering if it could be
worth it to include a pointer to struct phy_device directly in
ethnl_req_info.
This would share the logic for all netlink commands that target a
phy_device :
- plca
- pse-pd
- cabletest
- other future commands
Do you see this as acceptable ? we would grab the phy_device that
matches the passed phy_index in the request, and if none is specified,
we default to dev->phydev.
Thanks,
Maxime
On Thu, 14 Sep 2023 11:36:13 +0200 Maxime Chevallier wrote:
> I'm currently implementing this, and I was wondering if it could be
> worth it to include a pointer to struct phy_device directly in
> ethnl_req_info.
>
> This would share the logic for all netlink commands that target a
> phy_device :
>
> - plca
> - pse-pd
> - cabletest
> - other future commands
>
> Do you see this as acceptable ? we would grab the phy_device that
> matches the passed phy_index in the request, and if none is specified,
> we default to dev->phydev.
You may need to be careful with that. It could work in practice but
the req_info is parsed without holding any locks, IIRC. And there
may also be some interplay between PHY state and ethnl_ops_begin().
From netlink perspective putting the PHY info in the header nest makes
perfect sense to me. Just not sure if you can actually get the object
when the parsing happens or you'd need to just store the index and
resolve it later? PHYLIB maintainers may be best at advising on the
lifetime expectations for phys..
Sorry for the delayed response, #vacation.
On Tue, Oct 03, 2023 at 06:55:35AM -0700, Jakub Kicinski wrote:
> On Thu, 14 Sep 2023 11:36:13 +0200 Maxime Chevallier wrote:
> > I'm currently implementing this, and I was wondering if it could be
> > worth it to include a pointer to struct phy_device directly in
> > ethnl_req_info.
> >
> > This would share the logic for all netlink commands that target a
> > phy_device :
> >
> > - plca
> > - pse-pd
> > - cabletest
> > - other future commands
> >
> > Do you see this as acceptable ? we would grab the phy_device that
> > matches the passed phy_index in the request, and if none is specified,
> > we default to dev->phydev.
>
> You may need to be careful with that. It could work in practice but
> the req_info is parsed without holding any locks, IIRC. And there
> may also be some interplay between PHY state and ethnl_ops_begin().
We also need to ensure it is totally optional. There are MAC drivers
which reinvent the wheel in firmware. They can have multiple PHYs, or
PHY and SFP in parallel etc. All the typologies which you are
considering for phylink. Ideally we want the uAPI to work for
everybody, not just phylink. Its not our problem how said firmware
actually works, and what additional wheels they need to re-implement,
but we should try not to block them.
Andrew