netdev - Re: [PATCH] sis900: Fix the tx queue timeout issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 2 Aug 2013 16:13:14 +0400
From:	Denis Kirjanov <kirjanov@...il.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	Denis Kirjanov <kda@...ux-powerpc.org>, venza@...wnhat.org,
	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] sis900: Fix the tx queue timeout issue

On 8/2/13, Ben Hutchings <bhutchings@...arflare.com> wrote:
> On Wed, 2013-07-31 at 18:43 +0400, Denis Kirjanov wrote:
> [...]
>> We are trying to initiate a frame transmission even
>> if we are not ready to do so due auto negotiation progress:
>>
>> timer routine checks the link status and if it's up calls
>> netif_carrier_on() allowing upper layer to start the tx queue
>
> Right.  So why is sis900_start_xmit() still being called?  Is this
> handling the case where the link just went down and the TX scheduler has
> not yet reacted to this?
>
> Would it actually hurt if a packet is queued at this point?  I see no
> reason to think it would.  In that case you could remove the entire
> if-statement.

Yeah, it's completely wrong to handle such issue in tx path and it
must be removed.
I've prepared an another patch... I'll send it shortly.

Thanks.

> Ben.
>
>> Signed-off-by: Denis Kirjanov <kda@...ux-powerpc.org>
>> ---
>>  drivers/net/ethernet/sis/sis900.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sis/sis900.c
>> b/drivers/net/ethernet/sis/sis900.c
>> index eb4aea3..9516734 100644
>> --- a/drivers/net/ethernet/sis/sis900.c
>> +++ b/drivers/net/ethernet/sis/sis900.c
>> @@ -1613,9 +1613,13 @@ sis900_start_xmit(struct sk_buff *skb, struct
>> net_device *net_dev)
>>  	unsigned int  count_dirty_tx;
>>
>>  	/* Don't transmit data before the complete of auto-negotiation */
>> -	if(!sis_priv->autong_complete){
>> -		netif_stop_queue(net_dev);
>> -		return NETDEV_TX_BUSY;
>> +	if (unlikely(!sis_priv->autong_complete)) {
>> +		if (netif_msg_tx_err(sis_priv) && net_ratelimit())
>> +			printk(KERN_DEBUG "%s: Auto negotiation in progress\n",
>> +					net_dev->name);
>> +		net_dev->stats.tx_dropped++;
>> +		dev_kfree_skb(skb);
>> +		return NETDEV_TX_OK;
>>  	}
>>
>>  	spin_lock_irqsave(&sis_priv->lock, flags);
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Regards,
Denis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html