goofy mtd m25p80 patches in GIT ...

goofy mtd m25p80 patches in GIT ...

[David Brownell]

[resend cc'ing linux-mtd, sorry]

I noticed a couple goofy patches in MTD GIT, while poking around
wondering what happened to some patches that I expected would
already have gotten upstream.  Details below.

- Dave


First:

 http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=faff37508a104e9ec5285d5adecaab7e8dde472a

That patch is goofy because the command in question is *NOT* a block
erase command.  It's a chip-erase command ... entirely unlike the
existing *real* block erase commands used in the driver.

Could we get a fix that provides the correct name for the operations?
Having real block commands, and this new thing, is at the very least
confusing...


Second:

 http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81

That's just plain wrong ... the original code is correct, but the
patch changed it to be incorrect.  (DMA from the stack is never
legal.)

Just revert this one entirely.

RE: goofy mtd m25p80 patches in GIT ...

[Chen Gong-B11801]

> 
> Second:
> 
>  
> http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=
> commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81
> 
> That's just plain wrong ... the original code is correct, but the
> patch changed it to be incorrect.  (DMA from the stack is never
> legal.)
> 
> Just revert this one entirely.
> 
> 
I know spi_write_then_read is different from spi_write, but here n_rx & rxbuf are
both NULL, which means no real read operation are executed. Would you please
give me more explanation for potential hazard ?

Re: goofy mtd m25p80 patches in GIT ...

[David Brownell]

On Monday 20 October 2008, Chen Gong-B11801 wrote:
> 
> > 
> > Second:
> > 
> >  
> > http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=
> > commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81
> > 
> > That's just plain wrong ... the original code is correct, but the
> > patch changed it to be incorrect.  (DMA from the stack is never
> > legal.)
> > 
> > Just revert this one entirely.
>  
> I know spi_write_then_read is different from spi_write, but here n_rx & rxbuf are
> both NULL, which means no real read operation are executed. Would you please
> give me more explanation for potential hazard ?

Documentation/DMA-mapping.txt, in the "What memory is DMA'able?"
section (the first one after overview) lists several hazards.
I won't elaborate on that text, but it also states explicitly
that DMA from stacks is not allowed (para 4, sentence one).

Note that the spi_write_then_read() is documented as always
copying its (small) data buffers, while spi_write() just
passes it down to the spi_master driver ... which requires
DMA-safe memory, as described in "What memory is DMA'able?".

Have a look at the implementation of spi_write_then_read(),
in drivers/spi/spi.c ... one of the main reasons for that
routine is to make some of the common cases easier to work
with, by masking those DMA issues.

- Dave

Re: goofy mtd m25p80 patches in GIT ...

[David Woodhouse]

On Sun, 2008-10-19 at 15:35 -0700, David Brownell wrote:
> [resend cc'ing linux-mtd, sorry]
> 
> I noticed a couple goofy patches in MTD GIT, while poking around
> wondering what happened to some patches that I expected would
> already have gotten upstream.  Details below.

Did you find the patches you expected to be going upstream?

> First:
> 
>  http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=faff37508a104e9ec5285d5adecaab7e8dde472a
> 
> That patch is goofy because the command in question is *NOT* a block
> erase command.  It's a chip-erase command ... entirely unlike the
> existing *real* block erase commands used in the driver.
> 
> Could we get a fix that provides the correct name for the operations?
> Having real block commands, and this new thing, is at the very least
> confusing...

Makes sense. Chen Gong?

> 
> Second:
> 
>  http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81
> 
> That's just plain wrong ... the original code is correct, but the
> patch changed it to be incorrect.  (DMA from the stack is never
> legal.)

Ah, so spi_write() uses DMA, but spi_write_then_read() does not? 
Not entirely intuitive :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: goofy mtd m25p80 patches in GIT ...

[Chen Gong-B11801]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: 2008?10?20? 15:40
> To: David Brownell
> Cc: Chen Gong-B11801; linux-mtd@lists.infradead.org
> Subject: Re: goofy mtd m25p80 patches in GIT ...
> 
> On Sun, 2008-10-19 at 15:35 -0700, David Brownell wrote:
> > [resend cc'ing linux-mtd, sorry]
> > 
> > I noticed a couple goofy patches in MTD GIT, while poking around
> > wondering what happened to some patches that I expected would
> > already have gotten upstream.  Details below.
> 
> Did you find the patches you expected to be going upstream?
> 
> > First:
> > 
> >  
> http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=
> commitdiff;h=faff37508a104e9ec5285d5adecaab7e8dde472a
> > 
> > That patch is goofy because the command in question is *NOT* a block
> > erase command.  It's a chip-erase command ... entirely unlike the
> > existing *real* block erase commands used in the driver.
> > 
> > Could we get a fix that provides the correct name for the 
> operations?
> > Having real block commands, and this new thing, is at the very least
> > confusing...
> 
> Makes sense. Chen Gong?

I agree with you.

> 
> > 
> > Second:
> > 
> >  
> http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=
> commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81
> > 
> > That's just plain wrong ... the original code is correct, but the
> > patch changed it to be incorrect.  (DMA from the stack is never
> > legal.)
> 
> Ah, so spi_write() uses DMA, but spi_write_then_read() does not? 
> Not entirely intuitive :)
> 
> -- 
> David Woodhouse                            Open Source 
> Technology Centre
> David.Woodhouse@intel.com                              Intel 
> Corporation
> 
> 

RE: goofy mtd m25p80 patches in GIT ...

[David Woodhouse]

On Mon, 2008-10-20 at 15:42 +0800, Chen Gong-B11801 wrote:
> 
> > > Could we get a fix that provides the correct name for the 
> > operations?
> > > Having real block commands, and this new thing, is at the very least
> > > confusing...
> > 
> > Makes sense. Chen Gong?
> 
> I agree with you.

I was hoping for a response in 'diff -up' form... :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: goofy mtd m25p80 patches in GIT ...

[Chen Gong-B11801]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: 2008?10?20? 15:44
> To: Chen Gong-B11801
> Cc: David Brownell; linux-mtd@lists.infradead.org
> Subject: RE: goofy mtd m25p80 patches in GIT ...
> 
> On Mon, 2008-10-20 at 15:42 +0800, Chen Gong-B11801 wrote:
> > 
> > > > Could we get a fix that provides the correct name for the 
> > > operations?
> > > > Having real block commands, and this new thing, is at 
> the very least
> > > > confusing...
> > > 
> > > Makes sense. Chen Gong?
> > 
> > I agree with you.
> 
> I was hoping for a response in 'diff -up' form... :)

I'm not sure whether I catch your word. Frankly I agree that we need a
chip-erase
command but in fact current MTD doesn't support this kind of interface
(only one erase interface),
so I can't give you a new patch :-(

RE: goofy mtd m25p80 patches in GIT ...

[David Woodhouse]

On Mon, 2008-10-20 at 15:56 +0800, Chen Gong-B11801 wrote:
> I'm not sure whether I catch your word. Frankly I agree that we need a
> chip-erase command but in fact current MTD doesn't support this kind of interface
> (only one erase interface), so I can't give you a new patch :-(

I think the interface is fine -- if you ask it to erase the whole chip
(start == 0, len == size), then it'll use a chip erase command if it has
one, or fall back to erasing individual blocks.

I think David just wanted a cleanup to the nomenclature -- renaming
'erase_block' to 'erase_chip', perhaps, and changing the #define for the
command?

Or did I misunderstand?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: goofy mtd m25p80 patches in GIT ...

[Chen Gong-B11801]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: 2008?10?20? 16:03
> To: Chen Gong-B11801
> Cc: David Brownell; linux-mtd@lists.infradead.org
> Subject: RE: goofy mtd m25p80 patches in GIT ...
> 
> On Mon, 2008-10-20 at 15:56 +0800, Chen Gong-B11801 wrote:
> > I'm not sure whether I catch your word. Frankly I agree 
> that we need a
> > chip-erase command but in fact current MTD doesn't support 
> this kind of interface
> > (only one erase interface), so I can't give you a new patch :-(
> 
> I think the interface is fine -- if you ask it to erase the whole chip
> (start == 0, len == size), then it'll use a chip erase 
> command if it has
> one, or fall back to erasing individual blocks.
> 
> I think David just wanted a cleanup to the nomenclature -- renaming
> 'erase_block' to 'erase_chip', perhaps, and changing the 
> #define for the
> command?

I prefer to the first opinion -- renaming 'erase_block' to 'erase_chip',
it looks
more straightforward

Re: goofy mtd m25p80 patches in GIT ...

[David Brownell]

On Monday 20 October 2008, Chen Gong-B11801 wrote:
> I prefer to the first opinion -- renaming 'erase_block' to 'erase_chip',
> it looks more straightforward

*AND* fixing the name of the operation.

The pre-existing erase operations were:

 #define OPCODE_BE_4K            0x20    /* Erase 4KiB block */
 #define OPCODE_BE_32K           0x52    /* Erase 32KiB block */
 #define OPCODE_SE               0xd8    /* Sector erase (usually 64KiB) */

So I'm quite puzzled why you would call the chip erase
opcode a "block erase" opcode ... it's not a 4K block,
not a 32K block, not a sector (of, usually, 64K).

It's more typically something like multiple MBytes.

Having read or skimmed several dozen SPI flash specs,
none of them called chip erase a "block erase" operation.

RE: goofy mtd m25p80 patches in GIT ...

[Chen Gong-B11801]

 

> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net] 
> Sent: 2008?10?20? 16:31
> To: Chen Gong-B11801
> Cc: David Woodhouse; linux-mtd@lists.infradead.org
> Subject: Re: goofy mtd m25p80 patches in GIT ...
> 
> On Monday 20 October 2008, Chen Gong-B11801 wrote:
> > I prefer to the first opinion -- renaming 'erase_block' to 
> 'erase_chip',
> > it looks more straightforward
> 
> *AND* fixing the name of the operation.
> 
> The pre-existing erase operations were:
> 
>  #define OPCODE_BE_4K            0x20    /* Erase 4KiB block */
>  #define OPCODE_BE_32K           0x52    /* Erase 32KiB block */
>  #define OPCODE_SE               0xd8    /* Sector erase 
> (usually 64KiB) */
> 
> So I'm quite puzzled why you would call the chip erase
> opcode a "block erase" opcode ... it's not a 4K block,
> not a 32K block, not a sector (of, usually, 64K).

