Re: [PATCH 2.6-git] SPI core refresh

Re: [PATCH 2.6-git] SPI core refresh

[vitalhome]

Mark,

> > >I still do not see why you are stating this.  Why do you say this?
> > > 
> > >
> > Due to possible priority inversion problems in David's core.
> 
> Which you still haven't proven, in fact you now seem to be changing your mind and saying 
> that
> there might be a problem if an adapter driver was implemented badly although I still 
> don't see how
> this could happen (the priority inversion I mean not the badly implemented driver ;).

Truly admiring your deep understanding of the real-time technology, I should remind you 
that within the real-time conditions almost each event may happen and may not happen, for 
instance, two calls from different context to the same funtion may happen at the same or 
almost the same time, and may not happen that way. Therefore I used the word "possible". 
Hope I clarified that a bit for you.

Please also see my previous emails for the explanation of how priority inversion can 
happen. This is not gonna be a rare case, BTW.

Vitaly

Re: [PATCH 2.6-git] SPI core refresh

[Mark Underwood]

--- vitalhome@rbcmail.ru wrote:

> Mark,
> 
> > > >I still do not see why you are stating this.  Why do you say this?
> > > > 
> > > >
> > > Due to possible priority inversion problems in David's core.
> > 
> > Which you still haven't proven, in fact you now seem to be changing your mind and saying 
> > that
> > there might be a problem if an adapter driver was implemented badly although I still 
> > don't see how
> > this could happen (the priority inversion I mean not the badly implemented driver ;).
> 
> Truly admiring your deep understanding of the real-time technology, I should remind you 
> that within the real-time conditions almost each event may happen and may not happen, for 
> instance, two calls from different context to the same funtion may happen at the same or 
> almost the same time, and may not happen that way. Therefore I used the word "possible". 
> Hope I clarified that a bit for you.
> 
> Please also see my previous emails for the explanation of how priority inversion can 
> happen. This is not gonna be a rare case, BTW.

Vitaly, 
 
First, please can you not change the CC list in the midle of a thread. 
 
Second, I studied real-time OS's at university and even started to write my  own RTOS so I do know
the basic's of real-time technology. My problem wasn't understanding what you meant, just which
part of the code you where referring to :(. 
 
OK, looking through the code after a cup of coffe I can see the problem you are pointing out,
thank you :), for some reason I thought that that code was protected by a spin_lock :/. 
 
How to fix this? 
 
David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure? This
helper routine could then use this thus solving the one buffer to many callers problem (well
moving into the adapter driver, but as that serialise's transfers anyway I think this would remove
the priority inversion problem, Vitaly?) 
 
The other solution is to do a kmalloc for each caller (would could try to be smart and only do
that if the buffer is being used). 
 
Let me know, if noone else is interested in fixing this then I'll do it and send a patch. 
 
Mark

> 
> Vitaly
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



		
___________________________________________________________ 
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

Mark Underwood wrote:

>--- vitalhome@rbcmail.ru wrote:
>
>  
>
>>Mark,
>>
>>    
>>
>>>>>I still do not see why you are stating this.  Why do you say this?
>>>>>
>>>>>
>>>>>          
>>>>>
>>>>Due to possible priority inversion problems in David's core.
>>>>        
>>>>
>>>Which you still haven't proven, in fact you now seem to be changing your mind and saying 
>>>that
>>>there might be a problem if an adapter driver was implemented badly although I still 
>>>don't see how
>>>this could happen (the priority inversion I mean not the badly implemented driver ;).
>>>      
>>>
>>Truly admiring your deep understanding of the real-time technology, I should remind you 
>>that within the real-time conditions almost each event may happen and may not happen, for 
>>instance, two calls from different context to the same funtion may happen at the same or 
>>almost the same time, and may not happen that way. Therefore I used the word "possible". 
>>Hope I clarified that a bit for you.
>>
>>Please also see my previous emails for the explanation of how priority inversion can 
>>happen. This is not gonna be a rare case, BTW.
>>    
>>
>
>Vitaly, 
> 
>First, please can you not change the CC list in the midle of a thread.
>
Yeah, sorry for that. You see, I was emailing not from my computer.

