Re: [PATCH 01/10] hw/arm/realview: Simplify using 'break' statement

[Peter Maydell]

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