netdev - Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

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]

Date:	Tue, 16 Feb 2016 23:09:56 +0100
From:	Lino Sanfilippo <LinoSanfilippo@....de>
To:	Bryan.Whitehead@...rochip.com, davem@...emloft.net
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

On 12.02.2016 20:10, Bryan.Whitehead@...rochip.com wrote:
> Lino,
> 
> Regarding "a matching smp_rmb() in the irq handler"
> There is a smp_wmb() in the irq handler, since in both cases we are forcing a write operation on software_irq_signal.
> 
> I suppose using atomic operations on software_irq_signal would also work, but this driver was based on 
> drivers/net/ethernet/smsc/smsc911x.c
> And if possible I'd prefer to keep logical changes to a minimum.
> Plus this is not a "read modify write" scenario so I think the memory barrier is sufficient.
> Do you agree?
> 

Hi Bryan,

youre right, smsc911x.c does the same thing and probably its ok. As far
as I have understood smp memory barriers (mainly from reading
memory-barriers.txt), they normally should be paired to ensure that a
"reader" thread actually sees what an "updater" thread writes - paired
in a sense that there is a corresponding smp_rmb() for a smp_wmb().

So in this case I expected the need for a smp_rmb() at least in that
loop in open() which waits for the software_irq_signal flag to toggle.
Something like

	while (timeout--) {
		smp_rmb();
		if (pdata->software_irq_signal)
			break;
		usleep_range(1000, 10000);
	}

But AFAICS calling usleep_range() already implies memory barriers, so I
agree that there is probably no need for an explicit smp_rmb.

Regards,
Lino