by AngeloGioacchino Del Regno

On Tue, Dec 12, 2023 at 04:21:38PM +0000, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 03:46:26AM +0000, Daniel Golle wrote:
>
> > + mediatek,usxgmii-performance-errata:
> > + $ref: /schemas/types.yaml#/definitions/flag
> > + description:
> > + USXGMII0 on MT7988 suffers from a performance problem in 10GBase-R
> > + mode which needs a work-around in the driver. The work-around is
> > + enabled using this flag.
>
> Why do you need a property for this if you know that it is present on
> the MT7988?

Because it is only present in one of the two SerDes channels.
Channel 0 needs the work-around, Channel 1 doesn't.

See also this commit in the vendor driver for reference[1].

We previously discussed that[2] and it was decided that a property
would be the prefered way to represent this as there aren't any other
per-instance differences which would justify another compatible.

[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/a500d94cd47e279015ce22947e1ce396a7516598%5E%21/#F0

[2]: https://patchwork.kernel.org/comment/25592845/

2023-12-13 02:17:12

by Chunfeng Yun

[permalink] [raw]

Subject: Re: [RFC PATCH net-next v3 2/8] phy: add driver for MediaTek pextp 10GE SerDes PHY

On Tue, 2023-12-12 at 11:41 +0100, AngeloGioacchino Del Regno wrote:
> Il 12/12/23 04:46, Daniel Golle ha scritto:
> > Add driver for MediaTek's pextp 10 Gigabit/s Ethernet SerDes PHY
> > which
> > can be found in the MT7988 SoC.
> >
> > The PHY can operates only in PHY_MODE_ETHERNET, the submode is one
> > of
> > PHY_INTERFACE_MODE_* corresponding to the supported modes:
> >
> > * USXGMII
> > * 10GBase-R
> > * 5GBase-R
> > * 2500Base-X
> > * 1000Base-X
> > * Cisco SGMII (MAC side)
> >
> > In order to work-around a performance issue present on the first of
> > two PEXTP present in MT7988 special tuning is applied which can be
> > selected by adding the mediatek,usxgmii-performance-errata property
> > to
> > the device tree node.
> >
> > There is no documentation what-so-ever for the pextp registers and
> > this driver is based on a GPL licensed implementation found in
> > MediaTek's SDK.
> >
> > Signed-off-by: Daniel Golle <[email protected]>
> > ---
>
> ..snip..
>
> Can anyone from MediaTek **please** define those registers and
> fields?
I discussed with my software coworker who also don't have register map
doc, need our hardware designer help to provide the names of these
register and fields.

Thanks
>
> In this form, this driver is pretty obscure and nobody knows what
> it's doing;
> please remember that, by actually providing a definition for those
> registers and
> fields, the operation (reliability) of this PHY may be improved and
> this driver
> will be actually maintainable (and possibly support more than one
> variation of
> this PHY in the future with less efforts).
>
> MediaTek?
>
> Regards,
> Angelo
>
> > +
> > + /* Setup operation mode */
> > + if (is_10g)
> > + iowrite32(0x00c9071c, pextp->base + 0x9024);
> > + else
> > + iowrite32(0x00d9071c, pextp->base + 0x9024);
> > +
> > + if (is_5g)
> > + iowrite32(0xaaa5a5aa, pextp->base + 0x2020);
> > + else
> > + iowrite32(0xaa8585aa, pextp->base + 0x2020);
> > +
> > + if (is_2p5g || is_5g || is_10g) {
> > + iowrite32(0x0c020707, pextp->base + 0x2030);
> > + iowrite32(0x0e050f0f, pextp->base + 0x2034);
> > + iowrite32(0x00140032, pextp->base + 0x2040);
> > + } else {
> > + iowrite32(0x0c020207, pextp->base + 0x2030);
> > + iowrite32(0x0e05050f, pextp->base + 0x2034);
> > + iowrite32(0x00200032, pextp->base + 0x2040);
> > + }
> > +
> > + if (is_2p5g || is_10g)
> > + iowrite32(0x00c014aa, pextp->base + 0x50f0);
> > + else if (is_5g)
> > + iowrite32(0x00c018aa, pextp->base + 0x50f0);
> > + else
> > + iowrite32(0x00c014ba, pextp->base + 0x50f0);
> > +
> > + if (is_5g) {
> > + iowrite32(0x3777812b, pextp->base + 0x50e0);
> > + iowrite32(0x005c9cff, pextp->base + 0x506c);
> > + iowrite32(0x9dfafafa, pextp->base + 0x5070);
> > + iowrite32(0x273f3f3f, pextp->base + 0x5074);
> > + iowrite32(0xa8883868, pextp->base + 0x5078);
> > + iowrite32(0x14661466, pextp->base + 0x507c);
> > + } else {
> > + iowrite32(0x3777c12b, pextp->base + 0x50e0);
> > + iowrite32(0x005f9cff, pextp->base + 0x506c);
> > + iowrite32(0x9d9dfafa, pextp->base + 0x5070);
> > + iowrite32(0x27273f3f, pextp->base + 0x5074);
> > + iowrite32(0xa7883c68, pextp->base + 0x5078);
> > + iowrite32(0x11661166, pextp->base + 0x507c);
> > + }
> > +
> > + if (is_2p5g || is_10g) {
> > + iowrite32(0x0e000aaf, pextp->base + 0x5080);
> > + iowrite32(0x08080d0d, pextp->base + 0x5084);
> > + iowrite32(0x02030909, pextp->base + 0x5088);
> > + } else if (is_5g) {
> > + iowrite32(0x0e001abf, pextp->base + 0x5080);
> > + iowrite32(0x080b0d0d, pextp->base + 0x5084);
> > + iowrite32(0x02050909, pextp->base + 0x5088);
> > + } else {
> > + iowrite32(0x0e000eaf, pextp->base + 0x5080);
> > + iowrite32(0x08080e0d, pextp->base + 0x5084);
> > + iowrite32(0x02030b09, pextp->base + 0x5088);
> > + }
> > +
> > + if (is_5g) {
> > + iowrite32(0x0c000000, pextp->base + 0x50e4);
> > + iowrite32(0x04000000, pextp->base + 0x50e8);
> > + } else {
> > + iowrite32(0x0c0c0000, pextp->base + 0x50e4);
> > + iowrite32(0x04040000, pextp->base + 0x50e8);
> > + }
> > +
> > + if (is_2p5g || mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x0f0f0c06, pextp->base + 0x50eC);
> > + else
> > + iowrite32(0x0f0f0606, pextp->base + 0x50eC);
> > +
> > + if (is_5g) {
> > + iowrite32(0x50808c8c, pextp->base + 0x50a8);
> > + iowrite32(0x18000000, pextp->base + 0x6004);
> > + } else {
> > + iowrite32(0x506e8c8c, pextp->base + 0x50a8);
> > + iowrite32(0x18190000, pextp->base + 0x6004);
> > + }
> > +
> > + if (is_10g)
> > + iowrite32(0x01423342, pextp->base + 0x00f8);
> > + else if (is_5g)
> > + iowrite32(0x00a132a1, pextp->base + 0x00f8);
> > + else if (is_2p5g)
> > + iowrite32(0x009c329c, pextp->base + 0x00f8);
> > + else
> > + iowrite32(0x00fa32fa, pextp->base + 0x00f8);
> > +
> > + /* Force SGDT_OUT off and select PCS */
> > + if (mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x80201f20, pextp->base + 0x00f4);
> > + else
> > + iowrite32(0x80201f21, pextp->base + 0x00f4);
> > +
> > + /* Force GLB_CKDET_OUT */
> > + iowrite32(0x00050c00, pextp->base + 0x0030);
> > +
> > + /* Force AEQ on */
> > + iowrite32(0x02002800, pextp->base + 0x0070);
> > + ndelay(1020);
> > +
> > + /* Setup DA default value */
> > + iowrite32(0x00000020, pextp->base + 0x30b0);
> > + iowrite32(0x00008a01, pextp->base + 0x3028);
> > + iowrite32(0x0000a884, pextp->base + 0x302c);
> > + iowrite32(0x00083002, pextp->base + 0x3024);
> > + if (mtk_interface_mode_is_xgmii(interface)) {
> > + iowrite32(0x00022220, pextp->base + 0x3010);
> > + iowrite32(0x0f020a01, pextp->base + 0x5064);
> > + iowrite32(0x06100600, pextp->base + 0x50b4);
> > + if (interface == PHY_INTERFACE_MODE_USXGMII)
> > + iowrite32(0x40704000, pextp->base + 0x3048);
> > + else
> > + iowrite32(0x47684100, pextp->base + 0x3048);
> > + } else {
> > + iowrite32(0x00011110, pextp->base + 0x3010);
> > + iowrite32(0x40704000, pextp->base + 0x3048);
> > + }
> > +
> > + if (!mtk_interface_mode_is_xgmii(interface) && !is_2p5g)
> > + iowrite32(0x0000c000, pextp->base + 0x3064);
> > +
> > + if (interface != PHY_INTERFACE_MODE_10GBASER) {
> > + iowrite32(0xa8000000, pextp->base + 0x3050);
> > + iowrite32(0x000000aa, pextp->base + 0x3054);
> > + } else {
> > + iowrite32(0x00000000, pextp->base + 0x3050);
> > + iowrite32(0x00000000, pextp->base + 0x3054);
> > + }
> > +
> > + if (mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x00000f00, pextp->base + 0x306c);
> > + else if (is_2p5g)
> > + iowrite32(0x22000f00, pextp->base + 0x306c);
> > + else
> > + iowrite32(0x20200f00, pextp->base + 0x306c);
> > +
> > + if (interface == PHY_INTERFACE_MODE_10GBASER && pextp->da_war)
> > + iowrite32(0x0007b400, pextp->base + 0xa008);
> > +
> > + if (mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x00040000, pextp->base + 0xa060);
> > + else
> > + iowrite32(0x00050000, pextp->base + 0xa060);
> > +
> > + if (is_10g)
> > + iowrite32(0x00000001, pextp->base + 0x90d0);
> > + else if (is_5g)
> > + iowrite32(0x00000003, pextp->base + 0x90d0);
> > + else if (is_2p5g)
> > + iowrite32(0x00000005, pextp->base + 0x90d0);
> > + else
> > + iowrite32(0x00000007, pextp->base + 0x90d0);
> > +
> > + /* Release reset */
> > + iowrite32(0x0200e800, pextp->base + 0x0070);
> > + usleep_range(150, 500);
> > +
> > + /* Switch to P0 */
> > + iowrite32(0x0200c111, pextp->base + 0x0070);
> > + ndelay(1020);
> > + iowrite32(0x0200c101, pextp->base + 0x0070);
> > + usleep_range(15, 50);
> > +
> > + if (mtk_interface_mode_is_xgmii(interface)) {
> > + /* Switch to Gen3 */
> > + iowrite32(0x0202c111, pextp->base + 0x0070);
> > + } else {
> > + /* Switch to Gen2 */
> > + iowrite32(0x0201c111, pextp->base + 0x0070);
> > + }
> > + ndelay(1020);
> > + if (mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x0202c101, pextp->base + 0x0070);
> > + else
> > + iowrite32(0x0201c101, pextp->base + 0x0070);
> > + usleep_range(100, 500);
> > + iowrite32(0x00000030, pextp->base + 0x30b0);
> > + if (mtk_interface_mode_is_xgmii(interface))
> > + iowrite32(0x80201f00, pextp->base + 0x00f4);
> > + else
> > + iowrite32(0x80201f01, pextp->base + 0x00f4);
> > +
> > + iowrite32(0x30000000, pextp->base + 0x3040);
> > + usleep_range(400, 1000);
> > +}
> > +
> > +static int mtk_pextp_set_mode(struct phy *phy, enum phy_mode mode,
> > int submode)
> > +{
> > + struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
> > +
> > + if (mode != PHY_MODE_ETHERNET)
> > + return -EINVAL;
> > +
> > + switch (submode) {
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + case PHY_INTERFACE_MODE_2500BASEX:
> > + case PHY_INTERFACE_MODE_SGMII:
> > + case PHY_INTERFACE_MODE_5GBASER:
> > + case PHY_INTERFACE_MODE_10GBASER:
> > + case PHY_INTERFACE_MODE_USXGMII:
> > + mtk_pextp_setup(pextp, submode);
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mtk_pextp_reset(struct phy *phy)
> > +{
> > + struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
> > +
> > + reset_control_assert(pextp->reset);
> > + usleep_range(100, 500);
> > + reset_control_deassert(pextp->reset);
> > + usleep_range(1, 10);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_pextp_power_on(struct phy *phy)
> > +{
> > + struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
> > +
> > + return clk_bulk_prepare_enable(MTK_PEXTP_NUM_CLOCKS, pextp-
> > >clocks);
> > +}
> > +
> > +static int mtk_pextp_power_off(struct phy *phy)
> > +{
> > + struct mtk_pextp_phy *pextp = phy_get_drvdata(phy);
> > +
> > + clk_bulk_disable_unprepare(MTK_PEXTP_NUM_CLOCKS, pextp-
> > >clocks);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops mtk_pextp_ops = {
> > + .power_on = mtk_pextp_power_on,
> > + .power_off = mtk_pextp_power_off,
> > + .set_mode = mtk_pextp_set_mode,
> > + .reset = mtk_pextp_reset,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int mtk_pextp_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct phy_provider *phy_provider;
> > + struct mtk_pextp_phy *pextp;
> > + struct phy *phy;
> > +
> > + if (!np)
> > + return -ENODEV;
> > +
> > + pextp = devm_kzalloc(&pdev->dev, sizeof(*pextp), GFP_KERNEL);
> > + if (!pextp)
> > + return -ENOMEM;
> > +
> > + pextp->base = devm_of_iomap(&pdev->dev, np, 0, NULL);
> > + if (!pextp->base)
> > + return -EIO;
> > +
> > + pextp->dev = &pdev->dev;
> > +
> > + pextp->clocks[0].id = "topxtal";
> > + pextp->clocks[0].clk = devm_clk_get(&pdev->dev, pextp-
> > >clocks[0].id);
> > + if (IS_ERR(pextp->clocks[0].clk))
> > + return PTR_ERR(pextp->clocks[0].clk);
> > +
> > + pextp->clocks[1].id = "xfipll";
> > + pextp->clocks[1].clk = devm_clk_get(&pdev->dev, pextp-
> > >clocks[1].id);
> > + if (IS_ERR(pextp->clocks[1].clk))
> > + return PTR_ERR(pextp->clocks[1].clk);
> > +
> > + pextp->reset = devm_reset_control_get_exclusive(&pdev->dev,
> > NULL);
> > + if (IS_ERR(pextp->reset))
> > + return PTR_ERR(pextp->reset);
> > +
> > + pextp->da_war = of_property_read_bool(np, "mediatek,usxgmii-
> > performance-errata");
> > +
> > + phy = devm_phy_create(&pdev->dev, NULL, &mtk_pextp_ops);
> > + if (IS_ERR(phy))
> > + return PTR_ERR(phy);
> > +
> > + phy_set_drvdata(phy, pextp);
> > +
> > + phy_provider = devm_of_phy_provider_register(&pdev->dev,
> > of_phy_simple_xlate);
> > +
> > + return PTR_ERR_OR_ZERO(phy_provider);
> > +}
> > +
> > +static const struct of_device_id mtk_pextp_match[] = {
> > + { .compatible = "mediatek,mt7988-xfi-pextp", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_pextp_match);
> > +
> > +static struct platform_driver mtk_pextp_driver = {
> > + .probe = mtk_pextp_probe,
> > + .driver = {
> > + .name = "mtk-pextp",
> > + .of_match_table = mtk_pextp_match,
> > + },
> > +};
> > +module_platform_driver(mtk_pextp_driver);
> > +
> > +MODULE_DESCRIPTION("MediaTek pextp SerDes PHY driver");
> > +MODULE_AUTHOR("Daniel Golle <[email protected]>");
> > +MODULE_LICENSE("GPL");
>
>

2023-12-13 06:58:51

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [RFC PATCH net-next v3 7/8] dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding

On 12/12/2023 04:51, Daniel Golle wrote:
> Complete support for MT7988 which comes with 3 MACs, SRAM for DMA
> descriptors and uses a dedicated PCS for the SerDes units.
>
> Fixes: c94a9aabec36 ("dt-bindings: net: mediatek,net: add mt7988-eth binding")
> Signed-off-by: Daniel Golle <[email protected]>
> ---

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2023-12-13 09:47:16

Hi Russell,
Hi netdev crew,

I haven't received any reply for this email and am still waiting for
clarification regarding how inter-driver dependencies should be modelled
in that case of mtk_eth_soc. My search for good examples for that in the
kernel code was quite frustrating and all I've found are supposedly bugs
of that exact cathegory.

Please see my questions mentioned in the previous email I've sent and
also a summary of suggested solutions inline below:

On Wed, Feb 07, 2024 at 01:29:18AM +0000, Daniel Golle wrote:
> Hi Russell,
>
> sorry for the extended time it took me to get back to this patch and
> the comments you made for it. Understanding the complete scope of the
> problem took a while (plus there were holidays and other fun things),
> and also brought up further questions which I hope you can help me
> find good answers for, see below:
>
> On Wed, Dec 13, 2023 at 04:04:12PM +0000, Russell King (Oracle) wrote:
> > On Tue, Dec 12, 2023 at 03:47:18AM +0000, Daniel Golle wrote:
> > > Introduce a proper platform MFD driver for the LynxI (H)SGMII PCS which
> > > is going to initially be used for the MT7988 SoC.
> > >
> > > Signed-off-by: Daniel Golle <[email protected]>
> >
> > I made some specific suggestions about what I wanted to see for
> > "getting" PCS in the previous review, and I'm disappointed that this
> > patch set is still inventing its own solution.
> >
> > > +struct phylink_pcs *mtk_pcs_lynxi_get(struct device *dev, struct device_node *np)
> > > +{
> > > + struct platform_device *pdev;
> > > + struct mtk_pcs_lynxi *mpcs;
> > > +
> > > + if (!np)
> > > + return NULL;
> > > +
> > > + if (!of_device_is_available(np))
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + if (!of_match_node(mtk_pcs_lynxi_of_match, np))
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + pdev = of_find_device_by_node(np);
> > > + if (!pdev || !platform_get_drvdata(pdev)) {
> >
> > This is racy - as I thought I described before, userspace can unbind
> > the device in one thread, while another thread is calling this
> > function. With just the right timing, this check succeeds, but...
> >
> > > + if (pdev)
> > > + put_device(&pdev->dev);
> > > + return ERR_PTR(-EPROBE_DEFER);
> > > + }
> > > +
> > > + mpcs = platform_get_drvdata(pdev);
> >
> > mpcs ends up being read as NULL here. Even if you did manage to get a
> > valid pointer, "mpcs" being devm-alloced could be freed from under
> > you at this point...
> >
> > > + device_link_add(dev, mpcs->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> >
> > resulting in this accessing memory which has been freed.
> >
> > The solution would be either to suppress the bind/unbind attributes
> > (provided the underlying struct device can't go away, which probably
> > also means ensuring the same of the MDIO bus. Aternatively, adding
> > a lock around the remove path and around the checking of
> > platform_get_drvdata() down to adding the device link would probably
> > solve it.
> >
> > However, I come back to my general point - this kind of stuff is
> > hairy. Do we want N different implementations of it in various drivers
> > with subtle bugs, or do we want _one_ implemenatation.
> >
> > If we go with the one implemenation approach, then we need to think
> > about whether we should be using device links or not. The problem
> > could be for network interfaces where one struct device is
> > associated with multiple network interfaces. Using device links has
> > the unfortunate side effect that if the PCS for one of those network
> > interfaces is removed, _all_ network interfaces disappear.
>
> I agree, esp. on this MT7988 removal of a PCS which may then not
> even be in use also impairs connectivity on the built-in gigE DSA
> switch. It would be nice to try to avoid this.
>
> >
> > My original suggestion was to hook into phylink to cause that to
> > take the link down when an in-use PCS gets removed.
>
> I took a deep dive into how this could be done correctly and how
> similar things are done for other drivers. Apart from the PCS there
> often also is a muxing-PHY involved, eg. MediaTek's XFI T-PHY in this
> case (previously often called "pextp" for no apparent reason) or
> Marvell's comphy (mvebu-comphy).
>
> So let's try something simple on an already well-supported platform,
> I thought and grabbed Marvell Armada CN9131-DB running vanilla Linux,
> and it didn't even take some something racy, but plain:
>
> ip link set eth6 up
> cd /sys/bus/platform/drivers/mvebu-comphy
> echo f2120000.phy > unbind
> echo f4120000.phy > unbind
> echo f6120000.phy > unbind
> ip link set eth6 down
>
>
> That was enough. The result is a kernel crash, and the same should
> apply on popular platforms like the SolidRun MACCHIATOBin and other
> similar boards.
>
> So this gets me to think that there is a wider problem around
> non-phylink-managed resources which may disappear while in use by
> network drivers, and I guess that the same applies in many other
> places. I don't have a SATA drive connected to that Marvell board, but
> I can imagine what happens when suddenly removing the comphy instance
> in charge of the SATA link and then a subsequent SATA hotplug event
> happens or stuff like that...
>
> Anyway, back to network subsystem and phylink:
>
> Do you agree that we need a way to register (and unregister) PCS
> providers with phylink, so we don't need *_get_by_of_node implementations
> in each driver? If so, can you sketch out what the basic requirements
> for an acceptable solution would be?
>
> Also, do you agree that lack of handling of disappearing resources,
> such as PHYs (*not* network PHYs, but PHYs as in drivers/phy/*) or
> syscons, is already a problem right now which should be addressed?
>
> If you imagine phylink to take care of the life-cycle of all link-
> resources, what is vision about those things other than classic MDIO-
> connected PHYs?
>
> And well, of course, the easy fix for the example above would be:
> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> index b0dd133665986..9517c96319595 100644
> --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
> @@ -1099,6 +1099,7 @@ static const struct of_device_id mvebu_comphy_of_match_table[] = {
> MODULE_DEVICE_TABLE(of, mvebu_comphy_of_match_table);
>
> static struct platform_driver mvebu_comphy_driver = {
> + .suppress_bind_attrs = true,
> .probe = mvebu_comphy_probe,
> .driver = {
> .name = "mvebu-comphy",
>
> But that should then apply to every single driver in drivers/phy/...
>

My remaining questions are:
- Is it ok to just use .suppress_bind_attrs = true for PCS and PHY
drivers to avoid having to care out hot-removal?
Those components are anyway built-into the SoC so removing them
is physically not possible.

- In case of driver removal (rmmod -f) imho using a device-link would
be sufficient to prevent the worst. However, we would have to live
with the all-or-nothing nature of that approach, ie. once e.g. the
USXGMII driver is being removed, *all* Ethernet interfaces would
disappear with it (even those not actually using USXGMII).

If the solutions mentioned above do not sound agreeable, please suggest
how a good solution would look like, ideally also share an example of
any driver in the kernel where this is done in the way you would like
to have things done.

>
> >
> > > +
> > > + return &mpcs->pcs;
> > > +}
> > > +EXPORT_SYMBOL(mtk_pcs_lynxi_get);
> > > +
> > > +void mtk_pcs_lynxi_put(struct phylink_pcs *pcs)
> > > +{
> > > + struct mtk_pcs_lynxi *cur, *mpcs = NULL;
> > > +
> > > + if (!pcs)
> > > + return;
> > > +
> > > + mutex_lock(&instance_mutex);
> > > + list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
> > > + if (pcs == &cur->pcs) {
> > > + mpcs = cur;
> > > + break;
> > > + }
> > > + mutex_unlock(&instance_mutex);
> >
> > I don't see what this loop gains us, other than checking that the "pcs"
> > is still on the list and hasn't already been removed. If that is all
> > that this is about, then I would suggest:
> >
> > bool found = false;
> >
> > if (!pcs)
> > return;
> >
> > mpcs = pcs_to_mtk_pcs_lynxi(pcs);
> > mutex_lock(&instance_mutex);
> > list_for_each_entry(cur, &mtk_pcs_lynxi_instances, node)
> > if (cur == mpcs) {
> > found = true;
> > break;
> > }
> > mutex_unlock(&instance_mutex);
> >
> > if (WARN_ON(!found))
> > return;
> >
> > which makes it more obvious why this exists.
>
> The idea was not only to make sure it hasn't been removed, but also
> to be sure that what ever the private data pointer points to has
> actually been created by that very driver.
>
> But yes, doing it in the way you suggest will work in the same way,
> just when having my idea in mind it looks more fishy to use
> pcs_to_mtk_pcs_lynxi() before we are 100% sure that what we dealing
> with has previously been created by this driver.
>
>
> Cheers
>
>
> Daniel
>