[PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Maxime Chevallier]

In situations where phylib is a module, the topology can be NULL as it's
not initialized at netdev creation.

Allow passing a NULL topology pointer to phy_link_topo helpers.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
---

Hi,

This patch fixes a commit that is in net-next, hence the net-next tag and the
lack of "Fixes" tag.

Nathan, Heiner, can you confirm this solves what you're seeing ?

I think we can improve on this solution by moving the topology init at
the first PHY insertion and clearing it at netdev destruction.

Maxime

 drivers/net/phy/phy_link_topology.c | 10 +++++++++-
 include/linux/phy_link_topology.h   |  7 ++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 985941c5c558..0f3973f07fac 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
 	struct phy_device_node *pdn;
 	int ret;
 
+	if (!topo)
+		return 0;
+
 	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
 	if (!pdn)
 		return -ENOMEM;
@@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
 void phy_link_topo_del_phy(struct phy_link_topology *topo,
 			   struct phy_device *phy)
 {
-	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
+	struct phy_device_node *pdn;
+
+	if (!topo)
+		return;
+
+	pdn = xa_erase(&topo->phys, phy->phyindex);
 
 	/* We delete the PHY from the topology, however we don't re-set the
 	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
index 6b79feb607e7..21ca78127d0f 100644
--- a/include/linux/phy_link_topology.h
+++ b/include/linux/phy_link_topology.h
@@ -40,7 +40,12 @@ struct phy_link_topology {
 static inline struct phy_device *
 phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
 {
-	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
+	struct phy_device_node *pdn;
+
+	if (!topo)
+		return NULL;
+
+	pdn = xa_load(&topo->phys, phyindex);
 
 	if (pdn)
 		return pdn->phy;
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Heiner Kallweit]

On 12.04.2024 12:46, Maxime Chevallier wrote:
> In situations where phylib is a module, the topology can be NULL as it's
> not initialized at netdev creation.
> 
> Allow passing a NULL topology pointer to phy_link_topo helpers.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> ---
> 
> Hi,
> 
> This patch fixes a commit that is in net-next, hence the net-next tag and the
> lack of "Fixes" tag.
> 
> Nathan, Heiner, can you confirm this solves what you're seeing ?
> 
> I think we can improve on this solution by moving the topology init at
> the first PHY insertion and clearing it at netdev destruction.
> 
Not on every system the topology extension is needed, it may just create
overhead on such systems. Would it make sense to hide it behind a config symbol?

> Maxime
> 
>  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
>  include/linux/phy_link_topology.h   |  7 ++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 985941c5c558..0f3973f07fac 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
>  	struct phy_device_node *pdn;
>  	int ret;
>  
> +	if (!topo)
> +		return 0;
> +

This one I added as temporary workaround already, it fixes the issue for me.

>  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
>  	if (!pdn)
>  		return -ENOMEM;
> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
>  void phy_link_topo_del_phy(struct phy_link_topology *topo,
>  			   struct phy_device *phy)
>  {
> -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return;
> +
> +	pdn = xa_erase(&topo->phys, phy->phyindex);
>  
>  	/* We delete the PHY from the topology, however we don't re-set the
>  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 6b79feb607e7..21ca78127d0f 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h
> @@ -40,7 +40,12 @@ struct phy_link_topology {
>  static inline struct phy_device *
>  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
>  {
> -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return NULL;
> +
> +	pdn = xa_load(&topo->phys, phyindex);
>  
>  	if (pdn)
>  		return pdn->phy;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Antoine Tenart]

Hi Maxime,

Quoting Maxime Chevallier (2024-04-12 12:46:14)
> 
> This patch fixes a commit that is in net-next, hence the net-next tag and the
> lack of "Fixes" tag.

You can use Fixes: on net-next, that still helps to identify which
commit is being fixed (eg. for reviews, while looking at the history,
etc).

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 985941c5c558..0f3973f07fac 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
>         struct phy_device_node *pdn;
>         int ret;
>  
> +       if (!topo)
> +               return 0;
> +

With that phy_sfp_connect_phy does not need to check the topo validity
before calling phy_link_topo_add_phy. The other way around is fine too.

> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
>  void phy_link_topo_del_phy(struct phy_link_topology *topo,
>                            struct phy_device *phy)
>  {
> -       struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> +       struct phy_device_node *pdn;
> +
> +       if (!topo)
> +               return;
> +
> +       pdn = xa_erase(&topo->phys, phy->phyindex);

Same here with phy_sfp_disconnect_phy.

Thanks!
Antoine

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Maxime Chevallier]

Hi Antoine,

On Fri, 12 Apr 2024 15:03:10 +0200
Antoine Tenart <atenart@kernel.org> wrote:

> Hi Maxime,
> 
> Quoting Maxime Chevallier (2024-04-12 12:46:14)
> > 
> > This patch fixes a commit that is in net-next, hence the net-next tag and the
> > lack of "Fixes" tag.  
> 
> You can use Fixes: on net-next, that still helps to identify which
> commit is being fixed (eg. for reviews, while looking at the history,
> etc).

Won't the tag become invalid when the commit gets merged into an -rc
release then ?

> 
> > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> > index 985941c5c558..0f3973f07fac 100644
> > --- a/drivers/net/phy/phy_link_topology.c
> > +++ b/drivers/net/phy/phy_link_topology.c
> > @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
> >         struct phy_device_node *pdn;
> >         int ret;
> >  
> > +       if (!topo)
> > +               return 0;
> > +  
> 
> With that phy_sfp_connect_phy does not need to check the topo validity
> before calling phy_link_topo_add_phy. The other way around is fine too.
> 
> > @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
> >  void phy_link_topo_del_phy(struct phy_link_topology *topo,
> >                            struct phy_device *phy)
> >  {
> > -       struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> > +       struct phy_device_node *pdn;
> > +
> > +       if (!topo)
> > +               return;
> > +
> > +       pdn = xa_erase(&topo->phys, phy->phyindex);  
> 
> Same here with phy_sfp_disconnect_phy.

Ah right, well spotted, thanks !

Maxime

> 
> Thanks!
> Antoine


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Antoine Tenart]

Quoting Maxime Chevallier (2024-04-12 15:16:48)
> On Fri, 12 Apr 2024 15:03:10 +0200
> Antoine Tenart <atenart@kernel.org> wrote:
> > Quoting Maxime Chevallier (2024-04-12 12:46:14)
> > > 
> > > This patch fixes a commit that is in net-next, hence the net-next tag and the
> > > lack of "Fixes" tag.  
> > 
> > You can use Fixes: on net-next, that still helps to identify which
> > commit is being fixed (eg. for reviews, while looking at the history,
> > etc).
> 
> Won't the tag become invalid when the commit gets merged into an -rc
> release then ?

If the commit is in net or net-next it should not, as it'll be pulled &
merged.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Heiner Kallweit]

On 12.04.2024 12:46, Maxime Chevallier wrote:
> In situations where phylib is a module, the topology can be NULL as it's
> not initialized at netdev creation.
> 

What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's
false from net core, but true from r8169 driver. So topo_create is a stub,
but topo_add is not. IS_REACHABLE() hides dependencies.

topo_create et al don't really use something from phylib.
Therefore, could/should it be moved to net core?
At least for topo_create this would resolve the dependency.

We could also add a config symbol and the PHY topology an optional
extension of net core.

> Allow passing a NULL topology pointer to phy_link_topo helpers.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> ---
> 
> Hi,
> 
> This patch fixes a commit that is in net-next, hence the net-next tag and the
> lack of "Fixes" tag.
> 
> Nathan, Heiner, can you confirm this solves what you're seeing ?
> 
> I think we can improve on this solution by moving the topology init at
> the first PHY insertion and clearing it at netdev destruction.
> 
> Maxime
> 
>  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
>  include/linux/phy_link_topology.h   |  7 ++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 985941c5c558..0f3973f07fac 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
>  	struct phy_device_node *pdn;
>  	int ret;
>  
> +	if (!topo)
> +		return 0;
> +
>  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
>  	if (!pdn)
>  		return -ENOMEM;
> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
>  void phy_link_topo_del_phy(struct phy_link_topology *topo,
>  			   struct phy_device *phy)
>  {
> -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return;
> +
> +	pdn = xa_erase(&topo->phys, phy->phyindex);
>  
>  	/* We delete the PHY from the topology, however we don't re-set the
>  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 6b79feb607e7..21ca78127d0f 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h
> @@ -40,7 +40,12 @@ struct phy_link_topology {
>  static inline struct phy_device *
>  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
>  {
> -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return NULL;
> +
> +	pdn = xa_load(&topo->phys, phyindex);
>  
>  	if (pdn)
>  		return pdn->phy;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Maxime Chevallier]

Hello Heiner,

On Fri, 12 Apr 2024 15:07:46 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 12.04.2024 12:46, Maxime Chevallier wrote:
> > In situations where phylib is a module, the topology can be NULL as it's
> > not initialized at netdev creation.
> >   
> 
> What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's
> false from net core, but true from r8169 driver. So topo_create is a stub,
> but topo_add is not. IS_REACHABLE() hides dependencies.
> 
> topo_create et al don't really use something from phylib.
> Therefore, could/should it be moved to net core?

That's a valid point, and a better solution indeed.

> At least for topo_create this would resolve the dependency.
> 
> We could also add a config symbol and the PHY topology an optional
> extension of net core.

That could be a thing indeed. It could be selected by phylib then, I
don't see it being a user-controlled option, as this would make it very
confusing for users to only be able to see when there are mutiple PHYs
on the link when the relevant option is enabled (but I might be wrong).

Maxime

> 
> > Allow passing a NULL topology pointer to phy_link_topo helpers.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> > Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> > ---
> > 
> > Hi,
> > 
> > This patch fixes a commit that is in net-next, hence the net-next tag and the
> > lack of "Fixes" tag.
> > 
> > Nathan, Heiner, can you confirm this solves what you're seeing ?
> > 
> > I think we can improve on this solution by moving the topology init at
> > the first PHY insertion and clearing it at netdev destruction.
> > 
> > Maxime
> > 
> >  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
> >  include/linux/phy_link_topology.h   |  7 ++++++-
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> > index 985941c5c558..0f3973f07fac 100644
> > --- a/drivers/net/phy/phy_link_topology.c
> > +++ b/drivers/net/phy/phy_link_topology.c
> > @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
> >  	struct phy_device_node *pdn;
> >  	int ret;
> >  
> > +	if (!topo)
> > +		return 0;
> > +
> >  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> >  	if (!pdn)
> >  		return -ENOMEM;
> > @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
> >  void phy_link_topo_del_phy(struct phy_link_topology *topo,
> >  			   struct phy_device *phy)
> >  {
> > -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> > +	struct phy_device_node *pdn;
> > +
> > +	if (!topo)
> > +		return;
> > +
> > +	pdn = xa_erase(&topo->phys, phy->phyindex);
> >  
> >  	/* We delete the PHY from the topology, however we don't re-set the
> >  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> > index 6b79feb607e7..21ca78127d0f 100644
> > --- a/include/linux/phy_link_topology.h
> > +++ b/include/linux/phy_link_topology.h
> > @@ -40,7 +40,12 @@ struct phy_link_topology {
> >  static inline struct phy_device *
> >  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
> >  {
> > -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
> > +	struct phy_device_node *pdn;
> > +
> > +	if (!topo)
> > +		return NULL;
> > +
> > +	pdn = xa_load(&topo->phys, phyindex);
> >  
> >  	if (pdn)
> >  		return pdn->phy;  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Heiner Kallweit]

On 12.04.2024 15:23, Maxime Chevallier wrote:
> Hello Heiner,
> 
> On Fri, 12 Apr 2024 15:07:46 +0200
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> On 12.04.2024 12:46, Maxime Chevallier wrote:
>>> In situations where phylib is a module, the topology can be NULL as it's
>>> not initialized at netdev creation.
>>>   
>>
>> What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's
>> false from net core, but true from r8169 driver. So topo_create is a stub,
>> but topo_add is not. IS_REACHABLE() hides dependencies.
>>
>> topo_create et al don't really use something from phylib.
>> Therefore, could/should it be moved to net core?
> 
> That's a valid point, and a better solution indeed.
> 
>> At least for topo_create this would resolve the dependency.
>>
>> We could also add a config symbol and the PHY topology an optional
>> extension of net core.
> 
> That could be a thing indeed. It could be selected by phylib then, I
> don't see it being a user-controlled option, as this would make it very
> confusing for users to only be able to see when there are mutiple PHYs
> on the link when the relevant option is enabled (but I might be wrong).
> 
AFAIK the issue still exists on net-next. Are you going to submit
an updated version?

> Maxime
> 
>>
>>> Allow passing a NULL topology pointer to phy_link_topo helpers.
>>>
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
>>> Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
>>> ---
>>>
>>> Hi,
>>>
>>> This patch fixes a commit that is in net-next, hence the net-next tag and the
>>> lack of "Fixes" tag.
>>>
>>> Nathan, Heiner, can you confirm this solves what you're seeing ?
>>>
>>> I think we can improve on this solution by moving the topology init at
>>> the first PHY insertion and clearing it at netdev destruction.
>>>
>>> Maxime
>>>
>>>  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
>>>  include/linux/phy_link_topology.h   |  7 ++++++-
>>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
>>> index 985941c5c558..0f3973f07fac 100644
>>> --- a/drivers/net/phy/phy_link_topology.c
>>> +++ b/drivers/net/phy/phy_link_topology.c
>>> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
>>>  	struct phy_device_node *pdn;
>>>  	int ret;
>>>  
>>> +	if (!topo)
>>> +		return 0;
>>> +
>>>  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
>>>  	if (!pdn)
>>>  		return -ENOMEM;
>>> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
>>>  void phy_link_topo_del_phy(struct phy_link_topology *topo,
>>>  			   struct phy_device *phy)
>>>  {
>>> -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
>>> +	struct phy_device_node *pdn;
>>> +
>>> +	if (!topo)
>>> +		return;
>>> +
>>> +	pdn = xa_erase(&topo->phys, phy->phyindex);
>>>  
>>>  	/* We delete the PHY from the topology, however we don't re-set the
>>>  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
>>> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
>>> index 6b79feb607e7..21ca78127d0f 100644
>>> --- a/include/linux/phy_link_topology.h
>>> +++ b/include/linux/phy_link_topology.h
>>> @@ -40,7 +40,12 @@ struct phy_link_topology {
>>>  static inline struct phy_device *
>>>  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
>>>  {
>>> -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
>>> +	struct phy_device_node *pdn;
>>> +
>>> +	if (!topo)
>>> +		return NULL;
>>> +
>>> +	pdn = xa_load(&topo->phys, phyindex);
>>>  
>>>  	if (pdn)
>>>  		return pdn->phy;  
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: phy_link_topology: Handle NULL topologies

[Maxime Chevallier]

Hello Heiner,

On Sat, 27 Apr 2024 21:34:21 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 12.04.2024 15:23, Maxime Chevallier wrote:
> > Hello Heiner,
> > 
> > On Fri, 12 Apr 2024 15:07:46 +0200
> > Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >   
> >> On 12.04.2024 12:46, Maxime Chevallier wrote:  
> >>> In situations where phylib is a module, the topology can be NULL as it's
> >>> not initialized at netdev creation.
> >>>     
> >>
> >> What we see here is a bigger drawback of IS_REACHABLE(). For phylib it's
> >> false from net core, but true from r8169 driver. So topo_create is a stub,
> >> but topo_add is not. IS_REACHABLE() hides dependencies.
> >>
> >> topo_create et al don't really use something from phylib.
> >> Therefore, could/should it be moved to net core?  
> > 
> > That's a valid point, and a better solution indeed.
> >   
> >> At least for topo_create this would resolve the dependency.
> >>
> >> We could also add a config symbol and the PHY topology an optional
> >> extension of net core.  
> > 
> > That could be a thing indeed. It could be selected by phylib then, I
> > don't see it being a user-controlled option, as this would make it very
> > confusing for users to only be able to see when there are mutiple PHYs
> > on the link when the relevant option is enabled (but I might be wrong).
> >   
> AFAIK the issue still exists on net-next. Are you going to submit
> an updated version?

It still is indeed. I've been very busy last week unfortunately, I'll
followup today though. Thanks for the patience,

Maxime

> 
> > Maxime
> >   
> >>  
> >>> Allow passing a NULL topology pointer to phy_link_topo helpers.
> >>>
> >>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> >>> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> >>> Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> >>> ---
> >>>
> >>> Hi,
> >>>
> >>> This patch fixes a commit that is in net-next, hence the net-next tag and the
> >>> lack of "Fixes" tag.
> >>>
> >>> Nathan, Heiner, can you confirm this solves what you're seeing ?
> >>>
> >>> I think we can improve on this solution by moving the topology init at
> >>> the first PHY insertion and clearing it at netdev destruction.
> >>>
> >>> Maxime
> >>>
> >>>  drivers/net/phy/phy_link_topology.c | 10 +++++++++-
> >>>  include/linux/phy_link_topology.h   |  7 ++++++-
> >>>  2 files changed, 15 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> >>> index 985941c5c558..0f3973f07fac 100644
> >>> --- a/drivers/net/phy/phy_link_topology.c
> >>> +++ b/drivers/net/phy/phy_link_topology.c
> >>> @@ -42,6 +42,9 @@ int phy_link_topo_add_phy(struct phy_link_topology *topo,
> >>>  	struct phy_device_node *pdn;
> >>>  	int ret;
> >>>  
> >>> +	if (!topo)
> >>> +		return 0;
> >>> +
> >>>  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> >>>  	if (!pdn)
> >>>  		return -ENOMEM;
> >>> @@ -93,7 +96,12 @@ EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
> >>>  void phy_link_topo_del_phy(struct phy_link_topology *topo,
> >>>  			   struct phy_device *phy)
> >>>  {
> >>> -	struct phy_device_node *pdn = xa_erase(&topo->phys, phy->phyindex);
> >>> +	struct phy_device_node *pdn;
> >>> +
> >>> +	if (!topo)
> >>> +		return;
> >>> +
> >>> +	pdn = xa_erase(&topo->phys, phy->phyindex);
> >>>  
> >>>  	/* We delete the PHY from the topology, however we don't re-set the
> >>>  	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> >>> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> >>> index 6b79feb607e7..21ca78127d0f 100644
> >>> --- a/include/linux/phy_link_topology.h
> >>> +++ b/include/linux/phy_link_topology.h
> >>> @@ -40,7 +40,12 @@ struct phy_link_topology {
> >>>  static inline struct phy_device *
> >>>  phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex)
> >>>  {
> >>> -	struct phy_device_node *pdn = xa_load(&topo->phys, phyindex);
> >>> +	struct phy_device_node *pdn;
> >>> +
> >>> +	if (!topo)
> >>> +		return NULL;
> >>> +
> >>> +	pdn = xa_load(&topo->phys, phyindex);
> >>>  
> >>>  	if (pdn)
> >>>  		return pdn->phy;    
> >>  
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel