| Message ID | 20240304093220.4183179-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hsr: Use full string description when opening HSR network device | expand |
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;
> > 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 <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
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
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;
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(-)