Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

[David Rivshin (Allworx)]

On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@xxxxxx> wrote:

> On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> > On Fri, 22 Apr 2016 16:03:34 +0300
> > Grygorii Strashko <grygorii.strashko@xxxxxx> wrote:
> >   
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:  
> >>> From: David Rivshin <drivshin@xxxxxxxxxxx>
> >>>
> >>> 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 <drivshin@xxxxxxxxxxx>
> >>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> >>> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
> >>> ---
> >>> 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;
> >>>      
> >>
> >>  
> 
>