[PATCH] net: hsr: Use full string description when opening HSR network device

[PATCH] net: hsr: Use full string description when opening HSR network device

[Lukasz Majewski]

Up till now only single character ('A' or 'B') was used to provide
information of HSR slave network device status.

As it is also possible and valid, that Interlink network device may
be supported as well, the description must be more verbose. As a result
the full string description is now used.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 net/hsr/hsr_device.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 9d71b66183da..9a60489fba96 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -142,30 +142,29 @@ static int hsr_dev_open(struct net_device *dev)
 {
 	struct hsr_priv *hsr;
 	struct hsr_port *port;
-	char designation;
+	char *designation = NULL;
 
 	hsr = netdev_priv(dev);
-	designation = '\0';
 
 	hsr_for_each_port(hsr, port) {
 		if (port->type == HSR_PT_MASTER)
 			continue;
 		switch (port->type) {
 		case HSR_PT_SLAVE_A:
-			designation = 'A';
+			designation = "Slave A";
 			break;
 		case HSR_PT_SLAVE_B:
-			designation = 'B';
+			designation = "Slave B";
 			break;
 		default:
-			designation = '?';
+			designation = "Unknown";
 		}
 		if (!is_slave_up(port->dev))
-			netdev_warn(dev, "Slave %c (%s) is not up; please bring it up to get a fully working HSR network\n",
+			netdev_warn(dev, "%s (%s) is not up; please bring it up to get a fully working HSR network\n",
 				    designation, port->dev->name);
 	}
 
-	if (designation == '\0')
+	if (!designation)
 		netdev_warn(dev, "No slave devices configured\n");
 
 	return 0;
-- 
2.20.1

Re: [PATCH] net: hsr: Use full string description when opening HSR network device

[Ravi Gunasekaran]

On 3/4/24 3:02 PM, Lukasz Majewski wrote:
> Up till now only single character ('A' or 'B') was used to provide
> information of HSR slave network device status.
> 
> As it is also possible and valid, that Interlink network device may
> be supported as well, the description must be more verbose. As a result
> the full string description is now used.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  net/hsr/hsr_device.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 9d71b66183da..9a60489fba96 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -142,30 +142,29 @@ static int hsr_dev_open(struct net_device *dev)
>  {
>  	struct hsr_priv *hsr;
>  	struct hsr_port *port;
> -	char designation;
> +	char *designation = NULL;
>  
>  	hsr = netdev_priv(dev);
> -	designation = '\0';
>  
>  	hsr_for_each_port(hsr, port) {
>  		if (port->type == HSR_PT_MASTER)
>  			continue;
>  		switch (port->type) {
>  		case HSR_PT_SLAVE_A:
> -			designation = 'A';
> +			designation = "Slave A";

"designation" is now a pointer and is being assigned value
without even allocating memory for it.

>  			break;
>  		case HSR_PT_SLAVE_B:
> -			designation = 'B';
> +			designation = "Slave B";
>  			break;
>  		default:
> -			designation = '?';
> +			designation = "Unknown";
>  		}
>  		if (!is_slave_up(port->dev))
> -			netdev_warn(dev, "Slave %c (%s) is not up; please bring it up to get a fully working HSR network\n",
> +			netdev_warn(dev, "%s (%s) is not up; please bring it up to get a fully working HSR network\n",
>  				    designation, port->dev->name);
>  	}
>  
> -	if (designation == '\0')
> +	if (!designation)
>  		netdev_warn(dev, "No slave devices configured\n");
>  
>  	return 0;

-- 
Regards,
Ravi

Re: [PATCH] net: hsr: Use full string description when opening HSR network device

[Andrew Lunn]

> >  		case HSR_PT_SLAVE_A:
> > -			designation = 'A';
> > +			designation = "Slave A";
> 
> "designation" is now a pointer and is being assigned value
> without even allocating memory for it.

"Slave A" is in memory somewhere, probably the .rodata section. So
designation now points to that memory.

	Andrew

Re: [PATCH] net: hsr: Use full string description when opening HSR network device

[Andrew Lunn]

On Mon, Mar 04, 2024 at 10:32:20AM +0100, Lukasz Majewski wrote:
> Up till now only single character ('A' or 'B') was used to provide
> information of HSR slave network device status.
> 
> As it is also possible and valid, that Interlink network device may
> be supported as well, the description must be more verbose. As a result
> the full string description is now used.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  net/hsr/hsr_device.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 9d71b66183da..9a60489fba96 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -142,30 +142,29 @@ static int hsr_dev_open(struct net_device *dev)
>  {
>  	struct hsr_priv *hsr;
>  	struct hsr_port *port;
> -	char designation;
> +	char *designation = NULL;

Thanks for splitting this into a patch.

Reverse Christmas tree. I know it is broken already, but we should not
make it worse.

I guess you can also add a const, since "Slave A" is probably const.

	Andrew

Re: [PATCH] net: hsr: Use full string description when opening HSR network device

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

Hi Andrew,

> On Mon, Mar 04, 2024 at 10:32:20AM +0100, Lukasz Majewski wrote:
> > Up till now only single character ('A' or 'B') was used to provide
> > information of HSR slave network device status.
> > 
> > As it is also possible and valid, that Interlink network device may
> > be supported as well, the description must be more verbose. As a
> > result the full string description is now used.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  net/hsr/hsr_device.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> > index 9d71b66183da..9a60489fba96 100644
> > --- a/net/hsr/hsr_device.c
> > +++ b/net/hsr/hsr_device.c
> > @@ -142,30 +142,29 @@ static int hsr_dev_open(struct net_device
> > *dev) {
> >  	struct hsr_priv *hsr;
> >  	struct hsr_port *port;
> > -	char designation;
> > +	char *designation = NULL;  
> 
> Thanks for splitting this into a patch.
> 
> Reverse Christmas tree. I know it is broken already, but we should not
> make it worse.
> 
> I guess you can also add a const, since "Slave A" is probably const.
> 

Yes, "Slave A/B" would be allocated in .rodata (as it it just a string
label).

const keyword added would help with the readability. 

> 	Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]