| Message ID | 20230524145906.33156-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/realview: Introduce abstract RealviewMachineClass | expand |
On 5/24/23 07:58, Philippe Mathieu-Daudé wrote: > The 'break' statement terminates the execution of the nearest > enclosing 'for' statement in which it appears. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/arm/realview.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/arm/realview.c b/hw/arm/realview.c > index a5aa2f046a..a52ff35084 100644 > --- a/hw/arm/realview.c > +++ b/hw/arm/realview.c > @@ -88,7 +88,6 @@ static void realview_init(MachineState *machine, > I2CBus *i2c; > int n; > unsigned int smp_cpus = machine->smp.cpus; > - int done_nic = 0; > qemu_irq cpu_irq[4]; > int is_mpcore = 0; > int is_pb = 0; > @@ -294,14 +293,13 @@ static void realview_init(MachineState *machine, > for(n = 0; n < nb_nics; n++) { > nd = &nd_table[n]; > > - if (!done_nic && (!nd->model || > - strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0)) { > + if (!nd->model || strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0) { > if (is_pb) { > lan9118_init(nd, 0x4e000000, pic[28]); > } else { > smc91c111_init(nd, 0x4e000000, pic[28]); > } > - done_nic = 1; > + break; While I agree this preserves existing behaviour, it doesn't seem like the logic is actually correct. This will only ever connect 1 of nb_nics. r~ > } else { > if (pci_bus) { > pci_nic_init_nofail(nd, pci_bus, "rtl8139", NULL);
On Wed, 24 May 2023 at 20:01, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/24/23 07:58, Philippe Mathieu-Daudé wrote: > > The 'break' statement terminates the execution of the nearest > > enclosing 'for' statement in which it appears. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > --- > > hw/arm/realview.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/hw/arm/realview.c b/hw/arm/realview.c > > index a5aa2f046a..a52ff35084 100644 > > --- a/hw/arm/realview.c > > +++ b/hw/arm/realview.c > > @@ -88,7 +88,6 @@ static void realview_init(MachineState *machine, > > I2CBus *i2c; > > int n; > > unsigned int smp_cpus = machine->smp.cpus; > > - int done_nic = 0; > > qemu_irq cpu_irq[4]; > > int is_mpcore = 0; > > int is_pb = 0; > > @@ -294,14 +293,13 @@ static void realview_init(MachineState *machine, > > for(n = 0; n < nb_nics; n++) { > > nd = &nd_table[n]; > > > > - if (!done_nic && (!nd->model || > > - strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0)) { > > + if (!nd->model || strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0) { > > if (is_pb) { > > lan9118_init(nd, 0x4e000000, pic[28]); > > } else { > > smc91c111_init(nd, 0x4e000000, pic[28]); > > } > > - done_nic = 1; > > + break; > > While I agree this preserves existing behaviour, it doesn't seem like the logic is > actually correct. This will only ever connect 1 of nb_nics. Does it preserve the existing behaviour, though? I think the intent of the code is: * we only create one at most hard-wired NIC (the lan9118 or smc91c111, depending on the board type), and we do that for the first entry in the nd_table which specifies a matching 'model' * every other NIC is a PCI rtl8139 (or ignored if the board has no PCI) Maybe I'm misreading the current code, but it looks to me like it does this: done_nic is a flag for "did we create the hardwired device", and we only create the hardwired device if the flag is false and the NIC model matches. Once we've created the hardwired device we set done_nic to true and the if() will then always take us into the rtl8139 part. On the other hand, this patch using break means that once the hardwired NIC has been created we'll exit the loop entirely and won't create any subsequent PCI devices. thanks -- PMM
diff --git a/hw/arm/realview.c b/hw/arm/realview.c index a5aa2f046a..a52ff35084 100644 --- a/hw/arm/realview.c +++ b/hw/arm/realview.c @@ -88,7 +88,6 @@ static void realview_init(MachineState *machine, I2CBus *i2c; int n; unsigned int smp_cpus = machine->smp.cpus; - int done_nic = 0; qemu_irq cpu_irq[4]; int is_mpcore = 0; int is_pb = 0; @@ -294,14 +293,13 @@ static void realview_init(MachineState *machine, for(n = 0; n < nb_nics; n++) { nd = &nd_table[n]; - if (!done_nic && (!nd->model || - strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0)) { + if (!nd->model || strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0) { if (is_pb) { lan9118_init(nd, 0x4e000000, pic[28]); } else { smc91c111_init(nd, 0x4e000000, pic[28]); } - done_nic = 1; + break; } else { if (pci_bus) { pci_nic_init_nofail(nd, pci_bus, "rtl8139", NULL);
The 'break' statement terminates the execution of the nearest enclosing 'for' statement in which it appears. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/arm/realview.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)