From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8D7CDC4332F for ; Mon, 5 Dec 2022 18:07:50 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 72767853D3; Mon, 5 Dec 2022 19:07:41 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1670263661; bh=H5HjYKBuXoKo5boYnW1F82yg24QDvgTt+8qsY7ayWao=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=IRAboyxnZZN6B67TckhQGnTaIzdeljPyZQwWF7ChLyI7Kjn20zPQdeezXZN8NUAa8 1bvTePrell7wsHWRhsH5xZ2S0eCFQWYlM4SlCOqblnbkA24CQ+56WoJG27zu4KU5XX 7zyylRbBtNXC0zv/hNNkgPLRdQVQRC1BpWOe4wewwxmSvTyjv3WWx++sJ/1ef/yH5y ei5S1tC7k9WAiBtmzk8i20j/WX2+rx/JGDSTSUD6CQcRwJu7wBRFWMti47Qr6dqi4J D2IKqEGXVtTQs0FgOq4O0RR1rG9v7eDDmlyi5Tb+sKMFzMZf4yh/+93krO1SiQgWv/ Q7e/RKWLfl43g== Received: from [127.0.0.1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id CDE0C851B0; Mon, 5 Dec 2022 19:07:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1670263656; bh=H5HjYKBuXoKo5boYnW1F82yg24QDvgTt+8qsY7ayWao=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=eb4zIIjlqjrkZ3sXA5JdG5353XWGlDkaJ7TOtSHTIgc6KKWDdsoBXbKfyks6Q1Gy3 aWUKhXFf5cOJ7istFvx1d/QOQywkRjZr5Sx2a3bo8FhGd8odHlyMgZB+v/qoibroH9 f7vTEkf+orSlCi6wPkkYZHQio6CGdSYQIUSxJOlPHRkEQo8vPXp7RSn1obYT6IARFv cRekwNOU131BEcL6MhVUWZ4BmXB71/AWkuzYKPdI1M6oJYfjYS/zuIcuRmc7jZTeld lr4AqmHI65e3FlU3ibQNMzD+AgeyVCS7b5rz92/O0Y+nQpyFwHc/y0FEUUA3fXQ1qw IcP3K52cLbwdg== Message-ID: Date: Mon, 5 Dec 2022 17:20:19 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH] net: eth-uclass: change state before stop() in eth_halt() Content-Language: en-US To: Niel Fourie , Lukasz Majewski Cc: u-boot@lists.denx.de, Ramon Fried References: <20221130164225.766877-1-lusus@denx.de> <20221201092414.765944d9@wsk> <0bb976c3-0bc3-09fb-6afa-1643efe49d8e@denx.de> <222cd955-aad9-866a-75ba-73106c7091a0@denx.de> From: Marek Vasut In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 12/5/22 16:33, Niel Fourie wrote: > Hi Marek, Hi, > On 05/12/2022 13:06, Niel Fourie wrote: > [...] > >>> It does however show that this patch introduces a bug -- this patch >>> changes the order in which priv->state = ETH_STATE_PASSIVE; is >>> assigned from _after_ the ->stop callback to _before_ the -> stop >>> callback. This breaks drivers/net/ldpaa_eth/ldpaa_eth.c which checks >>> the priv->state in its ->stop callback, either on its own in non-DM >>> case, or in eth_is_active() implementation in DM case. With this >>> patch, the interface would never be stopped in the ->stop callback, >>> because the condition (net_dev->state == ETH_STATE_PASSIVE) test in >>> the ldpaa stop callback implementation would always be true. >>> >> >> In drivers/net/ldpaa_eth/ldpaa_eth.c:ldpaa_eth_stop(), priv is of type >> struct ldpaa_eth_priv*, defined in drivers/net/ldpaa_eth/ldpaa_eth.h >> and is accessed using dev_get_priv(). >> >> In net/eth-uclass.c:eht_halt(), priv is of type struct >> eth_device_priv* and defined in the same .c file, and is accessed >> using dev_get_uclass_priv(). As the structure is local to this file, >> nothing outside of this file should have any knowledge of its >> contents, and changing of the order of the calls should only impact >> this file. >> >> I sincerely hope that these two are not interfering with each other, >> otherwise we have much bigger problems... >> > > Shucks, I was thrown off by the the fact that net_dev is of type struct > eth_device, and its member state is separate from struct > eth_device_state and its member state, that I missed the implication of > eth_is_active() *setting* the value of struct eth_device_priv's state > not *reading* it. > > Well spotted, you are correct. The patch in its current form would > introduce that bug. Thank you for finding that. Good, so we agree this patch introduces a bug. > Adding back the call to dev_get_uclass_priv() to get priv and validating > it again *after* stop() as it was done before commit fa795f45254 ("net: > eth-uclass: avoid running start() twice without stop()") would fix this, > and perhaps also make the issue with stop() and Ethernet gadget more > obvious. A comment on why it needs to be repeated would also be useful. > Would this be an acceptable improvement? > > I agree that fixing the USB ethernet gadget is still the best solution, > but until that happens, we could at least limit everyone's pain. See my reply to the previous email. Keep the usb gadget device around, that should not be hard to implement and that should fix this problem once and for all, and for the future too.