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
