From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60206 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pzn0M-0003p5-M1 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:27:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pzn0K-0005ne-Qc for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:26:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20259) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pzn0K-0005nZ-F8 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:26:56 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2G9QtBX019018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 16 Mar 2011 05:26:55 -0400 Message-ID: <4D808246.8030704@redhat.com> Date: Wed, 16 Mar 2011 10:26:30 +0100 From: Jes Sorensen MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus References: <1298460024-23591-1-git-send-email-alevy@redhat.com> <1298460024-23591-2-git-send-email-alevy@redhat.com> <4D7E1E33.1070801@redhat.com> <20110316091551.GC7413@playa.tlv.redhat.com> In-Reply-To: <20110316091551.GC7413@playa.tlv.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: qemu-devel@nongnu.org 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