netdev - Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it should be toggled

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: <e39ae93c-60eb-7991-3b15-70a05aca3377@aquantia.com>
Date:   Wed, 16 Oct 2019 13:19:30 +0000
From:   Igor Russkikh <Igor.Russkikh@...antia.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it
 should be toggled

Hello Jakub,

>> workaround when stopping the device), register bit should actually
>> be toggled.
> 
> Does the bit get set by the driver or HW?
> 
> If it gets set by HW there is still a tiny race from reading to
> writing.. Perhaps doing two writes -> to 0 and to 1 would be a better
> option?  

No, set is done by the driver, not HW. HW just tracks for the toggle.

>> It was previosly always just set. Due to the way driver stops HW this
>> never actually caused any issues, but it still may, so cleaning this up.
> 
> Hm. So is it a cleanup of fix? Does the way code is written guarantee
> it will never cause issues?

Yes, thats a cleanup. We just had other products where this cache reset had to
be done multiple times. Obviously doing that second time was just no-op for
hardware ;)
On linux this always gets called on deinit only once - thus it was safe.
We just aligning here the linux driver with actual HW specification.

>> +	if (err)
>> +		goto err_exit;
>> +
>> +	readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get,
>> +				  self, val, val == 1, 1000U, 10000U);
> 
> It's a little strange to toggle, yet wait for it to be of a specific
> value..

Notice thats a different value - 'cache_init_done' bit.
This is used by HW to indicate completion of cache reset operation.

>> +err_exit:
>> +	return err;
> 
> Just return err instead of doing this pointless goto. It make the code
> harder to follow.

Sure

>> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0
>> +
>> +
> 
> two empty lines here?

Will fix.

Regards,
  Igor