Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-11 Thread Rusty Russell
"Michael S. Tsirkin" writes: >> virtio: don't crash when device is buggy >> >> Because of a sanity check in virtio_dev_remove, a buggy device can crash >> kernel. And in case of rproc it's userspace so it's not a good idea. >> We are unloading a driver so how bad can it be? >> Be less aggressive

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-10 Thread Michael S. Tsirkin
On Thu, Sep 06, 2012 at 08:27:35AM +0300, Michael S. Tsirkin wrote: > On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: > > "Michael S. Tsirkin" writes: > > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > > >> Hi Michael, > > >> > > >> > Exactly. Though if we jus

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-07 Thread Sjur Brændeland
Hi Michael. >> Not just fail to work, the kernel will panic on the BUG_ON(). >> Remoteproc gets the virtio configuration from firmware loaded >> from user space. So this type of problem might be triggered >> for other virtio drivers as well. > > how? ... >> Even if we fix this particular problem,

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-07 Thread Sjur Brændeland
Hi Ohad, >> A simplest thing to do is change dev id. rusty? > > For generic usage, this is correct. But my opinion is that fallback on > feature non-ack is quality-of-implementation issue: great to have, but > there are cases where you just want to fail with "you're too old". > > And in this case

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Michael S. Tsirkin
On Thu, Sep 06, 2012 at 11:34:25AM +0930, Rusty Russell wrote: > "Michael S. Tsirkin" writes: > > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > >> Hi Michael, > >> > >> > Exactly. Though if we just fail load it will be much less code. > >> > > >> > Generally, using a feature

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Rusty Russell
"Michael S. Tsirkin" writes: > On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: >> Hi Michael, >> >> > Exactly. Though if we just fail load it will be much less code. >> > >> > Generally, using a feature bit for this is a bit of a problem though: >> > normally driver is expected t

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Michael S. Tsirkin
On Wed, Sep 05, 2012 at 08:15:36PM +0200, Sjur Brændeland wrote: > Hi Michael. > > >> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM > >> when DMA is not supported, virtio will do BUG_ON() from > >> virtio_check_driver_offered_feature(). > >> > >> Is this acceptable or should we add a check

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Sjur Brændeland
Hi Michael. >> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM >> when DMA is not supported, virtio will do BUG_ON() from >> virtio_check_driver_offered_feature(). >> >> Is this acceptable or should we add a check in virtcons_probe() >> and let the probing fail instead? >> >> E.g: >> /*

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Michael S. Tsirkin
On Wed, Sep 05, 2012 at 03:00:20PM +0200, Sjur Brændeland wrote: > > The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you > > don't have DMA! > > OK, so the feature table could be done like this: > > static unsigned int features[] = { > VIRTIO_CONSOLE_F_SIZE, > VIRTIO

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-05 Thread Sjur Brændeland
> The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you > don't have DMA! OK, so the feature table could be done like this: static unsigned int features[] = { VIRTIO_CONSOLE_F_SIZE, VIRTIO_CONSOLE_F_MULTIPORT, #if VIRTIO_CONSOLE_HAS_DMA VIRTIO_CONSOLE_F_DMA

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-04 Thread Rusty Russell
"Michael S. Tsirkin" writes: > Also let's add a wrapper at top of file so ifdefs > do not litter the code like this. For example: > > #ifdef CONFIG_HAS_DMA > #define VIRTIO_CONSOLE_HAS_DMA (1) > #else > #define VIRTIO_CONSOLE_HAS_DMA (0) > #endif > > Now use if instead of ifdef. The driver certai

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-04 Thread Michael S. Tsirkin
On Tue, Sep 04, 2012 at 06:58:47PM +0200, Sjur Brændeland wrote: > Hi Michael, > > > Exactly. Though if we just fail load it will be much less code. > > > > Generally, using a feature bit for this is a bit of a problem though: > > normally driver is expected to be able to simply ignore > > a featu

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-04 Thread Sjur Brændeland
Hi Michael, > Exactly. Though if we just fail load it will be much less code. > > Generally, using a feature bit for this is a bit of a problem though: > normally driver is expected to be able to simply ignore > a feature bit. In this case driver is required to > do something so a feature bit is n

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-04 Thread Michael S. Tsirkin
On Tue, Sep 04, 2012 at 01:28:36PM +0200, Sjur Brændeland wrote: > Hi Michael, > > >> If an architecture do not support DMA you will get > >> a link error: "unknown symbol" for dma_alloc_coherent. > > > > Yes, it even seems intentional. > > But I have a question: can the device work without DMA? >

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-04 Thread Sjur Brændeland
Hi Michael, >> If an architecture do not support DMA you will get >> a link error: "unknown symbol" for dma_alloc_coherent. > > Yes, it even seems intentional. > But I have a question: can the device work without DMA? The main dependency is actually on the dma-allocation. In my case I do dma_decl

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-03 Thread Michael S. Tsirkin
On Mon, Sep 03, 2012 at 04:57:45PM +0200, Sjur Brændeland wrote: > Hi Michael, > > > How does access to descriptors work in this setup? > > When the ring is setup by remoteproc the descriptors are > also allocated using dma_alloc_coherent(). > > >> -static void free_buf(struct port_buffer *buf)

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-03 Thread Sjur Brændeland
Hi Michael, > How does access to descriptors work in this setup? When the ring is setup by remoteproc the descriptors are also allocated using dma_alloc_coherent(). >> -static void free_buf(struct port_buffer *buf) >> +/* Allcoate data buffer from DMA memory if requested */ > > typo Thanks. >>

Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-03 Thread Michael S. Tsirkin
On Mon, Sep 03, 2012 at 03:51:16PM +0200, sjur.brandel...@stericsson.com wrote: > From: Sjur Brændeland > > Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has > DMA support and this feature bit is set, the virtio data buffers > will be allocated from DMA memory. > > This is needed for

[RFC 1/2] virtio_console: Add support for DMA memory allocation

2012-09-03 Thread sjur . brandeland
From: Sjur Brændeland Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has DMA support and this feature bit is set, the virtio data buffers will be allocated from DMA memory. This is needed for using virtio_console from the remoteproc framework. Signed-off-by: Sjur Brændeland cc: Rust