Re: [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 03/16/11 10:15, Alon Levy wrote:
> On Mon, Mar 14, 2011 at 02:54:59PM +0100, Jes Sorensen wrote:
>>> +typedef struct __attribute__ ((__packed__)) CCID_Header {
>>> +    uint8_t     bMessageType;
>>> +    uint32_t    dwLength;
>>> +    uint8_t     bSlot;
>>> +    uint8_t     bSeq;
>>> +} CCID_Header;
>>
>> Is this header decided upon by the CCID spec or the code? It seems
>> suboptimal to have a uint8 in front of a uint32 like that. Inefficient
>> structure alignment :(
>>
> 
> In the spec.

I was afraid of that, clearly a spec written by the people doing the
wire protocol, without considering the software aspects.

>>> +/**
>>> + * powered - defaults to true, changed by PowerOn/PowerOff messages
>>> + */
>>> +struct USBCCIDState {
>>> +    USBDevice dev;
>>> +    CCIDBus *bus;
>>> +    CCIDCardState *card;
>>> +    CCIDCardInfo *cardinfo; /* caching the info pointer */
>>> +    uint8_t  debug;
>>> +    BulkIn bulk_in_pending[BULK_IN_PENDING_NUM]; /* circular */
>>> +    uint32_t bulk_in_pending_start;
>>> +    uint32_t bulk_in_pending_end; /* first free */
>>> +    uint32_t bulk_in_pending_num;
>>> +    BulkIn *current_bulk_in;
>>> +    uint8_t  bulk_out_data[BULK_OUT_DATA_SIZE];
>>> +    uint32_t bulk_out_pos;
>>> +    uint8_t  bmSlotICCState;
>>> +    uint8_t  powered;
>>> +    uint8_t  notify_slot_change;
>>> +    uint64_t last_answer_error;
>>> +    Answer pending_answers[PENDING_ANSWERS_NUM];
>>> +    uint32_t pending_answers_start;
>>> +    uint32_t pending_answers_end;
>>> +    uint32_t pending_answers_num;
>>> +    uint8_t  bError;
>>> +    uint8_t  bmCommandStatus;
>>> +    uint8_t  bProtocolNum;
>>> +    uint8_t  abProtocolDataStructure[MAX_PROTOCOL_SIZE];
>>> +    uint32_t ulProtocolDataStructureSize;
>>> +    uint32_t state_vmstate;
>>> +    uint8_t  migration_state;
>>> +    uint32_t migration_target_ip;
>>> +    uint16_t migration_target_port;
>>> +};
>>
>> Try to place  the struct elements a little better so you don't end up
>> with a lot of space wasted due to natural alignment by the compiler.
>>
> 
> ok, this one's me. I'm really not sure except for stuff that goes on the wire
> or get's allocated a bazillion times that this is worth the change in general,
> but since I'm respinning anyway I'll do it. (unless you're saying it should be
> a habit).

If it is a one-off allocation, it's really not a big deal, but it is a
good thing to keep in mind. In particular on non-x86 64 bit entities are
normally 64 bit aligned.

>>> +static void ccid_bulk_in_get(USBCCIDState *s)
>>> +{
>>> +    if (s->current_bulk_in != NULL || s->bulk_in_pending_num == 0) {
>>> +        return;
>>> +    }
>>> +    assert(s->bulk_in_pending_num > 0);
>>> +    s->bulk_in_pending_num--;
>>> +    s->current_bulk_in = &s->bulk_in_pending[
>>> +        (s->bulk_in_pending_start++) % BULK_IN_PENDING_NUM];
>>
>> That line break is really not good :( Either break it after the '=' or
>> calculate the index outside the assignment statement.
> 
> ok, after the =, but then I think the rest is >80, so it will neccessitate another
> break.

If it was my code, I would calculate the index on the previous line in a
tmp variable. It is a matter of personal preference of course.

>>> +static void ccid_write_data_block(
>>> +    USBCCIDState *s, uint8_t slot, uint8_t seq,
>>> +    const uint8_t *data, uint32_t len)
>>
>> Please fix this - keep some arguments on the first line, and align the
>> following ones to match.
> 
> Is that a coding style thing I missed or personal preferance? my personal preferance
> here is the way it is, since it looks shorter/more readable, but I don't care that
> much.

It is not written down :(, but it is common practice. I raise the issue
exactly because it is much more readable the other way :)

>>
>>> +/* handle a single USB_TOKEN_OUT, return value returned to guest.
>>> + * 0 - all ok
>>> + * USB_RET_STALL - failed to handle packet */
>>
>> another badly formatted comment
>>
> fixing them all.

Excellent!

>>> +void ccid_card_card_error(CCIDCardState *card, uint64_t error)
>>> +{
>>> +    USBCCIDState *s =
>>> +        DO_UPCAST(USBCCIDState, dev.qdev, card->qdev.parent_bus->parent);
>>> +
>>> +    s->bmCommandStatus = COMMAND_STATUS_FAILED;
>>> +    s->last_answer_error = error;
>>> +    DPRINTF(s, 1, "VSC_Error: %lX\n", s->last_answer_error);
>>> +    /* TODO: these error's should be more verbose and propogated to the guest.
>>> +     * */
>>> +    /* we flush all pending answers on CardRemove message in ccid-card-passthru,
>>> +     * so check that first to not trigger abort */
>>
>> !!! there's more below.
> ? more badly formated comments? more todos? more flushing?

Comments yeah. It doesn't affect the code, but for a new patch it really
is better to get it straightened before it goes upstream.

Cheers,
Jes