Re: [PATCH v7 12/16] emul/ns16550: implement dump_state() hook

[dmukhin@xxxxxxxx]

On Tue, Nov 18, 2025 at 08:00:00AM +0200, Mykola Kvach wrote:
> Hi Denis,
> 
> Thank you for the patch.
..
> 
> > +static void cf_check ns16x50_dump_state(void *arg)
> > +{
> > +#ifdef CONFIG_VUART_NS16X50_DEBUG
> > +    struct vuart_ns16x50 *vdev = arg;
> > +    const struct domain *d = vdev->owner;
> > +    const struct vuart_info *info = vdev->info;
> > +    const struct xencons_interface *cons;
> > +    const uint8_t *regs;
> > +
> > +    if ( !vdev )
> 
> Is this NULL check actually useful here? At this point we’ve already
> dereferenced vdev (vdev->owner / vdev->info), so if arg could be NULL
> we’d already be in UB. Either the hook never receives NULL (and we can
> drop the check or turn it into ASSERT(vdev)), or the check should be
> moved before the first dereference.

Will promote to ASSERT().

> 
> > +        return;
> > +
> > +    /* Allow printing state in case of a deadlock. */
> > +    if ( !spin_trylock(&vdev->lock) )
> > +        return;
> > +
> > +    cons = &vdev->cons;
> > +    regs = &vdev->regs[0];
> > +
> > +    printk("Virtual " pr_prefix " (%s) I/O port 0x%04x IRQ#%d owner %pd\n",
> > +            vdev->name, info->base_addr, info->irq, d);
> > +
> > +    printk("  RX FIFO size %ld in_prod %d in_cons %d used %d\n",
> > +            ARRAY_SIZE(cons->in), cons->in_prod, cons->in_cons,
> > +            cons->in_prod - cons->in_cons);
> > +
> > +    printk("  TX FIFO size %ld out_prod %d out_cons %d used %d\n",
> > +            ARRAY_SIZE(cons->out), cons->out_prod, cons->out_cons,
> > +            cons->out_prod - cons->out_cons);
> > +
> > +    printk("  %02"PRIx8" RBR %02"PRIx8" THR %02"PRIx8" DLL %02"PRIx8" DLM 
> > %02"PRIx8"\n",
> > +            UART_RBR,
> 
> Should this be using cons->in / cons->out instead of cons?

Yes, it should!
Thanks for the catch!

> 
> > +            cons->in[MASK_XENCONS_IDX(cons->in_prod, cons)],
> > +            cons->out[MASK_XENCONS_IDX(cons->out_prod, cons)],
> 
> As written, MASK_XENCONS_IDX() gets &vdev->cons (struct pointer), not the
> RX/TX arrays themselves, so its size/index calculation will use the size
> of the pointer/struct rather than the in[]/out[] ring size. I think this
> should be:
> 
>     cons->in[MASK_XENCONS_IDX(cons->in_prod, cons->in)],
>     cons->out[MASK_XENCONS_IDX(cons->out_prod, cons->out)],
> 
> 
> Best regards,
> Mykola