Hello,
>> @@ -521,19 +523,26 @@ static int xiic_bus_busy(struct xiic_i2c *i2c)
>> static int xiic_busy(struct xiic_i2c *i2c)
>> {
>> int tries = 3;
>> - int err;
>> + int err = 0;
>>
>> if (i2c->tx_msg)
>> return -EBUSY;
>>
>> - /* for instance if previous transfer was terminated due to TX error
>> - * it might be that the bus is on it's way to become available
>> - * give it at most 3 ms to wake
>> + /* In single master mode bus can only be busy, when in use by this
>> + * driver. If the register indicates bus being busy for some reason we
>> + * should ignore it, since bus will never be released and i2c will be
>> + * stuck forever.
>> */
>
>the other thing i was thinking how will multithreading .
>Should we have a lock here.
>
>> - err = xiic_bus_busy(i2c);
>> - while (err && tries--) {
>> - msleep(1);
>> + if (i2c->multimaster) {
>> + /* for instance if previous transfer was terminated due to TX
>> + * error it might be that the bus is on it's way to become
>> + * available give it at most 3 ms to wake
>> + */
>> err = xiic_bus_busy(i2c);
>> + while (err && tries--) {
>> + msleep(1);
>> + err = xiic_bus_busy(i2c);
>> + }
>> }
>>
>> return err;
Which resource specifically are you worried about needing locking here?
I don't think this patch introduces any new need for locking. Only new parameter, which wasn't accessed already is i2c->multimaster, which is a constant that is never changed after driver is loaded.
If i2c->multimaster, needed locking i2c->tx_msg would have needed it already before, since it is a parameter in the same struct and can actually get changed by some other thread.
In this section the only variables written to are local to the function. Shared variables are only read from, which seems pretty safe to me if considering this function alone.
However, now that you mention it multiple threads could be checking i2c->tx_msg at the same time inside this function or waiting for xiic_bus_busy(i2c) to not be busy anymore.
Since in "static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)" i2c->tx_msg is written with data before any locking, multiple threads could exit "xiic_busy(struct xiic_i2c *i2c)" and write their stuff to i2c->tx_msg, since buffer being empty was checked before anyone had a chance to write to it. If this happens, some data to be transmitted could be lost when i2c->tx_msg gets overwritten multiple times before data gets transmitted. This issue did already exist before, but it looks like it should be fixed to me.
Fixing would need locking here, but the possible msleep(1) -calls inside xiic_busy seem like an issue, so some more changes needed:
// lock here
err = xiic_busy(i2c);
if (err)
// unlock here
goto out;
i2c->tx_msg = msgs;
i2c->nmsgs = num;
// unlock here
>> + i2c->multimaster =
>> + of_property_read_bool(pdev->dev.of_node, "multi-master");
>> +
>Current will default to mustimaster is 0.
>May be the default should be 1 if not specified.
The multi-master -binding is documented here as boolean and encodes a Boolean by either existing or not existing in device tree.
It is also used in other drivers so I couldn't do much about it missing meaning False.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c.txt
I originally had a custom device tree entry where the default was for multi-master to be enabled before I noticed the pre-existing binding.
Maybe if the multi-master binding was changed from Boolean to for example a string property (multi-master = "ON" / multi-master = "OFF"), code could still just check the existence with "of_property_read_bool()" first, where property missing means "OFF" and property existing means "ON"(like before) if there is no text associated. Xiic driver would then only disable multimaster, if device tree explicitly contains multi-master = "OFF".
This should be able to maintain driver backwards compatibility with old device trees, but requires binding documentation change and all drivers should likely be updated to also accept the new style of multi-master property to be consistent. This is also not as clean as the old Boolean property in my opinion.
Thank you for comments,
Jaakko
