I am not sure that what you describe as a race between simultaneous ioctls really possible, esp. after the r281828.

I think that clarification of the mutex/io_lock semantic is required as the first step.

I think it is possible for calls into the smbus interface to get interleaved right now. If they come from smb(4) ioctls, they could get interleaved because that driver does not hold its own sx like iic(4) does (and did even before r281828). But those calls could also be made by another driver directly calling something like smbus_bwrite(), without going through smb(4).

It might be better to instead require callers to use smbus_request_bus(), which smb(4) already does. But for that to work, smbus_request_bus() needs some cleanup similar to what was done in iicbus_request_bus(). It also might be a pain to fixup the drivers that directly call smbus_* but don't use request_bus().

I think that clarification of the mutex/io_lock semantic is required as the first step.

call_lock protects a series of calls to the controller (set_controller/slave/wait_status) that form an ioctl so they won't interleave, while io_lock is used to coordinate the interrupt handler with other I/O to the controller, especially wait_status.

In D2744#53712, @grembo wrote:

I think that clarification of the mutex/io_lock semantic is required as the first step.

call_lock protects a series of calls to the controller (set_controller/slave/wait_status) that form an ioctl so they won't interleave, while io_lock is used to coordinate the interrupt handler with other I/O to the controller, especially wait_status.

That far I can understand myself from reading the code. What I cannot get from the code alone, without using some kind of the mind-reading low-tech solution, is what is the intent for the bounds of transactions manipulating the hardware state. E.g., why is it fine to allow interrupt handler to intervene while waiting for i/o completion: what hw state should be re-acquired after the sleep ? Does interrupt handler need exclusion with the top half of the driver, if needed, why ? ... etc

In D2744#53837, @kib wrote:

In D2744#53712, @grembo wrote:

I think that clarification of the mutex/io_lock semantic is required as the first step.

call_lock protects a series of calls to the controller (set_controller/slave/wait_status) that form an ioctl so they won't interleave, while io_lock is used to coordinate the interrupt handler with other I/O to the controller, especially wait_status.

That far I can understand myself from reading the code. What I cannot get from the code alone, without using some kind of mind-reading low-tech solution, is what is the intent for the bounds of transactions manipulating the hardware state. E.g., why is it fine to allow the interrupt handler to intervene while waiting for i/o completion:

Why do you think this wouldn't be the case?

what hw state should be re-acquired after the sleep ? Does the interrupt handler need exclusion with the top half of the driver, if needed, why ? ... etc

Which sleep are you referring to (mtx_sleep in wait_status maybe)? Why would a hardware state need to be reacquired? And where does the "top half of the driver" end?

In D2744#53895, @grembo wrote:

In D2744#53837, @kib wrote:

In D2744#53712, @grembo wrote:

I think that clarification of the mutex/io_lock semantic is required as the first step.

call_lock protects a series of calls to the controller (set_controller/slave/wait_status) that form an ioctl so they won't interleave, while io_lock is used to coordinate the interrupt handler with other I/O to the controller, especially wait_status.

That far I can understand myself from reading the code. What I cannot get from the code alone, without using some kind of mind-reading low-tech solution, is what is the intent for the bounds of transactions manipulating the hardware state. E.g., why is it fine to allow the interrupt handler to intervene while waiting for i/o completion:

Why do you think this wouldn't be the case?

what hw state should be re-acquired after the sleep ? Does the interrupt handler need exclusion with the top half of the driver, if needed, why ? ... etc

Which sleep are you referring to (mtx_sleep in wait_status maybe)? Why would a hardware state need to be reacquired? And where does the "top half of the driver" end?

Or do you mean mtx_sleep in set_controller (which allows the interrupt handler to read, which might not be good)? Stating the obvious, the interrupt handler can only read data if no ig4iic_smb_* call is in progress or during mtx_sleep in wait_status/set_controller. Exclusion is required in other places to make rpos/rnext predictable (e.g. whenever data_read is called and in smb_transaction).

I don't feel like I really understand what you mean, so more details/hints would be useful.

In D2744#53943, @grembo wrote:

In D2744#53895, @grembo wrote:

In D2744#53837, @kib wrote:

In D2744#53712, @grembo wrote:

I think that clarification of the mutex/io_lock semantic is required as the first step.

call_lock protects a series of calls to the controller (set_controller/slave/wait_status) that form an ioctl so they won't interleave, while io_lock is used to coordinate the interrupt handler with other I/O to the controller, especially wait_status.

That far I can understand myself from reading the code. What I cannot get from the code alone, without using some kind of mind-reading low-tech solution, is what is the intent for the bounds of transactions manipulating the hardware state. E.g., why is it fine to allow the interrupt handler to intervene while waiting for i/o completion:

Why do you think this wouldn't be the case?

what hw state should be re-acquired after the sleep ? Does the interrupt handler need exclusion with the top half of the driver, if needed, why ? ... etc

Which sleep are you referring to (mtx_sleep in wait_status maybe)? Why would a hardware state need to be reacquired? And where does the "top half of the driver" end?

Or do you mean mtx_sleep in set_controller (which allows the interrupt handler to read, which might not be good)? Stating the obvious, the interrupt handler can only read data if no ig4iic_smb_* call is in progress or during mtx_sleep in wait_status/set_controller. Exclusion is required in other places to make rpos/rnext predictable (e.g. whenever data_read is called and in smb_transaction).

Actually, this is a good explanation of what I asked for. If you add a comment somewhere stating that the interrupt handler can do that, and only interrupt handler can run when mtx is dropped (due to sx), it would provide the context which was missed when I tried to understand the locking mode.

I don't feel like I really understand what you mean, so more details/hints would be useful.