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] [day] [month] [year] [list]
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