[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [ping: PATCH v2 0/3] e1000: allow model/device_id selec
From: |
Gabriel L. Somlo |
Subject: |
Re: [Qemu-devel] [ping: PATCH v2 0/3] e1000: allow model/device_id selection on command line |
Date: |
Tue, 27 May 2014 15:14:59 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
ping ?
On Wed, May 21, 2014 at 02:27:41PM -0400, Gabriel L. Somlo wrote:
> This started out as a single patch (now patch 2/3):
>
> Allow selection of different card models from the qemu
> command line, to better accomodate a wider range of guests.
>
> New in v2:
>
> - moved check for 8257x out of the way of QOM, as suggested by
> Michael (patch 1/3)
http://patchwork.ozlabs.org/patch/351274/
> - resolved "Signed-off-by" misunderstanding and miscellaneous style
> issues (patch 2/3)
http://patchwork.ozlabs.org/patch/351273/
> - modified e1000 test to check for all supported models, as suggested
> by Andreas (patch 3/3). I used eepro100-test.c as an example for
> this change.
http://patchwork.ozlabs.org/patch/351275/
> On Wed, May 21, 2014 at 12:04:57PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas F?rber wrote:
> > > static inline uint16_t maybe?
> >
> > inline in c file is an optimization hint for a compiler, so
> > really useless without a benchmark.
> > It's useful in headers since some compilers warn about unused
> > non-inline static functions.
>
> I went with "static inline uint16_t" in both 1/3 and 2/3, since each
> only get called once, and I'm only using a function instead of a
> preprocessor macro for aesthetic reasons. I'm not religious about it
> though, so if anyone still objects I can change it to whatever we all
> agree on :)
>
> > > > + d->phy_reg[PHY_ID2] = e1000_phy_id2_init(dev_id);
> >
> > I would just pass E1000State to e1000_phy_id2_init.
>
> So I'm still passing "uint16_t device_id" instead of E1000State,
> because e1000_phy_id2_init() replaces "enum { PHY_ID2_INIT = ...}",
> which goes before the E1000State typedef in the source, and I wanted
> to have the new code show up in the same spot in the patch as the code
> it replaces. That, and we wouldn't save anything either way, I'd still
> have to de-ref E1000State to get at the device_id, either in
> e1000_reset() or in e1000_phy_id2_init(). Again, not religious about
> it, so I can change it if you really want me to :)
>
> Thanks again,
> Gabriel
>
> Gabriel L. Somlo (3):
> e1000: avoid relying on device id (and soon, QOM) on data path
> e1000: allow command-line selection of card model
> tests: e1000: test additional device IDs
>
> hw/net/e1000.c | 127
> ++++++++++++++++++++++++++++++++++++++++++++++-------
> tests/e1000-test.c | 33 +++++++++++---
> 2 files changed, 136 insertions(+), 24 deletions(-)
>
> --
> 1.9.0
>
- [Qemu-devel] [PATCH v2 0/3] e1000: allow model/device_id selection on command line, Gabriel L. Somlo, 2014/05/21
- [Qemu-devel] [PATCH v2 2/3] e1000: allow command-line selection of card model, Gabriel L. Somlo, 2014/05/21
- [Qemu-devel] [PATCH v2 1/3] e1000: avoid relying on device id (and soon, QOM) on data path, Gabriel L. Somlo, 2014/05/21
- [Qemu-devel] [PATCH v2 3/3] tests: e1000: test additional device IDs, Gabriel L. Somlo, 2014/05/21
- Re: [Qemu-devel] [ping: PATCH v2 0/3] e1000: allow model/device_id selection on command line,
Gabriel L. Somlo <=
- Re: [Qemu-devel] [PATCH v2 0/3] e1000: allow model/device_id selection on command line, Michael S. Tsirkin, 2014/05/27