LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dave Hansen <haveblue@us.ibm.com>
To: Jan-Bernd Themann <ossthema@de.ibm.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	linux-ppc <linuxppc-dev@ozlabs.org>,
	Thomas Klein <osstklei@de.ibm.com>,
	"Themann, Jan-Bernd" <themann@de.ibm.com>,
	Greg KH <greg@kroah.com>, Christoph Raisch <RAISCH@de.ibm.com>,
	Thomas Klein <tklein@de.ibm.com>, apw <apw@uk.ibm.com>,
	Badari Pulavarty <pbadari@us.ibm.com>
Subject: Re: [PATCH] drivers/base: export gpl (un)register_memory_notifier
Date: Mon, 11 Feb 2008 08:47:09 -0800	[thread overview]
Message-ID: <1202748429.8276.21.camel@nimitz.home.sr71.net> (raw)
In-Reply-To: <200802111724.12416.ossthema@de.ibm.com>

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


  reply	other threads:[~2008-02-11 16:47 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-11 16:24 Jan-Bernd Themann
2008-02-11 16:47 ` Dave Hansen [this message]
2008-02-13 15:17   ` Jan-Bernd Themann
2008-02-13 17:05     ` Dave Hansen
2008-02-14  8:46     ` Christoph Raisch
2008-02-14 17:12       ` Dave Hansen
2008-02-14 17:36         ` Badari Pulavarty
2008-02-14 17:38           ` Dave Hansen
2008-02-15 13:22         ` Christoph Raisch
2008-02-15 16:55           ` Dave Hansen
2008-02-18 10:00             ` Jan-Bernd Themann
2008-02-20 18:14               ` Dave Hansen
2008-02-11 16:50 ` Dave Hansen
2008-02-12 18:04 ` Dave Hansen
  -- strict thread matches above, loose matches on Subject: below --
2008-02-11 15:57 Jan-Bernd Themann
2008-02-11 16:02 ` Sam Ravnborg
2008-02-11 16:04 ` Dave Hansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1202748429.8276.21.camel@nimitz.home.sr71.net \
    --to=haveblue@us.ibm.com \
    --cc=RAISCH@de.ibm.com \
    --cc=apw@uk.ibm.com \
    --cc=greg@kroah.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=netdev@vger.kernel.org \
    --cc=ossthema@de.ibm.com \
    --cc=osstklei@de.ibm.com \
    --cc=pbadari@us.ibm.com \
    --cc=themann@de.ibm.com \
    --cc=tklein@de.ibm.com \
    --subject='Re: [PATCH] drivers/base: export gpl (un)register_memory_notifier' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).