linux-kernel - Re: [PATCH] drivers/base: export gpl (un)register_memory

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:	Mon, 11 Feb 2008 08:47:09 -0800
From:	Dave Hansen <haveblue@...ibm.com>
To:	Jan-Bernd Themann <ossthema@...ibm.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>,
	linux-ppc <linuxppc-dev@...abs.org>,
	Thomas Klein <osstklei@...ibm.com>,
	"Themann, Jan-Bernd" <themann@...ibm.com>,
	Greg KH <greg@...ah.com>, Christoph Raisch <RAISCH@...ibm.com>,
	Thomas Klein <tklein@...ibm.com>, apw <apw@...ibm.com>,
	Badari Pulavarty <pbadari@...ibm.com>
Subject: Re: [PATCH] drivers/base: export gpl (un)register_memory_notifier

On Mon, 2008-02-11 at 17:24 +0100, Jan-Bernd Themann wrote:
>  the eHEA patch belongs to a patchset that is usually
>  added by Jeff Garzik once this dependency (EXPORTS)
>  is resolved.

I know that's already in mainline but, man, that code is nasty.  It has
stuff indented 7 levels or so and is virtually impossible to read.

This:

> int ehea_create_busmap(void)
> {
>         u64 vaddr = EHEA_BUSMAP_START;
>         unsigned long high_section_index = 0;
>         int i;
> 
>         /*
>          * Sections are not in ascending order -> Loop over all sections and
>          * find the highest PFN to compute the required map size.
>         */
>         ehea_bmap.valid_sections = 0;
> 
>         for (i = 0; i < NR_MEM_SECTIONS; i++)
>                 if (valid_section_nr(i))
>                         high_section_index = i;

is also completely bogus for arch-independent code.  If someone ever
wants to do ppc without sparsemem (or redoes internal sparsemem
interfaces), this code will break.  

Also, keeping your own mapping of all of memory is really nasty.  With
SPARSEMEM_EXTREME/VMEMMAP you can have extremely sparse physical memory,
and this:

>         ehea_bmap.entries = high_section_index + 1;
>         ehea_bmap.vaddr = vmalloc(ehea_bmap.entries * sizeof(*ehea_bmap.vaddr));

could theoretically consume all of memory if the sections are very sparse.

Could you please try to explain what the heck this driver is doing?
We'll try to fix up the generic interfaces so that you can deal with
things in a more generic fashion, but it can't stay this way.

Also, just ripping down and completely re-doing the entire mass of cards
every time a 16MB area of memory is added or removed seems like an
awfully big sledgehammer to me.  I would *HATE* to see anybody else
using this driver as an example to work off of?  Can't you just keep
track of which areas the driver is actually *USING* and only worry about
changing mappings if that intersects with an area having hotplug done on
it?

Can you please give it some CodingStyle love and make it so that it
doesn't indent so much?  Something like this:

diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
index c051c7e..72c5652 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -2638,38 +2638,40 @@ static void ehea_rereg_mrs(struct work_struct *work)
 	down(&dlpar_mem_lock);
 	ehea_info("LPAR memory enlarged - re-initializing driver");
 
-	list_for_each_entry(adapter, &adapter_list, list)
-		if (adapter->active_ports) {
-			/* Shutdown all ports */
-			for (i = 0; i < EHEA_MAX_PORTS; i++) {
-				struct ehea_port *port = adapter->port[i];
-
-				if (port) {
-					struct net_device *dev = port->netdev;
-
-					if (dev->flags & IFF_UP) {
-						down(&port->port_lock);
-						netif_stop_queue(dev);
-						ret = ehea_stop_qps(dev);
-						if (ret) {
-							up(&port->port_lock);
-							goto out;
-						}
-						port_napi_disable(port);
-						up(&port->port_lock);
-					}
-				}
-			}
-
-			/* Unregister old memory region */
-			ret = ehea_rem_mr(&adapter->mr);
+	list_for_each_entry(adapter, &adapter_list, list) {
+		if (!adapter->active_ports)
+			continue;
+		/* Shutdown all ports */
+		for (i = 0; i < EHEA_MAX_PORTS; i++) {
+			struct ehea_port *port = adapter->port[i];
+			struct net_device *dev;
+			if (!port)
+				continue;
+
+			dev = port->netdev;
+			if (!(dev->flags & IFF_UP))
+				continue;
+
+			down(&port->port_lock);
+			netif_stop_queue(dev);
+			ret = ehea_stop_qps(dev);
 			if (ret) {
-				ehea_error("unregister MR failed - driver"
-					   " inoperable!");
+				up(&port->port_lock);
 				goto out;
 			}
+			port_napi_disable(port);
+			up(&port->port_lock);
 		}
 
+		/* Unregister old memory region */
+		ret = ehea_rem_mr(&adapter->mr);
+		if (ret) {
+			ehea_error("unregister MR failed - driver"
+				   " inoperable!");
+			goto out;
+		}
+	}
+
 	ehea_destroy_busmap();
 	ret = ehea_create_busmap();
 	if (ret) {


-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/