| 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
| ||
|
Message-ID: <DM2PR03MB50969DA7ABC7D4641BCF1C7E6EE0@DM2PR03MB509.namprd03.prod.outlook.com>
Date: Tue, 15 Dec 2015 11:55:57 +0000
From: Liberman Igal <Igal.Liberman@...escale.com>
To: Andy Fleming <afleming@...il.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-Cristian Bucur <madalin.bucur@...escale.com>,
"pebolle@...cali.nl" <pebolle@...cali.nl>,
Joakim Tjernlund <joakim.tjernlund@...nsmode.se>,
"ppc@...dchasers.com" <ppc@...dchasers.com>,
"stephen@...workplumber.org" <stephen@...workplumber.org>,
David Miller <davem@...emloft.net>
Subject: RE: [v9, 6/6] fsl/fman: Add FMan MAC driver
Hello Andy,
Thank you for your feedback. Some inline answers.
Regards,
Igal Liberman
> -----Original Message-----
> From: Andy Fleming [mailto:afleming@...il.com]
> Sent: Tuesday, December 08, 2015 10:18 PM
> To: Liberman Igal-B31950 <Igal.Liberman@...escale.com>
> Cc: netdev@...r.kernel.org; linuxppc-dev@...ts.ozlabs.org; linux-
> kernel@...r.kernel.org; Wood Scott-B07421 <scottwood@...escale.com>;
> Bucur Madalin-Cristian-B32716 <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.
>
> [...]
>
I removed MODULE_AUTHOR as per Scott's request.
We'll change the way we note contributors.
> > +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 *
>
OK, removed.
> [...]
>
> > +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?
>
Agree, changed the driver to use priv->phy_if instead of strstr in both places.
In both
>
> > + 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)
>
According to https://www.kernel.org/doc/Documentation/CodingStyle:
The preferred style for long (multi-line) comments is:
/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/
For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.
/* The preferred comment style for files in net/ and drivers/net
* looks like this.
*
* It is nearly the same as the generally preferred comment style,
* but there is no initial almost-blank line.
*/
> Andy
Powered by blists - more mailing lists