ah, simple, because I'v gotten it from its datasheet (BE means Bulk
Erase, not block erase here) :-)

> 
> It's more typically something like multiple MBytes.
> 
> Having read or skimmed several dozen SPI flash specs,
> none of them called chip erase a "block erase" operation.
> 
> 

Re: goofy mtd m25p80 patches in GIT ...

[David Brownell]

On Monday 20 October 2008, David Woodhouse wrote:
> I think the interface is fine -- if you ask it to erase the whole chip
> (start == 0, len == size), then it'll use a chip erase command if it has
> one, or fall back to erasing individual blocks.
> 
> I think David just wanted a cleanup to the nomenclature -- renaming
> 'erase_block' to 'erase_chip', perhaps, and changing the #define for the
> command?

Exactly.  Since the existing erase block calls are not
doing whole-chip erasure ... and since chip-erase is
a *VERY* different operation.

The MTD interface just says "erase this stuff", and
that patch just added a special case "if <stuff> is
the whole chip, use this operation instead of the
existing iterate-over-blocks logic".

- Dave

Re: goofy mtd m25p80 patches in GIT ...

[David Brownell]

On Monday 20 October 2008, David Woodhouse wrote:
> On Sun, 2008-10-19 at 15:35 -0700, David Brownell wrote:
> > [resend cc'ing linux-mtd, sorry]
> > 
> > I noticed a couple goofy patches in MTD GIT, while poking around
> > wondering what happened to some patches that I expected would
> > already have gotten upstream.  Details below.
> 
> Did you find the patches you expected to be going upstream?

Yes, sorry to have not made that explicit.
The were the mtd_dataflash.c patches.

(And I'm hoping we'll see an MTD "pull" request
for 2.6.28-rc0 ... )


> 
> > First:
> > 
> >  http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=faff37508a104e9ec5285d5adecaab7e8dde472a
> > 
> > That patch is goofy because the command in question is *NOT* a block
> > erase command.  It's a chip-erase command ... entirely unlike the
> > existing *real* block erase commands used in the driver.
> > 
> > Could we get a fix that provides the correct name for the operations?
> > Having real block commands, and this new thing, is at the very least
> > confusing...
> 
> Makes sense. Chen Gong?
> 
> > 
> > Second:
> > 
> >  http://git.kernel.org/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commitdiff;h=75d0ee2202b5740e94e913d8a52f91c6557c4c81
> > 
> > That's just plain wrong ... the original code is correct, but the
> > patch changed it to be incorrect.  (DMA from the stack is never
> > legal.)
> 
> Ah, so spi_write() uses DMA, but spi_write_then_read() does not? 
> Not entirely intuitive :)

Maybe not ... but documented.
(More info in response to Chen Gong.)


> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
> 
> 

Re: goofy mtd m25p80 patches in GIT ...

[David Woodhouse]

On Mon, 2008-10-20 at 01:04 -0700, David Brownell wrote:
> 
> Yes, sorry to have not made that explicit.
> The were the mtd_dataflash.c patches.
> 
> (And I'm hoping we'll see an MTD "pull" request
> for 2.6.28-rc0 ... )

It went out a couple of days ago.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation