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

From: Lino Sanfilippo <[email-masked]>
Date: Tue, 16 Feb 2016 23:09:56 +0100

On 12.02.2016 20:10, [email-masked] 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