Re: [PATCH 4/6] myri10ge - First half of the driver

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

 



Francois Romieu wrote:
>
>> +	spin_lock(&mgp->cmd_lock);
>> +	response->result = 0xffffffff;
>> +	mb();
>> +	myri10ge_pio_copy((void __iomem *) cmd_addr, buf, sizeof (*buf));
>> +
>> +	/* wait up to 2 seconds */
>>     
>
> You must not hold a spinlock for up to 2 seconds.
>   

We are working on reducing the delay to about 15ms. It only occurs when
the driver is loaded or the link brought up.

>> +	for (sleep_total = 0; sleep_total < (2 * 1000); sleep_total += 10) {
>> +		mb();
>> +		if (response->result != 0xffffffff) {
>> +			if (response->result == 0) {
>> +				data->data0 = ntohl(response->data);
>> +				spin_unlock(&mgp->cmd_lock);
>> +				return 0;
>> +			} else {
>> +				dev_err(&mgp->pdev->dev,
>> +					"command %d failed, result = %d\n",
>> +				       cmd, ntohl(response->result));
>> +				spin_unlock(&mgp->cmd_lock);
>> +				return -ENXIO;
>>     
>
> Return in a middle of a spinlock-intensive function. :o(
>   

What do you mean ?

>   
>> +{
>> +	struct sk_buff *skb;
>> +	unsigned long data, roundup;
>> +
>> +	skb = dev_alloc_skb(bytes + 4096 + MYRI10GE_MCP_ETHER_PAD);
>> +	if (skb == NULL)
>> +		return NULL;
>>     
>
> Imho you will want to work directly with pages shortly.
>   

We had thought about doing this, but were a little nervous since we did
not know of any other drivers that worked directly with pages.  If this
is an official direction to work directly with pages, we will. But the
existing approach is well tested through our beta cycle, and we would
prefer to leave it as is and update to a pages based approach in the
future.


Thanks a lot for all the comments.

Brice

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux