From mboxrd@z Thu Jan  1 00:00:00 1970
From: Oliver Neukum <oneukum@suse.com>
Subject: Re: [PATCH 02/19] usbnet: smsc95xx: Stop propagation of in_pm
Date: Mon, 07 Jan 2019 12:02:50 +0100
Message-ID: <1546858970.3037.21.camel@suse.com>
References: <20190103011040.25974-1-marex@denx.de>
         <20190103011040.25974-3-marex@denx.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Cc: "David S . Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Nisar Sayed <Nisar.Sayed@microchip.com>,
        Woojung Huh <Woojung.Huh@microchip.com>,
        linux-usb@vger.kernel.org
To: Marek Vasut <marex@denx.de>, netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mx2.suse.de ([195.135.220.15]:42690 "EHLO mx1.suse.de"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1726853AbfAGLCz (ORCPT <rfc822;netdev@vger.kernel.org>);
        Mon, 7 Jan 2019 06:02:55 -0500
In-Reply-To: <20190103011040.25974-3-marex@denx.de>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Do, 2019-01-03 at 02:10 +0100, Marek Vasut wrote:
> The information whether the SMSC95xx is in_pm or not can be derived from
> the usbdev->suspend_count. First thing called in smsc95xx_suspend() is
> usbnet_suspend(), which increments the usbdev->suspend_count and since
> then the driver only calls _nopm() functions and functions with in_pm
> set to 1. The smsc95xx_resume() does the inverse, it calls functions
> with _nopm() suffix and with in_pm set to 1 until usbnet_resume(), which
> sets the usbdev->suspend_count to 0.

Hi,

unfortunately I am forced to disagree in the strongest terms.

The logic behind this patch is wrong. The "in_pm" parameter
exists because some function need to be called in the code paths
needed to implement power management. Under those circumstances
they must not take a pm reference to keep the device awake, lest
the driver deadlock.
(For example __smsc95xx_read_reg)

However from other code paths precisely that reference must be taken
in order to make sure that the driver do not try to communicate with
a suspended device.
If you check the suspend counter, you will omit the necessary getting
of a reference precisely when it is needed. The driver will never
dedalock, but you will cause numerous races.

I must suggest to simply drop this part and redo the series.

	Regards
		Oliver