Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752779AbdHJOn2 (ORCPT ); Thu, 10 Aug 2017 10:43:28 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:45346 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752362AbdHJOn1 (ORCPT ); Thu, 10 Aug 2017 10:43:27 -0400 Subject: Re: [PATCH v2] net: stmmac: Use the right logging function in stmmac_mdio_register To: Andrew Lunn Cc: Giuseppe Cavallaro , Alexandre Torgue , netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20170810073826.24362-1-romain.perier@collabora.com> <20170810135653.GB24790@lunn.ch> From: Romain Perier Message-ID: <8e11fc8b-1385-bb0d-7a01-7ccd6b357352@collabora.com> Date: Thu, 10 Aug 2017 16:43:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170810135653.GB24790@lunn.ch> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3656 Lines: 92 Hi Andrew, Le 10/08/2017 à 15:56, Andrew Lunn a écrit : > On Thu, Aug 10, 2017 at 09:38:26AM +0200, Romain Perier wrote: >> Currently, the function stmmac_mdio_register() is only used by >> stmmac_dvr_probe() from stmmac_main.c, in order to register the MDIO bus >> and probe information about the PHY. As this function is called before >> calling register_netdev(), all messages logged from stmmac_mdio_register >> are prefixed by "(unnamed net_device)". The goal of netdev_info or >> netdev_err is to dump useful infos about a net_device, when this data >> structure is partially initialized, there is no point for using these >> functions. >> >> This commit fixes the issue by replacing all netdev_*() by the >> corresponding dev_*() function for logging. The last netdev_info is >> replaced by phy_attached_info(), as a valid phydev can be used at this >> point. >> >> Signed-off-by: Romain Perier >> --- >> >> Changes in v2: >> - Replaced dev_info() by phy_attached_print() >> - Simplified message because informations were redudant >> - Updated commit message >> >> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> index db157a47000c..e0fbf8657103 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> @@ -204,6 +204,7 @@ int stmmac_mdio_register(struct net_device *ndev) >> struct stmmac_priv *priv = netdev_priv(ndev); >> struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data; >> struct device_node *mdio_node = priv->plat->mdio_node; >> + struct device *dev = ndev->dev.parent; >> int addr, found; >> >> if (!mdio_bus_data) >> @@ -237,7 +238,7 @@ int stmmac_mdio_register(struct net_device *ndev) >> else >> err = mdiobus_register(new_bus); >> if (err != 0) { >> - netdev_err(ndev, "Cannot register the MDIO bus\n"); >> + dev_err(dev, "Cannot register the MDIO bus\n"); >> goto bus_register_fail; >> } >> >> @@ -285,14 +286,14 @@ int stmmac_mdio_register(struct net_device *ndev) >> irq_str = irq_num; >> break; >> } >> - netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n", >> - phydev->phy_id, addr, irq_str, phydev_name(phydev), >> - act ? " active" : ""); >> + phy_attached_print(phydev, "PHY ID %08x IRQ %s %s\n", >> + phydev->phy_id, irq_str, >> + act ? " active" : ""); > I don't want to go round and round with this patch, with feature creep. > > But there are only two drivers which use phy_attached_print() to print > more than the standard printed by phy_attached_info(). > phy_attached_info() will give you the driver name, so do you need the > ID? phy_attached_info() prints the irq as a number, rather than a nice > string. > > act is just odd. The loop is effectively phy_find_first(). The act > means it has found a phy, but not the phy indicated in the platform > data, but it is going to use it anyway! > > So i will give: > > Reviewed-by: Andrew Lunn > > but if you think phy_attached_info() is acceptable, that would be > great. > > Andrew Good point, initially I just wanted to propose a fix about logging and not change the message, in order to avoid to lose informations (but that's very arguable). phy_attached_print or phy_attached_info already display the model of the PHY, so I think that removing PHY ID is okay, because in this case it is completely useless. I will post a v3 with your Reviewed-by: Romain