On 05/31/2015 07:56 PM, Adam Baker wrote:
On 29/05/15 22:05, Antti Palosaari wrote:
We need own I2C locking because of tuner I2C adapter/repeater.
Firmware command is executed using I2C send + reply message. Default
I2C adapter locking protects only single I2C operation, not whole
send + reply sequence as needed. Due to that, it was possible tuner
I2C message interrupts firmware command sequence.
Reported-by: Adam Baker <linux@xxxxxxxxxxxxxxxx>
Signed-off-by: Antti Palosaari <crope@xxxxxx>
---
Reviewed-by: Adam Baker <linux@xxxxxxxxxxxxxxxx>
Having looked over this I can't see any remaining deadlocks or failures
to provide adequate locking.
Without a detailed device datasheet (the public datasheet is only the
short version) it is impossible to say
1) If accessing the I2C gate in between a read and write cycle would
actually cause a problem, if it doesn't then a simpler solution would be
possible but it seems reasonable to assume that it does.
Hey, you could very very easily make test and see what happens. Just add
dummy I2C gate open / close request to si2168_cmd_execute_unlocked() and
see what happens.
I suspect it will fail as I cannot see how firmware could even report
status of multiple operations happening same time. Firmware status is
always first byte of read operation, there is no flag to say which
operation status is for. OK, currently I2C gate status is not checked at
all, but still.
i2c_master_send("download firmware packet");
i2c_master_send("open I2C gate");
i2c_master_recv("read status"); <-- which operation status it will be?
Many fw operations are pretty fast and reply is always "firmware ready".
But there is some operations that will take up to 70ms.
2) How effective the retry mechanism is. The current behaviour that
retries the read cycle without retrying the preceding write means that
it isn't possible to pass the read and write messages as multiple
messages to i2c_transfer and let that handle the locking for us.
Passing multiple messages to i2c_transfer() is different that multiple
i2c_master_send() / i2c_master_recv(). Look what means "repeated start
condition" from some I2C documentation to understand the difference.
Do you know how likely it is for this issue to be triggered without the
signal stats patch applied? My suspicion is that it could only happen if
user space deliberately tried changing parameters on the tuner and
frontend at the same time from different threads and hence the fix isn't
worth pushing to stable.
Those callbacks are driven be DVB core which serializes all operations.
So it could not happen (without that statistics polling kernel thread).
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
