netdev - Re: [PATCH 2/2] net: mvneta: improve suspend/resume

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:   Fri, 30 Mar 2018 11:49:16 +0200
From:   Thomas Petazzoni <thomas.petazzoni@...tlin.com>
To:     Jisheng Zhang <Jisheng.Zhang@...aptics.com>
Cc:     David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] net: mvneta: improve suspend/resume

Hello,

On Fri, 30 Mar 2018 17:15:47 +0800, Jisheng Zhang wrote:

> > > +		cpuhp_state_remove_instance_nocalls(online_hpstate,
> > > +						    &pp->node_online);
> > > +		cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD,
> > > +						    &pp->node_dead);    
> > 
> > Do we need to remove/add those CPU notifiers when suspending/resuming ?  
> 
> Take mvneta_cpu_online() as an example, if we don't remove it during
> suspend, when system is resume back, it will touch mac when secondary
> cpu is ON, but at this point the mvneta isn't resumed, this is not safe.

Hm. I'm still a bit confused by this new CPU hotplug API. I understand
the issue you have and indeed unregistering the CPU hotplug callbacks
is a way to solve the problem, but I find it weird that we have to do
this.

Anyway, it's OK to do it, because it's anyway what was done so far. It
is just annoying that there is a duplication of the logic between
mvneta_suspend() and mvneta_stop() on one side, and duplication between
mvneta_resume() and mvnete_start() on the other side.

> > > +	for (queue = 0; queue < rxq_number; queue++) {
> > > +		struct mvneta_rx_queue *rxq = &pp->rxqs[queue];
> > > +
> > > +		mvneta_rxq_drop_pkts(pp, rxq);
> > > +	}    
> > 
> > Wouldn't it make sense to have
> > mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the
> > initialization ?  
> 
> For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation.
> So we reuse mvneta_rxq_drop_pkts() here.

Hum, OK, indeed. It would have been nicer to have something symmetric,
with the hw/sw parts split in a similar way for the init and deinit of
both txqs and rxqs, but I agree that dropping the RX packets before
going into suspend involves both HW and SW operations.

Thanks!

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com