diff mbox series

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

Message ID 20240412104615.3779632-1-maxime.chevallier@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: phy: phy_link_topology: Handle NULL topologies | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 928 this patch: 928
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 938 this patch: 938
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 939 this patch: 939
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 35 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-12--18-00 (tests: 959)

Commit Message

Maxime Chevallier April 12, 2024, 10:46 a.m. UTC
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(-)

Comments

Heiner Kallweit April 12, 2024, 11:35 a.m. UTC | #1
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;
Antoine Tenart April 12, 2024, 1:03 p.m. UTC | #2
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
Heiner Kallweit April 12, 2024, 1:07 p.m. UTC | #3
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;
Maxime Chevallier April 12, 2024, 1:16 p.m. UTC | #4
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
Antoine Tenart April 12, 2024, 1:20 p.m. UTC | #5
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.
Maxime Chevallier April 12, 2024, 1:23 p.m. UTC | #6
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;  
>
Heiner Kallweit April 27, 2024, 7:34 p.m. UTC | #7
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;  
>>
>
Maxime Chevallier April 29, 2024, 11:55 a.m. UTC | #8
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;    
> >>  
> >   
>
diff mbox series

Patch

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;