netdev - Re: [BUG,BISECTED] mvneta: second interface no more usable on mirabox

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:	Wed, 17 Jun 2015 16:24:14 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Arnaud Ebalard <arno@...isbad.org>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [BUG,BISECTED] mvneta: second interface no more usable on mirabox

17.06.2015 02:05, Arnaud Ebalard пишет:
>> But it seems the patch can still change a couple of flags
>> for you, and maybe that makes a problem?
> AFAICT, autoneg config register (MVNETA_GMAC_AUTONEG_CONFIG) is
> modified.
Or, else, prevented from being modified at a couple of
bits that were wrongly cleared before. That code is now
used by both autoneg and mdio modes, so clearing the
autoneg bits is not possible (and not needed).
Unfortunately I haven't checked if these bits are ever
properly initialized. They are not. :( I think my u-boot did
that for me, and for you - only for the first interface.

>   And the logic when link status changes is also modified:
>   - MVNETA_GMAC_FORCE_LINK_DOWN flag cleared when there is carrier. It was
>     previously set when that event occured.
>   - The link down event logic is also modified.
This was needed to properly update the MAC status
register. It wasn't used before my patch, because the
status was retrieved with MDIO, and even with my patch
it is used only for inbound status, but I felt it would be
good to have it properly updated in all cases, to avoid the
surprises in the future. AFAIK these bits are only mirrored
by the relevant bits in the status register, and nothing more.

>> Please try the attached (absolutely untested) patch.
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index ce5f7f9..74176ec 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -1013,6 +1013,12 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>>   		val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER);
>>   		val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;
>>   		mvreg_write(pp, MVNETA_GMAC_CLOCK_DIVIDER, val);
>> +	} else {
>> +		val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
>> +		val &= ~(MVNETA_GMAC_INBAND_AN_ENABLE |
>> +		       MVNETA_GMAC_AN_SPEED_EN |
>> +		       MVNETA_GMAC_AN_DUPLEX_EN);
>> +		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
>>   	}
>>   
>>   	mvneta_set_ucast_table(pp, -1);
> *Second interface is back w/ that patch applied*. Cannot tell if it is a
>   proper fix, though or a valid workaround.
I think it is a proper fix - adding a missing initialization.
Previously that initialization was hidden in a completely
unexpected place, from where I had to remove it, but
forgot to re-add elsewhere.

> Thanks for your feedback.
Thanks for your testing, I'll submit a patch in a few days.
--
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