* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-22 13:03 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-22 13:03 UTC (permalink / raw)
To: David Rivshin (Allworx), netdev, linux-omap, Rob Herring
Cc: Markus Brunner, devicetree, Mugunthan V N, Nicolas Chauvet,
linux-kernel, Andrew Goodbody, David Miller, linux-arm-kernel
On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
>
> 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.
>
> 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@allworx.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> 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".
> - 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().
> 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);
> 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;
>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-22 13:03 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-22 13:03 UTC (permalink / raw)
To: David Rivshin (Allworx), netdev, linux-omap, Rob Herring
Cc: linux-arm-kernel, devicetree, linux-kernel, David Miller,
Mugunthan V N, Andrew Goodbody, Markus Brunner, Nicolas Chauvet
On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
>
> 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.
>
> 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@allworx.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> 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".
> - 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().
> 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);
> 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;
>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-22 13:03 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-22 13:03 UTC (permalink / raw)
To: linux-arm-kernel
On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
>
> 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.
>
> 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@allworx.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
> 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".
> - 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().
> 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);
> 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;
>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
2016-04-22 13:03 ` Grygorii Strashko
(?)
@ 2016-04-22 15:45 ` David Rivshin (Allworx)
-1 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-22 15:45 UTC (permalink / raw)
To: Grygorii Strashko, Rob Herring
Cc: netdev, linux-omap, linux-arm-kernel, devicetree, linux-kernel,
David Miller, Mugunthan V N, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivshin@allworx.com>
> >
> > 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.
> >
> > 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@allworx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > 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);
} 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;
}
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.
>
>
>
> > 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);
> > 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;
> >
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-22 15:45 ` David Rivshin (Allworx)
0 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-22 15:45 UTC (permalink / raw)
To: Grygorii Strashko, Rob Herring
Cc: netdev, linux-omap, linux-arm-kernel, devicetree, linux-kernel,
David Miller, Mugunthan V N, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivshin@allworx.com>
> >
> > 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.
> >
> > 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@allworx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > 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);
} 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;
}
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.
>
>
>
> > 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);
> > 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;
> >
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-22 15:45 ` David Rivshin (Allworx)
0 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-22 15:45 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivshin@allworx.com>
> >
> > 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.
> >
> > 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@allworx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > ---
> > 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);
} 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;
}
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.
>
>
>
> > 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);
> > 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;
> >
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
2016-04-22 15:45 ` David Rivshin (Allworx)
(?)
@ 2016-04-25 19:12 ` Grygorii Strashko
-1 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-25 19:12 UTC (permalink / raw)
To: David Rivshin (Allworx), Rob Herring
Cc: netdev, linux-omap, linux-arm-kernel, devicetree, linux-kernel,
David Miller, Mugunthan V N, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>> From: David Rivshin <drivshin@allworx.com>
>>>
>>> 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.
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@allworx.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> 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?
>
>
>>
>>
>>
>>> 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;
>>>
>>
>>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-25 19:12 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-25 19:12 UTC (permalink / raw)
To: David Rivshin (Allworx), Rob Herring
Cc: netdev, linux-omap, linux-arm-kernel, devicetree, linux-kernel,
David Miller, Mugunthan V N, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>> From: David Rivshin <drivshin@allworx.com>
>>>
>>> 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.
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@allworx.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> 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?
>
>
>>
>>
>>
>>> 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;
>>>
>>
>>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-25 19:12 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-25 19:12 UTC (permalink / raw)
To: linux-arm-kernel
On 04/22/2016 06:45 PM, David Rivshin (Allworx) wrote:
> On Fri, 22 Apr 2016 16:03:34 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>
>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>> From: David Rivshin <drivshin@allworx.com>
>>>
>>> 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.
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@allworx.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> ---
>>> 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?
>
>
>>
>>
>>
>>> 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;
>>>
>>
>>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
2016-04-25 19:12 ` Grygorii Strashko
(?)
@ 2016-04-25 21:55 ` David Rivshin (Allworx)
-1 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-25 21:55 UTC (permalink / raw)
To: Grygorii Strashko, Mugunthan V N
Cc: Rob Herring, netdev, linux-omap, linux-arm-kernel, devicetree,
linux-kernel, David Miller, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
> >
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> >>> From: David Rivshin <drivshin@allworx.com>
> >>>
> >>> 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@allworx.com>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> 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;
> >>>
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-25 21:55 ` David Rivshin (Allworx)
0 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-25 21:55 UTC (permalink / raw)
To: Grygorii Strashko, Mugunthan V N
Cc: Rob Herring, netdev, linux-omap, linux-arm-kernel, devicetree,
linux-kernel, David Miller, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
> >
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> >>> From: David Rivshin <drivshin@allworx.com>
> >>>
> >>> 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@allworx.com>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> 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;
> >>>
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-25 21:55 ` David Rivshin (Allworx)
0 siblings, 0 replies; 45+ messages in thread
From: David Rivshin (Allworx) @ 2016-04-25 21:55 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 25 Apr 2016 22:12:20 +0300
Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
> >
> >> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> >>> From: David Rivshin <drivshin@allworx.com>
> >>>
> >>> 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@allworx.com>
> >>> Acked-by: Rob Herring <robh@kernel.org>
> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> >>> ---
> >>> 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;
> >>>
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
2016-04-25 21:55 ` David Rivshin (Allworx)
(?)
@ 2016-04-26 7:50 ` Grygorii Strashko
-1 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-26 7:50 UTC (permalink / raw)
To: David Rivshin (Allworx), Mugunthan V N
Cc: Rob Herring, netdev, linux-omap, linux-arm-kernel, devicetree,
linux-kernel, David Miller, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>>>> From: David Rivshin <drivshin@allworx.com>
>>>>>
>>>>> 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?
Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.
>
>>
>>
>> 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@allworx.com>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> 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.
>
Yes, please, if no other comments.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-26 7:50 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-26 7:50 UTC (permalink / raw)
To: David Rivshin (Allworx), Mugunthan V N
Cc: Rob Herring, netdev, linux-omap, linux-arm-kernel, devicetree,
linux-kernel, David Miller, Andrew Goodbody, Markus Brunner,
Nicolas Chauvet
On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>>>> From: David Rivshin <drivshin@allworx.com>
>>>>>
>>>>> 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?
Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.
>
>>
>>
>> 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@allworx.com>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> 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.
>
Yes, please, if no other comments.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property
@ 2016-04-26 7:50 ` Grygorii Strashko
0 siblings, 0 replies; 45+ messages in thread
From: Grygorii Strashko @ 2016-04-26 7:50 UTC (permalink / raw)
To: linux-arm-kernel
On 04/26/2016 12:55 AM, David Rivshin (Allworx) wrote:
> On Mon, 25 Apr 2016 22:12:20 +0300
> Grygorii Strashko <grygorii.strashko@ti.com> 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@ti.com> wrote:
>>>
>>>> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
>>>>> From: David Rivshin <drivshin@allworx.com>
>>>>>
>>>>> 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?
Sounds reasonable for me.
Hope patch 1 from this series could be merged separately.
>
>>
>>
>> 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@allworx.com>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>>>> ---
>>>>> 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.
>
Yes, please, if no other comments.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 45+ messages in thread