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 X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D5A98C43460 for ; Mon, 26 Apr 2021 10:54:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A34D7611CE for ; Mon, 26 Apr 2021 10:54:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233100AbhDZKzN (ORCPT ); Mon, 26 Apr 2021 06:55:13 -0400 Received: from ssl.serverraum.org ([176.9.125.105]:41077 "EHLO ssl.serverraum.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232266AbhDZKzK (ORCPT ); Mon, 26 Apr 2021 06:55:10 -0400 Received: from ssl.serverraum.org (web.serverraum.org [172.16.0.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ssl.serverraum.org (Postfix) with ESMTPSA id C353C22236; Mon, 26 Apr 2021 12:54:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=walle.cc; s=mail2016061301; t=1619434464; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Wk++B/L7nejW/Tu85IzTL8HKq5haXmWBuSRGMq5G9Vc=; b=aBLItzwTpMLhnbBUfRwBioPVMyZiTU0Pj1H6UNIlRFQlCvrRQC9+sCwg7Wn5J+XsvHZln5 Tw13ybstx1ba3ts9pwi2ty81rvOdnoEZ8zIsZqSHZ7psnqzBT46f85vFqJlTddaAXCPvqK P1t1rcsQHgB7iSTeBLTyQ5gE26gcAtQ= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 26 Apr 2021 12:54:08 +0200 From: Michael Walle To: Rob Herring Cc: Benjamin Herrenschmidt , QCA ath9k Development , Microchip Linux Driver Support , linux-arm-kernel , linux-kernel@vger.kernel.org, linuxppc-dev , netdev , "moderated list:ARM/Mediatek SoC support" , "open list:MEDIA DRIVERS FOR RENESAS - FCP" , "moderated list:ARM/STM32 ARCHITECTURE" , "open list:ARM/Amlogic Meson..." , linux-oxnas@groups.io, linux-omap , linux-wireless , devicetree@vger.kernel.org, linux-staging@lists.linux.dev, Andrew Lunn , Gregory Clement , Sebastian Hesselbarth , Russell King , Michael Ellerman , Paul Mackerras , Andreas Larsson , "David S . Miller" , Jakub Kicinski , Maxime Ripard , Chen-Yu Tsai , Jernej Skrabec , Joyce Ooi , Chris Snook , =?UTF-8?Q?Rafa=C5=82_Mi=C5=82ecki?= , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , Florian Fainelli , Nicolas Ferre , Claudiu Beznea , Sunil Goutham , Fugang Duan , Madalin Bucur , Pantelis Antoniou , Claudiu Manoil , Li Yang , Yisen Zhuang , Salil Mehta , Hauke Mehrtens , Thomas Petazzoni , Vadym Kochan , Taras Chornyi , Mirko Lindner , Stephen Hemminger , Felix Fietkau , John Crispin , Sean Wang , Mark Lee , Matthias Brugger , Bryan Whitehead , Vladimir Zapolskiy , Sergei Shtylyov , Byungho An , Kunihiko Hayashi , Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , Maxime Coquelin , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Kevin Hilman , Neil Armstrong , Jerome Brunet , Martin Blumenstingl , Vinod Koul , Nobuhiro Iwamatsu , Grygorii Strashko , Wingman Kwok , Murali Karicheri , Michal Simek , Radhey Shyam Pandey , Kalle Valo , Lorenzo Bianconi , Ryder Lee , Stanislaw Gruszka , Helmut Schaa , Heiner Kallweit , Frank Rowand , Greg Kroah-Hartman , =?UTF-8?Q?J=C3=A9r=C3=B4me_Pouiller?= , Vivien Didelot , Vladimir Oltean Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices In-Reply-To: References: <20210412174718.17382-1-michael@walle.cc> <20210412174718.17382-3-michael@walle.cc> <730d603b12e590c56770309b4df2bd668f7afbe3.camel@kernel.crashing.org> <8157eba9317609294da80472622deb28@walle.cc> User-Agent: Roundcube Webmail/1.4.11 Message-ID: <108f268a35843368466004f7fe5f9f88@walle.cc> X-Sender: michael@walle.cc Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Am 2021-04-16 17:19, schrieb Rob Herring: > On Fri, Apr 16, 2021 at 2:30 AM Michael Walle wrote: >> >> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt: >> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote: >> >> >> >> /** >> >> * of_get_phy_mode - Get phy mode for given device_node >> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, >> >> const char *name, u8 *addr) >> >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr) >> >> { >> >> struct platform_device *pdev = of_find_device_by_node(np); >> >> + struct nvmem_cell *cell; >> >> + const void *mac; >> >> + size_t len; >> >> int ret; >> >> >> >> - if (!pdev) >> >> - return -ENODEV; >> >> + /* Try lookup by device first, there might be a >> >> nvmem_cell_lookup >> >> + * associated with a given device. >> >> + */ >> >> + if (pdev) { >> >> + ret = nvmem_get_mac_address(&pdev->dev, addr); >> >> + put_device(&pdev->dev); >> >> + return ret; >> >> + } >> >> + >> > >> > This smells like the wrong band aid :) >> > >> > Any struct device can contain an OF node pointer these days. >> >> But not all nodes might have an associated device, see DSA for >> example. > > I believe what Ben is saying and what I said earlier is going from dev > -> OF node is right and OF node -> dev is wrong. If you only have an > OF node, then use an of_* function. > >> And as the name suggests of_get_mac_address() operates on a node. So >> if a driver calls of_get_mac_address() it should work on the node. >> What >> is wrong IMHO, is that the ethernet drivers where the corresponding >> board >> has a nvmem_cell_lookup registered is calling >> of_get_mac_address(node). >> It should rather call eth_get_mac_address(dev) in the first place. >> >> One would need to figure out if there is an actual device (with an >> assiciated of_node), then call eth_get_mac_address(dev) and if there >> isn't a device call of_get_mac_address(node). > > Yes, I think we're all in agreement. > >> But I don't know if that is easy to figure out. Well, one could start >> with just the device where nvmem_cell_lookup is used. Then we could >> drop the workaround above. > > Start with the ones just passing dev.of_node directly: > > $ git grep 'of_get_mac_address(.*of_node)' [..] Before I'll try to come up with a patch for this, I'd like to get your opinion on it. (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev) might sometimes lead to confusing comments like in drivers/net/ethernet/allwinner/sun4i-emac.c: /* Read MAC-address from DT */ ret = of_get_mac_address(np, ndev->dev_addr); Do we live with that or should the new name somehow reflect that it is taken from the device tree. (2) What do you think of eth_get_mac_address(ndev). That is, the second argument is missing and ndev->dev_addr is used. I'm unsure about it. We'd still need a second function for drivers which don't write ndev->dev_addr directly, but have some custom logic in between. OTOH it would be like eth_hw_addr_random(ndev). -michael