qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode


From: Hao Wu
Subject: Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
Date: Wed, 27 Jan 2021 13:59:07 -0800



On Wed, Jan 27, 2021 at 1:42 PM Corey Minyard <minyard@acm.org> wrote:
On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:
>
> > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > +
> > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > +{
> > > +    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > +
> > > +    if (received_bytes == 0) {
> > > +        npcm7xx_smbus_recv_fifo(s);
> > > +        return;
> > > +    }
> > > +
> > > +    s->sda = s->rx_fifo[s->rx_cur];
> > > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > +    --s->rxf_sts;
> >
> > This open-coded decrement seems a little risky.  Are you sure in every
> > case that s->rxf_sts > 0?  There's no way what's running in the VM can
> > game this and cause a buffer overrun?  One caller to this function seems
> > to protect against this, and another does not.
> >
> s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> also 0, so it'll take the if-branch and return without running --s->rxf_sts.

That is true if called from the
NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case.  There is no such check
in the NPCM7XX_SMBUS_STATUS_RECEIVING case.
I don't understand the reasoning here. The caller doesn't matter.
Previous code has:
 #define NPCM7XX_SMBRXF_STS_RX_BYTES(rv)     extract8((rv), 0, 5) 
So 
 uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
is guaranteed to be 0 if s->rxf_sts == 0.
As a result the code will take the following branch and returns:
 if (received_bytes == 0) {
    npcm7xx_smbus_recv_fifo(s);
    return;
 }
And will not execute the --s->rxf_sts sentence.
Please let me know if I missed anything here.

> I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.

You never want to do an assert if the hosted system can do something to
cause it.  If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
case, it would be ok, but really unnecessary.

If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
but you want to add a comment to that effect if so.  These sorts of
things look dangerous.

There is also the question about who takes these patches in.  I'm the
I2C maintainer, but there's other code in this series.  Once everything
is ready, I can ack them if we take it through the ARM tree.  Or I can
take it through my tree with the proper acks.
I think either  way is fine. Previous NPCM7XX patch series were taken in the ARM tree. But as i2c code taking into your tree is also fine.

-corey

>
> >
> > Other than this, I didn't see any issues with this patch.
> >
> > -corey
> >

reply via email to

[Prev in Thread] Current Thread [Next in Thread]