Re: [PATCH v4 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris <briannorris@chromium.org>]

On Tue, Nov 15, 2016 at 09:35:07AM -0800, Dmitry Torokhov wrote:
> On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> >  }
> >  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
> >  
> > +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> > +{
> > +	struct mwifiex_adapter *adapter = priv;
> > +
> > +	if (adapter->irq_wakeup >= 0) {
> > +		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > +		adapter->wake_by_wifi = true;
> > +		disable_irq_nosync(irq);
> > +	}
> > +
> > +	/* Notify PM core we are wakeup source */
> > +	pm_wakeup_event(adapter->dev, 0);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> >  {
> > +	int ret;
> >  	struct device *dev = adapter->dev;
> >  
> >  	if (!dev->of_node)
> >  		return;
> >  
> >  	adapter->dt_node = dev->of_node;
> > +	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> > +	if (!adapter->irq_wakeup) {
> > +		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> > +		return;
> > +	}
> 
> I'd rather we used of_irq_get() here, because ti allows handling
> deferrals. of_irq_get_byname() would be even better, but I guess we
> already have bindings in the wild...

This function doesn't handle errors very gracefully at all; we just try,
and if we fail, we just skip the rest...

This could be an argument for rewriting the error handling to stop just
returning -1 in mwifiex_add_card() and use real Linux error codes.
Perhaps that can be a later cleanup?

> > +
> > +	ret = devm_request_irq(dev, adapter->irq_wakeup,
> > +			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> 
> irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
> why do we override them?
> 
> > +			       "wifi_wake", adapter);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> > +			adapter->irq_wakeup, ret);
> > +		goto err_exit;
> > +	}
> > +
> > +	disable_irq(adapter->irq_wakeup);
> > +	if (device_init_wakeup(dev, true)) {
> > +		dev_err(dev, "fail to init wakeup for mwifiex\n");
> > +		goto err_exit;
> 
> Leaking interrupt (not forever, but if we are not using wakeup irq there
> is no need to have it claimed).
> 
> > +	}
> > +	return;
> > +
> > +err_exit:
> > +	adapter->irq_wakeup = 0;
> >  }
> 
> I also do not see anyone actually calling mwifiex_probe_of() in this
> patch?

It was added to mwifiex_add_card() in the previous patch, so all users
with a proper ->dt_node would call it.

> >  
> >  /*

Brian