| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Apr 2016 11:45:45 -0400
From: "David Rivshin (Allworx)" <drivshin.allworx@...il.com>
To: Grygorii Strashko <grygorii.strashko@...com>,
Rob Herring <robh+dt@...nel.org>
Cc: <netdev@...r.kernel.org>, <linux-omap@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Mugunthan V N <mugunthanvnm@...com>,
Andrew Goodbody <andrew.goodbody@...brionix.com>,
Markus Brunner <systemprogrammierung.brunner@...il.com>,
Nicolas Chauvet <kwizart@...il.com>
Subject: Re: [PATCH net v2 2/3] drivers: net: cpsw: fix error messages when
using phy-handle DT property
On Fri, 22 Apr 2016 16:03:34 +0300
Grygorii Strashko <grygorii.strashko@...com> wrote:
> On 04/21/2016 09:26 PM, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivshin@...worx.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@...worx.com>
> > Acked-by: Rob Herring <robh@...nel.org>
> > Tested-by: Nicolas Chauvet <kwizart@...il.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;
> >
>
>
Powered by blists - more mailing lists