netdev - Re: [v9, 6/6] fsl/fman: Add FMan MAC driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKWjMd7EDx8OnYGu-QJXM++fy=W5i2E74Fe4r9zNjUV_giyUKw@mail.gmail.com>
Date:	Tue, 8 Dec 2015 14:18:16 -0600
From:	Andy Fleming <afleming@...il.com>
To:	igal.liberman@...escale.com
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Scott Wood <scottwood@...escale.com>,
	madalin.bucur@...escale.com, pebolle@...cali.nl,
	Joakim Tjernlund <joakim.tjernlund@...nsmode.se>,
	ppc@...dchasers.com, stephen@...workplumber.org,
	David Miller <davem@...emloft.net>
Subject: Re: [v9, 6/6] fsl/fman: Add FMan MAC driver

On Thu, Dec 3, 2015 at 1:19 AM,  <igal.liberman@...escale.com> wrote:
> From: Igal Liberman <igal.liberman@...escale.com>
>
> This patch adds the Ethernet MAC driver supporting the three
> different types of MACs: dTSEC, tGEC and mEMAC.
>
> Signed-off-by: Igal Liberman <igal.liberman@...escale.com>

[...]

> +
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +MODULE_AUTHOR("Emil Medve <Emilian.Medve@...escale.com>");

I imagine this email address doesn't exist anymore, or won't soon.
This is also an issue in the ethernet driver (with my old address).
I'm not sure what the right approach is, but we shouldn't be putting
obsolete email addresses in the kernel.

[...]

> +static void mac_exception(void *_mac_dev, enum fman_mac_exceptions ex)
> +{
> +       struct mac_device       *mac_dev;
> +       struct mac_priv_s       *priv;
> +
> +       mac_dev = (struct mac_device *)_mac_dev;


Don't cast a void *

[...]

> +static int mac_probe(struct platform_device *_of_dev)
> +{
> +       int                      err, i, lenp, nph;
> +       struct device           *dev;
> +       struct device_node      *mac_node, *dev_node, *tbi_node;
> +       struct mac_device       *mac_dev;
> +       struct platform_device  *of_dev;
> +       struct resource          res;
> +       struct mac_priv_s       *priv;
> +       const u8                *mac_addr;
> +       const char              *char_prop;

[...]

> +       /* Get the PHY connection type */
> +       char_prop = (const char *)of_get_property(mac_node,
> +                                               "phy-connection-type", NULL);
> +       if (!char_prop) {
> +               dev_warn(dev,
> +                        "of_get_property(%s, phy-connection-type) failed. Defaulting to MII\n",
> +                        mac_node->full_name);
> +               priv->phy_if = PHY_INTERFACE_MODE_MII;
> +       } else {
> +               priv->phy_if = str2phy(char_prop);
> +       }
> +
> +       priv->speed             = phy2speed[priv->phy_if];
> +       priv->max_speed         = priv->speed;
> +       mac_dev->if_support     = DTSEC_SUPPORTED;
> +       /* We don't support half-duplex in SGMII mode */
> +       if (strstr(char_prop, "sgmii"))

This causes a crash if the device tree does not have a
"phy-connection-type" for this node. Also, you already have parsed the
property, and put the result in priv->phy_if. Why not use this to do
all of these determinations?


> +               mac_dev->if_support &= ~(SUPPORTED_10baseT_Half |
> +                                       SUPPORTED_100baseT_Half);
> +
> +       /* Gigabit support (no half-duplex) */
> +       if (priv->max_speed == 1000)
> +               mac_dev->if_support |= SUPPORTED_1000baseT_Full;
> +
> +       /* The 10G interface only supports one mode */
> +       if (strstr(char_prop, "xgmii"))
> +               mac_dev->if_support = SUPPORTED_10000baseT_Full;

Here, too.


[...]

> +       err = mac_dev->init(mac_dev);
> +       if (err < 0) {
> +               dev_err(dev, "mac_dev->init() = %d\n", err);
> +               of_node_put(priv->phy_node);
> +               goto _return_dev_set_drvdata;
> +       }
> +
> +       /* pause frame autonegotiation enabled */
> +       mac_dev->autoneg_pause = true;
> +
> +       /* by intializing the values to false, force FMD to enable PAUSE frames
> +        * on RX and TX
> +        */

Minor comment format issue (leave blank line at top of block comment)

Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html