Re: spidev: fix hang when transfer_one_message fails

From: Daniel Santos
Date: Mon Jan 27 2014 - 18:15:17 EST


On 01/24/2014 07:01 AM, Mark Brown wrote:
Please don't write enormous walls of text, it really doesn't make it
easy to read your messages or encourage doing so. Use blank lines
between paragraphs (including within lists) and try to either split or
condense your ideas so that what you're trying to say comes over more
clearly.

Indeed, that was pretty ugly. :) Sorry about that.


The only reason I'm using transfer_one_message() at all is because
transfer() is being deprecated. My driver (currently out-of-tree)
supports both but will prefer transfer() as long as it hasn't been
removed or become broken ( which I'm managing via a #if
LINUX_VERSION_CODE >= KERNEL_VERSION(4,99,99) check: https://github.com/daniel-santos/mcp2210-linux/blob/master/mcp2210-spi.c#L143).
No, don't do that - it's not sensible. If there's something you need
work upstream to get it implemented or understand how to use the
framework better. Don't code around the frameworks, talk to people
instead.

I suppose that at the time I worked on this, I had some time pressures and I did plan to come back to it and discuss this with linux-spi to figure out how to better manage this or if I should just simply use the spi's queue and leave it be. I've faced a lot of challenges thus far because:

a.) It's my first device driver, and

b.) I must dynamically create/destroy gpio_chips, irq_chips, spi_masters and their children since this is a USB "bridge" device that can be added & removed at any point in time.

I originally thought that it was a first in its class, but I've since discovered another out-of-tree project that is doing very similar things, USB to i2c/spi (https://github.com/groeck/diolan)


of other spi drivers in the mainline, I can see that at least two of
them do this as well: spi-pxa2xx and spi-bfin-v3. So perhaps we need
a non-deprecated mechanism to do our own queuing and avoid the
No, that's not what those drivers are doing (nor the others doing
similar things) - they have done some optimisation on the code that
pushes messages to hardware so they don't defer to task context when
they don't have to. There's very little hardware specific about what
they're doing, it's all about how we work with the scheduler to minimise
the idle time for the hardware. A major goal of factoring out the loops
that traverse the messages from the drivers is to allow us to move that
code out of the drivers and into the framework where it belongs.

Oh, that's cool! :) Thanks for the clarification.

overhead of the spi core providing a thread & queue which we'll just
ignore. Then, the core can take care of setting status and
finalizing when calls to transfer() fail (since there should be no
ambiguity about this here), but leave that up to the driver when
calling transfer_one_message()?
When the core refactoring is finished popping up into the thread will be
mostly optional. Things like PIO, clock reprogramming and delays will
need to be pushed up into task context as do some of the DMA operations
and the completions - you don't want to be doing anything slow in
interrupt context.

I suppose I need to read up more on the refactoring work happening in this subsystem. Yes, we definitely don't want to spend much time in interrupt context and my driver currently spends a lot of time there (at least to me). My strategy has been that when I get an spi message from transfer(), I create and submit an mcp2210-specific command for that message. If no command is currently in-process, I also submit 64-byte interrupt URB for that command prior to returning (the mcp2210 has a tiny buffer). I suppose I've been trying to follow the "first make it correct, then make it fast" credo.

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