Rcu improve

[kelvin-cao]

The RCU is used in the driver to serialize the hardware related operations and the module removal, this PR tries to improve RCU use in the driver in the following fields.

• Avoid sleep/blocking in the RCU read critical section.
• Avoid nesting of RCU read critical section. Nesting is allowed by RCU, however it makes it more clear to avoid nesting in the driver.
• Add necessary RCU protection for the serialization of module removal.
• Remove unnecessary RCU protection.

[lsgunth]

The RCU stuff seems fine, but the last patch to attempt to synchronize does not seem like a good idea.

[lsgunth]

This seems like a bad idea. For one, you can't solely rely on READ_ONCE for synchronization unless the other side is also careful about writing (WRITE_ONCE), but this is really not a good idea to implement your own atomic number.

For another thing, what if the device is hot removed or failed?

I think it'd be much better to tell the hardware to stop processing commands, then go through the jobs in the queue and forcibly cancel them. If the hardware is gone then telling it to stop will do nothing, but it won't be handling any other jobs so it will still be safe to cancel them.

[kelvin-cao]

I think synchronize_cmd here is to wait an stopped command execution to complete with timeout. Before this, the command execution is told to stop (no more command execution) by rcu_assign_pointer(swdma_dev->pdev, NULL);, so the only thing to do for this function is to wait the current (last) command to complete or timeout.

Since the cmd->status is updated by hardware, so I think READ_ONCE is necessary to tell the compiler to not to do any guess.

The command execution in the device cannot be stopped, but there's a timeout for the execution, after which no DMA will be performed by the hardware. So I think waiting for a timeout is good enough for a device hot removal, in which case the cmd->status will never be updated.

[lsgunth]

cmd->status is also updated CPU... and it could be subject to tearing and other scary issues.

This really seems like it's very buggy. Getting the synchronization absolutely right is hard to even reason about. I would not do it that way.

If the hardware cannot be told to stop, it's missing an incredibly important feature.

[kelvin-cao]

The command execution is actually performed by the firmware, like MRPC, we can just wait for completion with timeout, there's no way to stop it. I guess it's not too bad with a timeout value not too big, since even with a stop we will still need to wait for it for a while.

Besides RCU for synchronization with removal, the command execution is also using a mutex to ensure there's no outstanding command execution. In the new patch, I use it in removal to ensure that the command execution completed (or timed out), therefore no more DMA push to the host.

[lsgunth]

You don't need to stop it within a command, you just need to stop it from going on to the next queue item and a way to know when it has stopped. The MRPC didn't have a queue so this wasn't an issue. This is an important feature all DMA engines I've ever used has.

There's usually also rules about the shared memory. ie. it's either owned by hardware or software and one cannot touch it if they don't own it. And actually I'm not seeing any of that typical stuff in your code.

Timeouts are going to be troublesome because they have to be large enough to allow for a queue of large jobs the user might submit, so the delay you'll need will be significant.

[kelvin-cao]

I guess I didn't make it clear... The fabric DMA command is not queued, unlike the DMA traffic. There's only one fabric DMA command execution at a time, very similar implementation as MRPC. And the timeout is only for one command.

The host side shared memory for command execution is initialized by driver and filled by hardware.

Below code snippet in execute_cmd initialize the command status to idle and set the response data to 0xff.

swdma_dev->cmd->status = CMD_STATUS_IDLE;
memset(swdma_dev->cmd->data, 0xFF, CMD_OUTPUT_SIZE);

Then copy the command input to device and trigger the command.

memcpy_toio(&swdma_dev->mmio_fabric_cmd->input, input, input_size);
iowrite32(cmd, &swdma_dev->mmio_fabric_cmd->command);

When the command completed, the hardware (firmware) push the response to memory pointed by cmd->data, then update cmd->status to error or done.

For the the synchronization of command execution and removal, I think the new commit 285b565 has made it quite simple and clear.

[lsgunth]

Oh, that's not on the hot path then.... Well why not just use a lock and delay shutdown until you can take the lock? Don't invent your own locking primitive especially when it's not performance critical.

[kelvin-cao]

Yes agreed, I think that's what I'm doing in the hard pushed new patch 285b565 below...