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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
> > 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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: [email protected]