>-----Original Message-----
>> @@ -623,6 +627,14 @@ static int ifx_spi_write(struct tty_struct *tty,
>> const unsigned char *buf, int srdy_value;
>>
>> dev_dbg(&ifx_dev->spi_dev->dev, "%s called", __func__);
>> + if (test_bit(IFX_SPI_STATE_SUSPEND, &ifx_dev->flags)) {
>> + /*
>> + * FIXME:
>> + * some modems have been observed to die (requiring
>> reset)
>> + * if MRDY/SRDY handshake occurs w/o clocking data
>> + */
>> + return -EIO;
>> + }
>
>So what stops the suspend occurring between here after your test and
>the mrdy change - seems you are only narrowing the window ?
[Gorby, Russ] the issue here is the relative timing of D3 suspend between the
SPI protocol and controller driver. You may be right that it doesn't completely
close the hole. It's here to mitigate the problem. If I could count on top-down
suspend and bottom up resume this wouldn't be needed at all but I'm not aware
of any guarantees like that.
>> + * ifx_spi_retry_timer_fn - Timer function for ifx_spi_complete()
>> + * @data: Device pointer passed by ifx_spi_complete().
>> + *
>> + * The delay timer has expired so reschedule the tasklet
>> + */
>> +static void ifx_spi_retry_timer_fn(unsigned long data)
>> +{
>> + struct ifx_spi_device *ifx_dev = (struct ifx_spi_device
>> *)data; +
>> + tasklet_schedule(&ifx_dev->io_work_tasklet);
>> +}
>
>What is this change about - it isn't explained anywhere
[Gorby, Russ] It was explained in the preamble email (a little) but I should
have put it in this patch file as well. My apologies - I'm still learning some
of the process details.
The issue here is that if the controller driver has gone into
PM-runtime-suspend at the time I/O arrives for it from spi_async(), even though
the controller driver can autonomously resume, it takes some time and returns
EBUSY until it has resumed. Our protocol driver calls spi_async from a tasklet,
and we discovered during PM testing that if the tasklet is immediately
rescheduled we get a large number of iterations before the resume completes.
So, we added a delay timer in this case so the CPU is used more appropriately.
>
>> +
>> +/**
>> * ifx_spi_complete - SPI transfer completed
>> * @ctx: our SPI device
>> *
>> @@ -1030,6 +1055,9 @@ complete_exit:
>> }
>>
>> clear_bit(IFX_SPI_STATE_IO_IN_PROGRESS, &(ifx_dev->flags));
>> + clear_bit(IFX_SPI_STATE_IO_RETRY, &(ifx_dev->flags));
>> + del_timer(&ifx_dev->spi_retry_timer);
>
>And if the timer handler is running in parallel on another CPU isn't
>this going to cause confusion ?
[Gorby, Russ] That shouldn't be possible - unless you see something I do not.
There can be only one SPI msg being processed at a time, and one tasklet
running. The tasklet is the only thing that calls spi_async. Spi_complete runs
only if spi_async succeeded or returned something other than EBUSY and the
retry timer runs only if spi_async returned EBUSY.
>
>
>> + if (!test_and_set_bit(IFX_SPI_STATE_IO_RETRY,
>> &ifx_dev->flags))
>> + setup_timer(&ifx_dev->spi_retry_timer,
>> ifx_spi_retry_timer_fn,
>> + (unsigned long)ifx_dev);
>> + expires = jiffies + IFX_RETRY_TIMEOUT;
>> + mod_timer(&ifx_dev->spi_retry_timer, expires);
>
>What stops this racing the timer delete - it seems we can set the flag,
>clear it again, delete the timer then re-add it and end up in a right
>mess ?
>
[Gorby, Russ] same as above, the timer can't run while either the tasklet or
spi_complete is executing
>This is then attached to a pile of SPI driver changes which are also
>undocumented and relate to a driver which isn't actually going to get
>used in the normal course of things as the other SPI driver will claim
>its ID *and* supports the other ports too.
>
[Gorby, Russ] I gather you're referring to the intel_mid_ssp_... driver. We
have not determined which way to go on that.
There is missing functionality in each that would need to be filled-out.
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
