Michal Nazarewicz wrote:
> On Wed, 23 Nov 2011 18:10:41 +0100, Arvid Brodin <arvid.brodin@xxxxxxxx>
> wrote:
>> Small code refactoring to ease the real fix in patch #2.
>
> “patch #2” will be rather meaningless when the patch gets committed. :)
Thanks, you have a point there. :) I'll think about that in future patch
descriptions.
>> Signed-off-by: Arvid Brodin <arvid.brodin@xxxxxxxx>
>> Tested-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>
>> #define DEFAULT_I_TDPS 1024
>> @@ -403,12 +410,12 @@ static int priv_init(struct usb_hcd *hcd)
>> {
>> struct isp1760_hcd *priv = hcd_to_priv(hcd);
>> u32 hcc_params;
>> + int i;
>
> How about unsigned?
No. I'm comparing 'i' to QH_END, which is an enumerated constant.
According to the C90 standard, these are of type int. Mixing signed
and unsigned in the same expression is never a good idea in C.
Often the behaviour is up to the compiler implementation. I believe
it would work here, but why take the chance?
(I recently took a course named "Safer C" by Les Hatton, where
these problems were looked into in some detail, so I'm a bit primed
here. I recommend the course! And sorry for the plug. :)
>> spin_lock_init(&priv->lock);
>> - INIT_LIST_HEAD(&priv->interruptqhs);
>> - INIT_LIST_HEAD(&priv->controlqhs);
>> - INIT_LIST_HEAD(&priv->bulkqhs);
>> + for (i = 0; i < QH_END; i++)
>> + INIT_LIST_HEAD(&priv->qh_list[i]);
>> /*
>> * hw default: 1K periodic list heads, one per frame.
>> @@ -918,6 +925,7 @@ void schedule_ptds(struct usb_hcd *hcd)
>> struct usb_host_endpoint *ep;
>> LIST_HEAD(urb_list);
>> struct urb_listitem *urb_listitem, *urb_listitem_next;
>> + int i;
>
> Same her.
>
>> if (!hcd) {
>> WARN_ON(1);
>
> Feel free to ignore my comments, I just like “unsigned”. :P
>
--
Arvid Brodin
Enea Services Stockholm AB
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
