Subject: Re: [PATCH 16/44 take 2] [UBI] scanning unit implementation
From: Artem Bityutskiy <dedekind@infradead.org>
Reply-To: dedekind@infradead.org
To: Christoph Hellwig <hch@infradead.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
       Frank Haverkamp <haver@vnet.ibm.com>,
       Thomas Gleixner <tglx@linutronix.de>,
       David Woodhouse <dwmw2@infradead.org>,
       Josh Boyer <jwboyer@linux.vnet.ibm.com>
In-Reply-To: <20070219110543.GE16930@infradead.org>
References: <20070217165424.5845.4390.sendpatchset@localhost.localdomain>
	 <20070217165545.5845.27214.sendpatchset@localhost.localdomain>
	 <20070219110543.GE16930@infradead.org>
Content-Type: text/plain; charset=utf-8
Date: Mon, 19 Feb 2007 16:11:14 +0200
Message-Id: <1171894274.14817.9.camel@sauron>
Mime-Version: 1.0
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1656
Lines: 49

On Mon, 2007-02-19 at 11:05 +0000, Christoph Hellwig wrote:
> > +	cond_resched();
> > +	list_for_each_entry(seb, &si->erase, u.list)
> > +		if (seb->ec == NAND_SCAN_UNKNOWN_EC)
> > +			seb->ec = si->mean_ec;
> 
> You really shouldn't need random cond_resched all over the place.

Good point. I will review the code and clean up stuff like this.

While we are on the point, could you please formulate your criteria of
when cond_resched() should be used.

> > +static int compare_lebs(const struct ubi_info *ubi,
> > +			const struct ubi_scan_leb *seb, int pnum,
> > +			const struct ubi_vid_hdr *vid_hdr);
> 
> forward declarations in the middle of the file are really annoying.  Please try
> to reorder the code to not need them at all if needed, and if needed add them
> to the top of the file.

I tried to layout the code like: top level functions first, low level
last. This is matter of taste, isn't it? I am reluctant to change this,
because it is big useless work (I'll need to change it everywhere in
UBI). But will do this with great delight if it annoys too many people
(you are the second so far).

> > +	list_for_each_entry_safe(seb, seb_tmp, &si->alien, u.list) {
> > +		list_del(&seb->u.list);
> > +		ubi_free_scan_leb(seb);
> > +	}
> 
> no locking needed here?

No, initialization stage does not need any locking.


Thanks,
Artem.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

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