> 
>OK, looking through the code after a cup of coffe I can see the problem you are pointing out,
>thank you :), for some reason I thought that that code was protected by a spin_lock :/. 
> 
>How to fix this? 
> 
>David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure? This
>helper routine could then use this thus solving the one buffer to many callers problem (well
>moving into the adapter driver, but as that serialise's transfers anyway I think this would remove
>the priority inversion problem, Vitaly?) 
> 
>The other solution is to do a kmalloc for each caller (would could try to be smart and only do
>that if the buffer is being used). 
>  
>
And each one of the techniques suggested will make David's core closer 
to ours :)

Vitaly

Re: [PATCH 2.6-git] SPI core refresh

[David Brownell]

On Saturday 03 December 2005 9:10 am, Mark Underwood wrote:
> 
> David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure?

Not good; it'd mean that every controller driver would have to support
both PIO and DMA modes.  This minor issue (despite the noise!) isn't
worth making such an intrusive demand on all drivers.


> The other solution is to do a kmalloc for each caller (would could try to be smart and only do
> that if the buffer is being used). 

That's far preferable.  You could just submit the patch against rc3-mm1
and that'll do the job.

- Dave

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

David Brownell wrote:

>>I was trying to compare the approaches in a somehow deeper way.
>>That said, I meant that not exposing any structures you don't have to 
>>expose was usually a right way to do things.
>>We don't expose SPI message and you do.
>>    
>>
>
>What information there _isn't_ in the "have to expose" category?
>  
>
I'm sure that any message internals aren't; it leaves freedom to 
implement messages allocation in the way we like.

>Yours certainly has some ... clocks as per-message not per-device,
>and your message utilities still doing kmalloc() and memcpy() in
>places where it's not clearly necessary.  Plus the error-prone
>notion that completion callbacks could be optional, and a few
>other things I've pointed out previously.
>  
>
What's wrong with a) clocks per device b) no completion callback?
b) just makes it a sync call, it's a convenient thing for a developer.
As for places with kmalloc's and memcpy's, can you just point out one? 
I'm afraid it's all bare words here.
On the _contrary_, the approach we use allows to use lightweight 
non-standard allocation methods for messages which you just can't do.

>The choice of an array of spi_transfer rather than a custom
>singly linked list (not even list_head)?  I just picked the
>one with the smallest mandatory overhead (including adding the
>least number of fault cases to test and recover from).  Lots of
>APIs made the same choice; it works just fine here.  Heck, it's
>one of the few things here that resembles the I2C API!
>  
>
Yes, but the transfer array is not of a fixed size, therefore it's not 
that easy to use custom allocation techniques.
I'm afraid though that you don't quite understand what I'm meaning here, 
but you'll see it in the core I'll post today.

>
>  
>
>>On the other hand, our approach is flexible in means of message 
>>allocation. I. e. a small memory allocation library can be implemented 
>>transparently to device drivers that handles message allocation. It's 
>>very easy (and lightweight :)) since the message structure is always of 
>>a same length... Agree?
>>    
>>
>
>No, as I explained before.  But I'd not mind having optional layers
>like that, so long as they were in fact optional.  Yours is not; plus
>it's heavy-weight (embedding mallocation of dma bounce buffers and
>copying into/out of them, even when it's not necessary) and it's not
>refcounted.
>  
>
What? What optional layers are you talking about???
And please look *attentively* into the code, memory allocation/copying 
happens only if a specific flag is set so it's no way it can be unnecessary.

>
>  
>
>>>That said ... I know some people _do_ like krefcounted APIs that
>>>do that kind of stuff.  Strongly.  Greg's been silent here other
>>>than pointing out that your request alloc was too fat to inline.
>>>Mine is trivially inlined, but not refcounted.  Likely there's a
>>>happy middle ground, maybe
>>>
>>> mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
>>> mesg = spi_message_get(mesg);
>>> spi_message_put();
>>> 
>>>
>>>      
>>>
>>Well, it's pretty much what we do in the latest one...
>>    
>>
>
>No, you have no get/put krefcounting, and (to repeat an earlier
>comment) you also require "ntrans" separate allocations.  So
>"what 'we' do in the latest one" is substantially different.
>  
>
I was speaking about the API. Sorry for being not that clear here,

>
>  
>
>>Though we use one-by-one chaining and not specifying the number of 
>>messages in chain.
>>I'm not sure which approach is better, really.
>>    
>>
>
>In terms of potential faults that requires (debugging) driver code to
>recover from, having a single allocation is a clear win.  Have you
>ever noticed now many patches go into Linux to handle cases where
>driver fault cleanup got confused, and oopsed in one of the less
>common (but still observed in the Real World) scenarios?
>  
>
Not necessarily; if we've got a preallocated memory pool, than it's not 
that important.
On the contrary, having messages of a same size makes it easier for 
write a custom malloc routine.

Vitaly

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[David Brownell]

> >Synchronous transfers can easily use stack allocation for
> >the descriptions, yes.
> >
> >Not that they need to ... the ads7846 driver allocates its
> >spi_message and spi_transfer objects on the heap both for
> >synchronous operations (temperature and voltage sensing) and
> >for asynch ones (touchscreen tracking from timer and irq).
>  
> So are you talking about only kzalloc vs kmalloc?

Certainly not.  But I was re-emphasizing that zero-init rule,
in a different part of my reply.


> I was trying to compare the approaches in a somehow deeper way.
> That said, I meant that not exposing any structures you don't have to 
> expose was usually a right way to do things.
> We don't expose SPI message and you do.

What information there _isn't_ in the "have to expose" category?

Yours certainly has some ... clocks as per-message not per-device,
and your message utilities still doing kmalloc() and memcpy() in
places where it's not clearly necessary.  Plus the error-prone
notion that completion callbacks could be optional, and a few
other things I've pointed out previously.

The choice of an array of spi_transfer rather than a custom
singly linked list (not even list_head)?  I just picked the
one with the smallest mandatory overhead (including adding the
least number of fault cases to test and recover from).  Lots of
APIs made the same choice; it works just fine here.  Heck, it's
one of the few things here that resembles the I2C API!


> On the other hand, our approach is flexible in means of message 
> allocation. I. e. a small memory allocation library can be implemented 
> transparently to device drivers that handles message allocation. It's 
> very easy (and lightweight :)) since the message structure is always of 
> a same length... Agree?

No, as I explained before.  But I'd not mind having optional layers
like that, so long as they were in fact optional.  Yours is not; plus
it's heavy-weight (embedding mallocation of dma bounce buffers and
copying into/out of them, even when it's not necessary) and it's not
refcounted.


> >That said ... I know some people _do_ like krefcounted APIs that
> >do that kind of stuff.  Strongly.  Greg's been silent here other
> >than pointing out that your request alloc was too fat to inline.
> >Mine is trivially inlined, but not refcounted.  Likely there's a
> >happy middle ground, maybe
> >
> >  mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
> >  mesg = spi_message_get(mesg);
> >  spi_message_put();
> >  
> >
> Well, it's pretty much what we do in the latest one...

No, you have no get/put krefcounting, and (to repeat an earlier
comment) you also require "ntrans" separate allocations.  So
"what 'we' do in the latest one" is substantially different.


> Though we use one-by-one chaining and not specifying the number of 
> messages in chain.
> I'm not sure which approach is better, really.

In terms of potential faults that requires (debugging) driver code to
recover from, having a single allocation is a clear win.  Have you
ever noticed now many patches go into Linux to handle cases where
driver fault cleanup got confused, and oopsed in one of the less
common (but still observed in the Real World) scenarios?


> Maybe an API like 
> struct spi_mjsg *spi_message_alloc(struct spi_device *, unsigned ntrans);
> spi_add_transfer(struct spi_msg *, ...);
> ...

I thought about such an "add" call, but there'd need to be three of them
(to add a TX buffer, an RX buffer, or both TX and RX buffers) and it really
doesn't seem even conceptually hard to expect folk to write

	msg->transfer[0].tx_buf = ...;
	msg->transfer[0].length = ...;

	msg->transfer[1].rx_buf = ...;
	msg->transfer[1].length = ...;

	msg->transfer[2].tx_buf = ...;
	msg->transfer[2].rx_buf = ...;
	msg->transfer[2].length = ...;

And in fact, having that stuff explicit seems preferable to me; no point
in "convenience" wrappers for something already that simple.

The only potentially useful thing about an spi_transfer_add_{rx,tx,txrx}()
set of inlines would be that it might make the protocol tweaking options
stand out more.  Say, like the "drop chipselect for 20 usec after this
and before the next one", or other customization.  Me, I'd rather highlight
such options with intelligent use of whitespace and comments.


> is better than what we've got now (see patch sent 12/05; I plan to post 
> the update tomorrow).
> I would just like to say that defining such an API looks better thing to 
> me than dealing with SPI message structure explicitly.

You've heard where and why I disagree.  But I'd probably not turn down a
patch that adds optional krefcounting support for spi_message, returning
a message with the transfer[] array ready to be filled out.

- Dave

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

David Brownell wrote:

>>But  
>>this solves the problem only partially since this technique fits only 
>>the synchronous transfers.
>>    
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>  
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to 
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to 
allocate messages on stack but this approach has some limitations we've 
just agrred upon.
On the other hand, our approach is flexible in means of message 
allocation. I. e. a small memory allocation library can be implemented 
transparently to device drivers that handles message allocation. It's 
very easy (and lightweight :)) since the message structure is always of 
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff.  Strongly.  Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted.  Likely there's a
>happy middle ground, maybe
>
>  mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
>  mesg = spi_message_get(mesg);
>  spi_message_put();
>  
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of 
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_msg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post 
the update tomorrow).
I would just like to say that defining such an API looks better thing to 
me than dealing with SPI message structure explicitly.

Vitaly

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

David Brownell wrote:

>>But  
>>this solves the problem only partially since this technique fits only 
>>the synchronous transfers.
>>    
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>  
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to 
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to 
allocate messages on stack but this approach has some limitations we've 
just agrred upon.
On the other hand, our approach is flexible in means of message 
allocation. I. e. a small memory allocation library can be implemented 
transparently to device drivers that handles message allocation. It's 
very easy (and lightweight :)) since the message structure is always of 
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff.  Strongly.  Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted.  Likely there's a
>happy middle ground, maybe
>
>  mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
>  mesg = spi_message_get(mesg);
>  spi_message_put();
>  
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of 
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_msg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post 
the update tomorrow).
I would just like to say that defining such an API looks better thing to 
me than dealing with SPI message structure explicitly.

Vitaly

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

David Brownell wrote:

>>But  
>>this solves the problem only partially since this technique fits only 
>>the synchronous transfers.
>>    
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>  
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to 
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to 
allocate messages on stack but this approach has some limitations we've 
just agrred upon.
On the other hand, our approach is flexible in means of message 
allocation. I. e. a small memory allocation library can be implemented 
transparently to device drivers that handles message allocation. It's 
very easy (and lightweight :)) since the message structure is always of 
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff.  Strongly.  Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted.  Likely there's a
>happy middle ground, maybe
>
>  mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
>  mesg = spi_message_get(mesg);
>  spi_message_put();
>  
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of 
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_mjsg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post 
the update tomorrow).
I would just like to say that defining such an API looks better thing to 
me than dealing with SPI message structure explicitly.

Vitaly

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[David Brownell]

> > The benefit you're talking about is that you don't have to use 
> > heavyweight memory allocation. But... the transfer is basically async 
> > so spi->master->transfer will need to copy your message structure to 
> > its own-allocated structure so some memory copying will occur as this 

Incorrect, as you note below.


> > might be an async transfer (and therefore the stack-allocated message 
> > may be freed at some point when it's yet used!)
> > So your model implies concealed double message allocation/copying, 
> > doesn't it?
> > And if I'm wrong, can you please explain me this?
> 
> Oh, now looks like I understood what is meant. If a function uses 
> stack-allocated messages, it should ensure that it will not exit until 
> the message is processed (shouldn't it be documented somewhere?).

It is documented, but I'll make sure it comes up in a few more of the
places this confusion might arise.  It's a fairly basic rule for
C programming:  don't use pointers after they become invalid by
means of freeing back to the heap, or invalidating a stack frame.


> But  
> this solves the problem only partially since this technique fits only 
> the synchronous transfers.

Synchronous transfers can easily use stack allocation for
the descriptions, yes.

Not that they need to ... the ads7846 driver allocates its
spi_message and spi_transfer objects on the heap both for
synchronous operations (temperature and voltage sensing) and
for asynch ones (touchscreen tracking from timer and irq).


> Functions targeting async transfers will anyway have to kmalloc the 
> memory for message structure which makes your approach not really more 
> lightweight then ours. 

If you measure the number of error/fault cases when you ask how
lightweight an API is, it's clearly lighter weight to allow for
example one kzalloc -(with spi_message and its N spi_transfer
descriptors, plus possibly other driver state) rather than to
require many of them.  Just one fault path to write -- and debug.

My usual rule of thumb is that 1/3 of code (by lines) must handle
fault cases.   So APIs requiring more fault handling require more
driver code ... not lightweight!  That was a help, when fitting
into a tight size budget.  (As appropriate to what's more or less
a shift register API, needing to run quickly in uCLinux and such.) 

Plus, letting the driver do the kzalloc means there's no new API.
No-new-API is another way to promote lighter weight systems.  ;)


That said ... I know some people _do_ like krefcounted APIs that
do that kind of stuff.  Strongly.  Greg's been silent here other
than pointing out that your request alloc was too fat to inline.
Mine is trivially inlined, but not refcounted.  Likely there's a
happy middle ground, maybe

  mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
  mesg = spi_message_get(mesg);
  spi_message_put();

Or whatever.  Just add a kref to spi_message, and patch against
the current mm set (to the core and both drivers, but not
necessarily the spi_bitbang stuff).

- Dave

Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool]

Vitaly Wool wrote:

> David Brownell wrote:
>
>>> Yeah thus we don't have an ability to allocate SPI messages on stack 
>>> as you do, that's what votes for your approach. Yours is thus a bit 
>>> faster, though I suspect that this method is a possible *danger* for 
>>> really high-speed devices with data bursts on the SPI bus like WiFi 
>>> adapters: stack's gonna suffer from large amounts of data allocated.
>>>   
>>
>>
>> No, you're still thinking about a purely synchronous programming model;
>> which we had agreed ages ago was not required.
>>  
>>
> Ah yes. But wait... I've got an important question here.
> For instance, let's take your MTD driver. You're allocating a message 
> structure on stack and passing it then down to spi->master->transfer 
> function.
> The benefit you're talking about is that you don't have to use 
> heavyweight memory allocation. But... the transfer is basically async 
> so spi->master->transfer will need to copy your message structure to 
> its own-allocated structure so some memory copying will occur as this 
> might be an async transfer (and therefore the stack-allocated message 
> may be freed at some point when it's yet used!)
> So your model implies concealed double message allocation/copying, 
> doesn't it?
> And if I'm wrong, can you please explain me this?

Oh, now looks like I understood what is meant. If a function uses 
stack-allocated messages, it should ensure that it will not exit until 
the message is processed (shouldn't it be documented somewhere?). But 
this solves the problem only partially since this technique fits only 
the synchronous transfers.
Functions targeting async transfers will anyway have to kmalloc the 
memory for message structure which makes your approach not really more 
lightweight then ours. It's mainly async transfers that need high 
throughput; and the drivers based on your core will have to kmalloc 
memory for messages in that case.

Vitaly