Re: [Cbe-oss-dev] [patch 3/5] cell: updated driver for DDR2 memory on AXON

Re: [Cbe-oss-dev] [patch 3/5] cell: updated driver for DDR2 memory on AXON

[Arnd Bergmann]

On Tuesday 19 June 2007, Akinobu Mita wrote:
> > +	if (of_address_to_resource(device->node, 0, &resource) != 0) {
> > +		dev_err(&device->dev, "Cannot access device tree\n");
> > +		rc = -EFAULT;
> > +		goto failed;
> > +	}
> 
> of_address_to_resource() returns error code on failure:
> 
> 	rc = of_address_to_resource(device->node, 0, &resource);
> 	if (rc) {
> 		dev_err(&device->dev, "Cannot access device tree\n");
> 		goto failed;
> 	}
> 
> is better.

Right.

> > +	bank->ph_addr = resource.start;
> > +	bank->io_addr = (unsigned long) ioremap_flags(
> > +			bank->ph_addr, bank->size, _PAGE_NO_CACHE);
> > +	if (bank->io_addr == 0) {
> > +		dev_err(&device->dev, "ioremap() failed\n");
> > +		rc = -EFAULT;
> > +		goto failed;
> > +	}
> > +
> > +	bank->disk = alloc_disk(AXON_RAM_MINORS_PER_DISK);
> > +	if (bank->disk == NULL) {
> > +		dev_err(&device->dev, "Cannot register disk\n");
> > +		rc = -EFAULT;
> > +		goto failed;
> > +	}
> 
> -ENOMEM is better than -EFAULT. Because alloc_disk() failure happens
> only when it runs out of memory.

yes. EFAULT should only be used when an access to user memory has failed,
so it's wrong practically everywhere in here, your other comments are
obviously correct as well. 

	Arnd <><

Re: [Cbe-oss-dev] [patch 3/5] cell: updated driver for DDR2 memory on AXON

[Arnd Bergmann]

On Tuesday 26 June 2007, Maxim Shchetynin wrote:
> Actually, axon_ram's direct_access method would not be used by ext2 at all
> - ext2 feels comfortable with make_request().

not for mmap().

What will actually happen if you try to mount an axonram device with ext2?
I suppose mount should fail with a proper error code if the block size is
larger than 4kb, but does that happen?

If you have a 4k block size axonram device, the ext2 really should work
using XIP as expected, including the mmap() operation.

> We have the direct_access method here only because it is needed for the
> azfs file-system, which we recommend to use for accessing the Axon's RAM
> rather then ext2 or any other buffered file-systems.

I think I've understood what the problem is now. The generic xip code
assumes that the data to be mapped has a 'struct page' in mem_map,
and that it's part of the linear kernel mapping. This is actually
true on s390, where it is currently used, but not for us. The point
on s390 is that the kernel virtual address is _identical_ to the
physical address, so it may never have shown up as a problem.

I'll talk to Carsten about this, he already had plans to remove
the need for struct page from the filemap_xip infrastructure.

	Arnd <><

Re: [Cbe-oss-dev] [patch 3/5] cell: updated driver for DDR2 memory on AXON

[Arnd Bergmann]

On Tuesday 26 June 2007, Carsten Otte wrote:
> 
> Arnd Bergmann wrote:
> > What will actually happen if you try to mount an axonram device with ext2?
> > I suppose mount should fail with a proper error code if the block size is
> > larger than 4kb, but does that happen?
> > 
> > If you have a 4k block size axonram device, the ext2 really should work
> > using XIP as expected, including the mmap() operation.
> > 
> Absolutely. What's the symptom if you try mount -o xip? I am willing 
> to fix any bugs that show in the ext2/xip code in that scenario...

The real problem is that the ->direct_access function is supposed to return
a kernel virtual address in the linear mapping, which does not make any
sense for axonram. The physical address of the underlying device is
outside of the kernel mapping, and gets mapped into the kernel virtual
space using ioremap, with flags to disallow caching.

If ->direct_access returns a physical address, the file system cannot
access the data through the kernel mapping, but if it returns the
ioremapped address, it cannot know what address to map into the
user page tables, because virt_to_page() is not defined for the
ioremap space.

Maximchose to return the physical address, which is what he needs in
his file system, but that's just not how the interface was designed
to be used.

One way to change fix the problem might be to give a flag to the
->direct_access function to ask for either the page frame number,
or the virtual address (from ioremap or the linear mapping).
It's rather ugly though, so if you have a better idea, let us
know.

Another problem is that the mmap, or no_page, function really
needs to know about the pgprot value it should use. If we add
a field to struct block_device for this, the drivers could set
it there.

	Arnd <><

Re: [Cbe-oss-dev] [patch 3/5] cell: updated driver for DDR2 memory on AXON

[Carsten Otte]

Arnd Bergmann wrote:
> What will actually happen if you try to mount an axonram device with ext2?
> I suppose mount should fail with a proper error code if the block size is
> larger than 4kb, but does that happen?
> 
> If you have a 4k block size axonram device, the ext2 really should work
> using XIP as expected, including the mmap() operation.
> 
Absolutely. What's the symptom if you try mount -o xip? I am willing 
to fix any bugs that show in the ext2/xip code in that scenario...

>> We have the direct_access method here only because it is needed for the
>> azfs file-system, which we recommend to use for accessing the Axon's RAM
>> rather then ext2 or any other buffered file-systems.
> 
> I think I've understood what the problem is now. The generic xip code
> assumes that the data to be mapped has a 'struct page' in mem_map,
> and that it's part of the linear kernel mapping. This is actually
> true on s390, where it is currently used, but not for us. The point
> on s390 is that the kernel virtual address is _identical_ to the
> physical address, so it may never have shown up as a problem.
> 
> I'll talk to Carsten about this, he already had plans to remove
> the need for struct page from the filemap_xip infrastructure.
Removing the need for struct page is indeed impossible with current 
memory management because of the fact that VM_PFNMAP vmas have the 
downside that they require a linear physical image of the vma. Ext2 
does not comply to that requriement, a given file may be spread all 
over a media.