Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965243AbcDYVzt (ORCPT ); Mon, 25 Apr 2016 17:55:49 -0400 Received: from mail-qk0-f193.google.com ([209.85.220.193]:34944 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965184AbcDYVzr (ORCPT ); Mon, 25 Apr 2016 17:55:47 -0400 Date: Mon, 25 Apr 2016 17:55:42 -0400 From: "David Rivshin (Allworx)" To: Grygorii Strashko , Mugunthan V N Cc: Rob Herring , , , , , , David Miller , Andrew Goodbody , Markus Brunner , Nicolas Chauvet Subject: Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property Message-ID: <20160425175542.3654cc33.drivshin.allworx@gmail.com> In-Reply-To: <571E6C14.8060007@ti.com> References: <1461261035-5578-1-git-send-email-drivshin.allworx@gmail.com> <1461263172-10428-1-git-send-email-drivshin.allworx@gmail.com> <571A2126.2060708@ti.com> <20160422114545.57b477ad.drivshin.allworx@gmail.com> <571E6C14.8060007@ti.com> Organization: Allworx X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9097 Lines: 227 On Mon, 25 Apr 2016 22:12:20 +0300 Grygorii Strashko wrote: > On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote: > > On Fri, 22 Apr 2016 16:03:34 +0300 > > Grygorii Strashko wrote: > > > >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote: > >>> From: David Rivshin > >>> > >>> The phy-handle, phy_id, and fixed-link properties are mutually exclusive, > >>> and only one need be specified. However if phy-handle was specified, an > >>> error message would complain about the lack of phy_id or fixed-link. > > I think, commit message need to be updated. > You not only fix log messages - you also fix the issue with > of_get_phy_mode(slave_node); which will not be called if phy-handle is used. You are correct, and that is probably the more important fix compared to the error messages. Because the content is becoming less coherent, what I may do is split this patch into 3 small patches: A) devicetree binding documentation changes B) cpsw_probe_dt changes, with the fixes for of_get_phy_mode() and related error message C) cpsw_slave_open changes, with the fixes for crash if of_phy_connect returns NULL, and related error message Does that sound reasonable? > > > slave_data->phy_if = of_get_phy_mode(slave_node); > ^ see below > >>> > >>> Also, if phy-handle was specified and the subsequent of_phy_connect() > >>> failed, the error message still referenced slaved->data->phy_id, which > >>> would be empty. Instead, use the name of the device_node as a useful > >>> identifier. > >>> > >>> Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing") > >>> Signed-off-by: David Rivshin > >>> Acked-by: Rob Herring > >>> Tested-by: Nicolas Chauvet > >>> --- > >>> If would like this for -stable it should apply cleanly as far back > >>> as 4.5. It failes on 4.4 due to some context differences, but can be > >>> applied with 'git am -C2'. Or, I can produce a separate patch against > >>> linux-4.4.y if preferred. > >>> > >>> Changes since v1 [1]: > >>> - Rebased (no conflicts) > >>> - Added Tested-by from Nicolas Chauvet > >>> - Added Acked-by from Rob Herring for the binding change > >>> > >>> [1] https://patchwork.ozlabs.org/patch/560324/ > >>> > >>> Documentation/devicetree/bindings/net/cpsw.txt | 4 ++-- > >>> drivers/net/ethernet/ti/cpsw.c | 17 +++++++++++++---- > >>> 2 files changed, 15 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt > >>> index 28a4781..3033c0f 100644 > >>> --- a/Documentation/devicetree/bindings/net/cpsw.txt > >>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt > >>> @@ -46,16 +46,16 @@ Optional properties: > >>> - dual_emac_res_vlan : Specifies VID to be used to segregate the ports > >>> - mac-address : See ethernet.txt file in the same directory > >>> - phy_id : Specifies slave phy id > >> > >> May be the "phy_id" can be marked as deprecated? (while here) > >> The recommended property now is "phy-handle". > > > > I can certainly do that. Perhaps something like this? > > - phy_id : Specifies slave phy id (deprecated, use phy-handle) > > > > Rob, would you have any issues with bundling that? > > > >> > >>> - phy-handle : See ethernet.txt file in the same directory > >>> > >>> Slave sub-nodes: > >>> - fixed-link : See fixed-link.txt file in the same directory > >>> - Either the property phy_id, or the sub-node > >>> - fixed-link can be specified > >>> + > >>> +Note: Exactly one of phy_id, phy-handle, or fixed-link must be specified. > >>> > >>> Note: "ti,hwmods" field is used to fetch the base address and irq > >>> resources from TI, omap hwmod data base during device registration. > >>> Future plan is to migrate hwmod data base contents into device tree > >>> blob so that, all the required data will be used from device tree dts > >>> file. > >>> > >>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > >>> index d69cb3f..3c81413 100644 > >>> --- a/drivers/net/ethernet/ti/cpsw.c > >>> +++ b/drivers/net/ethernet/ti/cpsw.c > >>> @@ -1150,16 +1150,19 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv) > >>> if (slave->data->phy_node) > >>> slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node, > >>> &cpsw_adjust_link, 0, slave->data->phy_if); > >>> else > >>> slave->phy = phy_connect(priv->ndev, slave->data->phy_id, > >>> &cpsw_adjust_link, slave->data->phy_if); > >>> if (IS_ERR(slave->phy)) { > >>> - dev_err(priv->dev, "phy %s not found on slave %d\n", > >>> - slave->data->phy_id, slave->slave_num); > >>> + dev_err(priv->dev, "phy \"%s\" not found on slave %d\n", > >>> + slave->data->phy_node ? > >>> + slave->data->phy_node->full_name : > >>> + slave->data->phy_id, > >>> + slave->slave_num); > >> > >> Unfortunately, there are some inconsistency between legacy and FDT API :( > >> of_phy_connect() will return valid phy_device or NULL, but phy_connect() > >> can return valid phy_device or ERR_PTR(). > > > > Good catch, I hadn't noticed that. It looks like that's actually a more > > serious (pre-existing) bug: if of_phy_connect() returns NULL, we'd end > > up dereferencing it and pagefaulting. > > > > How about moving the IS_ERR() check into the phy_connect() case like this: > > if (slave->data->phy_node) { > > slave->phy = of_phy_connect(priv->ndev, slave->data->phy_node, > > &cpsw_adjust_link, 0, slave->data->phy_if); > > [1] > > > } else { > > slave->phy = phy_connect(priv->ndev, slave->data->phy_id, > > &cpsw_adjust_link, slave->data->phy_if); > > if (IS_ERR(slave->phy)) > > slave->phy = NULL; > [2] > > } > > if (!slave->phy) { > > dev_err(priv->dev, "phy \"%s\" not found on slave %d\n", > > slave->data->phy_node ? > > slave->data->phy_node->full_name : > > slave->data->phy_id, > > slave->slave_num); > > } else { > > > > Since you say the phy_id case is deprecated anyways, I'm not too concerned > > about not printing the error code returned by phy_connect() in that case > > (especially since it never did so in the past). That lets us still avoid > > duplicating the dev_err() itself. > > I'm not worry too much about duplicating dev_err() - it's always good to know > the reason of failure. > > So, may be for of_phy_connect() [1]: > dev_err(priv->dev, "phy \"%s\" not found on slave %d\n", > slave->data->phy_node->full_name, > slave->slave_num); > > and for phy_connect() [2]: > dev_err(priv->dev, "phy %s not found on slave %d, err %d\n", > slave->data->phy_id, slave->slave_num, PTR_ERR(slave->phy)); > > Mugunthan, any comments? If that's the preference, then I can incorporate that into V3. > > > > > > >> > >> > >> > >>> slave->phy = NULL; > >>> } else { > >>> phy_attached_info(slave->phy); > >>> > >>> phy_start(slave->phy); > >>> > >>> /* Configure GMII_SEL register */ > >>> @@ -2030,15 +2033,19 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data, > >>> /* This is no slave child node, continue */ > >>> if (strcmp(slave_node->name, "slave")) > >>> continue; > >>> > >>> slave_data->phy_node = of_parse_phandle(slave_node, > >>> "phy-handle", 0); > >>> parp = of_get_property(slave_node, "phy_id", &lenp); > >>> - if (of_phy_is_fixed_link(slave_node)) { > >>> + if (slave_data->phy_node) { > >>> + dev_dbg(&pdev->dev, > >>> + "slave[%d] using phy-handle=\"%s\"\n", > >>> + i, slave_data->phy_node->full_name); > >>> + } else if (of_phy_is_fixed_link(slave_node)) { > >>> struct device_node *phy_node; > >>> struct phy_device *phy_dev; > >>> > >>> /* In the case of a fixed PHY, the DT node associated > >>> * to the PHY is the Ethernet MAC DT node. > >>> */ > >>> ret = of_phy_register_fixed_link(slave_node); > >>> @@ -2067,15 +2074,17 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data, > >>> if (!mdio) { > >>> dev_err(&pdev->dev, "Missing mdio platform device\n"); > >>> return -EINVAL; > >>> } > >>> snprintf(slave_data->phy_id, sizeof(slave_data->phy_id), > >>> PHY_ID_FMT, mdio->name, phyid); > >>> } else { > >>> - dev_err(&pdev->dev, "No slave[%d] phy_id or fixed-link property\n", i); > >>> + dev_err(&pdev->dev, > >>> + "No slave[%d] phy_id, phy-handle, or fixed-link property\n", > >>> + i); > >>> goto no_phy_slave; > >>> } > >>> slave_data->phy_if = of_get_phy_mode(slave_node); > > Your change will allow the code to reach this point in case of phy-handle. > > >>> if (slave_data->phy_if < 0) { > >>> dev_err(&pdev->dev, "Missing or malformed slave[%d] phy-mode property\n", > >>> i); > >>> return slave_data->phy_if; > >>> > >> > >> > >