netdev - Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <645518234.216003.1436556107630.JavaMail.zimbra@savoirfairelinux.com>
Date:	Fri, 10 Jul 2015 15:21:47 -0400 (EDT)
From:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	netdev <netdev@...r.kernel.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	David <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kernel <kernel@...oirfairelinux.com>
Subject: Re: [PATCH 1/2] net: dsa: mv88e6xxx: sleep in _mv88e6xxx_stats_wait

Hi Guenter,

On Jul 10, 2015, at 2:36 PM, Guenter Roeck linux@...ck-us.net wrote:

> Hi Vivien,
> 
> On Fri, Jul 10, 2015 at 02:20:47PM -0400, Vivien Didelot wrote:
>> > 
>> > is this really beneficial and/or needed ?
>> 
>> Except using existing generic code, no.
>> 
>> > It adds at least 1ms delay to a loop which did not have any delay at
>> > all unless the register read itself was sleeping.
>> 
>> I must have missed where is the benefit from spin reading 10 times this
>> register, rather than sleeping 1ms between tests. Does this busy bit
>> behaves differently from the phy, atu, scratch, or vtu busy bits?
>> 
> Benefit is reaction time, mostly. If the result isn't ready after the
> first spin, the new code path adds a mandatory 1-2ms delay. This could
> add up to a lot if that kind of retry is seen a lot.

To me, it looks like if this mandatory 1-2ms delay is an issue, then
_mv88e6xxx_wait must be fixed. Maybe reducing this delay is an option?

> I don't now if there is a specific time limit for this busy bit,
> and/or if it behaves differently than the others in terms of timing.
> 
>> > Is the original function seen to return a timeout error under some
>> > circumstances ?
>> 
>> I didn't experience it myself, but I guess it may happen. In addition to
>> that, the current implementation doesn't check eventual read error.
>> That's why I saw a benefit in using _mv88e6xxx_wait().
> 
> Checking for a read error (or a timeout) is definitely a good thing.
> I could also imagine that, for example, a "clear statistics" request
> takes more time than currently supported. This is why I asked if you
> had seen a timeout with the old code.
> 
> Personally I'd rather leave the wait loop alone and only introduce
> error checking unless there is a reason to introduce a sleep,
> but I'd like to hear Andrew's and/or Florian's opinion.

Andrew may not reply since he's on vacation, but I add Florian in Cc.

Thanks,
-v
--
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