Re: spiflash.c process_write()

[Radoslaw Kujawa <radoslaw.kujawa%c0ff33.net@localhost>]

Hi.

I am not exactly qualified in the area of spiflash internals, but I have used it on several occasions. 

> On 08 Sep 2015, at 08:12, David Holland <dholland-tech%netbsd.org@localhost> wrote:
> 
> As noted in passing elsewhere, it seems that process_write() in
> spiflash.c allocates a scratch buffer on every call... and leaks it on
> every call too. This clearly isn't a good thing.
> 
> Meanwhile the size of buffer it tries to allocate doesn't have any
> obvious bound; I suspect it's limited to MAXPHYS, and if so that's
> probably ok, but if not there could be problems even if the leak is
> fixed.
> 
> Meanwhile, spiflash.c has only one client (m25p) which, in its man
> page, says:
> 
> BUGS
>     Some important bits that are vital for use as a mass storage device are
>     still missing.  Specifically, there is no API to erase the device, and
>     there is no support for device partitioning.
> 
> So, presumably this code is only lightly tested.

A few years ago, when I worked on bringing Armada XP into our tree, I had (still have somewhere in my closet) a Marvell dev board which featured m25p. The spiflash worked just fine, for what it is. So, yes, I never used it in the block device sense, but it was OK as a character device used to access the firmware contents. 

> ISTM that the code probably ought to be rewritten to handle one erase
> block at a time; and, unless there's a reason to worry about
> concurrent throughput (how fast is SPI anyway?), it ought to allocate
> one write buffer up front instead of churning through them on every
> write request.

Current SPI variants can be pretty fast. Some modern M25P derivatives like MT25Q can transfer 4 bits at 133MHz (around 66MB/s). 

> And the driver isn't MPSAFE, which seems fairly readily fixed if one's
> going to hack it anyhow. But it isn't clear to me how important this
> is. Among other things, NOR flash is in general not the future.
> 
> There has been some discussion of these points since the issue
> appeared (in maxv's Brainy posting a couple weeks ago) but no
> conclusions. I guess the questions at this point are:
> - does anyone have an m25p they can test driver changes with?

Yep. Probably it's easier for me to attach a standalone M25P16 to RPI than to search for that dev board.

> - how much do we care about this driver?

Not very much but it's nice to have it :P. Perhaps it should be converted to use flash(9)...

> I'm willing to fix the memory leak without testing, but no more than
> that.

I can test it.

Best regards,
Radoslaw