netdev - Re: [PATCH] net: pxa168_eth: add netconsole support

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: <alpine.LNX.2.20.13.1801311638170.604@monopod.intra.ispras.ru>
Date:   Wed, 31 Jan 2018 17:05:47 +0300 (MSK)
From:   Alexander Monakov <amonakov@...ras.ru>
To:     David Miller <davem@...emloft.net>
cc:     netdev@...r.kernel.org, rmk+kernel@....linux.org.uk,
        sebastian.hesselbarth@...il.com, f.fainelli@...il.com
Subject: Re: [PATCH] net: pxa168_eth: add netconsole support

> > +#ifdef CONFIG_NET_POLL_CONTROLLER
> > +static void pxa168_eth_netpoll(struct net_device *dev)
> > +{
> > +	struct pxa168_eth_private *pep = netdev_priv(dev);
> > +	napi_schedule(&pep->napi);
> > +}
> > +#endif
> 
> This definitely is not sufficient.
> 
> Look at what other drivers do.

Sorry, I did that, and got confused because some drivers bracket the interrupt
handler with disable_irq/enable_irq, some other drivers use local_irq_save/
local_irq_restore (which seems like a no-op because netconsole already uses
spin_lock_irqsave and netpoll_send_udp checks irqs_disabled), and a few drivers
call bare napi_schedule.

> You have to invoke the interrupt handler with the device's interrupts disabled,

Is this required for correctness? I have to admit I'm unsure why.

> collect the events, and most importantly mask chip interrupts before scheduling
> the NAPI instance.

And is this a matter of efficiency (not calling napi_schedule when there's
nothing to do and not keeping interrupts enabled for its duration), or also
a matter of correctness?

Sorry I'm not sending a revised patch yet, but without a firm understanding
of why changes are needed that would be a bit of a sin.

Thanks.
Alexander