public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Scott Wood <scottwood@freescale.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug
Date: Fri, 23 Oct 2015 15:34:14 -0500	[thread overview]
Message-ID: <1445632454.701.214.camel@freescale.com> (raw)
In-Reply-To: <562A90FD.9050907@elecsyscorp.com>

On Fri, 2015-10-23 at 19:56 +0000, Kevin Smith wrote:
> Hi Scott,
> 
> On 10/23/2015 01:20 PM, Scott Wood wrote:
> > 
> > Yuck.  Could you please rework this driver to not play games with pointers
> > and one giant allocation?  Why can't this function allocate each region it
> > needs separately?
> > 
> > -Scott
> > 
> This driver is taken from Linux.  There are a few API modifications to 
> make it work in U-Boot, but the main form and function of the driver is 
> the same.  The single allocation method is used by Linux and is kept 
> here in U-boot.

Sigh... At least do this in alloc_nand_resources() the way Linux does, rather 
than taking it a step further and allocating an array of these blobs.

> As for why Linux does this, it may be for cache coherency, avoiding
> memory fragmentation, speed (fewer calls to malloc), or something else.  
> I agree it is kind of opaque, but is probably done for a good reason.

I'm sure there was a reason but that doesn't mean it was a good reason.  I 
don't understand how cache coherency would be relevant, nor do I agree that 
trying to optimize a boot path to have fewer malloc calls at the expense of 
making the code more complicated is a "good reason".

>   I didn't port the driver, and I don't know if the reason is applicable to
> U-Boot or if a rework is appropriate.  Maybe Stefan can comment?
> 
> Either way, I am not able to rework it right now.  I think my patch 
> fixes a legitimate issue.  (It at least fixes the crashes I was 
> experiencing).  I hope it can be accepted as-is.

Does Linux have this problem?  Assuming no, please fix this by making the 
driver look more like Linux.  At least then it would be the same ugliness.

Can you explain how the change in the calculation of "chip" and the 
allocation size is relevant to the NULL dereference?  Couldn't that be fixed 
by just removing the "info->host[0]->mtd" line?

-Scott

  reply	other threads:[~2015-10-23 20:34 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-23 17:49 [U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug Kevin Smith
2015-10-23 18:20 ` Scott Wood
2015-10-23 19:56   ` Kevin Smith
2015-10-23 20:34     ` Scott Wood [this message]
2015-10-23 20:57       ` Kevin Smith
2015-10-23 21:14         ` Scott Wood
2015-10-23 21:18           ` Kevin Smith

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=1445632454.701.214.camel@freescale.com \
    --to=scottwood@freescale.com \
    --cc=u-boot@lists.denx.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox