Menu
▾
▴
spi-devel-general
|
From: Greg KH <gr...@kr...> - 2005-12-15 16:51:16
|
On Thu, Dec 15, 2005 at 09:47:42AM +0300, Vitaly Wool wrote: > David Brownell wrote: > > >No, "stupid drivers will suffer"; nothing new. Just observe > >how the ads7846 touchscreen driver does small async transfers. > > > > > One cannot allocate memory in interrupt context, so the way to go is > allocating it on stack, thus the buffer is not DMA-safe. > Making it DMA-safe in thread that does the very message processing is a > good way of overcoming this. > Using preallocated buffer is not a good way, since it may well be > already used by another interrupt or not yet processed by the worker > thread (or tasklet, or whatever). Yes it is a good way. That's the way USB currently works in the kernel, and it works just fine. It keeps the rules simple and everyone knows what needs to be done. thanks, greg k-h |
|
From: Vitaly W. <vw...@ru...> - 2005-12-15 22:35:10
|
Greg KH wrote: >On Thu, Dec 15, 2005 at 09:47:42AM +0300, Vitaly Wool wrote: > > >>David Brownell wrote: >> >> >> >>>No, "stupid drivers will suffer"; nothing new. Just observe >>>how the ads7846 touchscreen driver does small async transfers. >>> >>> >>> >>> >>One cannot allocate memory in interrupt context, so the way to go is >>allocating it on stack, thus the buffer is not DMA-safe. >>Making it DMA-safe in thread that does the very message processing is a >>good way of overcoming this. >>Using preallocated buffer is not a good way, since it may well be >>already used by another interrupt or not yet processed by the worker >>thread (or tasklet, or whatever). >> >> > >Yes it is a good way. That's the way USB currently works in the kernel, >and it works just fine. It keeps the rules simple and everyone knows >what needs to be done. > > Looking at my usbnet stuff, I can't share that opinion :-/ Are you really ready to lower the performance and quality of service just for approach uniformity? And, can you please point me out the examples of devices behind USB bus that need to write registers from an interrupt context? Vitaly |
|
From: Greg KH <gr...@kr...> - 2005-12-15 23:03:48
|
On Fri, Dec 16, 2005 at 01:23:32AM +0300, Vitaly Wool wrote: > Greg KH wrote: > > >On Thu, Dec 15, 2005 at 09:47:42AM +0300, Vitaly Wool wrote: > > > > > >>David Brownell wrote: > >> > >> > >> > >>>No, "stupid drivers will suffer"; nothing new. Just observe > >>>how the ads7846 touchscreen driver does small async transfers. > >>> > >>> > >>> > >>> > >>One cannot allocate memory in interrupt context, so the way to go is > >>allocating it on stack, thus the buffer is not DMA-safe. > >>Making it DMA-safe in thread that does the very message processing is a > >>good way of overcoming this. > >>Using preallocated buffer is not a good way, since it may well be > >>already used by another interrupt or not yet processed by the worker > >>thread (or tasklet, or whatever). > >> > >> > > > >Yes it is a good way. That's the way USB currently works in the kernel, > >and it works just fine. It keeps the rules simple and everyone knows > >what needs to be done. > > > > > Looking at my usbnet stuff, I can't share that opinion :-/ > Are you really ready to lower the performance and quality of service > just for approach uniformity? What performance issues? As an example, USB has this rule, and we can saturate a 480Mbit line with a _userspace_ driver (loads of memcopy calles involved there.) > And, can you please point me out the examples of devices behind USB bus > that need to write registers from an interrupt context? usb to serial drivers need to allocate buffers for their write functions as they can be called in irq context from a tty line dicipline, which causes a USB packet to be dynamically created and sent to the USB core. I also think the USB network and ATM drivers have these requirements too, just search for GFP_ATOMIC in the drivers/usb/ directory to find these instances. Anyway, this is my last response about this issue. Please consider it over. thanks, greg k-h |
|
From: David B. <da...@pa...> - 2005-12-15 20:06:41
|
On Wednesday 14 December 2005 10:47 pm, Vitaly Wool wrote: > One cannot allocate memory in interrupt context, so the way to go is > allocating it on stack, thus the buffer is not DMA-safe. kmalloc(..., GFP_ATOMIC) is the way to allocate memory in irq context. It's done that way throughout the kernel. > Making it DMA-safe in thread that does the very message processing is a > good way of overcoming this. The rest of Linux appears to work fine without needing such mechanisms... I really fail to see why you think SPI needs that. USB isn't the only counterexample, but it's particularly relevant since both USB and SPI use asynchronous message passing over serial links ... and USB has a rather complete driver stack over it. (None of the USB based WLAN drivers need those static buffers you worry about, by the way...) > Using preallocated buffer is not a good way, since it may well be > already used by another interrupt or not yet processed by the worker > thread (or tasklet, or whatever). We would call those "driver bugs" and expect patches. :) > The way it's done in this ads7846 driver is not quite acceptable. > Losing the transfer if the previous one is still processed is *not* the > way to go in some cases. That's not the way it works though. And you seem to have made up a "losing the transfer" failure mode out of whole cloth; I have no idea where that came from. > One can not predict how many transfers are > gonna be dropped due to "previous trransfer is being processed" problem, > it depends on the system load. And though it's not a problem for > touchscreen, it *will* be a problem if it were MMC, for instance. Huh, well I've already seen one nice "mmc/sd over SPI" driver, using a slightly earlier version of the framework than is found in mm3. It's being used for root filesystems. So demonstrably there is no problem for MMC/SD, either. - Dave |
|
From: Vitaly W. <vw...@ru...> - 2005-12-15 22:18:12
|
David Brownell wrote: >On Wednesday 14 December 2005 10:47 pm, Vitaly Wool wrote: > > > >>One cannot allocate memory in interrupt context, so the way to go is >>allocating it on stack, thus the buffer is not DMA-safe. >> >> > >kmalloc(..., GFP_ATOMIC) is the way to allocate memory in irq context. >It's done that way throughout the kernel. > > It's not applicable within the RT-related changes. kmalloc anyway takes mutexes, so allocationg it in interrupt context is buggy. *Legacy* kernel code does that but why produce a new code with that? > > > >>Making it DMA-safe in thread that does the very message processing is a >>good way of overcoming this. >> >> > >The rest of Linux appears to work fine without needing such mechanisms... > > The rest of Linux still has a lot of bugs. Noone I guess is ready to argue that. >I really fail to see why you think SPI needs that. USB isn't the only >counterexample, but it's particularly relevant since both USB and SPI >use asynchronous message passing over serial links ... and USB has a >rather complete driver stack over it. (None of the USB based WLAN >drivers need those static buffers you worry about, by the way...) > > I haven't heard of USB device registers needing to be written in IRQ context. I'm not that well familiar with USB, so if you give such an example, that'd be fine. >>Using preallocated buffer is not a good way, since it may well be >>already used by another interrupt or not yet processed by the worker >>thread (or tasklet, or whatever). >> >> > >We would call those "driver bugs" and expect patches. :) > > > Well, I know one such patch ;) >>One can not predict how many transfers are >>gonna be dropped due to "previous trransfer is being processed" problem, >>it depends on the system load. And though it's not a problem for >>touchscreen, it *will* be a problem if it were MMC, for instance. >> >> > >Huh, well I've already seen one nice "mmc/sd over SPI" driver, using >a slightly earlier version of the framework than is found in mm3. >It's being used for root filesystems. > >So demonstrably there is no problem for MMC/SD, either. > > No problem... Has the driver been tested in stress conditions? If not, then I guess you can't say this for sure :) Vitaly |
|
From: Greg KH <gr...@kr...> - 2005-12-15 22:34:09
|
On Fri, Dec 16, 2005 at 01:17:56AM +0300, Vitaly Wool wrote: > David Brownell wrote: > > >On Wednesday 14 December 2005 10:47 pm, Vitaly Wool wrote: > > > > > > > >>One cannot allocate memory in interrupt context, so the way to go is > >>allocating it on stack, thus the buffer is not DMA-safe. > >> > >> > > > >kmalloc(..., GFP_ATOMIC) is the way to allocate memory in irq context. > >It's done that way throughout the kernel. > > > > > It's not applicable within the RT-related changes. kmalloc anyway takes > mutexes, so allocationg it in interrupt context is buggy. What RT-related changes cause this? > *Legacy* kernel code does that but why produce a new code with that? In this terminoligy, you are calling 2.6.15-rc5 "legacy". Which is not true. > >>Making it DMA-safe in thread that does the very message processing is a > >>good way of overcoming this. > >> > >> > > > >The rest of Linux appears to work fine without needing such mechanisms... > > > > > The rest of Linux still has a lot of bugs. Noone I guess is ready to > argue that. Huh? Please point out these bugs in the mainline tree and we will be glad to fix them. > >I really fail to see why you think SPI needs that. USB isn't the only > >counterexample, but it's particularly relevant since both USB and SPI > >use asynchronous message passing over serial links ... and USB has a > >rather complete driver stack over it. (None of the USB based WLAN > >drivers need those static buffers you worry about, by the way...) > > > > > I haven't heard of USB device registers needing to be written in IRQ > context. I'm not that well familiar with USB, so if you give such an > example, that'd be fine. The USB host controller drivers routienly allocate memory in irq context as they are being asked to submit a new "packet" from a driver which was called in irq context. Lots of USB drivers also allocate buffers in irq context too. So, please, drop this line of argument, it will not go any further. greg k-h |
|
From: Andy I. <ad...@he...> - 2005-12-16 03:35:08
|
On Thu, Dec 15, 2005 at 02:33:22PM -0800, Greg KH wrote: > On Fri, Dec 16, 2005 at 01:17:56AM +0300, Vitaly Wool wrote: > > I haven't heard of USB device registers needing to be written in IRQ > > context. I'm not that well familiar with USB, so if you give such an > > example, that'd be fine. > > The USB host controller drivers routienly allocate memory in irq context > as they are being asked to submit a new "packet" from a driver which was > called in irq context. Lots of USB drivers also allocate buffers in irq > context too. > > So, please, drop this line of argument, it will not go any further. I know almost nothing about SPI, so I could be completely wrong here, but I think I might have an inkling about the problem Vitaly is trying to solve. Note that I haven't actually had to make a design like this work, so I'm not intimately familiar with the issues, but I have seen designs that I believe would suffer from the following problem. Suppose you have a chip (temp sensor for example) controlled via serial bus (I2C is what I'm familiar with, from what I know this scenario would apply to SPI too) that *also* drives an interrupt into the CPU. You want to be able to share the interrupt, so you need to be able to turn off the interrupt from IRQ context (when the driver decides that it is going to handle the interrupt). But the only way to turn off the interrupt on the peripheral chip is to send a message via the serial bus. Now if the IRQ gets entered while the serial bus is busy servicing another device, the device driver author is in trouble - she can't return from the irq handler until she's acked the IRQ and the device is no longer asserting the interrupt, but she can't ack the IRQ without sending a command on the serial bus, which she can't do because the bus is currently in use higher up the call stack. Now one could argue that this design is broken (requiring a slow serial bus access to ack an irq means that you end up with very high-latency interrupt handlers) but it's my impression that such designs are not unheard of in the embedded world. You end up having to leave the interrupt masked on return from the irq_handler routine, ack it at a higher level at some later point, and then re-enable it. -andy |
|
From: Greg KH <gr...@kr...> - 2005-12-16 06:51:47
|
On Thu, Dec 15, 2005 at 07:34:59PM -0800, Andy Isaacson wrote: > Now one could argue that this design is broken (requiring a slow serial > bus access to ack an irq means that you end up with very high-latency > interrupt handlers) but it's my impression that such designs are not > unheard of in the embedded world. Then you just atomically allocate a buffer like all of the current kernel drivers do :) Come on people, this really isn't an issue... thanks, greg k-h |
|
From: Greg KH <gr...@kr...> - 2005-12-14 19:17:50
|
On Wed, Dec 14, 2005 at 08:53:54PM +0300, Vitaly Wool wrote: > Greg KH wrote: > > >What is the speed of your SPI bus? > > > >And what are your preformance requirements? > > > > > The maximum frequency for the SPI bus is 26 MHz, WLAN driver is to work > at true 10 Mbit/sec. Then you should be fine with the copying data and memset stuff, based on the workload the rest of the kernel does for other busses which have this same requirement of DMAable buffers. And I'm sure David will be glad to have you point out any places in his code where it accidentally takes data off of the stack instead of allocating it. thanks, greg k-h |
|
From: Vitaly W. <vw...@ru...> - 2005-12-14 19:32:54
|
Greg KH wrote: >On Wed, Dec 14, 2005 at 08:53:54PM +0300, Vitaly Wool wrote: > > >>Greg KH wrote: >> >> >> >>>What is the speed of your SPI bus? >>> >>>And what are your preformance requirements? >>> >>> >>> >>> >>The maximum frequency for the SPI bus is 26 MHz, WLAN driver is to work >>at true 10 Mbit/sec. >> >> > >Then you should be fine with the copying data and memset stuff, based on >the workload the rest of the kernel does for other busses which have >this same requirement of DMAable buffers. > >And I'm sure David will be glad to have you point out any places in his >code where it accidentally takes data off of the stack instead of >allocating it. > > Hmm, I recall I've already posted some pieces of code with that stuff. Vitaly |
|
From: David B. <da...@pa...> - 2005-12-14 19:27:12
|
On Wednesday 14 December 2005 9:53 am, Vitaly Wool wrote: > Greg KH wrote: > > >What is the speed of your SPI bus? > > > >And what are your preformance requirements? > > > > > The maximum frequency for the SPI bus is 26 MHz, WLAN driver is to work > at true 10 Mbit/sec. Some SPI flash chips are rated at 60 MHz ... there's no "official" standard placing such limits on SPI. The various IEEE 802.15.4 (ZigBee) transceivers seem to take up to 10 MHz clocks, FWIW. USB controllers, 12 MHz (surprise! they are of course full speed, 12 MHz). Sensors with A-to-D converters are often lower rate; only 100K samples per second, say, at maybe 12 bits each; more like 2 MHz. - Dave |
|
From: Vitaly W. <vw...@ru...> - 2005-12-14 19:30:21
|
David Brownell wrote: >On Wednesday 14 December 2005 9:53 am, Vitaly Wool wrote: > > >>Greg KH wrote: >> >> >> >>>What is the speed of your SPI bus? >>> >>>And what are your preformance requirements? >>> >>> >>> >>> >>The maximum frequency for the SPI bus is 26 MHz, WLAN driver is to work >>at true 10 Mbit/sec. >> >> > >Some SPI flash chips are rated at 60 MHz ... there's no "official" >standard placing such limits on SPI. > > David, can you please stop treating me as an idiot? Of course I meant the specific SPI bus we're working with which is quite evident from the context. Thanks. Vitaly |
|
From: dmitry p. <dpe...@gm...> - 2005-12-15 10:09:41
|
On Wed, 2005-12-14 at 20:53 +0300, Vitaly Wool wrote: > Greg KH wrote: > > >What is the speed of your SPI bus? > > > >And what are your preformance requirements? > > > > > The maximum frequency for the SPI bus is 26 MHz, WLAN driver is to work > at true 10 Mbit/sec. My two cents: the faster is better; SPI bus itself can work on 52MHz, and AFAIK WLAN developers limit the speed due to their firmware reqiorements. > > Vitaly > > P. S. I'm speaking not about this particular case during most part of > this conversation. Sound cards behind the SPI bus will suffer a lot more > since it's their path to use wXrY functions (lotsa small transfers) > rather than WLAN's. > > > ------------------------------------------------------- > This SF.net email is sponsored by: Splunk Inc. Do you grep through log files > for problems? Stop! Download the new AJAX search engine that makes > searching your log files as easy as surfing the web. DOWNLOAD SPLUNK! > http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click > _______________________________________________ > spi-devel-general mailing list > spi...@li... > https://lists.sourceforge.net/lists/listinfo/spi-devel-general > > |
|
From: David B. <da...@pa...> - 2005-12-14 17:41:13
|
On Wednesday 14 December 2005 5:50 am, Vitaly Wool wrote: > >>It's way better to just insist that all I/O buffers (in all > >>generic APIs) be DMA-safe. AFAICT that's a pretty standard > >>rule everywhere in Linux. > > > >I agree. > > Well, why then David doesn't insist on that in his own code? > His synchronous transfer functions You seem to be referring to one non-generic function that's documented as OK to pass DMA-unsafe buffers to. There are several other synchronous transfer calls that don't make such guarantees. > are allocating transfer buffers on > stack which is not DMA-safe. I think the very first version did that, but nothing since has taken that shortcut. (Several months now.) It uses a buffer that's allocated on the heap. > Then he starts messing with allocate-or-use-preallocated stuff etc. etc. > Why isn't he just kmalloc'ing/kfree'ing buffers each time these > functions are called So that the typical case, with little SPI contention, doesn't hit the heap? That's sure what I thought ... though I can't speak for what other people may think I thought. You were the one that wanted to optimize the atypical case to remove a blocking path! > (as he proposes for upper layer drivers to do)? No I didn't. I actually said that the upper layer drivers should just not use DMA-unsafe areas for I/O buffers in the first place. Places like stacks or static data. Doing that means there's never a need for a new kmalloc buffer, unless maybe you're marshaling things into a scratch buffer. > The thing is that only controller driver is aware whether DMA is needed > or not, so it's controller driver that should work it out. Given the policy that all code avoids DMA-unsafe areas for I/O buffers, the issue has already been worked out globally. - Dave |
|
From: Vitaly W. <vw...@ru...> - 2005-12-14 17:57:08
|
David Brownell wrote:
>On Wednesday 14 December 2005 5:50 am, Vitaly Wool wrote:
>
>
>>>>It's way better to just insist that all I/O buffers (in all
>>>>generic APIs) be DMA-safe. AFAICT that's a pretty standard
>>>>rule everywhere in Linux.
>>>>
>>>>
>>>I agree.
>>>
>>>
>>Well, why then David doesn't insist on that in his own code?
>>His synchronous transfer functions
>>
>>are allocating transfer buffers on
>>stack which is not DMA-safe.
>>
>>
>
>I think the very first version did that, but nothing since
>has taken that shortcut. (Several months now.) It uses
>a buffer that's allocated on the heap.
>
>
You're wrong.
Isn't it a part of your code:
static inline ssize_t spi_w8r8(struct spi_device *spi, u8 cmd)
{
ssize_t status;
u8 result;
status = spi_write_then_read(spi, &cmd, 1, &result, 1);
/* return negative errno or unsigned value */
return (status < 0) ? status : result;
}
You're allocating u8 var on stack, then allocate a 1-byte-long buffer
and copy the data instead of letting the controller driver decide
whether this allocation/copy is necessary or not. And in 99% cases it's
not gonna be necessary as any controller driver w/o brain damage will
transfer this as PIO.
So the overhead for sending/receiving 1 byte will be _very big_
(trylock, kmalloc, memcpy). See now what I'm talking about?
>
>
>
>>Then he starts messing with allocate-or-use-preallocated stuff etc. etc.
>>Why isn't he just kmalloc'ing/kfree'ing buffers each time these
>>functions are called
>>
>>
>
>So that the typical case, with little SPI contention, doesn't
>hit the heap? That's sure what I thought ... though I can't speak
>for what other people may think I thought. You were the one that
>wanted to optimize the atypical case to remove a blocking path!
>
>
I meant kmalloc'ing/kfree'ing buffers is spi_w8r8/spi_w8r16/etc.
Vitaly
|
|
From: David B. <da...@pa...> - 2005-12-14 19:59:45
|
> static inline ssize_t spi_w8r8(struct spi_device *spi, u8 cmd)
> {
> ssize_t status;
> u8 result;
>
> status = spi_write_then_read(spi, &cmd, 1, &result, 1);
>
> /* return negative errno or unsigned value */
> return (status < 0) ? status : result;
> }
>
> You're allocating u8 var on stack, then allocate a 1-byte-long buffer
> and copy the data instead of letting the controller driver decide
> whether this allocation/copy is necessary or not.
Yeah, like that matters in the face of the overhead to queue
the message, get to the head of the SPI transfer queue, go
through that queue, then finally wake up the task that was
synchronously blocking in write_then_read(). Oh, and since
that's inlined, GCC may be re-using existing state...
If folk want an "it looks simple" convenient/friendly API,
there is always a price to pay. In this case, that cost is
dwarfed by the mere fact that they're using a synchronous
model to access shared resources (the SPI controller).
> >>Then he starts messing with allocate-or-use-preallocated stuff etc. etc.
> >>Why isn't he just kmalloc'ing/kfree'ing buffers each time these
> >>functions are called
> >
> >So that the typical case, with little SPI contention, doesn't
> >hit the heap? That's sure what I thought ... though I can't speak
> >for what other people may think I thought. You were the one that
> >wanted to optimize the atypical case to remove a blocking path!
>
> I meant kmalloc'ing/kfree'ing buffers is spi_w8r8/spi_w8r16/etc.
As I said: so the _typical_ case doesn't hit the heap. There are
inherent overheads for such RPC-style calls. But there's also no
point in gratuitously increasing them.
- Dave
|
|
From: Vitaly W. <vw...@ru...> - 2005-12-14 20:12:06
|
David Brownell wrote:
>>static inline ssize_t spi_w8r8(struct spi_device *spi, u8 cmd)
>>{
>> ssize_t status;
>> u8 result;
>>
>> status = spi_write_then_read(spi, &cmd, 1, &result, 1);
>>
>> /* return negative errno or unsigned value */
>> return (status < 0) ? status : result;
>>}
>>
>>You're allocating u8 var on stack, then allocate a 1-byte-long buffer
>>and copy the data instead of letting the controller driver decide
>>whether this allocation/copy is necessary or not.
>>
>>
>
>Yeah, like that matters in the face of the overhead to queue
>the message, get to the head of the SPI transfer queue, go
>through that queue, then finally wake up the task that was
>synchronously blocking in write_then_read(). Oh, and since
>that's inlined, GCC may be re-using existing state...
>
>If folk want an "it looks simple" convenient/friendly API,
>there is always a price to pay. In this case, that cost is
>dwarfed by the mere fact that they're using a synchronous
>model to access shared resources (the SPI controller).
>
>
It's just words; the patch I'm proposing cuts this price to nothing but
you're just being too stubborn to accept that. :(
>
>
>
>>>>Then he starts messing with allocate-or-use-preallocated stuff etc. etc.
>>>>Why isn't he just kmalloc'ing/kfree'ing buffers each time these
>>>>functions are called
>>>>
>>>>
>>>So that the typical case, with little SPI contention, doesn't
>>>hit the heap? That's sure what I thought ... though I can't speak
>>>for what other people may think I thought. You were the one that
>>>wanted to optimize the atypical case to remove a blocking path!
>>>
>>>
>>
>>I meant kmalloc'ing/kfree'ing buffers is spi_w8r8/spi_w8r16/etc.
>>
>>
>
>As I said: so the _typical_ case doesn't hit the heap. There are
>inherent overheads for such RPC-style calls. But there's also no
>point in gratuitously increasing
>
Sorry, increasing what?
Okay, lemme summarize: I've spent a day working out the minimal changes
I'd like to have added to your core and two days trying to convince
those changes are necessary. You just don't want to listen to me. So the
conclusion is that the convergence process has been deadlocked, and the
only way out for us is -- we'll continue to work on our core, as
there're things we'd like to have in SPI core and there're other people
using our core.
Vitaly
|
|
From: Vitaly W. <vw...@ru...> - 2005-12-14 19:48:22
|
Stephen Street wrote:
>On Tue, 2005-12-13 at 17:06 +0300, Vitaly Wool wrote:
>
>
>>Greetings,
>>
>>This thingie hasn't been thoroughly tested yet, but it's lightweight
>>and easy to understand so I don't think solving the problems that
>>may arise will take long. Though I haven't actually done that yet,
>>I'm sure that Stephen's PXA SSP driver will become easier to understand
>>and less in footprint and will work faster when it's rewritten using
>>this library. (Yes, I do expect performance improvement here as the
>>current implementation schedules multiple tasklets, so
>>scheduling penalty is high.)
>>
>>
>
>Is this really true? Is tasklet scheduling "harder" than kernal thread
>scheduling? A close look at my PXA SSP SPI implementation will reveal
>that my design is nearly lock-less and callable from any execution
>context (i.e. interrupt context).
>
>
It's harder in your case because the tasklet is created each time it's
scheduled again, as far as I see it in your impleemntation.
Each SPI controller thread is created only once so it's more lightweight
than what you do.
>>+ * spi_queue - (internal) queue the message to be processed asynchronously
>>+ * @spi: SPI device to perform transfer to/from
>>+ * @msg: message to be sent
>>+ * Description:
>>+ * This function queues the message to SPI controller's queue.
>>+ */
>>+static int spi_queue(struct spi_device *spi, struct spi_message *msg)
>>+{
>>+ struct threaded_async_data *td = spi->master->context;
>>+ int rc = 0;
>>+
>>+ if (!td) {
>>+ rc = -EINVAL;
>>+ goto out;
>>+ }
>>+
>>+ msg->spi = spi;
>>+ down(&td->lock);
>>+ list_add_tail(&msg->queue, &td->msgs);
>>+ dev_dbg(spi->dev.parent, "message has been queued\n");
>>+ up(&td->lock);
>>+ wake_up_interruptible(&td->wq);
>>+
>>+out:
>>+ return rc;
>>+}
>>+
>>
>>
>
>This can not be invoke this from "interrupt context" which is a
>requirement for my SPI devices (CS8415A, CS8405A, CS4341).
>
>
>
>
Okay, not a major issue though. Change mutexes to
spin_lock_irq/spin_unlock_irq and it's callable from an interrupt
context, right?
Vitaly
|
|
From: Stephen S. <st...@st...> - 2005-12-14 21:20:13
|
On Wed, 2005-12-14 at 22:41 +0300, Vitaly Wool wrote: > >Is this really true? Is tasklet scheduling "harder" than kernal thread > >scheduling? A close look at my PXA SSP SPI implementation will reveal > >that my design is nearly lock-less and callable from any execution > >context (i.e. interrupt context). > > > > > It's harder in your case because the tasklet is created each time it's > scheduled again, as far as I see it in your impleemntation. > Each SPI controller thread is created only once so it's more lightweight > than what you do. > I'm not sure what you mean by "create". The tasklet structures are created and initialized once in the driver probe function. I'm not an expert but I looked into the implementation (softirq.c) of tasklets and found the following design: 1) Tasklets are run by a softirq. 2) A softirq is really a kernel thread allocated on a per cpu basis. 3) A "scheduled" tasklet is simply a member of a link list maintained by the softirq thread. My driver implementation has the following features: 1) Uses only one kernel thread for all SPI controllers. 2) Reuses existing performance tuned kernel infrastructure (i.e. tasklets) 3) Implements a low latency locking scheme for dispatching SPI transfers via tasklet's serial scheduling guarantees. IMHO, from a system load perspective, my approach is lighter and simpler than adding a dedicated kernel thread for each SPI controller. Stephen |
|
From: Vitaly W. <vw...@ru...> - 2005-12-16 08:38:33
|
Greg KH wrote: >On Fri, Dec 16, 2005 at 01:23:32AM +0300, Vitaly Wool wrote: > > >>Greg KH wrote: >> >> >> >>>On Thu, Dec 15, 2005 at 09:47:42AM +0300, Vitaly Wool wrote: >>> >>> >>> >>> >>>>David Brownell wrote: >>>> >>>> >>>> >>>> >>>> >>>>>No, "stupid drivers will suffer"; nothing new. Just observe >>>>>how the ads7846 touchscreen driver does small async transfers. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>One cannot allocate memory in interrupt context, so the way to go is >>>>allocating it on stack, thus the buffer is not DMA-safe. >>>>Making it DMA-safe in thread that does the very message processing is a >>>>good way of overcoming this. >>>>Using preallocated buffer is not a good way, since it may well be >>>>already used by another interrupt or not yet processed by the worker >>>>thread (or tasklet, or whatever). >>>> >>>> >>>> >>>> >>>Yes it is a good way. That's the way USB currently works in the kernel, >>>and it works just fine. It keeps the rules simple and everyone knows >>>what needs to be done. >>> >>> >>> >>> >>Looking at my usbnet stuff, I can't share that opinion :-/ >>Are you really ready to lower the performance and quality of service >>just for approach uniformity? >> >> > >What performance issues? As an example, USB has this rule, and we can >saturate a 480Mbit line with a _userspace_ driver (loads of memcopy >calles involved there.) > > What CPU is used there? I guess it's not 144 MHz ARM ;-) > > >>And, can you please point me out the examples of devices behind USB bus >>that need to write registers from an interrupt context? >> >> > >usb to serial drivers need to allocate buffers for their write functions >as they can be called in irq context from a tty line dicipline, which >causes a USB packet to be dynamically created and sent to the USB core. >I also think the USB network and ATM drivers have these requirements >too, just search for GFP_ATOMIC in the drivers/usb/ directory to find >these instances. > > Oh BTW... I'm experiencing constant problems with root filesystem over NFS over usbnet on my target I'm getting "server not responding, still trying" error whenever the system (208-MHz ARM926 board) is under heavy load. I think it may well be related to the thing we discuss. Vitaly |
|
From: Greg KH <gr...@kr...> - 2005-12-16 17:44:55
|
On Fri, Dec 16, 2005 at 11:37:46AM +0300, Vitaly Wool wrote: > >What performance issues? As an example, USB has this rule, and we can > >saturate a 480Mbit line with a _userspace_ driver (loads of memcopy > >calles involved there.) > > > What CPU is used there? I guess it's not 144 MHz ARM ;-) I don't think so either, but don't remember the exact cpu (but it was an embedded system...) > >>And, can you please point me out the examples of devices behind USB bus > >>that need to write registers from an interrupt context? > >> > > > >usb to serial drivers need to allocate buffers for their write functions > >as they can be called in irq context from a tty line dicipline, which > >causes a USB packet to be dynamically created and sent to the USB core. > >I also think the USB network and ATM drivers have these requirements > >too, just search for GFP_ATOMIC in the drivers/usb/ directory to find > >these instances. > > > Oh BTW... I'm experiencing constant problems with root filesystem over > NFS over usbnet on my target > I'm getting "server not responding, still trying" error whenever the > system (208-MHz ARM926 board) is under heavy load. > I think it may well be related to the thing we discuss. I really doubt it. See David's other message to you about this. And if you have problems, please report them on the linux-usb-devel mailing list. We have never had bug reports due to issues involving the DMA requirements (with the exception of people finding drivers that were trying to send data off of the stack...) thanks, greg k-h |
|
From: David B. <da...@pa...> - 2005-12-16 18:32:17
|
> > Oh BTW... I'm experiencing constant problems with root filesystem over > > NFS over usbnet on my target As you clarified off-line ... it's "pegasus", not "usbnet", which is giving you problems. That's entirely different code; although "pegasus" could be modified to run on top of the core that "usbnet" provides. The problems I've seen with "pegasus" have more to do with wierd chip behaviors (and weak fault recovery logic) than with anything DMA-related. - Dave |
|
From: David B. <da...@pa...> - 2005-12-18 19:16:42
Attachments:
spi_bitbang.c
|
OK, I made some time to look at that. As I've mentioned already,
the "spi_bitbang" code needed to morph in this direction, and
that was in fact the patch I was hoping you'd be sending. So I
finished that up instead, and have sent it separately ... but
I've also attached the C file to this response.
> +struct threaded_async_data {
> + atomic_t exiting;
> + struct spi_master *master;
> + struct task_struct *thread;
> + wait_queue_head_t wq;
I just kept the workqueue, named after its device.
It makes the code look a lot simpler!
> + struct list_head msgs;
> + spinlock_t lock;
> + int (*xfer) (struct spi_master *, struct spi_message *);
> +};
> +
> +/**
> + * spi_start_async - start the thread
> + * @master: SPI controller structure which the thread is related to
> + * @return: abstract pointer to the thread context
> + */
> +int spi_start_async (struct spi_master *master, int (*xfer)(struct spi_master *,
> struct spi_message *))
I did this differently, but liked your start/stop names. :)
So:
int spi_bitbang_start(struct spi_bitbang *bitbang);
int spi_bitbang_stop(struct spi_bitbang *bitbang);
to start and stop processing the queue associated with the bitbanged
spi_master. Callbacks just get stored in the structure; simpler,
and the return value is just a normal zero-or-negative-errno.
> @@ -152,6 +152,7 @@ static inline void spi_unregister_driver
> * device's SPI controller; protocol code may call this.
> * @transfer: adds a message to the controller's transfer queue.
> * @cleanup: frees controller-specific state
> + * @context: controller-specific data
> *
> * Each SPI master controller can communicate with one or more spi_device
> * children. These make a small bus, sharing MOSI, MISO and SCK signals
I've not seen a need for that yet; the class_get_devdata() is doing
that already. And it's now wrapped up as spi_master_get_devdata().
- Dave
|
|
From: dmitry p. <dpe...@gm...> - 2005-12-19 15:50:26
|
On Sun, 2005-12-18 at 10:59 -0800, David Brownell wrote: > chipselect = !t->cs_change; > if (chipselect); > continue; > > bitbang->chipselect(spi, 0); > > /* REVISIT do we want the udelay here instead? > */ > msleep(1); Ohhh. Have you intentionally put the semicolon after "if (chipselect)" ? |
|
From: David B. <da...@pa...> - 2005-12-20 07:23:21
|
On Monday 19 December 2005 7:40 am, dmitry pervushin wrote: > On Sun, 2005-12-18 at 10:59 -0800, David Brownell wrote: > > chipselect = !t->cs_change; > > if (chipselect); > > continue; > > > > bitbang->chipselect(spi, 0); > > > > /* REVISIT do we want the udelay here instead? > > */ > > msleep(1); > Ohhh. Have you intentionally put the semicolon after "if > (chipselect)" ? Nope. Good catch ... haven't had a chance to try that yet with that driver; not all drivers actually need to drop chipselect like that. Any other comments about that approach to the "let someone else manage the queue"? This one's more clearly "implementation utilities" than what you had seemed to be talking about before, but I think it does address a point where I think we were agreeing: it should be easy for people to whip together a controller driver by providing just some lower level I/O calls. - Dave |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X