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]
Date:	Fri, 7 Aug 2015 13:56:22 +0200
From:	Robert Richter <robert.richter@...iumnetworks.com>
To:	Tomasz Nowicki <tomasz.nowicki@...aro.org>
CC:	David Daney <ddaney.cavm@...il.com>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	<linux-mips@...ux-mips.org>, Sunil Goutham <sgoutham@...ium.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-acpi@...r.kernel.org>,
	"David Daney" <david.daney@...ium.com>
Subject: Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

On 07.08.15 12:52:41, Tomasz Nowicki wrote:
> On 07.08.2015 12:43, Robert Richter wrote:
> >On 07.08.15 10:09:04, Tomasz Nowicki wrote:
> >>On 07.08.2015 02:33, David Daney wrote:
> >
> >...
> >
> >>>+#else
> >>>+
> >>>+static int bgx_init_acpi_phy(struct bgx *bgx)
> >>>+{
> >>>+	return -ENODEV;
> >>>+}
> >>>+
> >>>+#endif /* CONFIG_ACPI */
> >>>+
> >>>  #if IS_ENABLED(CONFIG_OF_MDIO)
> >>>
> >>>  static int bgx_init_of_phy(struct bgx *bgx)
> >>>@@ -882,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
> >>>
> >>>  static int bgx_init_phy(struct bgx *bgx)
> >>>  {
> >>>-	return bgx_init_of_phy(bgx);
> >>>+	int err = bgx_init_of_phy(bgx);
> >>>+
> >>>+	if (err != -ENODEV)
> >>>+		return err;
> >>>+
> >>>+	return bgx_init_acpi_phy(bgx);
> >>>  }
> >>>
> >>
> >>If kernel can work with DT and ACPI (both compiled in), it should take only
> >>one path instead of probing DT and ACPI sequentially. How about:
> >>
> >>@@ -902,10 +925,9 @@ static int bgx_probe(struct pci_dev *pdev, const struct
> >>pci_device_id *ent)
> >>  	bgx_vnic[bgx->bgx_id] = bgx;
> >>  	bgx_get_qlm_mode(bgx);
> >>
> >>-	snprintf(bgx_sel, 5, "bgx%d", bgx->bgx_id);
> >>-	np = of_find_node_by_name(NULL, bgx_sel);
> >>-	if (np)
> >>-		bgx_init_of(bgx, np);
> >>+	err = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
> >>+	if (err)
> >>+		goto err_enable;
> >>
> >>  	bgx_init_hw(bgx);
> >
> >I would not pollute bgx_probe() with acpi and dt specifics, and instead
> >keep bgx_init_phy(). The typical design pattern for this is:
> >
> >static int bgx_init_phy(struct bgx *bgx)
> >{
> >#ifdef CONFIG_ACPI
> >         if (!acpi_disabled)
> >                 return bgx_init_acpi_phy(bgx);
> >#endif
> >         return bgx_init_of_phy(bgx);
> >}
> >
> >This adds acpi runtime detection (acpi=no), does not call dt code in
> >case of acpi, and saves the #else for bgx_init_acpi_phy().
> >
> 
> I am fine with keeping it in bgx_init_phy(), however we can drop there
> #ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub for !ACPI
> and !OF case. Like that:
> 
> static int bgx_init_phy(struct bgx *bgx)
> {
> 
>         if (!acpi_disabled)
>                 return bgx_init_acpi_phy(bgx);
> 	else
>         	return bgx_init_of_phy(bgx);
> }

As said, keeping it in #ifdefs makes the empty stub function for !acpi
obsolete, which makes the code smaller and better readable. This style
is common practice in the kernel. Apart from that, the 'else' should
be dropped as it is useless.

-Robert
--
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

Powered by blists - more mailing lists