[PATCH V5 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Enrico Mioso]

Hi Oliver!
First of all - thank you very much for your time and review, and let's not 
forget your very useful and general cdc_wdm driver! :)
Please be patient with me - I'm a newbie and some things are not clear to me 
right now.


On Mon, 30 Sep 2013, Oliver Neukum wrote:

==Date: Mon, 30 Sep 2013 11:07:53 +0200
==From: Oliver Neukum <oneukum at suse.de>
==To: Enrico Mioso <mrkiko.rs at gmail.com>
==Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>,
==    David S. Miller <davem at davemloft.net>,
==    Steve Glendinning <steve.glendinning at shawell.net>,
==    Robert de Vries <rhdv at xs4all.nl>, Hayes Wang <hayeswang at realtek.com>,
==    Freddy Xin <freddy at asix.com.tw>, Bj?rn Mork <bjorn at mork.no>,
==    Liu Junliang <liujunliang_ljl at 163.com>,
==    open list <linux-kernel at vger.kernel.org>,
==    "open list:USB NETWORKING DR..." <linux-usb at vger.kernel.org>,
==    "open list:NETWORKING DRIVERS" <netdev at vger.kernel.org>,
==    ModemManager-devel at lists.freedesktop.org
==Subject: Re: [PATCH V5 net-next 2/3] net: huawei_cdc_ncm: Introduce the
==    huawei_cdc_ncm driver
==
==On Mon, 2013-09-30 at 04:50 +0000, Enrico Mioso wrote:
==
==> +static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
==> +{
==> +	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
==> +	int rv = 0;
==> +
==> +	if ((on && atomic_add_return(1, &drvstate->pmcount) == 1) ||
==> +			(!on && atomic_dec_and_test(&drvstate->pmcount))) {
==> +		rv = usb_autopm_get_interface(usbnet_dev->intf);
==> +		if (rv < 0)
==> +			goto err;
==
==The error case corrupts drvstate->pmcount

The problem here is that, when I get there in the code, I already changed the 
value of pmcount, right?
So - should I backup and restore the vlaue of the pmcount variable if any error 
occur?

==
==> +		usbnet_dev->intf->needs_remote_wakeup = on;
==> +		usb_autopm_put_interface(usbnet_dev->intf);
==> +	}
==> +err:
==> +	return rv;
==> +}
==
==
==> +static int huawei_cdc_ncm_suspend(struct usb_interface *intf, pm_message_t message)
==> +{
==> +	int ret = 0;
==> +	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
==> +	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
==> +	struct cdc_ncm_ctx *ctx = drvstate->ctx;
==> +
==> +	if (ctx == NULL) {
==> +		ret = -1;
==
==That is not a valid way to indicate an error.
==
Because we do not propagate info on what happened if the device vanished?
What might I do to improve the situation here? Looking right now I don't have 
any idea...

==> +		goto error;
==> +	}
==> +
==> +	ret = usbnet_suspend(intf, message);
==> +	if (ret < 0)
==> +		goto error;
==> +
==> +	if (intf == ctx->control &&
==> +		drvstate->subdriver &&
==> +		drvstate->subdriver->suspend)
==> +		ret = drvstate->subdriver->suspend(intf, message);
==> +	if (ret < 0)
==> +		usbnet_resume(intf);
==> +
==> +error:
==> +	return ret;
==> +}
==> +
==> +static int huawei_cdc_ncm_resume(struct usb_interface *intf)
==> +{
==> +	int ret = 0;
==> +	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
==> +	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
==> +	bool callsub;
==> +	struct cdc_ncm_ctx *ctx = drvstate->ctx;
==> +
==> +	/* should we call subdriver's resume function? */
==> +	callsub =
==> +		(intf == ctx->control &&
==> +		drvstate->subdriver &&
==> +		drvstate->subdriver->resume);
==> +
==> +	if (callsub)
==> +		ret = drvstate->subdriver->resume(intf);
==> +	if (ret < 0)
==> +		goto err;
==> +	ret = usbnet_resume(intf);
==> +	if (ret < 0 && callsub && drvstate->subdriver->suspend)
==
==You really want drivers with a resume() but no suspend() method?

You mean we could get rid of all this "callsub" thing, right?

==
==> +		drvstate->subdriver->suspend(intf, PMSG_SUSPEND);
==> +err:
==> +	return ret;
==> +}
==
==	Regards
==		Oliver
==
==
==

thank you again!
Regards - Enrico